[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 02/23] block: New BlockBackend
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v3 02/23] block: New BlockBackend |
Date: |
Fri, 19 Sep 2014 19:13:56 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
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!
- Re: [Qemu-devel] [PATCH v3 03/23] block: Connect BlockBackend to BlockDriverState, (continued)
[Qemu-devel] [PATCH v3 05/23] block: Code motion to get rid of stubs/blockdev.c, Markus Armbruster, 2014/09/16
[Qemu-devel] [PATCH v3 09/23] block: Merge BlockBackend and BlockDriverState name spaces, Markus Armbruster, 2014/09/16
[Qemu-devel] [PATCH v3 02/23] block: New BlockBackend, Markus Armbruster, 2014/09/16
[Qemu-devel] [PATCH v3 07/23] block: Eliminate bdrv_iterate(), use bdrv_next(), Markus Armbruster, 2014/09/16
[Qemu-devel] [PATCH v3 10/23] block: Eliminate DriveInfo member bdrv, use blk_by_legacy_dinfo(), Markus Armbruster, 2014/09/16