From: Jinoh Kang Subject: Re: [RFC PATCH 1/6] server: Allow calling async_handoff() with status code STATUS_ALERTED. Message-Id: <49e9478c-02f6-57c0-366c-353c697505f9@gmail.com> Date: Thu, 27 Jan 2022 17:44:58 +0900 In-Reply-To: References: <29a9011d-e9c6-cb7a-73aa-9859be53d4f5@gmail.com> On 1/27/22 08:52, Zebediah Figura (she/her) wrote: > On 1/23/22 11:29, Jinoh Kang wrote: >> +static int async_add_queue( struct object *obj, struct wait_queue_entry *entry ) >> +{ >> +    struct async *async = (struct async *)obj; >> +    assert( obj->ops == &async_ops ); >> + >> +    if (!async->pending && async->terminated && async->alerted) >> +    { >> +        /* The client has failed to complete synchronously (e.g. EWOULDBLOCK). >> +         * Restart the async as fully fledged asynchronous I/O, where >> +         * the completion port notification and APC call will be triggered >> +         * appropriately. */ >> +        async->pending = 1; >> + >> +        /* Unset the signaled flag if the client wants to block on this async. */ >> +        if (async->blocking) async->signaled = 0; >> + >> +        async_set_result( obj, STATUS_PENDING, 0 );  /* kick it off */ >> +    } >> + >> +    return add_queue( obj, entry ); >> +} >> + > > I'll admit, this kind of thing is why I didn't really want to have to try to optimize 3 server calls into 2. I concur. Hence, > (However, if it turns out that this goal is not of utmost significance, then > this patch serie can be easily modified so that it issues separate server > calls.) > Asyncs are already really complicated, To be fair, asynchronous I/O is inherently complicated in itself. > in terms of the many paths they can take, Although that one has a lot of room for improvement, yes. > and it seems like no matter what we do they're going to get worse. > > Still, I have a potential idea. > > What we need to do here is similar to the infrastructure that already exists for device asyncs, namely "unknown_status" etc. It would be nice to use that instead of reinventing it, and although I haven't tried, it seems possible. That one was on the table, too. In fact it can also help eliminate the initial_status == STATUS_ALERTED check. One catch is that async_set_unknown_status also sets direct_result to 0, which means to always fire off APC on completion. I wasn't entirely sure of what the effects of { .unknown_status = 1, .direct_result = 1 } would be. > > async_add_queue() as it is above is not great. I'm not sure that code actually works in every case; !pending && terminated && alerted was the condition I was able to deduce to detect this exact condition. It does sound a little arbitrary though, especially since it's testing for three unrelated conditions. > it definitely increases the mental burden even if it does. (Consider for instance that it would be triggered for *every* async). > > Instead what I'd suggest is to use the request introduced here in every case, even if the initial status was pending. You mean, along with use of unknown_status? > This introduces a new server call, but it only does so in cases where we already have to wait. Sounds reasonable. > > The end result would be not unlike get_next_device_request, which is essentially that request combined with some other things (and it doesn't deal in async object handles). -- Sincerely, Jinoh Kang