diff mbox series

xfs: force the log after remapping a synchronous-writes file

Message ID 20200904031100.GZ6096@magnolia (mailing list archive)
State Accepted
Headers show
Series xfs: force the log after remapping a synchronous-writes file | expand

Commit Message

Darrick J. Wong Sept. 4, 2020, 3:11 a.m. UTC 
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")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_file.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Brian Foster Sept. 4, 2020, 11:24 a.m. UTC | #1 
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);
>
Darrick J. Wong Sept. 4, 2020, 3:41 p.m. UTC | #2 
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);
> > 
>
Christoph Hellwig Sept. 8, 2020, 3:05 p.m. UTC | #3 
Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

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);