[PATCH 0/9] nfs: add CB_NOTIFY_LOCK support to nfs client

[PATCH 0/9] nfs: add CB_NOTIFY_LOCK support to nfs client

[Jeff Layton]

This patchset adds support for CB_NOTIFY_LOCK callbacks to the NFS
client. The basic idea is to add a waitqueue to the nfs_client and then
have blocking lock waiters wait on that queue for callbacks.

When a callback comes in, we use a keyed wakeup to wake any waiters. The
waitqueue handling is necessarily more "manual" than I would like, but I
don't see a real alternative there given that we need to insert the
waiters onto the waitqueue prior to sending the lock request, and
sending a lock request can involve blocking operations.

Tested in conjunction with the corresponding knfsd server-side patchset.

Jeff Layton (9):
  nfs: the length argument to read_buf should be unsigned
  nfs: eliminate pointless and confusing do_vfs_lock wrappers
  nfs: check for POSIX lock capability on server even for flock locks
  nfs: add a freezable_schedule_timeout_unsafe() and use it when waiting
    to retry LOCK
  nfs: add handling for CB_NOTIFY_LOCK in client
  nfs: move nfs4_set_lock_state call into caller
  nfs: add code to allow client to wait on lock callbacks
  nfs: ensure that the filehandle in CB_NOTIFY_LOCK request matches the
    inode
  nfs: track whether server sets MAY_NOTIFY_LOCK flag

 fs/nfs/callback.h         |   8 ++++
 fs/nfs/callback_proc.c    |  20 ++++++++
 fs/nfs/callback_xdr.c     |  51 +++++++++++++++++++-
 fs/nfs/file.c             |   9 +---
 fs/nfs/nfs4_fs.h          |   1 +
 fs/nfs/nfs4client.c       |   1 +
 fs/nfs/nfs4proc.c         | 118 +++++++++++++++++++++++++++++++++++++---------
 include/linux/freezer.h   |  13 +++++
 include/linux/nfs_fs_sb.h |   1 +
 9 files changed, 190 insertions(+), 32 deletions(-)

-- 
2.7.4

[PATCH 1/9] nfs: the length argument to read_buf should be unsigned

[Jeff Layton]

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/callback_xdr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
index 656f68f7fe53..d6a40c06ec26 100644
--- a/fs/nfs/callback_xdr.c
+++ b/fs/nfs/callback_xdr.c
@@ -72,7 +72,7 @@ static int nfs4_encode_void(struct svc_rqst *rqstp, __be32 *p, void *dummy)
 	return xdr_ressize_check(rqstp, p);
 }
 
-static __be32 *read_buf(struct xdr_stream *xdr, int nbytes)
+static __be32 *read_buf(struct xdr_stream *xdr, unsigned int nbytes)
 {
 	__be32 *p;
 
-- 
2.7.4

Re: [PATCH 1/9] nfs: the length argument to read_buf should be unsigned

[Anna Schumaker]

Hi Jeff,

On 09/06/2016 11:12 AM, Jeff Layton wrote:
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/nfs/callback_xdr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
> index 656f68f7fe53..d6a40c06ec26 100644
> --- a/fs/nfs/callback_xdr.c
> +++ b/fs/nfs/callback_xdr.c
> @@ -72,7 +72,7 @@ static int nfs4_encode_void(struct svc_rqst *rqstp, __be32 *p, void *dummy)
>  	return xdr_ressize_check(rqstp, p);
>  }
>  
> -static __be32 *read_buf(struct xdr_stream *xdr, int nbytes)
> +static __be32 *read_buf(struct xdr_stream *xdr, unsigned int nbytes)

Looks like the nbytes argument is only used by xdr_inline_decode(), which expects a size_t instead of an unsigned int.  If we're changing argument types, then maybe we should change it to a size_t instead.

Thoughts?
Anna

>  {
>  	__be32 *p;
>  
> 

Re: [PATCH 1/9] nfs: the length argument to read_buf should be unsigned

[Jeff Layton]

On Thu, 2016-09-08 at 13:39 -0400, Anna Schumaker wrote:
> Hi Jeff,
> 
> On 09/06/2016 11:12 AM, Jeff Layton wrote:
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/nfs/callback_xdr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
> > index 656f68f7fe53..d6a40c06ec26 100644
> > --- a/fs/nfs/callback_xdr.c
> > +++ b/fs/nfs/callback_xdr.c
> > @@ -72,7 +72,7 @@ static int nfs4_encode_void(struct svc_rqst
> > *rqstp, __be32 *p, void *dummy)
> >  	return xdr_ressize_check(rqstp, p);
> >  }
> >  
> > -static __be32 *read_buf(struct xdr_stream *xdr, int nbytes)
> > +static __be32 *read_buf(struct xdr_stream *xdr, unsigned int
> > nbytes)
> 
> Looks like the nbytes argument is only used by xdr_inline_decode(),
> which expects a size_t instead of an unsigned int.  If we're changing
> argument types, then maybe we should change it to a size_t instead.
> 
> Thoughts?
> Anna
> 

Sure, that works too. I'd have a hard time imagining when we'd ever use
those upper bits when size_t is 64 bits, but I doubt it'd hurt
anything. I'll go ahead and change that in my tree.

-- 
Jeff Layton <jlayton@redhat.com>

[PATCH 2/9] nfs: eliminate pointless and confusing do_vfs_lock wrappers

[Jeff Layton]

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/file.c     |  9 ++-------
 fs/nfs/nfs4proc.c | 15 +++++----------
 2 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 7d620970f2e1..a3332a78a0e5 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -705,11 +705,6 @@ out_noconflict:
 	goto out;
 }
 
-static int do_vfs_lock(struct file *file, struct file_lock *fl)
-{
-	return locks_lock_file_wait(file, fl);
-}
-
 static int
 do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 {
@@ -742,7 +737,7 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 	if (!is_local)
 		status = NFS_PROTO(inode)->lock(filp, cmd, fl);
 	else
-		status = do_vfs_lock(filp, fl);
+		status = locks_lock_file_wait(filp, fl);
 	return status;
 }
 
@@ -767,7 +762,7 @@ do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 	if (!is_local)
 		status = NFS_PROTO(inode)->lock(filp, cmd, fl);
 	else
-		status = do_vfs_lock(filp, fl);
+		status = locks_lock_file_wait(filp, fl);
 	if (status < 0)
 		goto out;
 
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index f5aecaabcb7c..85817e4103ea 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5600,11 +5600,6 @@ static int nfs4_proc_getlk(struct nfs4_state *state, int cmd, struct file_lock *
 	return err;
 }
 
-static int do_vfs_lock(struct inode *inode, struct file_lock *fl)
-{
-	return locks_lock_inode_wait(inode, fl);
-}
-
 struct nfs4_unlockdata {
 	struct nfs_locku_args arg;
 	struct nfs_locku_res res;
@@ -5657,7 +5652,7 @@ static void nfs4_locku_done(struct rpc_task *task, void *data)
 	switch (task->tk_status) {
 		case 0:
 			renew_lease(calldata->server, calldata->timestamp);
-			do_vfs_lock(calldata->lsp->ls_state->inode, &calldata->fl);
+			locks_lock_inode_wait(calldata->lsp->ls_state->inode, &calldata->fl);
 			if (nfs4_update_lock_stateid(calldata->lsp,
 					&calldata->res.stateid))
 				break;
@@ -5765,7 +5760,7 @@ static int nfs4_proc_unlck(struct nfs4_state *state, int cmd, struct file_lock *
 	mutex_lock(&sp->so_delegreturn_mutex);
 	/* Exclude nfs4_reclaim_open_stateid() - note nesting! */
 	down_read(&nfsi->rwsem);
-	if (do_vfs_lock(inode, request) == -ENOENT) {
+	if (locks_lock_inode_wait(inode, request) == -ENOENT) {
 		up_read(&nfsi->rwsem);
 		mutex_unlock(&sp->so_delegreturn_mutex);
 		goto out;
@@ -5906,7 +5901,7 @@ static void nfs4_lock_done(struct rpc_task *task, void *calldata)
 				data->timestamp);
 		if (data->arg.new_lock) {
 			data->fl.fl_flags &= ~(FL_SLEEP | FL_ACCESS);
-			if (do_vfs_lock(lsp->ls_state->inode, &data->fl) < 0) {
+			if (locks_lock_inode_wait(lsp->ls_state->inode, &data->fl) < 0) {
 				rpc_restart_call_prepare(task);
 				break;
 			}
@@ -6148,7 +6143,7 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
 	if (status != 0)
 		goto out;
 	request->fl_flags |= FL_ACCESS;
-	status = do_vfs_lock(state->inode, request);
+	status = locks_lock_inode_wait(state->inode, request);
 	if (status < 0)
 		goto out;
 	mutex_lock(&sp->so_delegreturn_mutex);
@@ -6157,7 +6152,7 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
 		/* Yes: cache locks! */
 		/* ...but avoid races with delegation recall... */
 		request->fl_flags = fl_flags & ~FL_SLEEP;
-		status = do_vfs_lock(state->inode, request);
+		status = locks_lock_inode_wait(state->inode, request);
 		up_read(&nfsi->rwsem);
 		mutex_unlock(&sp->so_delegreturn_mutex);
 		goto out;
-- 
2.7.4

[PATCH 3/9] nfs: check for POSIX lock capability on server even for flock locks

[Jeff Layton]

We may end up in here with a FL_FLOCK lock request. We translate
those to POSIX locks on the server, so we need to verify that the
server supports them no matter what sort of lock request this is.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/nfs4proc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 85817e4103ea..e3bf95369daf 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6135,8 +6135,7 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
 	unsigned char fl_flags = request->fl_flags;
 	int status = -ENOLCK;
 
-	if ((fl_flags & FL_POSIX) &&
-			!test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
+	if (!test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
 		goto out;
 	/* Is this a delegated open? */
 	status = nfs4_set_lock_state(state, request);
-- 
2.7.4

[PATCH 4/9] nfs: add a freezable_schedule_timeout_unsafe() and use it when waiting to retry LOCK

[Jeff Layton]

We actually want to use TASK_INTERRUPTIBLE sleeps here. Once the task
wakes up, if there is a signal pending then we'll be returning an error
anyway. So, we might as well wake up immediately for non-fatal signals
as well. That allows us to return to userland more quickly in that case,
but won't change the error that userland sees.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/nfs4proc.c       |  3 ++-
 include/linux/freezer.h | 13 +++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index e3bf95369daf..e9232d71bc64 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5537,7 +5537,8 @@ int nfs4_proc_delegreturn(struct inode *inode, struct rpc_cred *cred, const nfs4
 static unsigned long
 nfs4_set_lock_task_retry(unsigned long timeout)
 {
-	freezable_schedule_timeout_killable_unsafe(timeout);
+	set_current_state(TASK_INTERRUPTIBLE);
+	freezable_schedule_timeout_unsafe(timeout);
 	timeout <<= 1;
 	if (timeout > NFS4_LOCK_MAXTIMEOUT)
 		return NFS4_LOCK_MAXTIMEOUT;
diff --git a/include/linux/freezer.h b/include/linux/freezer.h
index dd03e837ebb7..fe31601e7f55 100644
--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -197,6 +197,19 @@ static inline long freezable_schedule_timeout(long timeout)
  * Like schedule_timeout_interruptible(), but should not block the freezer.  Do not
  * call this with locks held.
  */
+static inline long freezable_schedule_timeout_unsafe(long timeout)
+{
+	long __retval;
+	freezer_do_not_count();
+	__retval = schedule_timeout(timeout);
+	freezer_count_unsafe();
+	return __retval;
+}
+
+/*
+ * Like schedule_timeout_interruptible(), but should not block the freezer.  Do not
+ * call this with locks held.
+ */
 static inline long freezable_schedule_timeout_interruptible(long timeout)
 {
 	long __retval;
-- 
2.7.4

Re: [PATCH 4/9] nfs: add a freezable_schedule_timeout_unsafe() and use it when waiting to retry LOCK

[Jeff Layton]

On Tue, 2016-09-06 at 11:12 -0400, Jeff Layton wrote:
> We actually want to use TASK_INTERRUPTIBLE sleeps here. Once the task
> wakes up, if there is a signal pending then we'll be returning an error
> anyway. So, we might as well wake up immediately for non-fatal signals
> as well. That allows us to return to userland more quickly in that case,
> but won't change the error that userland sees.
> 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/nfs/nfs4proc.c       |  3 ++-
>  include/linux/freezer.h | 13 +++++++++++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index e3bf95369daf..e9232d71bc64 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5537,7 +5537,8 @@ int nfs4_proc_delegreturn(struct inode *inode, struct rpc_cred *cred, const nfs4
>  static unsigned long
>  nfs4_set_lock_task_retry(unsigned long timeout)
>  {
> > -	freezable_schedule_timeout_killable_unsafe(timeout);
> > +	set_current_state(TASK_INTERRUPTIBLE);
> > +	freezable_schedule_timeout_unsafe(timeout);
> >  	timeout <<= 1;
> >  	if (timeout > NFS4_LOCK_MAXTIMEOUT)
> >  		return NFS4_LOCK_MAXTIMEOUT;
> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> index dd03e837ebb7..fe31601e7f55 100644
> --- a/include/linux/freezer.h
> +++ b/include/linux/freezer.h
> @@ -197,6 +197,19 @@ static inline long freezable_schedule_timeout(long timeout)
>   * Like schedule_timeout_interruptible(), but should not block the freezer.  Do not
>   * call this with locks held.
>   */
> +static inline long freezable_schedule_timeout_unsafe(long timeout)
> +{
> > +	long __retval;
> > +	freezer_do_not_count();
> > +	__retval = schedule_timeout(timeout);
> > +	freezer_count_unsafe();
> > +	return __retval;
> +}
> +
> +/*
> + * Like schedule_timeout_interruptible(), but should not block the freezer.  Do not
> + * call this with locks held.
> + */
>  static inline long freezable_schedule_timeout_interruptible(long timeout)
>  {
> >  	long __retval;

Ahh, kbuild test bot tells me that I neglected to make an equivalent
function for the !CONFIG_FREEZER case. Fixed in my git tree. The next
posting should have that.
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 4/9] nfs: add a freezable_schedule_timeout_unsafe() and use it when waiting to retry LOCK

[Anna Schumaker]

Hi Jeff,

Can you CC the freezer folks on this patch during the second posting?  They'll probably need to at send an ACK for the changes to their include file.

Thanks,
Anna

On 09/06/2016 11:12 AM, Jeff Layton wrote:
> We actually want to use TASK_INTERRUPTIBLE sleeps here. Once the task
> wakes up, if there is a signal pending then we'll be returning an error
> anyway. So, we might as well wake up immediately for non-fatal signals
> as well. That allows us to return to userland more quickly in that case,
> but won't change the error that userland sees.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/nfs/nfs4proc.c       |  3 ++-
>  include/linux/freezer.h | 13 +++++++++++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index e3bf95369daf..e9232d71bc64 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5537,7 +5537,8 @@ int nfs4_proc_delegreturn(struct inode *inode, struct rpc_cred *cred, const nfs4
>  static unsigned long
>  nfs4_set_lock_task_retry(unsigned long timeout)
>  {
> -	freezable_schedule_timeout_killable_unsafe(timeout);
> +	set_current_state(TASK_INTERRUPTIBLE);
> +	freezable_schedule_timeout_unsafe(timeout);
>  	timeout <<= 1;
>  	if (timeout > NFS4_LOCK_MAXTIMEOUT)
>  		return NFS4_LOCK_MAXTIMEOUT;
> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> index dd03e837ebb7..fe31601e7f55 100644
> --- a/include/linux/freezer.h
> +++ b/include/linux/freezer.h
> @@ -197,6 +197,19 @@ static inline long freezable_schedule_timeout(long timeout)
>   * Like schedule_timeout_interruptible(), but should not block the freezer.  Do not
>   * call this with locks held.
>   */
> +static inline long freezable_schedule_timeout_unsafe(long timeout)
> +{
> +	long __retval;
> +	freezer_do_not_count();
> +	__retval = schedule_timeout(timeout);
> +	freezer_count_unsafe();
> +	return __retval;
> +}
> +
> +/*
> + * Like schedule_timeout_interruptible(), but should not block the freezer.  Do not
> + * call this with locks held.
> + */
>  static inline long freezable_schedule_timeout_interruptible(long timeout)
>  {
>  	long __retval;
> 

Re: [PATCH 4/9] nfs: add a freezable_schedule_timeout_unsafe() and use it when waiting to retry LOCK

[Jeff Layton]

On Thu, 2016-09-08 at 14:20 -0400, Anna Schumaker wrote:
> Hi Jeff,
> 
> Can you CC the freezer folks on this patch during the second
> posting?  They'll probably need to at send an ACK for the changes to
> their include file.
> 
> Thanks,
> Anna
> 

Will do...

OTOH, now that I look, I'm not sure we really need a *_unsafe variant
here at all.

File locking is somewhat different in that we're expected to sleep in
the kernel, and I don't think we hold any kernel locks that are the
usual worry here (i_rwsem and the like). So we might can just use
freezable_schedule_timeout here.

I'll see if I can experiment with that before the next posting. It'd be
nice to eliminate one of the unsafe sleep callsites.

> On 09/06/2016 11:12 AM, Jeff Layton wrote:
> > 
> > We actually want to use TASK_INTERRUPTIBLE sleeps here. Once the
> > task
> > wakes up, if there is a signal pending then we'll be returning an
> > error
> > anyway. So, we might as well wake up immediately for non-fatal
> > signals
> > as well. That allows us to return to userland more quickly in that
> > case,
> > but won't change the error that userland sees.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/nfs/nfs4proc.c       |  3 ++-
> >  include/linux/freezer.h | 13 +++++++++++++
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index e3bf95369daf..e9232d71bc64 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -5537,7 +5537,8 @@ int nfs4_proc_delegreturn(struct inode
> > *inode, struct rpc_cred *cred, const nfs4
> >  static unsigned long
> >  nfs4_set_lock_task_retry(unsigned long timeout)
> >  {
> > -	freezable_schedule_timeout_killable_unsafe(timeout);
> > +	set_current_state(TASK_INTERRUPTIBLE);
> > +	freezable_schedule_timeout_unsafe(timeout);
> >  	timeout <<= 1;
> >  	if (timeout > NFS4_LOCK_MAXTIMEOUT)
> >  		return NFS4_LOCK_MAXTIMEOUT;
> > diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> > index dd03e837ebb7..fe31601e7f55 100644
> > --- a/include/linux/freezer.h
> > +++ b/include/linux/freezer.h
> > @@ -197,6 +197,19 @@ static inline long
> > freezable_schedule_timeout(long timeout)
> >   * Like schedule_timeout_interruptible(), but should not block the
> > freezer.  Do not
> >   * call this with locks held.
> >   */
> > +static inline long freezable_schedule_timeout_unsafe(long timeout)
> > +{
> > +	long __retval;
> > +	freezer_do_not_count();
> > +	__retval = schedule_timeout(timeout);
> > +	freezer_count_unsafe();
> > +	return __retval;
> > +}
> > +
> > +/*
> > + * Like schedule_timeout_interruptible(), but should not block the
> > freezer.  Do not
> > + * call this with locks held.
> > + */
> >  static inline long freezable_schedule_timeout_interruptible(long
> > timeout)
> >  {
> >  	long __retval;
> > 
> 

-- 
Jeff Layton <jlayton@redhat.com>

[PATCH 5/9] nfs: add handling for CB_NOTIFY_LOCK in client

[Jeff Layton]

For now, the callback doesn't do anything. Support for that will be
added in later patches.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/callback.h      |  8 ++++++++
 fs/nfs/callback_proc.c | 19 +++++++++++++++++++
 fs/nfs/callback_xdr.c  | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/callback.h b/fs/nfs/callback.h
index 5fe1cecbf9f0..b486848306f0 100644
--- a/fs/nfs/callback.h
+++ b/fs/nfs/callback.h
@@ -179,6 +179,14 @@ extern __be32 nfs4_callback_devicenotify(
 	struct cb_devicenotifyargs *args,
 	void *dummy, struct cb_process_state *cps);
 
+struct cb_notify_lock_args {
+	struct nfs_fh			cbnl_fh;
+	struct nfs_lowner		cbnl_owner;
+};
+
+extern __be32 nfs4_callback_notify_lock(struct cb_notify_lock_args *args,
+					 void *dummy,
+					 struct cb_process_state *cps);
 #endif /* CONFIG_NFS_V4_1 */
 extern int check_gss_callback_principal(struct nfs_client *, struct svc_rqst *);
 extern __be32 nfs4_callback_getattr(struct cb_getattrargs *args,
diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index f953ef6b2f2e..a211af339f32 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -628,4 +628,23 @@ out:
 	dprintk("%s: exit with status = %d\n", __func__, ntohl(status));
 	return status;
 }
+
+__be32 nfs4_callback_notify_lock(struct cb_notify_lock_args *args, void *dummy,
+				 struct cb_process_state *cps)
+{
+	struct nfs4_slot_table	*fc_tbl;
+	__be32			status;
+
+	status = htonl(NFS4ERR_OP_NOT_IN_SESSION);
+	if (!cps->clp) /* set in cb_sequence */
+		return status;
+
+	dprintk_rcu("NFS: CB_RECALL_NOTIFY_LOCK request from %s\n",
+		rpc_peeraddr2str(cps->clp->cl_rpcclient, RPC_DISPLAY_ADDR));
+
+	fc_tbl = &cps->clp->cl_session->fc_slot_table;
+
+	status = htonl(NFS4_OK);
+	return status;
+}
 #endif /* CONFIG_NFS_V4_1 */
diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
index d6a40c06ec26..d3c0485cf437 100644
--- a/fs/nfs/callback_xdr.c
+++ b/fs/nfs/callback_xdr.c
@@ -35,6 +35,7 @@
 					 (1 + 3) * 4) // seqid, 3 slotids
 #define CB_OP_RECALLANY_RES_MAXSZ	(CB_OP_HDR_RES_MAXSZ)
 #define CB_OP_RECALLSLOT_RES_MAXSZ	(CB_OP_HDR_RES_MAXSZ)
+#define CB_OP_NOTIFY_LOCK_RES_MAXSZ	(CB_OP_HDR_RES_MAXSZ)
 #endif /* CONFIG_NFS_V4_1 */
 
 #define NFSDBG_FACILITY NFSDBG_CALLBACK
@@ -534,6 +535,47 @@ static __be32 decode_recallslot_args(struct svc_rqst *rqstp,
 	return 0;
 }
 
+static __be32 decode_lockowner(struct xdr_stream *xdr, struct cb_notify_lock_args *args)
+{
+	__be32		*p;
+	unsigned int	len;
+
+	p = read_buf(xdr, 12);
+	if (unlikely(p == NULL))
+		return htonl(NFS4ERR_BADXDR);
+
+	p = xdr_decode_hyper(p, &args->cbnl_owner.clientid);
+	len = be32_to_cpu(*p);
+
+	p = read_buf(xdr, len);
+	if (unlikely(p == NULL))
+		return htonl(NFS4ERR_BADXDR);
+
+	/* Only try to decode if the length is right */
+	if (len == 20) {
+		p += 2;	/* skip "lock id:" */
+		args->cbnl_owner.s_dev = be32_to_cpu(*p++);
+		xdr_decode_hyper(p, &args->cbnl_owner.id);
+	} else {
+		args->cbnl_owner.s_dev = 0;
+		args->cbnl_owner.id = 0;
+	}
+	return 0;
+}
+
+static __be32 decode_notify_lock_args(struct svc_rqst *rqstp, struct xdr_stream *xdr, struct cb_notify_lock_args *args)
+{
+	__be32 status;
+
+	status = decode_fh(xdr, &args->cbnl_fh);
+	if (unlikely(status != 0))
+		goto out;
+	status = decode_lockowner(xdr, args);
+out:
+	dprintk("%s: exit with status = %d\n", __func__, ntohl(status));
+	return status;
+}
+
 #endif /* CONFIG_NFS_V4_1 */
 
 static __be32 encode_string(struct xdr_stream *xdr, unsigned int len, const char *str)
@@ -746,6 +788,7 @@ preprocess_nfs41_op(int nop, unsigned int op_nr, struct callback_op **op)
 	case OP_CB_RECALL_SLOT:
 	case OP_CB_LAYOUTRECALL:
 	case OP_CB_NOTIFY_DEVICEID:
+	case OP_CB_NOTIFY_LOCK:
 		*op = &callback_ops[op_nr];
 		break;
 
@@ -753,7 +796,6 @@ preprocess_nfs41_op(int nop, unsigned int op_nr, struct callback_op **op)
 	case OP_CB_PUSH_DELEG:
 	case OP_CB_RECALLABLE_OBJ_AVAIL:
 	case OP_CB_WANTS_CANCELLED:
-	case OP_CB_NOTIFY_LOCK:
 		return htonl(NFS4ERR_NOTSUPP);
 
 	default:
@@ -1006,6 +1048,11 @@ static struct callback_op callback_ops[] = {
 		.decode_args = (callback_decode_arg_t)decode_recallslot_args,
 		.res_maxsize = CB_OP_RECALLSLOT_RES_MAXSZ,
 	},
+	[OP_CB_NOTIFY_LOCK] = {
+		.process_op = (callback_process_op_t)nfs4_callback_notify_lock,
+		.decode_args = (callback_decode_arg_t)decode_notify_lock_args,
+		.res_maxsize = CB_OP_NOTIFY_LOCK_RES_MAXSZ,
+	},
 #endif /* CONFIG_NFS_V4_1 */
 };
 
-- 
2.7.4

Re: [PATCH 5/9] nfs: add handling for CB_NOTIFY_LOCK in client

[Anna Schumaker]

Hi Jeff,

On 09/06/2016 11:12 AM, Jeff Layton wrote:
> For now, the callback doesn't do anything. Support for that will be
> added in later patches.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/nfs/callback.h      |  8 ++++++++
>  fs/nfs/callback_proc.c | 19 +++++++++++++++++++
>  fs/nfs/callback_xdr.c  | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/callback.h b/fs/nfs/callback.h
> index 5fe1cecbf9f0..b486848306f0 100644
> --- a/fs/nfs/callback.h
> +++ b/fs/nfs/callback.h
> @@ -179,6 +179,14 @@ extern __be32 nfs4_callback_devicenotify(
>  	struct cb_devicenotifyargs *args,
>  	void *dummy, struct cb_process_state *cps);
>  
> +struct cb_notify_lock_args {
> +	struct nfs_fh			cbnl_fh;
> +	struct nfs_lowner		cbnl_owner;
> +};
> +
> +extern __be32 nfs4_callback_notify_lock(struct cb_notify_lock_args *args,
> +					 void *dummy,
> +					 struct cb_process_state *cps);
>  #endif /* CONFIG_NFS_V4_1 */
>  extern int check_gss_callback_principal(struct nfs_client *, struct svc_rqst *);
>  extern __be32 nfs4_callback_getattr(struct cb_getattrargs *args,
> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
> index f953ef6b2f2e..a211af339f32 100644
> --- a/fs/nfs/callback_proc.c
> +++ b/fs/nfs/callback_proc.c
> @@ -628,4 +628,23 @@ out:
>  	dprintk("%s: exit with status = %d\n", __func__, ntohl(status));
>  	return status;
>  }
> +
> +__be32 nfs4_callback_notify_lock(struct cb_notify_lock_args *args, void *dummy,
> +				 struct cb_process_state *cps)
> +{
> +	struct nfs4_slot_table	*fc_tbl;
> +	__be32			status;
> +
> +	status = htonl(NFS4ERR_OP_NOT_IN_SESSION);
> +	if (!cps->clp) /* set in cb_sequence */
> +		return status;

Maybe we can "return hotnl(NFS4ERR_OP_NOT_IN_SESSION)" directly, rather than going through the status variable?

> +
> +	dprintk_rcu("NFS: CB_RECALL_NOTIFY_LOCK request from %s\n",
> +		rpc_peeraddr2str(cps->clp->cl_rpcclient, RPC_DISPLAY_ADDR));
> +
> +	fc_tbl = &cps->clp->cl_session->fc_slot_table;

Looks like you never do anything with the fc_tbl variable in this function.  Can you remove it?

> +
> +	status = htonl(NFS4_OK);

And maybe just "return htonl(NFS4_OK)" directly here, too?

Thanks,
Anna

> +	return status;
> +}
>  #endif /* CONFIG_NFS_V4_1 */
> diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
> index d6a40c06ec26..d3c0485cf437 100644
> --- a/fs/nfs/callback_xdr.c
> +++ b/fs/nfs/callback_xdr.c
> @@ -35,6 +35,7 @@
>  					 (1 + 3) * 4) // seqid, 3 slotids
>  #define CB_OP_RECALLANY_RES_MAXSZ	(CB_OP_HDR_RES_MAXSZ)
>  #define CB_OP_RECALLSLOT_RES_MAXSZ	(CB_OP_HDR_RES_MAXSZ)
> +#define CB_OP_NOTIFY_LOCK_RES_MAXSZ	(CB_OP_HDR_RES_MAXSZ)
>  #endif /* CONFIG_NFS_V4_1 */
>  
>  #define NFSDBG_FACILITY NFSDBG_CALLBACK
> @@ -534,6 +535,47 @@ static __be32 decode_recallslot_args(struct svc_rqst *rqstp,
>  	return 0;
>  }
>  
> +static __be32 decode_lockowner(struct xdr_stream *xdr, struct cb_notify_lock_args *args)
> +{
> +	__be32		*p;
> +	unsigned int	len;
> +
> +	p = read_buf(xdr, 12);
> +	if (unlikely(p == NULL))
> +		return htonl(NFS4ERR_BADXDR);
> +
> +	p = xdr_decode_hyper(p, &args->cbnl_owner.clientid);
> +	len = be32_to_cpu(*p);
> +
> +	p = read_buf(xdr, len);
> +	if (unlikely(p == NULL))
> +		return htonl(NFS4ERR_BADXDR);
> +
> +	/* Only try to decode if the length is right */
> +	if (len == 20) {
> +		p += 2;	/* skip "lock id:" */
> +		args->cbnl_owner.s_dev = be32_to_cpu(*p++);
> +		xdr_decode_hyper(p, &args->cbnl_owner.id);
> +	} else {
> +		args->cbnl_owner.s_dev = 0;
> +		args->cbnl_owner.id = 0;
> +	}
> +	return 0;
> +}
> +
> +static __be32 decode_notify_lock_args(struct svc_rqst *rqstp, struct xdr_stream *xdr, struct cb_notify_lock_args *args)
> +{
> +	__be32 status;
> +
> +	status = decode_fh(xdr, &args->cbnl_fh);
> +	if (unlikely(status != 0))
> +		goto out;
> +	status = decode_lockowner(xdr, args);
> +out:
> +	dprintk("%s: exit with status = %d\n", __func__, ntohl(status));
> +	return status;
> +}
> +
>  #endif /* CONFIG_NFS_V4_1 */
>  
>  static __be32 encode_string(struct xdr_stream *xdr, unsigned int len, const char *str)
> @@ -746,6 +788,7 @@ preprocess_nfs41_op(int nop, unsigned int op_nr, struct callback_op **op)
>  	case OP_CB_RECALL_SLOT:
>  	case OP_CB_LAYOUTRECALL:
>  	case OP_CB_NOTIFY_DEVICEID:
> +	case OP_CB_NOTIFY_LOCK:
>  		*op = &callback_ops[op_nr];
>  		break;
>  
> @@ -753,7 +796,6 @@ preprocess_nfs41_op(int nop, unsigned int op_nr, struct callback_op **op)
>  	case OP_CB_PUSH_DELEG:
>  	case OP_CB_RECALLABLE_OBJ_AVAIL:
>  	case OP_CB_WANTS_CANCELLED:
> -	case OP_CB_NOTIFY_LOCK:
>  		return htonl(NFS4ERR_NOTSUPP);
>  
>  	default:
> @@ -1006,6 +1048,11 @@ static struct callback_op callback_ops[] = {
>  		.decode_args = (callback_decode_arg_t)decode_recallslot_args,
>  		.res_maxsize = CB_OP_RECALLSLOT_RES_MAXSZ,
>  	},
> +	[OP_CB_NOTIFY_LOCK] = {
> +		.process_op = (callback_process_op_t)nfs4_callback_notify_lock,
> +		.decode_args = (callback_decode_arg_t)decode_notify_lock_args,
> +		.res_maxsize = CB_OP_NOTIFY_LOCK_RES_MAXSZ,
> +	},
>  #endif /* CONFIG_NFS_V4_1 */
>  };
>  
> 

[PATCH 6/9] nfs: move nfs4_set_lock_state call into caller

[Jeff Layton]

We need to have this info set up before adding the waiter to the
waitqueue, so move this out of the _nfs4_proc_setlk and into the
caller. That's more efficient anyway since we don't need to do
this more than once if we end up waiting on the lock.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/nfs4proc.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index e9232d71bc64..5573f19539a6 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6134,14 +6134,8 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
 	struct nfs_inode *nfsi = NFS_I(state->inode);
 	struct nfs4_state_owner *sp = state->owner;
 	unsigned char fl_flags = request->fl_flags;
-	int status = -ENOLCK;
+	int status;
 
-	if (!test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
-		goto out;
-	/* Is this a delegated open? */
-	status = nfs4_set_lock_state(state, request);
-	if (status != 0)
-		goto out;
 	request->fl_flags |= FL_ACCESS;
 	status = locks_lock_inode_wait(state->inode, request);
 	if (status < 0)
@@ -6215,6 +6209,10 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
 
 	if (state == NULL)
 		return -ENOLCK;
+
+	if (!test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
+		return -ENOLCK;
+
 	/*
 	 * Don't rely on the VFS having checked the file open mode,
 	 * since it won't do this for flock() locks.
@@ -6229,6 +6227,10 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
 			return -EBADF;
 	}
 
+	status = nfs4_set_lock_state(state, request);
+	if (status != 0)
+		return status;
+
 	do {
 		status = nfs4_proc_setlk(state, cmd, request);
 		if ((status != -EAGAIN) || IS_SETLK(cmd))
-- 
2.7.4

Re: [PATCH 6/9] nfs: move nfs4_set_lock_state call into caller

[Anna Schumaker]

Hi Jeff,

On 09/06/2016 11:12 AM, Jeff Layton wrote:
> We need to have this info set up before adding the waiter to the
> waitqueue, so move this out of the _nfs4_proc_setlk and into the
> caller. That's more efficient anyway since we don't need to do
> this more than once if we end up waiting on the lock.

Looks like you're moving this outside of the state recovery retry loop, too.  Do we need to worry about lock state changing during state recovery?

Thanks,
Anna

> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/nfs/nfs4proc.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index e9232d71bc64..5573f19539a6 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -6134,14 +6134,8 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
>  	struct nfs_inode *nfsi = NFS_I(state->inode);
>  	struct nfs4_state_owner *sp = state->owner;
>  	unsigned char fl_flags = request->fl_flags;
> -	int status = -ENOLCK;
> +	int status;
>  
> -	if (!test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
> -		goto out;
> -	/* Is this a delegated open? */
> -	status = nfs4_set_lock_state(state, request);
> -	if (status != 0)
> -		goto out;
>  	request->fl_flags |= FL_ACCESS;
>  	status = locks_lock_inode_wait(state->inode, request);
>  	if (status < 0)
> @@ -6215,6 +6209,10 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
>  
>  	if (state == NULL)
>  		return -ENOLCK;
> +
> +	if (!test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
> +		return -ENOLCK;
> +
>  	/*
>  	 * Don't rely on the VFS having checked the file open mode,
>  	 * since it won't do this for flock() locks.
> @@ -6229,6 +6227,10 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
>  			return -EBADF;
>  	}
>  
> +	status = nfs4_set_lock_state(state, request);
> +	if (status != 0)
> +		return status;
> +
>  	do {
>  		status = nfs4_proc_setlk(state, cmd, request);
>  		if ((status != -EAGAIN) || IS_SETLK(cmd))
> 

Re: [PATCH 6/9] nfs: move nfs4_set_lock_state call into caller

[Jeff Layton]

On Thu, 2016-09-08 at 15:47 -0400, Anna Schumaker wrote:
> Hi Jeff,
> 
> On 09/06/2016 11:12 AM, Jeff Layton wrote:
> > 
> > We need to have this info set up before adding the waiter to the
> > waitqueue, so move this out of the _nfs4_proc_setlk and into the
> > caller. That's more efficient anyway since we don't need to do
> > this more than once if we end up waiting on the lock.
> 
> Looks like you're moving this outside of the state recovery retry
> loop, too.  Do we need to worry about lock state changing during
> state recovery?
> 
> Thanks,
> Anna
> 

I'm not sure I understand. _nfs4_proc_setlk and nfs4_proc_setlk each
only have one caller so there are no cases where we'd call these
functions and not call nfs4_set_lock_state first.

The first thing that nfs4_set_lock_state does is this:

        if (fl->fl_ops != NULL)
                return 0;

So it's a no-op on every subsequent attempt.

> > 
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/nfs/nfs4proc.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index e9232d71bc64..5573f19539a6 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -6134,14 +6134,8 @@ static int _nfs4_proc_setlk(struct
> > nfs4_state *state, int cmd, struct file_lock
> >  	struct nfs_inode *nfsi = NFS_I(state->inode);
> >  	struct nfs4_state_owner *sp = state->owner;
> >  	unsigned char fl_flags = request->fl_flags;
> > -	int status = -ENOLCK;
> > +	int status;
> >  
> > -	if (!test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
> > -		goto out;
> > -	/* Is this a delegated open? */
> > -	status = nfs4_set_lock_state(state, request);
> > -	if (status != 0)
> > -		goto out;
> >  	request->fl_flags |= FL_ACCESS;
> >  	status = locks_lock_inode_wait(state->inode, request);
> >  	if (status < 0)
> > @@ -6215,6 +6209,10 @@ nfs4_proc_lock(struct file *filp, int cmd,
> > struct file_lock *request)
> >  
> >  	if (state == NULL)
> >  		return -ENOLCK;
> > +
> > +	if (!test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
> > +		return -ENOLCK;
> > +
> >  	/*
> >  	 * Don't rely on the VFS having checked the file open
> > mode,
> >  	 * since it won't do this for flock() locks.
> > @@ -6229,6 +6227,10 @@ nfs4_proc_lock(struct file *filp, int cmd,
> > struct file_lock *request)
> >  			return -EBADF;
> >  	}
> >  
> > +	status = nfs4_set_lock_state(state, request);
> > +	if (status != 0)
> > +		return status;
> > +
> >  	do {
> >  		status = nfs4_proc_setlk(state, cmd, request);
> >  		if ((status != -EAGAIN) || IS_SETLK(cmd))
> > 
> 

-- 
Jeff Layton <jlayton@redhat.com>

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

[Jeff Layton]

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);
 		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.
-- 
2.7.4

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

[Anna Schumaker]

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

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

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

[Jeff Layton]

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>

[PATCH 8/9] nfs: ensure that the filehandle in CB_NOTIFY_LOCK request matches the inode

[Jeff Layton]

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/callback_proc.c |  2 +-
 fs/nfs/nfs4proc.c      | 13 +++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index 4ba6a8763f91..39a34d5083fe 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -645,7 +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);
+	__wake_up(&cps->clp->cl_lock_waitq, TASK_NORMAL, 0, args);
 	return status;
 }
 #endif /* CONFIG_NFS_V4_1 */
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 3a6669063c44..6829b998776d 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5533,6 +5533,7 @@ int nfs4_proc_delegreturn(struct inode *inode, struct rpc_cred *cred, const nfs4
 
 struct nfs4_lock_waiter {
 	struct task_struct	*task;
+	struct inode		*inode;
 	struct nfs_lowner	*owner;
 	bool			notified;
 };
@@ -5541,8 +5542,10 @@ static int
 nfs4_wake_lock_waiter(wait_queue_t *wait, unsigned int mode, int flags, void *key)
 {
 	int ret;
+	struct cb_notify_lock_args *cbnl = key;
 	struct nfs4_lock_waiter	*waiter	= wait->private;
-	struct nfs_lowner	*lowner = key, *wowner = waiter->owner;
+	struct nfs_lowner	*lowner = &cbnl->cbnl_owner,
+				*wowner = waiter->owner;
 
 	/* Don't wake anybody if the string looked bogus */
 	if (!lowner->id && !lowner->s_dev)
@@ -5554,6 +5557,10 @@ nfs4_wake_lock_waiter(wait_queue_t *wait, unsigned int mode, int flags, void *ke
 	    lowner->s_dev != wowner->s_dev)
 		return 0;
 
+	/* Make sure it's for the right inode */
+	if (nfs_compare_fh(NFS_FH(waiter->inode), &cbnl->cbnl_fh))
+		return 0;
+
 	waiter->notified = true;
 
 	/* override "private" so we can use default_wake_function */
@@ -6274,12 +6281,14 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
 
 	do {
 		struct nfs4_lock_state *lsp = request->fl_u.nfs4_fl.owner;
-		struct nfs_server *server = NFS_SERVER(lsp->ls_state->inode);
+		struct inode *inode = lsp->ls_state->inode;
+		struct nfs_server *server = NFS_SERVER(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,
+						   .inode = inode,
 						   .owner = &owner,
 						   .notified = false };
 		wait_queue_t wait;
-- 
2.7.4

Re: [PATCH 8/9] nfs: ensure that the filehandle in CB_NOTIFY_LOCK request matches the inode

[Anna Schumaker]

Hi Jeff,

On 09/06/2016 11:12 AM, Jeff Layton wrote:
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/nfs/callback_proc.c |  2 +-
>  fs/nfs/nfs4proc.c      | 13 +++++++++++--
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
> index 4ba6a8763f91..39a34d5083fe 100644
> --- a/fs/nfs/callback_proc.c
> +++ b/fs/nfs/callback_proc.c
> @@ -645,7 +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);
> +	__wake_up(&cps->clp->cl_lock_waitq, TASK_NORMAL, 0, args);
>  	return status;
>  }
>  #endif /* CONFIG_NFS_V4_1 */
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 3a6669063c44..6829b998776d 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5533,6 +5533,7 @@ int nfs4_proc_delegreturn(struct inode *inode, struct rpc_cred *cred, const nfs4
>  
>  struct nfs4_lock_waiter {
>  	struct task_struct	*task;
> +	struct inode		*inode;
>  	struct nfs_lowner	*owner;
>  	bool			notified;
>  };
> @@ -5541,8 +5542,10 @@ static int
>  nfs4_wake_lock_waiter(wait_queue_t *wait, unsigned int mode, int flags, void *key)
>  {
>  	int ret;
> +	struct cb_notify_lock_args *cbnl = key;

I get this when I try compiling with CONFIG_NFS_V4=m but CONFIG_NFS_V4_1=n:

fs/nfs/nfs4proc.c: In function 'nfs4_wake_lock_waiter':
fs/nfs/nfs4proc.c:5491:35: error: dereferencing pointer to incomplete type 'struct cb_notify_lock_args'
  struct nfs_lowner *lowner = &cbnl->cbnl_owner,


Just thought you should know :)
Anna
                                   ^~
>  	struct nfs4_lock_waiter	*waiter	= wait->private;
> -	struct nfs_lowner	*lowner = key, *wowner = waiter->owner;
> +	struct nfs_lowner	*lowner = &cbnl->cbnl_owner,
> +				*wowner = waiter->owner;
>  
>  	/* Don't wake anybody if the string looked bogus */
>  	if (!lowner->id && !lowner->s_dev)
> @@ -5554,6 +5557,10 @@ nfs4_wake_lock_waiter(wait_queue_t *wait, unsigned int mode, int flags, void *ke
>  	    lowner->s_dev != wowner->s_dev)
>  		return 0;
>  
> +	/* Make sure it's for the right inode */
> +	if (nfs_compare_fh(NFS_FH(waiter->inode), &cbnl->cbnl_fh))
> +		return 0;
> +
>  	waiter->notified = true;
>  
>  	/* override "private" so we can use default_wake_function */
> @@ -6274,12 +6281,14 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
>  
>  	do {
>  		struct nfs4_lock_state *lsp = request->fl_u.nfs4_fl.owner;
> -		struct nfs_server *server = NFS_SERVER(lsp->ls_state->inode);
> +		struct inode *inode = lsp->ls_state->inode;
> +		struct nfs_server *server = NFS_SERVER(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,
> +						   .inode = inode,
>  						   .owner = &owner,
>  						   .notified = false };
>  		wait_queue_t wait;
> 

Re: [PATCH 8/9] nfs: ensure that the filehandle in CB_NOTIFY_LOCK request matches the inode

[Jeff Layton]

On Thu, 2016-09-08 at 16:07 -0400, Anna Schumaker wrote:
> Hi Jeff,
> 
> On 09/06/2016 11:12 AM, Jeff Layton wrote:
> > 
> > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/nfs/callback_proc.c |  2 +-
> >  fs/nfs/nfs4proc.c      | 13 +++++++++++--
> >  2 files changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
> > index 4ba6a8763f91..39a34d5083fe 100644
> > --- a/fs/nfs/callback_proc.c
> > +++ b/fs/nfs/callback_proc.c
> > @@ -645,7 +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);
> > > > +	__wake_up(&cps->clp->cl_lock_waitq, TASK_NORMAL, 0, args);
> > > >  	return status;
> >  }
> >  #endif /* CONFIG_NFS_V4_1 */
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 3a6669063c44..6829b998776d 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -5533,6 +5533,7 @@ int nfs4_proc_delegreturn(struct inode *inode, struct rpc_cred *cred, const nfs4
> >  
> >  struct nfs4_lock_waiter {
> > > > > >  	struct task_struct	*task;
> > > > > > +	struct inode		*inode;
> > > > > >  	struct nfs_lowner	*owner;
> > > > > >  	bool			notified;
> >  };
> > @@ -5541,8 +5542,10 @@ static int
> >  nfs4_wake_lock_waiter(wait_queue_t *wait, unsigned int mode, int flags, void *key)
> >  {
> > > >  	int ret;
> > > > +	struct cb_notify_lock_args *cbnl = key;
> 
> I get this when I try compiling with CONFIG_NFS_V4=m but CONFIG_NFS_V4_1=n:
> 
> fs/nfs/nfs4proc.c: In function 'nfs4_wake_lock_waiter':
> fs/nfs/nfs4proc.c:5491:35: error: dereferencing pointer to incomplete type 'struct cb_notify_lock_args'
>   struct nfs_lowner *lowner = &cbnl->cbnl_owner,
> 
> 
> Just thought you should know :)
> Anna
>   

Ouch! Nice catch. I'll fix that and make sure I try compiling with
those config options as well.

>                                  ^~
> > 
> > > > > > > >  	struct nfs4_lock_waiter	*waiter	= wait->private;
> > > > > > -	struct nfs_lowner	*lowner = key, *wowner = waiter->owner;
> > > > > > +	struct nfs_lowner	*lowner = &cbnl->cbnl_owner,
> > > > +				*wowner = waiter->owner;
> >  
> > > >  	/* Don't wake anybody if the string looked bogus */
> > > >  	if (!lowner->id && !lowner->s_dev)
> > @@ -5554,6 +5557,10 @@ nfs4_wake_lock_waiter(wait_queue_t *wait, unsigned int mode, int flags, void *ke
> > > >  	    lowner->s_dev != wowner->s_dev)
> > > >  		return 0;
> >  
> > > > +	/* Make sure it's for the right inode */
> > > > +	if (nfs_compare_fh(NFS_FH(waiter->inode), &cbnl->cbnl_fh))
> > > > +		return 0;
> > +
> > > >  	waiter->notified = true;
> >  
> > > >  	/* override "private" so we can use default_wake_function */
> > @@ -6274,12 +6281,14 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
> >  
> > > >  	do {
> > > >  		struct nfs4_lock_state *lsp = request->fl_u.nfs4_fl.owner;
> > > > -		struct nfs_server *server = NFS_SERVER(lsp->ls_state->inode);
> > > > +		struct inode *inode = lsp->ls_state->inode;
> > > > +		struct nfs_server *server = NFS_SERVER(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,
> > > > +						   .inode = inode,
> > > >  						   .owner = &owner,
> > > >  						   .notified = false };
> > > >  		wait_queue_t wait;
> > 
> 

-- 
Jeff Layton <jlayton@redhat.com>

[PATCH 9/9] nfs: track whether server sets MAY_NOTIFY_LOCK flag

[Jeff Layton]

If it does, then always have the client sleep for the max time before
repolling for the lock.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/nfs4_fs.h  | 1 +
 fs/nfs/nfs4proc.c | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index 9bf64eacba5b..91e4f135a5f2 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -156,6 +156,7 @@ enum {
 	NFS_STATE_RECLAIM_NOGRACE,	/* OPEN stateid needs to recover state */
 	NFS_STATE_POSIX_LOCKS,		/* Posix locks are supported */
 	NFS_STATE_RECOVERY_FAILED,	/* OPEN stateid state recovery failed */
+	NFS_STATE_MAY_NOTIFY_LOCK,	/* server may CB_NOTIFY_LOCK */
 };
 
 struct nfs4_state {
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 6829b998776d..ecd00431fe0c 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2537,6 +2537,8 @@ static int _nfs4_open_and_get_state(struct nfs4_opendata *opendata,
 		goto out;
 	if (server->caps & NFS_CAP_POSIX_LOCK)
 		set_bit(NFS_STATE_POSIX_LOCKS, &state->flags);
+	if (opendata->o_res.rflags & NFS4_OPEN_RESULT_MAY_NOTIFY_LOCK)
+		set_bit(NFS_STATE_MAY_NOTIFY_LOCK, &state->flags);
 
 	dentry = opendata->dentry;
 	if (d_really_is_negative(dentry)) {
@@ -6230,7 +6232,7 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
 {
 	struct nfs_open_context *ctx;
 	struct nfs4_state *state;
-	unsigned long timeout = NFS4_LOCK_MINTIMEOUT;
+	unsigned long timeout;
 	int status;
 
 	/* verify open state */
@@ -6279,6 +6281,9 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
 	if (status != 0)
 		return status;
 
+	timeout = test_bit(NFS_STATE_MAY_NOTIFY_LOCK, &state->flags) ?
+				NFS4_LOCK_MAXTIMEOUT : NFS4_LOCK_MINTIMEOUT;
+
 	do {
 		struct nfs4_lock_state *lsp = request->fl_u.nfs4_fl.owner;
 		struct inode *inode = lsp->ls_state->inode;
-- 
2.7.4

Re: [PATCH 9/9] nfs: track whether server sets MAY_NOTIFY_LOCK flag

[Anna Schumaker]

Hi Jeff,

On 09/06/2016 11:12 AM, Jeff Layton wrote:
> If it does, then always have the client sleep for the max time before
> repolling for the lock.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/nfs/nfs4_fs.h  | 1 +
>  fs/nfs/nfs4proc.c | 7 ++++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> index 9bf64eacba5b..91e4f135a5f2 100644
> --- a/fs/nfs/nfs4_fs.h
> +++ b/fs/nfs/nfs4_fs.h
> @@ -156,6 +156,7 @@ enum {
>  	NFS_STATE_RECLAIM_NOGRACE,	/* OPEN stateid needs to recover state */
>  	NFS_STATE_POSIX_LOCKS,		/* Posix locks are supported */
>  	NFS_STATE_RECOVERY_FAILED,	/* OPEN stateid state recovery failed */
> +	NFS_STATE_MAY_NOTIFY_LOCK,	/* server may CB_NOTIFY_LOCK */
>  };
>  
>  struct nfs4_state {
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 6829b998776d..ecd00431fe0c 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2537,6 +2537,8 @@ static int _nfs4_open_and_get_state(struct nfs4_opendata *opendata,
>  		goto out;
>  	if (server->caps & NFS_CAP_POSIX_LOCK)
>  		set_bit(NFS_STATE_POSIX_LOCKS, &state->flags);
> +	if (opendata->o_res.rflags & NFS4_OPEN_RESULT_MAY_NOTIFY_LOCK)
                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Where does the NFS4_OPEN_RESULT_MAY_NOTIFY_LOCK flag come from?  `git grep` isn't finding it in my source directory.

Thanks,
Anna


> +		set_bit(NFS_STATE_MAY_NOTIFY_LOCK, &state->flags);
>  
>  	dentry = opendata->dentry;
>  	if (d_really_is_negative(dentry)) {
> @@ -6230,7 +6232,7 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
>  {
>  	struct nfs_open_context *ctx;
>  	struct nfs4_state *state;
> -	unsigned long timeout = NFS4_LOCK_MINTIMEOUT;
> +	unsigned long timeout;
>  	int status;
>  
>  	/* verify open state */
> @@ -6279,6 +6281,9 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
>  	if (status != 0)
>  		return status;
>  
> +	timeout = test_bit(NFS_STATE_MAY_NOTIFY_LOCK, &state->flags) ?
> +				NFS4_LOCK_MAXTIMEOUT : NFS4_LOCK_MINTIMEOUT;
> +
>  	do {
>  		struct nfs4_lock_state *lsp = request->fl_u.nfs4_fl.owner;
>  		struct inode *inode = lsp->ls_state->inode;
> 

Re: [PATCH 9/9] nfs: track whether server sets MAY_NOTIFY_LOCK flag

[Jeff Layton]

On Thu, 2016-09-08 at 16:15 -0400, Anna Schumaker wrote:
> Hi Jeff,
> 
> On 09/06/2016 11:12 AM, Jeff Layton wrote:
> > 
> > If it does, then always have the client sleep for the max time
> > before
> > repolling for the lock.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/nfs/nfs4_fs.h  | 1 +
> >  fs/nfs/nfs4proc.c | 7 ++++++-
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> > index 9bf64eacba5b..91e4f135a5f2 100644
> > --- a/fs/nfs/nfs4_fs.h
> > +++ b/fs/nfs/nfs4_fs.h
> > @@ -156,6 +156,7 @@ enum {
> >  	NFS_STATE_RECLAIM_NOGRACE,	/* OPEN stateid needs to
> > recover state */
> >  	NFS_STATE_POSIX_LOCKS,		/* Posix locks are
> > supported */
> >  	NFS_STATE_RECOVERY_FAILED,	/* OPEN stateid state
> > recovery failed */
> > +	NFS_STATE_MAY_NOTIFY_LOCK,	/* server may
> > CB_NOTIFY_LOCK */
> >  };
> >  
> >  struct nfs4_state {
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 6829b998776d..ecd00431fe0c 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -2537,6 +2537,8 @@ static int _nfs4_open_and_get_state(struct
> > nfs4_opendata *opendata,
> >  		goto out;
> >  	if (server->caps & NFS_CAP_POSIX_LOCK)
> >  		set_bit(NFS_STATE_POSIX_LOCKS, &state->flags);
> > +	if (opendata->o_res.rflags &
> > NFS4_OPEN_RESULT_MAY_NOTIFY_LOCK)
>                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ^
> Where does the NFS4_OPEN_RESULT_MAY_NOTIFY_LOCK flag come from?  `git
> grep` isn't finding it in my source directory.
> 
> Thanks,
> Anna
> 

It's in:

    include/uapi/linux/nfs4.h

...which I think is already included here.

> 
> > 
> > +		set_bit(NFS_STATE_MAY_NOTIFY_LOCK, &state->flags);
> >  
> >  	dentry = opendata->dentry;
> >  	if (d_really_is_negative(dentry)) {
> > @@ -6230,7 +6232,7 @@ nfs4_proc_lock(struct file *filp, int cmd,
> > struct file_lock *request)
> >  {
> >  	struct nfs_open_context *ctx;
> >  	struct nfs4_state *state;
> > -	unsigned long timeout = NFS4_LOCK_MINTIMEOUT;
> > +	unsigned long timeout;
> >  	int status;
> >  
> >  	/* verify open state */
> > @@ -6279,6 +6281,9 @@ nfs4_proc_lock(struct file *filp, int cmd,
> > struct file_lock *request)
> >  	if (status != 0)
> >  		return status;
> >  
> > +	timeout = test_bit(NFS_STATE_MAY_NOTIFY_LOCK, &state-
> > >flags) ?
> > +				NFS4_LOCK_MAXTIMEOUT :
> > NFS4_LOCK_MINTIMEOUT;
> > +
> >  	do {
> >  		struct nfs4_lock_state *lsp = request-
> > >fl_u.nfs4_fl.owner;
> >  		struct inode *inode = lsp->ls_state->inode;
> > 
> 

-- 
Jeff Layton <jlayton@redhat.com>