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@xxxxxxx>
> 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@xxxxxxx>
> Signed-off-by: Suresh Jayaraman <sjayaraman@xxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
