On Mon, Feb 12 2018, Patrick Farrell wrote: > It's worth noting that the change from -EINTR to -ERESTARTSYS will > modify the behavior of userspace slightly. Specifically, when a > signal handler is setup with retry set (SA_RESTART flag set), the > syscall will be restarted rather than aborted.
Thanks for the review.
This state is true if the error status ever gets all the way up to user
space. I don't think it does, though I haven't carefully audited every
l_wait_event_abortable() call site. I did look at a few and the return
value is only used locally in the same function.
I suspect I would have checked for error codes escaping when I wrote
the patch, but I don't have a clear memory.
Some sort of audit wouldn't hurt of course.
Thanks,
NeilBrown
>
> This should be fine. It may eventually shake out some stuble bugs in Lustre
> when we abort an operation and then restart it from a point where we didn't
> in the past - past instances where we changed from -EINTR to -ERESTARTSYS
> certainly have - but it's for the best. As I understand it from past
> conversations with Andreas and others, Lustre is not really intending to
> claim it's not restartable, it's just an artifact of people copying older
> code that was written without awareness of the difference in EINTR vs
> ERESTARTSYS.
>
> Ideally someone should go through and audit the remaining uses of -EINTR and
> replace most of them with -ERESTARTSYS.
>
> James, maybe you want to add that to the TODO list?
>
> On 2/12/18, 3:24 PM, "lustre-devel on behalf of NeilBrown"
> <lustre-devel-boun...@lists.lustre.org on behalf of ne...@suse.com> wrote:
>
> lustre sometimes wants to wait for an event, but abort if
> one of a specific list of signals arrives. This is a little
> bit like wait_event_killable(), except that the signals are
> identified a different way.
>
> So introduce l_wait_event_abortable() which provides this
> functionality.
> Having separate functions for separate needs is more in line
> with the pattern set by include/linux/wait.h, than having a
> single function which tries to include all possible needs.
>
> Also introduce l_wait_event_abortable_exclusive().
>
> Note that l_wait_event() return -EINTR on a signal, while
> Linux wait_event functions return -ERESTARTSYS.
> l_wait_event_{abortable_,}exclusive follow the Linux pattern.
>
> Reviewed-by: James Simmons <jsimm...@infradead.org>
> Signed-off-by: NeilBrown <ne...@suse.com>
> ---
> drivers/staging/lustre/lustre/include/lustre_lib.h | 24
> ++++++++++++++++++++
> drivers/staging/lustre/lustre/ldlm/ldlm_resource.c | 12 +++++-----
> drivers/staging/lustre/lustre/llite/llite_lib.c | 12 +++-------
> drivers/staging/lustre/lustre/obdclass/genops.c | 9 +++-----
> drivers/staging/lustre/lustre/obdclass/llog_obd.c | 5 ++--
> drivers/staging/lustre/lustre/osc/osc_page.c | 6 ++---
> drivers/staging/lustre/lustre/osc/osc_request.c | 6 ++---
> 7 files changed, 43 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/include/lustre_lib.h
> b/drivers/staging/lustre/lustre/include/lustre_lib.h
> index 7d950c53e962..b2a64d0e682c 100644
> --- a/drivers/staging/lustre/lustre/include/lustre_lib.h
> +++ b/drivers/staging/lustre/lustre/include/lustre_lib.h
> @@ -336,4 +336,28 @@ do {
> \
> /** @} lib */
>
>
> +
> +/* l_wait_event_abortable() is a bit like wait_event_killable()
> + * except there is a fixed set of signals which will abort:
> + * LUSTRE_FATAL_SIGS
> + */
> +#define l_wait_event_abortable(wq, condition)
> \
> +({
> \
> + sigset_t __blocked; \
> + int __ret = 0; \
> + __blocked = cfs_block_sigsinv(LUSTRE_FATAL_SIGS); \
> + __ret = wait_event_interruptible(wq, condition); \
> + cfs_restore_sigs(__blocked); \
> + __ret; \
> +})
> +
> +#define l_wait_event_abortable_exclusive(wq, condition)
> \
> +({
> \
> + sigset_t __blocked; \
> + int __ret = 0; \
> + __blocked = cfs_block_sigsinv(LUSTRE_FATAL_SIGS); \
> + __ret = wait_event_interruptible_exclusive(wq, condition); \
> + cfs_restore_sigs(__blocked); \
> + __ret; \
> +})
> #endif /* _LUSTRE_LIB_H */
> diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_resource.c
> b/drivers/staging/lustre/lustre/ldlm/ldlm_resource.c
> index 2e66825c8f4b..4c44603ab6f9 100644
> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_resource.c
> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_resource.c
> @@ -879,7 +879,6 @@ static int __ldlm_namespace_free(struct
> ldlm_namespace *ns, int force)
> ldlm_namespace_cleanup(ns, force ? LDLM_FL_LOCAL_ONLY : 0);
>
> if (atomic_read(&ns->ns_bref) > 0) {
> - struct l_wait_info lwi = LWI_INTR(LWI_ON_SIGNAL_NOOP, NULL);
> int rc;
>
> CDEBUG(D_DLMTRACE,
> @@ -887,11 +886,12 @@ static int __ldlm_namespace_free(struct
> ldlm_namespace *ns, int force)
> ldlm_ns_name(ns), atomic_read(&ns->ns_bref));
> force_wait:
> if (force)
> - lwi = LWI_TIMEOUT(msecs_to_jiffies(obd_timeout *
> - MSEC_PER_SEC) / 4, NULL, NULL);
> -
> - rc = l_wait_event(ns->ns_waitq,
> - atomic_read(&ns->ns_bref) == 0, &lwi);
> + rc = wait_event_idle_timeout(ns->ns_waitq,
> + atomic_read(&ns->ns_bref)
> == 0,
> + obd_timeout * HZ / 4) ? 0
> : -ETIMEDOUT;
> + else
> + rc = l_wait_event_abortable(ns->ns_waitq,
> + atomic_read(&ns->ns_bref)
> == 0);
>
> /* Forced cleanups should be able to reclaim all references,
> * so it's safe to wait forever... we can't leak locks...
> diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c
> b/drivers/staging/lustre/lustre/llite/llite_lib.c
> index c820b201af71..ccb614bd7f53 100644
> --- a/drivers/staging/lustre/lustre/llite/llite_lib.c
> +++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
> @@ -986,16 +986,12 @@ void ll_put_super(struct super_block *sb)
> }
>
> /* Wait for unstable pages to be committed to stable storage */
> - if (!force) {
> - struct l_wait_info lwi = LWI_INTR(LWI_ON_SIGNAL_NOOP, NULL);
> -
> - rc = l_wait_event(sbi->ll_cache->ccc_unstable_waitq,
> -
> !atomic_long_read(&sbi->ll_cache->ccc_unstable_nr),
> - &lwi);
> - }
> + if (!force)
> + rc = l_wait_event_abortable(sbi->ll_cache->ccc_unstable_waitq,
> +
> !atomic_long_read(&sbi->ll_cache->ccc_unstable_nr));
>
> ccc_count = atomic_long_read(&sbi->ll_cache->ccc_unstable_nr);
> - if (!force && rc != -EINTR)
> + if (!force && rc != -ERESTARTSYS)
> LASSERTF(!ccc_count, "count: %li\n", ccc_count);
>
> /* We need to set force before the lov_disconnect in
> diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c
> b/drivers/staging/lustre/lustre/obdclass/genops.c
> index 3ff25b8d3b48..8f776a4058a9 100644
> --- a/drivers/staging/lustre/lustre/obdclass/genops.c
> +++ b/drivers/staging/lustre/lustre/obdclass/genops.c
> @@ -1332,7 +1332,6 @@ static bool obd_request_slot_avail(struct
> client_obd *cli,
> int obd_get_request_slot(struct client_obd *cli)
> {
> struct obd_request_slot_waiter orsw;
> - struct l_wait_info lwi;
> int rc;
>
> spin_lock(&cli->cl_loi_list_lock);
> @@ -1347,11 +1346,9 @@ int obd_get_request_slot(struct client_obd *cli)
> orsw.orsw_signaled = false;
> spin_unlock(&cli->cl_loi_list_lock);
>
> - lwi = LWI_INTR(LWI_ON_SIGNAL_NOOP, NULL);
> - rc = l_wait_event(orsw.orsw_waitq,
> - obd_request_slot_avail(cli, &orsw) ||
> - orsw.orsw_signaled,
> - &lwi);
> + rc = l_wait_event_abortable(orsw.orsw_waitq,
> + obd_request_slot_avail(cli, &orsw) ||
> + orsw.orsw_signaled);
>
> /*
> * Here, we must take the lock to avoid the on-stack 'orsw' to be
> diff --git a/drivers/staging/lustre/lustre/obdclass/llog_obd.c
> b/drivers/staging/lustre/lustre/obdclass/llog_obd.c
> index 28bbaa2136ac..26aea114a29b 100644
> --- a/drivers/staging/lustre/lustre/obdclass/llog_obd.c
> +++ b/drivers/staging/lustre/lustre/obdclass/llog_obd.c
> @@ -104,7 +104,6 @@ EXPORT_SYMBOL(__llog_ctxt_put);
>
> int llog_cleanup(const struct lu_env *env, struct llog_ctxt *ctxt)
> {
> - struct l_wait_info lwi = LWI_INTR(LWI_ON_SIGNAL_NOOP, NULL);
> struct obd_llog_group *olg;
> int rc, idx;
>
> @@ -129,8 +128,8 @@ int llog_cleanup(const struct lu_env *env, struct
> llog_ctxt *ctxt)
> CERROR("Error %d while cleaning up ctxt %p\n",
> rc, ctxt);
>
> - l_wait_event(olg->olg_waitq,
> - llog_group_ctxt_null(olg, idx), &lwi);
> + l_wait_event_abortable(olg->olg_waitq,
> + llog_group_ctxt_null(olg, idx));
>
> return rc;
> }
> diff --git a/drivers/staging/lustre/lustre/osc/osc_page.c
> b/drivers/staging/lustre/lustre/osc/osc_page.c
> index 20094b6309f9..6fdd521feb21 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_page.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_page.c
> @@ -759,7 +759,6 @@ static long osc_lru_reclaim(struct client_obd *cli,
> unsigned long npages)
> static int osc_lru_alloc(const struct lu_env *env, struct client_obd
> *cli,
> struct osc_page *opg)
> {
> - struct l_wait_info lwi = LWI_INTR(LWI_ON_SIGNAL_NOOP, NULL);
> struct osc_io *oio = osc_env_io(env);
> int rc = 0;
>
> @@ -782,9 +781,8 @@ static int osc_lru_alloc(const struct lu_env *env,
> struct client_obd *cli,
>
> cond_resched();
>
> - rc = l_wait_event(osc_lru_waitq,
> - atomic_long_read(cli->cl_lru_left) > 0,
> - &lwi);
> + rc = l_wait_event_abortable(osc_lru_waitq,
> + atomic_long_read(cli->cl_lru_left)
> > 0);
>
> if (rc < 0)
> break;
> diff --git a/drivers/staging/lustre/lustre/osc/osc_request.c
> b/drivers/staging/lustre/lustre/osc/osc_request.c
> index 45b1ebf33363..074b5ce6284c 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_request.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_request.c
> @@ -552,14 +552,12 @@ static int osc_destroy(const struct lu_env *env,
> struct obd_export *exp,
>
> req->rq_interpret_reply = osc_destroy_interpret;
> if (!osc_can_send_destroy(cli)) {
> - struct l_wait_info lwi = LWI_INTR(LWI_ON_SIGNAL_NOOP, NULL);
> -
> /*
> * Wait until the number of on-going destroy RPCs drops
> * under max_rpc_in_flight
> */
> - l_wait_event_exclusive(cli->cl_destroy_waitq,
> - osc_can_send_destroy(cli), &lwi);
> + l_wait_event_abortable_exclusive(cli->cl_destroy_waitq,
> + osc_can_send_destroy(cli));
> }
>
> /* Do not wait for response */
>
>
> _______________________________________________
> lustre-devel mailing list
> lustre-de...@lists.lustre.org
> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
>
signature.asc
Description: PGP signature
