[Qemu-devel] [PATCH 0/7] block: Generic file creation fallback

[Qemu-devel] [PATCH 0/7] block: Generic file creation fallback

[Max Reitz]

Hi,

Kevin commented on my RFC, so I got what an RFC wants, and he didn’t
object to the creation fallback part.  So I suppose I can go down that
route at least.  (Which was actually the more important part of the
series.)

So as in the RFC, 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.


v1 (as compared to the RFC):
- Changed patch 1 to match Kevin’s proposal
- Added patches 2 and 3 to replace the truncation fallback
- Changed patch 4 accordingly
- Added patches 5 and 6
- Truncated patch 7 (haha!), because there no longer is a truncation
  fallback, so we cannot test it.

I would like to note that the diff stat without patches 1 and 7 is
141+, 139-.  So this series is basically a net zero for our code size,
but it prevents future series that might add such pseudo-creation
support to other drivers but file-posix (for host devices) and iscsi
(e.g. I’m looking at you, nvme).


git-backport-diff against the RFC:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/7:[0016] [FC] 'block/nbd: Fix hang in .bdrv_close()'
002/7:[down] 'block: Add blk_truncate_for_formatting()'
003/7:[down] 'block: Use blk_truncate_for_formatting()'
004/7:[0012] [FC] 'block: Generic file creation fallback'
005/7:[down] 'file-posix: Drop hdev_co_create_opts()'
006/7:[down] 'iscsi: Drop iscsi_co_create_opts()'
007/7:[down] 'iotests: Add test for image creation fallback'


Max Reitz (7):
  block/nbd: Fix hang in .bdrv_close()
  block: Add blk_truncate_for_formatting()
  block: Use blk_truncate_for_formatting()
  block: Generic file creation fallback
  file-posix: Drop hdev_co_create_opts()
  iscsi: Drop iscsi_co_create_opts()
  iotests: Add test for image creation fallback

 include/sysemu/block-backend.h | 12 +++++
 block.c                        | 83 +++++++++++++++++++++++++++++-----
 block/block-backend.c          | 54 ++++++++++++++++++++++
 block/file-posix.c             | 67 ---------------------------
 block/iscsi.c                  | 56 -----------------------
 block/nbd.c                    | 14 +++++-
 block/parallels.c              |  2 +-
 block/qcow.c                   |  2 +-
 block/qcow2.c                  |  2 +-
 block/qed.c                    |  2 +-
 tests/qemu-iotests/259         | 61 +++++++++++++++++++++++++
 tests/qemu-iotests/259.out     | 14 ++++++
 tests/qemu-iotests/group       |  1 +
 13 files changed, 230 insertions(+), 140 deletions(-)
 create mode 100755 tests/qemu-iotests/259
 create mode 100644 tests/qemu-iotests/259.out

-- 
2.21.0

[Qemu-devel] [PATCH 1/7] block/nbd: Fix hang in .bdrv_close()

[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 81edabbf35..8f5ee86842 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -61,6 +61,7 @@ typedef struct BDRVNBDState {
     CoMutex send_mutex;
     CoQueue free_sema;
     Coroutine *connection_co;
+    Coroutine *teardown_co;
     int in_flight;
 
     NBDClientRequest requests[MAX_NBD_REQUESTS];
@@ -135,7 +136,15 @@ static void nbd_teardown_connection(BlockDriverState *bs)
     qio_channel_shutdown(s->ioc,
                          QIO_CHANNEL_SHUTDOWN_BOTH,
                          NULL);
-    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);
 
     nbd_client_detach_aio_context(bs);
     object_unref(OBJECT(s->sioc));
@@ -207,6 +216,9 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
     bdrv_dec_in_flight(s->bs);
 
     s->connection_co = NULL;
+    if (s->teardown_co) {
+        aio_co_wake(s->teardown_co);
+    }
     aio_wait_kick();
 }
 
-- 
2.21.0

[Qemu-devel] [PATCH 2/7] block: Add blk_truncate_for_formatting()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/sysemu/block-backend.h | 12 ++++++++
 block/block-backend.c          | 54 ++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 733c4957eb..cd9ec8bf52 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -236,6 +236,18 @@ int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
                           int bytes);
 int blk_truncate(BlockBackend *blk, int64_t offset, PreallocMode prealloc,
                  Error **errp);
+
+/**
+ * Wrapper of blk_truncate() for format drivers that need to truncate
+ * their protocol node before formatting it.
+ * Invoke blk_truncate() to truncate the file to @offset; if that
+ * fails with -ENOTSUP (and the file is already big enough), try to
+ * overwrite the first sector with zeroes.  If that succeeds, return
+ * success.
+ */
+int blk_truncate_for_formatting(BlockBackend *blk, int64_t offset,
+                                Error **errp);
+
 int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes);
 int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
                      int64_t pos, int size);
diff --git a/block/block-backend.c b/block/block-backend.c
index a8d160fd5d..c0e64b1ee1 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2041,6 +2041,60 @@ int blk_truncate(BlockBackend *blk, int64_t offset, PreallocMode prealloc,
     return bdrv_truncate(blk->root, offset, prealloc, errp);
 }
 
+int blk_truncate_for_formatting(BlockBackend *blk, int64_t offset, Error **errp)
+{
+    Error *local_err = NULL;
+    int64_t current_size;
+    int bytes_to_clear;
+    int ret;
+
+    ret = blk_truncate(blk, offset, PREALLOC_MODE_OFF, &local_err);
+    if (ret < 0 && ret != -ENOTSUP) {
+        error_propagate(errp, local_err);
+        return ret;
+    } else if (ret >= 0) {
+        return ret;
+    }
+
+    current_size = blk_getlength(blk);
+    if (current_size < 0) {
+        error_free(local_err);
+        error_setg_errno(errp, -current_size,
+                         "Failed to inquire new image file's current length");
+        return current_size;
+    }
+
+    if (current_size < offset) {
+        /* Need to grow the image, but we failed to do that */
+        error_propagate(errp, local_err);
+        return -ENOTSUP;
+    }
+
+    error_free(local_err);
+    /*
+     * We can deal with images that are too big.  We just need to
+     * clear the first sector.
+     */
+
+    bytes_to_clear = MIN(current_size, BDRV_SECTOR_SIZE) - offset;
+    if (bytes_to_clear) {
+        if (!(blk->root->perm & BLK_PERM_WRITE)) {
+            error_setg(errp, "Cannot clear first sector of new image: "
+                       "Write permission missing");
+            return -EPERM;
+        }
+
+        ret = blk_pwrite_zeroes(blk, offset, bytes_to_clear, 0);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Failed to clear the first sector of "
+                             "the new image");
+            return ret;
+        }
+    }
+
+    return 0;
+}
+
 static void blk_pdiscard_entry(void *opaque)
 {
     BlkRwCo *rwco = opaque;
-- 
2.21.0

[Qemu-devel] [PATCH 3/7] block: Use blk_truncate_for_formatting()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/parallels.c | 2 +-
 block/qcow.c      | 2 +-
 block/qcow2.c     | 2 +-
 block/qed.c       | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 00fae125d1..a17b2d92f2 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -563,7 +563,7 @@ static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
     blk_set_allow_write_beyond_eof(blk, true);
 
     /* Create image format */
-    ret = blk_truncate(blk, 0, PREALLOC_MODE_OFF, errp);
+    ret = blk_truncate_for_formatting(blk, 0, errp);
     if (ret < 0) {
         goto out;
     }
diff --git a/block/qcow.c b/block/qcow.c
index 5bdf72ba33..86034135f9 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -858,7 +858,7 @@ static int coroutine_fn qcow_co_create(BlockdevCreateOptions *opts,
     blk_set_allow_write_beyond_eof(qcow_blk, true);
 
     /* Create image format */
-    ret = blk_truncate(qcow_blk, 0, PREALLOC_MODE_OFF, errp);
+    ret = blk_truncate_for_formatting(qcow_blk, 0, errp);
     if (ret < 0) {
         goto exit;
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index 039bdc2f7e..f3e53c781d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3184,7 +3184,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
     blk_set_allow_write_beyond_eof(blk, true);
 
     /* Clear the protocol layer and preallocate it if necessary */
-    ret = blk_truncate(blk, 0, PREALLOC_MODE_OFF, errp);
+    ret = blk_truncate_for_formatting(blk, 0, errp);
     if (ret < 0) {
         goto out;
     }
diff --git a/block/qed.c b/block/qed.c
index 77c7cef175..ec244158b5 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -673,7 +673,7 @@ static int coroutine_fn bdrv_qed_co_create(BlockdevCreateOptions *opts,
     l1_size = header.cluster_size * header.table_size;
 
     /* File must start empty and grow, check truncate is supported */
-    ret = blk_truncate(blk, 0, PREALLOC_MODE_OFF, errp);
+    ret = blk_truncate_for_formatting(blk, 0, errp);
     if (ret < 0) {
         goto out;
     }
-- 
2.21.0

[Qemu-devel] [PATCH 4/7] block: Generic file creation fallback

[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 | 83 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 71 insertions(+), 12 deletions(-)

diff --git a/block.c b/block.c
index c139540f2b..5466585501 100644
--- a/block.c
+++ b/block.c
@@ -531,20 +531,63 @@ out:
     return ret;
 }
 
-int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
+static int bdrv_create_file_fallback(const char *filename, BlockDriver *drv,
+                                     QemuOpts *opts, Error **errp)
 {
-    BlockDriver *drv;
+    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;
+    }
+
+    ret = blk_truncate_for_formatting(blk, size, errp);
+    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);
+    }
 }
 
 /**
@@ -1420,6 +1463,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
  *
@@ -5681,14 +5742,12 @@ 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_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);
+    }
 
     /* Create parameter list with default values */
     opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
-- 
2.21.0

[Qemu-devel] [PATCH 5/7] file-posix: Drop hdev_co_create_opts()

[Max Reitz]

The generic fallback implementation effectively does the same.

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 4479cc7ab4..65bd6d3333 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -3325,67 +3325,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",
@@ -3398,8 +3337,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,
@@ -3525,8 +3462,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,
 
@@ -3659,8 +3594,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.21.0

[Qemu-devel] [PATCH 6/7] iscsi: Drop iscsi_co_create_opts()

[Max Reitz]

The generic fallback implementation effectively does the same.

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 267f160bf6..0e5729d335 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2157,58 +2157,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;
@@ -2479,8 +2427,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,
@@ -2518,8 +2464,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.21.0

[Qemu-devel] [PATCH 7/7] iotests: Add test for image creation fallback

[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 b34c8e3c0c..80e7603174 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -269,3 +269,4 @@
 254 rw auto backing quick
 255 rw auto quick
 256 rw auto quick
+259 rw auto quick
-- 
2.21.0

Re: [Qemu-devel] [PATCH 7/7] iotests: Add test for image creation fallback

[Thomas Huth]

On 12/07/2019 19.36, 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

Why is this stating "raw" here...

> +_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

... and using qcow2 here instead?

> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index b34c8e3c0c..80e7603174 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -269,3 +269,4 @@
>  254 rw auto backing quick
>  255 rw auto quick
>  256 rw auto quick
> +259 rw auto quick

If this test only supports "raw", I think it should not be in the "auto"
group anymore.

 Thomas

Re: [Qemu-devel] [PATCH 7/7] iotests: Add test for image creation fallback

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3281 bytes --]

On 15.07.19 11:31, Thomas Huth wrote:
> On 12/07/2019 19.36, 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
> 
> Why is this stating "raw" here...
> 
>> +_supported_proto nbd

Because it’s an NBD test.

>> +_supported_os Linux
>> +
>> +
>> +_make_test_img 64M

Also, because I don‘t want this to create a qcow2 image.  This should
just set up a raw NBD node.

>> +echo
>> +echo '--- Testing creation ---'
>> +
>> +$QEMU_IMG create -f qcow2 "$TEST_IMG" 64M | _filter_img_create
> 
> ... and using qcow2 here instead?

Practical answer: Nobody tests qcow2+nbd.  Ever.  Because it is
generally a stupid combination.  We need it for this test, though,
because NBD is the simplest way to get a fixed-size block device.

The more involved answer is because nobody has introduced anything yet
to simply let the test decide on which format/protocol combination to
use by default (and then let the check script just use that, unless
overridden by the user).

But there‘s also the _make_test_img thing.

>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
>> index b34c8e3c0c..80e7603174 100644
>> --- a/tests/qemu-iotests/group
>> +++ b/tests/qemu-iotests/group
>> @@ -269,3 +269,4 @@
>>  254 rw auto backing quick
>>  255 rw auto quick
>>  256 rw auto quick
>> +259 rw auto quick
> 
> If this test only supports "raw", I think it should not be in the "auto"
> group anymore.

Oh, I didn‘t know that only runs qcow2 tests.  OK then.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] iscsi: Drop iscsi_co_create_opts()

[Maxim Levitsky]

On Fri, 2019-07-12 at 19:35 +0200, Max Reitz wrote:
> The generic fallback implementation effectively does the same.
> 
> 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 267f160bf6..0e5729d335 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -2157,58 +2157,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;
> @@ -2479,8 +2427,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,
> @@ -2518,8 +2464,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,


Well, in theory the original code did not zero the first sector, like what the generic code will do now,
but this is OK due to the same reasons the original zeroing code was added.


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [Qemu-block] [PATCH 1/7] block/nbd: Fix hang in .bdrv_close()

[Maxim Levitsky]

On Fri, 2019-07-12 at 19:35 +0200, 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.

After diving into NBD's co-routines (this is 2nd time I do this) and speaking about this
with Max Reitz on IRC, could I suggest to extend the explanation a bit, something like that:


When nbd_close() is called from a coroutine, the connection_co never
gets to run, and thus nbd_teardown_connection() hangs.

This happens because the connection_co is woken up by nbd_teardown_connection() by closing the socket, 
which wakes up the IO channel on which the connection_co is blocked.

However connection_co is waken up by aio_co_wake, which has an assumption that if the caller is already in
a coroutine, the caller doesn't switch immediately to the woken coroutine, but rather it adds the coroutine to list
of coroutines to wake immediately after the current co-routine yields/terminates (self->co_queue_wakeup)
Since we instead do aio_poll, that never happens.


The patch itself looks fine, so
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 81edabbf35..8f5ee86842 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -61,6 +61,7 @@ typedef struct BDRVNBDState {
>      CoMutex send_mutex;
>      CoQueue free_sema;
>      Coroutine *connection_co;
> +    Coroutine *teardown_co;
>      int in_flight;
>  
>      NBDClientRequest requests[MAX_NBD_REQUESTS];
> @@ -135,7 +136,15 @@ static void nbd_teardown_connection(BlockDriverState *bs)
>      qio_channel_shutdown(s->ioc,
>                           QIO_CHANNEL_SHUTDOWN_BOTH,
>                           NULL);
> -    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);
>  
>      nbd_client_detach_aio_context(bs);
>      object_unref(OBJECT(s->sioc));
> @@ -207,6 +216,9 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
>      bdrv_dec_in_flight(s->bs);
>  
>      s->connection_co = NULL;
> +    if (s->teardown_co) {
> +        aio_co_wake(s->teardown_co);
> +    }
>      aio_wait_kick();
>  }
>  

Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] block: Add blk_truncate_for_formatting()

[Maxim Levitsky]

On Fri, 2019-07-12 at 19:35 +0200, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/sysemu/block-backend.h | 12 ++++++++
>  block/block-backend.c          | 54 ++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index 733c4957eb..cd9ec8bf52 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -236,6 +236,18 @@ int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
>                            int bytes);
>  int blk_truncate(BlockBackend *blk, int64_t offset, PreallocMode prealloc,
>                   Error **errp);
> +
> +/**
> + * Wrapper of blk_truncate() for format drivers that need to truncate
> + * their protocol node before formatting it.
> + * Invoke blk_truncate() to truncate the file to @offset; if that
> + * fails with -ENOTSUP (and the file is already big enough), try to
> + * overwrite the first sector with zeroes.  If that succeeds, return
> + * success.
> + */
> +int blk_truncate_for_formatting(BlockBackend *blk, int64_t offset,
> +                                Error **errp);
> +
>  int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes);
>  int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
>                       int64_t pos, int size);
> diff --git a/block/block-backend.c b/block/block-backend.c
> index a8d160fd5d..c0e64b1ee1 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -2041,6 +2041,60 @@ int blk_truncate(BlockBackend *blk, int64_t offset, PreallocMode prealloc,
>      return bdrv_truncate(blk->root, offset, prealloc, errp);
>  }
>  
> +int blk_truncate_for_formatting(BlockBackend *blk, int64_t offset, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    int64_t current_size;
> +    int bytes_to_clear;
> +    int ret;
> +
> +    ret = blk_truncate(blk, offset, PREALLOC_MODE_OFF, &local_err);
> +    if (ret < 0 && ret != -ENOTSUP) {
> +        error_propagate(errp, local_err);
> +        return ret;
> +    } else if (ret >= 0) {
> +        return ret;
> +    }

What if the truncate does succeed? For example the current implementation of raw_co_truncate,
does return zero when you truncate to less that block device size 
(and this is kind of wrong since you can't really change the block device size)

Even more, I see is that in the later patch, you call this with offset == 0 which
I think will always succeed on a raw block device, thus skipping the zeroing code.

How about just doing the zeroing in the bdrv_create_file_fallback?


Another idea:

blk_truncate_for_formatting would first truncate the file to 0, then
check if the size of the file became zero in addition to the successful return value.

If the file size became zero, truncate the file to the requested size - this should make sure that file is empty.
Otherwise, zero the first sector.

It might also be nice to add a check that if the size didn't became zero, that it remained the same
to avoid strange situations of semi broken truncate.


Also I would rename the function to something like blk_raw_format_file,
basically a function which tries its best to erase an existing file contents


Yet another idea would to drop the lying in the raw_co_truncate (on block devices), and fail always,
unless asked to truncate to the exact file size, and let the callers deal with that.
Callers where it is not critical for the truncate to work can just ignore this failure.
That is probably hard to implement 

Or we can add a truncate 'mode' to .bdrv_co_truncate, which would let the caller indicate its intention,
that is if the caller must truncate to that size or it can accept truncate ending up in bigger file that it asked for. 

As we once discussed on IRC, the fact that truncate on a block device 'succeeds',
despite not really beeing able to change the block device size, causes other issues,
like not beeing able to use preallocation=full when creating a qcow2 image on a block device.

Best regards,
	Maxim Levitsky

> +
> +    current_size = blk_getlength(blk);
> +    if (current_size < 0) {
> +        error_free(local_err);
> +        error_setg_errno(errp, -current_size,
> +                         "Failed to inquire new image file's current length");
> +        return current_size;
> +    }
> +
> +    if (current_size < offset) {
> +        /* Need to grow the image, but we failed to do that */
> +        error_propagate(errp, local_err);
> +        return -ENOTSUP;
> +    }
> +
> +    error_free(local_err);
> +    /*
> +     * We can deal with images that are too big.  We just need to
> +     * clear the first sector.
> +     */
> +
> +    bytes_to_clear = MIN(current_size, BDRV_SECTOR_SIZE) - offset;
> +    if (bytes_to_clear) {
> +        if (!(blk->root->perm & BLK_PERM_WRITE)) {
> +            error_setg(errp, "Cannot clear first sector of new image: "
> +                       "Write permission missing");
> +            return -EPERM;
> +        }
> +
> +        ret = blk_pwrite_zeroes(blk, offset, bytes_to_clear, 0);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "Failed to clear the first sector of "
> +                             "the new image");
> +            return ret;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static void blk_pdiscard_entry(void *opaque)
>  {
>      BlkRwCo *rwco = opaque;

Re: [Qemu-devel] [Qemu-block] [PATCH 3/7] block: Use blk_truncate_for_formatting()

[Maxim Levitsky]

On Fri, 2019-07-12 at 19:35 +0200, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/parallels.c | 2 +-
>  block/qcow.c      | 2 +-
>  block/qcow2.c     | 2 +-
>  block/qed.c       | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 00fae125d1..a17b2d92f2 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -563,7 +563,7 @@ static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
>      blk_set_allow_write_beyond_eof(blk, true);
>  
>      /* Create image format */
> -    ret = blk_truncate(blk, 0, PREALLOC_MODE_OFF, errp);
> +    ret = blk_truncate_for_formatting(blk, 0, errp);
>      if (ret < 0) {
>          goto out;
>      }
> diff --git a/block/qcow.c b/block/qcow.c
> index 5bdf72ba33..86034135f9 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -858,7 +858,7 @@ static int coroutine_fn qcow_co_create(BlockdevCreateOptions *opts,
>      blk_set_allow_write_beyond_eof(qcow_blk, true);
>  
>      /* Create image format */
> -    ret = blk_truncate(qcow_blk, 0, PREALLOC_MODE_OFF, errp);
> +    ret = blk_truncate_for_formatting(qcow_blk, 0, errp);
>      if (ret < 0) {
>          goto exit;
>      }
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 039bdc2f7e..f3e53c781d 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3184,7 +3184,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
>      blk_set_allow_write_beyond_eof(blk, true);
>  
>      /* Clear the protocol layer and preallocate it if necessary */
> -    ret = blk_truncate(blk, 0, PREALLOC_MODE_OFF, errp);
> +    ret = blk_truncate_for_formatting(blk, 0, errp);
>      if (ret < 0) {
>          goto out;
>      }
> diff --git a/block/qed.c b/block/qed.c
> index 77c7cef175..ec244158b5 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -673,7 +673,7 @@ static int coroutine_fn bdrv_qed_co_create(BlockdevCreateOptions *opts,
>      l1_size = header.cluster_size * header.table_size;
>  
>      /* File must start empty and grow, check truncate is supported */
> -    ret = blk_truncate(blk, 0, PREALLOC_MODE_OFF, errp);
> +    ret = blk_truncate_for_formatting(blk, 0, errp);
>      if (ret < 0) {
>          goto out;
>      }


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [Qemu-block] [PATCH 4/7] block: Generic file creation fallback

[Maxim Levitsky]

On Fri, 2019-07-12 at 19:35 +0200, 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 | 83 ++++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 71 insertions(+), 12 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c139540f2b..5466585501 100644
> --- a/block.c
> +++ b/block.c
> @@ -531,20 +531,63 @@ out:
>      return ret;
>  }
>  
> -int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
> +static int bdrv_create_file_fallback(const char *filename, BlockDriver *drv,
> +                                     QemuOpts *opts, Error **errp)
>  {
> -    BlockDriver *drv;
> +    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;
> +    }
> +
> +    ret = blk_truncate_for_formatting(blk, size, errp);
> +    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);
> +    }
>  }
>  
>  /**
> @@ -1420,6 +1463,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
>   *
> @@ -5681,14 +5742,12 @@ 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_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);
> +    }
>  
>      /* Create parameter list with default values */
>      opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);

Looks good!


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [Qemu-block] [PATCH 5/7] file-posix: Drop hdev_co_create_opts()

[Maxim Levitsky]

On Fri, 2019-07-12 at 19:35 +0200, Max Reitz wrote:
> The generic fallback implementation effectively does the same.
> 
> 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 4479cc7ab4..65bd6d3333 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -3325,67 +3325,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",
> @@ -3398,8 +3337,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,
> @@ -3525,8 +3462,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,
>  
> @@ -3659,8 +3594,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,


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [PATCH 7/7] iotests: Add test for image creation fallback

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1315 bytes --]

On 7/15/19 4:48 AM, Max Reitz wrote:

>>> +
>>> +_supported_fmt raw
>>
>> Why is this stating "raw" here...
>>
>>> +_supported_proto nbd
> 
> Because it’s an NBD test.
> 
>>> +_supported_os Linux
>>> +
>>> +
>>> +_make_test_img 64M
> 
> Also, because I don‘t want this to create a qcow2 image.  This should
> just set up a raw NBD node.
> 
>>> +echo
>>> +echo '--- Testing creation ---'
>>> +
>>> +$QEMU_IMG create -f qcow2 "$TEST_IMG" 64M | _filter_img_create
>>
>> ... and using qcow2 here instead?
> 
> Practical answer: Nobody tests qcow2+nbd.  Ever.  Because it is
> generally a stupid combination.  We need it for this test, though,
> because NBD is the simplest way to get a fixed-size block device.

There are definitely some broken things if you try qcow2+nbd. However, I
someday hope to implement a proposed NBD_CMD_RESIZE extension to the
protocol, at which point, it will be a lot easier to run qcow2+nbd
(where qcow2 can then advantage of automatic resizes of the protocol
layer, the same as it does for regular files), so part of that effort
may be figuring out how to make iotests cleanly support qcow2+nbd in
more situations.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] block: Add blk_truncate_for_formatting()

[Maxim Levitsky]

On Tue, 2019-07-16 at 16:08 +0300, Maxim Levitsky wrote:
> On Fri, 2019-07-12 at 19:35 +0200, Max Reitz wrote:
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  include/sysemu/block-backend.h | 12 ++++++++
> >  block/block-backend.c          | 54 ++++++++++++++++++++++++++++++++++
> >  2 files changed, 66 insertions(+)
> > 
> > diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> > index 733c4957eb..cd9ec8bf52 100644
> > --- a/include/sysemu/block-backend.h
> > +++ b/include/sysemu/block-backend.h
> > @@ -236,6 +236,18 @@ int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
> >                            int bytes);
> >  int blk_truncate(BlockBackend *blk, int64_t offset, PreallocMode prealloc,
> >                   Error **errp);
> > +
> > +/**
> > + * Wrapper of blk_truncate() for format drivers that need to truncate
> > + * their protocol node before formatting it.
> > + * Invoke blk_truncate() to truncate the file to @offset; if that
> > + * fails with -ENOTSUP (and the file is already big enough), try to
> > + * overwrite the first sector with zeroes.  If that succeeds, return
> > + * success.
> > + */
> > +int blk_truncate_for_formatting(BlockBackend *blk, int64_t offset,
> > +                                Error **errp);
> > +
> >  int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes);
> >  int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
> >                       int64_t pos, int size);
> > diff --git a/block/block-backend.c b/block/block-backend.c
> > index a8d160fd5d..c0e64b1ee1 100644
> > --- a/block/block-backend.c
> > +++ b/block/block-backend.c
> > @@ -2041,6 +2041,60 @@ int blk_truncate(BlockBackend *blk, int64_t offset, PreallocMode prealloc,
> >      return bdrv_truncate(blk->root, offset, prealloc, errp);
> >  }
> >  
> > +int blk_truncate_for_formatting(BlockBackend *blk, int64_t offset, Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +    int64_t current_size;
> > +    int bytes_to_clear;
> > +    int ret;
> > +
> > +    ret = blk_truncate(blk, offset, PREALLOC_MODE_OFF, &local_err);
> > +    if (ret < 0 && ret != -ENOTSUP) {
> > +        error_propagate(errp, local_err);
> > +        return ret;
> > +    } else if (ret >= 0) {
> > +        return ret;
> > +    }
> 
> What if the truncate does succeed? For example the current implementation of raw_co_truncate,
> does return zero when you truncate to less that block device size 
> (and this is kind of wrong since you can't really change the block device size)
> 
> Even more, I see is that in the later patch, you call this with offset == 0 which
> I think will always succeed on a raw block device, thus skipping the zeroing code.
> 
> How about just doing the zeroing in the bdrv_create_file_fallback?
> 
> 
> Another idea:
> 
> blk_truncate_for_formatting would first truncate the file to 0, then
> check if the size of the file became zero in addition to the successful return value.
> 
> If the file size became zero, truncate the file to the requested size - this should make sure that file is empty.
> Otherwise, zero the first sector.
> 
> It might also be nice to add a check that if the size didn't became zero, that it remained the same
> to avoid strange situations of semi broken truncate.
> 
> 
> Also I would rename the function to something like blk_raw_format_file,
> basically a function which tries its best to erase an existing file contents
> 
> 
> Yet another idea would to drop the lying in the raw_co_truncate (on block devices), and fail always,
> unless asked to truncate to the exact file size, and let the callers deal with that.
> Callers where it is not critical for the truncate to work can just ignore this failure.
> That is probably hard to implement 
> 
> Or we can add a truncate 'mode' to .bdrv_co_truncate, which would let the caller indicate its intention,
> that is if the caller must truncate to that size or it can accept truncate ending up in bigger file that it asked for. 
> 
> As we once discussed on IRC, the fact that truncate on a block device 'succeeds',
> despite not really beeing able to change the block device size, causes other issues,
> like not beeing able to use preallocation=full when creating a qcow2 image on a block device.
> 
> Best regards,
> 	Maxim Levitsky
> 
> > +
> > +    current_size = blk_getlength(blk);
> > +    if (current_size < 0) {
> > +        error_free(local_err);
> > +        error_setg_errno(errp, -current_size,
> > +                         "Failed to inquire new image file's current length");
> > +        return current_size;
> > +    }
> > +
> > +    if (current_size < offset) {
> > +        /* Need to grow the image, but we failed to do that */
> > +        error_propagate(errp, local_err);
> > +        return -ENOTSUP;
> > +    }
> > +
> > +    error_free(local_err);
> > +    /*
> > +     * We can deal with images that are too big.  We just need to
> > +     * clear the first sector.
> > +     */
> > +
> > +    bytes_to_clear = MIN(current_size, BDRV_SECTOR_SIZE) - offset;
Also this I think is wrong when offset !=0, since assuming real world device, the
MIN will be just BDRV_SECTOR_SIZE, so the result of this statement is negative number.

I think you want just
bytes_to_clear = MIN(current_size, BDRV_SECTOR_SIZE);


Best regards,
	Maxim Levitsky



> > +    if (bytes_to_clear) {
> > +        if (!(blk->root->perm & BLK_PERM_WRITE)) {
> > +            error_setg(errp, "Cannot clear first sector of new image: "
> > +                       "Write permission missing");
> > +            return -EPERM;
> > +        }
> > +
> > +        ret = blk_pwrite_zeroes(blk, offset, bytes_to_clear, 0);
> > +        if (ret < 0) {
> > +            error_setg_errno(errp, -ret, "Failed to clear the first sector of "
> > +                             "the new image");
> > +            return ret;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static void blk_pdiscard_entry(void *opaque)
> >  {
> >      BlkRwCo *rwco = opaque;
> 
> 

Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] block: Add blk_truncate_for_formatting()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 7407 bytes --]

On 16.07.19 17:45, Maxim Levitsky wrote:
> On Tue, 2019-07-16 at 16:08 +0300, Maxim Levitsky wrote:
>> On Fri, 2019-07-12 at 19:35 +0200, Max Reitz wrote:
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>  include/sysemu/block-backend.h | 12 ++++++++
>>>  block/block-backend.c          | 54 ++++++++++++++++++++++++++++++++++
>>>  2 files changed, 66 insertions(+)
>>>
>>> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
>>> index 733c4957eb..cd9ec8bf52 100644
>>> --- a/include/sysemu/block-backend.h
>>> +++ b/include/sysemu/block-backend.h
>>> @@ -236,6 +236,18 @@ int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
>>>                            int bytes);
>>>  int blk_truncate(BlockBackend *blk, int64_t offset, PreallocMode prealloc,
>>>                   Error **errp);
>>> +
>>> +/**
>>> + * Wrapper of blk_truncate() for format drivers that need to truncate
>>> + * their protocol node before formatting it.
>>> + * Invoke blk_truncate() to truncate the file to @offset; if that
>>> + * fails with -ENOTSUP (and the file is already big enough), try to
>>> + * overwrite the first sector with zeroes.  If that succeeds, return
>>> + * success.
>>> + */
>>> +int blk_truncate_for_formatting(BlockBackend *blk, int64_t offset,
>>> +                                Error **errp);
>>> +
>>>  int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes);
>>>  int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
>>>                       int64_t pos, int size);
>>> diff --git a/block/block-backend.c b/block/block-backend.c
>>> index a8d160fd5d..c0e64b1ee1 100644
>>> --- a/block/block-backend.c
>>> +++ b/block/block-backend.c
>>> @@ -2041,6 +2041,60 @@ int blk_truncate(BlockBackend *blk, int64_t offset, PreallocMode prealloc,
>>>      return bdrv_truncate(blk->root, offset, prealloc, errp);
>>>  }
>>>  
>>> +int blk_truncate_for_formatting(BlockBackend *blk, int64_t offset, Error **errp)
>>> +{
>>> +    Error *local_err = NULL;
>>> +    int64_t current_size;
>>> +    int bytes_to_clear;
>>> +    int ret;
>>> +
>>> +    ret = blk_truncate(blk, offset, PREALLOC_MODE_OFF, &local_err);
>>> +    if (ret < 0 && ret != -ENOTSUP) {
>>> +        error_propagate(errp, local_err);
>>> +        return ret;
>>> +    } else if (ret >= 0) {
>>> +        return ret;
>>> +    }
>>
>> What if the truncate does succeed? For example the current implementation of raw_co_truncate,
>> does return zero when you truncate to less that block device size 
>> (and this is kind of wrong since you can't really change the block device size)

Ah, yes, stupid me.

>> Even more, I see is that in the later patch, you call this with offset == 0 which
>> I think will always succeed on a raw block device, thus skipping the zeroing code.
>>
>> How about just doing the zeroing in the bdrv_create_file_fallback?

Hm.  I can try.  The block drivers that use
blk_truncate_for_formatting() write a full header to the image file, so
they don’t need the sector be zero.

Alternatively, I could just treat ret == 0 the same as -ENOTSUP.  Then
the code would just go on to invoke blk_getlength() and see for itself
what to do.

>> Another idea:
>>
>> blk_truncate_for_formatting would first truncate the file to 0, then
>> check if the size of the file became zero in addition to the successful return value.
>>
>> If the file size became zero, truncate the file to the requested size - this should make sure that file is empty.
>> Otherwise, zero the first sector.
>>
>> It might also be nice to add a check that if the size didn't became zero, that it remained the same
>> to avoid strange situations of semi broken truncate.

Hm, I would expect the block device to handle that.  A state between
“successful resize” and “did not change at all, as intended for this
device” should always be an error.

But the device should zero the first cluster like we do here.

>> Also I would rename the function to something like blk_raw_format_file,
>> basically a function which tries its best to erase an existing file contents
>>
>>
>> Yet another idea would to drop the lying in the raw_co_truncate (on block devices), and fail always,
>> unless asked to truncate to the exact file size, and let the callers deal with that.
>> Callers where it is not critical for the truncate to work can just ignore this failure.
>> That is probably hard to implement 
>>
>> Or we can add a truncate 'mode' to .bdrv_co_truncate, which would let the caller indicate its intention,
>> that is if the caller must truncate to that size or it can accept truncate ending up in bigger file that it asked for.

Hm.  That sounds interesting.  Currently, qemu-img resize tries to
inquire whether the truncate did anything useful by checking the length
post-truncate.  It prints a warning if the size didn’t change.

Adding a flag would simplify that and probably this, so that sounds
useful indeed.

>> As we once discussed on IRC, the fact that truncate on a block device 'succeeds',
>> despite not really beeing able to change the block device size, causes other issues,
>> like not beeing able to use preallocation=full when creating a qcow2 image on a block device.
>>
>> Best regards,
>> 	Maxim Levitsky
>>
>>> +
>>> +    current_size = blk_getlength(blk);
>>> +    if (current_size < 0) {
>>> +        error_free(local_err);
>>> +        error_setg_errno(errp, -current_size,
>>> +                         "Failed to inquire new image file's current length");
>>> +        return current_size;
>>> +    }
>>> +
>>> +    if (current_size < offset) {
>>> +        /* Need to grow the image, but we failed to do that */
>>> +        error_propagate(errp, local_err);
>>> +        return -ENOTSUP;
>>> +    }
>>> +
>>> +    error_free(local_err);
>>> +    /*
>>> +     * We can deal with images that are too big.  We just need to
>>> +     * clear the first sector.
>>> +     */
>>> +
>>> +    bytes_to_clear = MIN(current_size, BDRV_SECTOR_SIZE) - offset;
> Also this I think is wrong when offset !=0, since assuming real world device, the
> MIN will be just BDRV_SECTOR_SIZE, so the result of this statement is negative number.

Oh, damn, yes.  Thanks!

> I think you want just
> bytes_to_clear = MIN(current_size, BDRV_SECTOR_SIZE);

I don’t think I want that because I want to start clearing from @offset,
so I do need to subtract it.

But of course I don’t need to do anything if @offset >=
BDRV_SECTOR_SIZE, so there should just be an additional if () block.

Max

>>> +    if (bytes_to_clear) {
>>> +        if (!(blk->root->perm & BLK_PERM_WRITE)) {
>>> +            error_setg(errp, "Cannot clear first sector of new image: "
>>> +                       "Write permission missing");
>>> +            return -EPERM;
>>> +        }
>>> +
>>> +        ret = blk_pwrite_zeroes(blk, offset, bytes_to_clear, 0);
>>> +        if (ret < 0) {
>>> +            error_setg_errno(errp, -ret, "Failed to clear the first sector of "
>>> +                             "the new image");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>  static void blk_pdiscard_entry(void *opaque)
>>>  {
>>>      BlkRwCo *rwco = opaque;
>>
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 0/7] block: Generic file creation fallback

[Maxim Levitsky]

On Fri, 2019-07-12 at 19:35 +0200, Max Reitz wrote:
> Hi,
> 
> Kevin commented on my RFC, so I got what an RFC wants, and he didn’t
> object to the creation fallback part.  So I suppose I can go down that
> route at least.  (Which was actually the more important part of the
> series.)
> 
> So as in the RFC, 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.
> 

Hi!
Do you have any update on this patch series by a chance?

Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [Qemu-block] [PATCH 0/7] block: Generic file creation fallback

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 772 bytes --]

On 05.09.19 15:30, Maxim Levitsky wrote:
> On Fri, 2019-07-12 at 19:35 +0200, Max Reitz wrote:
>> Hi,
>>
>> Kevin commented on my RFC, so I got what an RFC wants, and he didn’t
>> object to the creation fallback part.  So I suppose I can go down that
>> route at least.  (Which was actually the more important part of the
>> series.)
>>
>> So as in the RFC, 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.
>>
> 
> Hi!
> Do you have any update on this patch series by a chance?

Unfortunately, no.  I was on PTO, and before that, there was just too
much else going on.  (And frankly, there’s still too much else going on.)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 0/7] block: Generic file creation fallback

[Maxim Levitsky]

On Tue, 2019-09-10 at 11:16 +0200, Max Reitz wrote:
> On 05.09.19 15:30, Maxim Levitsky wrote:
> > On Fri, 2019-07-12 at 19:35 +0200, Max Reitz wrote:
> > > Hi,
> > > 
> > > Kevin commented on my RFC, so I got what an RFC wants, and he didn’t
> > > object to the creation fallback part.  So I suppose I can go down that
> > > route at least.  (Which was actually the more important part of the
> > > series.)
> > > 
> > > So as in the RFC, 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.
> > > 
> > 
> > Hi!
> > Do you have any update on this patch series by a chance?
> 
> Unfortunately, no.  I was on PTO, and before that, there was just too
> much else going on.  (And frankly, there’s still too much else going on.)

No problem!


Best regards,
	Maxim Levitsky