| Message ID | 1473174760-29859-5-git-send-email-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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; > > >
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;
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(-)