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
--
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
