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

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

[teigland]

This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "Cluster Project".

http://sources.redhat.com/git/gitweb.cgi?p=cluster.git;a=commitdiff;h=a6b6a30358fd5e247a37e2fe493ef6a683174b66

The branch, STABLE2 has been updated
       via  a6b6a30358fd5e247a37e2fe493ef6a683174b66 (commit)
      from  edd597845e68e9826907ec1e23692e1fc394e9a4 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
commit a6b6a30358fd5e247a37e2fe493ef6a683174b66
Author: David Teigland <teigland@redhat.com>
Date:   Tue May 13 14:04:51 2008 -0500

    gfs_controld: ignore write(2) return value on plock dev
    
    bz 446128
    
    When plocks originate from nfs clients, the kernel mistakenly
    returns 0 instead of the number of bytes written to the plock
    device on write(2).  Don't spam /var/log/messages with errors
    reporting a bad return value from write(2).
    
    Signed-off-by: David Teigland <teigland@redhat.com>

-----------------------------------------------------------------------

Summary of changes:
 group/gfs_controld/plock.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/group/gfs_controld/plock.c b/group/gfs_controld/plock.c
index 42890df..beedf42 100644
--- a/group/gfs_controld/plock.c
+++ b/group/gfs_controld/plock.c
@@ -771,12 +771,8 @@ static int add_waiter(struct mountgroup *mg, struct resource *r,
 static void write_result(struct mountgroup *mg, struct gdlm_plock_info *in,
 			 int rv)
 {
-	int err;
-
 	in->rv = rv;
-	err = write(control_fd, in, sizeof(struct gdlm_plock_info));
-	if (err != sizeof(struct gdlm_plock_info))
-		log_error("plock result write err %d errno %d", err, errno);
+	write(control_fd, in, sizeof(struct gdlm_plock_info));
 }
 
 static void do_waiters(struct mountgroup *mg, struct resource *r)


hooks/post-receive
--
Cluster Project

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

[Steven Whitehouse]

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?

Steve.

On Tue, 2008-05-13 at 19:08 +0000, teigland at sourceware.org wrote:
> This is an automated email from the git hooks/post-receive script. It was
> generated because a ref change was pushed to the repository containing
> the project "Cluster Project".
> 
> http://sources.redhat.com/git/gitweb.cgi?p=cluster.git;a=commitdiff;h=a6b6a30358fd5e247a37e2fe493ef6a683174b66
> 
> The branch, STABLE2 has been updated
>        via  a6b6a30358fd5e247a37e2fe493ef6a683174b66 (commit)
>       from  edd597845e68e9826907ec1e23692e1fc394e9a4 (commit)
> 
> Those revisions listed above that are new to this repository have
> not appeared on any other notification email; so we list those
> revisions in full, below.
> 
> - Log -----------------------------------------------------------------
> commit a6b6a30358fd5e247a37e2fe493ef6a683174b66
> Author: David Teigland <teigland@redhat.com>
> Date:   Tue May 13 14:04:51 2008 -0500
> 
>     gfs_controld: ignore write(2) return value on plock dev
>     
>     bz 446128
>     
>     When plocks originate from nfs clients, the kernel mistakenly
>     returns 0 instead of the number of bytes written to the plock
>     device on write(2).  Don't spam /var/log/messages with errors
>     reporting a bad return value from write(2).
>     
>     Signed-off-by: David Teigland <teigland@redhat.com>
> 
> -----------------------------------------------------------------------
> 
> Summary of changes:
>  group/gfs_controld/plock.c |    6 +-----
>  1 files changed, 1 insertions(+), 5 deletions(-)
> 
> diff --git a/group/gfs_controld/plock.c b/group/gfs_controld/plock.c
> index 42890df..beedf42 100644
> --- a/group/gfs_controld/plock.c
> +++ b/group/gfs_controld/plock.c
> @@ -771,12 +771,8 @@ static int add_waiter(struct mountgroup *mg, struct resource *r,
>  static void write_result(struct mountgroup *mg, struct gdlm_plock_info *in,
>  			 int rv)
>  {
> -	int err;
> -
>  	in->rv = rv;
> -	err = write(control_fd, in, sizeof(struct gdlm_plock_info));
> -	if (err != sizeof(struct gdlm_plock_info))
> -		log_error("plock result write err %d errno %d", err, errno);
> +	write(control_fd, in, sizeof(struct gdlm_plock_info));
>  }
>  
>  static void do_waiters(struct mountgroup *mg, struct resource *r)
> 
> 
> hooks/post-receive
> --
> Cluster Project
> 

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

[David Teigland]

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.

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

[Steven Whitehouse]

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.

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. 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,

Steve.

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

[David Teigland]

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.