[Cluster-devel] [GFS2 PATCH] gfs2: Remove sb_start_write from gfs2_statfs_sync

[Andreas Gruenbacher]

Hi Bob,

On Tue, Nov 24, 2020 at 9:27 PM Bob Peterson <rpeterso at redhat.com> wrote:
> Before this patch, function gfs2_statfs_sync called sb_start_write. This is a
> violation of the basic vfs rules that state that sb_start_write should always
> be taken before s_umount. See this document:
>
> https://www.kernel.org/doc/htmldocs/filesystems/API-sb-start-write.html
>
> "Since freeze protection behaves as a lock, users have to preserve
> ordering of freeze protection and other filesystem locks. Generally,
> freeze protection should be the outermost lock. In particular, we have:
>
> sb_start_write -> i_mutex (write path, truncate, directory ops, ...) ->
> s_umount (freeze_super, thaw_super)"
>
> deactivate_super
>    down_write(&s->s_umount); <------------------------------------ s_umount
>    deactivate_locked_super
>       gfs2_kill_sb
>          kill_block_super
>             generic_shutdown_super
>                gfs2_put_super
>                   gfs2_make_fs_ro
>                      gfs2_statfs_sync(sdp->sd_vfs, 0);
>                         sb_start_write <--------------------- sb_start_write
>
> As far as I can tell, gfs2_statfs_sync doesn't need to call sb_start_write
> any more than any other write to the file system, which are policed by glocks.
> None of the other functions in gfs2 lock sb_start_write so it only affects
> how vfs calls gfs2.

you're quite right that the sb_start_write doesn't make sense in the
above code path. That was equally true when the call was added in
commit 2e60d7683c8d2 ("GFS2: update freeze code to use
freeze/thaw_super on all nodes"), so I'm wondering what the intention
may have been here. Are there any code paths not going through the vfs
that need protection from filesystem freezes?

I'll leave this patch out for now, at least until it's more obvious
what's going on exactly.

> This patch simply removes the call to sb_start_write.
>
> Signed-off-by: Bob Peterson <rpeterso at redhat.com>
> ---
>  fs/gfs2/super.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index b3d951ab8068..2f56acc41c04 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -353,7 +353,6 @@ int gfs2_statfs_sync(struct super_block *sb, int type)
>         struct buffer_head *m_bh, *l_bh;
>         int error;
>
> -       sb_start_write(sb);
>         error = gfs2_glock_nq_init(m_ip->i_gl, LM_ST_EXCLUSIVE, GL_NOCACHE,
>                                    &gh);
>         if (error)
> @@ -392,7 +391,6 @@ int gfs2_statfs_sync(struct super_block *sb, int type)
>  out_unlock:
>         gfs2_glock_dq_uninit(&gh);
>  out:
> -       sb_end_write(sb);
>         return error;
>  }
>

Thanks,
Andreas