[PATCH] block: virtio-blk: Fix handling of zone append command completion

[PATCH] block: virtio-blk: Fix handling of zone append command completion

[Damien Le Moal]

The introduction of completion batching with commit 07b679f70d73
("virtio-blk: support completion batching for the IRQ path") overlloked
handling correctly the completion of zone append operations, which
require an update of the request __sector field, as is done in
virtblk_request_done(): the function virtblk_complete_batch() only
executes virtblk_unmap_data() and virtblk_cleanup_cmd() without doing
this update. This causes problems with zone append operations, e.g.
zonefs complains about invalid zone append locations.

Fix this by introducing the function virtblk_end_request(), which is
almost identicatl to virtblk_request_done() but without the call to
blk_mq_end_request(). Use this new function to rewrite
virtblk_request_done() and call it in virtblk_complete_batch() to end
all request of a batch.

Reported-by: Sam Li <faithilikerun@gmail.com>
Fixes: 07b679f70d73 ("virtio-blk: support completion batching for the IRQ path")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/block/virtio_blk.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 2b918e28acaa..513d8b582aec 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -332,10 +332,9 @@ static inline u8 virtblk_vbr_status(struct virtblk_req *vbr)
 	return *((u8 *)&vbr->in_hdr + vbr->in_hdr_len - 1);
 }
 
-static inline void virtblk_request_done(struct request *req)
+static inline blk_status_t virtblk_end_request(struct request *req)
 {
 	struct virtblk_req *vbr = blk_mq_rq_to_pdu(req);
-	blk_status_t status = virtblk_result(virtblk_vbr_status(vbr));
 	struct virtio_blk *vblk = req->mq_hctx->queue->queuedata;
 
 	virtblk_unmap_data(req, vbr);
@@ -345,17 +344,21 @@ static inline void virtblk_request_done(struct request *req)
 		req->__sector = virtio64_to_cpu(vblk->vdev,
 						vbr->in_hdr.zone_append.sector);
 
-	blk_mq_end_request(req, status);
+	return virtblk_result(virtblk_vbr_status(vbr));
+}
+
+static inline void virtblk_request_done(struct request *req)
+{
+	blk_mq_end_request(req, virtblk_end_request(req));
 }
 
 static void virtblk_complete_batch(struct io_comp_batch *iob)
 {
 	struct request *req;
 
-	rq_list_for_each(&iob->req_list, req) {
-		virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
-		virtblk_cleanup_cmd(req);
-	}
+	rq_list_for_each(&iob->req_list, req)
+		virtblk_end_request(req);
+
 	blk_mq_end_request_batch(iob);
 }
 
-- 
2.40.1

Re: [PATCH] block: virtio-blk: Fix handling of zone append command completion

[Suwan Kim]

On Tue, Jun 20, 2023 at 5:39 PM Damien Le Moal <dlemoal@kernel.org> wrote:
>
> The introduction of completion batching with commit 07b679f70d73
> ("virtio-blk: support completion batching for the IRQ path") overlloked
> handling correctly the completion of zone append operations, which
> require an update of the request __sector field, as is done in
> virtblk_request_done(): the function virtblk_complete_batch() only
> executes virtblk_unmap_data() and virtblk_cleanup_cmd() without doing
> this update. This causes problems with zone append operations, e.g.
> zonefs complains about invalid zone append locations.
>

Hi Damien Le Moal,

Unfortunately, this commit was reverted due to io hang.
(afd384f0dbea2229fd11159efb86a5b41051c4a9)
You can see the mail thread at the block layer mailing list.

We don't have a solution about io hang yet..
So I have one question.
Is there any possibility of virtblk-driver io hang on zoned devices
without this patch?

Regards,
Suwan Kim

Re: [PATCH] block: virtio-blk: Fix handling of zone append command completion

[Damien Le Moal]

On 6/22/23 23:32, Suwan Kim wrote:
> On Tue, Jun 20, 2023 at 5:39 PM Damien Le Moal <dlemoal@kernel.org> wrote:
>>
>> The introduction of completion batching with commit 07b679f70d73
>> ("virtio-blk: support completion batching for the IRQ path") overlloked
>> handling correctly the completion of zone append operations, which
>> require an update of the request __sector field, as is done in
>> virtblk_request_done(): the function virtblk_complete_batch() only
>> executes virtblk_unmap_data() and virtblk_cleanup_cmd() without doing
>> this update. This causes problems with zone append operations, e.g.
>> zonefs complains about invalid zone append locations.
>>
> 
> Hi Damien Le Moal,
> 
> Unfortunately, this commit was reverted due to io hang.
> (afd384f0dbea2229fd11159efb86a5b41051c4a9)
> You can see the mail thread at the block layer mailing list.

There is no commit afd384f0dbea2229fd11159efb86a5b41051c4a9 in Linus tree. What
patch are you talking about ? Where is it ?

> We don't have a solution about io hang yet..
> So I have one question.
> Is there any possibility of virtblk-driver io hang on zoned devices
> without this patch?

If you are talking about the batch completion support being reverted, then my
fix patch is not necessary. The issue I fixed is not about IO hang but the fact
that completion processing was not identical for batch case vs non batch. That
led to breakage of the zone append command completion. The original support for
zone append without batch completion is fine.

> 
> Regards,
> Suwan Kim

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] block: virtio-blk: Fix handling of zone append command completion

[Michael S. Tsirkin]

On Fri, Jun 23, 2023 at 06:55:24AM +0900, Damien Le Moal wrote:
> On 6/22/23 23:32, Suwan Kim wrote:
> > On Tue, Jun 20, 2023 at 5:39 PM Damien Le Moal <dlemoal@kernel.org> wrote:
> >>
> >> The introduction of completion batching with commit 07b679f70d73
> >> ("virtio-blk: support completion batching for the IRQ path") overlloked
> >> handling correctly the completion of zone append operations, which
> >> require an update of the request __sector field, as is done in
> >> virtblk_request_done(): the function virtblk_complete_batch() only
> >> executes virtblk_unmap_data() and virtblk_cleanup_cmd() without doing
> >> this update. This causes problems with zone append operations, e.g.
> >> zonefs complains about invalid zone append locations.
> >>
> > 
> > Hi Damien Le Moal,
> > 
> > Unfortunately, this commit was reverted due to io hang.
> > (afd384f0dbea2229fd11159efb86a5b41051c4a9)
> > You can see the mail thread at the block layer mailing list.
> 
> There is no commit afd384f0dbea2229fd11159efb86a5b41051c4a9 in Linus tree. What
> patch are you talking about ? Where is it ?

Either you didn't check recently enough, or  there's some
breakage and your CDN's not updating. If the later try
poking kernel.org admins.

This is the commit:

commit afd384f0dbea2229fd11159efb86a5b41051c4a9
Author: Michael S. Tsirkin <mst@redhat.com>
Date:   Thu Jun 8 17:42:53 2023 -0400

    Revert "virtio-blk: support completion batching for the IRQ path"

you can get the patch from lore too:
    Message-Id: <336455b4f630f329380a8f53ee8cad3868764d5c.1686295549.git.mst@redhat.com>


> 
> > We don't have a solution about io hang yet..
> > So I have one question.
> > Is there any possibility of virtblk-driver io hang on zoned devices
> > without this patch?
> 
> If you are talking about the batch completion support being reverted, then my
> fix patch is not necessary. The issue I fixed is not about IO hang but the fact
> that completion processing was not identical for batch case vs non batch. That
> led to breakage of the zone append command completion. The original support for
> zone append without batch completion is fine.

Yes that's great! I expect we'll reapply the batch completion
down the road and then your patch would help!

> > 
> > Regards,
> > Suwan Kim
> 
> -- 
> Damien Le Moal
> Western Digital Research

Re: [PATCH] block: virtio-blk: Fix handling of zone append command completion

[Damien Le Moal]

On 6/23/23 07:19, Michael S. Tsirkin wrote:
> On Fri, Jun 23, 2023 at 06:55:24AM +0900, Damien Le Moal wrote:
>> On 6/22/23 23:32, Suwan Kim wrote:
>>> On Tue, Jun 20, 2023 at 5:39 PM Damien Le Moal <dlemoal@kernel.org> wrote:
>>>>
>>>> The introduction of completion batching with commit 07b679f70d73
>>>> ("virtio-blk: support completion batching for the IRQ path") overlloked
>>>> handling correctly the completion of zone append operations, which
>>>> require an update of the request __sector field, as is done in
>>>> virtblk_request_done(): the function virtblk_complete_batch() only
>>>> executes virtblk_unmap_data() and virtblk_cleanup_cmd() without doing
>>>> this update. This causes problems with zone append operations, e.g.
>>>> zonefs complains about invalid zone append locations.
>>>>
>>>
>>> Hi Damien Le Moal,
>>>
>>> Unfortunately, this commit was reverted due to io hang.
>>> (afd384f0dbea2229fd11159efb86a5b41051c4a9)
>>> You can see the mail thread at the block layer mailing list.
>>
>> There is no commit afd384f0dbea2229fd11159efb86a5b41051c4a9 in Linus tree. What
>> patch are you talking about ? Where is it ?
> 
> Either you didn't check recently enough, or  there's some
> breakage and your CDN's not updating. If the later try
> poking kernel.org admins.
> 
> This is the commit:
> 
> commit afd384f0dbea2229fd11159efb86a5b41051c4a9
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date:   Thu Jun 8 17:42:53 2023 -0400
> 
>     Revert "virtio-blk: support completion batching for the IRQ path"
> 
> you can get the patch from lore too:
>     Message-Id: <336455b4f630f329380a8f53ee8cad3868764d5c.1686295549.git.mst@redhat.com>

Yep. Got it after pulling from Linus master. Should have done that first :)

>>> We don't have a solution about io hang yet..
>>> So I have one question.
>>> Is there any possibility of virtblk-driver io hang on zoned devices
>>> without this patch?
>>
>> If you are talking about the batch completion support being reverted, then my
>> fix patch is not necessary. The issue I fixed is not about IO hang but the fact
>> that completion processing was not identical for batch case vs non batch. That
>> led to breakage of the zone append command completion. The original support for
>> zone append without batch completion is fine.
> 
> Yes that's great! I expect we'll reapply the batch completion
> down the road and then your patch would help!

OK, Thanks !

-- 
Damien Le Moal
Western Digital Research