[PATCH v1 0/1] virtio-block: switch to blk_get_max_hw_transfer

[PATCH v1 0/1] virtio-block: switch to blk_get_max_hw_transfer

[Or Ozeri]

Since moving to qemu 6.1.0 we've been seeing disk failures inside a qemu guest VM.
This happened when using if=virtio on a host /dev/nbdX device.
Binary searching on qemu commit history, I found these errors start on this commit:

Commit 18473467
file-posix: try BLKSECTGET on block devices too, do not round to power of 2

The above commit switched posix block device limits (including host /dev/nbdX devices)
to query limits from /sys/dev/block/..., instead of using predefined limits.

The scsi-generic driver was changed to use the queried limits,
whereas the virtio-blk driver was only the queried max_iov,
but still using the predefined max_transfer, which is unlimited in qemu.

For NBD devices, max_iov is unlimited by the kernel nbd driver.
As as consequence, the virtio-blk merged requests over the limit of our host /dev/nbdX device,
which apparently caused the guest disk errors.

The solution that worked for me was to change the virtio-blk driver to use the max_transfer
limit queried from the posix host device (given by blk_get_max_hw_transfer).

Or Ozeri (1):
  virtio-block: switch to blk_get_max_hw_transfer

 hw/block/virtio-blk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.25.1

[PATCH v1 1/1] virtio-block: switch to blk_get_max_hw_transfer

[Or Ozeri]

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

Re: [PATCH v1 1/1] virtio-block: switch to blk_get_max_hw_transfer

[Ilya Dryomov]

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

Re: [PATCH v1 1/1] virtio-block: switch to blk_get_max_hw_transfer

[Kevin Wolf]

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

RE: [PATCH v1 1/1] virtio-block: switch to blk_get_max_hw_transfer

[Or Ozeri]

> -----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