[Cluster-devel] [PATCH 1/5] NLM failover - nlm_unlock

[Cluster-devel] [PATCH 1/5] NLM failover - nlm_unlock

[Wendy Cheng]

By writing exported filesytem id into /proc/fs/nfsd/nlm_unlock, this
patch walks thru lockd's global nlm_files list to release all the locks
associated with the particular id. It is used to enable NFS lock
failover with active-active clustered servers.

Relevant steps:
1) Exports filesystem with "fsid" option as:
   /etc/exports entry> /mnt/ext3/exports *(fsid=1234,sync,rw)

2) Drops locks based on fsid by:
   shell> echo 1234 > /proc/fs/nfsd/nlm_unlock

Signed-off-by: S. Wendy Cheng <wcheng@redhat.com>
Signed-off-by: Lon Hohberger  <lhh@redhat.com>

 fs/lockd/svcsubs.c          |   79 ++++++++++++++++++++++++++++++++++++
+++++--- 
 fs/nfsd/nfsctl.c            |   41 ++++++++++++++++++++++
 include/linux/lockd/bind.h  |    5 ++
 include/linux/lockd/lockd.h |    2 +
 include/linux/nfsd/debug.h  |    1
 5 files changed, 123 insertions(+), 5 deletions(-)




-------------- next part --------------
A non-text attachment was scrubbed...
Name: gfs_nlm_unlock.patch
Type: text/x-patch
Size: 7370 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20060814/3f989c32/attachment.bin>

[Cluster-devel] Re: [NFS] [PATCH 1/5] NLM failover - nlm_unlock

[Greg Banks]

G'day Wendy,

Your fsid-based approach does seem to make dropping locks
less fiddly than using the virtual server address.  Some
minor nits...

On Mon, 2006-08-14 at 15:57, Wendy Cheng wrote:
> By writing exported filesytem id into /proc/fs/nfsd/nlm_unlock, this
> patch walks thru lockd's global nlm_files list to release all the locks
> associated with the particular id. It is used to enable NFS lock
> failover with active-active clustered servers.
> [...]
>  static int
> -nlm_traverse_files(struct nlm_host *host, int action)
> +nlm_traverse_files(struct nlm_host *host, int *fsid_p, int action)
> [...]
>  {
>  	struct nlm_file	*file, **fp;
> -	int		i;
> +	int		i, rc, fsid, act=action;
>  
>  	mutex_lock(&nlm_file_mutex);
> +	if (fsid_p) fsid = *fsid_p;

I don't see any point initialising fsid like this, as you
throw away that value before ever using it.  Initialising
to zero should be enough to stop the compiler complaining.

> +				dprintk("lockd: drop lock file=0x%x fsid=%d\n",
> +					(int) file, fsid);

`file' is a pointer, and will not be the same size as an int 
on 64bit machines.  This is why %p exists, see examples in
the existing NLM code.

> @@ -253,6 +307,8 @@ nlm_traverse_files(struct nlm_host *host
>  			/* No more references to this file. Let go of it. */
>  			if (!file->f_blocks && !file->f_locks
>  			 && !file->f_shares && !file->f_count) {
> +				dprintk("lockd: fo_unlock close file=0x%x\n", 
> +					(int) file);

Ditto.

> @@ -90,6 +103,7 @@ static ssize_t (*write_op[])(struct file
>  	[NFSD_Getfd] = write_getfd,
>  	[NFSD_Getfs] = write_getfs,
>  	[NFSD_Fh] = write_filehandle,
> +	[NFSD_Nlm_unlock] = do_nlm_fo_unlock,
>  	[NFSD_Threads] = write_threads,
>  	[NFSD_Versions] = write_versions,
>  #ifdef CONFIG_NFSD_V4

All the other entries in write_op[] have a consistent naming
scheme, being called write_foo().

> @@ -334,6 +348,32 @@ static ssize_t write_filehandle(struct f
>  	return mesg - buf;	
>  }
>  
> +static ssize_t do_nlm_fo_unlock(struct file *file, char *buf, size_t size)
> +{
> +	char *mesg = buf;
> +	int fsid, rc;
> +
> +	if (size <= 0) return -EINVAL;
> +
> +	/* convert string into a valid fsid */
> +	rc = get_int(&mesg, &fsid);
> +	if (rc) {
> +		dprintk("nfsd: do_nlm_ip_unlock invalid ip(%s)\n", buf);
> +		return rc;
> +	}

This dprintk seems to reflect the function's previous name
and semantics.

> +
> +	/* call nlm to release the locks */
> +	rc = nlmsvc_fo_unlock(&fsid);

I don't understand why you pass this parameter by reference?
It's not large and none of the functions called by nlmsvc_fo_unlock()
need to write to it.

>  #ifdef CONFIG_NFSD_V4
> --- linux-0/include/linux/nfsd/debug.h	2006-07-14 14:32:29.000000000 -0400
> +++ linux-1/include/linux/nfsd/debug.h	2006-08-11 10:12:29.000000000 -0400
> @@ -32,6 +32,7 @@
>  #define NFSDDBG_REPCACHE	0x0080
>  #define NFSDDBG_XDR		0x0100
>  #define NFSDDBG_LOCKD		0x0200
> +#define NFSDDBG_CLUSTER 	0x0400
>  #define NFSDDBG_ALL		0x7FFF
>  #define NFSDDBG_NOCHANGE	0xFFFF
>  

You don't seem to use this anywhere.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.

[Cluster-devel] Re: [NFS] [PATCH 1/5] NLM failover - nlm_unlock

[Wendy Cheng]

On Fri, 2006-08-18 at 18:23 +1000, Greg Banks wrote:
> G'day Wendy,
> 
> Your fsid-based approach does seem to make dropping locks
> less fiddly than using the virtual server address.  Some
> minor nits...

Thanks ! Another pair of eyes are always helpful. Will work on it -
ditto for Patch 5-2.

-- Wendy
> 
> On Mon, 2006-08-14 at 15:57, Wendy Cheng wrote:
> > By writing exported filesytem id into /proc/fs/nfsd/nlm_unlock, this
> > patch walks thru lockd's global nlm_files list to release all the locks
> > associated with the particular id. It is used to enable NFS lock
> > failover with active-active clustered servers.
> > [...]
> >  static int
> > -nlm_traverse_files(struct nlm_host *host, int action)
> > +nlm_traverse_files(struct nlm_host *host, int *fsid_p, int action)
> > [...]
> >  {
> >  	struct nlm_file	*file, **fp;
> > -	int		i;
> > +	int		i, rc, fsid, act=action;
> >  
> >  	mutex_lock(&nlm_file_mutex);
> > +	if (fsid_p) fsid = *fsid_p;
> 
> I don't see any point initialising fsid like this, as you
> throw away that value before ever using it.  Initialising
> to zero should be enough to stop the compiler complaining.
> 
> > +				dprintk("lockd: drop lock file=0x%x fsid=%d\n",
> > +					(int) file, fsid);
> 
> `file' is a pointer, and will not be the same size as an int 
> on 64bit machines.  This is why %p exists, see examples in
> the existing NLM code.
> 
> > @@ -253,6 +307,8 @@ nlm_traverse_files(struct nlm_host *host
> >  			/* No more references to this file. Let go of it. */
> >  			if (!file->f_blocks && !file->f_locks
> >  			 && !file->f_shares && !file->f_count) {
> > +				dprintk("lockd: fo_unlock close file=0x%x\n", 
> > +					(int) file);
> 
> Ditto.
> 
> > @@ -90,6 +103,7 @@ static ssize_t (*write_op[])(struct file
> >  	[NFSD_Getfd] = write_getfd,
> >  	[NFSD_Getfs] = write_getfs,
> >  	[NFSD_Fh] = write_filehandle,
> > +	[NFSD_Nlm_unlock] = do_nlm_fo_unlock,
> >  	[NFSD_Threads] = write_threads,
> >  	[NFSD_Versions] = write_versions,
> >  #ifdef CONFIG_NFSD_V4
> 
> All the other entries in write_op[] have a consistent naming
> scheme, being called write_foo().
> 
> > @@ -334,6 +348,32 @@ static ssize_t write_filehandle(struct f
> >  	return mesg - buf;	
> >  }
> >  
> > +static ssize_t do_nlm_fo_unlock(struct file *file, char *buf, size_t size)
> > +{
> > +	char *mesg = buf;
> > +	int fsid, rc;
> > +
> > +	if (size <= 0) return -EINVAL;
> > +
> > +	/* convert string into a valid fsid */
> > +	rc = get_int(&mesg, &fsid);
> > +	if (rc) {
> > +		dprintk("nfsd: do_nlm_ip_unlock invalid ip(%s)\n", buf);
> > +		return rc;
> > +	}
> 
> This dprintk seems to reflect the function's previous name
> and semantics.
> 
> > +
> > +	/* call nlm to release the locks */
> > +	rc = nlmsvc_fo_unlock(&fsid);
> 
> I don't understand why you pass this parameter by reference?
> It's not large and none of the functions called by nlmsvc_fo_unlock()
> need to write to it.
> 
> >  #ifdef CONFIG_NFSD_V4
> > --- linux-0/include/linux/nfsd/debug.h	2006-07-14 14:32:29.000000000 -0400
> > +++ linux-1/include/linux/nfsd/debug.h	2006-08-11 10:12:29.000000000 -0400
> > @@ -32,6 +32,7 @@
> >  #define NFSDDBG_REPCACHE	0x0080
> >  #define NFSDDBG_XDR		0x0100
> >  #define NFSDDBG_LOCKD		0x0200
> > +#define NFSDDBG_CLUSTER 	0x0400
> >  #define NFSDDBG_ALL		0x7FFF
> >  #define NFSDDBG_NOCHANGE	0xFFFF
> >  
> 
> You don't seem to use this anywhere.
> 
> Greg.