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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben:
>> 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>
>
>> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
>> new file mode 100644
>> index 0000000..3f8371c
>> --- /dev/null
>> +++ b/include/sysemu/block-backend.h
>> @@ -0,0 +1,26 @@
>> +/*
>> + * QEMU Block backends
>> + *
>> + * Copyright (C) 2014 Red Hat, Inc.
>> + *
>> + * Authors:
>> + *  Markus Armbruster <address@hidden>,
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> + * later.  See the COPYING file in the top-level directory.
>> + */
>
> This one should be LGPL as well.

Forgot %-}

> The distribution of blk_unref() should be good enough. To be correct it
> requires the assumption that the blockdev-add reference to any given BDS
> is the last one that goes away. I'm not sure if this is the case here (I
> suspect it isn't), but at the end of the series, we should be good.

You're doubting the code works when a BB dies, but something else still
holds a reference to its BDS, keeping it alive.  Yes, that's the
troublesome case.  Let me review how it develops throughout this series.

PATCH 02: BB and BDS are not yet connected, and independently reference
counted.  Anything that creates a root BDS also creates a BB.  The two
are unref'ed at the same time.  The BB dies for sure, because nothing
else has a reference.  The BDS may live on, but isn't affected by the
death of the BB.

PATCH 03: BB and BDS point to each other, but neither owns the other,
yet.  The BDS is now exposed to the BDS's death: it's pointer bs->blk
becomes null.  However, it's only ever used right after creation, when
the BB surely lives.

PATCH 04: BB owns DriveInfo.  Adds a few uses of bs->blk, but they're
still all in code where the BB surely lives: device model initialization
and destruction, drive_del command.

PATCH 06: BB owns BDS, taking over the reference from its creator.

PATCH 08: First patch that makes the BDS be affected by the death of the
BB: the value of bdrv_get_device_name() becomes "" then.  The two
interesting BB killers are do_drive_del() and blockdev_auto_del().  Both
were coded before BDS reference counting, and both used to actually kill
the BDS.  When do_drive_del() can't kill it, because a device model
still has an (uncounted!) reference, it makes it anonymous.  Which
changes the name to "".  I believe both killers need a deep think on
what they're supposed to do when the BDS reference count is > 1.

PATCH 23: Device models' references to BB become strong.

> With the license fixed, you can add:
>
>     Reviewed-by Kevin Wolf <address@hidden>

Thanks!