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....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
