Re: [PATCH] block: use the request length for iov alignment
On Tue, Sep 13, 2022 at 03:20:23PM +0100, Damien Le Moal wrote:
> On 2022/09/13 15:12, Keith Busch wrote:
> > On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote:
> >> From: Keith Busch
> >>
> >> An iov length needs to be aligned to the logical block size, which may
> >> be larger than the memory alignment.
> >
> > [cc'ing some other interested folks]
> >
> > Any thoughts on this patch? It is fixing an observed IO error when running
> > virtio-blk with the default 512b logical block size backed by a drive
> > formatted
> > with 4k logical block.
>
> The patch look OK to me, but having virtio expose a 512B LBA size for a
> backing
> device that has 4K LBAs will break all IOs if caching is turned off (direct
> IOs
> case), even if this patch is applied. No ?
Oh, as to why that type of setup "works" with O_DIRECT, when the check below
returns 'false', qemu allocates a bounce buffer. We want that to happen if the
guest's virtio driver tries to read/write 512b. The lengths just need to be
checked against the backing store's block size instead of the memory address
alignment.
> >> @@ -3243,13 +3243,14 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs,
> >> QEMUIOVector *qiov)
> >> {
> >> int i;
> >> size_t alignment = bdrv_min_mem_align(bs);
> >> +size_t len = bs->bl.request_alignment;
> >> IO_CODE();
> >>
> >> for (i = 0; i < qiov->niov; i++) {
> >> if ((uintptr_t) qiov->iov[i].iov_base % alignment) {
> >> return false;
> >> }
> >> -if (qiov->iov[i].iov_len % alignment) {
> >> +if (qiov->iov[i].iov_len % len) {
> >> return false;
> >> }
> >> }
Re: [PATCH] block: use the request length for iov alignment
On 2022/09/13 15:51, Keith Busch wrote:
> On Tue, Sep 13, 2022 at 03:20:23PM +0100, Damien Le Moal wrote:
>> On 2022/09/13 15:12, Keith Busch wrote:
>>> On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote:
From: Keith Busch
An iov length needs to be aligned to the logical block size, which may
be larger than the memory alignment.
>>>
>>> [cc'ing some other interested folks]
>>>
>>> Any thoughts on this patch? It is fixing an observed IO error when running
>>> virtio-blk with the default 512b logical block size backed by a drive
>>> formatted
>>> with 4k logical block.
>>
>> The patch look OK to me, but having virtio expose a 512B LBA size for a
>> backing
>> device that has 4K LBAs will break all IOs if caching is turned off (direct
>> IOs
>> case), even if this patch is applied. No ?
>
> Oh, as to why that type of setup "works" with O_DIRECT, when the check below
> returns 'false', qemu allocates a bounce buffer. We want that to happen if the
> guest's virtio driver tries to read/write 512b. The lengths just need to be
> checked against the backing store's block size instead of the memory address
> alignment.
Ah ! Got it. Thanks !
>
@@ -3243,13 +3243,14 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs,
QEMUIOVector *qiov)
{
int i;
size_t alignment = bdrv_min_mem_align(bs);
+size_t len = bs->bl.request_alignment;
IO_CODE();
for (i = 0; i < qiov->niov; i++) {
if ((uintptr_t) qiov->iov[i].iov_base % alignment) {
return false;
}
-if (qiov->iov[i].iov_len % alignment) {
+if (qiov->iov[i].iov_len % len) {
return false;
}
}
--
Damien Le Moal
Western Digital Research
Re: [PATCH] block: use the request length for iov alignment
On 2022/09/13 15:36, Keith Busch wrote: > On Tue, Sep 13, 2022 at 03:20:23PM +0100, Damien Le Moal wrote: >> On 2022/09/13 15:12, Keith Busch wrote: >>> On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote: From: Keith Busch An iov length needs to be aligned to the logical block size, which may be larger than the memory alignment. >>> >>> [cc'ing some other interested folks] >>> >>> Any thoughts on this patch? It is fixing an observed IO error when running >>> virtio-blk with the default 512b logical block size backed by a drive >>> formatted >>> with 4k logical block. >> >> The patch look OK to me, but having virtio expose a 512B LBA size for a >> backing >> device that has 4K LBAs will break all IOs if caching is turned off (direct >> IOs >> case), even if this patch is applied. No ? > > Yeah, it's just the default. I don't think anyone would do that on purpose > since it's so sub-optimal from a performance stand-point. As a follow up > patch, > perhaps if the logical_block_size parameter is not provided, we should default > to the backing device's block size? That sound like a reasonable approach to me. Note that I was not talking about the sub-optimal read-modify-write pattern for IOs not aligned on the backend device sector boundaries, but rather thinking about no IOs working at all if QEMU is started without caching (cache.direct=on option) -- Damien Le Moal Western Digital Research
Re: [PATCH] block: use the request length for iov alignment
On Tue, Sep 13, 2022 at 03:20:23PM +0100, Damien Le Moal wrote: > On 2022/09/13 15:12, Keith Busch wrote: > > On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote: > >> From: Keith Busch > >> > >> An iov length needs to be aligned to the logical block size, which may > >> be larger than the memory alignment. > > > > [cc'ing some other interested folks] > > > > Any thoughts on this patch? It is fixing an observed IO error when running > > virtio-blk with the default 512b logical block size backed by a drive > > formatted > > with 4k logical block. > > The patch look OK to me, but having virtio expose a 512B LBA size for a > backing > device that has 4K LBAs will break all IOs if caching is turned off (direct > IOs > case), even if this patch is applied. No ? Yeah, it's just the default. I don't think anyone would do that on purpose since it's so sub-optimal from a performance stand-point. As a follow up patch, perhaps if the logical_block_size parameter is not provided, we should default to the backing device's block size?
Re: [PATCH] block: use the request length for iov alignment
On 9/13/22 8:12 AM, Keith Busch wrote: > On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote: >> From: Keith Busch >> >> An iov length needs to be aligned to the logical block size, which may >> be larger than the memory alignment. > > [cc'ing some other interested folks] > > Any thoughts on this patch? It is fixing an observed IO error when running > virtio-blk with the default 512b logical block size backed by a drive > formatted > with 4k logical block. I ran into this issue and tested the patch. Works for me! Tested-by: Jens Axboe -- Jens Axboe
Re: [PATCH] block: use the request length for iov alignment
On 2022/09/13 15:12, Keith Busch wrote:
> On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote:
>> From: Keith Busch
>>
>> An iov length needs to be aligned to the logical block size, which may
>> be larger than the memory alignment.
>
> [cc'ing some other interested folks]
>
> Any thoughts on this patch? It is fixing an observed IO error when running
> virtio-blk with the default 512b logical block size backed by a drive
> formatted
> with 4k logical block.
The patch look OK to me, but having virtio expose a 512B LBA size for a backing
device that has 4K LBAs will break all IOs if caching is turned off (direct IOs
case), even if this patch is applied. No ?
>
>> ---
>> block/io.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index 0a8cbefe86..296d4b49a7 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -3243,13 +3243,14 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs,
>> QEMUIOVector *qiov)
>> {
>> int i;
>> size_t alignment = bdrv_min_mem_align(bs);
>> +size_t len = bs->bl.request_alignment;
>> IO_CODE();
>>
>> for (i = 0; i < qiov->niov; i++) {
>> if ((uintptr_t) qiov->iov[i].iov_base % alignment) {
>> return false;
>> }
>> -if (qiov->iov[i].iov_len % alignment) {
>> +if (qiov->iov[i].iov_len % len) {
>> return false;
>> }
>> }
>> --
>> 2.30.2
>>
--
Damien Le Moal
Western Digital Research
[PATCH] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
When use dpdk-vdpa tests vdpa device. You need to specify the mac address to
start the virtual machine through libvirt or qemu, but now, the libvirt or
qemu can call dpdk vdpa vendor driver's ops .get_config through
vhost_net_get_config
to get the mac address of the vdpa hardware without manual configuration.
Signed-off-by: Hao Chen
---
hw/block/vhost-user-blk.c | 1 -
hw/net/virtio-net.c | 3 ++-
hw/virtio/vhost-user.c| 19 ---
3 files changed, 2 insertions(+), 21 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 9117222456..5dca4eab09 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -337,7 +337,6 @@ static int vhost_user_blk_connect(DeviceState *dev, Error
**errp)
vhost_dev_set_config_notifier(>dev, _ops);
-s->vhost_user.supports_config = true;
ret = vhost_dev_init(>dev, >vhost_user, VHOST_BACKEND_TYPE_USER, 0,
errp);
if (ret < 0) {
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index dd0d056fde..274ea84644 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -149,7 +149,8 @@ static void virtio_net_get_config(VirtIODevice *vdev,
uint8_t *config)
* Is this VDPA? No peer means not VDPA: there's no way to
* disconnect/reconnect a VDPA peer.
*/
-if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
+if ((nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) ||
+(nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_USER)) {
ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *),
n->config_size);
if (ret != -1) {
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index bd24741be8..8b01078249 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2013,8 +2013,6 @@ static int vhost_user_backend_init(struct vhost_dev *dev,
void *opaque,
}
if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
-bool supports_f_config = vus->supports_config ||
-(dev->config_ops && dev->config_ops->vhost_dev_config_notifier);
uint64_t protocol_features;
dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
@@ -2033,23 +2031,6 @@ static int vhost_user_backend_init(struct vhost_dev
*dev, void *opaque,
*/
protocol_features &= VHOST_USER_PROTOCOL_FEATURE_MASK;
-if (supports_f_config) {
-if (!virtio_has_feature(protocol_features,
-VHOST_USER_PROTOCOL_F_CONFIG)) {
-error_setg(errp, "vhost-user device expecting "
- "VHOST_USER_PROTOCOL_F_CONFIG but the vhost-user
backend does "
- "not support it.");
-return -EPROTO;
-}
-} else {
-if (virtio_has_feature(protocol_features,
- VHOST_USER_PROTOCOL_F_CONFIG)) {
-warn_reportf_err(*errp, "vhost-user backend supports "
- "VHOST_USER_PROTOCOL_F_CONFIG but QEMU does
not.");
-protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG);
-}
-}
-
/* final set of protocol features */
dev->protocol_features = protocol_features;
err = vhost_user_set_protocol_features(dev, dev->protocol_features);
--
2.27.0
Re: [PATCH] block: use the request length for iov alignment
On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote:
> From: Keith Busch
>
> An iov length needs to be aligned to the logical block size, which may
> be larger than the memory alignment.
[cc'ing some other interested folks]
Any thoughts on this patch? It is fixing an observed IO error when running
virtio-blk with the default 512b logical block size backed by a drive formatted
with 4k logical block.
> ---
> block/io.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/block/io.c b/block/io.c
> index 0a8cbefe86..296d4b49a7 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -3243,13 +3243,14 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs,
> QEMUIOVector *qiov)
> {
> int i;
> size_t alignment = bdrv_min_mem_align(bs);
> +size_t len = bs->bl.request_alignment;
> IO_CODE();
>
> for (i = 0; i < qiov->niov; i++) {
> if ((uintptr_t) qiov->iov[i].iov_base % alignment) {
> return false;
> }
> -if (qiov->iov[i].iov_len % alignment) {
> +if (qiov->iov[i].iov_len % len) {
> return false;
> }
> }
> --
> 2.30.2
>
