diff mbox series

[19/29] block/export: Move strong user reference to block_exports

Message ID 20200907182011.521007-20-kwolf@redhat.com
State New, archived
Headers show
Series block/export: Add infrastructure and QAPI for block exports | expand

Commit Message

Kevin Wolf Sept. 7, 2020, 6:20 p.m. UTC
The reference owned by the user/monitor that is created when adding the
export and dropped when removing it was tied to the 'exports' list in
nbd/server.c. Every block export will have a user reference, so move it
to the block export level and tie it to the 'block_exports' list in
block/export/export.c instead. This is necessary for introducing a QMP
command for removing exports.

Note that exports are present in block_exports even after the user has
requested shutdown. This is different from NBD's exports where exports
are immediately removed on a shutdown request, even if they are still in
the process of shutting down. In order to avoid that the user still
interacts with an export that is shutting down (and possibly removes it
a second time), we need to remember if the user actually still owns it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/export.h | 8 ++++++++
 block/export/export.c  | 6 ++++++
 blockdev-nbd.c         | 5 -----
 nbd/server.c           | 2 --
 4 files changed, 14 insertions(+), 7 deletions(-)

Comments

Max Reitz Sept. 10, 2020, 1:33 p.m. UTC | #1
On 07.09.20 20:20, Kevin Wolf wrote:
> The reference owned by the user/monitor that is created when adding the
> export and dropped when removing it was tied to the 'exports' list in
> nbd/server.c. Every block export will have a user reference, so move it
> to the block export level and tie it to the 'block_exports' list in
> block/export/export.c instead. This is necessary for introducing a QMP
> command for removing exports.
> 
> Note that exports are present in block_exports even after the user has
> requested shutdown. This is different from NBD's exports where exports
> are immediately removed on a shutdown request, even if they are still in
> the process of shutting down. In order to avoid that the user still
> interacts with an export that is shutting down (and possibly removes it
> a second time), we need to remember if the user actually still owns it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/export.h | 8 ++++++++
>  block/export/export.c  | 6 ++++++
>  blockdev-nbd.c         | 5 -----
>  nbd/server.c           | 2 --
>  4 files changed, 14 insertions(+), 7 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

> diff --git a/include/block/export.h b/include/block/export.h
> index cdc6e161ea..4833947e89 100644
> --- a/include/block/export.h
> +++ b/include/block/export.h
> @@ -60,6 +60,14 @@ struct BlockExport {
>       */
>      int refcount;
>  
> +    /*
> +     * True if one of the references in refcount belongs to the user. After the
> +     * user has dropped their reference, they may not e.g. remove the same
> +     * export a second time (which would decrease the refcount without having
> +     * it incremented first).

Not sure though whether this (or the request_shutdown documentation)
should mention that request_shutdown drops the user reference.

Max

> +     */
> +    bool user_owned;
> +
Max Reitz Sept. 10, 2020, 1:36 p.m. UTC | #2
On 10.09.20 15:33, Max Reitz wrote:
> On 07.09.20 20:20, Kevin Wolf wrote:
>> The reference owned by the user/monitor that is created when adding the
>> export and dropped when removing it was tied to the 'exports' list in
>> nbd/server.c. Every block export will have a user reference, so move it
>> to the block export level and tie it to the 'block_exports' list in
>> block/export/export.c instead. This is necessary for introducing a QMP
>> command for removing exports.
>>
>> Note that exports are present in block_exports even after the user has
>> requested shutdown. This is different from NBD's exports where exports
>> are immediately removed on a shutdown request, even if they are still in
>> the process of shutting down. In order to avoid that the user still
>> interacts with an export that is shutting down (and possibly removes it
>> a second time), we need to remember if the user actually still owns it.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  include/block/export.h | 8 ++++++++
>>  block/export/export.c  | 6 ++++++
>>  blockdev-nbd.c         | 5 -----
>>  nbd/server.c           | 2 --
>>  4 files changed, 14 insertions(+), 7 deletions(-)
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
>> diff --git a/include/block/export.h b/include/block/export.h
>> index cdc6e161ea..4833947e89 100644
>> --- a/include/block/export.h
>> +++ b/include/block/export.h
>> @@ -60,6 +60,14 @@ struct BlockExport {
>>       */
>>      int refcount;
>>  
>> +    /*
>> +     * True if one of the references in refcount belongs to the user. After the
>> +     * user has dropped their reference, they may not e.g. remove the same
>> +     * export a second time (which would decrease the refcount without having
>> +     * it incremented first).
> 
> Not sure though whether this (or the request_shutdown documentation)
> should mention that request_shutdown drops the user reference.

Er, no, it shouldn’t, because it doesn’t.  Only
blk_exp_request_shutdown() does that.  Though since that’s a public
function (used by nbd/server.c), maybe it should be documented there.

Max
diff mbox series

Patch

diff --git a/include/block/export.h b/include/block/export.h
index cdc6e161ea..4833947e89 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -60,6 +60,14 @@  struct BlockExport {
      */
     int refcount;
 
+    /*
+     * True if one of the references in refcount belongs to the user. After the
+     * user has dropped their reference, they may not e.g. remove the same
+     * export a second time (which would decrease the refcount without having
+     * it incremented first).
+     */
+    bool user_owned;
+
     /* The AioContext whose lock protects this BlockExport object. */
     AioContext *ctx;
 
diff --git a/block/export/export.c b/block/export/export.c
index 21e9013fb6..42aac867e9 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -80,6 +80,7 @@  BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
     *exp = (BlockExport) {
         .drv        = &blk_exp_nbd,
         .refcount   = 1,
+        .user_owned = true,
         .id         = g_strdup(export->id),
     };
 
@@ -136,6 +137,11 @@  void blk_exp_request_shutdown(BlockExport *exp)
 
     aio_context_acquire(aio_context);
     exp->drv->request_shutdown(exp);
+
+    assert(exp->user_owned);
+    exp->user_owned = false;
+    blk_exp_unref(exp);
+
     aio_context_release(aio_context);
 }
 
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 814554dd90..9efbaef8f7 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -235,11 +235,6 @@  int nbd_export_create(BlockExport *exp, BlockExportOptions *exp_args,
         goto out;
     }
 
-    /* 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. */
-    blk_exp_unref(exp);
-
     ret = 0;
  out:
     aio_context_release(aio_context);
diff --git a/nbd/server.c b/nbd/server.c
index 32147e4871..22a1d66168 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1616,7 +1616,6 @@  int nbd_export_new(BlockExport *blk_exp, BlockDriverState *bs,
 
     blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
 
-    blk_exp_ref(&exp->common);
     QTAILQ_INSERT_TAIL(&exports, exp, next);
 
     return 0;
@@ -1663,7 +1662,6 @@  static void nbd_export_request_shutdown(BlockExport *blk_exp)
         client_close(client, true);
     }
     if (exp->name) {
-        blk_exp_unref(&exp->common);
         g_free(exp->name);
         exp->name = NULL;
         QTAILQ_REMOVE(&exports, exp, next);