[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