_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
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.
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
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.
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs