Re: [Qemu-block] [PATCH v20 13/30] block: new bdrv_reopen_bitmaps_rw interface

[Max Reitz]

On 2017-06-13 12:25, Vladimir Sementsov-Ogievskiy wrote:
> 09.06.2017 16:27, Max Reitz wrote:
>> On 2017-06-02 13:21, Vladimir Sementsov-Ogievskiy wrote:
>>> Add format driver handler, which should mark loaded read-only
>>> bitmaps as 'IN_USE' in the image and unset read_only field in
>>> corresponding BdrvDirtyBitmap's.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> ---
>>>   block.c                   | 17 +++++++++++++++++
>>>   include/block/block_int.h |  7 +++++++
>>>   2 files changed, 24 insertions(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index 04af7697dc..161db9e32a 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -2946,12 +2946,16 @@ void bdrv_reopen_commit(BDRVReopenState
>>> *reopen_state)
>>>   {
>>>       BlockDriver *drv;
>>>       BlockDriverState *bs;
>>> +    bool old_can_write, new_can_write;
>>>         assert(reopen_state != NULL);
>>>       bs = reopen_state->bs;
>>>       drv = bs->drv;
>>>       assert(drv != NULL);
>>>   +    old_can_write =
>>> +        !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) &
>>> BDRV_O_INACTIVE);
>>> +
>>>       /* If there are any driver level actions to take */
>>>       if (drv->bdrv_reopen_commit) {
>>>           drv->bdrv_reopen_commit(reopen_state);
>>> @@ -2965,6 +2969,19 @@ void bdrv_reopen_commit(BDRVReopenState
>>> *reopen_state)
>>>       bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
>>>         bdrv_refresh_limits(bs, NULL);
>>> +
>>> +    new_can_write =
>>> +        !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) &
>>> BDRV_O_INACTIVE);
>>> +    if (!old_can_write && new_can_write &&
>>> drv->bdrv_reopen_bitmaps_rw) {
>>> +        Error *local_err = NULL;
>>> +        if (drv->bdrv_reopen_bitmaps_rw(bs, &local_err) < 0) {
>>> +            /* This is not fatal, bitmaps just left read-only, so
>>> all following
>>> +             * writes will fail. User can remove read-only bitmaps
>>> to unblock
>>> +             * writes.
>>> +             */
>> In a sense, it pretty much is fatal. We were asked to make the image
>> non-read-only but we failed because it effectively still is read-only.
>>
>> But I can't think of anything better, and you're right, removing the
>> bitmaps would resolve the situation. This would require the user to know
>> that updating the bitmaps was the issue, and local_err may not actually
>> reflect that.
>>
>>> +            error_report_err(local_err);
>> So I'd prepend this with something like "$node_name: Failed to make
>> dirty bitmaps writable", and maybe append a hint like "Removing all
>> persistent dirty bitmaps from this node will allow writing to it".
> 
> Ok for prepending, but I don't want to add last note, as for the user it
> may better to retry an operation, leading reopening image rw..

Which operation do you mean? The reopening? Because that operation
already "succeeded" at this point, so you can't retry it...

Max

signature.asc
Description: OpenPGP digital signature