| Message ID | 20230620083857.611153-1-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: virtio-blk: Fix handling of zone append command completion | expand |
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
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
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
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 !
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); }
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(-)