diff mbox series

[RFC,12/22] nbd/server: Simplify export shutdown

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

Commit Message

Kevin Wolf Aug. 13, 2020, 4:29 p.m. UTC
Closing export is somewhat convoluted because nbd_export_close() and
nbd_export_put() call each other and the ways they actually end up being
nested is not necessarily obvious.

However, it is not really necessary to call nbd_export_close() from
nbd_export_put() when putting the last reference because it only does
three things:

1. Close all clients. We're going to refcount 0 and all clients hold a
   reference, so we know there is no active client any more.

2. Close the user reference (represented by exp->name being non-NULL).
   The same argument applies: If the export were still named, we would
   still have a reference.

3. Freeing exp->description. This is really cleanup work to be done when
   the export is finally freed. There is no reason to already clear it
   while clients are still in the process of shutting down.

So after moving the cleanup of exp->description, the code can be
simplified so that only nbd_export_close() calls nbd_export_put(), but
never the other way around.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 nbd/server.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

Comments

Max Reitz Aug. 17, 2020, 2:32 p.m. UTC | #1
On 13.08.20 18:29, Kevin Wolf wrote:
> Closing export is somewhat convoluted because nbd_export_close() and
> nbd_export_put() call each other and the ways they actually end up being
> nested is not necessarily obvious.
> 
> However, it is not really necessary to call nbd_export_close() from
> nbd_export_put() when putting the last reference because it only does
> three things:
> 
> 1. Close all clients. We're going to refcount 0 and all clients hold a
>    reference, so we know there is no active client any more.
> 
> 2. Close the user reference (represented by exp->name being non-NULL).
>    The same argument applies: If the export were still named, we would
>    still have a reference.
> 
> 3. Freeing exp->description. This is really cleanup work to be done when
>    the export is finally freed. There is no reason to already clear it
>    while clients are still in the process of shutting down.

Convincing.

> So after moving the cleanup of exp->description, the code can be
> simplified so that only nbd_export_close() calls nbd_export_put(), but
> never the other way around.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  nbd/server.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>
Eric Blake Aug. 19, 2020, 8:45 p.m. UTC | #2
On 8/13/20 11:29 AM, Kevin Wolf wrote:
> Closing export is somewhat convoluted because nbd_export_close() and
> nbd_export_put() call each other and the ways they actually end up being
> nested is not necessarily obvious.

You are in a maze of twisty little passages, all alike.

Yes, I've always hated that part of the code; thanks for tackling a cleanup.

> 
> However, it is not really necessary to call nbd_export_close() from
> nbd_export_put() when putting the last reference because it only does
> three things:
> 
> 1. Close all clients. We're going to refcount 0 and all clients hold a
>     reference, so we know there is no active client any more.
> 
> 2. Close the user reference (represented by exp->name being non-NULL).
>     The same argument applies: If the export were still named, we would
>     still have a reference.
> 
> 3. Freeing exp->description. This is really cleanup work to be done when
>     the export is finally freed. There is no reason to already clear it
>     while clients are still in the process of shutting down.
> 
> So after moving the cleanup of exp->description, the code can be
> simplified so that only nbd_export_close() calls nbd_export_put(), but
> never the other way around.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   nbd/server.c | 17 ++++-------------
>   1 file changed, 4 insertions(+), 13 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>
diff mbox series

Patch

diff --git a/nbd/server.c b/nbd/server.c
index eadc5b9804..4c594e6558 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1678,8 +1678,6 @@  void nbd_export_close(NBDExport *exp)
         QTAILQ_REMOVE(&exports, exp, next);
         QTAILQ_INSERT_TAIL(&closed_exports, exp, next);
     }
-    g_free(exp->description);
-    exp->description = NULL;
     nbd_export_put(exp);
 }
 
@@ -1706,19 +1704,12 @@  void nbd_export_get(NBDExport *exp)
 void nbd_export_put(NBDExport *exp)
 {
     assert(exp->refcount > 0);
-    if (exp->refcount == 1) {
-        nbd_export_close(exp);
-    }
-
-    /* nbd_export_close() may theoretically reduce refcount to 0. It may happen
-     * if someone calls nbd_export_put() on named export not through
-     * nbd_export_set_name() when refcount is 1. So, let's assert that
-     * it is > 0.
-     */
-    assert(exp->refcount > 0);
     if (--exp->refcount == 0) {
         assert(exp->name == NULL);
-        assert(exp->description == NULL);
+        assert(QTAILQ_EMPTY(&exp->clients));
+
+        g_free(exp->description);
+        exp->description = NULL;
 
         if (exp->blk) {
             if (exp->eject_notifier_blk) {