| Message ID | 20221206141125.93055-1-suwan.kim027@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is finished | expand |
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@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
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@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.
On Thu, Dec 08, 2022 at 09:48:23AM -0700, Jens Axboe wrote: > > 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 think the comment is still entirely correct. > > > 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. Exactly. Polling or not, drivers should go through the normal completion interface, that is blk_mq_complete_request or the lower-level options blk_mq_complete_request_remote and blk_mq_complete_request_direct.
On Sun, Dec 11, 2022 at 10:50:03PM -0800, Christoph Hellwig wrote: > On Thu, Dec 08, 2022 at 09:48:23AM -0700, Jens Axboe wrote: > > > 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 think the comment is still entirely correct. > > > > > > 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. > > Exactly. Polling or not, drivers should go through the normal completion > interface, that is blk_mq_complete_request or the lower-level options > blk_mq_complete_request_remote and blk_mq_complete_request_direct. Hi all, Thanks for the comment. It was the wrong use of the function... I will use blk_mq_complete_request_remote() instead of blk_mq_set_request_complete() and send next version soon. Regards, Suwan Kim
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); }
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@gmail.com> --- drivers/block/virtio_blk.c | 1 + 1 file changed, 1 insertion(+)