[Cluster-devel] [GFS2 PATCH 0/3] gfs2: Fix freeze/thaw journal check problems

[Cluster-devel] [GFS2 PATCH 0/3] gfs2: Fix freeze/thaw journal check problems

[Bob Peterson]

This patch set fixes some problems in which the freeze glock's glop
functions were not working as expected.

Bob Peterson (3):
  gfs2: switch go_xmote_bh glop to pass gh not gl
  gfs2: Fix broken freeze_go_xmote_bh
  gfs2: Eliminate go_xmote_bh in favor of go_lock

 fs/gfs2/glock.c      | 12 +-----------
 fs/gfs2/glops.c      | 40 +++++++++++++++++++++++++---------------
 fs/gfs2/incore.h     |  1 -
 fs/gfs2/ops_fstype.c |  5 +++--
 fs/gfs2/recovery.c   |  3 ++-
 fs/gfs2/super.c      | 34 ++++++++++++----------------------
 6 files changed, 43 insertions(+), 52 deletions(-)

-- 
2.31.1

[Cluster-devel] [GFS2 PATCH 1/3] gfs2: switch go_xmote_bh glop to pass gh not gl

[Bob Peterson]

Before this patch, the go_xmote_bh function was passed gl, the glock
pointer. This patch switches it to gh, the holder, which points to the gl.
This facilitates improvements for the next patch.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glock.c  | 4 ++--
 fs/gfs2/glops.c  | 5 +++--
 fs/gfs2/incore.h | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index e0eaa9cf9fb6..d43eed1696ab 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -562,9 +562,9 @@ static void finish_xmote(struct gfs2_glock *gl, unsigned int ret)
 	if (test_and_clear_bit(GLF_DEMOTE_IN_PROGRESS, &gl->gl_flags))
 		gfs2_demote_wake(gl);
 	if (state != LM_ST_UNLOCKED) {
-		if (glops->go_xmote_bh) {
+		if (gh && glops->go_xmote_bh) {
 			spin_unlock(&gl->gl_lockref.lock);
-			rv = glops->go_xmote_bh(gl);
+			rv = glops->go_xmote_bh(gh);
 			spin_lock(&gl->gl_lockref.lock);
 			if (rv) {
 				do_error(gl, rv);
diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
index 79c621c7863d..6d0770564493 100644
--- a/fs/gfs2/glops.c
+++ b/fs/gfs2/glops.c
@@ -598,10 +598,11 @@ static int freeze_go_sync(struct gfs2_glock *gl)
 
 /**
  * freeze_go_xmote_bh - After promoting/demoting the freeze glock
- * @gl: the glock
+ * @gh: the holder of the glock
  */
-static int freeze_go_xmote_bh(struct gfs2_glock *gl)
+static int freeze_go_xmote_bh(struct gfs2_holder *gh)
 {
+	struct gfs2_glock *gl = gh->gh_gl;
 	struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
 	struct gfs2_inode *ip = GFS2_I(sdp->sd_jdesc->jd_inode);
 	struct gfs2_glock *j_gl = ip->i_gl;
diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index 0fe49770166e..354d6230d0f7 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -217,7 +217,7 @@ struct lm_lockname {
 
 struct gfs2_glock_operations {
 	int (*go_sync) (struct gfs2_glock *gl);
-	int (*go_xmote_bh)(struct gfs2_glock *gl);
+	int (*go_xmote_bh)(struct gfs2_holder *gh);
 	void (*go_inval) (struct gfs2_glock *gl, int flags);
 	int (*go_demote_ok) (const struct gfs2_glock *gl);
 	int (*go_lock) (struct gfs2_holder *gh);
-- 
2.31.1

[Cluster-devel] [GFS2 PATCH 1/3] gfs2: switch go_xmote_bh glop to pass gh not gl

[Andreas Gruenbacher]

On Tue, Aug 24, 2021 at 4:02 PM Bob Peterson <rpeterso@redhat.com> wrote:
> Before this patch, the go_xmote_bh function was passed gl, the glock
> pointer. This patch switches it to gh, the holder, which points to the gl.
> This facilitates improvements for the next patch.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>  fs/gfs2/glock.c  | 4 ++--
>  fs/gfs2/glops.c  | 5 +++--
>  fs/gfs2/incore.h | 2 +-
>  3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index e0eaa9cf9fb6..d43eed1696ab 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -562,9 +562,9 @@ static void finish_xmote(struct gfs2_glock *gl, unsigned int ret)
>         if (test_and_clear_bit(GLF_DEMOTE_IN_PROGRESS, &gl->gl_flags))
>                 gfs2_demote_wake(gl);
>         if (state != LM_ST_UNLOCKED) {
> -               if (glops->go_xmote_bh) {
> +               if (gh && glops->go_xmote_bh) {

This changes when the callback is called. Please explain why that's okay.

>                         spin_unlock(&gl->gl_lockref.lock);
> -                       rv = glops->go_xmote_bh(gl);
> +                       rv = glops->go_xmote_bh(gh);
>                         spin_lock(&gl->gl_lockref.lock);
>                         if (rv) {
>                                 do_error(gl, rv);
> diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> index 79c621c7863d..6d0770564493 100644
> --- a/fs/gfs2/glops.c
> +++ b/fs/gfs2/glops.c
> @@ -598,10 +598,11 @@ static int freeze_go_sync(struct gfs2_glock *gl)
>
>  /**
>   * freeze_go_xmote_bh - After promoting/demoting the freeze glock
> - * @gl: the glock
> + * @gh: the holder of the glock
>   */
> -static int freeze_go_xmote_bh(struct gfs2_glock *gl)
> +static int freeze_go_xmote_bh(struct gfs2_holder *gh)
>  {
> +       struct gfs2_glock *gl = gh->gh_gl;
>         struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
>         struct gfs2_inode *ip = GFS2_I(sdp->sd_jdesc->jd_inode);
>         struct gfs2_glock *j_gl = ip->i_gl;
> diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
> index 0fe49770166e..354d6230d0f7 100644
> --- a/fs/gfs2/incore.h
> +++ b/fs/gfs2/incore.h
> @@ -217,7 +217,7 @@ struct lm_lockname {
>
>  struct gfs2_glock_operations {
>         int (*go_sync) (struct gfs2_glock *gl);
> -       int (*go_xmote_bh)(struct gfs2_glock *gl);
> +       int (*go_xmote_bh)(struct gfs2_holder *gh);
>         void (*go_inval) (struct gfs2_glock *gl, int flags);
>         int (*go_demote_ok) (const struct gfs2_glock *gl);
>         int (*go_lock) (struct gfs2_holder *gh);
> --
> 2.31.1
>

Thanks,
Andreas

[Cluster-devel] [GFS2 PATCH 1/3] gfs2: switch go_xmote_bh glop to pass gh not gl

[Bob Peterson]

Hi,

On 8/24/21 11:12 AM, Andreas Gruenbacher wrote:
> On Tue, Aug 24, 2021 at 4:02 PM Bob Peterson <rpeterso@redhat.com> wrote:
>> Before this patch, the go_xmote_bh function was passed gl, the glock
>> pointer. This patch switches it to gh, the holder, which points to the gl.
>> This facilitates improvements for the next patch.
>>
>> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
>> ---
>>   fs/gfs2/glock.c  | 4 ++--
>>   fs/gfs2/glops.c  | 5 +++--
>>   fs/gfs2/incore.h | 2 +-
>>   3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
>> index e0eaa9cf9fb6..d43eed1696ab 100644
>> --- a/fs/gfs2/glock.c
>> +++ b/fs/gfs2/glock.c
>> @@ -562,9 +562,9 @@ static void finish_xmote(struct gfs2_glock *gl, unsigned int ret)
>>          if (test_and_clear_bit(GLF_DEMOTE_IN_PROGRESS, &gl->gl_flags))
>>                  gfs2_demote_wake(gl);
>>          if (state != LM_ST_UNLOCKED) {
>> -               if (glops->go_xmote_bh) {
>> +               if (gh && glops->go_xmote_bh) {
> 
> This changes when the callback is called. Please explain why that's okay.

This is okay because patch 3 eliminates go_xmote_bh() completely anyway.
I just threw the "gh &&" as an abundance of precaution.

Regards,

Bob

[Cluster-devel] [GFS2 PATCH 1/3] gfs2: switch go_xmote_bh glop to pass gh not gl

[Andreas Gruenbacher]

On Tue, Aug 24, 2021 at 6:48 PM Bob Peterson <rpeterso@redhat.com> wrote:
> On 8/24/21 11:12 AM, Andreas Gruenbacher wrote:
> > On Tue, Aug 24, 2021 at 4:02 PM Bob Peterson <rpeterso@redhat.com> wrote:
> >> Before this patch, the go_xmote_bh function was passed gl, the glock
> >> pointer. This patch switches it to gh, the holder, which points to the gl.
> >> This facilitates improvements for the next patch.
> >>
> >> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> >> ---
> >>   fs/gfs2/glock.c  | 4 ++--
> >>   fs/gfs2/glops.c  | 5 +++--
> >>   fs/gfs2/incore.h | 2 +-
> >>   3 files changed, 6 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> >> index e0eaa9cf9fb6..d43eed1696ab 100644
> >> --- a/fs/gfs2/glock.c
> >> +++ b/fs/gfs2/glock.c
> >> @@ -562,9 +562,9 @@ static void finish_xmote(struct gfs2_glock *gl, unsigned int ret)
> >>          if (test_and_clear_bit(GLF_DEMOTE_IN_PROGRESS, &gl->gl_flags))
> >>                  gfs2_demote_wake(gl);
> >>          if (state != LM_ST_UNLOCKED) {
> >> -               if (glops->go_xmote_bh) {
> >> +               if (gh && glops->go_xmote_bh) {
> >
> > This changes when the callback is called. Please explain why that's okay.
>
> This is okay because patch 3 eliminates go_xmote_bh() completely anyway.
> I just threw the "gh &&" as an abundance of precaution.

I didn't mean this as a joke. This patch changes when the go_xmote_bh
hook is called, and there is no mention why that's a safe thing to do.
Then the go_xmote_bh hook is removed in favor of go_lock, which is yet
called under different circumstances, and again there is no mention
why that doesn't matter and we still end up calling freeze_go_xmote_bh
(now freeze_go_lock) in the right circumstances.

Also, it's never been okay to break things just because a patch
further down the line fixes it again (or breaks it in a different
way). Please explain your changes; this also serves to document that
you haven't accidentally missed a case.

Thanks,
Andreas

[Cluster-devel] [GFS2 PATCH 1/3] gfs2: switch go_xmote_bh glop to pass gh not gl

[Bob Peterson]

On 8/24/21 5:27 PM, Andreas Gruenbacher wrote:
> On Tue, Aug 24, 2021 at 6:48 PM Bob Peterson <rpeterso@redhat.com> wrote:
>> On 8/24/21 11:12 AM, Andreas Gruenbacher wrote:
>>> On Tue, Aug 24, 2021 at 4:02 PM Bob Peterson <rpeterso@redhat.com> wrote:
>>>> Before this patch, the go_xmote_bh function was passed gl, the glock
>>>> pointer. This patch switches it to gh, the holder, which points to the gl.
>>>> This facilitates improvements for the next patch.
>>>>
>>>> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
>>>> ---
>>>>    fs/gfs2/glock.c  | 4 ++--
>>>>    fs/gfs2/glops.c  | 5 +++--
>>>>    fs/gfs2/incore.h | 2 +-
>>>>    3 files changed, 6 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
>>>> index e0eaa9cf9fb6..d43eed1696ab 100644
>>>> --- a/fs/gfs2/glock.c
>>>> +++ b/fs/gfs2/glock.c
>>>> @@ -562,9 +562,9 @@ static void finish_xmote(struct gfs2_glock *gl, unsigned int ret)
>>>>           if (test_and_clear_bit(GLF_DEMOTE_IN_PROGRESS, &gl->gl_flags))
>>>>                   gfs2_demote_wake(gl);
>>>>           if (state != LM_ST_UNLOCKED) {
>>>> -               if (glops->go_xmote_bh) {
>>>> +               if (gh && glops->go_xmote_bh) {
>>>
>>> This changes when the callback is called. Please explain why that's okay.
>>
>> This is okay because patch 3 eliminates go_xmote_bh() completely anyway.
>> I just threw the "gh &&" as an abundance of precaution.
> 
> I didn't mean this as a joke. This patch changes when the go_xmote_bh
> hook is called, and there is no mention why that's a safe thing to do.
> Then the go_xmote_bh hook is removed in favor of go_lock, which is yet
> called under different circumstances, and again there is no mention
> why that doesn't matter and we still end up calling freeze_go_xmote_bh
> (now freeze_go_lock) in the right circumstances.
> 
> Also, it's never been okay to break things just because a patch
> further down the line fixes it again (or breaks it in a different
> way). Please explain your changes; this also serves to document that
> you haven't accidentally missed a case.
> 
> Thanks,
> Andreas
> 
Hi Andreas,

The patch doesn't really change when the go_xmote_bh hook is called
because of the various circumstances under which it's called:

First of all, bear in mind that the only user of go_xmote_bh is the 
freeze glock.
There are only three places from which finish_xmote() is called:
(1) do_xmote calls finish_xmote for target == LM_ST_UNLOCKED.
     In this case finish_xmote() only calls glops->go_xmote_bh if state
     != LM_ST_UNLOCKED, so in this case, go_xmote_bh (and also
     do_promote) can never be called.
(2) do_xmote calls finish_xmote but only if lock_nolock locking
     protocol, which grants a holder immediately rather than making a
     lock request to dlm. In this case a holder must exist in this case
     because:
        do_xmote has the following callers:
           (2a) run_queue's call to do_xmote at the end, which passes in
                target as gl->gl_target.
                This is preceded by one of two cases:
                (2aa) If the GLF_DEMOTE bit is set, gh is passed in NULL,
                   but in that case gl->gl_target = gl->gl_demote_state.
                   We know gl->gl_demote_state cannot be LM_ST_EXCLUSIVE
                   or it would BUG_ON before the call. Since the freeze
                   glock is never taken in DEFERRED, it must be either
                   LM_ST_UNLOCKED or LM_ST_SHARED. If it is
                   LM_ST_UNLOCKED, finish_xmote will never call
                   go_xmote_bh because of "if (state != LM_ST_UNLOCKED)".
                   So we need only worry about SHARED.
                   (2aaa) There's only one place gl_demote_state is set
                      to a variable that could result in SHARED:
                      handle_callback. There's only one place
                      handle_callback is called with a variable that
                      could result in SHARED: gfs2_glock_cb.
                      There's only one place gfs2_glock_cb is called with
                      the constant LM_ST_SHARED: a callback from dlm,
                      which is impossible because we stated in (2) that
                      this is lock_nolock.
                      There's only one place gfs2_glock_cb is called with
                      a variable that could result in SHARED: sys.c when
                      the special gfs2 sysfs file is used to manually
                      force-promote a glock. This should never be used by
                      customers except in extreme deadlock situations,
                      and under the direction of support. It is not
                      tested, advised, nor recommended. Under normal
                      running conditions, this will never be called.
                (2ab) The GLF_DEMOTE test fails in run_queue. Prior to
                      its call to do_xmote it does:
                      "gh = find_first_waiter(gl);" then immediately
                      dereferences gh. So we know gh must be valid or
                      we would see kernel panic: finish_xmote will surely
                      find the same gh it dereferences here.
           (2b) finish_xmote has two calls back into do_xmote if its lock
                state change was not granted by dlm (conversion
                deadlock). We don't need to worry about these two calls
                because this is lock_nolock as per (2).
(3) glock_work_func calls finish_xmote if the GLF_REPLY_PENDING bit it
     set, passing gl_reply. Essentially this is the async/dlm version of
     the synchronous/nolock found in (1). But gl_reply is only set one
     place: gfs2_glock_complete which is a dlm callback.
     We can only get a dlm callback to gfs2_glock_complete from gdlm_ast,
     which is a reply to a lock (not unlock) request. A lock request is
     only made from do_xmote(), in which case glock_work_func's call to
     finish_xmote is responding to an async reply from dlm to that lock
     request. A lock request implies there's a "waiter" holder record
     which will be found by finish_xmote and passed into go_xmote_bh.

If you like, I can add a comment similar to the above to patch 1, but
I didn't see the great need to document all this, since the whole
go_xmote_bh concept is being replaced by the third patch anyway.

Before the third patch, finish_xmote() called BOTH go_xmote_bh() glop
(if one exists) AND do_promote(), which calls go_lock() glop (if one
exists) when the head-of-the-list holder is promoted. Calling
go_xmote_bh multiple times for multiple holders doesn't even make sense
for the freeze glock: we should not check the journal for a clean
unmount multiple times during a single freeze/thaw process.

The freeze glock will never have multiple SH holders on the same
cluster node because of the way gfs2's freeze/thaw mechanism works.
There are only 2 possible holders: (1) the EX holder while the file
system is frozen and (2) the SH holder that waits for the unfreeze.

So patch 3 switches the freeze glock to use the standard go_lock()
mechanism rather than go_xmote_bh, which is so poorly-thought-out that
patch 2 is needed to fix its short-comings.

Since the freeze glock did not already have a go_lock glop, switching
it from the go_xmote_bh glop to the go_lock glop retains the same
function called at essentially the same time.

Regards,

Bob

[Cluster-devel] [GFS2 PATCH 2/3] gfs2: Fix broken freeze_go_xmote_bh

[Bob Peterson]

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.
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.

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

[Cluster-devel] [GFS2 PATCH 2/3] gfs2: Fix broken freeze_go_xmote_bh

[Andreas Gruenbacher]

On Tue, Aug 24, 2021 at 4:02 PM Bob Peterson <rpeterso@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@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

[Cluster-devel] [GFS2 PATCH 3/3] gfs2: Eliminate go_xmote_bh in favor of go_lock

[Bob Peterson]

Before this patch, the freeze glock was the only glock to use the
go_xmote_bh glock op (glop). The go_xmote_bh glop is done when
a glock is locked. But so is go_lock. This patch eliminates the
glop altogether in favor of just using go_lock for the freeze
glock. This is for better consistency, readability, etc.
Using go_lock was previously not possible because of the bug
fixed by the previous patch.

I also fixed a misleading comment in do_promote.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glock.c  | 12 +-----------
 fs/gfs2/glops.c  |  6 +++---
 fs/gfs2/incore.h |  1 -
 3 files changed, 4 insertions(+), 15 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index d43eed1696ab..1d421998535e 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -384,7 +384,7 @@ static void do_error(struct gfs2_glock *gl, const int ret)
  * do_promote - promote as many requests as possible on the current queue
  * @gl: The glock
  * 
- * Returns: 1 if there is a blocked holder at the head of the list, or 2
+ * Returns: 1 if there is a blocked waiter at the head of the list, or 2
  *          if a type specific operation is underway.
  */
 
@@ -504,7 +504,6 @@ static void gfs2_demote_wake(struct gfs2_glock *gl)
 
 static void finish_xmote(struct gfs2_glock *gl, unsigned int ret)
 {
-	const struct gfs2_glock_operations *glops = gl->gl_ops;
 	struct gfs2_holder *gh;
 	unsigned state = ret & LM_OUT_ST_MASK;
 	int rv;
@@ -562,15 +561,6 @@ static void finish_xmote(struct gfs2_glock *gl, unsigned int ret)
 	if (test_and_clear_bit(GLF_DEMOTE_IN_PROGRESS, &gl->gl_flags))
 		gfs2_demote_wake(gl);
 	if (state != LM_ST_UNLOCKED) {
-		if (gh && glops->go_xmote_bh) {
-			spin_unlock(&gl->gl_lockref.lock);
-			rv = glops->go_xmote_bh(gh);
-			spin_lock(&gl->gl_lockref.lock);
-			if (rv) {
-				do_error(gl, rv);
-				goto out;
-			}
-		}
 		rv = do_promote(gl);
 		if (rv == 2)
 			goto out_locked;
diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
index 8ae2f33e014e..5067e89c32e6 100644
--- a/fs/gfs2/glops.c
+++ b/fs/gfs2/glops.c
@@ -597,10 +597,10 @@ static int freeze_go_sync(struct gfs2_glock *gl)
 }
 
 /**
- * freeze_go_xmote_bh - After promoting/demoting the freeze glock
+ * freeze_go_lock - After promoting/demoting the freeze glock
  * @gh: the holder of the glock
  */
-static int freeze_go_xmote_bh(struct gfs2_holder *gh)
+static int freeze_go_lock(struct gfs2_holder *gh)
 {
 	struct gfs2_glock *gl = gh->gh_gl;
 	struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
@@ -768,7 +768,7 @@ const struct gfs2_glock_operations gfs2_rgrp_glops = {
 
 const struct gfs2_glock_operations gfs2_freeze_glops = {
 	.go_sync = freeze_go_sync,
-	.go_xmote_bh = freeze_go_xmote_bh,
+	.go_lock = freeze_go_lock,
 	.go_demote_ok = freeze_go_demote_ok,
 	.go_type = LM_TYPE_NONDISK,
 	.go_flags = GLOF_NONDISK,
diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index 354d6230d0f7..04ac1537fee7 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -217,7 +217,6 @@ struct lm_lockname {
 
 struct gfs2_glock_operations {
 	int (*go_sync) (struct gfs2_glock *gl);
-	int (*go_xmote_bh)(struct gfs2_holder *gh);
 	void (*go_inval) (struct gfs2_glock *gl, int flags);
 	int (*go_demote_ok) (const struct gfs2_glock *gl);
 	int (*go_lock) (struct gfs2_holder *gh);
-- 
2.31.1