Bug 1647104

Created attachment 1502547 [details]
output of qemu-img map accessing the image directly

Created attachment 1502548 [details]
output of qemu-img map accessing the image via qemu-nbd

Sounds like it is related (if not duplicate) of bug 1648622

Reproduced upstream as well, although it seems that the qemu-img convert is a bit faster now:

[root@virtlab415 test]# time qemu-img map -f qcow2 --output json nbd-test-disk1 > nbd-test-disk1-map.json 

real	0m0.053s
user	0m0.033s
sys	0m0.012s

[root@virtlab415 test]# time qemu-img convert -p -f qcow2 -O raw -T none nbd-test-disk1 /dev/shm/disk.img
    (100.00/100%)

real	0m51.823s
user	0m0.766s
sys	0m4.564s


[root@virtlab415 test]# time qemu-img map --output json  nbd:unix:/tmp/nbd.sock:exportname=export > nbd-test-disk1-map-via-nbd.json

real	0m6.389s
user	0m2.741s
sys	0m2.210s

[root@virtlab415 test]# time qemu-img convert -p -f raw -O raw nbd:unix:/tmp/nbd.sock:exportname=export /dev/shm/disk.img
    (100.00/100%)

real	1m9.238s
user	0m4.262s
sys	0m8.958s

I found the root cause(s) of the bug.


1. First of all, as I already said qemu-img iterates over the image in up to 1G chunks, so the larger the sparse qcow2 image is, the more nbd traffic will be needed.


2. QEMU's NBD server advertises the max transfer size (in NBD_INFO_BLOCK_SIZE) as bs->bl.max_transfer, capping it at NBD_MAX_BUFFER_SIZE (currently 32M)


3. QEMU NBD client uses this max transfer size to cap the chunk size it uses when it queries for the block status (map) and for convert


3. for files, file-posix doesn't set bs->bl.max_transfer, so 32M chunks are used, but for block devices it uses the BLKSECTGET IOCTL to get max transfer size of the underlying block device, which happens to be 128K on the nvme drive I use. 

IMHO this is wrong, as the kernel is able to split larger requests so it can do transfers of any size. However I don't know the kernel block driver well enough to be 100% sure that this will always work. At least for the limited testing I did, removing the BLKSECTGET code, brings back the performance to be the same as on a file, fixing this bug.

Also note that when exporting the qcow image as raw over NBD (

I hit submit too soon here and forgot to post the continuation of this comment.

So, Also note that when exporting the qcow image as raw over NBD (qemu-nbd -k /tmp/nbd.sock -v  -f raw  -x export --cache=none --aio=native --persistent /dev/nvme0n1p3),
qcow2 driver runs in the client, which avoids the issue all together, since it know without reading the whole underlying device which areas are allocated and which are not.

Also note that the code which added max transfer size to the file-posix is somewhat recent, it is from commit 

commit 6f6071745bd0366221f5a0160ed7d18d0e38b9f7
Author: Fam Zheng <famz>
Date:   Fri Jun 3 10:07:02 2016 +0800

    raw-posix: Fetch max sectors for host block device
    
    This is sometimes a useful value we should count in.
    
    Signed-off-by: Fam Zheng <famz>
    Reviewed-by: Eric Blake <eblake>
    Signed-off-by: Kevin Wolf <kwolf>

This code might not be needed, _or_ we can tune the nbd driver to allow larger that max transfer requests which would be split by qemu block layer thus avoiding the network overhead.
I need your advice here how to proceed on this.

(In reply to Maxim Levitsky from comment #18)
> I hit submit too soon here and forgot to post the continuation of this
> comment.
> 
> So, Also note that when exporting the qcow image as raw over NBD (qemu-nbd
> -k /tmp/nbd.sock -v  -f raw  -x export --cache=none --aio=native
> --persistent /dev/nvme0n1p3),
> qcow2 driver runs in the client, which avoids the issue all together, since
> it know without reading the whole underlying device which areas are
> allocated and which are not.
> 
> Also note that the code which added max transfer size to the file-posix is
> somewhat recent, it is from commit 
> 
> commit 6f6071745bd0366221f5a0160ed7d18d0e38b9f7
> Author: Fam Zheng <famz>
> Date:   Fri Jun 3 10:07:02 2016 +0800
> 
>     raw-posix: Fetch max sectors for host block device
>     
>     This is sometimes a useful value we should count in.
>     
>     Signed-off-by: Fam Zheng <famz>
>     Reviewed-by: Eric Blake <eblake>
>     Signed-off-by: Kevin Wolf <kwolf>
> 
> This code might not be needed, _or_ we can tune the nbd driver to allow
> larger that max transfer requests which would be split by qemu block layer
> thus avoiding the network overhead.
> I need your advice here how to proceed on this.

If I could find time to work on NBD protocol extensions, I've got several things lined up to see what will help with performance (some may help more than others, but we may still want all of them):

- One proposal is to add NBD_CMD_FLAG_FAST_ZERO: https://lists.debian.org/nbd/2019/03/msg00004.html
This would map nicely to qemu's recent BDRV_REQ_NO_FALLBACK addition, and means that attempts to pre-zero the destination image do not accidentally slow it down if zeroing is not fast.
- There's also been a proposal to advertise whether an image is known to contain all zeroes at initial connection time, which bypasses the need to query block status or attempt pre-zero. But it is also more limited in scope - it's a one shot flag (as soon as you write to the image, the flag is no longer valid)
- Another proposal is about how to expand NBD_OPT_GO to provide additional information about maximum sizing for zero/trim requests which is larger than the maximum transfer reported in NBD_INFO_BLOCK_SIZE. With this, it would be possible for NBD to report support for zeroing ~4G of an image in one go, rather than having to do it in a loop 32M at a time.

You also mentioned running qemu-nbd -f raw and having the client use -f qcow2 (instead of our more typical qemu-nbd -f qcow2 and client -f raw); that's okay for read-only images, but until we implement resize support in the NBD protocol, it requires pre-allocation on the server side before the client connects, or you risk ENOSPC situations that you don't get with local files holding the qcow2 format (since local files can resize as needed).

The problem is that as all of the above points are not yet standardized in the NBD protocol, they need a proof of concept code implementation (obviously in qemu, but preferably also in nbdkit and/or libnbd to prove interoperability of the extension). At this point, the soonest any of these extensions will be finalized will be for qemu 4.2. If we find other tricks in qemu proper for avoiding the slow paths in the first place, those can be applied even without NBD extensions.

I checked the kernel source, and basically I am pretty sure that the kernel can accept any size O_DIRECT read/writes on a block device.

First the in the __blkdev_direct_IO, the incoming iovec is split into bios which have limit of 256 pages each, then underlying block layer splits the bios futher according to the device limits.
This happens in the blk_queue_split which is called from the blk_mq_make_request, the 'only' make_request function remaining these days for hardware block layer (after removal of the non mq block layer).

The blk_queue_split splits the requests according to all the hardware limitations.

I also tested that if I set mdtd of qemu's virtual nvme drive to 1 (which corresponds to 8K maximum transfer size (4K << 1), and I run
dd if=/dev/nvme0n1 bs=1M count=1 iflag=direct of=/dev/null,
in the guest, I see in the qemu that the virtual nvme drive gets lots of nice 8K sized requests, and the dd succeeds.

So I think that the max transfer size limit on the raw block devices is wrong to have. But I would be more that happy you to prove me wrong, because there might be some corner cases yet.

Best regards,
     Maxim Levitsky

And on top of that for scsi passthrough, a char(!) device is used (/dev/sg*) which bypasses the whole block layer and indeed for sure carries the limitation of max transfer size.
So I think that the right solution here is to drop the max transfer size code from all but these devices.

Some more info here:

https://developer.ibm.com/articles/enhancing-qemu-virtio-scsi-with-block-limits-vpd-emulation/#sec3

Reproduced this issue in rhel8.1.0-av like below:

Tested with:
qemu-kvm-4.0.0-4.module+el8.1.0+3356+cda7f1ee
4.18.0-100.el8.x86_64


Steps:
=====================================================================================
Local:
# time qemu-img map --output=json /dev/nvme0n1p1 > test.json

real	0m0.017s
user	0m0.008s
sys	0m0.006s

# time qemu-img convert -f raw -O raw /dev/nvme0n1p1 /dev/shm/tgt.img 

real	0m2.805s
user	0m0.819s
sys	0m8.154s

======================================================================================
NBD:
# time qemu-img map --output=json nbd:unix:/home/test/my.socket:exportname=export > test_nbd.json 

real	0m0.704s
user	0m0.275s
sys	0m0.243s

# time qemu-img convert -f raw -O raw nbd:unix:/home/test/my.socket:exportname=export /dev/shm/tgt.img 

real	0m5.014s
user	0m0.797s
sys	0m2.602s
=======================================================================================


Additional info:
Source image info:
1. Partition info:
nvme0n1                          259:0    0 745.2G  0 disk 
└─nvme0n1p1                      259:1    0     5G  0 part 
2. Mount the partition and write 1.5G data to it
# mount /dev/nvme0n1p1 /mnt/
# cd /mnt/
# dd if=/dev/urandom of=f1 bs=1M count=512
# dd if=/dev/urandom of=f2 bs=1M count=1024
# umount /mnt

Export image via below CML:
# qemu-nbd -k /home/test/my.socket -v -f raw -x export --cache=none --aio=threads --persistent /dev/nvme0n1p1

Reproduced this issue in rhel7.7 like below:

Tested with:
kernel-3.10.0-1058.el7.x86_64
qemu-kvm-rhev-2.12.0-33.el7


Steps:
================================================================================================
Local:
# time qemu-img map --output=json /dev/nvme0n1p1 > test.json

real	0m0.028s
user	0m0.014s
sys	0m0.013s

# time qemu-img convert -f raw -O raw /dev/nvme0n1p1 /dev/shm/tgt.img 

real	0m2.795s
user	0m0.837s
sys	0m6.545s
==================================================================================================
NBD:
# time qemu-img map --output=json nbd:unix:/home/test/my.socket:exportname=export > test_nbd.json 

real	0m0.652s
user	0m0.210s
sys	0m0.252s

# time qemu-img convert -f raw -O raw nbd:unix:/home/test/my.socket:exportname=export /dev/shm/tgt.img

real	0m3.102s
user	0m0.722s
sys	0m2.278s
===================================================================================================


Additional info:
The source image and export line is the same as the ones in Comment 25.

After confirming the steps with Maxim via IRC, re-tested this issue in rhel7, and reproduced the issues. Thanks.


Tested with:
qemu-kvm-rhev-2.12.0-33.el7
kernel-3.10.0-1058.el7.x86_64


Steps:
1. Create the block file with qcow2 format
# qemu-img create -f qcow2 /dev/nvme0n1p1 5G

2. Compare the map and convert time 
===========================================================================================================
Local:
# time qemu-img map ***-f qcow2*** --output=json /dev/nvme0n1p1> test.json

real	0m0.025s
user	0m0.013s
sys	0m0.012s

# time qemu-img convert ***-f qcow2*** -O raw /dev/nvme0n1p1 /dev/shm/tgt.img 

real	0m0.166s
user	0m0.016s
sys	0m0.150s
=============================================================================================================
NBD:
# time qemu-img map ***-f raw*** --output=json nbd:unix:/home/test/my.socket:exportname=export > test.json 

real	0m0.655s
user	0m0.200s
sys	0m0.265s

# time qemu-img convert ***-f raw*** -O raw nbd:unix:/home/test/my.socket:exportname=export /dev/shm/tgt.img

real	0m3.020s
user	0m0.708s
sys	0m2.435s
==============================================================================================================


Note:
In comment 26, I did not create qcow2 image on the block device(i.e The source file is a raw block device), and exported the file as RAW and used as RAW in client.

Patch posted upstream:

https://www.mail-archive.com/qemu-devel@nongnu.org/msg627717.html

V2 of the patch:

https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01412.html

Patch is accepted upstream:

https://git.qemu.org/?p=qemu.git;a=commit;h=867eccfed84f96b54f4a432c510a02c2ce03b430

You appear to have assumed that the only "SCSI Passthrough" is `-device scsi-generic`, while the fact is there's also `-device scsi-block` (passthrough without the sg driver). Unlike `-device scsi-hd`, getting max_sectors is necessary to it (more precisely, hw_max_sectors might what matters, but BLKSECTGET reports max_sectors, so).

I'm unsure about how qemu-nbd works, but the commit clearly wasn't the right approach to fix the original issue it addresses. (It should for example ignore the max_transfer if it will never matter in to it, or overrides it in certain cases; when I glimpsed over this, I don't see how it could be file-posix problem when it is reporting the right thing, regardless of whether "removing" the code helps.)

I don't think we want to "mark" `-device scsi-block` as sg either. It will probably bring even more unexpected problems, because they are literally different sets of things behind the scene / in the kernel.

https://lists.nongnu.org/archive/html/qemu-block/2020-09/msg00281.html
https://lists.nongnu.org/archive/html/qemu-block/2020-09/msg00282.html

Maybe you want to add some condition for this:
https://github.com/qemu/qemu/blob/v5.1.0/nbd/server.c#L659
Or not clamp it at all.