qapi/transaction.json | 2 + include/block/dirty-bitmap.h | 2 + block/dirty-bitmap.c | 26 +++++++++ blockdev.c | 100 +++++++++++++++++++++++++++-------- tests/qemu-iotests/254 | 30 ++++++++++- tests/qemu-iotests/254.out | 82 ++++++++++++++++++++++++++++ 6 files changed, 219 insertions(+), 23 deletions(-)
Hi all!
Here is block-dirty-bitmap-remove transaction action.
It is used to do transactional movement of the bitmap (which is
possible in conjunction with merge command). Transactional bitmap
movement is needed in scenarios with external snapshot, when we don't
want to leave copy of the bitmap in the base image.
Implementation itself in 03, in short:
.prepare: make bitmap unnamed and non-persistent, delete stored version
of the bitmap from the image
.commit: release bitmap
.abort: restore bitmap name and persistence. We don't restore bitmap
version in the image. It's not critical, we have in-RAM version,
it will be stored on shutdown
Vladimir Sementsov-Ogievskiy (4):
blockdev: reduce aio_context locked sections in bitmap add/remove
block/dirty-bitmap: add hide/unhide API
qapi: implement block-dirty-bitmap-remove transaction action
iotests: test bitmap moving inside 254
qapi/transaction.json | 2 +
include/block/dirty-bitmap.h | 2 +
block/dirty-bitmap.c | 26 +++++++++
blockdev.c | 100 +++++++++++++++++++++++++++--------
tests/qemu-iotests/254 | 30 ++++++++++-
tests/qemu-iotests/254.out | 82 ++++++++++++++++++++++++++++
6 files changed, 219 insertions(+), 23 deletions(-)
--
2.18.0
On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote: > Hi all! > > Here is block-dirty-bitmap-remove transaction action. > > It is used to do transactional movement of the bitmap (which is > possible in conjunction with merge command). Transactional bitmap > movement is needed in scenarios with external snapshot, when we don't > want to leave copy of the bitmap in the base image. > Oh, interesting. I see why you want this now. OK, let's do it. > Implementation itself in 03, in short: > > .prepare: make bitmap unnamed and non-persistent, delete stored version > of the bitmap from the image > > .commit: release bitmap > > .abort: restore bitmap name and persistence. We don't restore bitmap > version in the image. It's not critical, we have in-RAM version, > it will be stored on shutdown > > Vladimir Sementsov-Ogievskiy (4): > blockdev: reduce aio_context locked sections in bitmap add/remove > block/dirty-bitmap: add hide/unhide API > qapi: implement block-dirty-bitmap-remove transaction action > iotests: test bitmap moving inside 254 > > qapi/transaction.json | 2 + > include/block/dirty-bitmap.h | 2 + > block/dirty-bitmap.c | 26 +++++++++ > blockdev.c | 100 +++++++++++++++++++++++++++-------- > tests/qemu-iotests/254 | 30 ++++++++++- > tests/qemu-iotests/254.out | 82 ++++++++++++++++++++++++++++ > 6 files changed, 219 insertions(+), 23 deletions(-) >
08.06.2019 1:26, John Snow wrote: > > > On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote: >> Hi all! >> >> Here is block-dirty-bitmap-remove transaction action. >> >> It is used to do transactional movement of the bitmap (which is >> possible in conjunction with merge command). Transactional bitmap >> movement is needed in scenarios with external snapshot, when we don't >> want to leave copy of the bitmap in the base image. >> > > Oh, interesting. I see why you want this now. OK, let's do it. > Hi John! Hmm, could you stage it, or should I fix something? Seems I've answered all questions. We need this for our nearest release and wanting to avoid x-vz- prefixes in the API, I'd be very grateful if we merge it soon. -- Best regards, Vladimir
Am 17.06.2019 um 13:37 hat Vladimir Sementsov-Ogievskiy geschrieben: > 08.06.2019 1:26, John Snow wrote: > > > > > > On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote: > >> Hi all! > >> > >> Here is block-dirty-bitmap-remove transaction action. > >> > >> It is used to do transactional movement of the bitmap (which is > >> possible in conjunction with merge command). Transactional bitmap > >> movement is needed in scenarios with external snapshot, when we don't > >> want to leave copy of the bitmap in the base image. > >> > > > > Oh, interesting. I see why you want this now. OK, let's do it. > > > > > Hi John! > > Hmm, could you stage it, or should I fix something? Seems I've answered all questions. > We need this for our nearest release and wanting to avoid x-vz- prefixes in the API, > I'd be very grateful if we merge it soon. I hope you won't have to do this, but in any case x-vz- isn't the right prefix. Please read section '6. Downstream extension of QMP' in docs/interop/qmp-spec.txt before adding your own extensions. According to the spec, your prefix would be something like __com.virtuozzo-... Kevin
17.06.2019 19:03, Kevin Wolf wrote: > Am 17.06.2019 um 13:37 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 08.06.2019 1:26, John Snow wrote: >>> >>> >>> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote: >>>> Hi all! >>>> >>>> Here is block-dirty-bitmap-remove transaction action. >>>> >>>> It is used to do transactional movement of the bitmap (which is >>>> possible in conjunction with merge command). Transactional bitmap >>>> movement is needed in scenarios with external snapshot, when we don't >>>> want to leave copy of the bitmap in the base image. >>>> >>> >>> Oh, interesting. I see why you want this now. OK, let's do it. >>> >> >> >> Hi John! >> >> Hmm, could you stage it, or should I fix something? Seems I've answered all questions. >> We need this for our nearest release and wanting to avoid x-vz- prefixes in the API, >> I'd be very grateful if we merge it soon. > > I hope you won't have to do this, but in any case x-vz- isn't the right > prefix. Please read section '6. Downstream extension of QMP' in > docs/interop/qmp-spec.txt before adding your own extensions. > > According to the spec, your prefix would be something like > __com.virtuozzo-... > Thanks for pointing to that, I thought about this some time ago when saw Red Hat prefixes.. Still x-vz- is a lot better than nothing and most probably will not intersect with future things. However, we'll move to correct prefixes of course. -- Best regards, Vladimir
Am 18.06.2019 um 09:31 hat Vladimir Sementsov-Ogievskiy geschrieben: > 17.06.2019 19:03, Kevin Wolf wrote: > > Am 17.06.2019 um 13:37 hat Vladimir Sementsov-Ogievskiy geschrieben: > >> 08.06.2019 1:26, John Snow wrote: > >>> > >>> > >>> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote: > >>>> Hi all! > >>>> > >>>> Here is block-dirty-bitmap-remove transaction action. > >>>> > >>>> It is used to do transactional movement of the bitmap (which is > >>>> possible in conjunction with merge command). Transactional bitmap > >>>> movement is needed in scenarios with external snapshot, when we don't > >>>> want to leave copy of the bitmap in the base image. > >>>> > >>> > >>> Oh, interesting. I see why you want this now. OK, let's do it. > >>> > >> > >> > >> Hi John! > >> > >> Hmm, could you stage it, or should I fix something? Seems I've answered all questions. > >> We need this for our nearest release and wanting to avoid x-vz- prefixes in the API, > >> I'd be very grateful if we merge it soon. > > > > I hope you won't have to do this, but in any case x-vz- isn't the right > > prefix. Please read section '6. Downstream extension of QMP' in > > docs/interop/qmp-spec.txt before adding your own extensions. > > > > According to the spec, your prefix would be something like > > __com.virtuozzo-... > > > > Thanks for pointing to that, I thought about this some time ago when saw Red Hat prefixes.. > Still x-vz- is a lot better than nothing and most probably will not intersect with future > things. However, we'll move to correct prefixes of course. Yes, I agree that x-vz- is unlikely to cause any trouble in practice, it's just out-of-spec strictly speaking. So for anything new that you introduce, it would be better to follow the spec. Kevin
On 6/17/19 7:37 AM, Vladimir Sementsov-Ogievskiy wrote: > 08.06.2019 1:26, John Snow wrote: >> >> >> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote: >>> Hi all! >>> >>> Here is block-dirty-bitmap-remove transaction action. >>> >>> It is used to do transactional movement of the bitmap (which is >>> possible in conjunction with merge command). Transactional bitmap >>> movement is needed in scenarios with external snapshot, when we don't >>> want to leave copy of the bitmap in the base image. >>> >> >> Oh, interesting. I see why you want this now. OK, let's do it. >> > > > Hi John! > > Hmm, could you stage it, or should I fix something? Seems I've answered all questions. > We need this for our nearest release and wanting to avoid x-vz- prefixes in the API, > I'd be very grateful if we merge it soon. > > Hi Vladimir, Sorry, I lost track of this thread. (In the future, if you have something pressing like a release and I don't seem to have noticed an email, try sending me an email directly without the word "patch" in the subject it to get my attention, or come ping me on IRC.) For this series: I think this is pretty confusing, as evidenced by how we both misunderstood what it did. So "block_dirty_bitmap_remove" now removes a bitmap from storage but might not actually release it. That's a little surprising, but I see why we want it. So what really happens is: 1. Remove a bitmap from storage, but actually don't release it; then 2. hide the bitmap we're holding onto (holding the persistence and name data aside) 3. On success, we release the bitmap. During this process, taking the bitmap's name away and marking it as non-persistent helps keep it from getting rewritten to disk or from having anything else interact with it. On Failure: 1. unhide: - Restore the persistent bit - Restore the name So we restore the persistent bit, but we don't actually go back through the trouble of making sure that there isn't a collision on the name. I suppose we are guaranteed this won't happen because if a bitmap got added, it MUST have been added AFTER this action, and if we are aborting, it MUST have been removed before we get here. .... phew, I think that this works, but isn't obvious that it does. However, if we ever use "hide" or "unhide" outside of the remove API, you might actually run into troubles where we collide on the name when you "unhide" it, and I don't like that very much. I feel like a "hidden" bitmap probably should still occupy namespace to avoid this problem. But then, it isn't truly hidden from API queries and such, and subsequent commands could interact with it... we could add a new permissions flag, but that starts to get kind of messy. Is this a problem of naming? do we need to "hide" bitmaps? Could we get away with something simpler? We could rename the "migration" bool to be something generic and then use that. This way, it keeps its persistence flag and name, but it won't get flushed back out to disk or anything at the conclusion of the transaction. This avoids the need for worrying about name collision on "unhide", and doesn't need any new fields. During this time, we can also mark the bitmap as BUSY which will prevent it from being used for any operations. The ones that we could use during this critical window are: - BACKUP - ADD - CLEAR - ENABLE - DISABLE - MERGE - REMOVE Backup, clear, enable, disable, and remove will all fail a BUSY check. Merge will fail for either the source or the destination being BUSY. So I think that it's possible to avoid the need for a hide() API right now, just by using the migration bool (renamed) and the busy status. I want to be very aware of your time constraints though, so I prepared a mock-up on top of your patches; https://github.com/jnsnow/qemu/commit/9b3434cc86bbd1cbd86f9fc845435d8d6883c205 If this seems agreeable to you I'll re-send and stage right away tomorrow. If you really DO want hide() semantics we can work on those instead. --js
© 2016 - 2024 Red Hat, Inc.