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

[Andreas Gruenbacher]

On Mon, 8 Oct 2018 at 15:27, Tim Smith <tim.smith at citrix.com> wrote:
>
> 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?

No, what's important here is that the wake-up happens after the glock
being freed has been removed from the rhashtable, and that's the case
here. wake_up_glock actually accesses the glock, which may no longer
be around after the call_rcu, so swapping the two lines would
introduce a use-after-free bug.

There must be another reason for the missed wake-up. I'll have to
study the code some more.

Thanks,
Andreas