[Cluster-devel] [PATCH 1/4 Revised] NLM failover - nlm_unlock

[Cluster-devel] [PATCH 1/4 Revised] 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            |   42 +++++++++++++++++++++++
 include/linux/lockd/bind.h  |    5 ++
 include/linux/lockd/lockd.h |    2 +
 include/linux/nfsd/debug.h  |    1
 5 files changed, 124 insertions(+), 5 deletions(-)

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

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

[Neil Brown]

Firstly, sorry for the delay in responding to these.


On Thursday September 14, wcheng at redhat.com 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.
> 
> 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

I actually felt a bit more comfortable with the server-ip based
approach, how I cannot really fault the fsid based approach, and it
does seem to have some advantages, so I guess we go with it.

>  /*
> + * Get fsid from nfs_fh:
> + * return 1 if *fsid contains a valid value.
> + */
> +static inline int
> +nlm_fo_get_fsid(struct nfs_fh *fh, int *fsid)
> +{
> +	struct nfs_fhbase_new *fh_base = (struct nfs_fhbase_new *) fh->data;
> + 	int data_left = fh->size/4;
> + 
> + 	nlm_debug_print_fh("nlm_fo_find_fsid", fh);
> + 
> + 	/* From fb_version to fb_auth - at least two u32 */
> + 	if (data_left < 2)		
> + 		return 0;
> +  
> + 	/* For various types, check out 
> + 	 * inlcude/linux/nfsd/nfsfsh.h
> + 	 */
> + 	if ((fh_base->fb_version != 1) ||  
> + 		(fh_base->fb_auth_type != 0) ||
> + 		(fh_base->fb_fsid_type != 1))
> + 		return 0;
> + 
> + 	/* The fb_auth is 0 bytes long - imply fb_auth[0] has
> + 	 * fsid value.
> + 	 */
> + 	*fsid = (int) fh_base->fb_auth[0];
> + 	return 1;
> +}

It would be really nice if this could be in the nfsd code rather than
in the lockd code.  Maybe if the nlm_fopen call passed back the fsid?

> +
> +/*
>   * Operate on a single file
>   */
>  static inline int
> @@ -234,21 +267,42 @@ nlm_inspect_file(struct nlm_host *host, 
>   * Loop over all files in the file table.
>   */
>  static int
> -nlm_traverse_files(struct nlm_host *host, int action)
> +nlm_traverse_files(struct nlm_host *host, int *fsidp, int action)
>  {
>  	struct nlm_file	*file, **fp;
> -	int i, ret = 0;
> +	int i, ret = 0, found, fsid_in=0, fsid, act=action;

Unfortunately all of this will have to change due to Olaf's recent
changes in lockd.  Should be quite do-able, just needs to be
different.

> +/*
> + * release locks associated with an export fsid upon failover
> + */
> +int
> +nlmsvc_fo_unlock(int *fsid)
> +{
> +	/* drop the locks */
> +	return (nlm_traverse_files(NULL, fsid, NLM_ACT_FO_UNLOCK)); 
> +}
> +

I really don't think fsid should be an 'int *', just an int.
Other callers of nlm_traverse_files can just pass an fsid of 0 - they
don't need to be able to pass NULL.


NeilBrown

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

[Wendy Cheng]

Neil Brown wrote:
> On Thursday September 14, wcheng at redhat.com 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.
>>
>> 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
>>     
>
> I actually felt a bit more comfortable with the server-ip based
> approach, how I cannot really fault the fsid based approach, and it
> does seem to have some advantages, so I guess we go with it.
>   
Neil,

I replaced the checking inside nlm_traverse_files with nlm_file_inuse() 
as we discussed in:
http://sourceforge.net/mailarchive/forum.php?thread_id=31885384&forum_id=4930

If a separate patch is a better idea, feel free to yank it out. The code 
is based on 2.6.21.rc4 kernel and can be used independently (without 
other NLM failover patches). We submit it earlier (others still being 
worked on) to avoid the tedious rebase efforts. There are also  customer 
requests from our distribution to ask for this function in a single 
server (no cluster) environment.

-- Wendy

-------------- next part --------------
A non-text attachment was scrubbed...
Name: nlm_unlock.patch
Type: text/x-patch
Size: 8956 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20070323/c90f29b2/attachment.bin>

[Cluster-devel] Re: [NFS] [PATCH 2/4 Revised] NLM failover - nlm_set_igrace

[Neil Brown]

On Thursday September 14, wcheng at redhat.com wrote:
> This change enables per NFS-export entry lockd grace period. The 
> implementation is based on a double linked list fo_fsid_list that 
> contains entries of fsid info. It is expected this would not be a 
> frequent event. The fo_fsid_list is short and the entries expire within 
> a maximum of 50 seconds.  The grace period setting follows the existing 
> NLM grace period handling logic and is triggered via echoing the NFS 
> export filesystem id into nfsd procfs entry as:
> 
> shell> echo 1234 > /proc/fs/nfsd/nlm_set_igrace

What is the 'i' for?
How about  /proc/fs/nfsd/nlm_set_grace_for_fsid ??

> --- linux-1/include/linux/lockd/lockd.h	2006-09-03 21:51:41.000000000 -0400
> +++ linux-2/include/linux/lockd/lockd.h	2006-09-13 22:48:00.000000000 -0400
> @@ -107,6 +107,17 @@ struct nlm_file {
>  	int		       	f_hash;		/* hash of f_handle */
>  };
>  
> +#define NLM_FO_MAX_FSID_GP	127

Why do you need this limit?

> + *
> + * Also, please don't ask why using opencoded list manipulation, 
> + * instead of <linux/list.h>, unless you can point to me where
> + * in that file have existing macro and/or functions that can do
> + * single linked list. 

I don't think this comment is appropriate any more.  You are using
list.h lists, not open coding them.


> +	/* check to see whether this_fsid is in fo_fsid_list list */
> +	list_for_each_safe(p, tlist, &fo_fsid_list) {
> +		e_this = list_entry(p, struct fo_fsid, g_list);
> +		if (time_before(e_this->g_expire, jiffies)) {
> +			printk("lockd: fsid=%d grace period expires\n",
> +				e_this->g_fsid);
> +			list_del(p);
> +			fo_fsid_cnt--;
> +			kfree(e_this);
> +		} else if (e_this->g_fsid == this_fsid) {
> +			if (!e_this->g_flag) {
> +				e_this->g_flag = 1;
> +				printk("lockd: fsid=%d in grace period\n",
> +					e_this->g_fsid);
> +			}
> +			rc = 1;

I assume this 'g_flag' was just for debugging.  Can it be take out
now?
Also, I though you should break out of the loop once you set rc = 1.

>   * Obtain client and file from arguments
>   */
> @@ -89,7 +102,7 @@ nlm4svc_proc_test(struct svc_rqst *rqstp
>  	resp->cookie = argp->cookie;
>  
>  	/* Don't accept test requests during grace period */
> -	if (nlmsvc_grace_period) {
> +	if ((nlmsvc_grace_period) || (nlm4svc_fo_grace_period(argp))) {
>  		resp->status = nlm_lck_denied_grace_period;
>  		return rpc_success;
>  	}

I think this (and the rest) would look cleaner if you put the
'nlmsvc_grace_period' test into the nlm4svc_fo_grace_period function.

NeilBrown

[Cluster-devel] Re: [NFS] [PATCH 2/4 Revised] NLM failover - nlm_set_igrace

[Wendy Cheng]

Revised patch based on 2.6.21-rc4.

The change enables per NFS-export entry lockd grace period. A linked 
list, fo_fsid_list, contains entries of fsid info. This list is expected 
to be short and entries expire within a maximum of 50 seconds. It 
follows the existing NLM grace period handling logic and is triggered 
via echoing the NFS export filesystem id (fsid) into nfsd procfs entry as:

shell> echo 1234 > /proc/fs/nfsd/nlm_set_grace_for_fsid

-- Wendy

-------------- next part --------------
A non-text attachment was scrubbed...
Name: nlm_grace.patch
Type: text/x-patch
Size: 15229 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20070326/8b4c1701/attachment.bin>

[Cluster-devel] Re: [NFS] [PATCH 2/4 Revised] NLM failover - nlm_set_igrace

[J. Bruce Fields]

On Mon, Mar 26, 2007 at 06:21:01PM -0400, Wendy Cheng wrote:
> Revised patch based on 2.6.21-rc4.

Were there supposed to be 4 patches?  If so, where can I find the full
series?

--b.

[Cluster-devel] Re: [NFS] [PATCH 2/4 Revised] NLM failover - nlm_set_igrace

[Wendy Cheng]

J. Bruce Fields wrote:

>On Mon, Mar 26, 2007 at 06:21:01PM -0400, Wendy Cheng wrote:
>  
>
>>Revised patch based on 2.6.21-rc4.
>>    
>>
>
>Were there supposed to be 4 patches?  If so, where can I find the full
>series?
>
>--b.
>  
>

These are revised patches based on the discussion we had last September 
(shame on me to put this off for so long).

General description:
https://www.redhat.com/archives/cluster-devel/2006-September/msg00034.html
Patch 4-1 is based on:
https://www.redhat.com/archives/cluster-devel/2006-September/msg00035.html
new patch at:
https://www.redhat.com/archives/cluster-devel/2007-March/msg00218.html
Patch 4-2 is based on:
https://www.redhat.com/archives/cluster-devel/2006-September/msg00036.html
new patch at:
https://www.redhat.com/archives/cluster-devel/2007-March/msg00231.html
patch 4-3 original patch on:
https://www.redhat.com/archives/cluster-devel/2006-September/msg00037.html
For 2.6.21 kernel, this patch is no longer needed.
Patch 4.4 is nfs-utils (user mode) original patch on:
https://www.redhat.com/archives/cluster-devel/2006-September/msg00038.html
New patch: to be followed.

-- Wendy

[Cluster-devel] Re: [NFS] [PATCH 2/4 Revised] NLM failover - nlm_set_igrace

[J. Bruce Fields]

On Wed, Mar 28, 2007 at 12:32:40AM -0500, Wendy Cheng wrote:
> These are revised patches based on the discussion we had last September 

OK.  Would you remind just resending the revised versions of all of them
whenever they're ready?

It's just that applying patches off a web email archive is a bit of a
pain (there's all this & stuff to deal with, and who knows what it
does to the whitespace), while we all have piles of scripts to deal with
plain emailed patches....

Looks like interesting stuff.

--b.

[Cluster-devel] Re: [NFS] [PATCH 2/4 Revised] NLM failover - nlm_set_igrace

[Wendy Cheng]

J. Bruce Fields wrote:

>On Wed, Mar 28, 2007 at 12:32:40AM -0500, Wendy Cheng wrote:
>  
>
>>These are revised patches based on the discussion we had last September 
>>    
>>
>
>OK.  Would you remind just resending the revised versions of all of them
>whenever they're ready?
>
>It's just that applying patches off a web email archive is a bit of a
>pain (there's all this & stuff to deal with, and who knows what it
>does to the whitespace), while we all have piles of scripts to deal with
>plain emailed patches....
>
>Looks like interesting stuff.
>
>--b.
>  
>
Will do. We have one more patch (user space statd) to finish up - need 
to look into what has been changed in Neil's new nfs-util release. I 
could be his first tester (a scary thought :))..

-- Wendy

[Cluster-devel] Re: [NFS] [PATCH 3/4 Revised] NLM failover - statd changes

[Neil Brown]

On Thursday September 14, wcheng at redhat.com wrote:
> This kernel patch should be paired with nfs-utils user mode changes (in 
> patch 4-4 based on nfs-utils-1.0.8-rc4) that is optional. If changes 
> made in patch 4-4 is not presented in nfs-utils, the rpc.statd will 
> ignore whatever this kernel patch does.
> 
> The changes record the ip interface that accepts the lock requests and 
> passes the correct "my_name" (in standard IPV4 dot notation) to user 
> mode statd (instead of system_utsname.nodename). This enables rpc.statd 
> to add the correct taken-over IPv4 address into the 3rd parameter of 
> ha_callout program. Current nfs-utils always resets "my_name" into 
> loopback address (127.0.0.1), regardless the statement made in rpc.statd 
> man page. Check out "man rpc.statd" for details.
> 
> Signed-off-by: S. Wendy Cheng <wcheng@redhat.com>
> Signed-off-by: Lon Hohberger  <lhh@redhat.com>
> 
>  fs/lockd/host.c                |    4 ++--
>  fs/lockd/mon.c                 |   21 +++++++++++++++++----
>  fs/lockd/svc4proc.c            |    8 +++++++-
>  fs/lockd/svcproc.c             |    2 +-
>  include/linux/lockd/lockd.h    |    5 +++--
>  include/linux/lockd/sm_inter.h |    1 +
>  include/linux/sunrpc/svc.h     |    5 +++--
>  net/sunrpc/svcsock.c           |    5 +++++
>  8 files changed, 39 insertions(+), 12 deletions(-)
> 
> 
> --- linux-2/include/linux/sunrpc/svc.h	2006-09-11 16:52:34.000000000 -0400
> +++ linux-3/include/linux/sunrpc/svc.h	2006-09-13 14:07:32.000000000 -0400
> @@ -102,6 +102,7 @@ static inline void svc_putu32(struct kve
>  	iov->iov_len += sizeof(u32);
>  }
>  
> +typedef __u32  svc_addr_t;
>  	
>  /*
>   * The context of a single thread, including the request currently being
> @@ -138,8 +139,8 @@ struct svc_rqst {
>  	unsigned short
>  				rq_secure  : 1;	/* secure port */
>  
> -
> -	__u32			rq_daddr;	/* dest addr of request - reply from here */
> +	svc_addr_t		rq_daddr;	/* dest addr of request 
> +						   - reply from here */
>  
>  	void *			rq_argp;	/* decoded arguments */
>  	void *			rq_resp;	/* xdr'd results */
> --- linux-2/include/linux/lockd/lockd.h	2006-09-13 22:48:00.000000000 -0400
> +++ linux-3/include/linux/lockd/lockd.h	2006-09-13 22:51:41.000000000 -0400
> @@ -39,12 +39,13 @@
>  struct nlm_host {
>  	struct nlm_host *	h_next;		/* linked list (hash table) */
>  	struct sockaddr_in	h_addr;		/* peer address */
> +	svc_addr_t		h_server;	/* server ip for NLM failover */

Comment should say 'or 0 for client'.

>  	struct rpc_clnt	*	h_rpcclnt;	/* RPC client to talk to peer */
>  	char			h_name[20];	/* remote hostname */
>  	u32			h_version;	/* interface version */
>  	unsigned short		h_proto;	/* transport proto */
>  	unsigned short		h_reclaiming : 1,
> -				h_server     : 1, /* server side, not client side */
> +				h_notused    : 1, 

Just discard this, no need to rename it to 'h_notused'.

>  
> +#if 0
> +	if (argp->reclaim) {
> +		printk("nlm4svc_proc_lock lock reclaim received\n");
> +	}
> +#endif

Can this be removed?


So: mostly looks OK, but will need quite a bit of reworking do to
recent changes in lockd code.  So if you can get me a version that
applies to latest -mm, I'll take it from there.

Thanks,
NeilBrown

[Cluster-devel] Re: [NFS] [PATCH 3/4 Revised] NLM failover - statd changes

[Wendy Cheng]

Neil Brown wrote:

>So: mostly looks OK, but will need quite a bit of reworking do to
>recent changes in lockd code.  So if you can get me a version that
>applies to latest -mm, I'll take it from there.
>
>  
>
Thanks ! I'm tied with another issue at this moment but will get this 
done (and tested) sometime next week.

-- Wendy

[Cluster-devel] [PATCH 2/4 Revised] NLM failover - nlm_set_igrace

[Wendy Cheng]

This change enables per NFS-export entry lockd grace period. The 
implementation is based on a double linked list fo_fsid_list that 
contains entries of fsid info. It is expected this would not be a 
frequent event. The fo_fsid_list is short and the entries expire within 
a maximum of 50 seconds.  The grace period setting follows the existing 
NLM grace period handling logic and is triggered via echoing the NFS 
export filesystem id into nfsd procfs entry as:

shell> echo 1234 > /proc/fs/nfsd/nlm_set_igrace

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

 fs/lockd/svc.c              |    8 +
 fs/lockd/svc4proc.c         |   28 +++++-
 fs/lockd/svcproc.c          |   29 +++++--
 fs/lockd/svcsubs.c          |  180 
++++++++++++++++++++++++++++++++++++++++++++
 fs/nfsd/nfsctl.c            |   32 +++++++
 include/linux/lockd/bind.h  |    3
 include/linux/lockd/lockd.h |   14 +++
 7 files changed, 279 insertions(+), 15 deletions(-)

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

[Cluster-devel] [PATCH 3/4 Revised] NLM failover - statd changes

[Wendy Cheng]

This kernel patch should be paired with nfs-utils user mode changes (in 
patch 4-4 based on nfs-utils-1.0.8-rc4) that is optional. If changes 
made in patch 4-4 is not presented in nfs-utils, the rpc.statd will 
ignore whatever this kernel patch does.

The changes record the ip interface that accepts the lock requests and 
passes the correct "my_name" (in standard IPV4 dot notation) to user 
mode statd (instead of system_utsname.nodename). This enables rpc.statd 
to add the correct taken-over IPv4 address into the 3rd parameter of 
ha_callout program. Current nfs-utils always resets "my_name" into 
loopback address (127.0.0.1), regardless the statement made in rpc.statd 
man page. Check out "man rpc.statd" for details.

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

 fs/lockd/host.c                |    4 ++--
 fs/lockd/mon.c                 |   21 +++++++++++++++++----
 fs/lockd/svc4proc.c            |    8 +++++++-
 fs/lockd/svcproc.c             |    2 +-
 include/linux/lockd/lockd.h    |    5 +++--
 include/linux/lockd/sm_inter.h |    1 +
 include/linux/sunrpc/svc.h     |    5 +++--
 net/sunrpc/svcsock.c           |    5 +++++
 8 files changed, 39 insertions(+), 12 deletions(-)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: gfs_statd.patch
Type: text/x-patch
Size: 6714 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20060914/282cca28/attachment.bin>

[Cluster-devel] [PATCH 3/4 Revised] NLM failover - statd changes

[Wendy Cheng]

Just did a browsing on 2.6.21.rc4 statd code (also with few quick test 
runs to confirm). Look like this patch is no longer needed. I 
tentatively withdraw this patch. Will continue the test this week. A 
summary will be sent after all the testings are completed.

-- Wendy

[Cluster-devel] [PATCH 3/4 Revised] NLM failover - statd changes

[Wendy Cheng]

Wendy Cheng wrote:

> Just did a browsing on 2.6.21.rc4 statd code (also with few quick test 
> runs to confirm). Look like this patch is no longer needed. I 
> tentatively withdraw this patch. Will continue the test this week. A 
> summary will be sent after all the testings are completed.
>

Neil,

I was wrong. Unlike other patches that can be individually tested, this 
patch has to combine with all other changes (including user mode statd 
patch) so my code reading deceived me. I'm mostly done but just found a 
new bug few minutes ago (that I never notice before). It is pretty late 
for me now so I'll continue tomorrow. G'night !

-- Wendy

[Cluster-devel] [PATCH 3/4 Revised] NLM failover - statd changes

[Wendy Cheng]

Wendy Cheng wrote:

> Wendy Cheng wrote:
>
>> Just did a browsing on 2.6.21.rc4 statd code (also with few quick 
>> test runs to confirm). Look like this patch is no longer needed. I 
>> tentatively withdraw this patch. Will continue the test this week. A 
>> summary will be sent after all the testings are completed.
>>
>
> Neil,
>
> I was wrong. Unlike other patches that can be individually tested, 
> this patch has to combine with all other changes (including user mode 
> statd patch) so my code reading deceived me. I'm mostly done but just 
> found a new bug few minutes ago (that I never notice before). It is 
> pretty late for me now so I'll continue tomorrow.
>
Sorry to bug the list for this - it was intended to be a private mail 
but somehow cluster-devel got included... Wendy