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.sm...@citrix.com>
