Re: [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2013-5-29 17:09, Stefan Hajnoczi 写道:
> On Wed, May 29, 2013 at 03:54:46PM +0800, Wenchao Xia wrote:
>> 于 2013-5-28 15:46, Stefan Hajnoczi 写道:
>>> On Mon, May 27, 2013 at 05:25:16PM +0200, Kevin Wolf wrote:
>>>> Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:
>>>>> From: Stefan Hajnoczi <stefanha@redhat.com>
>>>>>
>>>>> The bs_snapshots global variable points to the BlockDriverState which
>>>>> will be used to save vmstate.  This is really a savevm.c concept but was
>>>>> moved into block.c:bdrv_snapshots() when it became clear that hotplug
>>>>> could result in a dangling pointer.
>>>>>
>>>>> While auditing the block layer's global state I came upon bs_snapshots
>>>>> and realized that a variable is not necessary here.  Simply find the
>>>>> first BlockDriverState capable of internal snapshots each time this is
>>>>> needed.
>>>>>
>>>>> The behavior of bdrv_snapshots() is preserved across hotplug because new
>>>>> drives are always appended to the bdrv_states list.  This means that
>>>>> calling the new find_vmstate_bs() function is idempotent - it returns
>>>>> the same BlockDriverState unless it was hot-unplugged.
>>>>>
>>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>> Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>>> ---
>>>>>   block.c               |   28 ----------------------------
>>>>>   include/block/block.h |    1 -
>>>>>   savevm.c              |   19 +++++++++++++++----
>>>>>   3 files changed, 15 insertions(+), 33 deletions(-)
>>>>>
>>>>> diff --git a/block.c b/block.c
>>>>> index 3f87489..478a3b2 100644
>>>>> --- a/block.c
>>>>> +++ b/block.c
>>>>> @@ -99,9 +99,6 @@ static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
>>>>>   static QLIST_HEAD(, BlockDriver) bdrv_drivers =
>>>>>       QLIST_HEAD_INITIALIZER(bdrv_drivers);
>>>>>
>>>>> -/* The device to use for VM snapshots */
>>>>> -static BlockDriverState *bs_snapshots;
>>>>> -
>>>>>   /* If non-zero, use only whitelisted block drivers */
>>>>>   static int use_bdrv_whitelist;
>>>>>
>>>>> @@ -1357,9 +1354,6 @@ void bdrv_close(BlockDriverState *bs)
>>>>>       notifier_list_notify(&bs->close_notifiers, bs);
>>>>>
>>>>>       if (bs->drv) {
>>>>> -        if (bs == bs_snapshots) {
>>>>> -            bs_snapshots = NULL;
>>>>> -        }
>>>>>           if (bs->backing_hd) {
>>>>>               bdrv_delete(bs->backing_hd);
>>>>>               bs->backing_hd = NULL;
>>>>> @@ -1591,7 +1585,6 @@ void bdrv_delete(BlockDriverState *bs)
>>>>>
>>>>>       bdrv_close(bs);
>>>>>
>>>>> -    assert(bs != bs_snapshots);
>>>>>       g_free(bs);
>>>>>   }
>>>>>
>>>>> @@ -1635,9 +1628,6 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
>>>>>   {
>>>>>       bs->dev_ops = ops;
>>>>>       bs->dev_opaque = opaque;
>>>>> -    if (bdrv_dev_has_removable_media(bs) && bs == bs_snapshots) {
>>>>> -        bs_snapshots = NULL;
>>>>> -    }
>>>>
>>>> This hunk isn't replaced by any other code. If I understand correctly
>>>> what it's doing, it prevented you from saving the VM state to a
>>>> removable device, which would be allowed after this patch.
>>>>
>>>> Is this a change we want to make? Why isn't it described in the commit
>>>> message?
>>>
>>> My understanding of this change is different.  Markus is on CC so maybe
>>> he can confirm.
>>>
>>> The point of bs_snapshots = NULL is not to prevent you from saving
>>> snapshots.  It's simply to reset the pointer to the next snapshottable
>>> device (used by bdrv_snapshots()).
>>>
>>> See the bdrv_close() hunk above which does the same thing, as well as
>>> bdrv_snapshots() which iterates bdrv_states and updates bs_snapshots.
>>>
>>> So what this hunk does is to reset the bdrv_snapshots() iterator when a
>>> removable device is hooked up to an emulated storage controller.  It's
>>> no longer necessary since this patch drops the global state
>>> (bs_snapshots) and users will always iterate from scratch.
>>>
>>> The whole stateful approach was not necessary.
>>>
>>> Stefan
>>>
>>    Reading the code, original logic actually forbidded saving vmstate
>> into a removable device, now it is possible since find_vmstate_bs()
>> doesn't check it. How about forbid again in find_vmstate_bs()?
>
> I don't follow.
>
> The hunk that Kevin quoted ensures that we restart bs_snapshots
> iteration - this prevents us from choosing a device that has no medium
> inserted (also see bdrv_can_snapshot() which checks for an inserted
> medium).
>
> This behavior is preserved in this patch because we now always restart
> iteration and it's no longer necessary to reset global iterator state.
>
> But I don't see any code that forbids/skips inserted, read-write
> removable devices in the orginal code.  Please point out the specific
> piece of code you think has been dropped.
>
> Stefan
>
   OK, original code make sure following code is executed before
savm_vm(), after this patch following code will be always executed
before save_vm(), nothing need to be added.

     while ((bs = bdrv_next(bs))) {
         if (bdrv_can_snapshot(bs)) {
             bs_snapshots = bs;
             return bs;
         }
     }



-- 
Best Regards

Wenchao Xia