[Cluster-devel] [PATCH] dlm: send_bast_queue() skip list loop not only sending basts to convertqueue

[Cluster-devel] [PATCH] dlm: send_bast_queue() skip list loop not only sending basts to convertqueue

[cmaiolino]

From: Carlos Maiolino <cmaiolino@redhat.com>

with the current check condition: if (gr == lkb), the list will skip not only basts sent to convertqueue
since gr == lkb could be true on another situations, where such can cause a gfs2 corruption.

Corruption checked on gfs2 resource groups, adding a file on a node and removing the file on another node.
The resource groups got corrupted without this patch:

This is an output of the gfs2_edit without this patch:

Environment:
	-a two node cluster
	-gfs2 filesystem with 1G (counting journal space)
	- /dev/sda5             1.0G  259M  766M  26% /mnt (new fs after mkfs.gfs2)

- A new filesystem shared by two nodes:

RG #1 located at: 65551 (0x1000f)
  mh_magic              0x01161970(hex)
  mh_type               2                   0x2
  mh_format             200                 0xc8
  rg_flags              0                   0x0
  rg_free               64858               0xfd5a
  rg_dinodes            11                  0xb

- After fill the filesystem with just one file (a 764M file using dd) from node 1

RG #1 located at: 65551 (0x1000f)
  mh_magic              0x01161970(hex)
  mh_type               2                   0x2
  mh_format             200                 0xc8
  rg_flags              0                   0x0
  rg_free               18                  0x12
  rg_dinodes            12                  0xc

- After remove the file from the filesystem from node 2

RG #1 located at: 65551 (0x1000f)
  mh_magic              0x01161970(hex)
  mh_type               2                   0x2
  mh_format             200                 0xc8
  rg_flags              0                   0x0
  rg_free               18                  0x12
  rg_dinodes            12                  0xc

- The space of the filesystem is not freed:

[root at node2-vm stats]# df -h /mnt
Filesystem            Size  Used Avail Use% Mounted on
/dev/sda              1.0G  1.0G  216K 100% /mnt

- The problem also persists after umount the filesystem

- Applying the patch, the problem is not reproducible anymore and the resource group blocks are properly freed

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/dlm/lock.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c
index 64e5f3e..565c519 100644
--- a/fs/dlm/lock.c
+++ b/fs/dlm/lock.c
@@ -1847,7 +1847,7 @@ static void send_bast_queue(struct dlm_rsb *r, struct list_head *head,
 
 	list_for_each_entry(gr, head, lkb_statequeue) {
 		/* skip self when sending basts to convertqueue */
-		if (gr == lkb)
+		if (head == &r->res_grantqueue && gr == lkb)
 			continue;
 		if (gr->lkb_bastfn && modes_require_bast(gr, lkb)) {
 			queue_bast(r, gr, lkb->lkb_rqmode);
-- 
1.7.1

[Cluster-devel] [PATCH] dlm: send_bast_queue() skip list loop not only sending basts to convertqueue

[David Teigland]

On Tue, Jan 04, 2011 at 06:06:51PM -0200, cmaiolino at redhat.com wrote:
> The resource groups got corrupted without this patch:

I could see an extraneous bast leading to confusion in gfs2 about the lock
state, but gfs2 should probably be asserting somewhere before it actually
corrupts anything...

> diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c
> index 64e5f3e..565c519 100644
> --- a/fs/dlm/lock.c
> +++ b/fs/dlm/lock.c
> @@ -1847,7 +1847,7 @@ static void send_bast_queue(struct dlm_rsb *r, struct list_head *head,
>  
>  	list_for_each_entry(gr, head, lkb_statequeue) {
>  		/* skip self when sending basts to convertqueue */
> -		if (gr == lkb)
> +		if (head == &r->res_grantqueue && gr == lkb)
>  			continue;
>  		if (gr->lkb_bastfn && modes_require_bast(gr, lkb)) {
>  			queue_bast(r, gr, lkb->lkb_rqmode);

I haven't been able to figure out the problem or the fix; some printk's
around the case in question would be revealing.

This is the specific case where a TRY_1CB (NOQUEUBAST) conversion fails.
Here's how I step through the code for that case:

_convert_lock(lkb)
    error = do_convert(lkb)
        when error equals -EAGAIN, lkb remains on grantqueue
    do_convert_effects(lkb, -EAGAIN)
        -EAGAIN and NOQUEUEBAST -> send_blocking_asts_all ->
        send_bast_queue(grantqueue, lkb)
           [lkb is expected to be here, skip sending bast to self]
        send_bast_queue(convertqueue, lkb):
           [lkb should not be on here, but your patch implies there
            are cases where it can be?  I think that would be a bug.]

Dave

[Cluster-devel] [PATCH] dlm: send_bast_queue() skip list loop not only sending basts to convertqueue

[Steven Whitehouse]

Hi,

On Tue, 2011-01-04 at 16:27 -0500, David Teigland wrote:
> On Tue, Jan 04, 2011 at 06:06:51PM -0200, cmaiolino at redhat.com wrote:
> > The resource groups got corrupted without this patch:
> 
> I could see an extraneous bast leading to confusion in gfs2 about the lock
> state, but gfs2 should probably be asserting somewhere before it actually
> corrupts anything...
> 
I don't believe that this is the right solution, although it might be a
useful clue as to what is really going wrong. Also, nothing is getting
corrupted at all, the issue is that deallocation fails to take place at
the expected time. Thats not the end of the world, since there is
already a mechanism in place to catch that at a later point in time.

Carlos, why do you think that the dlm should be sending basts to the
lock requesting process for its own lock request?

That test was added specifically to prevent performance issues due to
extraneous basts being sent in such cases. I'd suggest getting some
traces of the glocks in question and checking to see what is happening,
and whether it is happening in the correct order,

Steve.

> > diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c
> > index 64e5f3e..565c519 100644
> > --- a/fs/dlm/lock.c
> > +++ b/fs/dlm/lock.c
> > @@ -1847,7 +1847,7 @@ static void send_bast_queue(struct dlm_rsb *r, struct list_head *head,
> >  
> >  	list_for_each_entry(gr, head, lkb_statequeue) {
> >  		/* skip self when sending basts to convertqueue */
> > -		if (gr == lkb)
> > +		if (head == &r->res_grantqueue && gr == lkb)
> >  			continue;
> >  		if (gr->lkb_bastfn && modes_require_bast(gr, lkb)) {
> >  			queue_bast(r, gr, lkb->lkb_rqmode);
> 
> I haven't been able to figure out the problem or the fix; some printk's
> around the case in question would be revealing.
> 
> This is the specific case where a TRY_1CB (NOQUEUBAST) conversion fails.
> Here's how I step through the code for that case:
> 
> _convert_lock(lkb)
>     error = do_convert(lkb)
>         when error equals -EAGAIN, lkb remains on grantqueue
>     do_convert_effects(lkb, -EAGAIN)
>         -EAGAIN and NOQUEUEBAST -> send_blocking_asts_all ->
>         send_bast_queue(grantqueue, lkb)
>            [lkb is expected to be here, skip sending bast to self]
>         send_bast_queue(convertqueue, lkb):
>            [lkb should not be on here, but your patch implies there
>             are cases where it can be?  I think that would be a bug.]
> 
> Dave
>