include/block/block.h | 8 ++-- include/block/block_int.h | 52 ++++++++++----------- block/backup-top.c | 5 +- block/blkdebug.c | 4 +- block/blklogwrites.c | 4 +- block/blkreplay.c | 4 +- block/blkverify.c | 6 +-- block/bochs.c | 2 +- block/cloop.c | 2 +- block/commit.c | 2 +- block/copy-on-read.c | 4 +- block/crypto.c | 4 +- block/curl.c | 2 +- block/dmg.c | 2 +- block/file-posix.c | 18 ++++---- block/filter-compress.c | 6 +-- block/io.c | 97 ++++++++++++++++++++------------------- block/iscsi.c | 12 ++--- block/mirror.c | 4 +- block/nbd.c | 8 ++-- block/nfs.c | 8 ++-- block/null.c | 8 ++-- block/nvme.c | 4 +- block/qcow.c | 12 ++--- block/qcow2.c | 18 ++++---- block/quorum.c | 8 ++-- block/raw-format.c | 28 +++++------ block/rbd.c | 4 +- block/throttle.c | 4 +- block/vdi.c | 4 +- block/vmdk.c | 8 ++-- block/vpc.c | 4 +- block/vvfat.c | 6 +-- tests/test-bdrv-drain.c | 8 ++-- block/trace-events | 14 +++--- 35 files changed, 192 insertions(+), 192 deletions(-)
Hi all! There is an idea to make NBD protocol extension to support 64bit write-zero/discard/block-status commands (commands, which doesn't transfer user data). It's needed to increase performance of zeroing large ranges (up to the whole image). Zeroing of the whole image is used as first step of mirror job, qemu-img convert, it should be also used at start of backup actually.. We need to support it in block-layer, so we want 64bit write_zeros. Currently driver handler now have int bytes parameter. write_zeros path goes through normal pwritev, so we need 64bit write, and then we need 64bit read for symmetry, and better, let's make all io path work with 64bit bytes parameter. Actually most of block-layer already have 64bit parameters: offset is sometimes int64_t and sometimes uint64_t. bytes parameter is one of int64_t, uint64_t, int, unsigned int... I think we need one type for all of this, and this one type is int64_t. Signed int64_t is a bit better than uint64_t: you can use same variable to get some result (including error < 0) and than reuse it as an argument without any type conversion. So, I propose, as a first step, convert all uint64_t parameters to int64_t. Still, I don't have good idea of how to split this into more than 3 patches, so, this is an RFC. What's next? Converting write_zero and discard is not as simple: we can't just s/int/uint64_t/, as some functions may use some int variables for calculations and this will be broken by something larger than int. So, I think the simplest way is to add .bdrv_co_pwritev_zeros64 and .bdrv_co_pdiscard64 and update drivers one-by-one. If at some point all drivers updated - drop unused 32bit functions, and then drop "64" suffix from API. If not - we'll live with both APIs. Another thing to do is updating default limiting of request (currently they are limited to INT_MAX). Then we may move some drivers to 64bit discard/write_zero: I think about qcow2, file-posix and nbd (as a proof-of-concept for already proposed NBD extension). Any ideas? Vladimir Sementsov-Ogievskiy (3): block: use int64_t as bytes type in tracked requests block/io: convert generic io path to use int64_t parameters block: use int64_t instead of uint64_t in driver handlers include/block/block.h | 8 ++-- include/block/block_int.h | 52 ++++++++++----------- block/backup-top.c | 5 +- block/blkdebug.c | 4 +- block/blklogwrites.c | 4 +- block/blkreplay.c | 4 +- block/blkverify.c | 6 +-- block/bochs.c | 2 +- block/cloop.c | 2 +- block/commit.c | 2 +- block/copy-on-read.c | 4 +- block/crypto.c | 4 +- block/curl.c | 2 +- block/dmg.c | 2 +- block/file-posix.c | 18 ++++---- block/filter-compress.c | 6 +-- block/io.c | 97 ++++++++++++++++++++------------------- block/iscsi.c | 12 ++--- block/mirror.c | 4 +- block/nbd.c | 8 ++-- block/nfs.c | 8 ++-- block/null.c | 8 ++-- block/nvme.c | 4 +- block/qcow.c | 12 ++--- block/qcow2.c | 18 ++++---- block/quorum.c | 8 ++-- block/raw-format.c | 28 +++++------ block/rbd.c | 4 +- block/throttle.c | 4 +- block/vdi.c | 4 +- block/vmdk.c | 8 ++-- block/vpc.c | 4 +- block/vvfat.c | 6 +-- tests/test-bdrv-drain.c | 8 ++-- block/trace-events | 14 +++--- 35 files changed, 192 insertions(+), 192 deletions(-) -- 2.21.0
Patchew URL: https://patchew.org/QEMU/20200330141818.31294-1-vsementsov@virtuozzo.com/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===
CC block/nbd.o
CC block/sheepdog.o
CC block/accounting.o
/tmp/qemu-test/src/block/file-win32.c:643:27: error: initialization of 'BlockAIOCB * (*)(BlockDriverState *, int64_t, int64_t, QEMUIOVector *, int, void (*)(void *, int), void *)' {aka 'struct BlockAIOCB * (*)(struct BlockDriverState *, long long int, long long int, struct QEMUIOVector *, int, void (*)(void *, int), void *)'} from incompatible pointer type 'BlockAIOCB * (*)(BlockDriverState *, uint64_t, uint64_t, QEMUIOVector *, int, void (*)(void *, int), void *)' {aka 'struct BlockAIOCB * (*)(struct BlockDriverState *, long long unsigned int, long long unsigned int, struct QEMUIOVector *, int, void (*)(void *, int), void *)'} [-Werror=incompatible-pointer-types]
.bdrv_aio_preadv = raw_aio_preadv,
^~~~~~~~~~~~~~
/tmp/qemu-test/src/block/file-win32.c:643:27: note: (near initialization for 'bdrv_file.bdrv_aio_preadv')
/tmp/qemu-test/src/block/file-win32.c:644:27: error: initialization of 'BlockAIOCB * (*)(BlockDriverState *, int64_t, int64_t, QEMUIOVector *, int, void (*)(void *, int), void *)' {aka 'struct BlockAIOCB * (*)(struct BlockDriverState *, long long int, long long int, struct QEMUIOVector *, int, void (*)(void *, int), void *)'} from incompatible pointer type 'BlockAIOCB * (*)(BlockDriverState *, uint64_t, uint64_t, QEMUIOVector *, int, void (*)(void *, int), void *)' {aka 'struct BlockAIOCB * (*)(struct BlockDriverState *, long long unsigned int, long long unsigned int, struct QEMUIOVector *, int, void (*)(void *, int), void *)'} [-Werror=incompatible-pointer-types]
.bdrv_aio_pwritev = raw_aio_pwritev,
^~~~~~~~~~~~~~~
/tmp/qemu-test/src/block/file-win32.c:644:27: note: (near initialization for 'bdrv_file.bdrv_aio_pwritev')
/tmp/qemu-test/src/block/file-win32.c:812:27: error: initialization of 'BlockAIOCB * (*)(BlockDriverState *, int64_t, int64_t, QEMUIOVector *, int, void (*)(void *, int), void *)' {aka 'struct BlockAIOCB * (*)(struct BlockDriverState *, long long int, long long int, struct QEMUIOVector *, int, void (*)(void *, int), void *)'} from incompatible pointer type 'BlockAIOCB * (*)(BlockDriverState *, uint64_t, uint64_t, QEMUIOVector *, int, void (*)(void *, int), void *)' {aka 'struct BlockAIOCB * (*)(struct BlockDriverState *, long long unsigned int, long long unsigned int, struct QEMUIOVector *, int, void (*)(void *, int), void *)'} [-Werror=incompatible-pointer-types]
.bdrv_aio_preadv = raw_aio_preadv,
^~~~~~~~~~~~~~
/tmp/qemu-test/src/block/file-win32.c:812:27: note: (near initialization for 'bdrv_host_device.bdrv_aio_preadv')
/tmp/qemu-test/src/block/file-win32.c:813:27: error: initialization of 'BlockAIOCB * (*)(BlockDriverState *, int64_t, int64_t, QEMUIOVector *, int, void (*)(void *, int), void *)' {aka 'struct BlockAIOCB * (*)(struct BlockDriverState *, long long int, long long int, struct QEMUIOVector *, int, void (*)(void *, int), void *)'} from incompatible pointer type 'BlockAIOCB * (*)(BlockDriverState *, uint64_t, uint64_t, QEMUIOVector *, int, void (*)(void *, int), void *)' {aka 'struct BlockAIOCB * (*)(struct BlockDriverState *, long long unsigned int, long long unsigned int, struct QEMUIOVector *, int, void (*)(void *, int), void *)'} [-Werror=incompatible-pointer-types]
.bdrv_aio_pwritev = raw_aio_pwritev,
^~~~~~~~~~~~~~~
/tmp/qemu-test/src/block/file-win32.c:813:27: note: (near initialization for 'bdrv_host_device.bdrv_aio_pwritev')
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: block/file-win32.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "./tests/docker/docker.py", line 664, in <module>
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=46f0957c6a3a47e5971905cb30141048', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-0szyhgs2/src/docker-src.2020-03-30-13.48.43.9903:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=46f0957c6a3a47e5971905cb30141048
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-0szyhgs2/src'
make: *** [docker-run-test-mingw@fedora] Error 2
real 2m13.293s
user 0m8.052s
The full log is available at
http://patchew.org/logs/20200330141818.31294-1-vsementsov@virtuozzo.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On Mon, Mar 30, 2020 at 05:18:15PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Hi all! > > There is an idea to make NBD protocol extension to support 64bit > write-zero/discard/block-status commands (commands, which doesn't > transfer user data). It's needed to increase performance of zeroing > large ranges (up to the whole image). Zeroing of the whole image is used > as first step of mirror job, qemu-img convert, it should be also used at > start of backup actually.. > > We need to support it in block-layer, so we want 64bit write_zeros. > Currently driver handler now have int bytes parameter. > > write_zeros path goes through normal pwritev, so we need 64bit write, > and then we need 64bit read for symmetry, and better, let's make all io > path work with 64bit bytes parameter. > > Actually most of block-layer already have 64bit parameters: offset is > sometimes int64_t and sometimes uint64_t. bytes parameter is one of > int64_t, uint64_t, int, unsigned int... > > I think we need one type for all of this, and this one type is int64_t. > Signed int64_t is a bit better than uint64_t: you can use same variable > to get some result (including error < 0) and than reuse it as an > argument without any type conversion. > > So, I propose, as a first step, convert all uint64_t parameters to > int64_t. > > Still, I don't have good idea of how to split this into more than 3 > patches, so, this is an RFC. > > What's next? > > Converting write_zero and discard is not as simple: we can't just > s/int/uint64_t/, as some functions may use some int variables for > calculations and this will be broken by something larger than int. > > So, I think the simplest way is to add .bdrv_co_pwritev_zeros64 and > .bdrv_co_pdiscard64 and update drivers one-by-one. If at some point all > drivers updated - drop unused 32bit functions, and then drop "64" suffix > from API. If not - we'll live with both APIs. > > Another thing to do is updating default limiting of request (currently > they are limited to INT_MAX). > > Then we may move some drivers to 64bit discard/write_zero: I think about > qcow2, file-posix and nbd (as a proof-of-concept for already proposed > NBD extension). Sounds good to me. This is a good series to merge early in the QEMU 5.1 release cycle. That way we'll have time to catch any issues. Stefan
30.03.2020 17:18, Vladimir Sementsov-Ogievskiy wrote: > Hi all! > > There is an idea to make NBD protocol extension to support 64bit > write-zero/discard/block-status commands (commands, which doesn't > transfer user data). It's needed to increase performance of zeroing > large ranges (up to the whole image). Zeroing of the whole image is used > as first step of mirror job, qemu-img convert, it should be also used at > start of backup actually.. > > We need to support it in block-layer, so we want 64bit write_zeros. > Currently driver handler now have int bytes parameter. > > write_zeros path goes through normal pwritev, so we need 64bit write, > and then we need 64bit read for symmetry, and better, let's make all io > path work with 64bit bytes parameter. > > Actually most of block-layer already have 64bit parameters: offset is > sometimes int64_t and sometimes uint64_t. bytes parameter is one of > int64_t, uint64_t, int, unsigned int... > > I think we need one type for all of this, and this one type is int64_t. > Signed int64_t is a bit better than uint64_t: you can use same variable > to get some result (including error < 0) and than reuse it as an > argument without any type conversion. > > So, I propose, as a first step, convert all uint64_t parameters to > int64_t. > > Still, I don't have good idea of how to split this into more than 3 > patches, so, this is an RFC. > > What's next? > > Converting write_zero and discard is not as simple: we can't just > s/int/uint64_t/, as some functions may use some int variables for > calculations and this will be broken by something larger than int. > > So, I think the simplest way is to add .bdrv_co_pwritev_zeros64 and > .bdrv_co_pdiscard64 and update drivers one-by-one. If at some point all > drivers updated - drop unused 32bit functions, and then drop "64" suffix > from API. If not - we'll live with both APIs. Hmm. Or, maybe nothing dangerous if we convert it to 64bit, and add comment, that function is not actually prepared for 64bit bytes and depends on max_transfer/max_zeroes being <= INT_MAX ? Or, maybe better, keep old functions as is and add 64bit wrappers, which assert that bytes < INT_MAX, and call old function? This is clean, driver maybe updated later on demand, and we don't need extra API. If no objections, I'll try this way in the next version. > > Another thing to do is updating default limiting of request (currently > they are limited to INT_MAX). > > Then we may move some drivers to 64bit discard/write_zero: I think about > qcow2, file-posix and nbd (as a proof-of-concept for already proposed > NBD extension). > > Any ideas? > > Vladimir Sementsov-Ogievskiy (3): > block: use int64_t as bytes type in tracked requests > block/io: convert generic io path to use int64_t parameters > block: use int64_t instead of uint64_t in driver handlers > > include/block/block.h | 8 ++-- > include/block/block_int.h | 52 ++++++++++----------- > block/backup-top.c | 5 +- > block/blkdebug.c | 4 +- > block/blklogwrites.c | 4 +- > block/blkreplay.c | 4 +- > block/blkverify.c | 6 +-- > block/bochs.c | 2 +- > block/cloop.c | 2 +- > block/commit.c | 2 +- > block/copy-on-read.c | 4 +- > block/crypto.c | 4 +- > block/curl.c | 2 +- > block/dmg.c | 2 +- > block/file-posix.c | 18 ++++---- > block/filter-compress.c | 6 +-- > block/io.c | 97 ++++++++++++++++++++------------------- > block/iscsi.c | 12 ++--- > block/mirror.c | 4 +- > block/nbd.c | 8 ++-- > block/nfs.c | 8 ++-- > block/null.c | 8 ++-- > block/nvme.c | 4 +- > block/qcow.c | 12 ++--- > block/qcow2.c | 18 ++++---- > block/quorum.c | 8 ++-- > block/raw-format.c | 28 +++++------ > block/rbd.c | 4 +- > block/throttle.c | 4 +- > block/vdi.c | 4 +- > block/vmdk.c | 8 ++-- > block/vpc.c | 4 +- > block/vvfat.c | 6 +-- > tests/test-bdrv-drain.c | 8 ++-- > block/trace-events | 14 +++--- > 35 files changed, 192 insertions(+), 192 deletions(-) > -- Best regards, Vladimir
On 4/22/20 1:24 PM, Vladimir Sementsov-Ogievskiy wrote: >> So, I think the simplest way is to add .bdrv_co_pwritev_zeros64 and >> .bdrv_co_pdiscard64 and update drivers one-by-one. If at some point all >> drivers updated - drop unused 32bit functions, and then drop "64" suffix >> from API. If not - we'll live with both APIs. > > Hmm. Or, maybe nothing dangerous if we convert it to 64bit, and add > comment, > that function is not actually prepared for 64bit bytes and depends on > max_transfer/max_zeroes being <= INT_MAX ? > > Or, maybe better, keep old functions as is and add 64bit wrappers, which > assert that bytes < INT_MAX, and call old function? This is clean, driver > maybe updated later on demand, and we don't need extra API. If no > objections, > I'll try this way in the next version. That approach sounds good; it matches how we added flags and iov support, as well as how we transitioned from sector interfaces over to byte interfaces: we added a new .bdrv_ callback for the drivers that could take advantage of the increased interface, without having to touch the older drivers using the old callbacks, then only later finally got rid of the old interfaces as we modernized other drivers. There's still the issue of how much we convert in the initial series - even if adding a new wrapper makes it easier to patch only one driver at a time, it's not nice to leave the job half-done by not visiting all of the drivers eventually. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Am 30.03.2020 um 16:18 hat Vladimir Sementsov-Ogievskiy geschrieben: > Hi all! > > There is an idea to make NBD protocol extension to support 64bit > write-zero/discard/block-status commands (commands, which doesn't > transfer user data). It's needed to increase performance of zeroing > large ranges (up to the whole image). Zeroing of the whole image is used > as first step of mirror job, qemu-img convert, it should be also used at > start of backup actually.. > > We need to support it in block-layer, so we want 64bit write_zeros. > Currently driver handler now have int bytes parameter. > > write_zeros path goes through normal pwritev, so we need 64bit write, > and then we need 64bit read for symmetry, and better, let's make all io > path work with 64bit bytes parameter. > > Actually most of block-layer already have 64bit parameters: offset is > sometimes int64_t and sometimes uint64_t. bytes parameter is one of > int64_t, uint64_t, int, unsigned int... > > I think we need one type for all of this, and this one type is int64_t. > Signed int64_t is a bit better than uint64_t: you can use same variable > to get some result (including error < 0) and than reuse it as an > argument without any type conversion. > > So, I propose, as a first step, convert all uint64_t parameters to > int64_t. > > Still, I don't have good idea of how to split this into more than 3 > patches, so, this is an RFC. I think the split in three patches isn't too bad because it's not a whole lot of code. But of course, it is little code that has lots of implications which does make it hard to review. If we think that we might bisect a bug in the series later, maybe it would be better to split it into more patches. write/write_zeroes has to be a single thing, I'm afraid. But I guess read could be a separate patch, as could be copy_range. Not sure about discard. > What's next? > > Converting write_zero and discard is not as simple: we can't just > s/int/uint64_t/, as some functions may use some int variables for > calculations and this will be broken by something larger than int. > > So, I think the simplest way is to add .bdrv_co_pwritev_zeros64 and > .bdrv_co_pdiscard64 and update drivers one-by-one. If at some point all > drivers updated - drop unused 32bit functions, and then drop "64" suffix > from API. If not - we'll live with both APIs. We already have too many unfinished conversions in QEMU, let's not add one more. Fortunately, we already have a tool that could help us here: Things like bs->bl.max_pwrite_zeroes. We could make BDRV_REQUEST_MAX_BYTES the default value and only drivers that override it can get bigger requests. > Another thing to do is updating default limiting of request (currently > they are limited to INT_MAX). As above, I wouldn't update the default, but rather enable drivers to overload the default with a larger value. This will involve changing some places where we use MIN() between INT_MAX and the driver's value. > Then we may move some drivers to 64bit discard/write_zero: I think about > qcow2, file-posix and nbd (as a proof-of-concept for already proposed > NBD extension). Makes sense to me. Kevin
Patchew URL: https://patchew.org/QEMU/20200330141818.31294-1-vsementsov@virtuozzo.com/
Hi,
This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===
clang -iquote /tmp/qemu-test/build/tests -iquote tests -iquote /tmp/qemu-test/src/tcg/i386 -isystem /tmp/qemu-test/src/linux-headers -isystem /tmp/qemu-test/build/linux-headers -iquote . -iquote /tmp/qemu-test/src -iquote /tmp/qemu-test/src/accel/tcg -iquote /tmp/qemu-test/src/include -iquote /tmp/qemu-test/src/disas/libvixl -I/usr/include/pixman-1 -Werror -fsanitize=undefined -fsanitize=address -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -Wno-string-plus-int -Wno-typedef-redefinition -Wno-initializer-overrides -Wexpansion-to-defined -Wendif-labels -Wno-shift-negative-value -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-definition -Wtype-limits -fstack-protector-strong -I/usr/include/p11-kit-1 -DLEGACY_RDMA_REG_MR -DSTRUCT_IOVEC_DEFINED -I/usr/include/libpng16 -I/usr/include/spice-1 -I/usr/include/spice-server -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 -I/usr/include/nspr4 -pthread -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/uuid -I/usr/include/pixman-1 -I/tmp/qemu-test/src/tests -I/tmp/qemu-test/src/tests/qtest -MMD -MP -MT tests/test-crypto-hash.o -MF tests/test-crypto-hash.d -g -c -o tests/test-crypto-hash.o /tmp/qemu-test/src/tests/test-crypto-hash.c
clang -iquote /tmp/qemu-test/build/tests -iquote tests -iquote /tmp/qemu-test/src/tcg/i386 -isystem /tmp/qemu-test/src/linux-headers -isystem /tmp/qemu-test/build/linux-headers -iquote . -iquote /tmp/qemu-test/src -iquote /tmp/qemu-test/src/accel/tcg -iquote /tmp/qemu-test/src/include -iquote /tmp/qemu-test/src/disas/libvixl -I/usr/include/pixman-1 -Werror -fsanitize=undefined -fsanitize=address -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -Wno-string-plus-int -Wno-typedef-redefinition -Wno-initializer-overrides -Wexpansion-to-defined -Wendif-labels -Wno-shift-negative-value -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-definition -Wtype-limits -fstack-protector-strong -I/usr/include/p11-kit-1 -DLEGACY_RDMA_REG_MR -DSTRUCT_IOVEC_DEFINED -I/usr/include/libpng16 -I/usr/include/spice-1 -I/usr/include/spice-server -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 -I/usr/include/nspr4 -pthread -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/uuid -I/usr/include/pixman-1 -I/tmp/qemu-test/src/tests -I/tmp/qemu-test/src/tests/qtest -MMD -MP -MT tests/test-crypto-hmac.o -MF tests/test-crypto-hmac.d -g -c -o tests/test-crypto-hmac.o /tmp/qemu-test/src/tests/test-crypto-hmac.c
clang -iquote /tmp/qemu-test/build/tests -iquote tests -iquote /tmp/qemu-test/src/tcg/i386 -isystem /tmp/qemu-test/src/linux-headers -isystem /tmp/qemu-test/build/linux-headers -iquote . -iquote /tmp/qemu-test/src -iquote /tmp/qemu-test/src/accel/tcg -iquote /tmp/qemu-test/src/include -iquote /tmp/qemu-test/src/disas/libvixl -I/usr/include/pixman-1 -Werror -fsanitize=undefined -fsanitize=address -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -Wno-string-plus-int -Wno-typedef-redefinition -Wno-initializer-overrides -Wexpansion-to-defined -Wendif-labels -Wno-shift-negative-value -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-definition -Wtype-limits -fstack-protector-strong -I/usr/include/p11-kit-1 -DLEGACY_RDMA_REG_MR -DSTRUCT_IOVEC_DEFINED -I/usr/include/libpng16 -I/usr/include/spice-1 -I/usr/include/spice-server -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 -I/usr/include/nspr4 -pthread -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/uuid -I/usr/include/pixman-1 -I/tmp/qemu-test/src/tests -I/tmp/qemu-test/src/tests/qtest -MMD -MP -MT tests/test-crypto-cipher.o -MF tests/test-crypto-cipher.d -g -c -o tests/test-crypto-cipher.o /tmp/qemu-test/src/tests/test-crypto-cipher.c
/tmp/qemu-test/src/tests/test-block-iothread.c:68:31: error: incompatible pointer types initializing 'int (*)(BlockDriverState *, int64_t, int64_t, QEMUIOVector *, int)' (aka 'int (*)(struct BlockDriverState *, long, long, struct QEMUIOVector *, int)') with an expression of type 'int (BlockDriverState *, uint64_t, uint64_t, QEMUIOVector *, int)' (aka 'int (struct BlockDriverState *, unsigned long, unsigned long, struct QEMUIOVector *, int)') [-Werror,-Wincompatible-pointer-types]
.bdrv_co_preadv = bdrv_test_co_prwv,
^~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/tests/test-block-iothread.c:69:31: error: incompatible pointer types initializing 'int (*)(BlockDriverState *, int64_t, int64_t, QEMUIOVector *, int)' (aka 'int (*)(struct BlockDriverState *, long, long, struct QEMUIOVector *, int)') with an expression of type 'int (BlockDriverState *, uint64_t, uint64_t, QEMUIOVector *, int)' (aka 'int (struct BlockDriverState *, unsigned long, unsigned long, struct QEMUIOVector *, int)') [-Werror,-Wincompatible-pointer-types]
.bdrv_co_pwritev = bdrv_test_co_prwv,
^~~~~~~~~~~~~~~~~
2 errors generated.
make: *** [/tmp/qemu-test/src/rules.mak:69: tests/test-block-iothread.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "./tests/docker/docker.py", line 664, in <module>
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=733d780c712b4d27a92e0973628b05b2', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-c0tiulxy/src/docker-src.2020-03-30-13.37.39.18228:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=733d780c712b4d27a92e0973628b05b2
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-c0tiulxy/src'
make: *** [docker-run-test-debug@fedora] Error 2
real 5m21.587s
user 0m8.451s
The full log is available at
http://patchew.org/logs/20200330141818.31294-1-vsementsov@virtuozzo.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Patchew URL: https://patchew.org/QEMU/20200330141818.31294-1-vsementsov@virtuozzo.com/
Hi,
This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
CC tests/test-int128.o
CC tests/rcutorture.o
CC tests/test-rcu-list.o
/tmp/qemu-test/src/tests/test-block-iothread.c:68:5: error: initialization from incompatible pointer type [-Werror]
.bdrv_co_preadv = bdrv_test_co_prwv,
^
/tmp/qemu-test/src/tests/test-block-iothread.c:68:5: error: (near initialization for 'bdrv_test.bdrv_co_preadv') [-Werror]
/tmp/qemu-test/src/tests/test-block-iothread.c:69:5: error: initialization from incompatible pointer type [-Werror]
.bdrv_co_pwritev = bdrv_test_co_prwv,
^
/tmp/qemu-test/src/tests/test-block-iothread.c:69:5: error: (near initialization for 'bdrv_test.bdrv_co_pwritev') [-Werror]
cc1: all warnings being treated as errors
make: *** [tests/test-block-iothread.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "./tests/docker/docker.py", line 664, in <module>
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=ac3e5859c1b54c68988c07cda02958cd', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-4cxd8d3k/src/docker-src.2020-03-30-13.44.40.29863:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=ac3e5859c1b54c68988c07cda02958cd
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-4cxd8d3k/src'
make: *** [docker-run-test-quick@centos7] Error 2
real 3m30.965s
user 0m8.371s
The full log is available at
http://patchew.org/logs/20200330141818.31294-1-vsementsov@virtuozzo.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Any thoughts here? I need to resend to update some more functions as patchew said. Is it OK in general? Or should we instead convert everything to uint64_t ? 30.03.2020 17:18, Vladimir Sementsov-Ogievskiy wrote: > Hi all! > > There is an idea to make NBD protocol extension to support 64bit > write-zero/discard/block-status commands (commands, which doesn't > transfer user data). It's needed to increase performance of zeroing > large ranges (up to the whole image). Zeroing of the whole image is used > as first step of mirror job, qemu-img convert, it should be also used at > start of backup actually.. > > We need to support it in block-layer, so we want 64bit write_zeros. > Currently driver handler now have int bytes parameter. > > write_zeros path goes through normal pwritev, so we need 64bit write, > and then we need 64bit read for symmetry, and better, let's make all io > path work with 64bit bytes parameter. > > Actually most of block-layer already have 64bit parameters: offset is > sometimes int64_t and sometimes uint64_t. bytes parameter is one of > int64_t, uint64_t, int, unsigned int... > > I think we need one type for all of this, and this one type is int64_t. > Signed int64_t is a bit better than uint64_t: you can use same variable > to get some result (including error < 0) and than reuse it as an > argument without any type conversion. > > So, I propose, as a first step, convert all uint64_t parameters to > int64_t. > > Still, I don't have good idea of how to split this into more than 3 > patches, so, this is an RFC. > > What's next? > > Converting write_zero and discard is not as simple: we can't just > s/int/uint64_t/, as some functions may use some int variables for > calculations and this will be broken by something larger than int. > > So, I think the simplest way is to add .bdrv_co_pwritev_zeros64 and > .bdrv_co_pdiscard64 and update drivers one-by-one. If at some point all > drivers updated - drop unused 32bit functions, and then drop "64" suffix > from API. If not - we'll live with both APIs. > > Another thing to do is updating default limiting of request (currently > they are limited to INT_MAX). > > Then we may move some drivers to 64bit discard/write_zero: I think about > qcow2, file-posix and nbd (as a proof-of-concept for already proposed > NBD extension). > > Any ideas? > > Vladimir Sementsov-Ogievskiy (3): > block: use int64_t as bytes type in tracked requests > block/io: convert generic io path to use int64_t parameters > block: use int64_t instead of uint64_t in driver handlers > > include/block/block.h | 8 ++-- > include/block/block_int.h | 52 ++++++++++----------- > block/backup-top.c | 5 +- > block/blkdebug.c | 4 +- > block/blklogwrites.c | 4 +- > block/blkreplay.c | 4 +- > block/blkverify.c | 6 +-- > block/bochs.c | 2 +- > block/cloop.c | 2 +- > block/commit.c | 2 +- > block/copy-on-read.c | 4 +- > block/crypto.c | 4 +- > block/curl.c | 2 +- > block/dmg.c | 2 +- > block/file-posix.c | 18 ++++---- > block/filter-compress.c | 6 +-- > block/io.c | 97 ++++++++++++++++++++------------------- > block/iscsi.c | 12 ++--- > block/mirror.c | 4 +- > block/nbd.c | 8 ++-- > block/nfs.c | 8 ++-- > block/null.c | 8 ++-- > block/nvme.c | 4 +- > block/qcow.c | 12 ++--- > block/qcow2.c | 18 ++++---- > block/quorum.c | 8 ++-- > block/raw-format.c | 28 +++++------ > block/rbd.c | 4 +- > block/throttle.c | 4 +- > block/vdi.c | 4 +- > block/vmdk.c | 8 ++-- > block/vpc.c | 4 +- > block/vvfat.c | 6 +-- > tests/test-bdrv-drain.c | 8 ++-- > block/trace-events | 14 +++--- > 35 files changed, 192 insertions(+), 192 deletions(-) > -- Best regards, Vladimir
On 4/22/20 9:29 AM, Vladimir Sementsov-Ogievskiy wrote: > Any thoughts here? I need to resend to update some more functions as > patchew said. > > Is it OK in general? Or should we instead convert everything to uint64_t ? I definitely prefer int64_t as our base (off_t is signed as well, making 63 bits an effective limit everywhere). > > 30.03.2020 17:18, Vladimir Sementsov-Ogievskiy wrote: >> Hi all! >> >> There is an idea to make NBD protocol extension to support 64bit >> write-zero/discard/block-status commands (commands, which doesn't >> transfer user data). It's needed to increase performance of zeroing >> large ranges (up to the whole image). Zeroing of the whole image is used >> as first step of mirror job, qemu-img convert, it should be also used at >> start of backup actually.. >> >> We need to support it in block-layer, so we want 64bit write_zeros. >> Currently driver handler now have int bytes parameter. >> >> write_zeros path goes through normal pwritev, so we need 64bit write, >> and then we need 64bit read for symmetry, and better, let's make all io >> path work with 64bit bytes parameter. >> >> Actually most of block-layer already have 64bit parameters: offset is >> sometimes int64_t and sometimes uint64_t. bytes parameter is one of >> int64_t, uint64_t, int, unsigned int... >> >> I think we need one type for all of this, and this one type is int64_t. >> Signed int64_t is a bit better than uint64_t: you can use same variable >> to get some result (including error < 0) and than reuse it as an >> argument without any type conversion. >> >> So, I propose, as a first step, convert all uint64_t parameters to >> int64_t. Makes sense. I haven't looked at the series closely in part because it was 5.1 material while we were still focused on getting 5.0 out the door, but it is now raising in my review queue again. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
© 2016 - 2024 Red Hat, Inc.