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). Well, your choice. I think both will work. :-) Max
signature.asc
Description: OpenPGP digital signature
