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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxx>
> 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@xxxxxxxxxxxxx
