Re: [PATCH 7/9] nfs: add code to allow client to wait on lock callbacks

[Jeff Layton <jlayton@redhat.com>]

On Thu, 2016-09-08 at 15:59 -0400, Anna Schumaker wrote:
> Hi Jeff,
> 
> On 09/06/2016 11:12 AM, Jeff Layton wrote:
> > 
> > Add a waitqueue head to the client structure. Have clients set a
> > wait
> > on that queue prior to requesting a lock from the server. If the
> > lock
> > is blocked, then we can use that to wait for wakeups.
> > 
> > Note that we do need to do this "manually" since we need to set the
> > wait on the waitqueue prior to requesting the lock, but requesting
> > a
> > lock can involve activities that can block.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/nfs/callback_proc.c    |  1 +
> >  fs/nfs/nfs4client.c       |  1 +
> >  fs/nfs/nfs4proc.c         | 67
> > ++++++++++++++++++++++++++++++++++++++++++++---
> >  include/linux/nfs_fs_sb.h |  1 +
> >  4 files changed, 67 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
> > index a211af339f32..4ba6a8763f91 100644
> > --- a/fs/nfs/callback_proc.c
> > +++ b/fs/nfs/callback_proc.c
> > @@ -645,6 +645,7 @@ __be32 nfs4_callback_notify_lock(struct
> > cb_notify_lock_args *args, void *dummy,
> >  	fc_tbl = &cps->clp->cl_session->fc_slot_table;
> >  
> >  	status = htonl(NFS4_OK);
> > +	__wake_up(&cps->clp->cl_lock_waitq, TASK_NORMAL, 0, &args-
> > >cbnl_owner);
> >  	return status;
> >  }
> >  #endif /* CONFIG_NFS_V4_1 */
> > diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
> > index cd3b7cfdde16..89b7a794ff10 100644
> > --- a/fs/nfs/nfs4client.c
> > +++ b/fs/nfs/nfs4client.c
> > @@ -199,6 +199,7 @@ struct nfs_client *nfs4_alloc_client(const
> > struct nfs_client_initdata *cl_init)
> >  	clp->cl_minorversion = cl_init->minorversion;
> >  	clp->cl_mvops = nfs_v4_minor_ops[cl_init->minorversion];
> >  	clp->cl_mig_gen = 1;
> > +	init_waitqueue_head(&clp->cl_lock_waitq);
> >  	return clp;
> >  
> >  error:
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 5573f19539a6..3a6669063c44 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -5531,13 +5531,54 @@ int nfs4_proc_delegreturn(struct inode
> > *inode, struct rpc_cred *cred, const nfs4
> >  #define NFS4_LOCK_MINTIMEOUT (1 * HZ)
> >  #define NFS4_LOCK_MAXTIMEOUT (30 * HZ)
> >  
> > +struct nfs4_lock_waiter {
> > +	struct task_struct	*task;
> > +	struct nfs_lowner	*owner;
> > +	bool			notified;
> > +};
> > +
> > +static int
> > +nfs4_wake_lock_waiter(wait_queue_t *wait, unsigned int mode, int
> > flags, void *key)
> > +{
> > +	int ret;
> > +	struct nfs4_lock_waiter	*waiter	= wait-
> > >private;
> > +	struct nfs_lowner	*lowner = key, *wowner = waiter-
> > >owner;
> > +
> > +	/* Don't wake anybody if the string looked bogus */
> > +	if (!lowner->id && !lowner->s_dev)
> > +		return 0;
> > +
> > +	/* Only wake if the callback was for the same owner */
> > +	if (lowner->clientid != wowner->clientid ||
> > +	    lowner->id != wowner->id		 ||
> > +	    lowner->s_dev != wowner->s_dev)
> > +		return 0;
> > +
> > +	waiter->notified = true;
> > +
> > +	/* override "private" so we can use default_wake_function
> > */
> > +	wait->private = waiter->task;
> > +	ret = autoremove_wake_function(wait, mode, flags, key);
> > +	wait->private = waiter;
> > +	return ret;
> > +}
> > +
> >  /* 
> >   * sleep, with exponential backoff, and retry the LOCK operation. 
> >   */
> >  static unsigned long
> > -nfs4_set_lock_task_retry(unsigned long timeout)
> > +nfs4_wait_for_lock_retry(unsigned long timeout, wait_queue_head_t
> > *q,
> > +			 struct nfs4_lock_waiter *waiter)
> >  {
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&q->lock, flags);
> > +	if (waiter->notified) {
> > +		spin_unlock_irqrestore(&q->lock, flags);
> > +		return timeout;
> > +	}
> >  	set_current_state(TASK_INTERRUPTIBLE);
> > +	spin_unlock_irqrestore(&q->lock, flags);
> >  	freezable_schedule_timeout_unsafe(timeout);
> >  	timeout <<= 1;
> >  	if (timeout > NFS4_LOCK_MAXTIMEOUT)
> > @@ -6232,10 +6273,30 @@ nfs4_proc_lock(struct file *filp, int cmd,
> > struct file_lock *request)
> >  		return status;
> >  
> >  	do {
> > +		struct nfs4_lock_state *lsp = request-
> > >fl_u.nfs4_fl.owner;
> > +		struct nfs_server *server = NFS_SERVER(lsp-
> > >ls_state->inode);
> > +		struct nfs_client *clp = server->nfs_client;
> > +		struct nfs_lowner owner = { .clientid = clp-
> > >cl_clientid,
> > +					    .id = lsp-
> > >ls_seqid.owner_id,
> > +					    .s_dev = server->s_dev 
> > };
> > +		struct nfs4_lock_waiter waiter = { .task  =
> > current,
> > +						   .owner =
> > &owner,
> > +						   .notified =
> > false };
> > +		wait_queue_t wait;
> > +
> > +		init_wait(&wait);
> > +		wait.private = &waiter;
> > +		wait.func = nfs4_wake_lock_waiter;
> > +
> > +		add_wait_queue(&clp->cl_lock_waitq, &wait);
> >  		status = nfs4_proc_setlk(state, cmd, request);
> > -		if ((status != -EAGAIN) || IS_SETLK(cmd))
> > +		if ((status != -EAGAIN) || IS_SETLK(cmd)) {
> > +			finish_wait(&clp->cl_lock_waitq, &wait);
> >  			break;
> > -		timeout = nfs4_set_lock_task_retry(timeout);
> > +		}
> > +		timeout = nfs4_wait_for_lock_retry(timeout,
> > +					&clp->cl_lock_waitq,
> > &waiter);
> > +		finish_wait(&clp->cl_lock_waitq, &wait);
> 
> I'm curious why you didn't turn everything in this do-while loop into
> a new function?  This code might be cleaner that way :)
> 
> Thanks,
> Anna
> 

I went through several iterations of this set before I found a scheme
that would work. Your point is entirely valid. I'll move it into a
helper function for the next posting.

Thanks!

> > 
> >  		status = -ERESTARTSYS;
> >  		if (signalled())
> >  			break;
> > diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> > index 14a762d2734d..fb2319a0ce65 100644
> > --- a/include/linux/nfs_fs_sb.h
> > +++ b/include/linux/nfs_fs_sb.h
> > @@ -103,6 +103,7 @@ struct nfs_client {
> >  #define NFS_SP4_MACH_CRED_WRITE    5	/* WRITE */
> >  #define NFS_SP4_MACH_CRED_COMMIT   6	/* COMMIT */
> >  #define NFS_SP4_MACH_CRED_PNFS_CLEANUP  7 /* LAYOUTRETURN */
> > +	wait_queue_head_t	cl_lock_waitq;
> >  #endif /* CONFIG_NFS_V4 */
> >  
> >  	/* Our own IP address, as a null-terminated string.
> > 
> 

-- 
Jeff Layton <jlayton@redhat.com>