diff mbox

[v3,8/9] nfs: move nfs4 lock retry attempt loop to a separate function

Message ID 1474057631-31209-9-git-send-email-jlayton@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jeff Layton Sept. 16, 2016, 8:27 p.m. UTC 
This also consolidates the waiting logic into a single function,
instead of having it spread across two like it is now.

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

Comments

Trond Myklebust Sept. 16, 2016, 9:20 p.m. UTC | #1 
> On Sep 16, 2016, at 16:27, Jeff Layton <jlayton@redhat.com> wrote:
> 
> This also consolidates the waiting logic into a single function,
> instead of having it spread across two like it is now.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> fs/nfs/nfs4proc.c | 51 ++++++++++++++++++++++++---------------------------
> 1 file changed, 24 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index c807850ac476..a7517abaf3c7 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5530,22 +5530,6 @@ int nfs4_proc_delegreturn(struct inode *inode, struct rpc_cred *cred, const nfs4
> 	return err;
> }
> 
> -#define NFS4_LOCK_MINTIMEOUT (1 * HZ)
> -#define NFS4_LOCK_MAXTIMEOUT (30 * HZ)
> -
> -/* 
> - * sleep, with exponential backoff, and retry the LOCK operation. 
> - */
> -static unsigned long
> -nfs4_set_lock_task_retry(unsigned long timeout)
> -{
> -	freezable_schedule_timeout_interruptible(timeout);
> -	timeout <<= 1;
> -	if (timeout > NFS4_LOCK_MAXTIMEOUT)
> -		return NFS4_LOCK_MAXTIMEOUT;
> -	return timeout;
> -}
> -
> static int _nfs4_proc_getlk(struct nfs4_state *state, int cmd, struct file_lock *request)
> {
> 	struct inode *inode = state->inode;
> @@ -6178,12 +6162,34 @@ static int nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock *
> 	return err;
> }
> 
> +#define NFS4_LOCK_MINTIMEOUT (1 * HZ)
> +#define NFS4_LOCK_MAXTIMEOUT (30 * HZ)
> +
> +static int
> +nfs4_retry_setlk(struct nfs4_state *state, int cmd, struct file_lock *request)
> +{
> +	int status;
> +	unsigned long timeout = NFS4_LOCK_MINTIMEOUT;
> +
> +	do {
> +		status = nfs4_proc_setlk(state, cmd, request);
> +		if ((status != -EAGAIN) || IS_SETLK(cmd))
> +			break;
> +		freezable_schedule_timeout_interruptible(timeout);
> +		timeout *= 2;
> +		timeout = min_t(unsigned long, NFS4_LOCK_MAXTIMEOUT, timeout);
> +		status = -ERESTARTSYS;
> +		if (signalled())
> +			break;
> +	} while(status < 0);

Can it ever be >= 0 here? Why not just use 'while (!signalled())'?

> +	return status;
> +}
> +
> static int
> 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;
> 	int status;
> 
> 	/* verify open state */
> @@ -6232,16 +6238,7 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
> 	if (status != 0)
> 		return status;
> 
> -	do {
> -		status = nfs4_proc_setlk(state, cmd, request);
> -		if ((status != -EAGAIN) || IS_SETLK(cmd))
> -			break;
> -		timeout = nfs4_set_lock_task_retry(timeout);
> -		status = -ERESTARTSYS;
> -		if (signalled())
> -			break;
> -	} while(status < 0);
> -	return status;
> +	return nfs4_retry_setlk(state, cmd, request);
> }
> 
> int nfs4_lock_delegation_recall(struct file_lock *fl, struct nfs4_state *state, const nfs4_stateid *stateid)
> -- 
> 2.7.4
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Layton Sept. 16, 2016, 9:47 p.m. UTC | #2 
On Fri, 2016-09-16 at 21:20 +0000, Trond Myklebust wrote:
> > 
> > On Sep 16, 2016, at 16:27, Jeff Layton <jlayton@redhat.com> wrote:
> > 
> > This also consolidates the waiting logic into a single function,
> > instead of having it spread across two like it is now.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> > fs/nfs/nfs4proc.c | 51 ++++++++++++++++++++++++------------------
> > ---------
> > 1 file changed, 24 insertions(+), 27 deletions(-)
> > 
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index c807850ac476..a7517abaf3c7 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -5530,22 +5530,6 @@ int nfs4_proc_delegreturn(struct inode
> > *inode, struct rpc_cred *cred, const nfs4
> > 	return err;
> > }
> > 
> > -#define NFS4_LOCK_MINTIMEOUT (1 * HZ)
> > -#define NFS4_LOCK_MAXTIMEOUT (30 * HZ)
> > -
> > -/* 
> > - * sleep, with exponential backoff, and retry the LOCK operation. 
> > - */
> > -static unsigned long
> > -nfs4_set_lock_task_retry(unsigned long timeout)
> > -{
> > -	freezable_schedule_timeout_interruptible(timeout);
> > -	timeout <<= 1;
> > -	if (timeout > NFS4_LOCK_MAXTIMEOUT)
> > -		return NFS4_LOCK_MAXTIMEOUT;
> > -	return timeout;
> > -}
> > -
> > static int _nfs4_proc_getlk(struct nfs4_state *state, int cmd,
> > struct file_lock *request)
> > {
> > 	struct inode *inode = state->inode;
> > @@ -6178,12 +6162,34 @@ static int nfs4_proc_setlk(struct
> > nfs4_state *state, int cmd, struct file_lock *
> > 	return err;
> > }
> > 
> > +#define NFS4_LOCK_MINTIMEOUT (1 * HZ)
> > +#define NFS4_LOCK_MAXTIMEOUT (30 * HZ)
> > +
> > +static int
> > +nfs4_retry_setlk(struct nfs4_state *state, int cmd, struct
> > file_lock *request)
> > +{
> > +	int status;
> > +	unsigned long timeout = NFS4_LOCK_MINTIMEOUT;
> > +
> > +	do {
> > +		status = nfs4_proc_setlk(state, cmd, request);
> > +		if ((status != -EAGAIN) || IS_SETLK(cmd))
> > +			break;
> > +		freezable_schedule_timeout_interruptible(timeout);
> > +		timeout *= 2;
> > +		timeout = min_t(unsigned long,
> > NFS4_LOCK_MAXTIMEOUT, timeout);
> > +		status = -ERESTARTSYS;
> > +		if (signalled())
> > +			break;
> > +	} while(status < 0);
> 
> Can it ever be >= 0 here? Why not just use 'while (!signalled())'?
> 


Good point. I'll make that change.

Thanks,

> > 
> > +	return status;
> > +}
> > +
> > static int
> > 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;
> > 	int status;
> > 
> > 	/* verify open state */
> > @@ -6232,16 +6238,7 @@ nfs4_proc_lock(struct file *filp, int cmd,
> > struct file_lock *request)
> > 	if (status != 0)
> > 		return status;
> > 
> > -	do {
> > -		status = nfs4_proc_setlk(state, cmd, request);
> > -		if ((status != -EAGAIN) || IS_SETLK(cmd))
> > -			break;
> > -		timeout = nfs4_set_lock_task_retry(timeout);
> > -		status = -ERESTARTSYS;
> > -		if (signalled())
> > -			break;
> > -	} while(status < 0);
> > -	return status;
> > +	return nfs4_retry_setlk(state, cmd, request);
> > }
> > 
> > int nfs4_lock_delegation_recall(struct file_lock *fl, struct
> > nfs4_state *state, const nfs4_stateid *stateid)
> >
diff mbox

Patch

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index c807850ac476..a7517abaf3c7 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5530,22 +5530,6 @@  int nfs4_proc_delegreturn(struct inode *inode, struct rpc_cred *cred, const nfs4
 	return err;
 }
 
-#define NFS4_LOCK_MINTIMEOUT (1 * HZ)
-#define NFS4_LOCK_MAXTIMEOUT (30 * HZ)
-
-/* 
- * sleep, with exponential backoff, and retry the LOCK operation. 
- */
-static unsigned long
-nfs4_set_lock_task_retry(unsigned long timeout)
-{
-	freezable_schedule_timeout_interruptible(timeout);
-	timeout <<= 1;
-	if (timeout > NFS4_LOCK_MAXTIMEOUT)
-		return NFS4_LOCK_MAXTIMEOUT;
-	return timeout;
-}
-
 static int _nfs4_proc_getlk(struct nfs4_state *state, int cmd, struct file_lock *request)
 {
 	struct inode *inode = state->inode;
@@ -6178,12 +6162,34 @@  static int nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock *
 	return err;
 }
 
+#define NFS4_LOCK_MINTIMEOUT (1 * HZ)
+#define NFS4_LOCK_MAXTIMEOUT (30 * HZ)
+
+static int
+nfs4_retry_setlk(struct nfs4_state *state, int cmd, struct file_lock *request)
+{
+	int status;
+	unsigned long timeout = NFS4_LOCK_MINTIMEOUT;
+
+	do {
+		status = nfs4_proc_setlk(state, cmd, request);
+		if ((status != -EAGAIN) || IS_SETLK(cmd))
+			break;
+		freezable_schedule_timeout_interruptible(timeout);
+		timeout *= 2;
+		timeout = min_t(unsigned long, NFS4_LOCK_MAXTIMEOUT, timeout);
+		status = -ERESTARTSYS;
+		if (signalled())
+			break;
+	} while(status < 0);
+	return status;
+}
+
 static int
 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;
 	int status;
 
 	/* verify open state */
@@ -6232,16 +6238,7 @@  nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
 	if (status != 0)
 		return status;
 
-	do {
-		status = nfs4_proc_setlk(state, cmd, request);
-		if ((status != -EAGAIN) || IS_SETLK(cmd))
-			break;
-		timeout = nfs4_set_lock_task_retry(timeout);
-		status = -ERESTARTSYS;
-		if (signalled())
-			break;
-	} while(status < 0);
-	return status;
+	return nfs4_retry_setlk(state, cmd, request);
 }
 
 int nfs4_lock_delegation_recall(struct file_lock *fl, struct nfs4_state *state, const nfs4_stateid *stateid)