| Message ID | 20200907182011.521007-1-kwolf@redhat.com |
|---|---|
| Headers | show |
| Series | block/export: Add infrastructure and QAPI for block exports | expand |
On 07.09.20 20:19, Kevin Wolf wrote: > nbd-server-add tries to be convenient and adds two questionable > features that we don't want to share in block-export-add, even for NBD > exports: > > 1. When requesting a writable export of a read-only device, the export > is silently downgraded to read-only. This should be an error in the > context of block-export-add. > > 2. When using a BlockBackend name, unplugging the device from the guest > will automatically stop the NBD server, too. This may sometimes be > what you want, but it could also be very surprising. Let's keep > things explicit with block-export-add. If the user wants to stop the > export, they should tell us so. > > Move these things into the nbd-server-add QMP command handler so that > they apply only there. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > include/block/export.h | 2 ++ > include/block/nbd.h | 3 ++- > block/export/export.c | 13 +++++++++--- > blockdev-nbd.c | 47 +++++++++++++++++++++++++++++++++++------- > nbd/server.c | 20 +++++++++++------- > qemu-nbd.c | 3 +-- > 6 files changed, 67 insertions(+), 21 deletions(-) [...] > + if (bdrv_is_read_only(bs)) { > + export_opts.u.nbd.has_writable = true; Ah, yes, setting that might be nice. :) Reviewed-by: Max Reitz <mreitz@redhat.com>
On 07.09.20 20:19, Kevin Wolf wrote: > Every block export needs a block node to export, so add a 'node-name' > option to BlockExportOptions and remove the replaced option 'device' > from BlockExportOptionsNbd. > > To maintain compatibility in nbd-server-add, BlockExportOptionsNbd needs > to be wrapped by a new type NbdServerAddOptions that adds 'device' back > because nbd-server-add doesn't use the BlockExportOptions base type at > all (so even without changing it to a 'node-name' option in > block-export-add, this compatibility code would be necessary). > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > qapi/block-export.json | 27 ++++++++++++++++----- > block/monitor/block-hmp-cmds.c | 6 ++--- > blockdev-nbd.c | 44 +++++++++++++++++++++++++--------- > qemu-nbd.c | 2 +- > 4 files changed, 58 insertions(+), 21 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com>
On 07.09.20 20:19, Kevin Wolf wrote: > This adds a function to shut down all block exports, and another one to > shut down the block exports of a single type. The latter is used for now > when stopping the NBD server. As soon as we implement support for > multiple NBD servers, we'll need a per-server list of exports and it > will be replaced by a function using that. > > As a side effect, the BlockExport layer has a list tracking all existing > exports now. closed_exports loses its only user and can go away. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > include/block/export.h | 15 ++++++++ > include/block/nbd.h | 2 -- > block.c | 2 +- > block/export/export.c | 79 ++++++++++++++++++++++++++++++++++++++++-- > blockdev-nbd.c | 2 +- > nbd/server.c | 34 +++--------------- > qemu-nbd.c | 2 +- > 7 files changed, 100 insertions(+), 36 deletions(-) [...] > /* Callers must hold exp->ctx lock */ > void blk_exp_unref(BlockExport *exp) > { > assert(exp->refcount > 0); > if (--exp->refcount == 0) { > - exp->drv->delete(exp); > - g_free(exp); > + /* Touch the block_exports list only in the main thread */ > + aio_bh_schedule_oneshot(qemu_get_aio_context(), blk_exp_delete_bh, > + exp); Looks safe. Reviewed-by: Max Reitz <mreitz@redhat.com> (The effort of special-casing this to delete the export immediately if we already run in the main thread doesn’t look worth it.)
On 07.09.20 20:20, Kevin Wolf wrote: > We'll need an id to identify block exports in monitor commands. This > adds one. > > Note that this is different from the 'name' option in the NBD server, > which is the externally visible export name. While block export ids need > to be unique in the whole process, export names must be unique only for > the same server. Different export types or (potentially in the future) > multiple NBD servers can have the same export name externally, but still > need different block export ids internally. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > qapi/block-export.json | 5 +++++ > include/block/export.h | 3 +++ > block/export/export.c | 26 ++++++++++++++++++++++++++ > blockdev-nbd.c | 1 + > qemu-nbd.c | 1 + > storage-daemon/qemu-storage-daemon.c | 2 +- > tests/qemu-iotests/223.out | 4 ++-- > 7 files changed, 39 insertions(+), 3 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com>