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

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

[Bob Peterson]

Hi,

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.

This patch simply removes the call to sb_start_write.

Signed-off-by: Bob Peterson <rpeterso@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;
 }
 

[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@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@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

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

[Bob Peterson]

Hi,

Before this patch, function gfs2_statfs_sync called sb_start_write and
sb_end_write. This is completely unnecessary because, aside from grabbing
glocks, gfs2_statfs_sync does all its updates to statfs with a transaction:
gfs2_trans_begin and _end. And transactions always do sb_start_intwrite in
gfs2_trans_begin and sb_end_intwrite in gfs2_trans_end.

This patch simply removes the call to sb_start_write.

Signed-off-by: Bob Peterson <rpeterso@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;
 }