On Tue, Dec 22, 2020 at 9:43 PM Bob Peterson <[email protected]> wrote:
>
> Before this patch, sister functions gfs2_make_fs_rw and gfs2_make_fs_ro locked
> (held) the freeze glock by calling gfs2_freeze_lock and gfs2_freeze_unlock.
> The problem is, not all the callers of gfs2_make_fs_ro should be doing this.
> The three callers of gfs2_make_fs_ro are: remount (gfs2_reconfigure),
> signal_our_withdraw, and unmount (gfs2_put_super). But when unmounting the
> file system we can get into the following circular lock dependency:
>
> 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_glock_nq_init sd_freeze_gl
> freeze_go_sync
> if (freeze glock in SH)
> freeze_super (vfs)
> down_write(&sb->s_umount); <------- s_umount
>
> This patch moves the hold of the freeze glock outside the two sister rw/ro
> functions to their callers, but it doesn't request the glock from
> gfs2_put_super, thus eliminating the circular dependency.
>
> Signed-off-by: Bob Peterson <[email protected]>
> ---
> fs/gfs2/ops_fstype.c | 31 +++++++++++++++++--------------
> fs/gfs2/super.c | 23 -----------------------
> fs/gfs2/util.c | 18 ++++++++++++++++--
> 3 files changed, 33 insertions(+), 39 deletions(-)
>
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index c71c7beb9257..cd3fd252c771 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -1084,6 +1084,7 @@ static int gfs2_fill_super(struct super_block *sb,
> struct fs_context *fc)
> int silent = fc->sb_flags & SB_SILENT;
> struct gfs2_sbd *sdp;
> struct gfs2_holder mount_gh;
> + struct gfs2_holder freeze_gh;
> int error;
>
> sdp = init_sbd(sb);
> @@ -1195,23 +1196,18 @@ static int gfs2_fill_super(struct super_block *sb,
> struct fs_context *fc)
> goto fail_per_node;
> }
>
> - if (sb_rdonly(sb)) {
> - struct gfs2_holder freeze_gh;
> + error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
> + if (error)
> + goto fail_per_node;
>
> - error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
> - if (error) {
> - fs_err(sdp, "can't make FS RO: %d\n", error);
> - goto fail_per_node;
> - }
> - gfs2_freeze_unlock(&freeze_gh);
> - } else {
> + if (!sb_rdonly(sb))
> error = gfs2_make_fs_rw(sdp);
> - if (error) {
> - fs_err(sdp, "can't make FS RW: %d\n", error);
> - goto fail_per_node;
> - }
> - }
>
> + gfs2_freeze_unlock(&freeze_gh);
> + if (error) {
> + fs_err(sdp, "can't make FS RW: %d\n", error);
> + goto fail_per_node;
> + }
> gfs2_glock_dq_uninit(&mount_gh);
> gfs2_online_uevent(sdp);
> return 0;
> @@ -1512,6 +1508,12 @@ static int gfs2_reconfigure(struct fs_context *fc)
> fc->sb_flags |= SB_RDONLY;
>
> if ((sb->s_flags ^ fc->sb_flags) & SB_RDONLY) {
> + struct gfs2_holder freeze_gh;
> +
> + error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
> + if (error)
> + return -EINVAL;
> +
> if (fc->sb_flags & SB_RDONLY) {
> error = gfs2_make_fs_ro(sdp);
> if (error)
> @@ -1521,6 +1523,7 @@ static int gfs2_reconfigure(struct fs_context *fc)
> if (error)
> errorfc(fc, "unable to remount read-write");
> }
> + gfs2_freeze_unlock(&freeze_gh);
> }
> sdp->sd_args = *newargs;
>
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index ea312a94ce69..754ea2a137b4 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -165,7 +165,6 @@ int gfs2_make_fs_rw(struct gfs2_sbd *sdp)
> {
> struct gfs2_inode *ip = GFS2_I(sdp->sd_jdesc->jd_inode);
> struct gfs2_glock *j_gl = ip->i_gl;
> - struct gfs2_holder freeze_gh;
> struct gfs2_log_header_host head;
> int error;
>
> @@ -173,10 +172,6 @@ int gfs2_make_fs_rw(struct gfs2_sbd *sdp)
> if (error)
> return error;
>
> - error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
> - if (error)
> - goto fail_threads;
> -
> j_gl->gl_ops->go_inval(j_gl, DIO_METADATA);
> if (gfs2_withdrawn(sdp)) {
> error = -EIO;
> @@ -203,13 +198,9 @@ int gfs2_make_fs_rw(struct gfs2_sbd *sdp)
>
> set_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
>
> - gfs2_freeze_unlock(&freeze_gh);
> -
> return 0;
>
> fail:
> - gfs2_freeze_unlock(&freeze_gh);
> -fail_threads:
> if (sdp->sd_quotad_process)
> kthread_stop(sdp->sd_quotad_process);
> sdp->sd_quotad_process = NULL;
> @@ -607,21 +598,9 @@ static void gfs2_dirty_inode(struct inode *inode, int
> flags)
>
> int gfs2_make_fs_ro(struct gfs2_sbd *sdp)
> {
> - struct gfs2_holder freeze_gh;
> int error = 0;
> int log_write_allowed = test_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
>
> - gfs2_holder_mark_uninitialized(&freeze_gh);
> - if (sdp->sd_freeze_gl &&
> - !gfs2_glock_is_locked_by_me(sdp->sd_freeze_gl)) {
> - error = gfs2_freeze_lock(sdp, &freeze_gh,
> - log_write_allowed ? 0 : LM_FLAG_TRY);
> - if (error == GLR_TRYFAILED)
> - error = 0;
> - if (error && !gfs2_withdrawn(sdp))
> - return error;
> - }
> -
> gfs2_flush_delete_work(sdp);
> if (!log_write_allowed && current == sdp->sd_quotad_process)
> fs_warn(sdp, "The quotad daemon is withdrawing.\n");
> @@ -650,8 +629,6 @@ int gfs2_make_fs_ro(struct gfs2_sbd *sdp)
> atomic_read(&sdp->sd_reserving_log) == 0,
> HZ * 5);
> }
> - gfs2_freeze_unlock(&freeze_gh);
> -
> gfs2_quota_cleanup(sdp);
>
> if (!log_write_allowed)
> diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c
> index a115c441e2a1..87070e433f96 100644
> --- a/fs/gfs2/util.c
> +++ b/fs/gfs2/util.c
> @@ -122,6 +122,7 @@ static void signal_our_withdraw(struct gfs2_sbd *sdp)
> struct inode *inode = sdp->sd_jdesc->jd_inode;
> struct gfs2_inode *ip = GFS2_I(inode);
> u64 no_formal_ino = ip->i_no_formal_ino;
> + int log_write_allowed = test_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
> int ret = 0;
> int tries;
>
> @@ -142,8 +143,21 @@ static void signal_our_withdraw(struct gfs2_sbd *sdp)
> * therefore we need to clear SDF_JOURNAL_LIVE manually.
> */
> clear_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
> - if (!sb_rdonly(sdp->sd_vfs))
> - ret = gfs2_make_fs_ro(sdp);
> + if (!sb_rdonly(sdp->sd_vfs)) {
> + struct gfs2_holder freeze_gh;
> +
> + gfs2_holder_mark_uninitialized(&freeze_gh);
> + if (sdp->sd_freeze_gl &&
> + !gfs2_glock_is_locked_by_me(sdp->sd_freeze_gl))
I'm adding a little cleanup here, but otherwise this patch looks good now:
--- a/fs/gfs2/util.c
+++ b/fs/gfs2/util.c
@@ -148,15 +148,15 @@ static void signal_our_withdraw(struct gfs2_sbd *sdp)
gfs2_holder_mark_uninitialized(&freeze_gh);
if (sdp->sd_freeze_gl &&
- !gfs2_glock_is_locked_by_me(sdp->sd_freeze_gl))
+ !gfs2_glock_is_locked_by_me(sdp->sd_freeze_gl)) {
ret = gfs2_freeze_lock(sdp, &freeze_gh,
log_write_allowed ? 0 : LM_FLAG_TRY);
- if (ret == GLR_TRYFAILED)
- ret = 0;
- if (!ret) {
- ret = gfs2_make_fs_ro(sdp);
- gfs2_freeze_unlock(&freeze_gh);
+ if (ret == GLR_TRYFAILED)
+ ret = 0;
}
+ if (!ret)
+ ret = gfs2_make_fs_ro(sdp);
+ gfs2_freeze_unlock(&freeze_gh);
}
if (sdp->sd_lockstruct.ls_ops->lm_lock == NULL) { /* lock_nolock */
> + ret = gfs2_freeze_lock(sdp, &freeze_gh,
> + log_write_allowed ? 0 : LM_FLAG_TRY);
> + if (ret == GLR_TRYFAILED)
> + ret = 0;
> + if (!ret) {
> + ret = gfs2_make_fs_ro(sdp);
> + gfs2_freeze_unlock(&freeze_gh);
> + }
> + }
>
> if (sdp->sd_lockstruct.ls_ops->lm_lock == NULL) { /* lock_nolock */
> if (!ret)
> --
> 2.29.2
>
Thanks,
Andreas
