Re: [Qemu-devel] [v9 Patch 5/6]Qemu: Framework for reopening images safely

[supriya kannery <supriyak@in.ibm.com>]

Kevin Wolf wrote:
> Am 22.11.2011 11:24, schrieb supriya kannery:
>   
>> Stefan Hajnoczi wrote:
>>     
>>> On Mon, Nov 21, 2011 at 12:13 PM, supriya kannery <supriyak@in.ibm.com> wrote:
>>>   
>>>       
>>>> Stefan Hajnoczi wrote:
>>>>     
>>>>         
>>>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>>>>> <supriyak@linux.vnet.ibm.com> wrote:
>>>>>
>>>>>       
>>>>>           
>>>>>> @@ -708,17 +731,31 @@ int bdrv_reopen(BlockDriverState *bs, in
>>>>>>        qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name);
>>>>>>        return ret;
>>>>>>    }
>>>>>> -    open_flags = bs->open_flags;
>>>>>> -    bdrv_close(bs);
>>>>>>
>>>>>> -    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
>>>>>> -    if (ret < 0) {
>>>>>> -        /* Reopen failed. Try to open with original flags */
>>>>>> -        qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
>>>>>> -        ret = bdrv_open(bs, bs->filename, open_flags, drv);
>>>>>> +    /* Use driver specific reopen() if available */
>>>>>> +    if (drv->bdrv_reopen_prepare) {
>>>>>>
>>>>>>         
>>>>>>             
>>>>> This seems weird to me because we're saying a driver may have
>>>>> drv->bdrv_reopen_prepare == NULL but the public bdrv_reopen_prepare()
>>>>> function doesn't check and return -ENOTSUP.
>>>>>
>>>>>       
>>>>>           
>>>> If drv->bdrv_reopen_prepare == NULL , then we are not calling the
>>>> publick bdrv_reopen_prepare at all. Unless we later call  public
>>>> bdrv_reopen_prepare
>>>> from elsewhere without checking drv->bdrv_reopen_prepare,  a check for
>>>> -ENOTSUP inside the public one is not needed right?
>>>>
>>>> Also, we are handling reopening for even those drivers which don't
>>>> have its own bdrv_reopen_prepare defined, by taking the "else"
>>>> control path. So condition for reporting "ENOTSUP" shouldn't come
>>>> up as of now. Please let me know your thoughts.
>>>>     
>>>>         
>>> How does VMDK implement its prepare/commit/abort?  It needs to use the
>>> "public" bdrv_reopen_prepare() function on its image files.
>>>   
>>>       
>> bdrv_reopen() is the public interface which gets called by any of the 
>> image formats.
>> So VMDK or any image format has to call bdrv_reopen which decides to call
>> driver specific prepare/commit/abort or simply close and reopen the file.
>>     
>
> No, that doesn't work. In order to get all-or-nothing semantics, you
> need to explicitly prepare all child images and only when you know the
> results of all preparations, you can decide whether to commit or abort all.
>   
bdrv_reopen_prepare/commit/abort will be implemented specific to VMDK in 
vmdk.c. Then for vmdk,
drv->bdrv_reopen_prepare() will handle  preparing child images and 
return success to bdrv_reopen ()
only if all of them get prepared successfully.  The prepare/commit/abort 
concept we took up considering
vmdk's special case of multiple files.

So it is bdrv_reopen() which is public and called by hostcache change 
request for any of the image formats.
It then routes the processing to respective prepare/commit/abort 
implemented by the drivers, including VMDK.
In cases where drivers don't have their own implementation, default 
route is taken which is simply
closing and opening the file.

- thanks, Supriya
> Kevin
>
>