Re: [PATCH] xfs: force the log after remapping a synchronous-writes file

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

On Fri, Sep 04, 2020 at 07:24:51AM -0400, Brian Foster wrote:
> On Thu, Sep 03, 2020 at 08:11:00PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Commit 5833112df7e9 tried to make it so that a remap operation would
> > force the log out to disk if the filesystem is mounted with mandatory
> > synchronous writes.  Unfortunately, that commit failed to handle the
> > case where the inode or the file descriptor require mandatory
> > synchronous writes.
> > 
> > Refactor the check into into a helper that will look for all three
> > conditions, and now we can treat reflink just like any other synchronous
> > write.
> > 
> > Fixes: 5833112df7e9 ("xfs: reflink should force the log out if mounted with wsync")
> 
> More of a process thought than an issue with this particular patch, but
> I feel like the Fixes tag thing gets more watered down as we attempt to
> apply it to more patches. Is it really necessary here? If so, what's the
> reasoning? I thought it was more of a "this previous patch has a bug,"
> but that link seems a bit tenuous here given the original patch refers
> specifically to wsync. Sure, a stable kernel probably wants both
> patches, but is that really the primary purpose of "Fixes?"

<shrug> I'm not sure -- both patches fix design flaws in the xfs reflink
implementation, and the second patch requires the first one.  The docs
merely say that you should add a Fixes tag "if your patch fixes a bug in
a specific commit" without elaborating if we ought to create a chain of
Fixes tags when adding patches that slowly broaden the scope of a code
change.

FWIW these days I add Fixes tags in the hopes of tricking the LTS bot
(or Eric Sandeen) into backporting things for me. ;)

> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Thanks for the review though. :)

--D

> >  fs/xfs/xfs_file.c |   17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index c31cd3be9fb2..ee43f137830c 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -1008,6 +1008,21 @@ xfs_file_fadvise(
> >  	return ret;
> >  }
> >  
> > +/* Does this file, inode, or mount want synchronous writes? */
> > +static inline bool xfs_file_sync_writes(struct file *filp)
> > +{
> > +	struct xfs_inode	*ip = XFS_I(file_inode(filp));
> > +
> > +	if (ip->i_mount->m_flags & XFS_MOUNT_WSYNC)
> > +		return true;
> > +	if (filp->f_flags & (__O_SYNC | O_DSYNC))
> > +		return true;
> > +	if (IS_SYNC(file_inode(filp)))
> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> >  STATIC loff_t
> >  xfs_file_remap_range(
> >  	struct file		*file_in,
> > @@ -1065,7 +1080,7 @@ xfs_file_remap_range(
> >  	if (ret)
> >  		goto out_unlock;
> >  
> > -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> > +	if (xfs_file_sync_writes(file_in) || xfs_file_sync_writes(file_out))
> >  		xfs_log_force_inode(dest);
> >  out_unlock:
> >  	xfs_iunlock2_io_mmap(src, dest);
> > 
>