Re: [Qemu-devel] [PATCH v3 02/23] block: New BlockBackend

[Markus Armbruster]

Max Reitz <address@hidden> writes:

> On 16.09.2014 20:12, Markus Armbruster wrote:
>> A block device consists of a frontend device model and a backend.
>>
>> A block backend has a tree of block drivers doing the actual work.
>> The tree is managed by the block layer.
>>
>> We currently use a single abstraction BlockDriverState both for tree
>> nodes and the backend as a whole.  Drawbacks:
>>
>> * Its API includes both stuff that makes sense only at the block
>>    backend level (root of the tree) and stuff that's only for use
>>    within the block layer.  This makes the API bigger and more complex
>>    than necessary.  Moreover, it's not obvious which interfaces are
>>    meant for device models, and which really aren't.
>>
>> * Since device models keep a reference to their backend, the backend
>>    object can't just be destroyed.  But for media change, we need to
>>    replace the tree.  Our solution is to make the BlockDriverState
>>    generic, with actual driver state in a separate object, pointed to
>>    by member opaque.  That lets us replace the tree by deinitializing
>>    and reinitializing its root.  This special need of the root makes
>>    the data structure awkward everywhere in the tree.
>>
>> The general plan is to separate the APIs into "block backend", for use
>> by device models, monitor and whatever other code dealing with block
>> backends, and "block driver", for use by the block layer and whatever
>> other code (if any) dealing with trees and tree nodes.
>>
>> Code dealing with block backends, device models in particular, should
>> become completely oblivious of BlockDriverState.  This should let us
>> clean up both APIs, and the tree data structures.
>>
>> This commit is a first step.  It creates a minimal "block backend"
>> API: type BlockBackend and functions to create, destroy and find them.
>>
>> BlockBackend objects are created and destroyed exactly when root
>> BlockDriverState objects are created and destroyed.  "Root" in the
>> sense of "in bdrv_states".  They're not yet used for anything; that'll
>> come shortly.
>>
>> BlockBackend is reference-counted.  Its reference count never exceeds
>> one so far, but that's going to change.
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>   block/Makefile.objs            |   2 +-
>>   block/block-backend.c          | 120 
>> +++++++++++++++++++++++++++++++++++++++++
>>   blockdev.c                     |  11 +++-
>>   hw/block/xen_disk.c            |  11 ++++
>>   include/qemu/typedefs.h        |   1 +
>>   include/sysemu/block-backend.h |  26 +++++++++
>>   qemu-img.c                     |  70 +++++++++++++++++++++---
>>   qemu-io.c                      |   8 +++
>>   qemu-nbd.c                     |   5 +-
>>   9 files changed, 243 insertions(+), 11 deletions(-)
>>   create mode 100644 block/block-backend.c
>>   create mode 100644 include/sysemu/block-backend.h
>
> [snip]
>
>> diff --git a/blockdev.c b/blockdev.c
>> index c9463e3..583235a 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>
> [snip]
>
>> @@ -1791,6 +1799,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict, 
>> QObject **ret_data)
>>       } else {
>>           drive_del(dinfo);
>>       }
>> +    blk_unref(blk_by_name(id));
>>         aio_context_release(aio_context);
>>       return 0;
>
> Well, now if a BB is created by blockdev_init() but not deleted from
> the monitor, it will not be unref'd. But first of all this is not bad,
> because the time it should be unref'd is exactly when qemu is exiting
> anyway and second, the BDS is not unref'd either.

Yes.  Such a BDS can get destroyed either by explicit monitor command
(here), or when the device using it is unplugged (blockdev_auto_del(),
but only when blockdev_init() created it on behalf of drive_new()).

In this patch, the idea is to match the BB's lifetime to its BDS's
lifetime.

> Therefore:
>
> Reviewed-by: Max Reitz <address@hidden>

Thanks!