[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: |
Fri, 8 Mar 2019 13:46:44 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
On 3/7/19 11:37 AM, Eric Blake wrote:
> On 3/6/19 3:46 PM, John Snow wrote:
>
>>> 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 like that wording (keeping read-only and inconsistent orthogonal),
>
>>
>> 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."
>>
>
> Yes, it does work, but it is a bit more of a stretch, so I'm not quite
> as sure about using it.
>
>>> 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?
>
> No strong preferences, other than let's get it in before soft freeze, to
> make sure we don't miss out on it entirely due to waiting for the coin
> flip :)
>
>
OK. I have a slight preference for keeping the flag meanings orthogonal
and logically consistent, because I believe it keeps the code less
surprising to anyone who isn't Eric, Vlad, or myself.
So I think I will stage this as-is, and if it poses a problem I will
accept counter-proposals during freeze to shore it up if necessary.
--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, 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