Re: [PATCH V9 05/17] block/export: track IOThread reference in BlockExport

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 8096 bytes --]

On Wed, Jun 24, 2026 at 03:08:39PM +0800, Zhang Chen wrote:
> Users currently lack visibility into which block exports
> are utilizing specific IOThreads. This patch integrates IOThread
> referencing into the BlockExport lifecycle.
> 
> - Add iothreads array and holder_name to BlockExport struct.
> - Use iothread_ref_and_get_aio_context during export creation.
> - Implement proper cleanup in blk_exp_add fail path and blk_exp_delete_bh.
> - Support both single and multi-iothread export configurations.
> 
> This ensures IOThread 'holders' status correctly reflects active block
> exports for better debugging and resource tracking.
> 
> Signed-off-by: Zhang Chen <zhangckid@gmail.com>
> ---
>  block/export/export.c  | 63 ++++++++++++++++++++++++++++++++++++------
>  include/block/export.h |  6 ++++
>  2 files changed, 60 insertions(+), 9 deletions(-)

If AI was used to generate this patch series, please see QEMU's AI
policy:
https://www.qemu.org/docs/master/devel/code-provenance.html#use-of-ai-generated-content

This feels like AI generated code in that it is verbose and has lots of
structure (e.g. commit messages that look like an AI summary of the code
change), but the details are not correct (misleading comments, leaks,
etc).

I will pause my review at this patch and wait for your response.

> 
> diff --git a/block/export/export.c b/block/export/export.c
> index b733f269f3..b6c07f69b5 100644
> --- a/block/export/export.c
> +++ b/block/export/export.c
> @@ -15,7 +15,6 @@
>  
>  #include "block/block.h"
>  #include "system/block-backend.h"
> -#include "system/iothread.h"
>  #include "block/export.h"
>  #include "block/fuse.h"
>  #include "block/nbd.h"
> @@ -85,6 +84,8 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
>      AioContext *ctx;
>      AioContext **multithread_ctxs = NULL;
>      size_t multithread_count = 0;
> +    g_autofree IOThread **local_iothreads = NULL;
> +    const char *holder_name = NULL;
>      uint64_t perm;
>      int ret;
>  
> @@ -139,7 +140,16 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
>              goto fail;
>          }
>  
> -        new_ctx = iothread_get_aio_context(iothread);
> +        holder_name = bdrv_get_node_name(bs);
> +        IOThreadHolder holder = {
> +            .type = IO_THREAD_HOLDER_KIND_BLOCK_NODE,

This is not a block node, this is a block export (a separate type of
object). Reporting the export as a node is confusing because users will
not know which block export id is assigned using this IOThread, they
will only know the block node (which could be doing other things too).

> +            .u.block_node.node_name = (char *)holder_name,

To remove the suspicious-looking const cast, try:

  const IOThreadHolder holder = {
      .type = ...,
      .u.block_node.node_name = holder_name,
  };

> +        };
> +
> +        new_ctx = iothread_ref_and_get_aio_context(iothread, &holder);
> +        multithread_count = 1;
> +        local_iothreads = g_new0(IOThread *, 1);
> +        local_iothreads[0] = iothread;
>  
>          /* Ignore errors with fixed-iothread=false */
>          set_context_errp = fixed_iothread ? errp : NULL;
> @@ -163,8 +173,15 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
>              return NULL;
>          }
>  
> +        local_iothreads = g_new0(IOThread *, multithread_count);
>          multithread_ctxs = g_new(AioContext *, multithread_count);
>          i = 0;
> +        holder_name = bdrv_get_node_name(bs);
> +        IOThreadHolder holder = {
> +            .type = IO_THREAD_HOLDER_KIND_BLOCK_NODE,
> +            .u.block_node.node_name = (char *)holder_name,
> +        };
> +
>          for (strList *e = iothread_list; e; e = e->next) {
>              IOThread *iothread = iothread_by_id(e->value);
>  
> @@ -172,7 +189,9 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
>                  error_setg(errp, "iothread \"%s\" not found", e->value);
>                  goto fail;
>              }
> -            multithread_ctxs[i++] = iothread_get_aio_context(iothread);
> +            local_iothreads[i] = iothread;
> +            multithread_ctxs[i++] = iothread_ref_and_get_aio_context(iothread,
> +                                                                     &holder);
>          }
>          assert(i == multithread_count);
>      }
> @@ -225,12 +244,15 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
>      assert(drv->instance_size >= sizeof(BlockExport));
>      exp = g_malloc0(drv->instance_size);
>      *exp = (BlockExport) {
> -        .drv        = drv,
> -        .refcount   = 1,
> -        .user_owned = true,
> -        .id         = g_strdup(export->id),
> -        .ctx        = ctx,
> -        .blk        = blk,
> +        .drv                  = drv,
> +        .refcount             = 1,
> +        .user_owned           = true,
> +        .id                   = g_strdup(export->id),
> +        .ctx                  = ctx,
> +        .blk                  = blk,
> +        .iothreads            = g_steal_pointer(&local_iothreads),
> +        .iothread_count       = multithread_count,
> +        .iothread_holder_name = g_strdup(holder_name),
>      };
>  
>      ret = drv->create(exp, export, multithread_ctxs, multithread_count, errp);
> @@ -253,6 +275,18 @@ fail:
>          g_free(exp->id);
>          g_free(exp);
>      }
> +    if (local_iothreads) {

This doesn't handle the drv->create() goto fail code path where
g_steal_pointer(&local_iothreads) has moved the pointer into exp.
local_iothreads will be leaked.

iothread_holder_name will also be leaked (it should be handled like
g_free(exp->id) above).

> +        IOThreadHolder holder = {
> +            .type = IO_THREAD_HOLDER_KIND_BLOCK_NODE,
> +            .u.block_node.node_name = (char *)holder_name,
> +        };
> +
> +        for (size_t j = 0; j < multithread_count; j++) {
> +            if (local_iothreads[j]) {
> +                iothread_put_aio_context(local_iothreads[j], &holder);
> +            }
> +        }
> +    }
>      g_free(multithread_ctxs);
>      return NULL;
>  }
> @@ -269,6 +303,17 @@ static void blk_exp_delete_bh(void *opaque)
>      BlockExport *exp = opaque;
>  
>      assert(exp->refcount == 0);
> +    if (exp->iothreads) {
> +        IOThreadHolder holder = {
> +            .type = IO_THREAD_HOLDER_KIND_BLOCK_NODE,
> +            .u.block_node.node_name = (char *)exp->iothread_holder_name,
> +        };
> +
> +        for (size_t i = 0; i < exp->iothread_count; i++) {
> +            iothread_put_aio_context(exp->iothreads[i], &holder);
> +        }
> +        g_free(exp->iothreads);
> +    }

iothread_holder_name is leaked.

>      QLIST_REMOVE(exp, next);
>      exp->drv->delete(exp);

iothread_put_aio_context() must be moved after ->delete() since exports
may still need to do some AioContext cleanup (e.g.
vduse_blk_detach_ctx()).

>      blk_set_dev_ops(exp->blk, NULL, NULL);
> diff --git a/include/block/export.h b/include/block/export.h
> index ca45da928c..2bb98aae31 100644
> --- a/include/block/export.h
> +++ b/include/block/export.h
> @@ -16,6 +16,7 @@
>  
>  #include "qapi/qapi-types-block-export.h"
>  #include "qemu/queue.h"
> +#include "system/iothread.h"
>  
>  typedef struct BlockExport BlockExport;
>  
> @@ -89,6 +90,11 @@ struct BlockExport {
>  
>      /* List entry for block_exports */
>      QLIST_ENTRY(BlockExport) next;
> +
> +    /* The iothreads list for block_exports */

This comment is confusing: block_export is the global list of
BlockExports. This iothreads[] array is not for block_exports, it's for
this specific BlockExport only.

> +    IOThread **iothreads;
> +    size_t iothread_count;
> +    char *iothread_holder_name;
>  };
>  
>  BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp);
> -- 
> 2.49.0
> 

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