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

Message ID 1473174760-29859-5-git-send-email-jlayton@redhat.com
State New
Headers show

Commit Message

Jeff Layton Sept. 6, 2016, 3:12 p.m. UTC
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(-)

Comments

Jeff Layton Sept. 6, 2016, 4:39 p.m. UTC | #1
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.
Schumaker, Anna Sept. 8, 2016, 6:20 p.m. UTC | #2
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;
> 

--
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. 8, 2016, 6:36 p.m. UTC | #3
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;
> > 
>

Patch
diff mbox

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;