| Message ID | 1425413591-31413-11-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/03/2015 01:13 PM, Max Reitz wrote: > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 2 ++ > blockdev.c | 18 ++++++++++++++++++ > include/block/block_int.h | 4 ++++ > stubs/Makefile.objs | 1 + > stubs/blockdev-close-all-bdrv-states.c | 5 +++++ > 5 files changed, 30 insertions(+) > create mode 100644 stubs/blockdev-close-all-bdrv-states.c Again, might be nice for the commit message to document why adding this is useful, but doesn't affect the code. Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 03/03/2015 01:13 PM, Max Reitz wrote: >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block.c | 2 ++ >> blockdev.c | 18 ++++++++++++++++++ >> include/block/block_int.h | 4 ++++ >> stubs/Makefile.objs | 1 + >> stubs/blockdev-close-all-bdrv-states.c | 5 +++++ >> 5 files changed, 30 insertions(+) >> create mode 100644 stubs/blockdev-close-all-bdrv-states.c > > Again, might be nice for the commit message to document why adding this > is useful, but doesn't affect the code. > > Reviewed-by: Eric Blake <eblake@redhat.com> Might be nice? This absolutely needs an explanation, in the code! Why do we need a separate list of "monitor-owned BDS"? What makes a BDS "monitor-owned"? What are the invariants governing relations among bdrv_states, graph_bdrv_states (what a horrible name) and blk_backends?
On 2015-03-20 at 04:04, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 03/03/2015 01:13 PM, Max Reitz wrote: >>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>> --- >>> block.c | 2 ++ >>> blockdev.c | 18 ++++++++++++++++++ >>> include/block/block_int.h | 4 ++++ >>> stubs/Makefile.objs | 1 + >>> stubs/blockdev-close-all-bdrv-states.c | 5 +++++ >>> 5 files changed, 30 insertions(+) >>> create mode 100644 stubs/blockdev-close-all-bdrv-states.c >> Again, might be nice for the commit message to document why adding this >> is useful, but doesn't affect the code. >> >> Reviewed-by: Eric Blake <eblake@redhat.com> > Might be nice? This absolutely needs an explanation, in the code! > > Why do we need a separate list of "monitor-owned BDS"? I thought it self-evident, but you're right, of course: The concept being that the monitor can hold references to BDS ("owning them", see below), of course it should keep track of those references (which I thought didn't need an explanation, because if you hold a reference, well, you better actually hold one). > What makes a BDS "monitor-owned"? Being created explicitly. BDS created implicitly because a BB was created (-drive, blockdev-add with @id; in these cases, the BBs are monitor-owned) or that are not the root of a BDS tree (thus created implicitly in that tree) are not monitor-owned. All BDS created explicitly (blockdev-add without @id) are monitor-owned. > What are the invariants governing relations among bdrv_states, > graph_bdrv_states (what a horrible name) and blk_backends? bdrv_states is removed in the follow-up series. I think it's legacy cruft which we needed at a time where we did not have BlockBackends: It tracks all the BDSs with a BB, basically (a BDS is inserted there in bdrv_new_root() which is only called from blk_new_with_bs()). Therefore, we don't need it, as we have a list of all the BBs already. graph_bdrv_states tracks all BDSs with a node name. We need that because we want to find BDSs by node name. blk_backends, which supersedes bdrv_states, basically, will, after the follow-up, contain every single BlockBackend there is. It will be different from monitor_block_backends (follow-up...) in that the latter only contains the monitor-owned BBs. Why we need that difference is a question for the follow-up. However, I can answer it here, too: We do have some operations that should actually be run over all BlockBackends, like blk_name_taken(), blk_drain_all(), blk_flush_all(), and so on. However, all the BBs that are not monitor-owned should not be visible to the user (the monitor!), so for operations like blk_by_name() or qmp_query_blockstats(), only the monitor-owned BBs should be considered. What makes a BB monitor-owned? It's monitor-owned if it has been created using -drive or blockdev-add. Are there any other BBs right now? Except for qemu-{img,io,nbd}, there is only xen_disk.c, so practically, no. What makes a BB monitor-unowned? Deleting its reference through a monitor command while there are still other users (in theory that would be NBD, block jobs, etc., although I don't know whether that actually works that way right now). So it is probably possible to have non-monitor-owned BBs, but those should not be visible to the user. However, we need to consider them inside of the block layer whenever we want to do something for actually all the BBs. Side note: As far as I know, we currently only have drive_del to drop the monitor reference to a BB. What happens there before the follow-up is the BB is made anonymous and removed from blk_backends (blk_hide_on_behalf_of_hmp_drive_del()) and the root BDS from bdrv_states (it's really redundant), so before that series, blk_backends is basically the list of monitor-owned BBs (which I don't think is right, it should contain all BBs, and we need a separate list for the monitor-owned ones). I hope I was able to explain myself. Max
diff --git a/block.c b/block.c index 9698b1d..fce9d5d 100644 --- a/block.c +++ b/block.c @@ -2097,6 +2097,8 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, bs_dest->device_list = bs_src->device_list; /* keep the same entry in all_bdrv_states */ bs_dest->bs_list = bs_src->bs_list; + /* keep the same entry in the list of monitor-owned BDS */ + bs_dest->monitor_list = bs_src->monitor_list; bs_dest->blk = bs_src->blk; diff --git a/blockdev.c b/blockdev.c index 546acfe..a4283a7 100644 --- a/blockdev.c +++ b/blockdev.c @@ -47,6 +47,9 @@ #include "trace.h" #include "sysemu/arch_init.h" +static QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states = + QTAILQ_HEAD_INITIALIZER(monitor_bdrv_states); + static const char *const if_name[IF_COUNT] = { [IF_NONE] = "none", [IF_IDE] = "ide", @@ -646,6 +649,19 @@ fail: return NULL; } +void blockdev_close_all_bdrv_states(void) +{ + BlockDriverState *bs, *next_bs; + + QTAILQ_FOREACH_SAFE(bs, &monitor_bdrv_states, monitor_list, next_bs) { + AioContext *ctx = bdrv_get_aio_context(bs); + + aio_context_acquire(ctx); + bdrv_unref(bs); + aio_context_release(ctx); + } +} + static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to, Error **errp) { @@ -3161,6 +3177,8 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp) if (!bs) { goto fail; } + + QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list); } if (bs && bdrv_key_required(bs)) { diff --git a/include/block/block_int.h b/include/block/block_int.h index 622fbfc..f9968da 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -401,6 +401,8 @@ struct BlockDriverState { QTAILQ_ENTRY(BlockDriverState) device_list; /* element of the list of all BlockDriverStates (all_bdrv_states) */ QTAILQ_ENTRY(BlockDriverState) bs_list; + /* element of the list of monitor-owned BDS */ + QTAILQ_ENTRY(BlockDriverState) monitor_list; QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps; int refcnt; @@ -624,4 +626,6 @@ bool blk_dev_is_tray_open(BlockBackend *blk); bool blk_dev_is_medium_locked(BlockBackend *blk); void blk_dev_resize_cb(BlockBackend *blk); +void blockdev_close_all_bdrv_states(void); + #endif /* BLOCK_INT_H */ diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs index 5e347d0..9169a09 100644 --- a/stubs/Makefile.objs +++ b/stubs/Makefile.objs @@ -1,5 +1,6 @@ stub-obj-y += arch-query-cpu-def.o stub-obj-y += bdrv-commit-all.o +stub-obj-y += blockdev-close-all-bdrv-states.o stub-obj-y += chr-baum-init.o stub-obj-y += chr-msmouse.o stub-obj-y += chr-testdev.o diff --git a/stubs/blockdev-close-all-bdrv-states.c b/stubs/blockdev-close-all-bdrv-states.c new file mode 100644 index 0000000..12d2442 --- /dev/null +++ b/stubs/blockdev-close-all-bdrv-states.c @@ -0,0 +1,5 @@ +#include "block/block_int.h" + +void blockdev_close_all_bdrv_states(void) +{ +}
Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 2 ++ blockdev.c | 18 ++++++++++++++++++ include/block/block_int.h | 4 ++++ stubs/Makefile.objs | 1 + stubs/blockdev-close-all-bdrv-states.c | 5 +++++ 5 files changed, 30 insertions(+) create mode 100644 stubs/blockdev-close-all-bdrv-states.c