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

[Stefan Hajnoczi]

On Tue, Nov 22, 2011 at 11:16 AM, supriya kannery <address@hidden> wrote:
> 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 <address@hidden>
>>>> wrote:
>>>>
>>>>>
>>>>> Stefan Hajnoczi wrote:
>>>>>
>>>>>>
>>>>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>>>>>> <address@hidden> 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.

VMDK must call bdrv_reopen_prepare()/bdrv_reopen_commit()/bdrv_reopen_abort()
on its child images in order to support aborting when there is a
failure half-way through.  If it used bdrv_reopen() on its child
images then it could not roll back later when there is a failure on
the next child.

My bigger picture comment was that safe reopen support for raw-posix
is great but we should be able to take advantage of that for image
formats.  I'd rather see all image formats except VMDK have safe
reopen in this series than only raw-posix and vmdk.  How about the
generic prepare/commit/abort implementation that Kevin suggested in a
previous thread - something that qcow2, qed, etc can use in order to
get the safe reopen ability?

(If we don't get safe reopen support for qcow2, qed, etc then dynamic
hostcache changing will take the unsafe reopen path in some of the
common usecases with image file.)

Stefan