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

[Christoph Hellwig <hch@infradead.org>]

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:

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);
 	error = xfs_trans_commit(tp);
 	if (error)
 		goto out_error;