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

[John Snow <jsnow@redhat.com>]

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.

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.


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
>