[Cluster-devel] [GFS2 PATCH 2/3] gfs2: Fix broken freeze_go_xmote_bh
Andreas Gruenbacher
agruenba at redhat.com
Tue Aug 24 16:10:25 UTC 2021
On Tue, Aug 24, 2021 at 4:02 PM Bob Peterson <rpeterso at redhat.com> wrote:
> The freeze glock was used in several places whenever a gfs2 file system
> was frozen, thawed, mounted, unmounted, remounted, or withdrawn. It was
> used to prevent those things from clashing with one another.
> Function freeze_go_xmote_bh only checked if the journal was clean in
> cases where the journal was LIVE. That's wrong because if the journal is
> really live, it's a moving target and almost guaranteed to be dirty.
> If the journal was not live, the check for a clean journal was skipped.
> That's also wrong because sometimes the journal would be LIVE and
> sometimes not: It was LIVE when gfs2_reconfigure locks the freeze lock
> for "remount -o remount,ro." It was not LIVE when gfs2_fill_super called
> gfs2_freeze_lock before gfs2_make_fs_rw sets the LIVE bit.
This is confusing to a point where it's useless as documentation.
Could you please better explain what's going on?
> The problem was never noticed because gfs2_make_fs_rw had redundant code
> for the exact same checks as freeze_go_xmote_bh.
>
> This patch attempts to clean up the mess by removing the redundant code
> from gfs2_make_fs_rw and changing the callers of gfs2_freeze_lock to
> specify whether they need the journal checked for consistency:
> If the journal consistency check is unwanted, they specify GL_SKIP in
> the holder. If the check is required, they do not specify GL_SKIP.
> Most callers determine if the consistency check is needed based on if
> the journal is being transitioned from "not live" to "live": If it is
> becoming live, the check is needed, otherwise it is not.
The patch itself looks good at first sight.
> Signed-off-by: Bob Peterson <rpeterso at redhat.com>
> ---
> fs/gfs2/glops.c | 31 ++++++++++++++++++++-----------
> fs/gfs2/ops_fstype.c | 5 +++--
> fs/gfs2/recovery.c | 3 ++-
> fs/gfs2/super.c | 34 ++++++++++++----------------------
> 4 files changed, 37 insertions(+), 36 deletions(-)
>
> diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> index 6d0770564493..8ae2f33e014e 100644
> --- a/fs/gfs2/glops.c
> +++ b/fs/gfs2/glops.c
> @@ -609,18 +609,27 @@ static int freeze_go_xmote_bh(struct gfs2_holder *gh)
> struct gfs2_log_header_host head;
> int error;
>
> - if (test_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags)) {
> - j_gl->gl_ops->go_inval(j_gl, DIO_METADATA);
> + if (gh->gh_flags & GL_SKIP)
> + return 0;
>
> - error = gfs2_find_jhead(sdp->sd_jdesc, &head, false);
> - if (gfs2_assert_withdraw_delayed(sdp, !error))
> - return error;
> - if (gfs2_assert_withdraw_delayed(sdp, head.lh_flags &
> - GFS2_LOG_HEAD_UNMOUNT))
> - return -EIO;
> - sdp->sd_log_sequence = head.lh_sequence + 1;
> - gfs2_log_pointers_init(sdp, head.lh_blkno);
> - }
> + /*
> + * If our journal is truly live, rw, it is guaranteed to be dirty.
> + * If our journal is ro, and we are soon to make it live, we need to
> + * check whether it was cleanly unmounted. If not, we withdraw.
> + */
> + if (gfs2_assert_withdraw_delayed(sdp,
> + !test_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags)))
> + return -EIO;
> + j_gl->gl_ops->go_inval(j_gl, DIO_METADATA);
> +
> + error = gfs2_find_jhead(sdp->sd_jdesc, &head, false);
> + if (gfs2_assert_withdraw_delayed(sdp, !error))
> + return error;
> + if (gfs2_assert_withdraw_delayed(sdp, head.lh_flags &
> + GFS2_LOG_HEAD_UNMOUNT))
> + return -EIO;
> + sdp->sd_log_sequence = head.lh_sequence + 1;
> + gfs2_log_pointers_init(sdp, head.lh_blkno);
> return 0;
> }
>
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index 7f8410d8fdc1..6f4be312bd34 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -1263,7 +1263,7 @@ static int gfs2_fill_super(struct super_block *sb, struct fs_context *fc)
> }
> }
>
> - error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
> + error = gfs2_freeze_lock(sdp, &freeze_gh, sb_rdonly(sb) ? GL_SKIP : 0);
> if (error)
> goto fail_per_node;
>
> @@ -1584,7 +1584,8 @@ static int gfs2_reconfigure(struct fs_context *fc)
> if ((sb->s_flags ^ fc->sb_flags) & SB_RDONLY) {
> struct gfs2_holder freeze_gh;
>
> - error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
> + error = gfs2_freeze_lock(sdp, &freeze_gh,
> + fc->sb_flags & SB_RDONLY ? GL_SKIP : 0);
> if (error)
> return -EINVAL;
>
> diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
> index 016ed1b2ca1d..be0037da3bb4 100644
> --- a/fs/gfs2/recovery.c
> +++ b/fs/gfs2/recovery.c
> @@ -472,7 +472,8 @@ void gfs2_recover_func(struct work_struct *work)
>
> /* Acquire a shared hold on the freeze lock */
>
> - error = gfs2_freeze_lock(sdp, &thaw_gh, LM_FLAG_PRIORITY);
> + error = gfs2_freeze_lock(sdp, &thaw_gh, LM_FLAG_PRIORITY |
> + GL_SKIP);
> if (error)
> goto fail_gunlock_ji;
>
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index 6e00d15ef0a8..fe6cea188199 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -128,28 +128,8 @@ int gfs2_jdesc_check(struct gfs2_jdesc *jd)
>
> 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_log_header_host head;
> int error;
>
> - j_gl->gl_ops->go_inval(j_gl, DIO_METADATA);
> - if (gfs2_withdrawn(sdp))
> - return -EIO;
> -
> - error = gfs2_find_jhead(sdp->sd_jdesc, &head, false);
> - if (error || gfs2_withdrawn(sdp))
> - return error;
> -
> - if (!(head.lh_flags & GFS2_LOG_HEAD_UNMOUNT)) {
> - gfs2_consist(sdp);
> - return -EIO;
> - }
> -
> - /* Initialize some head of the log stuff */
> - sdp->sd_log_sequence = head.lh_sequence + 1;
> - gfs2_log_pointers_init(sdp, head.lh_blkno);
> -
> error = gfs2_quota_init(sdp);
> if (!error && !gfs2_withdrawn(sdp))
> set_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
> @@ -346,7 +326,8 @@ static int gfs2_lock_fs_check_clean(struct gfs2_sbd *sdp)
> }
>
> error = gfs2_glock_nq_init(sdp->sd_freeze_gl, LM_ST_EXCLUSIVE,
> - LM_FLAG_NOEXP, &sdp->sd_freeze_gh);
> + LM_FLAG_NOEXP | GL_SKIP,
> + &sdp->sd_freeze_gh);
> if (error)
> goto out;
>
> @@ -654,7 +635,16 @@ void gfs2_freeze_func(struct work_struct *work)
> struct super_block *sb = sdp->sd_vfs;
>
> atomic_inc(&sb->s_active);
> - error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
> + /*
> + * Here we take the freeze lock in SH to wait until a freeze operation
> + * (that began with gfs2_freeze's call to gfs2_lock_fs_check_clean
> + * which takes the freeze gl in EX) has been thawed. Function
> + * gfs2_lock_fs_check_clean checks that all the journals are clean,
> + * so we don't need to do it again with this gfs2_freeze_lock.
> + * In fact, our journal is live when this glock is granted, so the
> + * freeze_go_xmote_bh will withdraw unless we specify GL_SKIP.
> + */
> + error = gfs2_freeze_lock(sdp, &freeze_gh, GL_SKIP);
> if (error) {
> gfs2_assert_withdraw(sdp, 0);
> } else {
> --
> 2.31.1
>
Thanks,
Andreas
More information about the Cluster-devel
mailing list