Re: [PATCH 5/8] Keep read and write fds with each nlm_file

[bfields@fieldses.org (J. Bruce Fields)]

On Sat, Aug 21, 2021 at 04:30:43PM +0000, Chuck Lever III wrote:
> > @@ -654,7 +658,12 @@ nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
> > 	nlmsvc_cancel_blocked(net, file, lock);
> > 
> > 	lock->fl.fl_type = F_UNLCK;
> > -	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
> > +	if (file->f_file[0])
> > +		error = vfs_lock_file(file->f_file[0], F_SETLK,
> > +					&lock->fl, NULL);
> > +	if (file->f_file[1])
> > +		error = vfs_lock_file(file->f_file[1], F_SETLK,
> > +					&lock->fl, NULL);
> 
> Eschew raw integers :-) Should the f_file array be indexed
> using O_ flags as the comment below suggests?

Sure, done.

> > diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
> > index f43d89e89c45..a0adaee245ae 100644
> > --- a/fs/lockd/svcsubs.c
> > +++ b/fs/lockd/svcsubs.c
> > @@ -71,14 +71,38 @@ static inline unsigned int file_hash(struct nfs_fh *f)
> > 	return tmp & (FILE_NRHASH - 1);
> > }
> > 
> > +int lock_to_openmode(struct file_lock *lock)
> > +{
> > +	if (lock->fl_type == F_WRLCK)
> > +		return O_WRONLY;
> > +	else
> > +		return O_RDONLY;
> 
> Style: a ternary would be more consistent with other areas
> of the code you change in this patch.

OK.

> > +static int nlm_unlock_files(struct nlm_file *file)
> > +{
> > +	struct file_lock lock;
> > +	struct file *f;
> > +
> > +	lock.fl_type  = F_UNLCK;
> > +	lock.fl_start = 0;
> > +	lock.fl_end   = OFFSET_MAX;
> > +	for (f = file->f_file[0]; f <= file->f_file[1]; f++) {
> > +		if (f && vfs_lock_file(f, F_SETLK, &lock, NULL) < 0) {
> > +			printk("lockd: unlock failure in %s:%d\n",
> > +				__FILE__, __LINE__);
> 
> This needs a KERN_LEVEL and maybe a _once.

How about going with pr_warn for now.  I don't *think* there's much
danger of spamming the logs from this one.  And I'm wondering there
might be some causes (unresponsive server?) that could resolve
themselves, and then come back, and that you'd still want to hear about
the second time.

> > @@ -27,7 +27,8 @@ struct rpc_task;
> > struct nlmsvc_binding {
> > 	__be32			(*fopen)(struct svc_rqst *,
> > 						struct nfs_fh *,
> > -						struct file **);
> > +						struct file **,
> > +						int mode);
> 
> Style: "mode_t mode" might be better internal documentation.

It's confusing that we use the word "mode" both for "access mode" (O_
flags) and "mode bits" (permission bits).  This is the former, and I
thought mode_t was the for the latter.

> > @@ -154,7 +156,8 @@ struct nlm_rqst {
> > struct nlm_file {
> > 	struct hlist_node	f_list;		/* linked list */
> > 	struct nfs_fh		f_handle;	/* NFS file handle */
> > -	struct file *		f_file;		/* VFS file pointer */
> > +	struct file *		f_file[2];	/* VFS file pointers,
> > +						   indexed by O_ flags */
> 
> Right, except that the new code in this patch always indexes
> f_file with raw integers, making the comment misleading. My
> preference is to keep the comment and change the new code to
> index f_file symbolically.

OK.

--b.