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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

13.06.2017 18:28, Max Reitz wrote:
> 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 <vsementsov@virtuozzo.com>
>>>> ---
>>>>    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...

So, firstly, reopen r-o again and then reopen r-w? Is it possible?

>
> Max
>

-- 
Best regards,
Vladimir