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

[John Snow]

On 05/31/2017 11:53 AM, Max Reitz wrote:
> On 2017-05-31 17:05, Vladimir Sementsov-Ogievskiy wrote:
>> 31.05.2017 17:44, Max Reitz wrote:
>>> On 2017-05-31 16:29, Vladimir Sementsov-Ogievskiy wrote:
>>>> 31.05.2017 16:43, Max Reitz wrote:
>>>>> On 2017-05-30 08:50, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Thank you for this scenario. Hmm.
>>>>>>
>>>>>> So, as I need guarantee that image and bitmap are unchanged,
>>>>>> bdrv_set_dirty should return error and fail the whole write. Ok?
>>>>> I don't know. That would mean that you couldn't commit to an image that
>>>>> has a persistent auto-loading bitmap, which doesn't seem very nice
>>>>> to me.
>>>>>
>>>>> I'm not quite sure what to do myself. So first I'd definitely want the
>>>>> commit operation to succeed. That means we'd have to automatically make
>>>>> the bitmap non-readonly once we write to it. The "readonly" flag would
>>>>> then be an "unchanged" flag, rather, to signify that the bitmap has not
>>>>> been changed since it was loaded, which means that it does not need to
>>>>> be written back to the image file.
>>>>>
>>>>> Now the issue remains that if you modify a persistent bitmap that is
>>>>> stored in an image file that is opened RO when it's closed, you
>>>>> won't be
>>>>> able to write the modifications back.
>>>>>
>>>>> So in addition, I guess we'd need to "flush" all persistent bitmaps
>>>>> (that is, write all modifications back to the file and set the
>>>>> "unchanged" flag (you could also call it "dirty" and then mean the
>>>>> opposite) for each bitmap) not only when the image is closed or
>>>>> invalidated, but also when it is reopened read-only.
>>>>>
>>>>> (block-commit reopens the backing BDS R/W, then writes to them, thus
>>>>> modifying the dirty bitmaps, and finally reopens the BDS as read-only;
>>>>> before that happens, we will have to flush the modified bitmap data.)
>>>> Ok, understand.
>>>>
>>>> We need to consider also setting in_use flag in the image. We _must not_
>>>> write to image with dirty bitmap,
>>>> if in_use flag of this dirty bitmap is not set, as in case of something
>>>> fail we will have image with wrong bitmap with
>>>> unset in_use flag (which looks ok).
>>> Right.
>>>
>>>> I see two ways to handle it:
>>>>
>>>> variant 1:
>>>> 1. readonly field stays as is (see v19, with normal errors, not only
>>>> asserts)
>>>> 2. immediately after reopening r/w we do "reopening bitmaps r/w", i.e.
>>>> set in_use in the image and set BdrvDirtyBitmap.readonly = false
>>>> 3. in reopen_prepare, if reopening r-o do "reopening bitmaps r-o", i.e.
>>>> save them into the image and set BdrvDirtyBitmap.readonly = true
>>> Sounds good, yes.
>>>
>>>> variant 2:
>>>> 1. instead of 'readonly' add 'dirty' field, set dirty to 0 for all
>>>> bitmaps on create
>>>> 2. before write/discard check this field in all related bitmaps, and if
>>>> dirty=0 (and persistent=1), write IN_USE flag into the image first, set
>>>> dirty=1, and only then do write. (if writing IN_USE=1 failed, fail the
>>>> whole write)
>>>> 3. in reopen_prepare, if reopening r-o do "reopening bitmaps r-o", i.e.
>>>> save them into the image and set BdrvDirtyBitmap.dirty = 0
>>> Works, too.
>>>
>>> I think the second variant would the more "efficient" way (because you
>>> only have to flush out dirty dirty bitmaps), but the first one would be
>>> simpler and has the great advantage of not requiring a write to the
>>> image file when you just want to set a bit in the in-memory dirty
>>> bitmap. So I'd personally go for the first variant.
>>
>> Hmm, why not requiring? Both 1 and 2 do write in_use=1, but (1) do this
>> on open/reopen, and (2) before the first write to the image.
> 
> Oh, I didn't read the "before write/discard". Yes, if you check it
> before writing, then you won't have to set the flag through
> bdrv_set_dirty().
> 
>> "set a bit in the in-memory..." - are you saying about not-persistent
>> dirty bitmaps? In this case, of course, nothing should be written into
>> the image, just set dirty=1.
> 
> No, I did mean persistent bitmaps, but bdrv_set_dirty() just sets the
> bit in main memory, of course. It only gets written to the image later
> (on reopen/close/invalidate).
> 

There may be some benefit to setting in_use immediately as soon as we
admit that we are willing to tolerate writes to the bitmap. It's a
performance hit, but it may help on-disk consistency.

Or maybe that's a fool's errand? This is a design question we've largely
ignored so far, but it's something that will need investigating sooner
or later.

> Well, your choice. I think both will work. :-)
> 
> Max
>