Re: [Qemu-devel] [v2 Patch 1/9]block: Framework for reopening image files safely

[Jeff Cody <jcody@redhat.com>]

On 08/09/2012 05:20 AM, Kevin Wolf wrote:
> Am 09.08.2012 06:26, schrieb Jeff Cody:
>> On 07/30/2012 05:34 PM, Supriya Kannery wrote:
>>> Struct BDRVReopenState along with three reopen related functions
>>> introduced for handling reopening of images safely. This can be
>>> extended by each of the block drivers to reopen respective
>>> image files.
>>>
>>> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
>>>
>>> ---
>>> Index: qemu/block.c
>>> ===================================================================
>>> --- qemu.orig/block.c
>>> +++ qemu/block.c
>>> @@ -859,6 +859,60 @@ unlink_and_fail:
>>>      return ret;
>>>  }
>>>  
>>> +int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags)
>>> +{
>>> +     BlockDriver *drv = bs->drv;
>>> +
>>> +     return drv->bdrv_reopen_prepare(bs, prs, flags);
>>> +}
>>> +
>>> +void bdrv_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
>>> +{
>>> +    BlockDriver *drv = bs->drv;
>>> +
>>> +    drv->bdrv_reopen_commit(bs, rs);
>>> +}
>>> +
>>> +void bdrv_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
>>> +{
>>> +    BlockDriver *drv = bs->drv;
>>> +
>>> +    drv->bdrv_reopen_abort(bs, rs);
>>> +}
>>> +
>>> +void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
>>> +{
>>> +    BlockDriver *drv = bs->drv;
>>> +    int ret = 0;
>>> +    BDRVReopenState *reopen_state = NULL;
>>> +
>>
>> We should assert if drv is NULL:
>>
>>     assert(drv != NULL);
>>
>>
>>> +    /* Quiesce IO for the given block device */
>>> +    bdrv_drain_all();
>>> +    ret = bdrv_flush(bs);
>>> +    if (ret != 0) {
>>> +        error_set(errp, QERR_IO_ERROR);
>>> +        return;
>>> +    }
>>> +
>>
>> We also need to reopen bs->file, so maybe something like this:
>>
>>     /* open any file images */
>>     if (bs->file) {
>>         bdrv_reopen(bs->file, bdrv_flags, errp);
>>         if (errp && *errp) {
>>             goto exit;
>>         }
>>     }
>>
>> This will necessitate making the handlers in the raw.c file just stubs
>> (I'll respond to that patch as well).
> 
> Doesn't this break the transactional semantics? I think you should only
> prepare the bs->file reopen here and commit it when committing this one.
> 

Yes, it would, so if everything stayed as it is now, that would be the
right way to do it... but one thing that would be nice (I also mention
this in my comments on patch 0/9) is that it become transactional for
multiple BlockDriverStates to be reopened.  That would obviously change
the interface a bit, so that multiple BDS entries could be queued, but
then it shouldn't be any different to queue the bs->file as well as the
bs.

All prepare() stages would happen on queued items, and only
progress to commit() if all prepare() stages passed, as you mentioned.

One other thing, and perhaps this is nit-picking some - but the
prepare() functions all modify the 'live' structures, after backing them
up into stashes.  So, on abort, the structures are restored from the
stashes, and on commit the stashes are discarded.  Conceptually, it
seems cleaner to this the opposite way - prepare() does it work into
temporary stashes, and the commit() then copies the data over from the
stash to the live structure, and abort() would just discard the stashes.