[PATCH 0/5] Some fixes and improvements for vduse-blk

[PATCH 0/5] Some fixes and improvements for vduse-blk

[Xie Yongji]

This series includes few fixes and improvements for the
vduse-blk export.

Patch 1 fixes resources leak when vduse fd is zero.

Patch 2, 3 fixes two bugs which could be triggered
by force deleting a vduse-blk export with high I/O loads.

Patch 4, 5 adds two new options for vduse-blk export.

Xie Yongji (5):
  libvduse: Fix resources leak in vduse_dev_destroy()
  vduse-blk: Don't unlink the reconnect file if device exists
  vduse-blk: Don't delete the export until all inflight I/Os completed
  vduse-blk: Add serial option
  vduse-blk: Add name option

 block/export/vduse-blk.c             | 53 ++++++++++++++++++++++------
 block/export/vhost-user-blk-server.c |  4 ++-
 block/export/virtio-blk-handler.h    |  2 +-
 docs/tools/qemu-storage-daemon.rst   |  5 +--
 qapi/block-export.json               | 11 +++---
 storage-daemon/qemu-storage-daemon.c |  9 ++---
 subprojects/libvduse/libvduse.c      |  4 +--
 7 files changed, 64 insertions(+), 24 deletions(-)

-- 
2.20.1

[PATCH 1/5] libvduse: Fix resources leak in vduse_dev_destroy()

[Xie Yongji]

This fixes resource leak when the fd is zero in
vduse_dev_destroy().

Fixes: 8dbd281c1675 ("libvduse: Add VDUSE (vDPA Device in Userspace) library")
Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 subprojects/libvduse/libvduse.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/subprojects/libvduse/libvduse.c b/subprojects/libvduse/libvduse.c
index 78bb777402..e781bfa907 100644
--- a/subprojects/libvduse/libvduse.c
+++ b/subprojects/libvduse/libvduse.c
@@ -1374,11 +1374,11 @@ int vduse_dev_destroy(VduseDev *dev)
         free(dev->vqs[i].resubmit_list);
     }
     free(dev->vqs);
-    if (dev->fd > 0) {
+    if (dev->fd >= 0) {
         close(dev->fd);
         dev->fd = -1;
     }
-    if (dev->ctrl_fd > 0) {
+    if (dev->ctrl_fd >= 0) {
         if (ioctl(dev->ctrl_fd, VDUSE_DESTROY_DEV, dev->name)) {
             ret = -errno;
         }
-- 
2.20.1

[PATCH 2/5] vduse-blk: Don't unlink the reconnect file if device exists

[Xie Yongji]

We should not unlink the reconnect file if vduse_dev_destroy()
fails with -EBUSY which means the VDUSE device has not been
removed from the vDPA bus. Otherwise, we might fail on
the reconnection later.

Fixes: 730abef0e873 ("libvduse: Add support for reconnecting")
Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 block/export/vduse-blk.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/export/vduse-blk.c b/block/export/vduse-blk.c
index 3b10349173..c3a89894ae 100644
--- a/block/export/vduse-blk.c
+++ b/block/export/vduse-blk.c
@@ -316,12 +316,15 @@ static int vduse_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
 static void vduse_blk_exp_delete(BlockExport *exp)
 {
     VduseBlkExport *vblk_exp = container_of(exp, VduseBlkExport, export);
+    int ret;
 
     blk_remove_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach,
                                     vblk_exp);
     blk_set_dev_ops(exp->blk, NULL, NULL);
-    vduse_dev_destroy(vblk_exp->dev);
-    unlink(vblk_exp->recon_file);
+    ret = vduse_dev_destroy(vblk_exp->dev);
+    if (ret != -EBUSY) {
+        unlink(vblk_exp->recon_file);
+    }
     g_free(vblk_exp->recon_file);
 }
 
-- 
2.20.1

[PATCH 3/5] vduse-blk: Don't delete the export until all inflight I/Os completed

[Xie Yongji]

Don't delete the export until all inflight I/Os completed.
Otherwise, it might lead to a use-after-free.

Fixes: cc241b5505b2 ("vduse-blk: Implement vduse-blk export")
Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 block/export/vduse-blk.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/block/export/vduse-blk.c b/block/export/vduse-blk.c
index c3a89894ae..251d73c841 100644
--- a/block/export/vduse-blk.c
+++ b/block/export/vduse-blk.c
@@ -31,6 +31,7 @@ typedef struct VduseBlkExport {
     VduseDev *dev;
     uint16_t num_queues;
     char *recon_file;
+    unsigned int inflight;
 } VduseBlkExport;
 
 typedef struct VduseBlkReq {
@@ -38,6 +39,18 @@ typedef struct VduseBlkReq {
     VduseVirtq *vq;
 } VduseBlkReq;
 
+static void vduse_blk_inflight_inc(VduseBlkExport *vblk_exp)
+{
+    vblk_exp->inflight++;
+}
+
+static void vduse_blk_inflight_dec(VduseBlkExport *vblk_exp)
+{
+    if (--vblk_exp->inflight == 0) {
+        aio_wait_kick();
+    }
+}
+
 static void vduse_blk_req_complete(VduseBlkReq *req, size_t in_len)
 {
     vduse_queue_push(req->vq, &req->elem, in_len);
@@ -68,10 +81,13 @@ static void coroutine_fn vduse_blk_virtio_process_req(void *opaque)
     }
 
     vduse_blk_req_complete(req, in_len);
+    vduse_blk_inflight_dec(vblk_exp);
 }
 
 static void vduse_blk_vq_handler(VduseDev *dev, VduseVirtq *vq)
 {
+    VduseBlkExport *vblk_exp = vduse_dev_get_priv(dev);
+
     while (1) {
         VduseBlkReq *req;
 
@@ -83,6 +99,8 @@ static void vduse_blk_vq_handler(VduseDev *dev, VduseVirtq *vq)
 
         Coroutine *co =
             qemu_coroutine_create(vduse_blk_virtio_process_req, req);
+
+        vduse_blk_inflight_inc(vblk_exp);
         qemu_coroutine_enter(co);
     }
 }
@@ -168,6 +186,8 @@ static void vduse_blk_detach_ctx(VduseBlkExport *vblk_exp)
     }
     aio_set_fd_handler(vblk_exp->export.ctx, vduse_dev_get_fd(vblk_exp->dev),
                        true, NULL, NULL, NULL, NULL, NULL);
+
+    AIO_WAIT_WHILE(vblk_exp->export.ctx, vblk_exp->inflight > 0);
 }
 
 
@@ -332,7 +352,9 @@ static void vduse_blk_exp_request_shutdown(BlockExport *exp)
 {
     VduseBlkExport *vblk_exp = container_of(exp, VduseBlkExport, export);
 
+    aio_context_acquire(vblk_exp->export.ctx);
     vduse_blk_detach_ctx(vblk_exp);
+    aio_context_acquire(vblk_exp->export.ctx);
 }
 
 const BlockExportDriver blk_exp_vduse_blk = {
-- 
2.20.1

[PATCH 4/5] vduse-blk: Add serial option

[Xie Yongji]

Add a 'serial' option to allow user to specify this value
explicitly. And the default value is changed to an empty
string as what we did in "hw/block/virtio-blk.c".

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 block/export/vduse-blk.c             | 20 ++++++++++++++------
 block/export/vhost-user-blk-server.c |  4 +++-
 block/export/virtio-blk-handler.h    |  2 +-
 docs/tools/qemu-storage-daemon.rst   |  2 +-
 qapi/block-export.json               |  4 +++-
 storage-daemon/qemu-storage-daemon.c |  1 +
 6 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/block/export/vduse-blk.c b/block/export/vduse-blk.c
index 251d73c841..066e088b00 100644
--- a/block/export/vduse-blk.c
+++ b/block/export/vduse-blk.c
@@ -235,7 +235,7 @@ static int vduse_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
     Error *local_err = NULL;
     struct virtio_blk_config config = { 0 };
     uint64_t features;
-    int i;
+    int i, ret;
 
     if (vblk_opts->has_num_queues) {
         num_queues = vblk_opts->num_queues;
@@ -265,7 +265,8 @@ static int vduse_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
     }
     vblk_exp->num_queues = num_queues;
     vblk_exp->handler.blk = exp->blk;
-    vblk_exp->handler.serial = exp->id;
+    vblk_exp->handler.serial = g_strdup(vblk_opts->has_serial ?
+                                        vblk_opts->serial : "");
     vblk_exp->handler.logical_block_size = logical_block_size;
     vblk_exp->handler.writable = opts->writable;
 
@@ -306,16 +307,16 @@ static int vduse_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
                                      vblk_exp);
     if (!vblk_exp->dev) {
         error_setg(errp, "failed to create vduse device");
-        return -ENOMEM;
+        ret = -ENOMEM;
+        goto err_dev;
     }
 
     vblk_exp->recon_file = g_strdup_printf("%s/vduse-blk-%s",
                                            g_get_tmp_dir(), exp->id);
     if (vduse_set_reconnect_log_file(vblk_exp->dev, vblk_exp->recon_file)) {
         error_setg(errp, "failed to set reconnect log file");
-        vduse_dev_destroy(vblk_exp->dev);
-        g_free(vblk_exp->recon_file);
-        return -EINVAL;
+        ret = -EINVAL;
+        goto err;
     }
 
     for (i = 0; i < num_queues; i++) {
@@ -331,6 +332,12 @@ static int vduse_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
     blk_set_dev_ops(exp->blk, &vduse_block_ops, exp);
 
     return 0;
+err:
+    vduse_dev_destroy(vblk_exp->dev);
+    g_free(vblk_exp->recon_file);
+err_dev:
+    g_free(vblk_exp->handler.serial);
+    return ret;
 }
 
 static void vduse_blk_exp_delete(BlockExport *exp)
@@ -346,6 +353,7 @@ static void vduse_blk_exp_delete(BlockExport *exp)
         unlink(vblk_exp->recon_file);
     }
     g_free(vblk_exp->recon_file);
+    g_free(vblk_exp->handler.serial);
 }
 
 static void vduse_blk_exp_request_shutdown(BlockExport *exp)
diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
index c9c290cc4c..3409d9e02e 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -282,7 +282,7 @@ static int vu_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
         return -EINVAL;
     }
     vexp->handler.blk = exp->blk;
-    vexp->handler.serial = "vhost_user_blk";
+    vexp->handler.serial = g_strdup("vhost_user_blk");
     vexp->handler.logical_block_size = logical_block_size;
     vexp->handler.writable = opts->writable;
 
@@ -296,6 +296,7 @@ static int vu_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
                                  num_queues, &vu_blk_iface, errp)) {
         blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
                                         blk_aio_detach, vexp);
+        g_free(vexp->handler.serial);
         return -EADDRNOTAVAIL;
     }
 
@@ -308,6 +309,7 @@ static void vu_blk_exp_delete(BlockExport *exp)
 
     blk_remove_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach,
                                     vexp);
+    g_free(vexp->handler.serial);
 }
 
 const BlockExportDriver blk_exp_vhost_user_blk = {
diff --git a/block/export/virtio-blk-handler.h b/block/export/virtio-blk-handler.h
index 1c7a5e32ad..150d44cff2 100644
--- a/block/export/virtio-blk-handler.h
+++ b/block/export/virtio-blk-handler.h
@@ -23,7 +23,7 @@
 
 typedef struct {
     BlockBackend *blk;
-    const char *serial;
+    char *serial;
     uint32_t logical_block_size;
     bool writable;
 } VirtioBlkHandler;
diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst
index fbeaf76954..034f2809a6 100644
--- a/docs/tools/qemu-storage-daemon.rst
+++ b/docs/tools/qemu-storage-daemon.rst
@@ -77,7 +77,7 @@ Standard options:
   --export [type=]vhost-user-blk,id=<id>,node-name=<node-name>,addr.type=unix,addr.path=<socket-path>[,writable=on|off][,logical-block-size=<block-size>][,num-queues=<num-queues>]
   --export [type=]vhost-user-blk,id=<id>,node-name=<node-name>,addr.type=fd,addr.str=<fd>[,writable=on|off][,logical-block-size=<block-size>][,num-queues=<num-queues>]
   --export [type=]fuse,id=<id>,node-name=<node-name>,mountpoint=<file>[,growable=on|off][,writable=on|off][,allow-other=on|off|auto]
-  --export [type=]vduse-blk,id=<id>,node-name=<node-name>[,writable=on|off][,num-queues=<num-queues>][,queue-size=<queue-size>][,logical-block-size=<block-size>]
+  --export [type=]vduse-blk,id=<id>,node-name=<node-name>[,writable=on|off][,num-queues=<num-queues>][,queue-size=<queue-size>][,logical-block-size=<block-size>][,serial=<serial-number>]
 
   is a block export definition. ``node-name`` is the block node that should be
   exported. ``writable`` determines whether or not the export allows write
diff --git a/qapi/block-export.json b/qapi/block-export.json
index e4bd4de363..d7aeb1fbf7 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -186,13 +186,15 @@
 # @queue-size: the size of virtqueue. Defaults to 256.
 # @logical-block-size: Logical block size in bytes. Range [512, PAGE_SIZE]
 #                      and must be power of 2. Defaults to 512 bytes.
+# @serial: the serial number of virtio block device. Defaults to empty string.
 #
 # Since: 7.1
 ##
 { 'struct': 'BlockExportOptionsVduseBlk',
   'data': { '*num-queues': 'uint16',
             '*queue-size': 'uint16',
-            '*logical-block-size': 'size'} }
+            '*logical-block-size': 'size',
+            '*serial': 'str' } }
 
 ##
 # @NbdServerAddOptions:
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index 17fd3f2f5f..4e18d3fc85 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -126,6 +126,7 @@ static void help(void)
 "           [,writable=on|off][,num-queues=<num-queues>]\n"
 "           [,queue-size=<queue-size>]\n"
 "           [,logical-block-size=<logical-block-size>]\n"
+"           [,serial=<serial-number>]\n"
 "                         export the specified block node as a vduse-blk\n"
 "                         device using the id as the VDUSE device name\n"
 "\n"
-- 
2.20.1

[PATCH 5/5] vduse-blk: Add name option

[Xie Yongji]

Currently we use 'id' option as the name of VDUSE device.
It's a bit confusing since we use one value for two different
purposes: the ID to identfy the export within QEMU (must be
distinct from any other exports in the same QEMU process, but
can overlap with names used by other processes), and the VDUSE
name to uniquely identify it on the host (must be distinct from
other VDUSE devices on the same host, but can overlap with other
export types like NBD in the same process). To make it clear,
this patch adds a separate 'name' option to specify the VDUSE
name for the vduse-blk export instead.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 block/export/vduse-blk.c             | 4 ++--
 docs/tools/qemu-storage-daemon.rst   | 5 +++--
 qapi/block-export.json               | 7 ++++---
 storage-daemon/qemu-storage-daemon.c | 8 ++++----
 4 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/block/export/vduse-blk.c b/block/export/vduse-blk.c
index 066e088b00..f101c24c3f 100644
--- a/block/export/vduse-blk.c
+++ b/block/export/vduse-blk.c
@@ -300,7 +300,7 @@ static int vduse_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
         features |= 1ULL << VIRTIO_BLK_F_RO;
     }
 
-    vblk_exp->dev = vduse_dev_create(exp->id, VIRTIO_ID_BLOCK, 0,
+    vblk_exp->dev = vduse_dev_create(vblk_opts->name, VIRTIO_ID_BLOCK, 0,
                                      features, num_queues,
                                      sizeof(struct virtio_blk_config),
                                      (char *)&config, &vduse_blk_ops,
@@ -312,7 +312,7 @@ static int vduse_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
     }
 
     vblk_exp->recon_file = g_strdup_printf("%s/vduse-blk-%s",
-                                           g_get_tmp_dir(), exp->id);
+                                           g_get_tmp_dir(), vblk_opts->name);
     if (vduse_set_reconnect_log_file(vblk_exp->dev, vblk_exp->recon_file)) {
         error_setg(errp, "failed to set reconnect log file");
         ret = -EINVAL;
diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst
index 034f2809a6..ea00149a63 100644
--- a/docs/tools/qemu-storage-daemon.rst
+++ b/docs/tools/qemu-storage-daemon.rst
@@ -77,7 +77,7 @@ Standard options:
   --export [type=]vhost-user-blk,id=<id>,node-name=<node-name>,addr.type=unix,addr.path=<socket-path>[,writable=on|off][,logical-block-size=<block-size>][,num-queues=<num-queues>]
   --export [type=]vhost-user-blk,id=<id>,node-name=<node-name>,addr.type=fd,addr.str=<fd>[,writable=on|off][,logical-block-size=<block-size>][,num-queues=<num-queues>]
   --export [type=]fuse,id=<id>,node-name=<node-name>,mountpoint=<file>[,growable=on|off][,writable=on|off][,allow-other=on|off|auto]
-  --export [type=]vduse-blk,id=<id>,node-name=<node-name>[,writable=on|off][,num-queues=<num-queues>][,queue-size=<queue-size>][,logical-block-size=<block-size>][,serial=<serial-number>]
+  --export [type=]vduse-blk,id=<id>,node-name=<node-name>,name=<vduse-name>[,writable=on|off][,num-queues=<num-queues>][,queue-size=<queue-size>][,logical-block-size=<block-size>][,serial=<serial-number>]
 
   is a block export definition. ``node-name`` is the block node that should be
   exported. ``writable`` determines whether or not the export allows write
@@ -111,7 +111,8 @@ Standard options:
   ``allow-other`` to auto (the default) will try enabling this option, and on
   error fall back to disabling it.
 
-  The ``vduse-blk`` export type uses the ``id`` as the VDUSE device name.
+  The ``vduse-blk`` export type takes a ``name`` (must be unique across the host)
+  to create the VDUSE device.
   ``num-queues`` sets the number of virtqueues (the default is 1).
   ``queue-size`` sets the virtqueue descriptor table size (the default is 256).
 
diff --git a/qapi/block-export.json b/qapi/block-export.json
index d7aeb1fbf7..81ef1e3dcd 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -182,6 +182,7 @@
 #
 # A vduse-blk block export.
 #
+# @name: the name of VDUSE device (must be unique across the host).
 # @num-queues: the number of virtqueues. Defaults to 1.
 # @queue-size: the size of virtqueue. Defaults to 256.
 # @logical-block-size: Logical block size in bytes. Range [512, PAGE_SIZE]
@@ -191,7 +192,8 @@
 # Since: 7.1
 ##
 { 'struct': 'BlockExportOptionsVduseBlk',
-  'data': { '*num-queues': 'uint16',
+  'data': { 'name': 'str',
+            '*num-queues': 'uint16',
             '*queue-size': 'uint16',
             '*logical-block-size': 'size',
             '*serial': 'str' } }
@@ -316,8 +318,7 @@
 # Describes a block export, i.e. how single node should be exported on an
 # external interface.
 #
-# @id: A unique identifier for the block export (across the host for vduse-blk
-#      export type or across all export types for other types)
+# @id: A unique identifier for the block export (across all export types)
 #
 # @node-name: The node name of the block node to be exported (since: 5.2)
 #
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index 4e18d3fc85..b8e910f220 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -123,12 +123,12 @@ static void help(void)
 #endif /* CONFIG_VHOST_USER_BLK_SERVER */
 #ifdef CONFIG_VDUSE_BLK_EXPORT
 "  --export [type=]vduse-blk,id=<id>,node-name=<node-name>\n"
-"           [,writable=on|off][,num-queues=<num-queues>]\n"
-"           [,queue-size=<queue-size>]\n"
+"           ,name=<vduse-name>[,writable=on|off]\n"
+"           [,num-queues=<num-queues>][,queue-size=<queue-size>]\n"
 "           [,logical-block-size=<logical-block-size>]\n"
 "           [,serial=<serial-number>]\n"
-"                         export the specified block node as a vduse-blk\n"
-"                         device using the id as the VDUSE device name\n"
+"                         export the specified block node as a\n"
+"                         vduse-blk device\n"
 "\n"
 #endif /* CONFIG_VDUSE_BLK_EXPORT */
 "  --monitor [chardev=]name[,mode=control][,pretty[=on|off]]\n"
-- 
2.20.1