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

[Andreas Gruenbacher]

On Wed, Feb 16, 2022 at 5:16 PM Alexander Aring <aahringo at redhat.com> wrote:
>
> Hi,
>
> On Wed, Feb 16, 2022 at 11:08 AM Andreas Gruenbacher
> <agruenba at redhat.com> wrote:
> >
> > On Wed, Feb 16, 2022 at 4:53 PM Alexander Aring <aahringo at redhat.com> wrote:
> > > There are several sanity checks and recover handling if they occur in
> > > the dlm plock handling. They should never occur otherwise we have a bug
> > > in the code. To make such bugs more visible we remove the recover
> > > handling and add a WARN_ON() on those sanity checks.
> > >
> > > Signed-off-by: Alexander Aring <aahringo at redhat.com>
> > > ---
> > >  fs/dlm/plock.c | 32 ++++----------------------------
> > >  1 file changed, 4 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
> > > index a10d2bcfe75a..55fba2f0234f 100644
> > > --- a/fs/dlm/plock.c
> > > +++ b/fs/dlm/plock.c
> > > @@ -157,13 +157,7 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file,
> > >                 goto out;
> > >         }
> > >
> > > -       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. So please, when
changing the locking somewhere, explain why the change is correct
beyond just stating that the locking isn't needed.

Thanks,
Andreas

> If those checks hit there is a bug and trying to recover from it makes
> no sense from my point of view.