[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