qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH 13/22] block/export: Move refcount from NBDExport to Bloc


From: Eric Blake
Subject: Re: [RFC PATCH 13/22] block/export: Move refcount from NBDExport to BlockExport
Date: Wed, 19 Aug 2020 15:58:39 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0

On 8/13/20 11:29 AM, Kevin Wolf wrote:
Having a refcount makes sense for all types of block exports. It is also
a prerequisite for keeping a list of all exports at the BlockExport
level.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---

+++ b/include/block/export.h
@@ -21,14 +21,24 @@ typedef struct BlockExport BlockExport;
  typedef struct BlockExportDriver {
      BlockExportType type;
      BlockExport *(*create)(BlockExportOptions *, Error **);
+    void (*delete)(BlockExport *);
  } BlockExportDriver;
struct BlockExport {
      const BlockExportDriver *drv;
+
+    /*
+     * Reference count for this block export. This includes strong references
+     * both from the owner (qemu-nbd or the monitor) and clients connected to
+     * the export.

I guess 'the monitor' includes qemu-storage-daemon.

+     */
+    int refcount;
  };
extern const BlockExportDriver blk_exp_nbd; BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp);
+void blk_exp_ref(BlockExport *exp);
+void blk_exp_unref(BlockExport *exp);

Yay, I think this naming is more consistent with the rest of qemu...

#endif
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 23030db3f1..af8509ab70 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -336,8 +336,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
  void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
  void nbd_export_close(NBDExport *exp);
  void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error 
**errp);
-void nbd_export_get(NBDExport *exp);
-void nbd_export_put(NBDExport *exp);

...as opposed to this which is common in kernel but less so in this project. No hard feelings from me :)

+++ b/blockdev-nbd.c
@@ -232,7 +232,7 @@ BlockExport *nbd_export_create(BlockExportOptions 
*exp_args, Error **errp)
      /* The list of named exports has a strong reference to this export now and
       * our only way of accessing it is through nbd_export_find(), so we can 
drop
       * the strong reference that is @exp. */
-    nbd_export_put(exp);
+    blk_exp_unref((BlockExport*) exp);

Even a helper function that converts NBDBlockExport* to BlockExport* rather than a cast might be nicer, but then again, I see from Max's review that this may be a temporary state of things.

(The QAPI contains such type-safe container casts, such as qapi_DriveBackup_base(), if that helps...)


@@ -1537,7 +1536,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
exp = g_new0(NBDExport, 1);
      exp->common = (BlockExport) {
-        .drv = &blk_exp_nbd,
+        .drv        = &blk_exp_nbd,
+        .refcount   = 1,

I'm not sure whether trying to align the '=' is good, because the moment you add a longer field name, every other line has to be touched. I'm fine with just one space on both side of =, even if it is more ragged to read. But you're the author, so you get to pick.


@@ -1626,8 +1625,9 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
      exp->ctx = ctx;
      blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
+ blk_exp_ref(&exp->common);
      QTAILQ_INSERT_TAIL(&exports, exp, next);
-    nbd_export_get(exp);
+

Is there any consequence to this changed ordering in grabbing the reference vs. updating the list?


Overall looks nice.

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]