[Cluster-devel] [PATCH 1/3] NLM add resume procfs file

[Cluster-devel] [PATCH 1/3] NLM add resume procfs file

[Wendy Cheng]

Add a new nfsd procfs file "resume_ip" to enable per-ip base lockd grace 
period:

Example Usage:
root-shell> echo 10.1.1.64 > /proc/fs/nfsd/resume_ip

Only support IPV4 address for this patch submission.

-- Wendy

-------------- next part --------------
A non-text attachment was scrubbed...
Name: resume_001.patch
Type: text/x-patch
Size: 4357 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20080125/9266b25f/attachment.bin>

[Cluster-devel] Re: [PATCH 1/3] NLM add resume procfs file

[J. Bruce Fields]

On Fri, Jan 25, 2008 at 12:15:05AM -0500, Wendy Cheng wrote:
> Add a new nfsd procfs file "resume_ip" to enable per-ip base lockd grace  
> period:
>
> Example Usage:
> root-shell> echo 10.1.1.64 > /proc/fs/nfsd/resume_ip
>
> Only support IPV4 address for this patch submission.
>
> -- Wendy
>

> Add a new nfsd procfs file "resume_ip" to enable per-ip base lockd grace 
> period:
> 
> Example Usage:
> root-shell> echo 10.1.1.64 > /proc/fs/nfsd/resume_ip
> 
> Only support IPV4 address for this patch submission.
> 
> Signed-off-by: S. Wendy Cheng <wcheng@redhat.com>
> Signed-off-by: Lon Hohberger  <lhh@redhat.com>
> 
>  fs/lockd/svcsubs.c          |    9 +++++++++
>  fs/nfsd/nfsctl.c            |   43 ++++++++++++++++++++++++++++++++++++++-----
>  include/linux/lockd/lockd.h |    1 +
>  3 files changed, 48 insertions(+), 5 deletions(-)
> 
> --- linux-1/fs/nfsd/nfsctl.c	2008-01-22 10:36:24.000000000 -0500
> +++ linux-2/fs/nfsd/nfsctl.c	2008-01-24 17:03:12.000000000 -0500
> @@ -56,6 +56,7 @@ enum {
>  	NFSD_Fh,
>  	NFSD_FO_UnlockIP,
>  	NFSD_FO_UnlockFS,
> +	NFSD_FO_ResumeIP,
>  	NFSD_Threads,
>  	NFSD_Pool_Threads,
>  	NFSD_Versions,
> @@ -94,6 +95,7 @@ static ssize_t write_recoverydir(struct 
>  
>  static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size);
>  static ssize_t failover_unlock_fs(struct file *file, char *buf, size_t size);
> +static ssize_t failover_resume_ip(struct file *file, char *buf, size_t size);
>  
>  static ssize_t (*write_op[])(struct file *, char *, size_t) = {
>  	[NFSD_Svc] = write_svc,
> @@ -106,6 +108,7 @@ static ssize_t (*write_op[])(struct file
>  	[NFSD_Fh] = write_filehandle,
>  	[NFSD_FO_UnlockIP] = failover_unlock_ip,
>  	[NFSD_FO_UnlockFS] = failover_unlock_fs,
> +	[NFSD_FO_ResumeIP] = failover_resume_ip,
>  	[NFSD_Threads] = write_threads,
>  	[NFSD_Pool_Threads] = write_pool_threads,
>  	[NFSD_Versions] = write_versions,
> @@ -297,11 +300,13 @@ static ssize_t write_getfd(struct file *
>  	return err;
>  }
>  
> -static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size)
> +static int failover_parse_ip(struct file *file,
> +			     char *buf,
> +			     size_t size,
> +			     __be32 *server_ip)
>  {
> -	__be32 server_ip;
>  	char *fo_path, c;
> -	int b1, b2, b3, b4;
> +	int b1, b2, b3, b4, len;
>  
>  	/* sanity check */
>  	if (size == 0)
> @@ -315,13 +320,39 @@ static ssize_t failover_unlock_ip(struct
>  		return -EINVAL;
>  
>  	/* get ipv4 address */
> -	if (sscanf(fo_path, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c) != 4)
> +	len = sscanf(fo_path, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c);
> +	if (len != 4)
>  		return -EINVAL;
> -	server_ip = htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);
> +
> +	*server_ip = htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);
> +
> +	return len;
> +}
> +
> +static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size)
> +{
> +	__be32 server_ip;
> +	int rc;
> +
> +	rc = failover_parse_ip(file, buf, size, &server_ip);
> +	if (rc < 0)
> +		return rc;
>  
>  	return nlmsvc_failover_ip(server_ip);
>  }

Looks great, but it would fit more logically with the previous patch.
(If you know you're going to end up using this code in two places, may
as well write it that way from the start.)

>  
> +static ssize_t failover_resume_ip(struct file *file, char *buf, size_t size)
> +{
> +	__be32 server_ip;
> +	int len;
> +
> +	len = failover_parse_ip(file, buf, size, &server_ip);
> +	if (len < 0)
> +		return len;
> +
> +	return nlmsvc_failover_setgrace(&server_ip, len);

Does this really need to take a void *?  And the &server_ip makes it
look like server_ip may be modified, which is slightly confusing.

Maybe the next patches will remind me why it's being done this way....

> +}
> +
>  static ssize_t failover_unlock_fs(struct file *file, char *buf, size_t size)
>  {
>  	struct nameidata nd;
> @@ -711,6 +742,8 @@ static int nfsd_fill_super(struct super_
>  					&transaction_ops, S_IWUSR|S_IRUSR},
>  		[NFSD_FO_UnlockFS] = {"unlock_filesystem",
>  					&transaction_ops, S_IWUSR|S_IRUSR},
> +		[NFSD_FO_ResumeIP] = {"resume_ip",
> +					&transaction_ops, S_IWUSR|S_IRUSR},
>  		[NFSD_Fh] = {"filehandle", &transaction_ops, S_IWUSR|S_IRUSR},
>  		[NFSD_Threads] = {"threads", &transaction_ops, S_IWUSR|S_IRUSR},
>  		[NFSD_Pool_Threads] = {"pool_threads", &transaction_ops, S_IWUSR|S_IRUSR},
> --- linux-1/fs/lockd/svcsubs.c	2008-01-22 10:36:24.000000000 -0500
> +++ linux-2/fs/lockd/svcsubs.c	2008-01-22 11:45:44.000000000 -0500
> @@ -422,3 +422,12 @@ nlmsvc_failover_ip(__be32 server_addr)
>  			nlmsvc_failover_file_ok_ip);
>  }
>  EXPORT_SYMBOL_GPL(nlmsvc_failover_ip);
> +
> +int
> +nlmsvc_failover_setgrace(void *server_ip, int ip_size)
> +{
> +	/* implemented by resume_002.patch */
> +	return ENOSYS;

OK, terrifically trivial nit, but: that should be -ENOSYS?

--b.

> +}
> +EXPORT_SYMBOL_GPL(nlmsvc_failover_setgrace);
> +
> --- linux-1/include/linux/lockd/lockd.h	2008-01-22 10:36:24.000000000 -0500
> +++ linux-2/include/linux/lockd/lockd.h	2008-01-22 11:45:44.000000000 -0500
> @@ -220,6 +220,7 @@ void		  nlmsvc_invalidate_all(void);
>   */
>  int           nlmsvc_failover_path(struct nameidata *nd);
>  int           nlmsvc_failover_ip(__be32 server_addr);
> +int           nlmsvc_failover_setgrace(void *server_ip, int ip_size);
>  
>  static __inline__ struct inode *
>  nlmsvc_file_inode(struct nlm_file *file)

[Cluster-devel] Re: [PATCH 1/3] NLM add resume procfs file

[J. Bruce Fields]

On Mon, Jan 28, 2008 at 09:19:10PM -0500, bfields wrote:
> On Fri, Jan 25, 2008 at 12:15:05AM -0500, Wendy Cheng wrote:
> > +static ssize_t failover_resume_ip(struct file *file, char *buf, size_t size)
> > +{
> > +	__be32 server_ip;
> > +	int len;
> > +
> > +	len = failover_parse_ip(file, buf, size, &server_ip);
> > +	if (len < 0)
> > +		return len;
> > +
> > +	return nlmsvc_failover_setgrace(&server_ip, len);
> 
> Does this really need to take a void *?  And the &server_ip makes it
> look like server_ip may be modified, which is slightly confusing.
> 
> Maybe the next patches will remind me why it's being done this way....

After another look--no, I'm assuming this is just left over from
previous versions that allowed per-path grace-period setting as well?
So we should just pass some kind of integer instead of a void *.

--b.

> 
> > +}
> > +
> >  static ssize_t failover_unlock_fs(struct file *file, char *buf, size_t size)
> >  {
> >  	struct nameidata nd;
> > @@ -711,6 +742,8 @@ static int nfsd_fill_super(struct super_
> >  					&transaction_ops, S_IWUSR|S_IRUSR},
> >  		[NFSD_FO_UnlockFS] = {"unlock_filesystem",
> >  					&transaction_ops, S_IWUSR|S_IRUSR},
> > +		[NFSD_FO_ResumeIP] = {"resume_ip",
> > +					&transaction_ops, S_IWUSR|S_IRUSR},
> >  		[NFSD_Fh] = {"filehandle", &transaction_ops, S_IWUSR|S_IRUSR},
> >  		[NFSD_Threads] = {"threads", &transaction_ops, S_IWUSR|S_IRUSR},
> >  		[NFSD_Pool_Threads] = {"pool_threads", &transaction_ops, S_IWUSR|S_IRUSR},
> > --- linux-1/fs/lockd/svcsubs.c	2008-01-22 10:36:24.000000000 -0500
> > +++ linux-2/fs/lockd/svcsubs.c	2008-01-22 11:45:44.000000000 -0500
> > @@ -422,3 +422,12 @@ nlmsvc_failover_ip(__be32 server_addr)
> >  			nlmsvc_failover_file_ok_ip);
> >  }
> >  EXPORT_SYMBOL_GPL(nlmsvc_failover_ip);
> > +
> > +int
> > +nlmsvc_failover_setgrace(void *server_ip, int ip_size)
> > +{
> > +	/* implemented by resume_002.patch */
> > +	return ENOSYS;
> 
> OK, terrifically trivial nit, but: that should be -ENOSYS?
> 
> --b.
> 
> > +}
> > +EXPORT_SYMBOL_GPL(nlmsvc_failover_setgrace);
> > +
> > --- linux-1/include/linux/lockd/lockd.h	2008-01-22 10:36:24.000000000 -0500
> > +++ linux-2/include/linux/lockd/lockd.h	2008-01-22 11:45:44.000000000 -0500
> > @@ -220,6 +220,7 @@ void		  nlmsvc_invalidate_all(void);
> >   */
> >  int           nlmsvc_failover_path(struct nameidata *nd);
> >  int           nlmsvc_failover_ip(__be32 server_addr);
> > +int           nlmsvc_failover_setgrace(void *server_ip, int ip_size);
> >  
> >  static __inline__ struct inode *
> >  nlmsvc_file_inode(struct nlm_file *file)
> 

[Cluster-devel] Re: [PATCH 1/3] NLM add resume procfs file

[Wendy Cheng]

J. Bruce Fields wrote:
>> +	int rc;
>> +
>> +	rc = failover_parse_ip(file, buf, size, &server_ip);
>> +	if (rc < 0)
>> +		return rc;
>>  
>>  	return nlmsvc_failover_ip(server_ip);
>>  }
>>     
>
> Looks great, but it would fit more logically with the previous patch.
> (If you know you're going to end up using this code in two places, may
> as well write it that way from the start.)
>   

The original unlock patch did have a shared routine for this purpose. 
After review, its code structure got changed a little bit. Since the 
revised version has non-trivial amount of testing efforts behind it, I 
think it is better to do the change here, instead of the well-tested 
unlock patch.

On the other hand, I cut the resume patch into three pieces mostly for 
review purpose. Do you think it would be easier (for your git tree 
works) that I combine these three small patches into a big resume patch 
after all the review comments are incorporated into the code ?

-- Wendy

[Cluster-devel] Re: [PATCH 1/3] NLM add resume procfs file

[J. Bruce Fields]

On Wed, Jan 30, 2008 at 11:43:50AM -0500, Wendy Cheng wrote:
> J. Bruce Fields wrote:
>>> +	int rc;
>>> +
>>> +	rc = failover_parse_ip(file, buf, size, &server_ip);
>>> +	if (rc < 0)
>>> +		return rc;
>>>   	return nlmsvc_failover_ip(server_ip);
>>>  }
>>>     
>>
>> Looks great, but it would fit more logically with the previous patch.
>> (If you know you're going to end up using this code in two places, may
>> as well write it that way from the start.)
>>   
>
> The original unlock patch did have a shared routine for this purpose.  
> After review, its code structure got changed a little bit. Since the  
> revised version has non-trivial amount of testing efforts behind it, I  
> think it is better to do the change here, instead of the well-tested  
> unlock patch.
>
> On the other hand, I cut the resume patch into three pieces mostly for  
> review purpose. Do you think it would be easier (for your git tree  
> works) that I combine these three small patches into a big resume patch  
> after all the review comments are incorporated into the code ?

As long as they each compile and run without introducing any new bugs
(even temporarily) along the way, then I'll almost always prefer more
smaller patches to fewer larger ones.

--b.