Cleanup generic_osync_inode?

Cleanup generic_osync_inode?

[Jan Kara]

  Hi,

  I was looking at generic_osync_inode() and found out it's kind of
inconsistent with what we have in fsync() path and does not work in all
cases e.g. on ext3 / ext4. The problem is that filesystem never actually
gets to know that it should sync all metadata needed to reach the data -
generic_osync_inode() only does sync_mapping_buffers() but e.g. ext3 / ext4
don't track metadata buffers there. Then it does write_inode_now() which
would actually flush the journal, but it does so only in case inode is
I_DIRTY_DATASYNC... So there are cases where we sync the data but leave
metadata uncommitted.
  What I'd imagine is that generic_osync_inode() would be just like
fdatasync call, only we'd have to add a possibility to avoid fdatawrite /
fdatawait as some callers submit / wait for data themselves. That would
nicely unify those syncing paths.
  The only small problem is with an interface since ->fsync() callback
takes preferably struct file * and at least struct dentry *, while
generic_osync_inode takes just inode. Most of the callers actually have
a struct file * pointer but sync_page_range[_nolock]() do not, so that
would have to be solved somehow.
  Any opinions / ideas?

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: Cleanup generic_osync_inode?

[Christoph Hellwig]

On Fri, Aug 14, 2009 at 11:48:31PM +0200, Jan Kara wrote:
>   Hi,
> 
>   I was looking at generic_osync_inode() and found out it's kind of
> inconsistent with what we have in fsync() path and does not work in all
> cases e.g. on ext3 / ext4. The problem is that filesystem never actually
> gets to know that it should sync all metadata needed to reach the data -
> generic_osync_inode() only does sync_mapping_buffers() but e.g. ext3 / ext4
> don't track metadata buffers there. Then it does write_inode_now() which
> would actually flush the journal, but it does so only in case inode is
> I_DIRTY_DATASYNC... So there are cases where we sync the data but leave
> metadata uncommitted.

Yeah, it's not very useful.  That's the reason why XFS doesn't rely on
it but rather does a manual log force for the metadata.

>   What I'd imagine is that generic_osync_inode() would be just like
> fdatasync call, only we'd have to add a possibility to avoid fdatawrite /
> fdatawait as some callers submit / wait for data themselves. That would
> nicely unify those syncing paths.
>   The only small problem is with an interface since ->fsync() callback
> takes preferably struct file * and at least struct dentry *, while
> generic_osync_inode takes just inode. Most of the callers actually have
> a struct file * pointer but sync_page_range[_nolock]() do not, so that
> would have to be solved somehow.

Note that the current way ->fsync works is also rather problematic,
for one thing all filesystems that touch metadata on data I/O completion
(XFS, btrfs, ext4 and probably more) really want to first write out
the data _and_ wait for it, and only then sync the inode.  Right now it
every filesystems has to do that itself and do it under i_mutex or
drop/reacquire it which is quite stupid.  The second issue is that
was pass a file which is required for some filesystems (e.g. NFS) but
which may be NULL when coming from NFSD.  I'll need to look into fixing
NFSD and always pass a file here.

No back to the original generic_osync_inode / O_SYNC handling problem:
All callers of sync_page_range actually do have a file pointer, and
all of them are called from generic or filesystem specific write code,
so passing the file pointer to it is no problem at all.
sync_page_range_nolock just has a single caller in fat which does not
have a file pointer.  The right thing here is IMHO:

 (1) open-code sync_page_range_nolock in fat and just get rid of it as a
     generic helper
 (2) replace sync_page_range with a generic_write_sync or similar does
     the range writeouts + a call to ->fsync.  Bonus points for also
     moving the O_SYNC / IS_SYNC checks into that helper.

Re: Cleanup generic_osync_inode?

[Christoph Hellwig]

And actually merge our new helper generic_osync_inode.  We should
never had those two different helpers doing pretty much the same.