From: Jinoh Kang <jinoh.kang.kr@gmail.com>
Subject: Re: [RFC PATCH v2 2/5] server: Add a new server request "notify_async_direct_result."
Message-Id: <98480b97-74f2-922e-83d9-bff0a4adbabe@gmail.com>
Date: Fri, 28 Jan 2022 04:22:32 +0900
In-Reply-To: <53a3b943-dd1b-12b7-b9dd-8666a139ca69@gmail.com>
References: <148c894d-c8f1-bbe1-1c26-25ed265f7f37@gmail.com> <bc6cf1fd-457e-61d2-280f-69de024016db@gmail.com> <53a3b943-dd1b-12b7-b9dd-8666a139ca69@gmail.com>

On 1/28/22 04:13, Jinoh Kang wrote:
> On 1/28/22 04:05, Jinoh Kang wrote:
>> +
>> +/* Notify direct completion of async and close the wait handle */
>> +DECL_HANDLER(notify_async_direct_result)
>> +{
>> +    struct async *async = (struct async *)get_handle_obj( current->process, req->handle, 0, &async_ops );
>> +
>> +    if (!async) return;
>> +
>> +    if (async->iosb && async->unknown_status && !async->pending && async->terminated)
>> +    {
>> +        /* Reactivate async.  We call async_reselect() later. */
>> +        async->terminated = 0;
>> +
>> +        /* Set result for async_handoff(). */
>> +        set_error( req->status );
>> +        async->iosb->result = req->information;
>> +
>> +        /* The async_handoff() call prior to the current server request was
>> +         * effectively a no-op since async->unknown_status is 1.  Calling it
>> +         * again with async->unknown_status = 0 will do the remaining steps.
>> +         */
>> +        async->unknown_status = 0;
>> +        async_handoff( async, NULL, 0 );
>> +
>> +        if (get_error() == STATUS_PENDING)
>> +        {
>> +            async_reselect( async );
>> +        }
>> +        else if (NT_ERROR( get_error() ))
>> +        {
>> +            /* synchronous I/O failure: don't invoke callbacks, only dequeue it. */
>> +            async_dequeue( async );
> 
> Note: we can't use async_set_result() here.  async_handoff() leaves async->terminated as 0, and async_set_result() has "assert( async->terminated )".
> 
> Thus, reusing async_set_result() here requires either:
> 
> 1. Setting async->pending to 0 so that async_handoff() will call async_terminate().
>    This is obviously incorrect since unwanted IOCP packets and APCs will fire on synchronous failure.
> 
> 2. Not using async_handoff().  We have to copy a lot of code (particulary setting pending and direct_result flags) out of async_handoff() to do this.

Maybe just merge both branches (error/success) and force async_terminate() before calling async_set_result()?

> 
>> +        }
>> +        else
>> +        {
>> +            /* I/O completed successfully.  The client has already set the IOSB,
>> +             * so we can skip waiting on wait_handle and do async_set_result()
>> +             * directly.
>> +             *
>> +             * If !async->direct_result, an APC_ASYNC_IO has been fired.
>> +             * async_set_result() will be called when the APC returns.
>> +             */
>> +            if (async->direct_result)
>> +            {
>> +                async_set_result( &async->obj, async->iosb->status, async->iosb->result );
>> +                async->direct_result = 0;
>> +            }
>> +
>> +            /* close wait handle here to avoid extra server round trip */
>> +            if (async->wait_handle)
>> +            {
>> +                close_handle( async->thread->process, async->wait_handle );
>> +                async->wait_handle = 0;
>> +            }
>> +        }
>> +
>> +        /* The wait handle is preserved only when the status is STATUS_PENDING
>> +         * and async->blocking is set (i.e. we're going to block on it). */
>> +        reply->handle = async->wait_handle;
>> +    }
>> +    else set_error( STATUS_ACCESS_DENIED );
>> +
>> +    release_object( &async->obj );
>> +}
>> diff --git a/server/protocol.def b/server/protocol.def
>> index 02e73047f9b..2c3b8dbc619 100644
>> --- a/server/protocol.def
>> +++ b/server/protocol.def
>> @@ -2163,6 +2163,16 @@ enum message_type
>>  @END
>>  
>>  
>> +/* Notify direct completion of async and close the wait handle */
>> +@REQ(notify_async_direct_result)
>> +    obj_handle_t   handle;        /* wait handle */
>> +    unsigned int   status;        /* completion status */
>> +    apc_param_t    information;   /* IO_STATUS_BLOCK Information */
>> +@REPLY
>> +    obj_handle_t   handle;        /* wait handle, or NULL if closed */
>> +@END
>> +
>> +
>>  /* Perform a read on a file object */
>>  @REQ(read)
>>      async_data_t   async;         /* async I/O parameters */
>>
> 
> 


-- 
Sincerely,
Jinoh Kang