[RFC][PATCH] nfs: support legacy NFS flock behavior via mount option

[RFC][PATCH] nfs: support legacy NFS flock behavior via mount option

[Suresh Jayaraman]

NFS clients since 2.6.12 support flock()locks by emulating the
BSD-style locks in terms of POSIX byte range locks. So the NFS client
does not allow to lock the same file using both flock() and fcntl
byte-range locks.

For some Windows applications which seem to use both share mode locks
(flock()) and fcntl byte range locks sequentially on the same file,
the locking is failing as the lock has already been acquired. i.e. the
flock mapped as posix locks collide with actual byte range locks from
the same process. The problem was observed on a setup with Windows
clients accessing Excel files on a Samba exported share which is
originally a NFS mount from a NetApp filer. Since kernels < 2.6.12 does
not support flock, what was working (as flock locks were local) in
older kernels is not working with newer kernels.

This could be seen as a bug in the implementation of the windows
application or a NFS client regression, but that is debatable.
In the spirit of not breaking existing setups, this patch adds mount
options "flock=local" that enables older flock behavior and
"flock=fcntl" that allows the current flock behavior.

Here's the test program used to test the locking behavior:

/*
 * nfs-lock: Simple program to lock a file using flock and then using fcntl
 * Used to Test flock behavior on NFS (to be run on NFS mounts)
 *      Usage: ./nfs-lock <file> 
 */
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <sys/file.h>

void usage()
{
        fprintf(stdout, "Usage: nfs-lock <file>\n");
        exit(1);
}

int main(int argc, char *argv[])
{
        char *file = argv[1];
        struct flock lock;
        int fd, rc;

        if (argc !=2)
                usage();

        fd = open(file, O_RDWR);
        if (fd < 0) {
                perror("open");
                return 1;
        }

        /* acquire flock */
        printf("nfs-lock: Trying to acquire flock lock\n");
        rc = flock(fd, LOCK_EX);
        if (rc)
                perror("flock");

        memset(&lock, 0, sizeof(lock));
        lock.l_type = F_WRLCK;
        printf("nfs-lock: Trying to acquire fcntl lock\n");
        rc = fcntl(fd, F_SETLK, &lock);
        if (rc) {
                perror("fcntl");
                return 1;
        }
        printf("nfs-lock: fcntl obtained successfully on the file\n");

        flock(fd, LOCK_UN);
        fcntl(fd, F_UNLCK, &lock);
        close(fd);

        return 0;
}

Cc: Neil Brown <neilb@suse.de>
Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
---
 fs/nfs/file.c             |    6 +++++-
 fs/nfs/super.c            |   33 +++++++++++++++++++++++++++++++++
 include/linux/nfs_mount.h |    2 ++
 3 files changed, 40 insertions(+), 1 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index eb51bd6..2384382 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -825,12 +825,16 @@ out_err:
  */
 static int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)
 {
+	struct inode *inode = filp->f_mapping->host;
+
 	dprintk("NFS: flock(%s/%s, t=%x, fl=%x)\n",
 			filp->f_path.dentry->d_parent->d_name.name,
 			filp->f_path.dentry->d_name.name,
 			fl->fl_type, fl->fl_flags);
 
-	if (!(fl->fl_flags & FL_FLOCK))
+	/* flock is considered local if mounted with "-o flock=local" */
+	if ((NFS_SERVER(inode)->flags & NFS_MOUNT_FLOCK_LOCAL) ||
+			(!(fl->fl_flags & FL_FLOCK)))
 		return -ENOLCK;
 
 	/* We're simulating flock() locks using posix locks on the server */
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index ec3966e..65c780d 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -100,6 +100,7 @@ enum {
 	Opt_addr, Opt_mountaddr, Opt_clientaddr,
 	Opt_lookupcache,
 	Opt_fscache_uniq,
+	Opt_flock,
 
 	/* Special mount options */
 	Opt_userspace, Opt_deprecated, Opt_sloppy,
@@ -171,6 +172,7 @@ static const match_table_t nfs_mount_option_tokens = {
 
 	{ Opt_lookupcache, "lookupcache=%s" },
 	{ Opt_fscache_uniq, "fsc=%s" },
+	{ Opt_flock, "flock=%s" },
 
 	{ Opt_err, NULL }
 };
@@ -236,6 +238,18 @@ static match_table_t nfs_lookupcache_tokens = {
 	{ Opt_lookupcache_err, NULL }
 };
 
+enum {
+	Opt_flock_local, Opt_flock_fcntl,
+	Opt_flock_err
+};
+
+static match_table_t nfs_flock_tokens = {
+	{ Opt_flock_local, "local" },
+	{ Opt_flock_fcntl, "fcntl" },
+
+	{ Opt_flock_err, NULL }
+};
+
 
 static void nfs_umount_begin(struct super_block *);
 static int  nfs_statfs(struct dentry *, struct kstatfs *);
@@ -1412,6 +1426,25 @@ static int nfs_parse_mount_options(char *raw,
 			mnt->fscache_uniq = string;
 			mnt->options |= NFS_OPTION_FSCACHE;
 			break;
+		case Opt_flock:
+			string = match_strdup(args);
+			if (string == NULL)
+				goto out_nomem;
+			token = match_token(string, nfs_flock_tokens, args);
+			kfree(string);
+			switch (token) {
+				case Opt_flock_local:
+					mnt->flags |= NFS_MOUNT_FLOCK_LOCAL;
+					break;
+				case Opt_flock_fcntl:
+					mnt->flags &= ~NFS_MOUNT_FLOCK_LOCAL;
+					break;
+				default:
+					dfprintk(MOUNT, "NFS:	invalid	"
+							"flock argument\n");
+					return 0;
+			};
+			break;
 
 		/*
 		 * Special options
diff --git a/include/linux/nfs_mount.h b/include/linux/nfs_mount.h
index 5d59ae8..ee08dae 100644
--- a/include/linux/nfs_mount.h
+++ b/include/linux/nfs_mount.h
@@ -71,4 +71,6 @@ struct nfs_mount_data {
 #define NFS_MOUNT_NORESVPORT		0x40000
 #define NFS_MOUNT_LEGACY_INTERFACE	0x80000
 
+#define NFS_MOUNT_FLOCK_LOCAL	0x100000
+
 #endif

Re: [RFC][PATCH] nfs: support legacy NFS flock behavior via mount option

[Jeff Layton]

On Mon, 06 Sep 2010 18:03:43 +0530
Suresh Jayaraman <sjayaraman@suse.de> wrote:

> NFS clients since 2.6.12 support flock()locks by emulating the
> BSD-style locks in terms of POSIX byte range locks. So the NFS client
> does not allow to lock the same file using both flock() and fcntl
> byte-range locks.
> 
> For some Windows applications which seem to use both share mode locks
> (flock()) and fcntl byte range locks sequentially on the same file,
> the locking is failing as the lock has already been acquired. i.e. the
> flock mapped as posix locks collide with actual byte range locks from
> the same process. The problem was observed on a setup with Windows
> clients accessing Excel files on a Samba exported share which is
> originally a NFS mount from a NetApp filer. Since kernels < 2.6.12 does
> not support flock, what was working (as flock locks were local) in
> older kernels is not working with newer kernels.
> 
> This could be seen as a bug in the implementation of the windows
> application or a NFS client regression, but that is debatable.
> In the spirit of not breaking existing setups, this patch adds mount
> options "flock=local" that enables older flock behavior and
> "flock=fcntl" that allows the current flock behavior.
> 
> Here's the test program used to test the locking behavior:
> 
> /*
>  * nfs-lock: Simple program to lock a file using flock and then using fcntl
>  * Used to Test flock behavior on NFS (to be run on NFS mounts)
>  *      Usage: ./nfs-lock <file> 
>  */
> #include <stdio.h>
> #include <string.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <errno.h>
> #include <sys/file.h>
> 
> void usage()
> {
>         fprintf(stdout, "Usage: nfs-lock <file>\n");
>         exit(1);
> }
> 
> int main(int argc, char *argv[])
> {
>         char *file = argv[1];
>         struct flock lock;
>         int fd, rc;
> 
>         if (argc !=2)
>                 usage();
> 
>         fd = open(file, O_RDWR);
>         if (fd < 0) {
>                 perror("open");
>                 return 1;
>         }
> 
>         /* acquire flock */
>         printf("nfs-lock: Trying to acquire flock lock\n");
>         rc = flock(fd, LOCK_EX);
>         if (rc)
>                 perror("flock");
> 
>         memset(&lock, 0, sizeof(lock));
>         lock.l_type = F_WRLCK;
>         printf("nfs-lock: Trying to acquire fcntl lock\n");
>         rc = fcntl(fd, F_SETLK, &lock);
>         if (rc) {
>                 perror("fcntl");
>                 return 1;
>         }
>         printf("nfs-lock: fcntl obtained successfully on the file\n");
> 
>         flock(fd, LOCK_UN);
>         fcntl(fd, F_UNLCK, &lock);
>         close(fd);
> 
>         return 0;
> }
> 
> Cc: Neil Brown <neilb@suse.de>
> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
> ---
>  fs/nfs/file.c             |    6 +++++-
>  fs/nfs/super.c            |   33 +++++++++++++++++++++++++++++++++
>  include/linux/nfs_mount.h |    2 ++
>  3 files changed, 40 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index eb51bd6..2384382 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -825,12 +825,16 @@ out_err:
>   */
>  static int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)
>  {
> +	struct inode *inode = filp->f_mapping->host;
> +
>  	dprintk("NFS: flock(%s/%s, t=%x, fl=%x)\n",
>  			filp->f_path.dentry->d_parent->d_name.name,
>  			filp->f_path.dentry->d_name.name,
>  			fl->fl_type, fl->fl_flags);
>  
> -	if (!(fl->fl_flags & FL_FLOCK))
> +	/* flock is considered local if mounted with "-o flock=local" */
> +	if ((NFS_SERVER(inode)->flags & NFS_MOUNT_FLOCK_LOCAL) ||
> +			(!(fl->fl_flags & FL_FLOCK)))
>  		return -ENOLCK;
>  
>  	/* We're simulating flock() locks using posix locks on the server */
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index ec3966e..65c780d 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -100,6 +100,7 @@ enum {
>  	Opt_addr, Opt_mountaddr, Opt_clientaddr,
>  	Opt_lookupcache,
>  	Opt_fscache_uniq,
> +	Opt_flock,
>  
>  	/* Special mount options */
>  	Opt_userspace, Opt_deprecated, Opt_sloppy,
> @@ -171,6 +172,7 @@ static const match_table_t nfs_mount_option_tokens = {
>  
>  	{ Opt_lookupcache, "lookupcache=%s" },
>  	{ Opt_fscache_uniq, "fsc=%s" },
> +	{ Opt_flock, "flock=%s" },
>  
>  	{ Opt_err, NULL }
>  };
> @@ -236,6 +238,18 @@ static match_table_t nfs_lookupcache_tokens = {
>  	{ Opt_lookupcache_err, NULL }
>  };
>  
> +enum {
> +	Opt_flock_local, Opt_flock_fcntl,
> +	Opt_flock_err
> +};
> +
> +static match_table_t nfs_flock_tokens = {
> +	{ Opt_flock_local, "local" },
> +	{ Opt_flock_fcntl, "fcntl" },
> +
> +	{ Opt_flock_err, NULL }
> +};
> +
>  
>  static void nfs_umount_begin(struct super_block *);
>  static int  nfs_statfs(struct dentry *, struct kstatfs *);
> @@ -1412,6 +1426,25 @@ static int nfs_parse_mount_options(char *raw,
>  			mnt->fscache_uniq = string;
>  			mnt->options |= NFS_OPTION_FSCACHE;
>  			break;
> +		case Opt_flock:
> +			string = match_strdup(args);
> +			if (string == NULL)
> +				goto out_nomem;
> +			token = match_token(string, nfs_flock_tokens, args);
> +			kfree(string);
> +			switch (token) {
> +				case Opt_flock_local:
> +					mnt->flags |= NFS_MOUNT_FLOCK_LOCAL;
> +					break;
> +				case Opt_flock_fcntl:
> +					mnt->flags &= ~NFS_MOUNT_FLOCK_LOCAL;
> +					break;
> +				default:
> +					dfprintk(MOUNT, "NFS:	invalid	"
> +							"flock argument\n");
> +					return 0;
> +			};
> +			break;
>  
>  		/*
>  		 * Special options
> diff --git a/include/linux/nfs_mount.h b/include/linux/nfs_mount.h
> index 5d59ae8..ee08dae 100644
> --- a/include/linux/nfs_mount.h
> +++ b/include/linux/nfs_mount.h
> @@ -71,4 +71,6 @@ struct nfs_mount_data {
>  #define NFS_MOUNT_NORESVPORT		0x40000
>  #define NFS_MOUNT_LEGACY_INTERFACE	0x80000
>  
> +#define NFS_MOUNT_FLOCK_LOCAL	0x100000
> +
>  #endif
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

I like the overall concept of this patch. I too think flock emulation
needs to be tunable since it's not really part of the NFS spec and it
can be problematic in some cases.

I had considered doing something similar a while back with a module
option. This seems better though since it's more granular.

Acked-by: Jeff Layton <jlayton@redhat.com>

Re: [RFC][PATCH] nfs: support legacy NFS flock behavior via mount option

[Trond Myklebust]

On Mon, 2010-09-06 at 18:03 +0530, Suresh Jayaraman wrote:
> NFS clients since 2.6.12 support flock()locks by emulating the
> BSD-style locks in terms of POSIX byte range locks. So the NFS client
> does not allow to lock the same file using both flock() and fcntl
> byte-range locks.
> 
> For some Windows applications which seem to use both share mode locks
> (flock()) and fcntl byte range locks sequentially on the same file,
> the locking is failing as the lock has already been acquired. i.e. the
> flock mapped as posix locks collide with actual byte range locks from
> the same process. The problem was observed on a setup with Windows
> clients accessing Excel files on a Samba exported share which is
> originally a NFS mount from a NetApp filer. Since kernels < 2.6.12 does
> not support flock, what was working (as flock locks were local) in
> older kernels is not working with newer kernels.
> 
> This could be seen as a bug in the implementation of the windows
> application or a NFS client regression, but that is debatable.
> In the spirit of not breaking existing setups, this patch adds mount
> options "flock=local" that enables older flock behavior and
> "flock=fcntl" that allows the current flock behavior.

So instead of having a special option for flock only, what say we rather
introduce an option of the form

  -olocal_lock=

which can take the values 'none', 'flock', 'fcntl' (or 'posix'?) and
'all'?

Cheers
  Trond

Re: [RFC][PATCH] nfs: support legacy NFS flock behavior via mount option

[Jeff Layton]

On Tue, 07 Sep 2010 10:17:19 -0400
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> On Mon, 2010-09-06 at 18:03 +0530, Suresh Jayaraman wrote:
> > NFS clients since 2.6.12 support flock()locks by emulating the
> > BSD-style locks in terms of POSIX byte range locks. So the NFS client
> > does not allow to lock the same file using both flock() and fcntl
> > byte-range locks.
> > 
> > For some Windows applications which seem to use both share mode locks
> > (flock()) and fcntl byte range locks sequentially on the same file,
> > the locking is failing as the lock has already been acquired. i.e. the
> > flock mapped as posix locks collide with actual byte range locks from
> > the same process. The problem was observed on a setup with Windows
> > clients accessing Excel files on a Samba exported share which is
> > originally a NFS mount from a NetApp filer. Since kernels < 2.6.12 does
> > not support flock, what was working (as flock locks were local) in
> > older kernels is not working with newer kernels.
> > 
> > This could be seen as a bug in the implementation of the windows
> > application or a NFS client regression, but that is debatable.
> > In the spirit of not breaking existing setups, this patch adds mount
> > options "flock=local" that enables older flock behavior and
> > "flock=fcntl" that allows the current flock behavior.
> 
> So instead of having a special option for flock only, what say we rather
> introduce an option of the form
> 
>   -olocal_lock=
> 
> which can take the values 'none', 'flock', 'fcntl' (or 'posix'?) and
> 'all'?
> 
> Cheers
>   Trond

Another thought -- we already have "-olock" and "-onolock" and we'll
have to keep them for compatability. Maybe this should be "-oflock" and
"-onoflock"?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [RFC][PATCH] nfs: support legacy NFS flock behavior via mount option

[Trond Myklebust]

On Tue, 2010-09-07 at 12:08 -0400, Jeff Layton wrote:
> On Tue, 07 Sep 2010 10:17:19 -0400
> Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> 
> > On Mon, 2010-09-06 at 18:03 +0530, Suresh Jayaraman wrote:
> > > NFS clients since 2.6.12 support flock()locks by emulating the
> > > BSD-style locks in terms of POSIX byte range locks. So the NFS client
> > > does not allow to lock the same file using both flock() and fcntl
> > > byte-range locks.
> > > 
> > > For some Windows applications which seem to use both share mode locks
> > > (flock()) and fcntl byte range locks sequentially on the same file,
> > > the locking is failing as the lock has already been acquired. i.e. the
> > > flock mapped as posix locks collide with actual byte range locks from
> > > the same process. The problem was observed on a setup with Windows
> > > clients accessing Excel files on a Samba exported share which is
> > > originally a NFS mount from a NetApp filer. Since kernels < 2.6.12 does
> > > not support flock, what was working (as flock locks were local) in
> > > older kernels is not working with newer kernels.
> > > 
> > > This could be seen as a bug in the implementation of the windows
> > > application or a NFS client regression, but that is debatable.
> > > In the spirit of not breaking existing setups, this patch adds mount
> > > options "flock=local" that enables older flock behavior and
> > > "flock=fcntl" that allows the current flock behavior.
> > 
> > So instead of having a special option for flock only, what say we rather
> > introduce an option of the form
> > 
> >   -olocal_lock=
> > 
> > which can take the values 'none', 'flock', 'fcntl' (or 'posix'?) and
> > 'all'?
> > 
> > Cheers
> >   Trond
> 
> Another thought -- we already have "-olock" and "-onolock" and we'll
> have to keep them for compatability. Maybe this should be "-oflock" and
> "-onoflock"?
> 

No thanks. That would require us to add a -onofcntl too, since -onolock
is already defines as being equivalent to -olocal_lock=all

Trond

Re: [RFC][PATCH] nfs: support legacy NFS flock behavior via mount option

[Suresh Jayaraman]

On 09/07/2010 07:47 PM, Trond Myklebust wrote:
> On Mon, 2010-09-06 at 18:03 +0530, Suresh Jayaraman wrote:
>> NFS clients since 2.6.12 support flock()locks by emulating the
>> BSD-style locks in terms of POSIX byte range locks. So the NFS client
>> does not allow to lock the same file using both flock() and fcntl
>> byte-range locks.
>>
>> For some Windows applications which seem to use both share mode locks
>> (flock()) and fcntl byte range locks sequentially on the same file,
>> the locking is failing as the lock has already been acquired. i.e. the
>> flock mapped as posix locks collide with actual byte range locks from
>> the same process. The problem was observed on a setup with Windows
>> clients accessing Excel files on a Samba exported share which is
>> originally a NFS mount from a NetApp filer. Since kernels < 2.6.12 does
>> not support flock, what was working (as flock locks were local) in
>> older kernels is not working with newer kernels.
>>
>> This could be seen as a bug in the implementation of the windows
>> application or a NFS client regression, but that is debatable.
>> In the spirit of not breaking existing setups, this patch adds mount
>> options "flock=local" that enables older flock behavior and
>> "flock=fcntl" that allows the current flock behavior.
> 
> So instead of having a special option for flock only, what say we rather
> introduce an option of the form
> 
>   -olocal_lock=
> 
> which can take the values 'none', 'flock', 'fcntl' (or 'posix'?) and
> 'all'?
> 

Sounds good. I just figured out Solaris and HPUX (perhaps few other
Unixes too) already support "llock" mount option (short form of local
lock) with similar semantics. So, I think it would make sense for us to
adapt the same. But, if you think "local_lock" is better, please do a
s/llock/local_lock on the below patch.

---
From: Suresh Jayaraman <sjayaraman@suse.de>
Subject: [PATCH] nfs: introduce mount option 'llock' to make locks local

NFS clients since 2.6.12 support flock locks by emulating fcntl byte-range
locks. Due to this, some windows applications which seem to use both flock and
fcntl locks sequentially on the same file, can't lock as they falsely assume
the file is already locked. The problem was reported on a setup with windows
clients accessing excel files on a Samba exported share which is originally a
NFS mount from a NetApp filer.

Older NFS clients (< 2.6.12) did not see this problem as flock locks were
considered local. To support legacy flock behavior, this patch adds a mount
option "-ollock=" which can take the following values:

   'none'  		- Neither of flock locks and fcntl/posix locks are local
   'flock'/'posix' 	- flock locks are local
   'fcntl' 		- fcntl locks are local
   'all'		- Both flock locks and fcntl/posix locks are local


Cc: Neil Brown <neilb@suse.de>
Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
---
 fs/nfs/file.c             |   15 ++++++++++++---
 fs/nfs/super.c            |   44 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/nfs_mount.h |    3 +++
 3 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index eb51bd6..a13a83e 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -800,8 +800,13 @@ static int nfs_lock(struct file *filp, int cmd, struct file_lock *fl)
 
 	nfs_inc_stats(inode, NFSIOS_VFSLOCK);
 
-	/* No mandatory locks over NFS */
-	if (__mandatory_lock(inode) && fl->fl_type != F_UNLCK)
+	/*
+	 * No mandatory locks over NFS.
+	 * fcntl lock is local if mounted with "-o llock=fcntl" or
+	 * "-o llock=posix"
+	 */
+	if ((__mandatory_lock(inode) && fl->fl_type != F_UNLCK) ||
+			(NFS_SERVER(inode)->flags & NFS_MOUNT_LOCAL_FCNTL))
 		goto out_err;
 
 	if (NFS_PROTO(inode)->lock_check_bounds != NULL) {
@@ -825,12 +830,16 @@ out_err:
  */
 static int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)
 {
+	struct inode *inode = filp->f_mapping->host;
+
 	dprintk("NFS: flock(%s/%s, t=%x, fl=%x)\n",
 			filp->f_path.dentry->d_parent->d_name.name,
 			filp->f_path.dentry->d_name.name,
 			fl->fl_type, fl->fl_flags);
 
-	if (!(fl->fl_flags & FL_FLOCK))
+	/* flock is local if mounted with "-o llock=flock" */
+	if ((NFS_SERVER(inode)->flags & NFS_MOUNT_LOCAL_FLOCK) ||
+			(!(fl->fl_flags & FL_FLOCK)))
 		return -ENOLCK;
 
 	/* We're simulating flock() locks using posix locks on the server */
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index ec3966e..7769dd3 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -100,6 +100,7 @@ enum {
 	Opt_addr, Opt_mountaddr, Opt_clientaddr,
 	Opt_lookupcache,
 	Opt_fscache_uniq,
+	Opt_llock,
 
 	/* Special mount options */
 	Opt_userspace, Opt_deprecated, Opt_sloppy,
@@ -171,6 +172,7 @@ static const match_table_t nfs_mount_option_tokens = {
 
 	{ Opt_lookupcache, "lookupcache=%s" },
 	{ Opt_fscache_uniq, "fsc=%s" },
+	{ Opt_llock, "llock=%s" },
 
 	{ Opt_err, NULL }
 };
@@ -236,6 +238,21 @@ static match_table_t nfs_lookupcache_tokens = {
 	{ Opt_lookupcache_err, NULL }
 };
 
+enum {
+	Opt_llock_all, Opt_llock_flock, Opt_llock_fcntl, Opt_llock_none,
+	Opt_llock_err
+};
+
+static match_table_t nfs_llock_tokens = {
+	{ Opt_llock_all, "all" },
+	{ Opt_llock_flock, "flock" },
+	{ Opt_llock_fcntl, "fcntl" },
+	{ Opt_llock_fcntl, "posix" },
+	{ Opt_llock_none, "none" },
+
+	{ Opt_llock_err, NULL }
+};
+
 
 static void nfs_umount_begin(struct super_block *);
 static int  nfs_statfs(struct dentry *, struct kstatfs *);
@@ -1412,6 +1429,33 @@ static int nfs_parse_mount_options(char *raw,
 			mnt->fscache_uniq = string;
 			mnt->options |= NFS_OPTION_FSCACHE;
 			break;
+		case Opt_llock:
+			string = match_strdup(args);
+			if (string == NULL)
+				goto out_nomem;
+			token = match_token(string, nfs_llock_tokens, args);
+			kfree(string);
+			switch (token) {
+			case Opt_llock_all:
+				mnt->flags |= (NFS_MOUNT_LOCAL_FLOCK |
+					       NFS_MOUNT_LOCAL_FCNTL);
+				break;
+			case Opt_llock_flock:
+				mnt->flags |= NFS_MOUNT_LOCAL_FLOCK;
+				break;
+			case Opt_llock_fcntl:
+				mnt->flags |= NFS_MOUNT_LOCAL_FCNTL;
+				break;
+			case Opt_llock_none:
+				mnt->flags &= ~(NFS_MOUNT_LOCAL_FLOCK |
+						NFS_MOUNT_LOCAL_FCNTL);
+				break;
+			default:
+				dfprintk(MOUNT, "NFS:	invalid	"
+						"llock argument\n");
+				return 0;
+			};
+			break;
 
 		/*
 		 * Special options
diff --git a/include/linux/nfs_mount.h b/include/linux/nfs_mount.h
index 5d59ae8..576bddd 100644
--- a/include/linux/nfs_mount.h
+++ b/include/linux/nfs_mount.h
@@ -71,4 +71,7 @@ struct nfs_mount_data {
 #define NFS_MOUNT_NORESVPORT		0x40000
 #define NFS_MOUNT_LEGACY_INTERFACE	0x80000
 
+#define NFS_MOUNT_LOCAL_FLOCK	0x100000
+#define NFS_MOUNT_LOCAL_FCNTL	0x200000
+
 #endif

Re: [RFC][PATCH] nfs: support legacy NFS flock behavior via mount option

[Trond Myklebust]

On Wed, 2010-09-08 at 01:43 +0530, Suresh Jayaraman wrote:
> On 09/07/2010 07:47 PM, Trond Myklebust wrote:
> > On Mon, 2010-09-06 at 18:03 +0530, Suresh Jayaraman wrote:
> >> NFS clients since 2.6.12 support flock()locks by emulating the
> >> BSD-style locks in terms of POSIX byte range locks. So the NFS client
> >> does not allow to lock the same file using both flock() and fcntl
> >> byte-range locks.
> >>
> >> For some Windows applications which seem to use both share mode locks
> >> (flock()) and fcntl byte range locks sequentially on the same file,
> >> the locking is failing as the lock has already been acquired. i.e. the
> >> flock mapped as posix locks collide with actual byte range locks from
> >> the same process. The problem was observed on a setup with Windows
> >> clients accessing Excel files on a Samba exported share which is
> >> originally a NFS mount from a NetApp filer. Since kernels < 2.6.12 does
> >> not support flock, what was working (as flock locks were local) in
> >> older kernels is not working with newer kernels.
> >>
> >> This could be seen as a bug in the implementation of the windows
> >> application or a NFS client regression, but that is debatable.
> >> In the spirit of not breaking existing setups, this patch adds mount
> >> options "flock=local" that enables older flock behavior and
> >> "flock=fcntl" that allows the current flock behavior.
> > 
> > So instead of having a special option for flock only, what say we rather
> > introduce an option of the form
> > 
> >   -olocal_lock=
> > 
> > which can take the values 'none', 'flock', 'fcntl' (or 'posix'?) and
> > 'all'?
> > 
> 
> Sounds good. I just figured out Solaris and HPUX (perhaps few other
> Unixes too) already support "llock" mount option (short form of local
> lock) with similar semantics. So, I think it would make sense for us to
> adapt the same. But, if you think "local_lock" is better, please do a
> s/llock/local_lock on the below patch.
> 
> ---
> From: Suresh Jayaraman <sjayaraman@suse.de>
> Subject: [PATCH] nfs: introduce mount option 'llock' to make locks local
> 
> NFS clients since 2.6.12 support flock locks by emulating fcntl byte-range
> locks. Due to this, some windows applications which seem to use both flock and
> fcntl locks sequentially on the same file, can't lock as they falsely assume
> the file is already locked. The problem was reported on a setup with windows
> clients accessing excel files on a Samba exported share which is originally a
> NFS mount from a NetApp filer.
> 
> Older NFS clients (< 2.6.12) did not see this problem as flock locks were
> considered local. To support legacy flock behavior, this patch adds a mount
> option "-ollock=" which can take the following values:
> 
>    'none'  		- Neither of flock locks and fcntl/posix locks are local
>    'flock'/'posix' 	- flock locks are local
>    'fcntl' 		- fcntl locks are local
>    'all'		- Both flock locks and fcntl/posix locks are local

So, the main reason I had for not suggesting llock was because on
Solaris, HPUX, AIX etc, llock is the same as our nolock, and doesn't
really take an argument.
IOW: I'm worried about introducing a syntax that is deliberately
confusing to people who are used to administrating non-Linux systems.

> Cc: Neil Brown <neilb@suse.de>
> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
> ---
>  fs/nfs/file.c             |   15 ++++++++++++---
>  fs/nfs/super.c            |   44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/nfs_mount.h |    3 +++
>  3 files changed, 59 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index eb51bd6..a13a83e 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -800,8 +800,13 @@ static int nfs_lock(struct file *filp, int cmd, struct file_lock *fl)
>  
>  	nfs_inc_stats(inode, NFSIOS_VFSLOCK);
>  
> -	/* No mandatory locks over NFS */
> -	if (__mandatory_lock(inode) && fl->fl_type != F_UNLCK)
> +	/*
> +	 * No mandatory locks over NFS.
> +	 * fcntl lock is local if mounted with "-o llock=fcntl" or
> +	 * "-o llock=posix"
> +	 */
> +	if ((__mandatory_lock(inode) && fl->fl_type != F_UNLCK) ||
> +			(NFS_SERVER(inode)->flags & NFS_MOUNT_LOCAL_FCNTL))
>  		goto out_err;
>  
>  	if (NFS_PROTO(inode)->lock_check_bounds != NULL) {
> @@ -825,12 +830,16 @@ out_err:
>   */
>  static int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)
>  {
> +	struct inode *inode = filp->f_mapping->host;
> +
>  	dprintk("NFS: flock(%s/%s, t=%x, fl=%x)\n",
>  			filp->f_path.dentry->d_parent->d_name.name,
>  			filp->f_path.dentry->d_name.name,
>  			fl->fl_type, fl->fl_flags);
>  
> -	if (!(fl->fl_flags & FL_FLOCK))
> +	/* flock is local if mounted with "-o llock=flock" */
> +	if ((NFS_SERVER(inode)->flags & NFS_MOUNT_LOCAL_FLOCK) ||
> +			(!(fl->fl_flags & FL_FLOCK)))
>  		return -ENOLCK;

Is this really what we want to do? Shouldn't we rather default to
treating this as if NFS_MOUNT_NONLM were set?

Also, don't we need similar code in the fcntl cases?

>  	/* We're simulating flock() locks using posix locks on the server */
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index ec3966e..7769dd3 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -100,6 +100,7 @@ enum {
>  	Opt_addr, Opt_mountaddr, Opt_clientaddr,
>  	Opt_lookupcache,
>  	Opt_fscache_uniq,
> +	Opt_llock,
>  
>  	/* Special mount options */
>  	Opt_userspace, Opt_deprecated, Opt_sloppy,
> @@ -171,6 +172,7 @@ static const match_table_t nfs_mount_option_tokens = {
>  
>  	{ Opt_lookupcache, "lookupcache=%s" },
>  	{ Opt_fscache_uniq, "fsc=%s" },
> +	{ Opt_llock, "llock=%s" },
>  
>  	{ Opt_err, NULL }
>  };
> @@ -236,6 +238,21 @@ static match_table_t nfs_lookupcache_tokens = {
>  	{ Opt_lookupcache_err, NULL }
>  };
>  
> +enum {
> +	Opt_llock_all, Opt_llock_flock, Opt_llock_fcntl, Opt_llock_none,
> +	Opt_llock_err
> +};
> +
> +static match_table_t nfs_llock_tokens = {
> +	{ Opt_llock_all, "all" },
> +	{ Opt_llock_flock, "flock" },
> +	{ Opt_llock_fcntl, "fcntl" },
> +	{ Opt_llock_fcntl, "posix" },
> +	{ Opt_llock_none, "none" },
> +
> +	{ Opt_llock_err, NULL }
> +};
> +
>  
>  static void nfs_umount_begin(struct super_block *);
>  static int  nfs_statfs(struct dentry *, struct kstatfs *);
> @@ -1412,6 +1429,33 @@ static int nfs_parse_mount_options(char *raw,
>  			mnt->fscache_uniq = string;
>  			mnt->options |= NFS_OPTION_FSCACHE;
>  			break;
> +		case Opt_llock:
> +			string = match_strdup(args);
> +			if (string == NULL)
> +				goto out_nomem;
> +			token = match_token(string, nfs_llock_tokens, args);
> +			kfree(string);
> +			switch (token) {
> +			case Opt_llock_all:
> +				mnt->flags |= (NFS_MOUNT_LOCAL_FLOCK |
> +					       NFS_MOUNT_LOCAL_FCNTL);
> +				break;
> +			case Opt_llock_flock:
> +				mnt->flags |= NFS_MOUNT_LOCAL_FLOCK;
> +				break;
> +			case Opt_llock_fcntl:
> +				mnt->flags |= NFS_MOUNT_LOCAL_FCNTL;
> +				break;
> +			case Opt_llock_none:
> +				mnt->flags &= ~(NFS_MOUNT_LOCAL_FLOCK |
> +						NFS_MOUNT_LOCAL_FCNTL);
> +				break;
> +			default:
> +				dfprintk(MOUNT, "NFS:	invalid	"
> +						"llock argument\n");
> +				return 0;
> +			};
> +			break;

Could we perhaps also convert Opt_nolock so that it just sets
NFS_MOUNT_LOCAL_FLOCK|NFS_MOUNT_LOCAL_FCNTL (and ditto for the legacy
NFS_MOUNT_NONLM).

>  
>  		/*
>  		 * Special options
> diff --git a/include/linux/nfs_mount.h b/include/linux/nfs_mount.h
> index 5d59ae8..576bddd 100644
> --- a/include/linux/nfs_mount.h
> +++ b/include/linux/nfs_mount.h
> @@ -71,4 +71,7 @@ struct nfs_mount_data {
>  #define NFS_MOUNT_NORESVPORT		0x40000
>  #define NFS_MOUNT_LEGACY_INTERFACE	0x80000
>  
> +#define NFS_MOUNT_LOCAL_FLOCK	0x100000
> +#define NFS_MOUNT_LOCAL_FCNTL	0x200000
> +
>  #endif
> 
> 

Cheers
  Trond

Re: [RFC][PATCH] nfs: support legacy NFS flock behavior via mount option

[Suresh Jayaraman]

On 09/08/2010 02:19 AM, Trond Myklebust wrote:
> On Wed, 2010-09-08 at 01:43 +0530, Suresh Jayaraman wrote:

>> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
>> index eb51bd6..a13a83e 100644
>> --- a/fs/nfs/file.c
>> +++ b/fs/nfs/file.c
>> @@ -800,8 +800,13 @@ static int nfs_lock(struct file *filp, int cmd, struct file_lock *fl)
>>  
>>  	nfs_inc_stats(inode, NFSIOS_VFSLOCK);
>>  
>> -	/* No mandatory locks over NFS */
>> -	if (__mandatory_lock(inode) && fl->fl_type != F_UNLCK)
>> +	/*
>> +	 * No mandatory locks over NFS.
>> +	 * fcntl lock is local if mounted with "-o llock=fcntl" or
>> +	 * "-o llock=posix"
>> +	 */
>> +	if ((__mandatory_lock(inode) && fl->fl_type != F_UNLCK) ||
>> +			(NFS_SERVER(inode)->flags & NFS_MOUNT_LOCAL_FCNTL))
>>  		goto out_err;
>>  
>>  	if (NFS_PROTO(inode)->lock_check_bounds != NULL) {
>> @@ -825,12 +830,16 @@ out_err:
>>   */
>>  static int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)
>>  {
>> +	struct inode *inode = filp->f_mapping->host;
>> +
>>  	dprintk("NFS: flock(%s/%s, t=%x, fl=%x)\n",
>>  			filp->f_path.dentry->d_parent->d_name.name,
>>  			filp->f_path.dentry->d_name.name,
>>  			fl->fl_type, fl->fl_flags);
>>  
>> -	if (!(fl->fl_flags & FL_FLOCK))
>> +	/* flock is local if mounted with "-o llock=flock" */
>> +	if ((NFS_SERVER(inode)->flags & NFS_MOUNT_LOCAL_FLOCK) ||
>> +			(!(fl->fl_flags & FL_FLOCK)))
>>  		return -ENOLCK;
> 
> Is this really what we want to do? Shouldn't we rather default to
> treating this as if NFS_MOUNT_NONLM were set?
> 

ah, yes, you're right.

>>  
>>  static void nfs_umount_begin(struct super_block *);
>>  static int  nfs_statfs(struct dentry *, struct kstatfs *);
>> @@ -1412,6 +1429,33 @@ static int nfs_parse_mount_options(char *raw,
>>  			mnt->fscache_uniq = string;
>>  			mnt->options |= NFS_OPTION_FSCACHE;
>>  			break;
>> +		case Opt_llock:
>> +			string = match_strdup(args);
>> +			if (string == NULL)
>> +				goto out_nomem;
>> +			token = match_token(string, nfs_llock_tokens, args);
>> +			kfree(string);
>> +			switch (token) {
>> +			case Opt_llock_all:
>> +				mnt->flags |= (NFS_MOUNT_LOCAL_FLOCK |
>> +					       NFS_MOUNT_LOCAL_FCNTL);
>> +				break;
>> +			case Opt_llock_flock:
>> +				mnt->flags |= NFS_MOUNT_LOCAL_FLOCK;
>> +				break;
>> +			case Opt_llock_fcntl:
>> +				mnt->flags |= NFS_MOUNT_LOCAL_FCNTL;
>> +				break;
>> +			case Opt_llock_none:
>> +				mnt->flags &= ~(NFS_MOUNT_LOCAL_FLOCK |
>> +						NFS_MOUNT_LOCAL_FCNTL);
>> +				break;
>> +			default:
>> +				dfprintk(MOUNT, "NFS:	invalid	"
>> +						"llock argument\n");
>> +				return 0;
>> +			};
>> +			break;
> 
> Could we perhaps also convert Opt_nolock so that it just sets
> NFS_MOUNT_LOCAL_FLOCK|NFS_MOUNT_LOCAL_FCNTL (and ditto for the legacy
> NFS_MOUNT_NONLM).
> 

Did you mean defining NFS_MOUNT_NONLM as

#define NFS_MOUNT_NONLM         (NFS_MOUNT_LOCAL_FLOCK|NFS_MOUNT_LOCAL_FCNTL) ?

This would be problematic when we want to say make flock local but
fcntl NLM lock.. or make fcntl local but flock NLM lock.. i.e
when we setup lockd, we check

 if (server->flags & NFS_MOUNT_NONLM)

which will succeed and nlmclnt would remain uninitialized..


Thanks,

-- 
Suresh Jayaraman

Re: [RFC][PATCH] nfs: support legacy NFS flock behavior via mount option

[Trond Myklebust]

On Wed, 2010-09-08 at 20:06 +0530, Suresh Jayaraman wrote:
> On 09/08/2010 02:19 AM, Trond Myklebust wrote:
> > On Wed, 2010-09-08 at 01:43 +0530, Suresh Jayaraman wrote:
> 
> >> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> >> index eb51bd6..a13a83e 100644
> >> --- a/fs/nfs/file.c
> >> +++ b/fs/nfs/file.c
> >> @@ -800,8 +800,13 @@ static int nfs_lock(struct file *filp, int cmd, struct file_lock *fl)
> >>  
> >>  	nfs_inc_stats(inode, NFSIOS_VFSLOCK);
> >>  
> >> -	/* No mandatory locks over NFS */
> >> -	if (__mandatory_lock(inode) && fl->fl_type != F_UNLCK)
> >> +	/*
> >> +	 * No mandatory locks over NFS.
> >> +	 * fcntl lock is local if mounted with "-o llock=fcntl" or
> >> +	 * "-o llock=posix"
> >> +	 */
> >> +	if ((__mandatory_lock(inode) && fl->fl_type != F_UNLCK) ||
> >> +			(NFS_SERVER(inode)->flags & NFS_MOUNT_LOCAL_FCNTL))
> >>  		goto out_err;
> >>  
> >>  	if (NFS_PROTO(inode)->lock_check_bounds != NULL) {
> >> @@ -825,12 +830,16 @@ out_err:
> >>   */
> >>  static int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)
> >>  {
> >> +	struct inode *inode = filp->f_mapping->host;
> >> +
> >>  	dprintk("NFS: flock(%s/%s, t=%x, fl=%x)\n",
> >>  			filp->f_path.dentry->d_parent->d_name.name,
> >>  			filp->f_path.dentry->d_name.name,
> >>  			fl->fl_type, fl->fl_flags);
> >>  
> >> -	if (!(fl->fl_flags & FL_FLOCK))
> >> +	/* flock is local if mounted with "-o llock=flock" */
> >> +	if ((NFS_SERVER(inode)->flags & NFS_MOUNT_LOCAL_FLOCK) ||
> >> +			(!(fl->fl_flags & FL_FLOCK)))
> >>  		return -ENOLCK;
> > 
> > Is this really what we want to do? Shouldn't we rather default to
> > treating this as if NFS_MOUNT_NONLM were set?
> > 
> 
> ah, yes, you're right.
> 
> >>  
> >>  static void nfs_umount_begin(struct super_block *);
> >>  static int  nfs_statfs(struct dentry *, struct kstatfs *);
> >> @@ -1412,6 +1429,33 @@ static int nfs_parse_mount_options(char *raw,
> >>  			mnt->fscache_uniq = string;
> >>  			mnt->options |= NFS_OPTION_FSCACHE;
> >>  			break;
> >> +		case Opt_llock:
> >> +			string = match_strdup(args);
> >> +			if (string == NULL)
> >> +				goto out_nomem;
> >> +			token = match_token(string, nfs_llock_tokens, args);
> >> +			kfree(string);
> >> +			switch (token) {
> >> +			case Opt_llock_all:
> >> +				mnt->flags |= (NFS_MOUNT_LOCAL_FLOCK |
> >> +					       NFS_MOUNT_LOCAL_FCNTL);
> >> +				break;
> >> +			case Opt_llock_flock:
> >> +				mnt->flags |= NFS_MOUNT_LOCAL_FLOCK;
> >> +				break;
> >> +			case Opt_llock_fcntl:
> >> +				mnt->flags |= NFS_MOUNT_LOCAL_FCNTL;
> >> +				break;
> >> +			case Opt_llock_none:
> >> +				mnt->flags &= ~(NFS_MOUNT_LOCAL_FLOCK |
> >> +						NFS_MOUNT_LOCAL_FCNTL);
> >> +				break;
> >> +			default:
> >> +				dfprintk(MOUNT, "NFS:	invalid	"
> >> +						"llock argument\n");
> >> +				return 0;
> >> +			};
> >> +			break;
> > 
> > Could we perhaps also convert Opt_nolock so that it just sets
> > NFS_MOUNT_LOCAL_FLOCK|NFS_MOUNT_LOCAL_FCNTL (and ditto for the legacy
> > NFS_MOUNT_NONLM).
> > 
> 
> Did you mean defining NFS_MOUNT_NONLM as
> 
> #define NFS_MOUNT_NONLM         (NFS_MOUNT_LOCAL_FLOCK|NFS_MOUNT_LOCAL_FCNTL) ?
> 
> This would be problematic when we want to say make flock local but
> fcntl NLM lock.. or make fcntl local but flock NLM lock.. i.e
> when we setup lockd, we check
> 
>  if (server->flags & NFS_MOUNT_NONLM)
> 
> which will succeed and nlmclnt would remain uninitialized..

Agreed, you do need to replace NFS_MOUNT_NONLM in comparisons like the
above, but it seems to me that we need to do that anyway.

I think the easiest is to add a parameter to
do_setlk()/do_getlk()/do_unlck() to indicate whether or not this should
be treated as a local lock, and then set that parameter accordingly in
the calls from nfs_flock() (if NFS_MOUNT_LOCAL_FLOCK) and
nfs_file_lock() (if NFS_MOUNT_LOCAL_FCNTL).

Note also that there might be issues with NFSv4 and lock recovery (this
was IIRC why we disallow -onolock with NFSv4). IIRC, we were getting
Oopses because the struct file_lock wasn't initialised as expected by
the recovery routines.

Cheers
  Trond

Re: [RFC][PATCH] nfs: support legacy NFS flock behavior via mount option

[Neil Brown]

On Tue, 07 Sep 2010 10:17:19 -0400
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> On Mon, 2010-09-06 at 18:03 +0530, Suresh Jayaraman wrote:
> > NFS clients since 2.6.12 support flock()locks by emulating the
> > BSD-style locks in terms of POSIX byte range locks. So the NFS client
> > does not allow to lock the same file using both flock() and fcntl
> > byte-range locks.
> > 
> > For some Windows applications which seem to use both share mode locks
> > (flock()) and fcntl byte range locks sequentially on the same file,
> > the locking is failing as the lock has already been acquired. i.e. the
> > flock mapped as posix locks collide with actual byte range locks from
> > the same process. The problem was observed on a setup with Windows
> > clients accessing Excel files on a Samba exported share which is
> > originally a NFS mount from a NetApp filer. Since kernels < 2.6.12 does
> > not support flock, what was working (as flock locks were local) in
> > older kernels is not working with newer kernels.
> > 
> > This could be seen as a bug in the implementation of the windows
> > application or a NFS client regression, but that is debatable.
> > In the spirit of not breaking existing setups, this patch adds mount
> > options "flock=local" that enables older flock behavior and
> > "flock=fcntl" that allows the current flock behavior.
> 
> So instead of having a special option for flock only, what say we rather
> introduce an option of the form
> 
>   -olocal_lock=
> 
> which can take the values 'none', 'flock', 'fcntl' (or 'posix'?) and
> 'all'?

I observe that the NLM protocol has support for 'share' reservations.
Requesting 'access == READ, mode==DENY_WRITE' is like a shared lock,
and 'access = WRITE, mode== DENY_READ_WRITE' is like an exclusive lock.

As samba maps theh share reservations into flock locks, it could make sense
for NFS to (optionally) map flock locks into share reservations.

The current Linux nfsd handles contention between these reservations entirely
internally, but it could conceivably grow an option to map them into flock
lock, just like samba does.

If this were at all a possible future direction, I would like to ensure that
option names chosen now allowed for that extension.
 flock=local and flock=fcntl naturally extends to flock=share

local_lock= doesn't really extend ... unless shared_lock=flock, but that
seems a bit backwards.

Is that a direction we could ever want to go?

NeilBrown

Re: [RFC][PATCH] nfs: support legacy NFS flock behavior via mount option

[Trond Myklebust]

On Wed, 2010-09-08 at 08:23 +1000, Neil Brown wrote:
> On Tue, 07 Sep 2010 10:17:19 -0400
> Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> 
> > On Mon, 2010-09-06 at 18:03 +0530, Suresh Jayaraman wrote:
> > > NFS clients since 2.6.12 support flock()locks by emulating the
> > > BSD-style locks in terms of POSIX byte range locks. So the NFS client
> > > does not allow to lock the same file using both flock() and fcntl
> > > byte-range locks.
> > > 
> > > For some Windows applications which seem to use both share mode locks
> > > (flock()) and fcntl byte range locks sequentially on the same file,
> > > the locking is failing as the lock has already been acquired. i.e. the
> > > flock mapped as posix locks collide with actual byte range locks from
> > > the same process. The problem was observed on a setup with Windows
> > > clients accessing Excel files on a Samba exported share which is
> > > originally a NFS mount from a NetApp filer. Since kernels < 2.6.12 does
> > > not support flock, what was working (as flock locks were local) in
> > > older kernels is not working with newer kernels.
> > > 
> > > This could be seen as a bug in the implementation of the windows
> > > application or a NFS client regression, but that is debatable.
> > > In the spirit of not breaking existing setups, this patch adds mount
> > > options "flock=local" that enables older flock behavior and
> > > "flock=fcntl" that allows the current flock behavior.
> > 
> > So instead of having a special option for flock only, what say we rather
> > introduce an option of the form
> > 
> >   -olocal_lock=
> > 
> > which can take the values 'none', 'flock', 'fcntl' (or 'posix'?) and
> > 'all'?
> 
> I observe that the NLM protocol has support for 'share' reservations.
> Requesting 'access == READ, mode==DENY_WRITE' is like a shared lock,
> and 'access = WRITE, mode== DENY_READ_WRITE' is like an exclusive lock.
> 
> As samba maps theh share reservations into flock locks, it could make sense
> for NFS to (optionally) map flock locks into share reservations.
> 
> The current Linux nfsd handles contention between these reservations entirely
> internally, but it could conceivably grow an option to map them into flock
> lock, just like samba does.
> 
> If this were at all a possible future direction, I would like to ensure that
> option names chosen now allowed for that extension.
>  flock=local and flock=fcntl naturally extends to flock=share
> 
> local_lock= doesn't really extend ... unless shared_lock=flock, but that
> seems a bit backwards.
> 
> Is that a direction we could ever want to go?

I'd be against it for several reasons:

      * Ordinary flock locks are advisory, whereas deny share
        reservations are special mandatory locks. In fact if you look at
        the Samba implementation, it uses a special 'LOCK_MAND' flag in
        addition to the usual flock() flag.
      * DENY_WRITE and DENY_BOTH share reservation modes break unlink()
        behaviour on posix systems.
      * Once the file has been opened with a given access mode, my
        interpretation of the protocol is that you cannot change the
        deny mode without closing any conflicting shares first. (Section
        8.9 says: This checking of share reservations on OPEN is done
        with no exception for an existing OPEN for the same open_owner.)

Cheers
  Trond

Re: [RFC][PATCH] nfs: support legacy NFS flock behavior via mount option

[Neil Brown]

On Tue, 07 Sep 2010 18:42:42 -0400
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> On Wed, 2010-09-08 at 08:23 +1000, Neil Brown wrote:
> > On Tue, 07 Sep 2010 10:17:19 -0400
> > Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> > 
> > > On Mon, 2010-09-06 at 18:03 +0530, Suresh Jayaraman wrote:
> > > > NFS clients since 2.6.12 support flock()locks by emulating the
> > > > BSD-style locks in terms of POSIX byte range locks. So the NFS client
> > > > does not allow to lock the same file using both flock() and fcntl
> > > > byte-range locks.
> > > > 
> > > > For some Windows applications which seem to use both share mode locks
> > > > (flock()) and fcntl byte range locks sequentially on the same file,
> > > > the locking is failing as the lock has already been acquired. i.e. the
> > > > flock mapped as posix locks collide with actual byte range locks from
> > > > the same process. The problem was observed on a setup with Windows
> > > > clients accessing Excel files on a Samba exported share which is
> > > > originally a NFS mount from a NetApp filer. Since kernels < 2.6.12 does
> > > > not support flock, what was working (as flock locks were local) in
> > > > older kernels is not working with newer kernels.
> > > > 
> > > > This could be seen as a bug in the implementation of the windows
> > > > application or a NFS client regression, but that is debatable.
> > > > In the spirit of not breaking existing setups, this patch adds mount
> > > > options "flock=local" that enables older flock behavior and
> > > > "flock=fcntl" that allows the current flock behavior.
> > > 
> > > So instead of having a special option for flock only, what say we rather
> > > introduce an option of the form
> > > 
> > >   -olocal_lock=
> > > 
> > > which can take the values 'none', 'flock', 'fcntl' (or 'posix'?) and
> > > 'all'?
> > 
> > I observe that the NLM protocol has support for 'share' reservations.
> > Requesting 'access == READ, mode==DENY_WRITE' is like a shared lock,
> > and 'access = WRITE, mode== DENY_READ_WRITE' is like an exclusive lock.
> > 
> > As samba maps theh share reservations into flock locks, it could make sense
> > for NFS to (optionally) map flock locks into share reservations.
> > 
> > The current Linux nfsd handles contention between these reservations entirely
> > internally, but it could conceivably grow an option to map them into flock
> > lock, just like samba does.
> > 
> > If this were at all a possible future direction, I would like to ensure that
> > option names chosen now allowed for that extension.
> >  flock=local and flock=fcntl naturally extends to flock=share
> > 
> > local_lock= doesn't really extend ... unless shared_lock=flock, but that
> > seems a bit backwards.
> > 
> > Is that a direction we could ever want to go?
> 
> I'd be against it for several reasons:
> 
>       * Ordinary flock locks are advisory, whereas deny share
>         reservations are special mandatory locks. In fact if you look at
>         the Samba implementation, it uses a special 'LOCK_MAND' flag in
>         addition to the usual flock() flag.
>       * DENY_WRITE and DENY_BOTH share reservation modes break unlink()
>         behaviour on posix systems.
>       * Once the file has been opened with a given access mode, my
>         interpretation of the protocol is that you cannot change the
>         deny mode without closing any conflicting shares first. (Section
>         8.9 says: This checking of share reservations on OPEN is done
>         with no exception for an existing OPEN for the same open_owner.)

This all seems to assume NFSv4 rather than NFSv3 and NLM - I don't think NLM
is nearly that specific on how shares should work and doesn't mention 'open'
at all.

However I can understand your desire to implement it the same way for v4 as
v3 and in that case your arguments stand.

My other idea was for flock to just lock one byte of the file at a very high
offset, so it would interact properly with other flocks but almost never with
any fcntl lock.  That would probably have other problems though....

I guess 'local' or 'fcntl' are really the only options for flock.

oh well...

NeilBrown

> 
> Cheers
>   Trond