[Cluster-devel] [PATCH 1/2] GFS2: use schedule timeout in find insert glock

[Steven Whitehouse <swhiteho@redhat.com>]

On 09/10/18 09:13, Mark Syms wrote:
> Having swapped the line below around we still see the timeout on schedule fire, but only once in
> a fairly mega stress test. This is why we weren't worried about the timeout being HZ, the situation
> is hardly ever hit as having to wait is rare and normally we are woken from schedule and without
> a timeout on schedule we never wake up so a rare occurrence of waiting a second really doesn't
> seem too bad.
>
> Mark.
We should still get to the bottom of why the wake up is missing though, 
since without that fix we won't know if there is something else wrong 
somewhere,

Steve.

>
> -----Original Message-----
> From: Tim Smith <tim.smith@citrix.com>
> Sent: 08 October 2018 14:27
> To: Steven Whitehouse <swhiteho@redhat.com>
> Cc: Mark Syms <Mark.Syms@citrix.com>; cluster-devel at redhat.com; Ross Lagerwall <ross.lagerwall@citrix.com>
> Subject: Re: [Cluster-devel] [PATCH 1/2] GFS2: use schedule timeout in find insert glock
>
> On Monday, 8 October 2018 14:13:10 BST Steven Whitehouse wrote:
>> Hi,
>>
>> On 08/10/18 14:10, Tim Smith wrote:
>>> On Monday, 8 October 2018 14:03:24 BST Steven Whitehouse wrote:
>>>> On 08/10/18 13:59, Mark Syms wrote:
>>>>> That sounds entirely reasonable so long as you are absolutely sure
>>>>> that nothing is ever going to mess with that glock, we erred on
>>>>> the side of more caution not knowing whether it would be guaranteed safe or not.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> 	Mark
>>>> We should have a look at the history to see how that wait got added.
>>>> However the "dead" flag here means "don't touch this glock" and is
>>>> there so that we can separate the marking dead from the actual
>>>> removal from the list (which simplifies the locking during the
>>>> scanning procedures)
>>> You beat me to it :-)
>>>
>>> I think there might be a bit of a problem inserting a new entry with
>>> the same name before the old entry has been fully destroyed (or at
>>> least removed), which would be why the schedule() is there.
>> If the old entry is marked dead, all future lookups should ignore it.
>> We should only have a single non-dead entry at a time, but that
>> doesn't seem like it should need us to wait for it.
> On the second call we do have the new glock to insert as arg2, so we could try to swap them cleanly, yeah.
>
>> If we do discover that the wait is really required, then it sounds
>> like as you mentioned above there is a lost wakeup, and that must
>> presumably be on a code path that sets the dead flag and then fails to
>> send a wake up later on. If we can drop the wait in the first place,
>> that seems like a better plan,
> Ooooh, I wonder if these two lines:
>
> 	wake_up_glock(gl);
> 	call_rcu(&gl->gl_rcu, gfs2_glock_dealloc);
>
> in gfs2_glock_free() are the wrong way round?
>
> --
> Tim Smith <tim.smith@citrix.com>
>
>