Re: [PATCH] xfs: reflink should force the log out if mounted with wsync

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Apr 02, 2020 at 07:56:48AM -0700, Christoph Hellwig wrote:
> On Thu, Apr 02, 2020 at 07:53:44AM -0700, Darrick J. Wong wrote:
> > > > Looks reasonable.  That being said I really hate the way we handle
> > > > this - I've been wanting to rework the wsync/dirsync code to just mark
> > > > as transaction as dirsync or wsync and then let xfs_trans_commit handle
> > > > checking if the file system is mounted with the option to clean this
> > > > mess up.  Let me see if I could resurrect that quickly.
> > > 
> > > Resurrected and under testing now.  While forward porting your patch
> > > I noticed it could be much simpler even without the refactor by just
> > > using xfs_trans_set_sync.  The downside of that is that the log force
> > > is under the inode locks, but so are the log forces for all other wysnc
> > > induced log forces.  So I think you should just submit this in the
> > > simplified version matching the rest of the wsync users as a fix. If
> > > we want to optimize it later on that should be done as a separate patch
> > > and for all wsync/dirsync users.
> > 
> > Can you please send in a Reviewed-by so I can get this moving? :)
> 
> In case the above wasn't clear: I don't think this is the right way
> to go.  Just to fix the reflink vs wsync bug I think we want a
> one-liner like this:

Sorry, I thought "you should just submit this in the simplified version"
was referring to the first patch I sent, as opposed to the cleanups
you're testing.

> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index b0ce04ffd3cd..e2cc7b84ca6c 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -948,6 +948,7 @@ xfs_reflink_update_dest(
>  
>  	xfs_trans_log_inode(tp, dest, XFS_ILOG_CORE);
>  
> +	xfs_trans_set_sync(tp);

This isn't enough because this is only the last transaction in the
reflink sequence if we have to set the destination inode's size.  If
(say) we're reflinking a range inside EOF of two files that were already
sharing blocks, we still won't force the log out.

The other thing I thought of was simply invoking fsync after dropping
the iolock, but that seemed like more work than was strictly necessary
to land the reflink transactions on disk.

--D

>  	error = xfs_trans_commit(tp);
>  	if (error)
>  		goto out_error;