Patchwork [3/3] block: change rollback sequence in qmp_transaction

login
register
mail settings
Submitter Wayne Xia
Date April 1, 2013, 10:01 a.m.
Message ID <1364810491-21404-4-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/232721/
State New
Headers show

Comments

Wayne Xia - April 1, 2013, 10:01 a.m.
Last operaton should be cancelled first.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Eric Blake - April 1, 2013, 3:52 p.m.
On 04/01/2013 04:01 AM, Wenchao Xia wrote:
>   Last operaton should be cancelled first.

s/operaton/operation/

[I don't care enough about US vs. UK to say whether canceled or
cancelled looks better]

> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  blockdev.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 75416fb..a24d10e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -954,7 +954,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
>          dev_entry = dev_entry->next;
>  
>          states = g_malloc0(sizeof(BlkTransactionStates));
> -        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
> +        QSIMPLEQ_INSERT_HEAD(&snap_bdrv_states, states, entry);

Is this a bug fix that for something that can be triggered by existing
use of the 'transaction' command even without the additions you made in
patches 1 and 2?  If so, this probably ought to come first in the
series, and you probably ought to consider enhancing the testsuite to
show why it matters.
Wayne Xia - April 2, 2013, 2:34 a.m.
于 2013-4-1 23:52, Eric Blake 写道:
> On 04/01/2013 04:01 AM, Wenchao Xia wrote:
>>    Last operaton should be cancelled first.
>
> s/operaton/operation/
>
> [I don't care enough about US vs. UK to say whether canceled or
> cancelled looks better]
>
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   blockdev.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 75416fb..a24d10e 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -954,7 +954,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
>>           dev_entry = dev_entry->next;
>>
>>           states = g_malloc0(sizeof(BlkTransactionStates));
>> -        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
>> +        QSIMPLEQ_INSERT_HEAD(&snap_bdrv_states, states, entry);
>
> Is this a bug fix that for something that can be triggered by existing
> use of the 'transaction' command even without the additions you made in
> patches 1 and 2?  If so, this probably ought to come first in the
> series, and you probably ought to consider enhancing the testsuite to
> show why it matters.
>
   Originally it only support backing chain, it will have no
difference about the sequence to delete it, but matters if
other types of operation are added.
Kevin Wolf - April 2, 2013, 1:59 p.m.
Am 01.04.2013 um 12:01 hat Wenchao Xia geschrieben:
>   Last operaton should be cancelled first.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Should it? This commit message does little to convince me.

Kevin
Wayne Xia - April 3, 2013, 5:35 a.m.
于 2013-4-2 21:59, Kevin Wolf 写道:
> Am 01.04.2013 um 12:01 hat Wenchao Xia geschrieben:
>>    Last operaton should be cancelled first.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>
> Should it? This commit message does little to convince me.
>
> Kevin
>
   I think so, if two request are "snapshot_blkdev ide0 newimage.qcow2",
"snapshot_blkdev_internal ide0 snap0", then in rollback delete of
snap0 would be wrong.
Kevin Wolf - April 3, 2013, 9:03 a.m.
Am 03.04.2013 um 07:35 hat Wenchao Xia geschrieben:
> 于 2013-4-2 21:59, Kevin Wolf 写道:
> >Am 01.04.2013 um 12:01 hat Wenchao Xia geschrieben:
> >>   Last operaton should be cancelled first.
> >>
> >>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >
> >Should it? This commit message does little to convince me.
> >
> >Kevin
> >
>   I think so, if two request are "snapshot_blkdev ide0 newimage.qcow2",
> "snapshot_blkdev_internal ide0 snap0", then in rollback delete of
> snap0 would be wrong.

I think this problem exists only because you didn't properly separate
the prepare and the commit stage. If any changes only took effect in a
non-failing commit stage, then you don't ever have to delete a snapshot.
Instead, you just abort the creation of the snapshot.

Kevin
Wayne Xia - April 3, 2013, 10:35 a.m.
于 2013-4-3 17:03, Kevin Wolf 写道:
> Am 03.04.2013 um 07:35 hat Wenchao Xia geschrieben:
>> 于 2013-4-2 21:59, Kevin Wolf 写道:
>>> Am 01.04.2013 um 12:01 hat Wenchao Xia geschrieben:
>>>>    Last operaton should be cancelled first.
>>>>
>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>
>>> Should it? This commit message does little to convince me.
>>>
>>> Kevin
>>>
>>    I think so, if two request are "snapshot_blkdev ide0 newimage.qcow2",
>> "snapshot_blkdev_internal ide0 snap0", then in rollback delete of
>> snap0 would be wrong.
>
> I think this problem exists only because you didn't properly separate
> the prepare and the commit stage. If any changes only took effect in a
> non-failing commit stage, then you don't ever have to delete a snapshot.
> Instead, you just abort the creation of the snapshot.
>
> Kevin
>
   OK, I got your point, will split bdrv_snapshot_create() before adding
it in qmp_transaction.

Patch

diff --git a/blockdev.c b/blockdev.c
index 75416fb..a24d10e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -954,7 +954,7 @@  void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
         dev_entry = dev_entry->next;
 
         states = g_malloc0(sizeof(BlkTransactionStates));
-        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
+        QSIMPLEQ_INSERT_HEAD(&snap_bdrv_states, states, entry);
 
         states->action = dev_info;
         switch (dev_info->kind) {