* [PATCH v2 1/5] block/nbd: Fix hang in .bdrv_close()
2020-01-22 16:45 [PATCH v2 0/5] block: Generic file creation fallback Max Reitz
@ 2020-01-22 16:45 ` Max Reitz
2020-01-22 18:55 ` Eric Blake
2020-01-23 22:54 ` Maxim Levitsky
2020-01-22 16:45 ` [PATCH v2 2/5] block: Generic file creation fallback Max Reitz
` (4 subsequent siblings)
5 siblings, 2 replies; 13+ messages in thread
From: Max Reitz @ 2020-01-22 16:45 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Maxim Levitsky, qemu-devel, Max Reitz
When nbd_close() is called from a coroutine, the connection_co never
gets to run, and thus nbd_teardown_connection() hangs.
This is because aio_co_enter() only puts the connection_co into the main
coroutine's wake-up queue, so this main coroutine needs to yield and
wait for connection_co to terminate.
Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/nbd.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/block/nbd.c b/block/nbd.c
index d085554f21..6d3b22f844 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -70,6 +70,7 @@ typedef struct BDRVNBDState {
CoMutex send_mutex;
CoQueue free_sema;
Coroutine *connection_co;
+ Coroutine *teardown_co;
QemuCoSleepState *connection_co_sleep_ns_state;
bool drained;
bool wait_drained_end;
@@ -203,7 +204,15 @@ static void nbd_teardown_connection(BlockDriverState *bs)
qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
}
}
- BDRV_POLL_WHILE(bs, s->connection_co);
+ if (qemu_in_coroutine()) {
+ s->teardown_co = qemu_coroutine_self();
+ /* connection_co resumes us when it terminates */
+ qemu_coroutine_yield();
+ s->teardown_co = NULL;
+ } else {
+ BDRV_POLL_WHILE(bs, s->connection_co);
+ }
+ assert(!s->connection_co);
}
static bool nbd_client_connecting(BDRVNBDState *s)
@@ -395,6 +404,9 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
s->ioc = NULL;
}
+ if (s->teardown_co) {
+ aio_co_wake(s->teardown_co);
+ }
aio_wait_kick();
}
--
2.24.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 1/5] block/nbd: Fix hang in .bdrv_close()
2020-01-22 16:45 ` [PATCH v2 1/5] block/nbd: Fix hang in .bdrv_close() Max Reitz
@ 2020-01-22 18:55 ` Eric Blake
2020-01-23 22:54 ` Maxim Levitsky
1 sibling, 0 replies; 13+ messages in thread
From: Eric Blake @ 2020-01-22 18:55 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On 1/22/20 10:45 AM, Max Reitz wrote:
> When nbd_close() is called from a coroutine, the connection_co never
> gets to run, and thus nbd_teardown_connection() hangs.
>
> This is because aio_co_enter() only puts the connection_co into the main
> coroutine's wake-up queue, so this main coroutine needs to yield and
> wait for connection_co to terminate.
>
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block/nbd.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
I'm assuming it is better to take the entire series through somewhere
other than my NBD tree, since the remaining patches touch more than NBD?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/5] block/nbd: Fix hang in .bdrv_close()
2020-01-22 16:45 ` [PATCH v2 1/5] block/nbd: Fix hang in .bdrv_close() Max Reitz
2020-01-22 18:55 ` Eric Blake
@ 2020-01-23 22:54 ` Maxim Levitsky
1 sibling, 0 replies; 13+ messages in thread
From: Maxim Levitsky @ 2020-01-23 22:54 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On Wed, 2020-01-22 at 17:45 +0100, Max Reitz wrote:
> When nbd_close() is called from a coroutine, the connection_co never
> gets to run, and thus nbd_teardown_connection() hangs.
>
> This is because aio_co_enter() only puts the connection_co into the main
> coroutine's wake-up queue, so this main coroutine needs to yield and
> wait for connection_co to terminate.
Took me a while to understand this back then, good that I explained
my thoughts in the review comment, although the commit message
is alright when you already understand qemu co-routines I guess.
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
Maxim Levitsky
>
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block/nbd.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/block/nbd.c b/block/nbd.c
> index d085554f21..6d3b22f844 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -70,6 +70,7 @@ typedef struct BDRVNBDState {
> CoMutex send_mutex;
> CoQueue free_sema;
> Coroutine *connection_co;
> + Coroutine *teardown_co;
> QemuCoSleepState *connection_co_sleep_ns_state;
> bool drained;
> bool wait_drained_end;
> @@ -203,7 +204,15 @@ static void nbd_teardown_connection(BlockDriverState *bs)
> qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
> }
> }
> - BDRV_POLL_WHILE(bs, s->connection_co);
> + if (qemu_in_coroutine()) {
> + s->teardown_co = qemu_coroutine_self();
> + /* connection_co resumes us when it terminates */
> + qemu_coroutine_yield();
> + s->teardown_co = NULL;
> + } else {
> + BDRV_POLL_WHILE(bs, s->connection_co);
> + }
> + assert(!s->connection_co);
> }
>
> static bool nbd_client_connecting(BDRVNBDState *s)
> @@ -395,6 +404,9 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
> s->ioc = NULL;
> }
>
> + if (s->teardown_co) {
> + aio_co_wake(s->teardown_co);
> + }
> aio_wait_kick();
> }
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 2/5] block: Generic file creation fallback
2020-01-22 16:45 [PATCH v2 0/5] block: Generic file creation fallback Max Reitz
2020-01-22 16:45 ` [PATCH v2 1/5] block/nbd: Fix hang in .bdrv_close() Max Reitz
@ 2020-01-22 16:45 ` Max Reitz
2020-01-23 22:58 ` Maxim Levitsky
2020-01-22 16:45 ` [PATCH v2 3/5] file-posix: Drop hdev_co_create_opts() Max Reitz
` (3 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Max Reitz @ 2020-01-22 16:45 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Maxim Levitsky, qemu-devel, Max Reitz
If a protocol driver does not support image creation, we can see whether
maybe the file exists already. If so, just truncating it will be
sufficient.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 159 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 147 insertions(+), 12 deletions(-)
diff --git a/block.c b/block.c
index 99ce26d64d..e167eca04b 100644
--- a/block.c
+++ b/block.c
@@ -532,20 +532,139 @@ out:
return ret;
}
-int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
+/**
+ * Helper function for bdrv_create_file_fallback(): Resize @blk to at
+ * least the given @minimum_size.
+ *
+ * On success, return @blk's actual length.
+ * Otherwise, return -errno.
+ */
+static int64_t create_file_fallback_truncate(BlockBackend *blk,
+ int64_t minimum_size, Error **errp)
{
- BlockDriver *drv;
+ Error *local_err = NULL;
+ int64_t size;
+ int ret;
+
+ ret = blk_truncate(blk, minimum_size, false, PREALLOC_MODE_OFF, &local_err);
+ if (ret < 0 && ret != -ENOTSUP) {
+ error_propagate(errp, local_err);
+ return ret;
+ }
+
+ size = blk_getlength(blk);
+ if (size < 0) {
+ error_free(local_err);
+ error_setg_errno(errp, -size,
+ "Failed to inquire the new image file's length");
+ return size;
+ }
+
+ if (size < minimum_size) {
+ /* Need to grow the image, but we failed to do that */
+ error_propagate(errp, local_err);
+ return -ENOTSUP;
+ }
+
+ error_free(local_err);
+ local_err = NULL;
+
+ return size;
+}
+
+/**
+ * Helper function for bdrv_create_file_fallback(): Zero the first
+ * sector to remove any potentially pre-existing image header.
+ */
+static int create_file_fallback_zero_first_sector(BlockBackend *blk,
+ int64_t current_size,
+ Error **errp)
+{
+ int64_t bytes_to_clear;
+ int ret;
+
+ bytes_to_clear = MIN(current_size, BDRV_SECTOR_SIZE);
+ if (bytes_to_clear) {
+ ret = blk_pwrite_zeroes(blk, 0, bytes_to_clear, BDRV_REQ_MAY_UNMAP);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret,
+ "Failed to clear the new image's first sector");
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static int bdrv_create_file_fallback(const char *filename, BlockDriver *drv,
+ QemuOpts *opts, Error **errp)
+{
+ BlockBackend *blk;
+ QDict *options = qdict_new();
+ int64_t size = 0;
+ char *buf = NULL;
+ PreallocMode prealloc;
Error *local_err = NULL;
int ret;
+ size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
+ buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
+ prealloc = qapi_enum_parse(&PreallocMode_lookup, buf,
+ PREALLOC_MODE_OFF, &local_err);
+ g_free(buf);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return -EINVAL;
+ }
+
+ if (prealloc != PREALLOC_MODE_OFF) {
+ error_setg(errp, "Unsupported preallocation mode '%s'",
+ PreallocMode_str(prealloc));
+ return -ENOTSUP;
+ }
+
+ qdict_put_str(options, "driver", drv->format_name);
+
+ blk = blk_new_open(filename, NULL, options,
+ BDRV_O_RDWR | BDRV_O_RESIZE, errp);
+ if (!blk) {
+ error_prepend(errp, "Protocol driver '%s' does not support image "
+ "creation, and opening the image failed: ",
+ drv->format_name);
+ return -EINVAL;
+ }
+
+ size = create_file_fallback_truncate(blk, size, errp);
+ if (size < 0) {
+ ret = size;
+ goto out;
+ }
+
+ ret = create_file_fallback_zero_first_sector(blk, size, errp);
+ if (ret < 0) {
+ goto out;
+ }
+
+ ret = 0;
+out:
+ blk_unref(blk);
+ return ret;
+}
+
+int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
+{
+ BlockDriver *drv;
+
drv = bdrv_find_protocol(filename, true, errp);
if (drv == NULL) {
return -ENOENT;
}
- ret = bdrv_create(drv, filename, opts, &local_err);
- error_propagate(errp, local_err);
- return ret;
+ if (drv->bdrv_co_create_opts) {
+ return bdrv_create(drv, filename, opts, errp);
+ } else {
+ return bdrv_create_file_fallback(filename, drv, opts, errp);
+ }
}
/**
@@ -1422,6 +1541,24 @@ QemuOptsList bdrv_runtime_opts = {
},
};
+static QemuOptsList fallback_create_opts = {
+ .name = "fallback-create-opts",
+ .head = QTAILQ_HEAD_INITIALIZER(fallback_create_opts.head),
+ .desc = {
+ {
+ .name = BLOCK_OPT_SIZE,
+ .type = QEMU_OPT_SIZE,
+ .help = "Virtual disk size"
+ },
+ {
+ .name = BLOCK_OPT_PREALLOC,
+ .type = QEMU_OPT_STRING,
+ .help = "Preallocation mode (allowed values: off)"
+ },
+ { /* end of list */ }
+ }
+};
+
/*
* Common part for opening disk images and files
*
@@ -5749,15 +5886,13 @@ void bdrv_img_create(const char *filename, const char *fmt,
return;
}
- if (!proto_drv->create_opts) {
- error_setg(errp, "Protocol driver '%s' does not support image creation",
- proto_drv->format_name);
- return;
- }
-
/* Create parameter list */
create_opts = qemu_opts_append(create_opts, drv->create_opts);
- create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
+ if (proto_drv->create_opts) {
+ create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
+ } else {
+ create_opts = qemu_opts_append(create_opts, &fallback_create_opts);
+ }
opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
--
2.24.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 2/5] block: Generic file creation fallback
2020-01-22 16:45 ` [PATCH v2 2/5] block: Generic file creation fallback Max Reitz
@ 2020-01-23 22:58 ` Maxim Levitsky
0 siblings, 0 replies; 13+ messages in thread
From: Maxim Levitsky @ 2020-01-23 22:58 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On Wed, 2020-01-22 at 17:45 +0100, Max Reitz wrote:
> If a protocol driver does not support image creation, we can see whether
> maybe the file exists already. If so, just truncating it will be
> sufficient.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block.c | 159 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 147 insertions(+), 12 deletions(-)
>
> diff --git a/block.c b/block.c
> index 99ce26d64d..e167eca04b 100644
> --- a/block.c
> +++ b/block.c
> @@ -532,20 +532,139 @@ out:
> return ret;
> }
>
> -int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
> +/**
> + * Helper function for bdrv_create_file_fallback(): Resize @blk to at
> + * least the given @minimum_size.
> + *
> + * On success, return @blk's actual length.
> + * Otherwise, return -errno.
> + */
> +static int64_t create_file_fallback_truncate(BlockBackend *blk,
> + int64_t minimum_size, Error **errp)
> {
> - BlockDriver *drv;
> + Error *local_err = NULL;
> + int64_t size;
> + int ret;
> +
> + ret = blk_truncate(blk, minimum_size, false, PREALLOC_MODE_OFF, &local_err);
> + if (ret < 0 && ret != -ENOTSUP) {
> + error_propagate(errp, local_err);
> + return ret;
> + }
> +
...
> + size = blk_getlength(blk);
> + if (size < 0) {
> + error_free(local_err);
> + error_setg_errno(errp, -size,
> + "Failed to inquire the new image file's length");
> + return size;
> + }
> +
> + if (size < minimum_size) {
> + /* Need to grow the image, but we failed to do that */
> + error_propagate(errp, local_err);
> + return -ENOTSUP;
> + }
Very minor nitpick:
The above code basically handles case when truncate is not supported,
by trying to see file size is large enough anyway.
If truncate succeed this is a bit redundant, but doesn't hurt to be honest.
If truncate is not supported, it also works, but I am thinking that
maybe its is better to create a generic truncate failback instead
that will 'work' when asked for same size resize or smaller that existing size + exact=false.
Currently when the above failback doesn't work (meaning that we indeed have to enlarge the file)
we get this error message because truncate is not supported.
[root@fedora31vm ~/qemu]# qemu-img create -f raw nvme://0000:03:00.0 10000M
Formatting 'nvme://0000:03:00.0', fmt=raw size=10485760000
qemu-img: nvme://0000:03:00.0: Image format driver does not support resize
If we had generic truncate failback, it could maybe be smarter about this and say something like
'Can increase the size of the image'
But if you feel like that is not important, I don't have any issue to keep this as is,
since the code does work.
> + error_free(local_err);
> + local_err = NULL;
> +
> + return size;
> +}
> +
> +/**
> + * Helper function for bdrv_create_file_fallback(): Zero the first
> + * sector to remove any potentially pre-existing image header.
> + */
> +static int create_file_fallback_zero_first_sector(BlockBackend *blk,
> + int64_t current_size,
> + Error **errp)
> +{
> + int64_t bytes_to_clear;
> + int ret;
> +
> + bytes_to_clear = MIN(current_size, BDRV_SECTOR_SIZE);
> + if (bytes_to_clear) {
> + ret = blk_pwrite_zeroes(blk, 0, bytes_to_clear, BDRV_REQ_MAY_UNMAP);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret,
> + "Failed to clear the new image's first sector");
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int bdrv_create_file_fallback(const char *filename, BlockDriver *drv,
> + QemuOpts *opts, Error **errp)
> +{
> + BlockBackend *blk;
> + QDict *options = qdict_new();
> + int64_t size = 0;
> + char *buf = NULL;
> + PreallocMode prealloc;
> Error *local_err = NULL;
> int ret;
>
> + size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
> + buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
> + prealloc = qapi_enum_parse(&PreallocMode_lookup, buf,
> + PREALLOC_MODE_OFF, &local_err);
> + g_free(buf);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return -EINVAL;
> + }
> +
> + if (prealloc != PREALLOC_MODE_OFF) {
> + error_setg(errp, "Unsupported preallocation mode '%s'",
> + PreallocMode_str(prealloc));
> + return -ENOTSUP;
> + }
> +
> + qdict_put_str(options, "driver", drv->format_name);
> +
> + blk = blk_new_open(filename, NULL, options,
> + BDRV_O_RDWR | BDRV_O_RESIZE, errp);
> + if (!blk) {
> + error_prepend(errp, "Protocol driver '%s' does not support image "
> + "creation, and opening the image failed: ",
> + drv->format_name);
> + return -EINVAL;
> + }
> +
> + size = create_file_fallback_truncate(blk, size, errp);
> + if (size < 0) {
> + ret = size;
> + goto out;
> + }
> +
> + ret = create_file_fallback_zero_first_sector(blk, size, errp);
> + if (ret < 0) {
> + goto out;
> + }
> +
> + ret = 0;
> +out:
> + blk_unref(blk);
> + return ret;
> +}
Looks all right, very good code.
> +
> +int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
> +{
> + BlockDriver *drv;
> +
> drv = bdrv_find_protocol(filename, true, errp);
> if (drv == NULL) {
> return -ENOENT;
> }
>
> - ret = bdrv_create(drv, filename, opts, &local_err);
> - error_propagate(errp, local_err);
> - return ret;
> + if (drv->bdrv_co_create_opts) {
> + return bdrv_create(drv, filename, opts, errp);
> + } else {
> + return bdrv_create_file_fallback(filename, drv, opts, errp);
> + }
> }
>
> /**
> @@ -1422,6 +1541,24 @@ QemuOptsList bdrv_runtime_opts = {
> },
> };
>
> +static QemuOptsList fallback_create_opts = {
> + .name = "fallback-create-opts",
> + .head = QTAILQ_HEAD_INITIALIZER(fallback_create_opts.head),
> + .desc = {
> + {
> + .name = BLOCK_OPT_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Virtual disk size"
> + },
> + {
> + .name = BLOCK_OPT_PREALLOC,
> + .type = QEMU_OPT_STRING,
> + .help = "Preallocation mode (allowed values: off)"
> + },
> + { /* end of list */ }
> + }
> +};
> +
> /*
> * Common part for opening disk images and files
> *
> @@ -5749,15 +5886,13 @@ void bdrv_img_create(const char *filename, const char *fmt,
> return;
> }
>
> - if (!proto_drv->create_opts) {
> - error_setg(errp, "Protocol driver '%s' does not support image creation",
> - proto_drv->format_name);
> - return;
> - }
> -
> /* Create parameter list */
> create_opts = qemu_opts_append(create_opts, drv->create_opts);
> - create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
> + if (proto_drv->create_opts) {
> + create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
> + } else {
> + create_opts = qemu_opts_append(create_opts, &fallback_create_opts);
> + }
>
> opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
I also tested the code a bit on the qemu virtual nvme drive.
Thanks for the patch series,
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 3/5] file-posix: Drop hdev_co_create_opts()
2020-01-22 16:45 [PATCH v2 0/5] block: Generic file creation fallback Max Reitz
2020-01-22 16:45 ` [PATCH v2 1/5] block/nbd: Fix hang in .bdrv_close() Max Reitz
2020-01-22 16:45 ` [PATCH v2 2/5] block: Generic file creation fallback Max Reitz
@ 2020-01-22 16:45 ` Max Reitz
2020-01-22 16:45 ` [PATCH v2 4/5] iscsi: Drop iscsi_co_create_opts() Max Reitz
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Max Reitz @ 2020-01-22 16:45 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Maxim Levitsky, qemu-devel, Max Reitz
The generic fallback implementation effectively does the same.
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/file-posix.c | 67 ----------------------------------------------
1 file changed, 67 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 1b805bd938..fd29372599 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -3418,67 +3418,6 @@ static coroutine_fn int hdev_co_pwrite_zeroes(BlockDriverState *bs,
return raw_do_pwrite_zeroes(bs, offset, bytes, flags, true);
}
-static int coroutine_fn hdev_co_create_opts(const char *filename, QemuOpts *opts,
- Error **errp)
-{
- int fd;
- int ret = 0;
- struct stat stat_buf;
- int64_t total_size = 0;
- bool has_prefix;
-
- /* This function is used by both protocol block drivers and therefore either
- * of these prefixes may be given.
- * The return value has to be stored somewhere, otherwise this is an error
- * due to -Werror=unused-value. */
- has_prefix =
- strstart(filename, "host_device:", &filename) ||
- strstart(filename, "host_cdrom:" , &filename);
-
- (void)has_prefix;
-
- ret = raw_normalize_devicepath(&filename, errp);
- if (ret < 0) {
- return ret;
- }
-
- /* Read out options */
- total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
- BDRV_SECTOR_SIZE);
-
- fd = qemu_open(filename, O_WRONLY | O_BINARY);
- if (fd < 0) {
- ret = -errno;
- error_setg_errno(errp, -ret, "Could not open device");
- return ret;
- }
-
- if (fstat(fd, &stat_buf) < 0) {
- ret = -errno;
- error_setg_errno(errp, -ret, "Could not stat device");
- } else if (!S_ISBLK(stat_buf.st_mode) && !S_ISCHR(stat_buf.st_mode)) {
- error_setg(errp,
- "The given file is neither a block nor a character device");
- ret = -ENODEV;
- } else if (lseek(fd, 0, SEEK_END) < total_size) {
- error_setg(errp, "Device is too small");
- ret = -ENOSPC;
- }
-
- if (!ret && total_size) {
- uint8_t buf[BDRV_SECTOR_SIZE] = { 0 };
- int64_t zero_size = MIN(BDRV_SECTOR_SIZE, total_size);
- if (lseek(fd, 0, SEEK_SET) == -1) {
- ret = -errno;
- } else {
- ret = qemu_write_full(fd, buf, zero_size);
- ret = ret == zero_size ? 0 : -errno;
- }
- }
- qemu_close(fd);
- return ret;
-}
-
static BlockDriver bdrv_host_device = {
.format_name = "host_device",
.protocol_name = "host_device",
@@ -3491,8 +3430,6 @@ static BlockDriver bdrv_host_device = {
.bdrv_reopen_prepare = raw_reopen_prepare,
.bdrv_reopen_commit = raw_reopen_commit,
.bdrv_reopen_abort = raw_reopen_abort,
- .bdrv_co_create_opts = hdev_co_create_opts,
- .create_opts = &raw_create_opts,
.mutable_opts = mutable_opts,
.bdrv_co_invalidate_cache = raw_co_invalidate_cache,
.bdrv_co_pwrite_zeroes = hdev_co_pwrite_zeroes,
@@ -3619,8 +3556,6 @@ static BlockDriver bdrv_host_cdrom = {
.bdrv_reopen_prepare = raw_reopen_prepare,
.bdrv_reopen_commit = raw_reopen_commit,
.bdrv_reopen_abort = raw_reopen_abort,
- .bdrv_co_create_opts = hdev_co_create_opts,
- .create_opts = &raw_create_opts,
.mutable_opts = mutable_opts,
.bdrv_co_invalidate_cache = raw_co_invalidate_cache,
@@ -3753,8 +3688,6 @@ static BlockDriver bdrv_host_cdrom = {
.bdrv_reopen_prepare = raw_reopen_prepare,
.bdrv_reopen_commit = raw_reopen_commit,
.bdrv_reopen_abort = raw_reopen_abort,
- .bdrv_co_create_opts = hdev_co_create_opts,
- .create_opts = &raw_create_opts,
.mutable_opts = mutable_opts,
.bdrv_co_preadv = raw_co_preadv,
--
2.24.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 4/5] iscsi: Drop iscsi_co_create_opts()
2020-01-22 16:45 [PATCH v2 0/5] block: Generic file creation fallback Max Reitz
` (2 preceding siblings ...)
2020-01-22 16:45 ` [PATCH v2 3/5] file-posix: Drop hdev_co_create_opts() Max Reitz
@ 2020-01-22 16:45 ` Max Reitz
2020-01-22 16:45 ` [PATCH v2 5/5] iotests: Add test for image creation fallback Max Reitz
2020-02-19 10:38 ` [PATCH v2 0/5] block: Generic file " Max Reitz
5 siblings, 0 replies; 13+ messages in thread
From: Max Reitz @ 2020-01-22 16:45 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Maxim Levitsky, qemu-devel, Max Reitz
The generic fallback implementation effectively does the same.
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/iscsi.c | 56 ---------------------------------------------------
1 file changed, 56 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index 2aea7e3f13..68562e108f 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2163,58 +2163,6 @@ static int coroutine_fn iscsi_co_truncate(BlockDriverState *bs, int64_t offset,
return 0;
}
-static int coroutine_fn iscsi_co_create_opts(const char *filename, QemuOpts *opts,
- Error **errp)
-{
- int ret = 0;
- int64_t total_size = 0;
- BlockDriverState *bs;
- IscsiLun *iscsilun = NULL;
- QDict *bs_options;
- Error *local_err = NULL;
-
- bs = bdrv_new();
-
- /* Read out options */
- total_size = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
- BDRV_SECTOR_SIZE);
- bs->opaque = g_new0(struct IscsiLun, 1);
- iscsilun = bs->opaque;
-
- bs_options = qdict_new();
- iscsi_parse_filename(filename, bs_options, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- ret = -EINVAL;
- } else {
- ret = iscsi_open(bs, bs_options, 0, NULL);
- }
- qobject_unref(bs_options);
-
- if (ret != 0) {
- goto out;
- }
- iscsi_detach_aio_context(bs);
- if (iscsilun->type != TYPE_DISK) {
- ret = -ENODEV;
- goto out;
- }
- if (bs->total_sectors < total_size) {
- ret = -ENOSPC;
- goto out;
- }
-
- ret = 0;
-out:
- if (iscsilun->iscsi != NULL) {
- iscsi_destroy_context(iscsilun->iscsi);
- }
- g_free(bs->opaque);
- bs->opaque = NULL;
- bdrv_unref(bs);
- return ret;
-}
-
static int iscsi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
{
IscsiLun *iscsilun = bs->opaque;
@@ -2485,8 +2433,6 @@ static BlockDriver bdrv_iscsi = {
.bdrv_parse_filename = iscsi_parse_filename,
.bdrv_file_open = iscsi_open,
.bdrv_close = iscsi_close,
- .bdrv_co_create_opts = iscsi_co_create_opts,
- .create_opts = &iscsi_create_opts,
.bdrv_reopen_prepare = iscsi_reopen_prepare,
.bdrv_reopen_commit = iscsi_reopen_commit,
.bdrv_co_invalidate_cache = iscsi_co_invalidate_cache,
@@ -2524,8 +2470,6 @@ static BlockDriver bdrv_iser = {
.bdrv_parse_filename = iscsi_parse_filename,
.bdrv_file_open = iscsi_open,
.bdrv_close = iscsi_close,
- .bdrv_co_create_opts = iscsi_co_create_opts,
- .create_opts = &iscsi_create_opts,
.bdrv_reopen_prepare = iscsi_reopen_prepare,
.bdrv_reopen_commit = iscsi_reopen_commit,
.bdrv_co_invalidate_cache = iscsi_co_invalidate_cache,
--
2.24.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 5/5] iotests: Add test for image creation fallback
2020-01-22 16:45 [PATCH v2 0/5] block: Generic file creation fallback Max Reitz
` (3 preceding siblings ...)
2020-01-22 16:45 ` [PATCH v2 4/5] iscsi: Drop iscsi_co_create_opts() Max Reitz
@ 2020-01-22 16:45 ` Max Reitz
2020-01-22 19:05 ` Eric Blake
2020-01-23 22:59 ` Maxim Levitsky
2020-02-19 10:38 ` [PATCH v2 0/5] block: Generic file " Max Reitz
5 siblings, 2 replies; 13+ messages in thread
From: Max Reitz @ 2020-01-22 16:45 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Maxim Levitsky, qemu-devel, Max Reitz
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/259 | 61 ++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/259.out | 14 +++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 76 insertions(+)
create mode 100755 tests/qemu-iotests/259
create mode 100644 tests/qemu-iotests/259.out
diff --git a/tests/qemu-iotests/259 b/tests/qemu-iotests/259
new file mode 100755
index 0000000000..22b4c10241
--- /dev/null
+++ b/tests/qemu-iotests/259
@@ -0,0 +1,61 @@
+#!/usr/bin/env bash
+#
+# Test generic image creation fallback (by using NBD)
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# 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=mreitz@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
+. ./common.rc
+. ./common.filter
+
+_supported_fmt raw
+_supported_proto nbd
+_supported_os Linux
+
+
+_make_test_img 64M
+
+echo
+echo '--- Testing creation ---'
+
+$QEMU_IMG create -f qcow2 "$TEST_IMG" 64M | _filter_img_create
+$QEMU_IMG info "$TEST_IMG" | _filter_img_info
+
+echo
+echo '--- Testing creation for which the node would need to grow ---'
+
+$QEMU_IMG create -f qcow2 -o preallocation=metadata "$TEST_IMG" 64M 2>&1 \
+ | _filter_img_create
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/259.out b/tests/qemu-iotests/259.out
new file mode 100644
index 0000000000..ffed19c2a0
--- /dev/null
+++ b/tests/qemu-iotests/259.out
@@ -0,0 +1,14 @@
+QA output created by 259
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+
+--- Testing creation ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=qcow2 size=67108864
+image: TEST_DIR/t.IMGFMT
+file format: qcow2
+virtual size: 64 MiB (67108864 bytes)
+disk size: unavailable
+
+--- Testing creation for which the node would need to grow ---
+qemu-img: TEST_DIR/t.IMGFMT: Could not resize image: Image format driver does not support resize
+Formatting 'TEST_DIR/t.IMGFMT', fmt=qcow2 size=67108864 preallocation=metadata
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 1904223020..ec47c2216a 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -273,6 +273,7 @@
256 rw auto quick
257 rw
258 rw quick
+259 rw auto quick
260 rw quick
261 rw
262 rw quick migration
--
2.24.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 5/5] iotests: Add test for image creation fallback
2020-01-22 16:45 ` [PATCH v2 5/5] iotests: Add test for image creation fallback Max Reitz
@ 2020-01-22 19:05 ` Eric Blake
2020-01-23 22:59 ` Maxim Levitsky
1 sibling, 0 replies; 13+ messages in thread
From: Eric Blake @ 2020-01-22 19:05 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel, Maxim Levitsky
On 1/22/20 10:45 AM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> +
> +_make_test_img 64M
> +
> +echo
> +echo '--- Testing creation ---'
> +
> +$QEMU_IMG create -f qcow2 "$TEST_IMG" 64M | _filter_img_create
> +$QEMU_IMG info "$TEST_IMG" | _filter_img_info
> +
> +echo
> +echo '--- Testing creation for which the node would need to grow ---'
> +
> +$QEMU_IMG create -f qcow2 -o preallocation=metadata "$TEST_IMG" 64M 2>&1 \
> + | _filter_img_create
> +
> +# success, all done
> +echo "*** done"
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/259.out b/tests/qemu-iotests/259.out
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 5/5] iotests: Add test for image creation fallback
2020-01-22 16:45 ` [PATCH v2 5/5] iotests: Add test for image creation fallback Max Reitz
2020-01-22 19:05 ` Eric Blake
@ 2020-01-23 22:59 ` Maxim Levitsky
1 sibling, 0 replies; 13+ messages in thread
From: Maxim Levitsky @ 2020-01-23 22:59 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On Wed, 2020-01-22 at 17:45 +0100, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/259 | 61 ++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/259.out | 14 +++++++++
> tests/qemu-iotests/group | 1 +
> 3 files changed, 76 insertions(+)
> create mode 100755 tests/qemu-iotests/259
> create mode 100644 tests/qemu-iotests/259.out
>
> diff --git a/tests/qemu-iotests/259 b/tests/qemu-iotests/259
> new file mode 100755
> index 0000000000..22b4c10241
> --- /dev/null
> +++ b/tests/qemu-iotests/259
> @@ -0,0 +1,61 @@
> +#!/usr/bin/env bash
> +#
> +# Test generic image creation fallback (by using NBD)
> +#
> +# Copyright (C) 2019 Red Hat, Inc.
> +#
> +# 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=mreitz@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
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt raw
> +_supported_proto nbd
> +_supported_os Linux
> +
> +
> +_make_test_img 64M
> +
> +echo
> +echo '--- Testing creation ---'
> +
> +$QEMU_IMG create -f qcow2 "$TEST_IMG" 64M | _filter_img_create
> +$QEMU_IMG info "$TEST_IMG" | _filter_img_info
> +
> +echo
> +echo '--- Testing creation for which the node would need to grow ---'
> +
> +$QEMU_IMG create -f qcow2 -o preallocation=metadata "$TEST_IMG" 64M 2>&1 \
> + | _filter_img_create
> +
> +# success, all done
> +echo "*** done"
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/259.out b/tests/qemu-iotests/259.out
> new file mode 100644
> index 0000000000..ffed19c2a0
> --- /dev/null
> +++ b/tests/qemu-iotests/259.out
> @@ -0,0 +1,14 @@
> +QA output created by 259
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +
> +--- Testing creation ---
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=qcow2 size=67108864
> +image: TEST_DIR/t.IMGFMT
> +file format: qcow2
> +virtual size: 64 MiB (67108864 bytes)
> +disk size: unavailable
> +
> +--- Testing creation for which the node would need to grow ---
> +qemu-img: TEST_DIR/t.IMGFMT: Could not resize image: Image format driver does not support resize
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=qcow2 size=67108864 preallocation=metadata
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 1904223020..ec47c2216a 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -273,6 +273,7 @@
> 256 rw auto quick
> 257 rw
> 258 rw quick
> +259 rw auto quick
> 260 rw quick
> 261 rw
> 262 rw quick migration
Very minor nitpick: maybe add a note that nbd doesn't support resize / truncate.
It might start supporting it one day.
I didn't notice it first.
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/5] block: Generic file creation fallback
2020-01-22 16:45 [PATCH v2 0/5] block: Generic file creation fallback Max Reitz
` (4 preceding siblings ...)
2020-01-22 16:45 ` [PATCH v2 5/5] iotests: Add test for image creation fallback Max Reitz
@ 2020-02-19 10:38 ` Max Reitz
2020-02-19 10:47 ` Maxim Levitsky
5 siblings, 1 reply; 13+ messages in thread
From: Max Reitz @ 2020-02-19 10:38 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Maxim Levitsky
[-- Attachment #1.1: Type: text/plain, Size: 1177 bytes --]
On 22.01.20 17:45, Max Reitz wrote:
> Hi,
>
> As version 1, this series adds a fallback path for creating files (on
> the protocol layer) if the protocol driver does not support file
> creation, but the file already exists.
>
>
> Branch: https://github.com/XanClic/qemu.git skip-proto-create-v2
> Branch: https://git.xanclic.moe/XanClic/qemu.git skip-proto-create-v2
>
>
> v2:
> - Drop blk_truncate_for_formatting(): It doesn’t make sense to introduce
> this function any more after 26536c7fc25917d1bd13781f81fe3ab039643bff
> (“block: Do not truncate file node when formatting”), because we’d
> only use it in bdrv_create_file_fallback().
> Thus, it makes more sense to create special helper functions
> specifically for bdrv_create_file_fallback().
>
> - Thus, dropped patches 2 and 3.
>
> - And changed patch 4 to include those helper functions.
>
> - Rebased, which was a bit of a pain.
Thanks for the reviews, added a note to the new test why the second case
is expected to fail (as requested by Maxim), and applied the series to
my block branch:
https://git.xanclic.moe/XanClic/qemu/commits/branch/block
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 0/5] block: Generic file creation fallback
2020-02-19 10:38 ` [PATCH v2 0/5] block: Generic file " Max Reitz
@ 2020-02-19 10:47 ` Maxim Levitsky
0 siblings, 0 replies; 13+ messages in thread
From: Maxim Levitsky @ 2020-02-19 10:47 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On Wed, 2020-02-19 at 11:38 +0100, Max Reitz wrote:
> On 22.01.20 17:45, Max Reitz wrote:
> > Hi,
> >
> > As version 1, this series adds a fallback path for creating files (on
> > the protocol layer) if the protocol driver does not support file
> > creation, but the file already exists.
> >
> >
> > Branch: https://github.com/XanClic/qemu.git skip-proto-create-v2
> > Branch: https://git.xanclic.moe/XanClic/qemu.git skip-proto-create-v2
> >
> >
> > v2:
> > - Drop blk_truncate_for_formatting(): It doesn’t make sense to introduce
> > this function any more after 26536c7fc25917d1bd13781f81fe3ab039643bff
> > (“block: Do not truncate file node when formatting”), because we’d
> > only use it in bdrv_create_file_fallback().
> > Thus, it makes more sense to create special helper functions
> > specifically for bdrv_create_file_fallback().
> >
> > - Thus, dropped patches 2 and 3.
> >
> > - And changed patch 4 to include those helper functions.
> >
> > - Rebased, which was a bit of a pain.
>
> Thanks for the reviews, added a note to the new test why the second case
> is expected to fail (as requested by Maxim), and applied the series to
> my block branch:
>
> https://git.xanclic.moe/XanClic/qemu/commits/branch/block
>
> Max
>
Thank you too!
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 13+ messages in thread