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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
>
> Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
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);
> >
>
