[Cluster-devel] [PATCH dlm/next 1/4] fs: dlm: replace sanity checks with WARN_ON

[David Teigland <teigland@redhat.com>]

On Thu, Feb 17, 2022 at 01:36:44AM +0100, Andreas Gruenbacher wrote:
> On Wed, Feb 16, 2022 at 5:16 PM Alexander Aring <aahringo@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