[Cluster-devel] Cluster Project branch, STABLE2, updated. cluster-2.03.02-7-ga6b6a30

[David Teigland <teigland@redhat.com>]

On Wed, May 14, 2008 at 09:56:11AM +0100, Steven Whitehouse wrote:
> Hi,
> 
> On Tue, 2008-05-13 at 15:13 -0500, David Teigland wrote:
> > On Tue, May 13, 2008 at 09:01:13PM +0100, Steven Whitehouse wrote:
> > > Hi,
> > > 
> > > It might be a silly question, but this looks to me like trying to fix a
> > > kernel bug by adding a userland one. Why not simply update the kernel to
> > > return the correct value?
> > 
> > Yes, there's already a kernel fix in dlm.git, see
> >   "dlm: fix plock dev_write return value"
> > 
> > http://git.kernel.org/gitweb.cgi?p=linux/kernel/git/teigland/dlm.git;a=shortlog;h=next
> > 
> > Suppressing the message spamming is a good solution in the mean time, and
> > has a better chance of getting to customers before the kernel patch.
> > 
> I'm afraid you've still not convinced me on this one. Why not just check
> errno as well as the return value, then you can detect both "correct"
> instances and still report errors when they occur.

I really didn't find it worth the time... there are lots of highly
unlikely error conditions that are just not worth logging a message about.
When I'm modifying that code again I'll look at putting back a check.

> Also the kernel fix doesn't look quite right to me. Surely we should be
> reporting the error from dlm_plock_callback() if it occurs, rather than
> just ignoring it?
> 
> In dlm_plock_callback() the return value from "notify()" seems to be
> ignored in one case too.

Errors from notify() are the point where this whole scheme (plocks from
nfs) falls apart.  There's nothing that can be done to recover from an
error there, and the nfs people basically have to wait indefinitely for
the notify to make sure it never causes an error.  Logging the error is
the best we can do.

> I spotted this while looking at the code:
> 
> struct plock_xop {
>         struct plock_op xop;
>         void *callback;
>         void *fl;
>         void *file;
>         struct file_lock flc;
> };
> 
> and I can't see the need for void pointers here, why not just use the
> correct types? It looks like this code could do with some cleanup,

I don't know, it doesn't look like they need to be void.  Marc Eshel
<eshel@almaden.ibm.com> is the one to ask, he added the support for nfs
plocks.