On Thu, Feb 17, 2022 at 01:36:44AM +0100, Andreas Gruenbacher wrote:
> On Wed, Feb 16, 2022 at 5:16 PM Alexander Aring <aahri...@redhat.com> wrote:
> > > > - spin_lock(&ops_lock);
> > > > - if (!list_empty(&op->list)) {
> > > > - log_error(ls, "dlm_posix_lock: op on list %llx",
> > > > - (unsigned long long)number);
> > > > - list_del(&op->list);
> > > > - }
> > > > - spin_unlock(&ops_lock);
> > > > + WARN_ON(!list_empty(&op->list));
> > >
> > > Why don't those checks need the ops_lock spin lock anymore?
> > > Why does it make sense to get rid of the list_del calls?
> >
> > My understanding is that those list_del() calls try to recover
> > something if a sanity check hits. The list_emptry() check should
> > always be true at this point no matter if lock is held or not.
> > Therefore no lock is required here to do some sanity checking.
>
> I don't immediately see what, other than the spin lock, would
> guarantee a consistent memory view. In other words, without taking the
> spin lock, 'list_empty(&op->list)' might still be true on one CPU even
> though another CPU has already added 'op' to a list.
I'm not sure what thread contexts are running on the CPUs in your example.
> So please, when changing the locking somewhere, explain why the change
> is correct beyond just stating that the locking isn't needed.
Since the removed locking was not actually doing anything useful, there's
a limited amount that can be said about what it changes.
It's clear that the ops_lock protects the lists. The op is not on any
list at this point, and if it were the code is broken. WARN_ON seems like
the preferred way to indicate failed assumptions in the code. In other
words, the code was making a shallow and cosmetic attempt to look robust
rather than broken.
Dave
