[Cluster-devel] [PATCH] gfs2: Fix missed wakeups in find_insert_glock

[Cluster-devel] [PATCH] gfs2: Fix missed wakeups in find_insert_glock

[Andreas Gruenbacher]

Mark Syms has reported seeing tasks that are stuck waiting in
find_insert_glock.  It turns out that struct lm_lockname contains four padding
bytes on 64-bit architectures that function glock_waitqueue doesn't skip when
hashing the glock name.  As a result, we can end up waking up the wrong
waitqueue, and the waiting tasks will be stuck.

Use offsetofend instead of sizeof to get the struct size without the padding.

Reported-by: Mark Syms <mark.syms@citrix.com>
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/glock.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index f66773c71bcde..01611f363cd59 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -107,7 +107,8 @@ static int glock_wake_function(wait_queue_entry_t *wait, unsigned int mode,
 
 static wait_queue_head_t *glock_waitqueue(struct lm_lockname *name)
 {
-	u32 hash = jhash2((u32 *)name, sizeof(*name) / 4, 0);
+	unsigned int size = offsetofend(struct lm_lockname, ln_type);
+	u32 hash = jhash2((u32 *)name, size / 4, 0);
 
 	return glock_wait_table + hash_32(hash, GLOCK_WAIT_TABLE_BITS);
 }
-- 
2.20.1

[Cluster-devel] [PATCH] gfs2: Fix missed wakeups in find_insert_glock

[Edwin Török]

On 06/03/2019 16:14, Andreas Gruenbacher wrote:
> Mark Syms has reported seeing tasks that are stuck waiting in
> find_insert_glock.  It turns out that struct lm_lockname contains four padding
> bytes on 64-bit architectures that function glock_waitqueue doesn't skip when
> hashing the glock name.  As a result, we can end up waking up the wrong
> waitqueue, and the waiting tasks will be stuck.
> 
> Use offsetofend instead of sizeof to get the struct size without the padding.
> 
> Reported-by: Mark Syms <mark.syms@citrix.com>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>

Thank you for your patch, initial testing looks promising when replacing the schedule_timeout patch with this one (no deadlocks/stuck tasks found so far).
Will let you know when our testing completes.

> ---
>  fs/gfs2/glock.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index f66773c71bcde..01611f363cd59 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -107,7 +107,8 @@ static int glock_wake_function(wait_queue_entry_t *wait, unsigned int mode,
>  
>  static wait_queue_head_t *glock_waitqueue(struct lm_lockname *name)
>  {
> -	u32 hash = jhash2((u32 *)name, sizeof(*name) / 4, 0);
> +	unsigned int size = offsetofend(struct lm_lockname, ln_type);

This is the same as ht_parms.key_len, which I assume was the intention.
Should we just use ht_parms.key_len for size instead of calculating it again to avoid the 2 values becoming
different in the future again?

> +	u32 hash = jhash2((u32 *)name, size / 4, 0);
>  
>  	return glock_wait_table + hash_32(hash, GLOCK_WAIT_TABLE_BITS);
>  }
> 

Best regards,
--Edwin

[Cluster-devel] [PATCH] gfs2: Fix missed wakeups in find_insert_glock

[Andreas Gruenbacher]

Hi Edwin,

On Thu, 7 Mar 2019 at 16:55, Edwin T?r?k <edvin.torok@citrix.com> wrote:
> On 06/03/2019 16:14, Andreas Gruenbacher wrote:
> > Mark Syms has reported seeing tasks that are stuck waiting in
> > find_insert_glock.  It turns out that struct lm_lockname contains four padding
> > bytes on 64-bit architectures that function glock_waitqueue doesn't skip when
> > hashing the glock name.  As a result, we can end up waking up the wrong
> > waitqueue, and the waiting tasks will be stuck.
> >
> > Use offsetofend instead of sizeof to get the struct size without the padding.
> >
> > Reported-by: Mark Syms <mark.syms@citrix.com>
> > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
>
> Thank you for your patch, initial testing looks promising when replacing the schedule_timeout patch with this one (no deadlocks/stuck tasks found so far).
> Will let you know when our testing completes.

okay great, thanks. Hope we can still get this into the 5.1 kernel.

> > ---
> >  fs/gfs2/glock.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> > index f66773c71bcde..01611f363cd59 100644
> > --- a/fs/gfs2/glock.c
> > +++ b/fs/gfs2/glock.c
> > @@ -107,7 +107,8 @@ static int glock_wake_function(wait_queue_entry_t *wait, unsigned int mode,
> >
> >  static wait_queue_head_t *glock_waitqueue(struct lm_lockname *name)
> >  {
> > -     u32 hash = jhash2((u32 *)name, sizeof(*name) / 4, 0);
> > +     unsigned int size = offsetofend(struct lm_lockname, ln_type);
>
> This is the same as ht_parms.key_len, which I assume was the intention.
> Should we just use ht_parms.key_len for size instead of calculating it again to avoid the 2 values becoming
> different in the future again?

We can do that, yes.

> > +     u32 hash = jhash2((u32 *)name, size / 4, 0);
> >
> >       return glock_wait_table + hash_32(hash, GLOCK_WAIT_TABLE_BITS);
> >  }

Thanks,
Andreas

[Cluster-devel] [PATCH] gfs2: Fix missed wakeups in find_insert_glock

[Edwin Török]

On 07/03/2019 16:23, Andreas Gruenbacher wrote:
> Hi Edwin,
> 
> On Thu, 7 Mar 2019 at 16:55, Edwin T?r?k <edvin.torok@citrix.com> wrote:
>> On 06/03/2019 16:14, Andreas Gruenbacher wrote:
>>> Mark Syms has reported seeing tasks that are stuck waiting in
>>> find_insert_glock.  It turns out that struct lm_lockname contains four padding
>>> bytes on 64-bit architectures that function glock_waitqueue doesn't skip when
>>> hashing the glock name.  As a result, we can end up waking up the wrong
>>> waitqueue, and the waiting tasks will be stuck.
>>>
>>> Use offsetofend instead of sizeof to get the struct size without the padding.
>>>
>>> Reported-by: Mark Syms <mark.syms@citrix.com>
>>> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
>>
>> Thank you for your patch, initial testing looks promising when replacing the schedule_timeout patch with this one (no deadlocks/stuck tasks found so far).
>> Will let you know when our testing completes.
> 
> okay great, thanks. Hope we can still get this into the 5.1 kernel.


Testing of this patch itself looked good (no new issues), however GFS2 still deadlocks on 4.19 even with this patch, see:
https://www.redhat.com/archives/cluster-devel/2019-March/msg00000.html
[the iomap deadlocks seems to be more rare now though]

Thanks,
--Edwin