diff mbox series

[5.10,CANDIDATE,09/11] xfs: only bother with sync_filesystem during readonly remount

Message ID 20220617100641.1653164-10-amir73il@gmail.com (mailing list archive)
State New, archived
Headers show
Series xfs stable candidate patches for 5.10.y (v5.15+) | expand

Commit Message

Amir Goldstein June 17, 2022, 10:06 a.m. UTC 
From: "Darrick J. Wong" <djwong@kernel.org>

commit b97cca3ba9098522e5a1c3388764ead42640c1a5 upstream.

In commit 02b9984d6408, we pushed a sync_filesystem() call from the VFS
into xfs_fs_remount.  The only time that we ever need to push dirty file
data or metadata to disk for a remount is if we're remounting the
filesystem read only, so this really could be moved to xfs_remount_ro.

Once we've moved the call site, actually check the return value from
sync_filesystem.

Fixes: 02b9984d6408 ("fs: push sync_filesystem() down to the file system's remount_fs()")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/xfs_super.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Darrick J. Wong June 22, 2022, 4:38 p.m. UTC | #1 
On Fri, Jun 17, 2022 at 01:06:39PM +0300, Amir Goldstein wrote:
> From: "Darrick J. Wong" <djwong@kernel.org>
> 
> commit b97cca3ba9098522e5a1c3388764ead42640c1a5 upstream.
> 
> In commit 02b9984d6408, we pushed a sync_filesystem() call from the VFS
> into xfs_fs_remount.  The only time that we ever need to push dirty file
> data or metadata to disk for a remount is if we're remounting the
> filesystem read only, so this really could be moved to xfs_remount_ro.
> 
> Once we've moved the call site, actually check the return value from
> sync_filesystem.
> 
> Fixes: 02b9984d6408 ("fs: push sync_filesystem() down to the file system's remount_fs()")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/xfs/xfs_super.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 6323974d6b3e..dd0439ae6732 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1716,6 +1716,11 @@ xfs_remount_ro(
>  	};
>  	int			error;
>  
> +	/* Flush all the dirty data to disk. */
> +	error = sync_filesystem(mp->m_super);

Looking at 5.10.124's fsync.c and xfs_super.c:

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/fs/sync.c?h=v5.10.124#n31
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/fs/xfs/xfs_super.c?h=v5.10.124#n755

I think this kernel needs the patch(es) that make __sync_filesystem return
the errors passed back by ->sync_fs, and I think also the patch that
makes xfs_fs_sync_fs return errors encountered by xfs_log_force, right?

--D

> +	if (error)
> +		return error;
> +
>  	/*
>  	 * Cancel background eofb scanning so it cannot race with the final
>  	 * log force+buftarg wait and deadlock the remount.
> @@ -1786,8 +1791,6 @@ xfs_fc_reconfigure(
>  	if (error)
>  		return error;
>  
> -	sync_filesystem(mp->m_super);
> -
>  	/* inode32 -> inode64 */
>  	if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) &&
>  	    !(new_mp->m_flags & XFS_MOUNT_SMALL_INUMS)) {
> -- 
> 2.25.1
>
Amir Goldstein June 22, 2022, 4:54 p.m. UTC | #2 
On Wed, Jun 22, 2022 at 7:38 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Fri, Jun 17, 2022 at 01:06:39PM +0300, Amir Goldstein wrote:
> > From: "Darrick J. Wong" <djwong@kernel.org>
> >
> > commit b97cca3ba9098522e5a1c3388764ead42640c1a5 upstream.
> >
> > In commit 02b9984d6408, we pushed a sync_filesystem() call from the VFS
> > into xfs_fs_remount.  The only time that we ever need to push dirty file
> > data or metadata to disk for a remount is if we're remounting the
> > filesystem read only, so this really could be moved to xfs_remount_ro.
> >
> > Once we've moved the call site, actually check the return value from
> > sync_filesystem.

This part is not really relevant for this backport, do you want me to
emphasise that?

> >
> > Fixes: 02b9984d6408 ("fs: push sync_filesystem() down to the file system's remount_fs()")
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > Reviewed-by: Dave Chinner <dchinner@redhat.com>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/xfs/xfs_super.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 6323974d6b3e..dd0439ae6732 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1716,6 +1716,11 @@ xfs_remount_ro(
> >       };
> >       int                     error;
> >
> > +     /* Flush all the dirty data to disk. */
> > +     error = sync_filesystem(mp->m_super);
>
> Looking at 5.10.124's fsync.c and xfs_super.c:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/fs/sync.c?h=v5.10.124#n31
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/fs/xfs/xfs_super.c?h=v5.10.124#n755
>
> I think this kernel needs the patch(es) that make __sync_filesystem return
> the errors passed back by ->sync_fs, and I think also the patch that
> makes xfs_fs_sync_fs return errors encountered by xfs_log_force, right?

It wasn't my intention to fix syncfs() does not return errors in 5.10.
It has always been that way and IIRC, the relevant patches did not
apply cleanly.

THIS patch however, fixes something else, not only the return of the error
to its caller, so I thought it was worth backporting.
If you think otherwise, I'll drop it.

Thanks,
Amir.
Darrick J. Wong June 22, 2022, 11:42 p.m. UTC | #3 
On Wed, Jun 22, 2022 at 07:54:33PM +0300, Amir Goldstein wrote:
> On Wed, Jun 22, 2022 at 7:38 PM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Fri, Jun 17, 2022 at 01:06:39PM +0300, Amir Goldstein wrote:
> > > From: "Darrick J. Wong" <djwong@kernel.org>
> > >
> > > commit b97cca3ba9098522e5a1c3388764ead42640c1a5 upstream.
> > >
> > > In commit 02b9984d6408, we pushed a sync_filesystem() call from the VFS
> > > into xfs_fs_remount.  The only time that we ever need to push dirty file
> > > data or metadata to disk for a remount is if we're remounting the
> > > filesystem read only, so this really could be moved to xfs_remount_ro.
> > >
> > > Once we've moved the call site, actually check the return value from
> > > sync_filesystem.
> 
> This part is not really relevant for this backport, do you want me to
> emphasise that?

Not relevant?  Making sync_fs return error codes to callers was the
entire reason for creating this series...

> > >
> > > Fixes: 02b9984d6408 ("fs: push sync_filesystem() down to the file system's remount_fs()")
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > Reviewed-by: Dave Chinner <dchinner@redhat.com>
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >  fs/xfs/xfs_super.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > index 6323974d6b3e..dd0439ae6732 100644
> > > --- a/fs/xfs/xfs_super.c
> > > +++ b/fs/xfs/xfs_super.c
> > > @@ -1716,6 +1716,11 @@ xfs_remount_ro(
> > >       };
> > >       int                     error;
> > >
> > > +     /* Flush all the dirty data to disk. */
> > > +     error = sync_filesystem(mp->m_super);
> >
> > Looking at 5.10.124's fsync.c and xfs_super.c:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/fs/sync.c?h=v5.10.124#n31
> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/fs/xfs/xfs_super.c?h=v5.10.124#n755
> >
> > I think this kernel needs the patch(es) that make __sync_filesystem return
> > the errors passed back by ->sync_fs, and I think also the patch that
> > makes xfs_fs_sync_fs return errors encountered by xfs_log_force, right?
> 
> It wasn't my intention to fix syncfs() does not return errors in 5.10.
> It has always been that way and IIRC, the relevant patches did not
> apply cleanly.

...because right now userspace can call syncfs() on a filesystem that
dies in the process, and the VFS eats the EIO and returns 0 to
userspace.  Yes, that's the historical behavior fo 5.10, but that's a
serious problem that needs addressing.  Eliding the sync_filesystem call
during a rw remount is not itself all that exciting.

> THIS patch however, fixes something else, not only the return of the error
> to its caller, so I thought it was worth backporting.

Assuming "something else" means "moving the sync_filesystem callsite" --
that was a secondary piece that I did to get the requisite RVB tag under
time pressure after 5.17-rc6 dropped.

> If you think otherwise, I'll drop it.

On the contrary, I think the ->sync_fs fixes *also* need backporting.
It should be as simple as patching __sync_filesystem:

static int __sync_filesystem(struct super_block *sb, int wait)
{
	if (wait)
		sync_inodes_sb(sb);
	else
		writeback_inodes_sb(sb, WB_REASON_SYNC);

	if (sb->s_op->sync_fs) {
		int ret = sb->s_op->sync_fs(sb, wait);
		if (ret)
			return ret;
	}
	return __sync_blockdev(sb->s_bdev, wait);
}

Granted, that can be a part of the next batch.  If you plan to pick up
the vfs sync_fs changes then I guess this one's ok for inclusion now.

--D

> 
> Thanks,
> Amir.
Amir Goldstein June 23, 2022, 6:38 a.m. UTC | #4 
On Thu, Jun 23, 2022 at 2:42 AM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Wed, Jun 22, 2022 at 07:54:33PM +0300, Amir Goldstein wrote:
> > On Wed, Jun 22, 2022 at 7:38 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > >
> > > On Fri, Jun 17, 2022 at 01:06:39PM +0300, Amir Goldstein wrote:
> > > > From: "Darrick J. Wong" <djwong@kernel.org>
> > > >
> > > > commit b97cca3ba9098522e5a1c3388764ead42640c1a5 upstream.
> > > >
> > > > In commit 02b9984d6408, we pushed a sync_filesystem() call from the VFS
> > > > into xfs_fs_remount.  The only time that we ever need to push dirty file
> > > > data or metadata to disk for a remount is if we're remounting the
> > > > filesystem read only, so this really could be moved to xfs_remount_ro.
> > > >
> > > > Once we've moved the call site, actually check the return value from
> > > > sync_filesystem.
> >
> > This part is not really relevant for this backport, do you want me to
> > emphasise that?
>
> Not relevant?  Making sync_fs return error codes to callers was the
> entire reason for creating this series...
>
> > > >
> > > > Fixes: 02b9984d6408 ("fs: push sync_filesystem() down to the file system's remount_fs()")
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > Reviewed-by: Dave Chinner <dchinner@redhat.com>
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > ---
> > > >  fs/xfs/xfs_super.c | 7 +++++--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > > index 6323974d6b3e..dd0439ae6732 100644
> > > > --- a/fs/xfs/xfs_super.c
> > > > +++ b/fs/xfs/xfs_super.c
> > > > @@ -1716,6 +1716,11 @@ xfs_remount_ro(
> > > >       };
> > > >       int                     error;
> > > >
> > > > +     /* Flush all the dirty data to disk. */
> > > > +     error = sync_filesystem(mp->m_super);
> > >
> > > Looking at 5.10.124's fsync.c and xfs_super.c:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/fs/sync.c?h=v5.10.124#n31
> > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/fs/xfs/xfs_super.c?h=v5.10.124#n755
> > >
> > > I think this kernel needs the patch(es) that make __sync_filesystem return
> > > the errors passed back by ->sync_fs, and I think also the patch that
> > > makes xfs_fs_sync_fs return errors encountered by xfs_log_force, right?
> >
> > It wasn't my intention to fix syncfs() does not return errors in 5.10.
> > It has always been that way and IIRC, the relevant patches did not
> > apply cleanly.
>
> ...because right now userspace can call syncfs() on a filesystem that
> dies in the process, and the VFS eats the EIO and returns 0 to
> userspace.  Yes, that's the historical behavior fo 5.10, but that's a
> serious problem that needs addressing.  Eliding the sync_filesystem call
> during a rw remount is not itself all that exciting.

Yes, this is a just cause.

>
> > THIS patch however, fixes something else, not only the return of the error
> > to its caller, so I thought it was worth backporting.
>
> Assuming "something else" means "moving the sync_filesystem callsite" --
> that was a secondary piece that I did to get the requisite RVB tag under
> time pressure after 5.17-rc6 dropped.

Right. looking back at my notes:
https://github.com/amir73il/b4/commit/fddd6d961c029441d2f0e9dd86d0f83b754d0c47

I didn't pick this patch at all because of the missing dependencies.
I must have picked it up later from Laeh's series, because
I thought it was harmless to apply the "something else".

>
> > If you think otherwise, I'll drop it.
>
> On the contrary, I think the ->sync_fs fixes *also* need backporting.
> It should be as simple as patching __sync_filesystem:
>
> static int __sync_filesystem(struct super_block *sb, int wait)
> {
>         if (wait)
>                 sync_inodes_sb(sb);
>         else
>                 writeback_inodes_sb(sb, WB_REASON_SYNC);
>
>         if (sb->s_op->sync_fs) {
>                 int ret = sb->s_op->sync_fs(sb, wait);
>                 if (ret)
>                         return ret;
>         }
>         return __sync_blockdev(sb->s_bdev, wait);
> }
>
> Granted, that can be a part of the next batch.  If you plan to pick up
> the vfs sync_fs changes then I guess this one's ok for inclusion now.

I picked up the vfs fixes, it was pretty simple as you say.
I even backported "fs: remove __sync_filesystem" for dependency
to make the backport more straightforward.

But that adds 3 more more patches to the series and needs to go
back to testing, so I will just drop patch 9 from this batch.

Thanks,
Amir.
diff mbox series

Patch

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 6323974d6b3e..dd0439ae6732 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1716,6 +1716,11 @@  xfs_remount_ro(
 	};
 	int			error;
 
+	/* Flush all the dirty data to disk. */
+	error = sync_filesystem(mp->m_super);
+	if (error)
+		return error;
+
 	/*
 	 * Cancel background eofb scanning so it cannot race with the final
 	 * log force+buftarg wait and deadlock the remount.
@@ -1786,8 +1791,6 @@  xfs_fc_reconfigure(
 	if (error)
 		return error;
 
-	sync_filesystem(mp->m_super);
-
 	/* inode32 -> inode64 */
 	if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) &&
 	    !(new_mp->m_flags & XFS_MOUNT_SMALL_INUMS)) {