Message ID | 20200904031100.GZ6096@magnolia (mailing list archive) |
---|---|
State | Accepted |
Headers | show |
Series | xfs: force the log after remapping a synchronous-writes file | expand |
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?" > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- Reviewed-by: Brian Foster <bfoster@redhat.com> > 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); >
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); > > >
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
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);