| Message ID | 20211209092815.778066-2-oro@il.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-block: switch to blk_get_max_hw_transfer | expand |
On Thu, Dec 9, 2021 at 10:34 AM Or Ozeri <oro@il.ibm.com> wrote: > > The blk_get_max_hw_transfer API was recently added in 6.1.0. > It allows querying an underlying block device its max transfer capability. > This commit changes virtio-blk to use this. > > Signed-off-by: Or Ozeri <oro@il.ibm.com> > --- > hw/block/virtio-blk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index f139cd7cc9..1ba9a06888 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -458,7 +458,7 @@ static void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb) > return; > } > > - max_transfer = blk_get_max_transfer(mrb->reqs[0]->dev->blk); > + max_transfer = blk_get_max_hw_transfer(mrb->reqs[0]->dev->blk); > > qsort(mrb->reqs, mrb->num_reqs, sizeof(*mrb->reqs), > &multireq_compare); > -- > 2.25.1 > > Hi Or, Superficially, this makes sense to me. A couple of things to consider: - Move the explanation from the cover letter into the patch description. The current patch description is pretty much meaningless. - Should the actual limit be consulted for the number of segments as well? IOW should blk_get_max_hw_iov() be called instead of blk_get_max_iov() a dozen lines below? I'm adding Stefan and Kevin to CC to get more eyes on this patch as it fixes a regression. I believe this was encountered with the following NBD device, Or please correct me if I'm wrong: $ cat /sys/block/nbd0/queue/max_sectors_kb 128 $ cat /sys/block/nbd0/queue/max_segments 65535 Thanks, Ilya
Am 12.01.2023 um 21:28 hat Ilya Dryomov geschrieben: > On Thu, Dec 9, 2021 at 10:34 AM Or Ozeri <oro@il.ibm.com> wrote: > > > > The blk_get_max_hw_transfer API was recently added in 6.1.0. > > It allows querying an underlying block device its max transfer capability. > > This commit changes virtio-blk to use this. > > > > Signed-off-by: Or Ozeri <oro@il.ibm.com> > > --- > > hw/block/virtio-blk.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > index f139cd7cc9..1ba9a06888 100644 > > --- a/hw/block/virtio-blk.c > > +++ b/hw/block/virtio-blk.c > > @@ -458,7 +458,7 @@ static void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb) > > return; > > } > > > > - max_transfer = blk_get_max_transfer(mrb->reqs[0]->dev->blk); > > + max_transfer = blk_get_max_hw_transfer(mrb->reqs[0]->dev->blk); > > > > qsort(mrb->reqs, mrb->num_reqs, sizeof(*mrb->reqs), > > &multireq_compare); > > Hi Or, > > Superficially, this makes sense to me. I'm not sure I understand. This is not a passthrough device (unlike scsi-generic), so why should we consider the hardware limits rather than the kernel/other backend limits for read/write requests? See the documentation of both fields: /* * Maximal transfer length in bytes. Need not be power of 2, but * must be multiple of opt_transfer and bl.request_alignment, or 0 * for no 32-bit limit. For now, anything larger than INT_MAX is * clamped down. */ uint32_t max_transfer; /* * Maximal hardware transfer length in bytes. Applies whenever * transfers to the device bypass the kernel I/O scheduler, for * example with SG_IO. If larger than max_transfer or if zero, * blk_get_max_hw_transfer will fall back to max_transfer. */ uint64_t max_hw_transfer; Is the real problem that max_transfer isn't right? Kevin
> -----Original Message----- > From: Kevin Wolf <kwolf@redhat.com> > Sent: Friday, 13 January 2023 13:45 > To: Ilya Dryomov <idryomov@gmail.com> > Cc: Or Ozeri <ORO@il.ibm.com>; qemu-devel@nongnu.org; > dupadhya@redhat.com; to.my.trociny@gmail.com; qemu- > block@nongnu.org; Danny Harnik <DANNYH@il.ibm.com>; Stefan Hajnoczi > <stefanha@redhat.com>; Paolo Bonzini <pbonzini@redhat.com> > Subject: [EXTERNAL] Re: [PATCH v1 1/1] virtio-block: switch to > blk_get_max_hw_transfer > > > I'm not sure I understand. This is not a passthrough device (unlike scsi- > generic), so why should we consider the hardware limits rather than the > kernel/other backend limits for read/write requests? I don't understand much about it as well :) But anyway, this bug was tested on 6.1.0, and Ilya suggested that I will test it on newer versions. After doing that, I found out that the bug is not reproduceable in 6.1.1. The commit that fixed things in 6.1.1 is this: block: introduce max_hw_iov for use in scsi-generic https://lists.gnu.org/archive/html/qemu-block/2021-09/msg00807.html I guess we can just discard this thread. Thanks, Or
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index f139cd7cc9..1ba9a06888 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -458,7 +458,7 @@ static void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb) return; } - max_transfer = blk_get_max_transfer(mrb->reqs[0]->dev->blk); + max_transfer = blk_get_max_hw_transfer(mrb->reqs[0]->dev->blk); qsort(mrb->reqs, mrb->num_reqs, sizeof(*mrb->reqs), &multireq_compare);
The blk_get_max_hw_transfer API was recently added in 6.1.0. It allows querying an underlying block device its max transfer capability. This commit changes virtio-blk to use this. Signed-off-by: Or Ozeri <oro@il.ibm.com> --- hw/block/virtio-blk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)