[Cluster-devel] [GFS2 PATCH] gfs2: Close timing window with GLF_INVALIDATE_IN_PROGRESS

[Cluster-devel] [GFS2 PATCH] gfs2: Close timing window with GLF_INVALIDATE_IN_PROGRESS

[Bob Peterson]

Hi,

This patch closes a timing window in which two processes compete
and overlap in the execution of do_xmote for the same glock:

             Process A                              Process B
   ------------------------------------   -----------------------------
1. Grabs gl_lockref and calls do_xmote
2.                                        Grabs gl_lockref but is blocked
3. Sets GLF_INVALIDATE_IN_PROGRESS
4. Unlocks gl_lockref
5.                                        Calls do_xmote
6. Call glops->go_sync
7. test_and_clear_bit GLF_DIRTY
8. Call gfs2_log_flush                    Call glops->go_sync
9. (slow IO, so it blocks a long time)    test_and_clear_bit GLF_DIRTY
                                          It's not dirty (step 7) returns
10.                                       Tests GLF_INVALIDATE_IN_PROGRESS
11.                                       Calls go_inval (rgrp_go_inval)
12.                                       gfs2_rgrp_relse does brelse
13.                                       truncate_inode_pages_range
14.                                       Calls lm_lock UN

In step 14 we've just told dlm to give the glock to another node
when, in fact, process A has not finished the IO and synced all
buffer_heads to disk and make sure their revokes are done.

This patch fixes the problem by changing the GLF_INVALIDATE_IN_PROGRESS
to use test_and_set_bit, and if the bit is already set, process B just
ignores it and trusts that process A will do the do_xmote in the proper
order.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glock.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index faa88bd594e2..4a4a390ffd00 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -558,7 +558,19 @@ __acquires(&gl->gl_lockref.lock)
 	GLOCK_BUG_ON(gl, gl->gl_state == gl->gl_target);
 	if ((target == LM_ST_UNLOCKED || target == LM_ST_DEFERRED) &&
 	    glops->go_inval) {
-		set_bit(GLF_INVALIDATE_IN_PROGRESS, &gl->gl_flags);
+		/*
+		 * If another process is already doing the invalidate we should
+		 * not interfere. If we call go_sync and it finds the glock is
+		 * not dirty, we might call go_inval prematurely before the
+		 * other go_sync has finished with its revokes. If we then call
+		 * lm_lock prematurely, we've really screwed up: we cannot tell
+		 * dlm to give the glock away until we're synced and
+		 * invalidated. Best thing is to return and trust the other
+		 * process will finish do_xmote tasks in their proper order.
+		 */
+		if (test_and_set_bit(GLF_INVALIDATE_IN_PROGRESS,
+				     &gl->gl_flags))
+			return;
 		do_error(gl, 0); /* Fail queued try locks */
 	}
 	gl->gl_req = target;

[Cluster-devel] [GFS2 PATCH] gfs2: Close timing window with GLF_INVALIDATE_IN_PROGRESS

[Andreas Gruenbacher]

On Fri, Nov 15, 2019 at 4:45 PM Bob Peterson <rpeterso@redhat.com> wrote:
>
> Hi,
>
> This patch closes a timing window in which two processes compete
> and overlap in the execution of do_xmote for the same glock:
>
>              Process A                              Process B
>    ------------------------------------   -----------------------------
> 1. Grabs gl_lockref and calls do_xmote
> 2.                                        Grabs gl_lockref but is blocked
> 3. Sets GLF_INVALIDATE_IN_PROGRESS
> 4. Unlocks gl_lockref
> 5.                                        Calls do_xmote
> 6. Call glops->go_sync
> 7. test_and_clear_bit GLF_DIRTY
> 8. Call gfs2_log_flush                    Call glops->go_sync
> 9. (slow IO, so it blocks a long time)    test_and_clear_bit GLF_DIRTY
>                                           It's not dirty (step 7) returns
> 10.                                       Tests GLF_INVALIDATE_IN_PROGRESS
> 11.                                       Calls go_inval (rgrp_go_inval)
> 12.                                       gfs2_rgrp_relse does brelse
> 13.                                       truncate_inode_pages_range
> 14.                                       Calls lm_lock UN
>
> In step 14 we've just told dlm to give the glock to another node
> when, in fact, process A has not finished the IO and synced all
> buffer_heads to disk and make sure their revokes are done.
>
> This patch fixes the problem by changing the GLF_INVALIDATE_IN_PROGRESS
> to use test_and_set_bit, and if the bit is already set, process B just
> ignores it and trusts that process A will do the do_xmote in the proper
> order.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>  fs/gfs2/glock.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index faa88bd594e2..4a4a390ffd00 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -558,7 +558,19 @@ __acquires(&gl->gl_lockref.lock)
>         GLOCK_BUG_ON(gl, gl->gl_state == gl->gl_target);
>         if ((target == LM_ST_UNLOCKED || target == LM_ST_DEFERRED) &&
>             glops->go_inval) {
> -               set_bit(GLF_INVALIDATE_IN_PROGRESS, &gl->gl_flags);
> +               /*
> +                * If another process is already doing the invalidate we should
> +                * not interfere. If we call go_sync and it finds the glock is
> +                * not dirty, we might call go_inval prematurely before the
> +                * other go_sync has finished with its revokes. If we then call
> +                * lm_lock prematurely, we've really screwed up: we cannot tell
> +                * dlm to give the glock away until we're synced and
> +                * invalidated. Best thing is to return and trust the other
> +                * process will finish do_xmote tasks in their proper order.
> +                */

That's a bit too much information here. Can we please change that as follows?

                * If another process is already doing the invalidate, let that
                * finish first.  The glock state machine will get back to this
                * holder again later.

> +               if (test_and_set_bit(GLF_INVALIDATE_IN_PROGRESS,
> +                                    &gl->gl_flags))
> +                       return;
>                 do_error(gl, 0); /* Fail queued try locks */
>         }
>         gl->gl_req = target;
>

Thanks,
Andreas

[Cluster-devel] [GFS2 PATCH] gfs2: Close timing window with GLF_INVALIDATE_IN_PROGRESS

[Bob Peterson]

----- Original Message -----
> On Fri, Nov 15, 2019 at 4:45 PM Bob Peterson <rpeterso@redhat.com> wrote:
> >
> > Hi,
> >
> > This patch closes a timing window in which two processes compete
> > and overlap in the execution of do_xmote for the same glock:
> >
> >              Process A                              Process B
> >    ------------------------------------   -----------------------------
> > 1. Grabs gl_lockref and calls do_xmote
> > 2.                                        Grabs gl_lockref but is blocked
> > 3. Sets GLF_INVALIDATE_IN_PROGRESS
> > 4. Unlocks gl_lockref
> > 5.                                        Calls do_xmote
> > 6. Call glops->go_sync
> > 7. test_and_clear_bit GLF_DIRTY
> > 8. Call gfs2_log_flush                    Call glops->go_sync
> > 9. (slow IO, so it blocks a long time)    test_and_clear_bit GLF_DIRTY
> >                                           It's not dirty (step 7) returns
> > 10.                                       Tests GLF_INVALIDATE_IN_PROGRESS
> > 11.                                       Calls go_inval (rgrp_go_inval)
> > 12.                                       gfs2_rgrp_relse does brelse
> > 13.                                       truncate_inode_pages_range
> > 14.                                       Calls lm_lock UN
> >
> > In step 14 we've just told dlm to give the glock to another node
> > when, in fact, process A has not finished the IO and synced all
> > buffer_heads to disk and make sure their revokes are done.
> >
> > This patch fixes the problem by changing the GLF_INVALIDATE_IN_PROGRESS
> > to use test_and_set_bit, and if the bit is already set, process B just
> > ignores it and trusts that process A will do the do_xmote in the proper
> > order.
> >
> > Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> > ---
> >  fs/gfs2/glock.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> > index faa88bd594e2..4a4a390ffd00 100644
> > --- a/fs/gfs2/glock.c
> > +++ b/fs/gfs2/glock.c
> > @@ -558,7 +558,19 @@ __acquires(&gl->gl_lockref.lock)
> >         GLOCK_BUG_ON(gl, gl->gl_state == gl->gl_target);
> >         if ((target == LM_ST_UNLOCKED || target == LM_ST_DEFERRED) &&
> >             glops->go_inval) {
> > -               set_bit(GLF_INVALIDATE_IN_PROGRESS, &gl->gl_flags);
> > +               /*
> > +                * If another process is already doing the invalidate we
> > should
> > +                * not interfere. If we call go_sync and it finds the glock
> > is
> > +                * not dirty, we might call go_inval prematurely before the
> > +                * other go_sync has finished with its revokes. If we then
> > call
> > +                * lm_lock prematurely, we've really screwed up: we cannot
> > tell
> > +                * dlm to give the glock away until we're synced and
> > +                * invalidated. Best thing is to return and trust the other
> > +                * process will finish do_xmote tasks in their proper
> > order.
> > +                */
> 
> That's a bit too much information here. Can we please change that as follows?
> 
>                 * If another process is already doing the invalidate, let
>                 that
>                 * finish first.  The glock state machine will get back to
>                 this
>                 * holder again later.
> 
> > +               if (test_and_set_bit(GLF_INVALIDATE_IN_PROGRESS,
> > +                                    &gl->gl_flags))
> > +                       return;
> >                 do_error(gl, 0); /* Fail queued try locks */
> >         }
> >         gl->gl_req = target;
> >
> 
> Thanks,
> Andreas
> 
> 
Sure. Make it so.

Bob

[Cluster-devel] [GFS2 PATCH] gfs2: Close timing window with GLF_INVALIDATE_IN_PROGRESS

[Andreas Gruenbacher]

Ok, pushed.

Thanks,
Andreas