[PATCH] block/export: call blk_set_dev_ops(blk, NULL, NULL)

[PATCH] block/export: call blk_set_dev_ops(blk, NULL, NULL)

[Stefan Hajnoczi]

Most export types install BlockDeviceOps pointers. It is easy to forget
to remove them because that happens automatically via the "drive" qdev
property in hw/ but not block/export/.

Put blk_set_dev_ops(blk, NULL, NULL) calls in the core export.c code so
the export types don't need to remember.

This fixes the nbd and vhost-user-blk export types.

Fixes: fd6afc501a01 ("nbd/server: Use drained block ops to quiesce the server")
Fixes: ca858a5fe94c ("vhost-user-blk-server: notify client about disk resize")
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/export/export.c    | 2 ++
 block/export/vduse-blk.c | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/export/export.c b/block/export/export.c
index e3fee60611..62c7c22d45 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -192,6 +192,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
     return exp;
 
 fail:
+    blk_set_dev_ops(exp->blk, NULL, NULL);
     blk_unref(blk);
     aio_context_release(ctx);
     if (exp) {
@@ -219,6 +220,7 @@ static void blk_exp_delete_bh(void *opaque)
     assert(exp->refcount == 0);
     QLIST_REMOVE(exp, next);
     exp->drv->delete(exp);
+    blk_set_dev_ops(exp->blk, NULL, NULL);
     blk_unref(exp->blk);
     qapi_event_send_block_export_deleted(exp->id);
     g_free(exp->id);
diff --git a/block/export/vduse-blk.c b/block/export/vduse-blk.c
index f7ae44e3ce..b53ef39da0 100644
--- a/block/export/vduse-blk.c
+++ b/block/export/vduse-blk.c
@@ -346,7 +346,6 @@ static void vduse_blk_exp_delete(BlockExport *exp)
 
     blk_remove_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach,
                                     vblk_exp);
-    blk_set_dev_ops(exp->blk, NULL, NULL);
     ret = vduse_dev_destroy(vblk_exp->dev);
     if (ret != -EBUSY) {
         unlink(vblk_exp->recon_file);
-- 
2.40.1

Re: [PATCH] block/export: call blk_set_dev_ops(blk, NULL, NULL)

[Eric Blake]

On Tue, May 02, 2023 at 05:11:19PM -0400, Stefan Hajnoczi wrote:
> Most export types install BlockDeviceOps pointers. It is easy to forget
> to remove them because that happens automatically via the "drive" qdev
> property in hw/ but not block/export/.
> 
> Put blk_set_dev_ops(blk, NULL, NULL) calls in the core export.c code so
> the export types don't need to remember.
> 
> This fixes the nbd and vhost-user-blk export types.
> 
> Fixes: fd6afc501a01 ("nbd/server: Use drained block ops to quiesce the server")
> Fixes: ca858a5fe94c ("vhost-user-blk-server: notify client about disk resize")
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/export/export.c    | 2 ++
>  block/export/vduse-blk.c | 1 -
>  2 files changed, 2 insertions(+), 1 deletion(-)

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

I'm happy to add this through my NBD queue.

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

Re: [PATCH] block/export: call blk_set_dev_ops(blk, NULL, NULL)

[Stefan Hajnoczi]

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

On Wed, May 03, 2023 at 10:43:16AM -0500, Eric Blake wrote:
> On Tue, May 02, 2023 at 05:11:19PM -0400, Stefan Hajnoczi wrote:
> > Most export types install BlockDeviceOps pointers. It is easy to forget
> > to remove them because that happens automatically via the "drive" qdev
> > property in hw/ but not block/export/.
> > 
> > Put blk_set_dev_ops(blk, NULL, NULL) calls in the core export.c code so
> > the export types don't need to remember.
> > 
> > This fixes the nbd and vhost-user-blk export types.
> > 
> > Fixes: fd6afc501a01 ("nbd/server: Use drained block ops to quiesce the server")
> > Fixes: ca858a5fe94c ("vhost-user-blk-server: notify client about disk resize")
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  block/export/export.c    | 2 ++
> >  block/export/vduse-blk.c | 1 -
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> I'm happy to add this through my NBD queue.

Sure, go ahead!

Stefan

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

Re: [PATCH] block/export: call blk_set_dev_ops(blk, NULL, NULL)

[Kevin Wolf]

Am 02.05.2023 um 23:11 hat Stefan Hajnoczi geschrieben:
> Most export types install BlockDeviceOps pointers. It is easy to forget
> to remove them because that happens automatically via the "drive" qdev
> property in hw/ but not block/export/.
> 
> Put blk_set_dev_ops(blk, NULL, NULL) calls in the core export.c code so
> the export types don't need to remember.
> 
> This fixes the nbd and vhost-user-blk export types.
> 
> Fixes: fd6afc501a01 ("nbd/server: Use drained block ops to quiesce the server")
> Fixes: ca858a5fe94c ("vhost-user-blk-server: notify client about disk resize")
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/export/export.c    | 2 ++
>  block/export/vduse-blk.c | 1 -
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/export/export.c b/block/export/export.c
> index e3fee60611..62c7c22d45 100644
> --- a/block/export/export.c
> +++ b/block/export/export.c
> @@ -192,6 +192,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
>      return exp;
>  
>  fail:
> +    blk_set_dev_ops(exp->blk, NULL, NULL);
>      blk_unref(blk);
>      aio_context_release(ctx);
>      if (exp) {

The last line of the context already shows that dereferencing exp
unconditionally is wrong. I'll fix it in my next series that tries to
address Fiona's concern that we need to take the graph lock even in the
main thread if we're in a coroutine.

Kevin