[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v3 7/7] block/dirty-bitmaps: implement inconsist
From: |
John Snow |
Subject: |
Re: [Qemu-block] [PATCH v3 7/7] block/dirty-bitmaps: implement inconsistent bit |
Date: |
Wed, 6 Mar 2019 16:46:33 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
On 3/6/19 9:26 AM, Vladimir Sementsov-Ogievskiy wrote:
> 01.03.2019 22:15, John Snow wrote:
>> Set the inconsistent bit on load instead of rejecting such bitmaps.
>> There is no way to un-set it; the only option is to delete it.
>
> I think, s/delete it/delete the bitmap/, as "it" is the bit after "un-set it".
>
You are absolutely correct.
Don't let anyone fool you into thinking that Americans know English.
>>
>> Obvervations:
>> - bitmap loading does not need to update the header for in_use bitmaps.
>> - inconsistent bitmaps don't need to have their data loaded; they're
>> glorified corruption sentinels.
>> - bitmap saving does not need to save inconsistent bitmaps back to disk.
>> - bitmap reopening DOES need to drop the readonly flag from inconsistent
>> bitmaps to allow reopening of qcow2 files with non-qemu-owned bitmaps
>> being eventually flushed back to disk.
>
> hmm, and inconsitent bitmaps don't have this flag, isn't it?
>
I didn't make any effort to treat them differently -- the readonly flag
only gets applied when the image is actually read only. I felt it was
conceptually simpler that way.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>> block/qcow2-bitmap.c | 103 ++++++++++++++++++++++---------------------
>> 1 file changed, 53 insertions(+), 50 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 3ee524da4b..c3b210ede1 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>
> [..]
>
>> @@ -962,35 +963,39 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs,
>> Error **errp)
>> }
>>
>> QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> - if (!(bm->flags & BME_FLAG_IN_USE)) {
>> - BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
>> - if (bitmap == NULL) {
>> - goto fail;
>> - }
>> -
>> - if (!(bm->flags & BME_FLAG_AUTO)) {
>> - bdrv_disable_dirty_bitmap(bitmap);
>> - }
>> - bdrv_dirty_bitmap_set_persistance(bitmap, true);
>> - bm->flags |= BME_FLAG_IN_USE;
>> - created_dirty_bitmaps =
>> - g_slist_append(created_dirty_bitmaps, bitmap);
>> + BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
>> + if (bitmap == NULL) {
>> + goto fail;
>> }
>> - }
>>
>> - if (created_dirty_bitmaps != NULL) {
>> - if (can_write(bs)) {
>> - /* in_use flags must be updated */
>> - int ret = update_ext_header_and_dir_in_place(bs, bm_list);
>> - if (ret < 0) {
>> - error_setg_errno(errp, -ret, "Can't update bitmap
>> directory");
>> - goto fail;
>> - }
>> - header_updated = true;
>> + if (bm->flags & BME_FLAG_IN_USE) {
>> + bdrv_dirty_bitmap_set_inconsistent(bitmap);
>> } else {
>> - g_slist_foreach(created_dirty_bitmaps, set_readonly_helper,
>> - (gpointer)true);
>> + /* NB: updated flags only get written if can_write(bs) is true.
>> */
>> + bm->flags |= BME_FLAG_IN_USE;
>> + needs_update = true;
>> }
>> + if (!(bm->flags & BME_FLAG_AUTO)) {
>> + bdrv_disable_dirty_bitmap(bitmap);
>> + }
>> + bdrv_dirty_bitmap_set_persistance(bitmap, true);
>> + created_dirty_bitmaps =
>> + g_slist_append(created_dirty_bitmaps, bitmap);
>> + }
>> +
>> + if (needs_update && can_write(bs)) {
>> + /* in_use flags must be updated */
>> + int ret = update_ext_header_and_dir_in_place(bs, bm_list);
>> + if (ret < 0) {
>> + error_setg_errno(errp, -ret, "Can't update bitmap directory");
>> + goto fail;
>> + }
>> + header_updated = true;
>> + }
>> +
>> + if (!can_write(bs)) {
>> + g_slist_foreach(created_dirty_bitmaps, set_readonly_helper,
>> + (gpointer)true);
>
> hmmm..
>
> I think, inconsistent bitmaps should have readonly flag always set or always
> unset, independently on can_write,
> as we are not going to write something related to inconsistent bitmaps.
>
We'll never change metadata for inconsistent bitmaps, no. We might
delete them which causes a metadata change, though -- and right now the
readonly check stops that in blockdev.c before we attempt (and fail) the
operation. It does have at least a light usage.
It's not a crucial feature, but I think the error message is nicer.
> And I think, better is always unset, to have inconsistent bitmaps as
> something unrelated to readonly.
>
The way I think of it is as unrelated, which is why I set readonly and
inconsistent orthogonally.
> Readonly are bitmaps, which are not marked IN_USE on open for some reason..
>
I tend to think of readonly bitmaps as persistent bitmaps attached to
storage we are unable to write back to, so we must prevent their
modification.
I suppose the IN_USE viewpoint works too; "This is a bitmap that we have
not marked as IN_USE so it must not be used."
> I understand, that inconsistent bitmaps are some kind of readonly too, as we
> can't write to them..
> But we can't read too anyway, and we don't interfere with reopen logic at
> all. So again, better is
> don't mark inconsistent bitmaps as readonly.
>
Hm, so in your model; we just NEVER set readonly for persistent bitmaps,
which incurs some changes at load time, but allows the reload_rw
function to go completely unmodified.
That's not unreasonable in terms of SLOC, but semantically I am not sure
which approach is better. I'm leaning towards keeping this as written
for now, but... well. Any thoughts, Eric? Would you like to flip a coin?
--js
- Re: [Qemu-block] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source, (continued)
- Re: [Qemu-block] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source, John Snow, 2019/03/01
- Re: [Qemu-block] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source, Eric Blake, 2019/03/01
- Re: [Qemu-block] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source, John Snow, 2019/03/01
- Re: [Qemu-block] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source, Vladimir Sementsov-Ogievskiy, 2019/03/06
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source, John Snow, 2019/03/06
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source, Eric Blake, 2019/03/06
Re: [Qemu-block] [PATCH v3 6/7] block/dirty-bitmaps: disallow busy bitmaps as merge source, Vladimir Sementsov-Ogievskiy, 2019/03/06
[Qemu-block] [PATCH v3 7/7] block/dirty-bitmaps: implement inconsistent bit, John Snow, 2019/03/01
Re: [Qemu-block] [PATCH v3 0/7] bitmaps: add inconsistent bit, John Snow, 2019/03/05