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

[Trond Myklebust <Trond.Myklebust@netapp.com>]

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