Re: nfs MAP_SHARED corruption fix

[Andrea Arcangeli]

On Wed, May 09, 2001 at 07:38:01PM -0300, Marcelo Tosatti wrote:
> 
> 
> On Thu, 10 May 2001, Andrea Arcangeli wrote:
> 
> > On Wed, May 09, 2001 at 07:02:16PM -0300, Marcelo Tosatti wrote:
> > > Why don't you clean I_DIRTY_PAGES ? 
> > 
> > we don't have visibilty on the inode_lock from there, we could make a
> > function in fs/inode.c or export the inode_lock to do that, but the flag
> > will be collected when the inode is released anyways, and it's only an
> > hint to make the common case fast (the common case is when nobody ever
> > did a MAP_SHARED on the inode). Other places msync/fsync doesn't even
> > check for such bit but they fdatasync/fdatawait unconditionally. 
> 
> Actually msync/fsync _can't_ rely on this bit because there is no
> guarantee that data is fully synced on disk even if the bit is cleared.
> (__sync_one (fs/inode.c) clears the bit _before_ starting the writeout,
> and thats it).

correct sorry, fsync/msync cannot check that bit of course.

> You have the same problem with your code, so I guess its better to just
> remove the I_DIRTY_PAGES check. 

The point you have to clarify before claming we should remove the check
is if the munmap flush needs to be synchronous or not. In general munmap
doesn't need to be synchronous. If you want to commit the writes an
explicit msync(MS_SYNC) or fsync on the file is required.  Otherwise the
updates will hit the platter in a rasonable amount of finite time
asynchronously. If somebody just intiated the fdatasync he will have to
finish before we can collect away the inode and in turn drop all its
cache, so those dirty pages cannot get lost in iput if somebody started
doing the flush under us either, and the guy doing the fdatasync under
us will have to wait synchronously for the stuff to be committed before
it can return.

If some page wasn't yet visible in the dirty_pages list by the time
__sync_one started, we'll find I_DIRTY_PAGES set. This is enforced by
the locking order (sync_one first clears the I_DIRTY_PAGES and then
it starts browsing the dirty_pages list while set_page_dirty first make the
page visible and then marks the inode dirty).

So the I_DIRTY_PAGES check guarantees that those dirty pages cannot be
lost in iput, that was the _only_ object of the patch and that is
certainly enough to fix the nfs fs data corruption reported.

Now if you claim that munmap needs to be synchronous for nfs that's a
completly different matter. I didn't even tried to make it synchronous.
It is possible it has to be synchronous, even write(2) (in theory ;) has
to behave like O_SYNC with nfs, but I'm not sure.

Another thing (completly unrelated to the above issues) that I noticed
while looking over this nfs code is that the __sync_one() for example
called by generic_file_write(O_SYNC) will recall fdatasync but no nfs_wb_all
is put before the fdatawait, and I'm not sure that the nfs_sync_page
called by the fdatawait is enough to rapidly flush the writepaged stuff
to the nfs server. nfs_sync_page apparently only cares about speculative
reads, not at all about committing writebacks. It would look much saner
to me if nfs_sync_page also does a nfs_wb_all() on the inode, so that
the ->sync_page callback gets the same semantics it has for the real
filesystems.

Comments?

Andrea