by Patrick Farrell

[permalink] [raw]

Subject: Re: [lustre-devel] [PATCH 15/16] staging: lustre: use explicit poll loop in ptlrpc_unregister_reply

This should not contribute to load, since it?s called out of the ptlrpcd
daemons.

On 12/18/17, 1:18 AM, "lustre-devel on behalf of NeilBrown"
<[email protected] on behalf of [email protected]> wrote:

>replace l_wait_event() with wait_event_timeout() and explicit
>loop. This approach is easier to understand.
>
>Signed-off-by: NeilBrown <[email protected]>
>---
> drivers/staging/lustre/lustre/ptlrpc/client.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/staging/lustre/lustre/ptlrpc/client.c
>b/drivers/staging/lustre/lustre/ptlrpc/client.c
>index 3e6d22beb9f5..bb8c9ab68f5f 100644
>--- a/drivers/staging/lustre/lustre/ptlrpc/client.c
>+++ b/drivers/staging/lustre/lustre/ptlrpc/client.c
>@@ -2500,7 +2500,6 @@ static int ptlrpc_unregister_reply(struct
>ptlrpc_request *request, int async)
> {
> int rc;
> wait_queue_head_t *wq;
>- struct l_wait_info lwi;
>
> /* Might sleep. */
> LASSERT(!in_interrupt());
>@@ -2543,16 +2542,17 @@ static int ptlrpc_unregister_reply(struct
>ptlrpc_request *request, int async)
> * Network access will complete in finite time but the HUGE
> * timeout lets us CWARN for visibility of sluggish NALs
> */
>- lwi = LWI_TIMEOUT_INTERVAL(LONG_UNLINK * HZ,
>- HZ, NULL, NULL);
>- rc = l_wait_event(*wq, !ptlrpc_client_recv_or_unlink(request),
>- &lwi);
>- if (rc == 0) {
>+ int cnt = 0;
>+ while (cnt < LONG_UNLINK &&
>+ (rc = wait_event_timeout(*wq,
>+ !ptlrpc_client_recv_or_unlink(request),
>+ HZ)) == 0)
>+ cnt += 1;
>+ if (rc > 0) {
> ptlrpc_rqphase_move(request, request->rq_next_phase);
> return 1;
> }
>
>- LASSERT(rc == -ETIMEDOUT);
> DEBUG_REQ(D_WARNING, request,
> "Unexpectedly long timeout receiving_reply=%d req_ulinked=%d
>reply_unlinked=%d",
> request->rq_receiving_reply,
>
>
>_______________________________________________
>lustre-devel mailing list
>[email protected]
>http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org

2017-12-18 21:37:39

On Tue, Dec 19 2017, Dilger, Andreas wrote:

> On Dec 18, 2017, at 11:03, Patrick Farrell <[email protected]> wrote:
>>
>> The wait calls in ll_statahead_thread are done in a service thread, and
>> should probably *not* contribute to load.
>>
>> The one in osc_extent_wait is perhaps tough - It is called both from user
>> threads & daemon threads depending on the situation. The effect of adding
>> that to load average could be significant for some activities, even when
>> no user threads are busy. Thoughts from other Lustre people would be
>> welcome here.
>
> The main reasons we started using l_wait_event() were:
> - it is used by the request handling threads, and wait_event() caused the
> load average to always be == number of service threads, which was
> wrong if those threads were idle waiting for requests to arrive.
> That is mostly a server problem, but a couple of request handlers are
> on the client also (DLM lock cancellation threads, etc.) that shouldn't
> contribute to load. It looks like there is a better solution for this
> today with TASK_IDLE.
> - we want the userspace threads to be interruptible if the server is not
> functional, but the client should at least get a chance to complete the
> RPC if the server is just busy. Since Lustre needs to work in systems
> with 10,000+ clients pounding a server, the server response time is not
> necessarily fast. The l_wait_event() behavior is that it blocks signals
> until the RPC timeout, which will normally succeed, but after the timeout
> the signals are unblocked and the user thread can be interrupted if the
> user wants, but it will keep waiting for the RPC to finish if not. This
> is half-way between NFS soft and hard mounts. I don't think there is an
> equivalent wait_event_* for that behaviour.

Thanks for the historical background, it can often help to understand
why the code is the way it is!

Thanks particularly for that second point. I missed it in the code, and
skimmed over the explanatory comment too quickly (I'm afraid I don't
trust comments very much).
I would implement this two-stage wait by first calling
swait_event_idle_timeout(),
then if that times out,
swait_event_interruptible()

rather than trying to combine then both into one super-macro. At least,
that is my thought before I try to write the code - maybe I'll change my
mind.

Anyway, it is clear now that this l_wait_event() series needs to be
rewritten now that I have a better understanding.

Thanks,
NeilBrown

>
> Cheers, Andreas
>
>> Similar issues for osc_object_invalidate.
>>
>> (If no one else speaks up, my vote is no contribution to load for those
>> OSC waits.)
>>
>> Otherwise this one looks good...
>>
>> On 12/18/17, 1:17 AM, "lustre-devel on behalf of NeilBrown"
>> <[email protected] on behalf of [email protected]> wrote:
>>
>>> @@ -968,7 +964,6 @@ static int ll_statahead_thread(void *arg)
>>> int first = 0;
>>> int rc = 0;
>>> struct md_op_data *op_data;
>>> - struct l_wait_info lwi = { 0 };
>>> sai = ll_sai_get(dir);
>>> sa_thread = &sai->sai_thread;
>>> @@ -1069,12 +1064,11 @@ static int ll_statahead_thread(void *arg)
>>> /* wait for spare statahead window */
>>> do {
>>> - l_wait_event(sa_thread->t_ctl_waitq,
>>> - !sa_sent_full(sai) ||
>>> - sa_has_callback(sai) ||
>>> - !list_empty(&sai->sai_agls) ||
>>> - !thread_is_running(sa_thread),
>>> - &lwi);
>>> + wait_event(sa_thread->t_ctl_waitq,
>>> + !sa_sent_full(sai) ||
>>> + sa_has_callback(sai) ||
>>> + !list_empty(&sai->sai_agls) ||
>>> + !thread_is_running(sa_thread));
>>> sa_handle_callback(sai);
>>> spin_lock(&lli->lli_agl_lock);
>>> @@ -1128,11 +1122,10 @@ static int ll_statahead_thread(void *arg)
>>> * for file release to stop me.
>>> */
>>> while (thread_is_running(sa_thread)) {
>>> - l_wait_event(sa_thread->t_ctl_waitq,
>>> - sa_has_callback(sai) ||
>>> - !agl_list_empty(sai) ||
>>> - !thread_is_running(sa_thread),
>>> - &lwi);
>>> + wait_event(sa_thread->t_ctl_waitq,
>>> + sa_has_callback(sai) ||
>>> + !agl_list_empty(sai) ||
>>> + !thread_is_running(sa_thread));
>>> sa_handle_callback(sai);
>>> }
>>> @@ -1145,9 +1138,8 @@ static int ll_statahead_thread(void *arg)
>>> CDEBUG(D_READA, "stop agl thread: sai %p pid %u\n",
>>> sai, (unsigned int)agl_thread->t_pid);
>>> - l_wait_event(agl_thread->t_ctl_waitq,
>>> - thread_is_stopped(agl_thread),
>>> - &lwi);
>>> + wait_event(agl_thread->t_ctl_waitq,
>>> + thread_is_stopped(agl_thread));
>>> } else {
>>> /* Set agl_thread flags anyway. */
>>> thread_set_flags(agl_thread, SVC_STOPPED);
>>> @@ -1159,8 +1151,8 @@ static int ll_statahead_thread(void *arg)
>>> */
>>> while (sai->sai_sent != sai->sai_replied) {
>>> /* in case we're not woken up, timeout wait */
>>> - lwi = LWI_TIMEOUT(msecs_to_jiffies(MSEC_PER_SEC >> 3),
>>> - NULL, NULL);
>>> + struct l_wait_info lwi = LWI_TIMEOUT(msecs_to_jiffies(MSEC_PER_SEC >>
>>> 3),
>>> + NULL, NULL);
>>> l_wait_event(sa_thread->t_ctl_waitq,
>>> sai->sai_sent == sai->sai_replied, &lwi);
>>> }
>>
>
> Cheers, Andreas
> --
> Andreas Dilger
> Lustre Principal Architect
> Intel Corporation

Attachments:

signature.asc (832.00 B)