[PATCH] xfs: return error from sync_filesystem during remount

[PATCH] xfs: return error from sync_filesystem during remount

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

In xfs_fs_reconfigure, check the return value from sync_filesystem and
fail the remount if there was an internal error.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_super.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 4c0dee78b2f8..5f3781879c63 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1831,7 +1831,9 @@ xfs_fs_reconfigure(
 	if (error)
 		return error;
 
-	sync_filesystem(mp->m_super);
+	error = sync_filesystem(mp->m_super);
+	if (error)
+		return error;
 
 	/* inode32 -> inode64 */
 	if (xfs_has_small_inums(mp) && !xfs_has_small_inums(new_mp)) {

Re: [PATCH] xfs: return error from sync_filesystem during remount

[Dave Chinner]

On Fri, Feb 04, 2022 at 06:56:52PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> In xfs_fs_reconfigure, check the return value from sync_filesystem and
> fail the remount if there was an internal error.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_super.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 4c0dee78b2f8..5f3781879c63 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1831,7 +1831,9 @@ xfs_fs_reconfigure(
>  	if (error)
>  		return error;
>  
> -	sync_filesystem(mp->m_super);
> +	error = sync_filesystem(mp->m_super);
> +	if (error)
> +		return error;
>  
>  	/* inode32 -> inode64 */
>  	if (xfs_has_small_inums(mp) && !xfs_has_small_inums(new_mp)) {

Ummm, so why do we even need to call sync_filesystem()
unconditionally here?  The only case where we have to actually write
back anything on a remount is the rw->ro case, otherwise we aren't
changing any state that requires data or metadata writeback.

I had a look at why sync_filesytem() was there, and it goes back to
this commit that moved it from the VFS remount code:

commit 02b9984d640873b7b3809e63f81a0d7e13496886
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Thu Mar 13 10:14:33 2014 -0400

    fs: push sync_filesystem() down to the file system's remount_fs()
    
    Previously, the no-op "mount -o mount /dev/xxx" operation when the
    file system is already mounted read-write causes an implied,
    unconditional syncfs().  This seems pretty stupid, and it's certainly
    documented or guaraunteed to do this, nor is it particularly useful,
    except in the case where the file system was mounted rw and is getting
    remounted read-only.
    
    However, it's possible that there might be some file systems that are
    actually depending on this behavior.  In most file systems, it's
    probably fine to only call sync_filesystem() when transitioning from
    read-write to read-only, and there are some file systems where this is
    not needed at all (for example, for a pseudo-filesystem or something
    like romfs).

And later on __ext4_remount got modified to only call
sync_filesystem() on rw->ro transition. We do not depend on
sync_filesystem() for anything here except in the rw->ro remount
case, so why don't we just move the sync_filesystem() call to
xfs_remount_ro()? We already have an error path there for failing to
clean the filesystem, and avoids the possibility of other random
mount option changes failing because some because there is some
random pending data writeback error....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: return error from sync_filesystem during remount

[Darrick J. Wong]

On Mon, Feb 07, 2022 at 09:18:25AM +1100, Dave Chinner wrote:
> On Fri, Feb 04, 2022 at 06:56:52PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > In xfs_fs_reconfigure, check the return value from sync_filesystem and
> > fail the remount if there was an internal error.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_super.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 4c0dee78b2f8..5f3781879c63 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1831,7 +1831,9 @@ xfs_fs_reconfigure(
> >  	if (error)
> >  		return error;
> >  
> > -	sync_filesystem(mp->m_super);
> > +	error = sync_filesystem(mp->m_super);
> > +	if (error)
> > +		return error;
> >  
> >  	/* inode32 -> inode64 */
> >  	if (xfs_has_small_inums(mp) && !xfs_has_small_inums(new_mp)) {
> 
> Ummm, so why do we even need to call sync_filesystem()
> unconditionally here?  The only case where we have to actually write
> back anything on a remount is the rw->ro case, otherwise we aren't
> changing any state that requires data or metadata writeback.
> 
> I had a look at why sync_filesytem() was there, and it goes back to
> this commit that moved it from the VFS remount code:
> 
> commit 02b9984d640873b7b3809e63f81a0d7e13496886
> Author: Theodore Ts'o <tytso@mit.edu>
> Date:   Thu Mar 13 10:14:33 2014 -0400
> 
>     fs: push sync_filesystem() down to the file system's remount_fs()
>     
>     Previously, the no-op "mount -o mount /dev/xxx" operation when the
>     file system is already mounted read-write causes an implied,
>     unconditional syncfs().  This seems pretty stupid, and it's certainly
>     documented or guaraunteed to do this, nor is it particularly useful,
>     except in the case where the file system was mounted rw and is getting
>     remounted read-only.
>     
>     However, it's possible that there might be some file systems that are
>     actually depending on this behavior.  In most file systems, it's
>     probably fine to only call sync_filesystem() when transitioning from
>     read-write to read-only, and there are some file systems where this is
>     not needed at all (for example, for a pseudo-filesystem or something
>     like romfs).
> 
> And later on __ext4_remount got modified to only call
> sync_filesystem() on rw->ro transition. We do not depend on
> sync_filesystem() for anything here except in the rw->ro remount
> case, so why don't we just move the sync_filesystem() call to
> xfs_remount_ro()? We already have an error path there for failing to
> clean the filesystem, and avoids the possibility of other random
> mount option changes failing because some because there is some
> random pending data writeback error....

That's a good question.  I was doing the minimum needed to fix a
callsite.  Ted's commit seems to have pushed an unconditional (and
unchecked) call from the VFS into the first line of the filesystem
implementation.

Anyway I'll change the patch to move the call to xfs_remount_ro and
report back.  (Or rather, I did that last night, so we'll see what the
CI system reports...)

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com