* [PATCH v3 0/4] block: fix detect-zeroes= with BDRV_REQ_REGISTERED_BUF
@ 2023-02-01 15:27 Stefan Hajnoczi
2023-02-01 15:27 ` [PATCH v3 1/4] " Stefan Hajnoczi
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2023-02-01 15:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, qemu-block, Hanna Reitz, Fam Zheng,
Fiona Ebner
v3:
- Restore alphabetical order in getopt strings [Eric]
v2:
- Add comment explaining unbalanced error code path in
qemu_io_alloc_from_file() [Eric]
- List options alphabetically in help output [Eric]
- Add Tested-by/Reviewed-by
- CC qemu-stable on the fix
The first patch fixes a regression in QEMU 7.2 where detect-zeroes breaks with
virtio-blk devices due to a BDRV_REQ_REGISTERED_BUF bug. Details of the
regression can be found here:
https://gitlab.com/qemu-project/qemu/-/issues/1404
The remaining patches add a regression test that will protect this code path in
the future. The qemu-io command is extended with the new -r option that calls
blk_register_buf(). This allows a qemu-iotests test case to trigger the same
bug as virtio-blk.
Stefan Hajnoczi (4):
block: fix detect-zeroes= with BDRV_REQ_REGISTERED_BUF
qemu-io: use BdrvRequestFlags instead of int
qemu-io: add -r option to register I/O buffer
iotests/detect-zeroes-registered-buf: add new test
block/io.c | 3 +
qemu-io-cmds.c | 180 ++++++++++++------
.../tests/detect-zeroes-registered-buf | 58 ++++++
.../tests/detect-zeroes-registered-buf.out | 7 +
4 files changed, 189 insertions(+), 59 deletions(-)
create mode 100755 tests/qemu-iotests/tests/detect-zeroes-registered-buf
create mode 100644 tests/qemu-iotests/tests/detect-zeroes-registered-buf.out
--
2.39.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/4] block: fix detect-zeroes= with BDRV_REQ_REGISTERED_BUF
2023-02-01 15:27 [PATCH v3 0/4] block: fix detect-zeroes= with BDRV_REQ_REGISTERED_BUF Stefan Hajnoczi
@ 2023-02-01 15:27 ` Stefan Hajnoczi
2023-02-07 11:48 ` Hanna Czenczek
2023-02-01 15:27 ` [PATCH v3 2/4] qemu-io: use BdrvRequestFlags instead of int Stefan Hajnoczi
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Stefan Hajnoczi @ 2023-02-01 15:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, qemu-block, Hanna Reitz, Fam Zheng,
Fiona Ebner, qemu-stable, Eric Blake
When a write request is converted into a write zeroes request by the
detect-zeroes= feature, it is no longer associated with an I/O buffer.
The BDRV_REQ_REGISTERED_BUF flag doesn't make sense without an I/O
buffer and must be cleared because bdrv_co_do_pwrite_zeroes() fails with
-EINVAL when it's set.
Fiona Ebner <f.ebner@proxmox.com> bisected and diagnosed this QEMU 7.2
regression where writes containing zeroes to a blockdev with
discard=unmap,detect-zeroes=unmap fail.
Buglink: https://gitlab.com/qemu-project/qemu/-/issues/1404
Fixes: e8b6535533be ("block: add BDRV_REQ_REGISTERED_BUF request flag")
Tested-by: Fiona Ebner <f.ebner@proxmox.com>
Cc: qemu-stable@nongnu.org
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
block/io.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/block/io.c b/block/io.c
index a09a19f7a7..24a2bc42d3 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1926,6 +1926,9 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
if (bs->detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP) {
flags |= BDRV_REQ_MAY_UNMAP;
}
+
+ /* Can't use optimization hint with bufferless zero write */
+ flags &= ~BDRV_REQ_REGISTERED_BUF;
}
if (ret < 0) {
--
2.39.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 2/4] qemu-io: use BdrvRequestFlags instead of int
2023-02-01 15:27 [PATCH v3 0/4] block: fix detect-zeroes= with BDRV_REQ_REGISTERED_BUF Stefan Hajnoczi
2023-02-01 15:27 ` [PATCH v3 1/4] " Stefan Hajnoczi
@ 2023-02-01 15:27 ` Stefan Hajnoczi
2023-02-07 11:47 ` Hanna Czenczek
2023-02-01 15:27 ` [PATCH v3 3/4] qemu-io: add -r option to register I/O buffer Stefan Hajnoczi
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Stefan Hajnoczi @ 2023-02-01 15:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, qemu-block, Hanna Reitz, Fam Zheng,
Fiona Ebner, Eric Blake
The block layer APIs use BdrvRequestFlags while qemu-io code uses int.
Although the code compiles and runs fine, BdrvRequestFlags is clearer
because it differentiates between other types of flags like bdrv_open()
flags.
This is purely refactoring.
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
qemu-io-cmds.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 952dc940f1..c0125d14c0 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -556,7 +556,7 @@ static int do_pread(BlockBackend *blk, char *buf, int64_t offset,
}
static int do_pwrite(BlockBackend *blk, char *buf, int64_t offset,
- int64_t bytes, int flags, int64_t *total)
+ int64_t bytes, BdrvRequestFlags flags, int64_t *total)
{
int ret;
@@ -577,7 +577,7 @@ typedef struct {
int64_t offset;
int64_t bytes;
int64_t *total;
- int flags;
+ BdrvRequestFlags flags;
int ret;
bool done;
} CoWriteZeroes;
@@ -598,7 +598,8 @@ static void coroutine_fn co_pwrite_zeroes_entry(void *opaque)
}
static int do_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
- int64_t bytes, int flags, int64_t *total)
+ int64_t bytes, BdrvRequestFlags flags,
+ int64_t *total)
{
Coroutine *co;
CoWriteZeroes data = {
@@ -688,7 +689,7 @@ static int do_aio_readv(BlockBackend *blk, QEMUIOVector *qiov,
}
static int do_aio_writev(BlockBackend *blk, QEMUIOVector *qiov,
- int64_t offset, int flags, int *total)
+ int64_t offset, BdrvRequestFlags flags, int *total)
{
int async_ret = NOT_DONE;
@@ -1065,7 +1066,7 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
struct timespec t1, t2;
bool Cflag = false, qflag = false, bflag = false;
bool Pflag = false, zflag = false, cflag = false, sflag = false;
- int flags = 0;
+ BdrvRequestFlags flags = 0;
int c, cnt, ret;
char *buf = NULL;
int64_t offset;
@@ -1266,7 +1267,7 @@ static int writev_f(BlockBackend *blk, int argc, char **argv)
{
struct timespec t1, t2;
bool Cflag = false, qflag = false;
- int flags = 0;
+ BdrvRequestFlags flags = 0;
int c, cnt, ret;
char *buf;
int64_t offset;
@@ -1581,7 +1582,7 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv)
int nr_iov, c;
int pattern = 0xcd;
struct aio_ctx *ctx = g_new0(struct aio_ctx, 1);
- int flags = 0;
+ BdrvRequestFlags flags = 0;
ctx->blk = blk;
while ((c = getopt(argc, argv, "CfiqP:uz")) != -1) {
--
2.39.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 3/4] qemu-io: add -r option to register I/O buffer
2023-02-01 15:27 [PATCH v3 0/4] block: fix detect-zeroes= with BDRV_REQ_REGISTERED_BUF Stefan Hajnoczi
2023-02-01 15:27 ` [PATCH v3 1/4] " Stefan Hajnoczi
2023-02-01 15:27 ` [PATCH v3 2/4] qemu-io: use BdrvRequestFlags instead of int Stefan Hajnoczi
@ 2023-02-01 15:27 ` Stefan Hajnoczi
2023-02-07 11:47 ` Hanna Czenczek
2023-02-01 15:27 ` [PATCH v3 4/4] iotests/detect-zeroes-registered-buf: add new test Stefan Hajnoczi
2023-02-06 21:01 ` [PATCH v3 0/4] block: fix detect-zeroes= with BDRV_REQ_REGISTERED_BUF Stefan Hajnoczi
4 siblings, 1 reply; 11+ messages in thread
From: Stefan Hajnoczi @ 2023-02-01 15:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, qemu-block, Hanna Reitz, Fam Zheng,
Fiona Ebner, Eric Blake
The blk_register_buf() API is an optimization hint that allows some
block drivers to avoid I/O buffer housekeeping or bounce buffers.
Add an -r option to register the I/O buffer so that qemu-io can be used
to test the blk_register_buf() API. The next commit will add a test that
uses the new option.
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
qemu-io-cmds.c | 167 +++++++++++++++++++++++++++++++++----------------
1 file changed, 114 insertions(+), 53 deletions(-)
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index c0125d14c0..4b8dbef36d 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -338,7 +338,8 @@ static int parse_pattern(const char *arg)
*/
#define MISALIGN_OFFSET 16
-static void *qemu_io_alloc(BlockBackend *blk, size_t len, int pattern)
+static void *qemu_io_alloc(BlockBackend *blk, size_t len, int pattern,
+ bool register_buf)
{
void *buf;
@@ -347,17 +348,24 @@ static void *qemu_io_alloc(BlockBackend *blk, size_t len, int pattern)
}
buf = blk_blockalign(blk, len);
memset(buf, pattern, len);
+ if (register_buf) {
+ blk_register_buf(blk, buf, len, &error_abort);
+ }
if (qemuio_misalign) {
buf += MISALIGN_OFFSET;
}
return buf;
}
-static void qemu_io_free(void *p)
+static void qemu_io_free(BlockBackend *blk, void *p, size_t len,
+ bool unregister_buf)
{
if (qemuio_misalign) {
p -= MISALIGN_OFFSET;
}
+ if (unregister_buf) {
+ blk_unregister_buf(blk, p, len);
+ }
qemu_vfree(p);
}
@@ -371,12 +379,13 @@ static void qemu_io_free(void *p)
* @blk - the block backend where the buffer content is going to be written to
* @len - the buffer length
* @file_name - the file to read the content from
+ * @register_buf - call blk_register_buf()
*
* Returns: the buffer pointer on success
* NULL on error
*/
static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
- const char *file_name)
+ const char *file_name, bool register_buf)
{
char *buf, *buf_origin;
FILE *f = fopen(file_name, "r");
@@ -414,6 +423,10 @@ static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
fclose(f);
f = NULL;
+ if (register_buf) {
+ blk_register_buf(blk, buf_origin, len, &error_abort);
+ }
+
if (len > pattern_len) {
len -= pattern_len;
buf += pattern_len;
@@ -431,7 +444,11 @@ static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
return buf_origin;
error:
- qemu_io_free(buf_origin);
+ /*
+ * This code path is only taken before blk_register_buf() is called, so
+ * hardcode the qemu_io_free() unregister_buf argument to false.
+ */
+ qemu_io_free(blk, buf_origin, len, false);
if (f) {
fclose(f);
}
@@ -490,7 +507,7 @@ static void print_report(const char *op, struct timespec *t, int64_t offset,
*/
static void *
create_iovec(BlockBackend *blk, QEMUIOVector *qiov, char **argv, int nr_iov,
- int pattern)
+ int pattern, bool register_buf)
{
size_t *sizes = g_new0(size_t, nr_iov);
size_t count = 0;
@@ -526,7 +543,7 @@ create_iovec(BlockBackend *blk, QEMUIOVector *qiov, char **argv, int nr_iov,
qemu_iovec_init(qiov, nr_iov);
- buf = p = qemu_io_alloc(blk, count, pattern);
+ buf = p = qemu_io_alloc(blk, count, pattern, register_buf);
for (i = 0; i < nr_iov; i++) {
qemu_iovec_add(qiov, p, sizes[i]);
@@ -539,7 +556,7 @@ fail:
}
static int do_pread(BlockBackend *blk, char *buf, int64_t offset,
- int64_t bytes, int64_t *total)
+ int64_t bytes, BdrvRequestFlags flags, int64_t *total)
{
int ret;
@@ -547,7 +564,7 @@ static int do_pread(BlockBackend *blk, char *buf, int64_t offset,
return -ERANGE;
}
- ret = blk_pread(blk, offset, bytes, (uint8_t *)buf, 0);
+ ret = blk_pread(blk, offset, bytes, (uint8_t *)buf, flags);
if (ret < 0) {
return ret;
}
@@ -675,11 +692,11 @@ static void aio_rw_done(void *opaque, int ret)
}
static int do_aio_readv(BlockBackend *blk, QEMUIOVector *qiov,
- int64_t offset, int *total)
+ int64_t offset, BdrvRequestFlags flags, int *total)
{
int async_ret = NOT_DONE;
- blk_aio_preadv(blk, offset, qiov, 0, aio_rw_done, &async_ret);
+ blk_aio_preadv(blk, offset, qiov, flags, aio_rw_done, &async_ret);
while (async_ret == NOT_DONE) {
main_loop_wait(false);
}
@@ -719,6 +736,7 @@ static void read_help(void)
" -p, -- ignored for backwards compatibility\n"
" -P, -- use a pattern to verify read data\n"
" -q, -- quiet mode, do not show I/O statistics\n"
+" -r, -- register I/O buffer\n"
" -s, -- start offset for pattern verification (only with -P)\n"
" -v, -- dump buffer to standard output\n"
"\n");
@@ -732,7 +750,7 @@ static const cmdinfo_t read_cmd = {
.cfunc = read_f,
.argmin = 2,
.argmax = -1,
- .args = "[-abCqv] [-P pattern [-s off] [-l len]] off len",
+ .args = "[-abCqrv] [-P pattern [-s off] [-l len]] off len",
.oneline = "reads a number of bytes at a specified offset",
.help = read_help,
};
@@ -750,6 +768,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
int64_t total = 0;
int pattern = 0;
int64_t pattern_offset = 0, pattern_count = 0;
+ BdrvRequestFlags flags = 0;
while ((c = getopt(argc, argv, "bCl:pP:qs:v")) != -1) {
switch (c) {
@@ -780,6 +799,9 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
case 'q':
qflag = true;
break;
+ case 'r':
+ flags |= BDRV_REQ_REGISTERED_BUF;
+ break;
case 's':
sflag = true;
pattern_offset = cvtnum(optarg);
@@ -844,15 +866,20 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
count);
return -EINVAL;
}
+ if (flags & BDRV_REQ_REGISTERED_BUF) {
+ printf("I/O buffer registration is not supported when reading "
+ "from vmstate\n");
+ return -EINVAL;
+ }
}
- buf = qemu_io_alloc(blk, count, 0xab);
+ buf = qemu_io_alloc(blk, count, 0xab, flags & BDRV_REQ_REGISTERED_BUF);
clock_gettime(CLOCK_MONOTONIC, &t1);
if (bflag) {
ret = do_load_vmstate(blk, buf, offset, count, &total);
} else {
- ret = do_pread(blk, buf, offset, count, &total);
+ ret = do_pread(blk, buf, offset, count, flags, &total);
}
clock_gettime(CLOCK_MONOTONIC, &t2);
@@ -889,7 +916,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
print_report("read", &t2, offset, count, total, cnt, Cflag);
out:
- qemu_io_free(buf);
+ qemu_io_free(blk, buf, count, flags & BDRV_REQ_REGISTERED_BUF);
return ret;
}
@@ -907,8 +934,9 @@ static void readv_help(void)
" Uses multiple iovec buffers if more than one byte range is specified.\n"
" -C, -- report statistics in a machine parsable format\n"
" -P, -- use a pattern to verify read data\n"
-" -v, -- dump buffer to standard output\n"
" -q, -- quiet mode, do not show I/O statistics\n"
+" -r, -- register I/O buffer\n"
+" -v, -- dump buffer to standard output\n"
"\n");
}
@@ -919,7 +947,7 @@ static const cmdinfo_t readv_cmd = {
.cfunc = readv_f,
.argmin = 2,
.argmax = -1,
- .args = "[-Cqv] [-P pattern] off len [len..]",
+ .args = "[-Cqrv] [-P pattern] off len [len..]",
.oneline = "reads a number of bytes at a specified offset",
.help = readv_help,
};
@@ -937,8 +965,9 @@ static int readv_f(BlockBackend *blk, int argc, char **argv)
QEMUIOVector qiov;
int pattern = 0;
bool Pflag = false;
+ BdrvRequestFlags flags = 0;
- while ((c = getopt(argc, argv, "CP:qv")) != -1) {
+ while ((c = getopt(argc, argv, "CP:qrv")) != -1) {
switch (c) {
case 'C':
Cflag = true;
@@ -953,6 +982,9 @@ static int readv_f(BlockBackend *blk, int argc, char **argv)
case 'q':
qflag = true;
break;
+ case 'r':
+ flags |= BDRV_REQ_REGISTERED_BUF;
+ break;
case 'v':
vflag = true;
break;
@@ -976,13 +1008,14 @@ static int readv_f(BlockBackend *blk, int argc, char **argv)
optind++;
nr_iov = argc - optind;
- buf = create_iovec(blk, &qiov, &argv[optind], nr_iov, 0xab);
+ buf = create_iovec(blk, &qiov, &argv[optind], nr_iov, 0xab,
+ flags & BDRV_REQ_REGISTERED_BUF);
if (buf == NULL) {
return -EINVAL;
}
clock_gettime(CLOCK_MONOTONIC, &t1);
- ret = do_aio_readv(blk, &qiov, offset, &total);
+ ret = do_aio_readv(blk, &qiov, offset, flags, &total);
clock_gettime(CLOCK_MONOTONIC, &t2);
if (ret < 0) {
@@ -1017,8 +1050,8 @@ static int readv_f(BlockBackend *blk, int argc, char **argv)
print_report("read", &t2, offset, qiov.size, total, cnt, Cflag);
out:
+ qemu_io_free(blk, buf, qiov.size, flags & BDRV_REQ_REGISTERED_BUF);
qemu_iovec_destroy(&qiov);
- qemu_io_free(buf);
return ret;
}
@@ -1035,13 +1068,14 @@ static void write_help(void)
" filled with a set pattern (0xcdcdcdcd).\n"
" -b, -- write to the VM state rather than the virtual disk\n"
" -c, -- write compressed data with blk_write_compressed\n"
+" -C, -- report statistics in a machine parsable format\n"
" -f, -- use Force Unit Access semantics\n"
" -n, -- with -z, don't allow slow fallback\n"
" -p, -- ignored for backwards compatibility\n"
" -P, -- use different pattern to fill file\n"
+" -q, -- quiet mode, do not show I/O statistics\n"
+" -r, -- register I/O buffer\n"
" -s, -- use a pattern file to fill the write buffer\n"
-" -C, -- report statistics in a machine parsable format\n"
-" -q, -- quiet mode, do not show I/O statistics\n"
" -u, -- with -z, allow unmapping\n"
" -z, -- write zeroes using blk_co_pwrite_zeroes\n"
"\n");
@@ -1056,7 +1090,7 @@ static const cmdinfo_t write_cmd = {
.perm = BLK_PERM_WRITE,
.argmin = 2,
.argmax = -1,
- .args = "[-bcCfnquz] [-P pattern | -s source_file] off len",
+ .args = "[-bcCfnqruz] [-P pattern | -s source_file] off len",
.oneline = "writes a number of bytes at a specified offset",
.help = write_help,
};
@@ -1076,7 +1110,7 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
int pattern = 0xcd;
const char *file_name = NULL;
- while ((c = getopt(argc, argv, "bcCfnpP:qs:uz")) != -1) {
+ while ((c = getopt(argc, argv, "bcCfnpP:qrs:uz")) != -1) {
switch (c) {
case 'b':
bflag = true;
@@ -1106,6 +1140,9 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
case 'q':
qflag = true;
break;
+ case 'r':
+ flags |= BDRV_REQ_REGISTERED_BUF;
+ break;
case 's':
sflag = true;
file_name = optarg;
@@ -1185,14 +1222,21 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
}
}
- if (!zflag) {
+ if (zflag) {
+ if (flags & BDRV_REQ_REGISTERED_BUF) {
+ printf("cannot combine zero write with register I/O buffer\n");
+ return -EINVAL;
+ }
+ } else {
if (sflag) {
- buf = qemu_io_alloc_from_file(blk, count, file_name);
+ buf = qemu_io_alloc_from_file(blk, count, file_name,
+ flags & BDRV_REQ_REGISTERED_BUF);
if (!buf) {
return -EINVAL;
}
} else {
- buf = qemu_io_alloc(blk, count, pattern);
+ buf = qemu_io_alloc(blk, count, pattern,
+ flags & BDRV_REQ_REGISTERED_BUF);
}
}
@@ -1226,7 +1270,7 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
out:
if (!zflag) {
- qemu_io_free(buf);
+ qemu_io_free(blk, buf, count, flags & BDRV_REQ_REGISTERED_BUF);
}
return ret;
}
@@ -1243,10 +1287,11 @@ writev_help(void)
"\n"
" Writes into a segment of the currently open file, using a buffer\n"
" filled with a set pattern (0xcdcdcdcd).\n"
-" -P, -- use different pattern to fill file\n"
" -C, -- report statistics in a machine parsable format\n"
" -f, -- use Force Unit Access semantics\n"
+" -P, -- use different pattern to fill file\n"
" -q, -- quiet mode, do not show I/O statistics\n"
+" -r, -- register I/O buffer\n"
"\n");
}
@@ -1258,7 +1303,7 @@ static const cmdinfo_t writev_cmd = {
.perm = BLK_PERM_WRITE,
.argmin = 2,
.argmax = -1,
- .args = "[-Cfq] [-P pattern] off len [len..]",
+ .args = "[-Cfqr] [-P pattern] off len [len..]",
.oneline = "writes a number of bytes at a specified offset",
.help = writev_help,
};
@@ -1277,7 +1322,7 @@ static int writev_f(BlockBackend *blk, int argc, char **argv)
int pattern = 0xcd;
QEMUIOVector qiov;
- while ((c = getopt(argc, argv, "CfqP:")) != -1) {
+ while ((c = getopt(argc, argv, "CfP:qr")) != -1) {
switch (c) {
case 'C':
Cflag = true;
@@ -1288,6 +1333,9 @@ static int writev_f(BlockBackend *blk, int argc, char **argv)
case 'q':
qflag = true;
break;
+ case 'r':
+ flags |= BDRV_REQ_REGISTERED_BUF;
+ break;
case 'P':
pattern = parse_pattern(optarg);
if (pattern < 0) {
@@ -1313,7 +1361,8 @@ static int writev_f(BlockBackend *blk, int argc, char **argv)
optind++;
nr_iov = argc - optind;
- buf = create_iovec(blk, &qiov, &argv[optind], nr_iov, pattern);
+ buf = create_iovec(blk, &qiov, &argv[optind], nr_iov, pattern,
+ flags & BDRV_REQ_REGISTERED_BUF);
if (buf == NULL) {
return -EINVAL;
}
@@ -1338,8 +1387,8 @@ static int writev_f(BlockBackend *blk, int argc, char **argv)
t2 = tsub(t2, t1);
print_report("wrote", &t2, offset, qiov.size, total, cnt, Cflag);
out:
+ qemu_io_free(blk, buf, qiov.size, flags & BDRV_REQ_REGISTERED_BUF);
qemu_iovec_destroy(&qiov);
- qemu_io_free(buf);
return ret;
}
@@ -1355,6 +1404,7 @@ struct aio_ctx {
bool zflag;
BlockAcctCookie acct;
int pattern;
+ BdrvRequestFlags flags;
struct timespec t1;
};
@@ -1384,8 +1434,9 @@ static void aio_write_done(void *opaque, int ret)
ctx->qiov.size, 1, ctx->Cflag);
out:
if (!ctx->zflag) {
- qemu_io_free(ctx->buf);
qemu_iovec_destroy(&ctx->qiov);
+ qemu_io_free(ctx->blk, ctx->buf, ctx->qiov.size,
+ ctx->flags & BDRV_REQ_REGISTERED_BUF);
}
g_free(ctx);
}
@@ -1429,7 +1480,8 @@ static void aio_read_done(void *opaque, int ret)
print_report("read", &t2, ctx->offset, ctx->qiov.size,
ctx->qiov.size, 1, ctx->Cflag);
out:
- qemu_io_free(ctx->buf);
+ qemu_io_free(ctx->blk, ctx->buf, ctx->qiov.size,
+ ctx->flags & BDRV_REQ_REGISTERED_BUF);
qemu_iovec_destroy(&ctx->qiov);
g_free(ctx);
}
@@ -1451,10 +1503,11 @@ static void aio_read_help(void)
" considered successful once the request is submitted, independently\n"
" of potential I/O errors or pattern mismatches.\n"
" -C, -- report statistics in a machine parsable format\n"
-" -P, -- use a pattern to verify read data\n"
" -i, -- treat request as invalid, for exercising stats\n"
-" -v, -- dump buffer to standard output\n"
+" -P, -- use a pattern to verify read data\n"
" -q, -- quiet mode, do not show I/O statistics\n"
+" -r, -- register I/O buffer\n"
+" -v, -- dump buffer to standard output\n"
"\n");
}
@@ -1465,7 +1518,7 @@ static const cmdinfo_t aio_read_cmd = {
.cfunc = aio_read_f,
.argmin = 2,
.argmax = -1,
- .args = "[-Ciqv] [-P pattern] off len [len..]",
+ .args = "[-Ciqrv] [-P pattern] off len [len..]",
.oneline = "asynchronously reads a number of bytes",
.help = aio_read_help,
};
@@ -1476,7 +1529,7 @@ static int aio_read_f(BlockBackend *blk, int argc, char **argv)
struct aio_ctx *ctx = g_new0(struct aio_ctx, 1);
ctx->blk = blk;
- while ((c = getopt(argc, argv, "CP:iqv")) != -1) {
+ while ((c = getopt(argc, argv, "CiP:qrv")) != -1) {
switch (c) {
case 'C':
ctx->Cflag = true;
@@ -1497,6 +1550,9 @@ static int aio_read_f(BlockBackend *blk, int argc, char **argv)
case 'q':
ctx->qflag = true;
break;
+ case 'r':
+ ctx->flags |= BDRV_REQ_REGISTERED_BUF;
+ break;
case 'v':
ctx->vflag = true;
break;
@@ -1523,7 +1579,8 @@ static int aio_read_f(BlockBackend *blk, int argc, char **argv)
optind++;
nr_iov = argc - optind;
- ctx->buf = create_iovec(blk, &ctx->qiov, &argv[optind], nr_iov, 0xab);
+ ctx->buf = create_iovec(blk, &ctx->qiov, &argv[optind], nr_iov, 0xab,
+ ctx->flags & BDRV_REQ_REGISTERED_BUF);
if (ctx->buf == NULL) {
block_acct_invalid(blk_get_stats(blk), BLOCK_ACCT_READ);
g_free(ctx);
@@ -1533,7 +1590,8 @@ static int aio_read_f(BlockBackend *blk, int argc, char **argv)
clock_gettime(CLOCK_MONOTONIC, &ctx->t1);
block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->qiov.size,
BLOCK_ACCT_READ);
- blk_aio_preadv(blk, ctx->offset, &ctx->qiov, 0, aio_read_done, ctx);
+ blk_aio_preadv(blk, ctx->offset, &ctx->qiov, ctx->flags, aio_read_done,
+ ctx);
return 0;
}
@@ -1554,11 +1612,12 @@ static void aio_write_help(void)
" Note that due to its asynchronous nature, this command will be\n"
" considered successful once the request is submitted, independently\n"
" of potential I/O errors or pattern mismatches.\n"
-" -P, -- use different pattern to fill file\n"
" -C, -- report statistics in a machine parsable format\n"
" -f, -- use Force Unit Access semantics\n"
" -i, -- treat request as invalid, for exercising stats\n"
+" -P, -- use different pattern to fill file\n"
" -q, -- quiet mode, do not show I/O statistics\n"
+" -r, -- register I/O buffer\n"
" -u, -- with -z, allow unmapping\n"
" -z, -- write zeroes using blk_aio_pwrite_zeroes\n"
"\n");
@@ -1572,7 +1631,7 @@ static const cmdinfo_t aio_write_cmd = {
.perm = BLK_PERM_WRITE,
.argmin = 2,
.argmax = -1,
- .args = "[-Cfiquz] [-P pattern] off len [len..]",
+ .args = "[-Cfiqruz] [-P pattern] off len [len..]",
.oneline = "asynchronously writes a number of bytes",
.help = aio_write_help,
};
@@ -1582,22 +1641,24 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv)
int nr_iov, c;
int pattern = 0xcd;
struct aio_ctx *ctx = g_new0(struct aio_ctx, 1);
- BdrvRequestFlags flags = 0;
ctx->blk = blk;
- while ((c = getopt(argc, argv, "CfiqP:uz")) != -1) {
+ while ((c = getopt(argc, argv, "CfiP:qruz")) != -1) {
switch (c) {
case 'C':
ctx->Cflag = true;
break;
case 'f':
- flags |= BDRV_REQ_FUA;
+ ctx->flags |= BDRV_REQ_FUA;
break;
case 'q':
ctx->qflag = true;
break;
+ case 'r':
+ ctx->flags |= BDRV_REQ_REGISTERED_BUF;
+ break;
case 'u':
- flags |= BDRV_REQ_MAY_UNMAP;
+ ctx->flags |= BDRV_REQ_MAY_UNMAP;
break;
case 'P':
pattern = parse_pattern(optarg);
@@ -1633,7 +1694,7 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv)
return -EINVAL;
}
- if ((flags & BDRV_REQ_MAY_UNMAP) && !ctx->zflag) {
+ if ((ctx->flags & BDRV_REQ_MAY_UNMAP) && !ctx->zflag) {
printf("-u requires -z to be specified\n");
g_free(ctx);
return -EINVAL;
@@ -1663,12 +1724,12 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv)
}
ctx->qiov.size = count;
- blk_aio_pwrite_zeroes(blk, ctx->offset, count, flags, aio_write_done,
- ctx);
+ blk_aio_pwrite_zeroes(blk, ctx->offset, count, ctx->flags,
+ aio_write_done, ctx);
} else {
nr_iov = argc - optind;
ctx->buf = create_iovec(blk, &ctx->qiov, &argv[optind], nr_iov,
- pattern);
+ pattern, ctx->flags & BDRV_REQ_REGISTERED_BUF);
if (ctx->buf == NULL) {
block_acct_invalid(blk_get_stats(blk), BLOCK_ACCT_WRITE);
g_free(ctx);
@@ -1679,8 +1740,8 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv)
block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->qiov.size,
BLOCK_ACCT_WRITE);
- blk_aio_pwritev(blk, ctx->offset, &ctx->qiov, flags, aio_write_done,
- ctx);
+ blk_aio_pwritev(blk, ctx->offset, &ctx->qiov, ctx->flags,
+ aio_write_done, ctx);
}
return 0;
--
2.39.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 4/4] iotests/detect-zeroes-registered-buf: add new test
2023-02-01 15:27 [PATCH v3 0/4] block: fix detect-zeroes= with BDRV_REQ_REGISTERED_BUF Stefan Hajnoczi
` (2 preceding siblings ...)
2023-02-01 15:27 ` [PATCH v3 3/4] qemu-io: add -r option to register I/O buffer Stefan Hajnoczi
@ 2023-02-01 15:27 ` Stefan Hajnoczi
2023-02-07 11:51 ` Hanna Czenczek
2023-02-06 21:01 ` [PATCH v3 0/4] block: fix detect-zeroes= with BDRV_REQ_REGISTERED_BUF Stefan Hajnoczi
4 siblings, 1 reply; 11+ messages in thread
From: Stefan Hajnoczi @ 2023-02-01 15:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, qemu-block, Hanna Reitz, Fam Zheng,
Fiona Ebner, Eric Blake
This regression test demonstrates that detect-zeroes works with
registered buffers. Bug details:
https://gitlab.com/qemu-project/qemu/-/issues/1404
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
.../tests/detect-zeroes-registered-buf | 58 +++++++++++++++++++
.../tests/detect-zeroes-registered-buf.out | 7 +++
2 files changed, 65 insertions(+)
create mode 100755 tests/qemu-iotests/tests/detect-zeroes-registered-buf
create mode 100644 tests/qemu-iotests/tests/detect-zeroes-registered-buf.out
diff --git a/tests/qemu-iotests/tests/detect-zeroes-registered-buf b/tests/qemu-iotests/tests/detect-zeroes-registered-buf
new file mode 100755
index 0000000000..edb5f2cee5
--- /dev/null
+++ b/tests/qemu-iotests/tests/detect-zeroes-registered-buf
@@ -0,0 +1,58 @@
+#!/usr/bin/env bash
+# group: rw auto quick
+#
+# Check that detect-zeroes=unmap works on writes with registered I/O buffers.
+# This is a regression test for
+# https://gitlab.com/qemu-project/qemu/-/issues/1404 where I/O requests failed
+# unexpectedly.
+#
+# Copyright Red Hat
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=stefanha@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1 # failure is the default!
+
+_cleanup()
+{
+ _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+cd ..
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto generic
+
+size=128M
+_make_test_img $size
+IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,discard=unmap,detect-zeroes=unmap"
+
+echo
+echo "== writing zero buffer to image =="
+QEMU_IO_OPTIONS="$QEMU_IO_OPTIONS_NO_FMT" $QEMU_IO -c "write -r -P 0 0 4k" --image-opts "$IMGSPEC" | _filter_qemu_io
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/tests/detect-zeroes-registered-buf.out b/tests/qemu-iotests/tests/detect-zeroes-registered-buf.out
new file mode 100644
index 0000000000..42c56fcc8d
--- /dev/null
+++ b/tests/qemu-iotests/tests/detect-zeroes-registered-buf.out
@@ -0,0 +1,7 @@
+QA output created by detect-zeroes-registered-buf
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+
+== writing zero buffer to image ==
+wrote 4096/4096 bytes at offset 0
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
--
2.39.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 0/4] block: fix detect-zeroes= with BDRV_REQ_REGISTERED_BUF
2023-02-01 15:27 [PATCH v3 0/4] block: fix detect-zeroes= with BDRV_REQ_REGISTERED_BUF Stefan Hajnoczi
` (3 preceding siblings ...)
2023-02-01 15:27 ` [PATCH v3 4/4] iotests/detect-zeroes-registered-buf: add new test Stefan Hajnoczi
@ 2023-02-06 21:01 ` Stefan Hajnoczi
4 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2023-02-06 21:01 UTC (permalink / raw)
To: Kevin Wolf, hreitz; +Cc: qemu-devel, qemu-block, Fam Zheng, Fiona Ebner
[-- Attachment #1: Type: text/plain, Size: 1684 bytes --]
On Wed, Feb 01, 2023 at 10:27:47AM -0500, Stefan Hajnoczi wrote:
> v3:
> - Restore alphabetical order in getopt strings [Eric]
> v2:
> - Add comment explaining unbalanced error code path in
> qemu_io_alloc_from_file() [Eric]
> - List options alphabetically in help output [Eric]
> - Add Tested-by/Reviewed-by
> - CC qemu-stable on the fix
>
> The first patch fixes a regression in QEMU 7.2 where detect-zeroes breaks with
> virtio-blk devices due to a BDRV_REQ_REGISTERED_BUF bug. Details of the
> regression can be found here:
> https://gitlab.com/qemu-project/qemu/-/issues/1404
>
> The remaining patches add a regression test that will protect this code path in
> the future. The qemu-io command is extended with the new -r option that calls
> blk_register_buf(). This allows a qemu-iotests test case to trigger the same
> bug as virtio-blk.
>
> Stefan Hajnoczi (4):
> block: fix detect-zeroes= with BDRV_REQ_REGISTERED_BUF
> qemu-io: use BdrvRequestFlags instead of int
> qemu-io: add -r option to register I/O buffer
> iotests/detect-zeroes-registered-buf: add new test
>
> block/io.c | 3 +
> qemu-io-cmds.c | 180 ++++++++++++------
> .../tests/detect-zeroes-registered-buf | 58 ++++++
> .../tests/detect-zeroes-registered-buf.out | 7 +
> 4 files changed, 189 insertions(+), 59 deletions(-)
> create mode 100755 tests/qemu-iotests/tests/detect-zeroes-registered-buf
> create mode 100644 tests/qemu-iotests/tests/detect-zeroes-registered-buf.out
Hi Kevin and Hanna,
Would you like to review this or should I go ahead and merge it?
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/4] qemu-io: add -r option to register I/O buffer
2023-02-01 15:27 ` [PATCH v3 3/4] qemu-io: add -r option to register I/O buffer Stefan Hajnoczi
@ 2023-02-07 11:47 ` Hanna Czenczek
2023-02-07 19:32 ` Stefan Hajnoczi
0 siblings, 1 reply; 11+ messages in thread
From: Hanna Czenczek @ 2023-02-07 11:47 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: Kevin Wolf, qemu-block, Fam Zheng, Fiona Ebner, Eric Blake
On 01.02.23 16:27, Stefan Hajnoczi wrote:
> The blk_register_buf() API is an optimization hint that allows some
> block drivers to avoid I/O buffer housekeeping or bounce buffers.
>
> Add an -r option to register the I/O buffer so that qemu-io can be used
> to test the blk_register_buf() API. The next commit will add a test that
> uses the new option.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> qemu-io-cmds.c | 167 +++++++++++++++++++++++++++++++++----------------
> 1 file changed, 114 insertions(+), 53 deletions(-)
>
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index c0125d14c0..4b8dbef36d 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
[...]
> @@ -347,17 +348,24 @@ static void *qemu_io_alloc(BlockBackend *blk, size_t len, int pattern)
> }
> buf = blk_blockalign(blk, len);
> memset(buf, pattern, len);
> + if (register_buf) {
> + blk_register_buf(blk, buf, len, &error_abort);
> + }
> if (qemuio_misalign) {
> buf += MISALIGN_OFFSET;
> }
> return buf;
> }
>
> -static void qemu_io_free(void *p)
> +static void qemu_io_free(BlockBackend *blk, void *p, size_t len,
> + bool unregister_buf)
> {
> if (qemuio_misalign) {
> p -= MISALIGN_OFFSET;
> }
> + if (unregister_buf) {
> + blk_unregister_buf(blk, p, len);
If `qemuio_misalign` is set, we must also increase `len` by
`MISALIGN_OFFSET`, I think, or it won’t match what’s been used in
`qemu_io_alloc()`.
> + }
> qemu_vfree(p);
> }
[...]
> @@ -414,6 +423,10 @@ static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
> fclose(f);
> f = NULL;
>
> + if (register_buf) {
> + blk_register_buf(blk, buf_origin, len, &error_abort);
`qemu_io_alloc()` registers the buffer before `MISALIGN_OFFSET` is/might
be applied, and `qemu_io_free()` assumes this is the case (the buffer to
be unregistered is passed after the offset has been subtracted again).
Here, however, the offset has already been applied, so there’ll be a
mismatch with `blk_unregister_buf()` when `qemu_io_free()` is called
(and `qemuio_misalign` is set).
> + }
> +
> if (len > pattern_len) {
> len -= pattern_len;
> buf += pattern_len;
[...]
> @@ -750,6 +768,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
> int64_t total = 0;
> int pattern = 0;
> int64_t pattern_offset = 0, pattern_count = 0;
> + BdrvRequestFlags flags = 0;
>
> while ((c = getopt(argc, argv, "bCl:pP:qs:v")) != -1) {
I think we’ll need the "r" here.
> switch (c) {
[...]
> @@ -1384,8 +1434,9 @@ static void aio_write_done(void *opaque, int ret)
> ctx->qiov.size, 1, ctx->Cflag);
> out:
> if (!ctx->zflag) {
> - qemu_io_free(ctx->buf);
> qemu_iovec_destroy(&ctx->qiov);
> + qemu_io_free(ctx->blk, ctx->buf, ctx->qiov.size,
> + ctx->flags & BDRV_REQ_REGISTERED_BUF);
So far in this patch, you’ve always swapped the existing
qemu_iovec_destroy(); qemu_io_free() combination to qemu_io_free();
qemu_iovec_destroy(). I think that is correct because
qemu_iovec_destroy() overwrites the qiov by 0, so that accessing
qiov.size will then read 0, regardless of what it was before.
Here, you’re swapping it the other way around, which means that
`ctx->qiov.size` will read 0 when `qemu_io_free()` is called, which
seems wrong.
> }
> g_free(ctx);
> }
[...]
> @@ -1663,12 +1724,12 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv)
> }
>
> ctx->qiov.size = count;
> - blk_aio_pwrite_zeroes(blk, ctx->offset, count, flags, aio_write_done,
> - ctx);
> + blk_aio_pwrite_zeroes(blk, ctx->offset, count, ctx->flags,
> + aio_write_done, ctx);
write_f() emits an error when -r is used together with -z – why doesn’t
this function, too? (Or, alternatively, why does write_f()? Maybe we
want to check what happens when you call a zero-writing function with
that flag. Or we don’t.)
Hanna
> } else {
> nr_iov = argc - optind;
> ctx->buf = create_iovec(blk, &ctx->qiov, &argv[optind], nr_iov,
> - pattern);
> + pattern, ctx->flags & BDRV_REQ_REGISTERED_BUF);
> if (ctx->buf == NULL) {
> block_acct_invalid(blk_get_stats(blk), BLOCK_ACCT_WRITE);
> g_free(ctx);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/4] qemu-io: use BdrvRequestFlags instead of int
2023-02-01 15:27 ` [PATCH v3 2/4] qemu-io: use BdrvRequestFlags instead of int Stefan Hajnoczi
@ 2023-02-07 11:47 ` Hanna Czenczek
0 siblings, 0 replies; 11+ messages in thread
From: Hanna Czenczek @ 2023-02-07 11:47 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: Kevin Wolf, qemu-block, Fam Zheng, Fiona Ebner, Eric Blake
On 01.02.23 16:27, Stefan Hajnoczi wrote:
> The block layer APIs use BdrvRequestFlags while qemu-io code uses int.
> Although the code compiles and runs fine, BdrvRequestFlags is clearer
> because it differentiates between other types of flags like bdrv_open()
> flags.
>
> This is purely refactoring.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> qemu-io-cmds.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
With a rebase on264dcbb2b1e5b66d7a5b08662b200c2b315dce0f:
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/4] block: fix detect-zeroes= with BDRV_REQ_REGISTERED_BUF
2023-02-01 15:27 ` [PATCH v3 1/4] " Stefan Hajnoczi
@ 2023-02-07 11:48 ` Hanna Czenczek
0 siblings, 0 replies; 11+ messages in thread
From: Hanna Czenczek @ 2023-02-07 11:48 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: Kevin Wolf, qemu-block, Fam Zheng, Fiona Ebner, qemu-stable, Eric Blake
On 01.02.23 16:27, Stefan Hajnoczi wrote:
> When a write request is converted into a write zeroes request by the
> detect-zeroes= feature, it is no longer associated with an I/O buffer.
> The BDRV_REQ_REGISTERED_BUF flag doesn't make sense without an I/O
> buffer and must be cleared because bdrv_co_do_pwrite_zeroes() fails with
> -EINVAL when it's set.
>
> Fiona Ebner <f.ebner@proxmox.com> bisected and diagnosed this QEMU 7.2
> regression where writes containing zeroes to a blockdev with
> discard=unmap,detect-zeroes=unmap fail.
>
> Buglink: https://gitlab.com/qemu-project/qemu/-/issues/1404
> Fixes: e8b6535533be ("block: add BDRV_REQ_REGISTERED_BUF request flag")
> Tested-by: Fiona Ebner <f.ebner@proxmox.com>
> Cc: qemu-stable@nongnu.org
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> block/io.c | 3 +++
> 1 file changed, 3 insertions(+)
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 4/4] iotests/detect-zeroes-registered-buf: add new test
2023-02-01 15:27 ` [PATCH v3 4/4] iotests/detect-zeroes-registered-buf: add new test Stefan Hajnoczi
@ 2023-02-07 11:51 ` Hanna Czenczek
0 siblings, 0 replies; 11+ messages in thread
From: Hanna Czenczek @ 2023-02-07 11:51 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: Kevin Wolf, qemu-block, Fam Zheng, Fiona Ebner, Eric Blake
On 01.02.23 16:27, Stefan Hajnoczi wrote:
> This regression test demonstrates that detect-zeroes works with
> registered buffers. Bug details:
> https://gitlab.com/qemu-project/qemu/-/issues/1404
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> .../tests/detect-zeroes-registered-buf | 58 +++++++++++++++++++
> .../tests/detect-zeroes-registered-buf.out | 7 +++
> 2 files changed, 65 insertions(+)
> create mode 100755 tests/qemu-iotests/tests/detect-zeroes-registered-buf
> create mode 100644 tests/qemu-iotests/tests/detect-zeroes-registered-buf.out
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/4] qemu-io: add -r option to register I/O buffer
2023-02-07 11:47 ` Hanna Czenczek
@ 2023-02-07 19:32 ` Stefan Hajnoczi
0 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2023-02-07 19:32 UTC (permalink / raw)
To: Hanna Czenczek
Cc: qemu-devel, Kevin Wolf, qemu-block, Fam Zheng, Fiona Ebner, Eric Blake
[-- Attachment #1: Type: text/plain, Size: 4866 bytes --]
On Tue, Feb 07, 2023 at 12:47:06PM +0100, Hanna Czenczek wrote:
> On 01.02.23 16:27, Stefan Hajnoczi wrote:
> > The blk_register_buf() API is an optimization hint that allows some
> > block drivers to avoid I/O buffer housekeeping or bounce buffers.
> >
> > Add an -r option to register the I/O buffer so that qemu-io can be used
> > to test the blk_register_buf() API. The next commit will add a test that
> > uses the new option.
> >
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > qemu-io-cmds.c | 167 +++++++++++++++++++++++++++++++++----------------
> > 1 file changed, 114 insertions(+), 53 deletions(-)
> >
> > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> > index c0125d14c0..4b8dbef36d 100644
> > --- a/qemu-io-cmds.c
> > +++ b/qemu-io-cmds.c
>
> [...]
>
> > @@ -347,17 +348,24 @@ static void *qemu_io_alloc(BlockBackend *blk, size_t len, int pattern)
> > }
> > buf = blk_blockalign(blk, len);
> > memset(buf, pattern, len);
> > + if (register_buf) {
> > + blk_register_buf(blk, buf, len, &error_abort);
> > + }
> > if (qemuio_misalign) {
> > buf += MISALIGN_OFFSET;
> > }
> > return buf;
> > }
> > -static void qemu_io_free(void *p)
> > +static void qemu_io_free(BlockBackend *blk, void *p, size_t len,
> > + bool unregister_buf)
> > {
> > if (qemuio_misalign) {
> > p -= MISALIGN_OFFSET;
> > }
> > + if (unregister_buf) {
> > + blk_unregister_buf(blk, p, len);
>
> If `qemuio_misalign` is set, we must also increase `len` by
> `MISALIGN_OFFSET`, I think, or it won’t match what’s been used in
> `qemu_io_alloc()`.
Good catch, thank you!
>
> > + }
> > qemu_vfree(p);
> > }
>
> [...]
>
> > @@ -414,6 +423,10 @@ static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
> > fclose(f);
> > f = NULL;
> > + if (register_buf) {
> > + blk_register_buf(blk, buf_origin, len, &error_abort);
>
> `qemu_io_alloc()` registers the buffer before `MISALIGN_OFFSET` is/might be
> applied, and `qemu_io_free()` assumes this is the case (the buffer to be
> unregistered is passed after the offset has been subtracted again). Here,
> however, the offset has already been applied, so there’ll be a mismatch with
> `blk_unregister_buf()` when `qemu_io_free()` is called (and
> `qemuio_misalign` is set).
>
> > + }
> > +
> > if (len > pattern_len) {
> > len -= pattern_len;
> > buf += pattern_len;
>
> [...]
>
> > @@ -750,6 +768,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
> > int64_t total = 0;
> > int pattern = 0;
> > int64_t pattern_offset = 0, pattern_count = 0;
> > + BdrvRequestFlags flags = 0;
> > while ((c = getopt(argc, argv, "bCl:pP:qs:v")) != -1) {
>
> I think we’ll need the "r" here.
Oops, thanks!
>
> > switch (c) {
>
> [...]
>
> > @@ -1384,8 +1434,9 @@ static void aio_write_done(void *opaque, int ret)
> > ctx->qiov.size, 1, ctx->Cflag);
> > out:
> > if (!ctx->zflag) {
> > - qemu_io_free(ctx->buf);
> > qemu_iovec_destroy(&ctx->qiov);
> > + qemu_io_free(ctx->blk, ctx->buf, ctx->qiov.size,
> > + ctx->flags & BDRV_REQ_REGISTERED_BUF);
>
> So far in this patch, you’ve always swapped the existing
> qemu_iovec_destroy(); qemu_io_free() combination to qemu_io_free();
> qemu_iovec_destroy(). I think that is correct because qemu_iovec_destroy()
> overwrites the qiov by 0, so that accessing qiov.size will then read 0,
> regardless of what it was before.
>
> Here, you’re swapping it the other way around, which means that
> `ctx->qiov.size` will read 0 when `qemu_io_free()` is called, which seems
> wrong.
Yes, you're right. I will reverse the order here.
>
> > }
> > g_free(ctx);
> > }
>
> [...]
>
> > @@ -1663,12 +1724,12 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv)
> > }
> > ctx->qiov.size = count;
> > - blk_aio_pwrite_zeroes(blk, ctx->offset, count, flags, aio_write_done,
> > - ctx);
> > + blk_aio_pwrite_zeroes(blk, ctx->offset, count, ctx->flags,
> > + aio_write_done, ctx);
>
> write_f() emits an error when -r is used together with -z – why doesn’t this
> function, too? (Or, alternatively, why does write_f()? Maybe we want to
> check what happens when you call a zero-writing function with that flag. Or
> we don’t.)
I added an explicit check in write_f() and forgot to add the same check
to aio_write_f(). Will fix.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-02-07 19:33 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-01 15:27 [PATCH v3 0/4] block: fix detect-zeroes= with BDRV_REQ_REGISTERED_BUF Stefan Hajnoczi
2023-02-01 15:27 ` [PATCH v3 1/4] " Stefan Hajnoczi
2023-02-07 11:48 ` Hanna Czenczek
2023-02-01 15:27 ` [PATCH v3 2/4] qemu-io: use BdrvRequestFlags instead of int Stefan Hajnoczi
2023-02-07 11:47 ` Hanna Czenczek
2023-02-01 15:27 ` [PATCH v3 3/4] qemu-io: add -r option to register I/O buffer Stefan Hajnoczi
2023-02-07 11:47 ` Hanna Czenczek
2023-02-07 19:32 ` Stefan Hajnoczi
2023-02-01 15:27 ` [PATCH v3 4/4] iotests/detect-zeroes-registered-buf: add new test Stefan Hajnoczi
2023-02-07 11:51 ` Hanna Czenczek
2023-02-06 21:01 ` [PATCH v3 0/4] block: fix detect-zeroes= with BDRV_REQ_REGISTERED_BUF Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.