Virtualization - Dec 2022 - [PATCH 1/2] virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is finished

On Tue, Dec 06, 2022 at 11:11:24PM +0900, Suwan Kim
wrote:
> Driver should set req->state to MQ_RQ_COMPLETE after it finishes to
process
> req. But virtio-blk doesn't set MQ_RQ_COMPLETE after virtblk_poll()
handles
> req and req->state still remains MQ_RQ_IN_FLIGHT. Fortunately so far
there
> is no issue about it because blk_mq_end_request_batch() sets req->state
to
> MQ_RQ_IDLE. This patch properly sets req->state after polling I/O is
finished.
> 
> Fixes: 4e0400525691 ("virtio-blk: support polling I/O")
> Signed-off-by: Suwan Kim <suwan.kim027 at gmail.com>
> ---
>  drivers/block/virtio_blk.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 19da5defd734..cf64d256787e 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -839,6 +839,7 @@ static void virtblk_complete_batch(struct io_comp_batch
*iob)
>  	rq_list_for_each(&iob->req_list, req) {
>  		virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
>  		virtblk_cleanup_cmd(req);
> +		blk_mq_set_request_complete(req);
>  	}
>  	blk_mq_end_request_batch(iob);
>  }
The doc comment for blk_mq_set_request_complete() mentions this being
used in ->queue_rq(), but that's not the case here. Does the doc comment
need to be updated if we're using the function in a different way?

I'm not familiar enough with the Linux block APIs, but this feels weird
to me. Shouldn't blk_mq_end_request_batch(iob) take care of this for us?
Why does it set the state to IDLE instead of COMPLETE?

I think Jens can confirm whether we really want all drivers that use
polling and io_comp_batch to manually call
blk_mq_set_request_complete().

Thanks,
Stefan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20221207/1c76fde4/attachment.sig>

On 12/7/22 2:47?PM, Stefan Hajnoczi wrote:
> On Tue, Dec 06, 2022 at 11:11:24PM +0900, Suwan Kim wrote:
>> Driver should set req->state to MQ_RQ_COMPLETE after it finishes to
process
>> req. But virtio-blk doesn't set MQ_RQ_COMPLETE after virtblk_poll()
handles
>> req and req->state still remains MQ_RQ_IN_FLIGHT. Fortunately so far
there
>> is no issue about it because blk_mq_end_request_batch() sets
req->state to
>> MQ_RQ_IDLE. This patch properly sets req->state after polling I/O is
finished.
>>
>> Fixes: 4e0400525691 ("virtio-blk: support polling I/O")
>> Signed-off-by: Suwan Kim <suwan.kim027 at gmail.com>
>> ---
>>  drivers/block/virtio_blk.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index 19da5defd734..cf64d256787e 100644
>> --- a/drivers/block/virtio_blk.c
>> +++ b/drivers/block/virtio_blk.c
>> @@ -839,6 +839,7 @@ static void virtblk_complete_batch(struct
io_comp_batch *iob)
>>  	rq_list_for_each(&iob->req_list, req) {
>>  		virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
>>  		virtblk_cleanup_cmd(req);
>> +		blk_mq_set_request_complete(req);
>>  	}
>>  	blk_mq_end_request_batch(iob);
>>  }
> 
> The doc comment for blk_mq_set_request_complete() mentions this being
> used in ->queue_rq(), but that's not the case here. Does the doc
comment
> need to be updated if we're using the function in a different way?
Looks like it's a bit outdated...
> I'm not familiar enough with the Linux block APIs, but this feels weird
> to me. Shouldn't blk_mq_end_request_batch(iob) take care of this for
us?
> Why does it set the state to IDLE instead of COMPLETE?
> 
> I think Jens can confirm whether we really want all drivers that use
> polling and io_comp_batch to manually call
> blk_mq_set_request_complete().
Should not be a need to call blk_mq_set_request_complete() directly in
the driver for this.

-- 
Jens Axboe