Re: [Qemu-devel] [PATCH 09/25] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap

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

02.06.2017 11:56, Vladimir Sementsov-Ogievskiy wrote:
> 02.06.2017 02:25, John Snow wrote:
>>
>> On 06/01/2017 03:30 AM, Sementsov-Ogievskiy Vladimir wrote:
>>> Hi John!
>>>
>>> Look at our discussion about this in v18 thread.
>>>> Shortly: readonly is not the same as disabled. disabled= bitmap just
>>> ignores all writes. readonly= writes are not allowed at all.
>>>
>>> And I think, I'll try to go through way 2: "dirty" field instead of
>>> "readonly" (look at v18 discussion), as it a bit more flexible.
>>>
>> Not sure which I prefer...
>>
>> Method 1 is attractive in that it is fairly simple, and enforces fairly
>> loudly the inability to write to devices with RO bitmaps. It's a natural
>> extension of your current approach.
>
> For now I decided to realize this one, I think I'll publish it today. 
> Also, I'm going to rename s/readonly/in_use - to avoid the confuse 
> with disabled. So let this field just be mirror of IN_USE in the image 
> and just say "persistent storage knows, that bitmap is in use and may 
> be dirty".
>
> Also, optimization with 'dirty' flag may be added later.

And, also, I don't want to influence this "first write", on which we 
will set "IN_USE" in all bitmaps (for way (2). Reopening rw is less 
performance-demanding place than write.


>
>>
>> Method 2 is attractive in that it seems a little more efficient, and is
>> a little more clever. A dirty flag lets us avoid flushing bitmaps we
>> never even changed (though we still need to clean up the in_use flags.)
>>
>> What I wonder about #2 is what happens when a write sneaks in (due to a
>> bug or a use case we didn't see) on a bitmap attached to a read-only
>> node. We fail later on invalidate? It shouldn't happen in normal
>> circumstances, but I worry that the failure mode is messier.
>
> if bitmap is dirty - all ok, the problems will appear when we'll try 
> to save it, but these problems are not fatal - bitmap should be marked 
> 'in_use' in the image, so it will be lost (the worst case is when 
> in_use not set and bitmap is incorrect - it may lead to data loss for 
> user)
>
> if it is not dirty - we will fail to write 'in_use' before actual 
> write and the whole write will fail.
>
>>
>>
>> Well, either way I will be happy for now I think -- pick whichever
>> option feels easiest or best for you to implement.
>>
>> Thanks!
>>
>>> On 01.06.2017 02:48, John Snow wrote:
>>>> On 05/30/2017 04:17 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> It will be needed in following commits for persistent bitmaps.
>>>>> If bitmap is loaded from read-only storage (and we can't mark it
>>>>> "in use" in this storage) corresponding BdrvDirtyBitmap should be
>>>>> read-only.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>>>> <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>    block/dirty-bitmap.c         | 28 ++++++++++++++++++++++++++++
>>>>>    block/io.c                   |  8 ++++++++
>>>>>    blockdev.c                   |  6 ++++++
>>>>>    include/block/dirty-bitmap.h |  4 ++++
>>>>>    4 files changed, 46 insertions(+)
>>>>>
>>>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>>>> index 90af37287f..733f19ca5e 100644
>>>>> --- a/block/dirty-bitmap.c
>>>>> +++ b/block/dirty-bitmap.c
>>>>> @@ -44,6 +44,8 @@ struct BdrvDirtyBitmap {
>>>>>        int64_t size;               /* Size of the bitmap (Number of
>>>>> sectors) */
>>>>>        bool disabled;              /* Bitmap is read-only */
>>>>>        int active_iterators;       /* How many iterators are 
>>>>> active */
>>>>> +    bool readonly;              /* Bitmap is read-only and may be
>>>>> changed only
>>>>> +                                   by deserialize* functions */
>>>>>        QLIST_ENTRY(BdrvDirtyBitmap) list;
>>>>>    };
>>>>>    @@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap
>>>>> *bitmap,
>>>>>                               int64_t cur_sector, int64_t nr_sectors)
>>>>>    {
>>>>>        assert(bdrv_dirty_bitmap_enabled(bitmap));
>>>>> +    assert(!bdrv_dirty_bitmap_readonly(bitmap));
>>>> Not reasonable to add the condition for !readonly into
>>>> bdrv_dirty_bitmap_enabled?
>>>>
>>>> As is:
>>>>
>>>> If readonly is set to true on a bitmap, bdrv_dirty_bitmap_status is
>>>> going to return ACTIVE for such bitmaps, but DISABLED might be more
>>>> appropriate to indicate the read-only nature.
>>>>
>>>> If you add this condition into _enabled(), you can skip the extra
>>>> assertions you've added here.
>>>>
>>>>> hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
>>>>>    }
>>>>>    @@ -443,12 +446,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap
>>>>> *bitmap,
>>>>>                                 int64_t cur_sector, int64_t 
>>>>> nr_sectors)
>>>>>    {
>>>>>        assert(bdrv_dirty_bitmap_enabled(bitmap));
>>>>> +    assert(!bdrv_dirty_bitmap_readonly(bitmap));
>>>>>        hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
>>>>>    }
>>>>>      void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap 
>>>>> **out)
>>>>>    {
>>>>>        assert(bdrv_dirty_bitmap_enabled(bitmap));
>>>>> +    assert(!bdrv_dirty_bitmap_readonly(bitmap));
>>>>>        if (!out) {
>>>>>            hbitmap_reset_all(bitmap->bitmap);
>>>>>        } else {
>>>>> @@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t
>>>>> cur_sector,
>>>>>            if (!bdrv_dirty_bitmap_enabled(bitmap)) {
>>>>>                continue;
>>>>>            }
>>>>> +        assert(!bdrv_dirty_bitmap_readonly(bitmap));
>>>>>            hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
>>>>>        }
>>>>>    }
>>>>> @@ -540,3 +546,25 @@ int64_t
>>>>> bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
>>>>>    {
>>>>>        return hbitmap_count(bitmap->meta);
>>>>>    }
>>>>> +
>>>>> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap)
>>>>> +{
>>>>> +    return bitmap->readonly;
>>>>> +}
>>>>> +
>>>>> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap)
>>>>> +{
>>>>> +    bitmap->readonly = true;
>>>>> +}
>>>>> +
>>>>> +bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
>>>>> +{
>>>>> +    BdrvDirtyBitmap *bm;
>>>>> +    QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
>>>>> +        if (bm->readonly) {
>>>>> +            return true;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    return false;
>>>>> +}
>>>>> diff --git a/block/io.c b/block/io.c
>>>>> index fdd7485c22..0e28a1f595 100644
>>>>> --- a/block/io.c
>>>>> +++ b/block/io.c
>>>>> @@ -1349,6 +1349,10 @@ static int coroutine_fn
>>>>> bdrv_aligned_pwritev(BdrvChild *child,
>>>>>        uint64_t bytes_remaining = bytes;
>>>>>        int max_transfer;
>>>>>    +    if (bdrv_has_readonly_bitmaps(bs)) {
>>>>> +        return -EPERM;
>>>>> +    }
>>>>> +
>>>> Oh, hardcore. The original design for "disabled" was just that they
>>>> would become invalid after writes; but in this case a read-only bitmap
>>>> literally enforces the concept.
>>>>
>>>> I can envision usages for both.
>>>>
>>>>>        assert(is_power_of_2(align));
>>>>>        assert((offset & (align - 1)) == 0);
>>>>>        assert((bytes & (align - 1)) == 0);
>>>>> @@ -2437,6 +2441,10 @@ int coroutine_fn
>>>>> bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset,
>>>>>            return -ENOMEDIUM;
>>>>>        }
>>>>>    +    if (bdrv_has_readonly_bitmaps(bs)) {
>>>>> +        return -EPERM;
>>>>> +    }
>>>>> +
>>>>>        ret = bdrv_check_byte_request(bs, offset, count);
>>>>>        if (ret < 0) {
>>>>>            return ret;
>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>> index c63f4e82c7..2b397abf66 100644
>>>>> --- a/blockdev.c
>>>>> +++ b/blockdev.c
>>>>> @@ -2033,6 +2033,9 @@ static void
>>>>> block_dirty_bitmap_clear_prepare(BlkActionState *common,
>>>>>        } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
>>>>>            error_setg(errp, "Cannot clear a disabled bitmap");
>>>>>            return;
>>>>> +    } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) {
>>>>> +        error_setg(errp, "Cannot clear a readonly bitmap");
>>>>> +        return;
>>>>>        }
>>>> Oh, I see -- perhaps you wanted a better error message? That makes 
>>>> sense
>>>> to me....
>>>>
>>>>
>>>> ...Though, you know, bdrv_disable_dirty_bitmap accomplishes something
>>>> pretty similar here: we take the bitmap out of active RW state and put
>>>> it into RO mode, it's just done under the name enabled/disabled 
>>>> instead
>>>> of RO.
>>>>
>>>> As of this patch, nothing uses it yet. Is this patch necessary? Would
>>>> setting a bitmap as "disabled" to mean "RO" be sufficient, or are the
>>>> two flags truly semantically necessary?
>>>>
>>>>> bdrv_clear_dirty_bitmap(state->bitmap, &state->backup);
>>>>> @@ -2813,6 +2816,9 @@ void qmp_block_dirty_bitmap_clear(const char
>>>>> *node, const char *name,
>>>>>                       "Bitmap '%s' is currently disabled and 
>>>>> cannot be
>>>>> cleared",
>>>>>                       name);
>>>>>            goto out;
>>>>> +    } else if (bdrv_dirty_bitmap_readonly(bitmap)) {
>>>>> +        error_setg(errp, "Bitmap '%s' is readonly and cannot be
>>>>> cleared", name);
>>>>> +        goto out;
>>>>>        }
>>>>>          bdrv_clear_dirty_bitmap(bitmap, NULL);
>>>>> diff --git a/include/block/dirty-bitmap.h 
>>>>> b/include/block/dirty-bitmap.h
>>>>> index 1e17729ac2..c0c3ce9f85 100644
>>>>> --- a/include/block/dirty-bitmap.h
>>>>> +++ b/include/block/dirty-bitmap.h
>>>>> @@ -75,4 +75,8 @@ void
>>>>> bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
>>>>>                                            bool finish);
>>>>>    void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap 
>>>>> *bitmap);
>>>>>    +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>>>>> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap);
>>>>> +bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>>>>> +
>>>>>    #endif
>>>>>
>>>> I realize I am being very bike-sheddy about this, so I might give 
>>>> an r-b
>>>> with a light justification.
>>>>
>>>> --js
>
>

-- 
Best regards,
Vladimir