| Message ID | 1369451385-23452-2-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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> I am not totally convinced by this approach, especially when it's nowhere documented that the order of BDSes in the list is important. However, I think the current code is suboptimal as well, so I'll apply this for now. What we really want is this: savevm lets you choose which image to save the VM state to, and if you don't specify one, it automatically picks one like today. loadvm checks all images and loads the VM state from the image that has the VM state for this snapshot. If loadvm finds that it's not exactly one image that has a VM state, this is an error condition. Is anyone interested in implementing this? Kevin
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? Kevin
于 2013-5-27 23:18, Kevin Wolf 写道: > 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> > > I am not totally convinced by this approach, especially when it's nowhere > documented that the order of BDSes in the list is important. However, I > think the current code is suboptimal as well, so I'll apply this for > now. > > What we really want is this: savevm lets you choose which image to save > the VM state to, and if you don't specify one, it automatically picks > one like today. loadvm checks all images and loads the VM state from the > image that has the VM state for this snapshot. If loadvm finds that it's > not exactly one image that has a VM state, this is an error condition. > > Is anyone interested in implementing this? > > Kevin > I think that can came up after Pavel's savevm transaction, which add parameter telling which image to save vmstate. This patch simply keep what it is now not touching that part.
于 2013-5-27 23:25, Kevin Wolf 写道: > 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? How about adding it back in find_vmstate_bs()? > > Kevin >
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
于 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()?
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
于 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; } }
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; - } } void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv, @@ -3381,24 +3371,6 @@ int bdrv_is_snapshot(BlockDriverState *bs) return !!(bs->open_flags & BDRV_O_SNAPSHOT); } -BlockDriverState *bdrv_snapshots(void) -{ - BlockDriverState *bs; - - if (bs_snapshots) { - return bs_snapshots; - } - - bs = NULL; - while ((bs = bdrv_next(bs))) { - if (bdrv_can_snapshot(bs)) { - bs_snapshots = bs; - return bs; - } - } - return NULL; -} - int bdrv_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) { diff --git a/include/block/block.h b/include/block/block.h index 1251c5c..38263b9 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -332,7 +332,6 @@ BlockInfo *bdrv_query_info(BlockDriverState *s); BlockStats *bdrv_query_stats(const BlockDriverState *bs); int bdrv_can_snapshot(BlockDriverState *bs); int bdrv_is_snapshot(BlockDriverState *bs); -BlockDriverState *bdrv_snapshots(void); int bdrv_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info); int bdrv_snapshot_goto(BlockDriverState *bs, diff --git a/savevm.c b/savevm.c index 31dcce9..baa1a09 100644 --- a/savevm.c +++ b/savevm.c @@ -2262,6 +2262,17 @@ out: return ret; } +static BlockDriverState *find_vmstate_bs(void) +{ + BlockDriverState *bs = NULL; + while ((bs = bdrv_next(bs))) { + if (bdrv_can_snapshot(bs)) { + return bs; + } + } + return NULL; +} + static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, const char *name) { @@ -2338,7 +2349,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) } } - bs = bdrv_snapshots(); + bs = find_vmstate_bs(); if (!bs) { monitor_printf(mon, "No block device can accept snapshots\n"); return; @@ -2440,7 +2451,7 @@ int load_vmstate(const char *name) QEMUFile *f; int ret; - bs_vm_state = bdrv_snapshots(); + bs_vm_state = find_vmstate_bs(); if (!bs_vm_state) { error_report("No block device supports snapshots"); return -ENOTSUP; @@ -2519,7 +2530,7 @@ void do_delvm(Monitor *mon, const QDict *qdict) int ret; const char *name = qdict_get_str(qdict, "name"); - bs = bdrv_snapshots(); + bs = find_vmstate_bs(); if (!bs) { monitor_printf(mon, "No block device supports snapshots\n"); return; @@ -2551,7 +2562,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) int *available_snapshots; char buf[256]; - bs = bdrv_snapshots(); + bs = find_vmstate_bs(); if (!bs) { monitor_printf(mon, "No available block device supports snapshots\n"); return;