| Message ID | 1475232808-4852-13-git-send-email-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: > This flag means that the bitmap is now in use by the software or was not > successfully saved. In any way, with this flag set the bitmap data must > be considered inconsistent and should not be loaded. > > With current implementation this flag is never set, as we just remove > bitmaps from the image after loading. But it defined in qcow2 spec and > must be handled. Also, it can be used in future, if async schemes of > bitmap loading/saving are implemented. > > We also remove in-use bitmaps from the image on open. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2-bitmap.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) Don't you want to make use of this flag? It would appear useful to me if you just marked autoload bitmaps as in_use instead of deleting them from the image when it's opened and then overwrite them when the image is closed. > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index a5be25a..8cf40f0 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -28,6 +28,7 @@ > #include "qemu/osdep.h" > #include "qapi/error.h" > #include "qemu/cutils.h" > +#include "exec/log.h" > > #include "block/block_int.h" > #include "block/qcow2.h" > @@ -44,7 +45,8 @@ > #define BME_MAX_NAME_SIZE 1023 > > /* Bitmap directory entry flags */ > -#define BME_RESERVED_FLAGS 0xfffffffd > +#define BME_RESERVED_FLAGS 0xfffffffc > +#define BME_FLAG_IN_USE 1 This should be (1U << 0) to be consistent with the definition of BME_FLAG_AUTO. > #define BME_FLAG_AUTO (1U << 1) > > /* bits [1, 8] U [56, 63] are reserved */ > @@ -287,6 +289,11 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs, > BdrvDirtyBitmap *bitmap = NULL; > char *name = g_strndup(dir_entry_name_notcstr(entry), entry->name_size); > > + if (entry->flags & BME_FLAG_IN_USE) { > + error_setg(errp, "Bitmap '%s' is in use", name); > + goto fail; > + } > + > ret = bitmap_table_load(bs, entry, &bitmap_table); > if (ret < 0) { > error_setg_errno(errp, -ret, > @@ -533,6 +540,14 @@ int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp) > for_each_bitmap_dir_entry(e, dir, dir_size) { > uint32_t fl = e->flags; > > + if (fl & BME_FLAG_IN_USE) { > + qemu_log("qcow2 warning: " > + "removing in_use bitmap '%.*s' for image %s.\n", > + e->name_size, (char *)(e + 1), bdrv_get_device_or_node_name(bs)); I'm not sure whether qemu_log() is the right way to do this. I think fprintf() to stderr might actually be more appropriate. qemu_log() looks like it's mostly used for debugging purposes. Also, this is not a warning. You are just doing it. You are not warning someone about a cliff when he's already fallen off. But you can actually make it a warning: Just leave the bitmap in the image (maybe someone can do something with it?) and instead let qemu-img check clean it up. The warning should then just be "Warning: Ignoring in_use bitmap %.*s, use qemu-img check -r all to delete it". Max > + > + continue; > + } > + > if (fl & BME_FLAG_AUTO) { > BdrvDirtyBitmap *bitmap = load_bitmap(bs, e, errp); > if (bitmap == NULL) { >
07.10.2016 22:44, Max Reitz пишет: > On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >> This flag means that the bitmap is now in use by the software or was not >> successfully saved. In any way, with this flag set the bitmap data must >> be considered inconsistent and should not be loaded. >> >> With current implementation this flag is never set, as we just remove >> bitmaps from the image after loading. But it defined in qcow2 spec and >> must be handled. Also, it can be used in future, if async schemes of >> bitmap loading/saving are implemented. >> >> We also remove in-use bitmaps from the image on open. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/qcow2-bitmap.c | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) > Don't you want to make use of this flag? It would appear useful to me if > you just marked autoload bitmaps as in_use instead of deleting them from > the image when it's opened and then overwrite them when the image is closed. And what is the use of it? Any way, storing an invalid copy of online data is bad I think. Therefore I will have to free bitmap data clusters and zero bitmap table in file. > >> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >> index a5be25a..8cf40f0 100644 >> --- a/block/qcow2-bitmap.c >> +++ b/block/qcow2-bitmap.c >> @@ -28,6 +28,7 @@ >> #include "qemu/osdep.h" >> #include "qapi/error.h" >> #include "qemu/cutils.h" >> +#include "exec/log.h" >> >> #include "block/block_int.h" >> #include "block/qcow2.h" >> @@ -44,7 +45,8 @@ >> #define BME_MAX_NAME_SIZE 1023 >> >> /* Bitmap directory entry flags */ >> -#define BME_RESERVED_FLAGS 0xfffffffd >> +#define BME_RESERVED_FLAGS 0xfffffffc >> +#define BME_FLAG_IN_USE 1 > This should be (1U << 0) to be consistent with the definition of > BME_FLAG_AUTO. > >> #define BME_FLAG_AUTO (1U << 1) >> >> /* bits [1, 8] U [56, 63] are reserved */ >> @@ -287,6 +289,11 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs, >> BdrvDirtyBitmap *bitmap = NULL; >> char *name = g_strndup(dir_entry_name_notcstr(entry), entry->name_size); >> >> + if (entry->flags & BME_FLAG_IN_USE) { >> + error_setg(errp, "Bitmap '%s' is in use", name); >> + goto fail; >> + } >> + >> ret = bitmap_table_load(bs, entry, &bitmap_table); >> if (ret < 0) { >> error_setg_errno(errp, -ret, >> @@ -533,6 +540,14 @@ int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp) >> for_each_bitmap_dir_entry(e, dir, dir_size) { >> uint32_t fl = e->flags; >> >> + if (fl & BME_FLAG_IN_USE) { >> + qemu_log("qcow2 warning: " >> + "removing in_use bitmap '%.*s' for image %s.\n", >> + e->name_size, (char *)(e + 1), bdrv_get_device_or_node_name(bs)); > I'm not sure whether qemu_log() is the right way to do this. I think > fprintf() to stderr might actually be more appropriate. qemu_log() looks > like it's mostly used for debugging purposes. > > Also, this is not a warning. You are just doing it. You are not warning > someone about a cliff when he's already fallen off. > > But you can actually make it a warning: Just leave the bitmap in the > image (maybe someone can do something with it?) and instead let qemu-img > check clean it up. The warning should then just be "Warning: Ignoring > in_use bitmap %.*s, use qemu-img check -r all to delete it". > > Max > >> + >> + continue; >> + } >> + >> if (fl & BME_FLAG_AUTO) { >> BdrvDirtyBitmap *bitmap = load_bitmap(bs, e, errp); >> if (bitmap == NULL) { >> >
On 21.10.2016 17:34, Vladimir Sementsov-Ogievskiy wrote: > 07.10.2016 22:44, Max Reitz пишет: >> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>> This flag means that the bitmap is now in use by the software or was not >>> successfully saved. In any way, with this flag set the bitmap data must >>> be considered inconsistent and should not be loaded. >>> >>> With current implementation this flag is never set, as we just remove >>> bitmaps from the image after loading. But it defined in qcow2 spec and >>> must be handled. Also, it can be used in future, if async schemes of >>> bitmap loading/saving are implemented. >>> >>> We also remove in-use bitmaps from the image on open. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> block/qcow2-bitmap.c | 17 ++++++++++++++++- >>> 1 file changed, 16 insertions(+), 1 deletion(-) >> Don't you want to make use of this flag? It would appear useful to me if >> you just marked autoload bitmaps as in_use instead of deleting them from >> the image when it's opened and then overwrite them when the image is >> closed. > > And what is the use of it? You don't need to free any bitmaps when opening the file, and you don't need to allocate any new bitmap space when closing it. > Any way, storing an invalid copy of online data is bad I think. > Therefore I will have to free bitmap data clusters and zero bitmap table > in file. Why can't you just set this flag to mark it invalid? In case qemu crashes, in one case you'll have some invalid bitmaps and in the other you won't have any of them at all. I don't think there's a difference from that perspective. Max
21.10.2016 22:58, Max Reitz пишет: > On 21.10.2016 17:34, Vladimir Sementsov-Ogievskiy wrote: >> 07.10.2016 22:44, Max Reitz пишет: >>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>>> This flag means that the bitmap is now in use by the software or was not >>>> successfully saved. In any way, with this flag set the bitmap data must >>>> be considered inconsistent and should not be loaded. >>>> >>>> With current implementation this flag is never set, as we just remove >>>> bitmaps from the image after loading. But it defined in qcow2 spec and >>>> must be handled. Also, it can be used in future, if async schemes of >>>> bitmap loading/saving are implemented. >>>> >>>> We also remove in-use bitmaps from the image on open. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> block/qcow2-bitmap.c | 17 ++++++++++++++++- >>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>> Don't you want to make use of this flag? It would appear useful to me if >>> you just marked autoload bitmaps as in_use instead of deleting them from >>> the image when it's opened and then overwrite them when the image is >>> closed. >> And what is the use of it? > You don't need to free any bitmaps when opening the file, and you don't > need to allocate any new bitmap space when closing it. As bitmaps are sparce in file, I need to allocate new space when closing. Or free it... > >> Any way, storing an invalid copy of online data is bad I think. >> Therefore I will have to free bitmap data clusters and zero bitmap table >> in file. > Why can't you just set this flag to mark it invalid? In case qemu > crashes, in one case you'll have some invalid bitmaps and in the other > you won't have any of them at all. I don't think there's a difference > from that perspective. > > Max >
24.10.2016 13:32, Vladimir Sementsov-Ogievskiy пишет: > 21.10.2016 22:58, Max Reitz пишет: >> On 21.10.2016 17:34, Vladimir Sementsov-Ogievskiy wrote: >>> 07.10.2016 22:44, Max Reitz пишет: >>>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>>>> This flag means that the bitmap is now in use by the software or >>>>> was not >>>>> successfully saved. In any way, with this flag set the bitmap data >>>>> must >>>>> be considered inconsistent and should not be loaded. >>>>> >>>>> With current implementation this flag is never set, as we just remove >>>>> bitmaps from the image after loading. But it defined in qcow2 spec >>>>> and >>>>> must be handled. Also, it can be used in future, if async schemes of >>>>> bitmap loading/saving are implemented. >>>>> >>>>> We also remove in-use bitmaps from the image on open. >>>>> >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>> <vsementsov@virtuozzo.com> >>>>> --- >>>>> block/qcow2-bitmap.c | 17 ++++++++++++++++- >>>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>>> Don't you want to make use of this flag? It would appear useful to >>>> me if >>>> you just marked autoload bitmaps as in_use instead of deleting them >>>> from >>>> the image when it's opened and then overwrite them when the image is >>>> closed. >>> And what is the use of it? >> You don't need to free any bitmaps when opening the file, and you don't >> need to allocate any new bitmap space when closing it. > > As bitmaps are sparce in file, I need to allocate new space when > closing. Or free it... Is it a real problem to reallocate space in qcow2? If so, to minimaze allocations, I will have to make a list of clusters of in_use bitmaps on close, and then save current bitmaps to these clusters (and allocated some additional clusters, or free extra clusters). Also, if I don't free them on open, I'll have to free them on remove of bdrv dirty bitmap.. > >> >>> Any way, storing an invalid copy of online data is bad I think. >>> Therefore I will have to free bitmap data clusters and zero bitmap >>> table >>> in file. >> Why can't you just set this flag to mark it invalid? In case qemu >> crashes, in one case you'll have some invalid bitmaps and in the other >> you won't have any of them at all. I don't think there's a difference >> from that perspective. >> >> Max >> > >
On 24.10.2016 12:32, Vladimir Sementsov-Ogievskiy wrote: > 21.10.2016 22:58, Max Reitz пишет: >> On 21.10.2016 17:34, Vladimir Sementsov-Ogievskiy wrote: >>> 07.10.2016 22:44, Max Reitz пишет: >>>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>>>> This flag means that the bitmap is now in use by the software or >>>>> was not >>>>> successfully saved. In any way, with this flag set the bitmap data >>>>> must >>>>> be considered inconsistent and should not be loaded. >>>>> >>>>> With current implementation this flag is never set, as we just remove >>>>> bitmaps from the image after loading. But it defined in qcow2 spec and >>>>> must be handled. Also, it can be used in future, if async schemes of >>>>> bitmap loading/saving are implemented. >>>>> >>>>> We also remove in-use bitmaps from the image on open. >>>>> >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>>> --- >>>>> block/qcow2-bitmap.c | 17 ++++++++++++++++- >>>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>>> Don't you want to make use of this flag? It would appear useful to >>>> me if >>>> you just marked autoload bitmaps as in_use instead of deleting them >>>> from >>>> the image when it's opened and then overwrite them when the image is >>>> closed. >>> And what is the use of it? >> You don't need to free any bitmaps when opening the file, and you don't >> need to allocate any new bitmap space when closing it. > > As bitmaps are sparce in file, I need to allocate new space when > closing. Or free it... May happen. But not necessarily, and it will probably still save time as you can reuse existing allocations and don't have to free everything. Max
On 24.10.2016 13:35, Vladimir Sementsov-Ogievskiy wrote: > 24.10.2016 13:32, Vladimir Sementsov-Ogievskiy пишет: >> 21.10.2016 22:58, Max Reitz пишет: >>> On 21.10.2016 17:34, Vladimir Sementsov-Ogievskiy wrote: >>>> 07.10.2016 22:44, Max Reitz пишет: >>>>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>>>>> This flag means that the bitmap is now in use by the software or >>>>>> was not >>>>>> successfully saved. In any way, with this flag set the bitmap data >>>>>> must >>>>>> be considered inconsistent and should not be loaded. >>>>>> >>>>>> With current implementation this flag is never set, as we just remove >>>>>> bitmaps from the image after loading. But it defined in qcow2 spec >>>>>> and >>>>>> must be handled. Also, it can be used in future, if async schemes of >>>>>> bitmap loading/saving are implemented. >>>>>> >>>>>> We also remove in-use bitmaps from the image on open. >>>>>> >>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>>> <vsementsov@virtuozzo.com> >>>>>> --- >>>>>> block/qcow2-bitmap.c | 17 ++++++++++++++++- >>>>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>>>> Don't you want to make use of this flag? It would appear useful to >>>>> me if >>>>> you just marked autoload bitmaps as in_use instead of deleting them >>>>> from >>>>> the image when it's opened and then overwrite them when the image is >>>>> closed. >>>> And what is the use of it? >>> You don't need to free any bitmaps when opening the file, and you don't >>> need to allocate any new bitmap space when closing it. >> >> As bitmaps are sparce in file, I need to allocate new space when >> closing. Or free it... > > > Is it a real problem to reallocate space in qcow2? If so, to minimaze > allocations, I will have to make a list of clusters of in_use bitmaps on > close, and then save current bitmaps to these clusters (and allocated > some additional clusters, or free extra clusters). It's not a real problem, but it does take time, and I maintain that it's time it doesn't need to take because you can just use the in_use flag. I wouldn't worry about reusing clusters of other bitmaps. Of course we can do that later on in some optimization but not now. I just mean the basic case of some auto-loaded bitmap that is not being deleted while the VM is running and is just saved back to the image file once it is closed. I don't expect that users will always consume all of the auto-loaded bitmaps while the VM is active... > Also, if I don't free them on open, I'll have to free them on remove of > bdrv dirty bitmap.. Yes, so? That takes time, but that is something the user will probably expect. I wouldn't expect opening of the file to take that time. Overall, it doesn't matter time-wise whether you free the bitmap data when opening the image file or when the dirty bitmap is actually deleted by the user or when the image file is closed. Just setting the single in_use flag for all of the auto-loaded bitmaps will basically not take any time. On the other hand, as soon as just a single auto-loaded bitmap survives a VM (or qemu-img) invocation, you will very, very likely safe at least some time because writing the bitmap to the disk can reuse at least some of the existing clusters. Max
On 24.10.2016 19:08, Max Reitz wrote: > On 24.10.2016 13:35, Vladimir Sementsov-Ogievskiy wrote: >> 24.10.2016 13:32, Vladimir Sementsov-Ogievskiy пишет: >>> 21.10.2016 22:58, Max Reitz пишет: >>>> On 21.10.2016 17:34, Vladimir Sementsov-Ogievskiy wrote: >>>>> 07.10.2016 22:44, Max Reitz пишет: >>>>>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>>>>>> This flag means that the bitmap is now in use by the software or >>>>>>> was not >>>>>>> successfully saved. In any way, with this flag set the bitmap data >>>>>>> must >>>>>>> be considered inconsistent and should not be loaded. >>>>>>> >>>>>>> With current implementation this flag is never set, as we just remove >>>>>>> bitmaps from the image after loading. But it defined in qcow2 spec >>>>>>> and >>>>>>> must be handled. Also, it can be used in future, if async schemes of >>>>>>> bitmap loading/saving are implemented. >>>>>>> >>>>>>> We also remove in-use bitmaps from the image on open. >>>>>>> >>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>>>> <vsementsov@virtuozzo.com> >>>>>>> --- >>>>>>> block/qcow2-bitmap.c | 17 ++++++++++++++++- >>>>>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>>>>> Don't you want to make use of this flag? It would appear useful to >>>>>> me if >>>>>> you just marked autoload bitmaps as in_use instead of deleting them >>>>>> from >>>>>> the image when it's opened and then overwrite them when the image is >>>>>> closed. >>>>> And what is the use of it? >>>> You don't need to free any bitmaps when opening the file, and you don't >>>> need to allocate any new bitmap space when closing it. >>> >>> As bitmaps are sparce in file, I need to allocate new space when >>> closing. Or free it... >> >> >> Is it a real problem to reallocate space in qcow2? If so, to minimaze >> allocations, I will have to make a list of clusters of in_use bitmaps on >> close, and then save current bitmaps to these clusters (and allocated >> some additional clusters, or free extra clusters). > > It's not a real problem, but it does take time, and I maintain that it's > time it doesn't need to take because you can just use the in_use flag. > > I wouldn't worry about reusing clusters of other bitmaps. Of course we > can do that later on in some optimization but not now. > > I just mean the basic case of some auto-loaded bitmap that is not being > deleted while the VM is running and is just saved back to the image file > once it is closed. I don't expect that users will always consume all of > the auto-loaded bitmaps while the VM is active... > >> Also, if I don't free them on open, I'll have to free them on remove of >> bdrv dirty bitmap.. > > Yes, so? That takes time, but that is something the user will probably > expect. I wouldn't expect opening of the file to take that time. > > Overall, it doesn't matter time-wise whether you free the bitmap data > when opening the image file or when the dirty bitmap is actually deleted > by the user or when the image file is closed. Just setting the single > in_use flag for all of the auto-loaded bitmaps will basically not take > any time. > > On the other hand, as soon as just a single auto-loaded bitmap survives > a VM (or qemu-img) invocation, you will very, very likely safe at least > some time because writing the bitmap to the disk can reuse at least some > of the existing clusters. By the way, dealing with removal of bitmaps when they are deleted by the user is also positive when it comes to migration or read-only disks that are later reopened R/W: Currently, as far as I can see, you just keep the auto-load bitmaps in the image if the image is part of an incoming migration or opened read-only, but then you continue under the assumption that they are removed from the image. That's not very good. If you delete the bitmaps only when the user asks you to, then you can just return an error that the bitmap cannot be removed at this time because the image file is read-only or used in migration (and I don't think the latter can even happen when it's the user who has requested removal of the bitmap). Max
24.10.2016 20:18, Max Reitz wrote: > On 24.10.2016 19:08, Max Reitz wrote: >> On 24.10.2016 13:35, Vladimir Sementsov-Ogievskiy wrote: >>> 24.10.2016 13:32, Vladimir Sementsov-Ogievskiy пишет: >>>> 21.10.2016 22:58, Max Reitz пишет: >>>>> On 21.10.2016 17:34, Vladimir Sementsov-Ogievskiy wrote: >>>>>> 07.10.2016 22:44, Max Reitz пишет: >>>>>>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>>>>>>> This flag means that the bitmap is now in use by the software or >>>>>>>> was not >>>>>>>> successfully saved. In any way, with this flag set the bitmap data >>>>>>>> must >>>>>>>> be considered inconsistent and should not be loaded. >>>>>>>> >>>>>>>> With current implementation this flag is never set, as we just remove >>>>>>>> bitmaps from the image after loading. But it defined in qcow2 spec >>>>>>>> and >>>>>>>> must be handled. Also, it can be used in future, if async schemes of >>>>>>>> bitmap loading/saving are implemented. >>>>>>>> >>>>>>>> We also remove in-use bitmaps from the image on open. >>>>>>>> >>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>>>>> <vsementsov@virtuozzo.com> >>>>>>>> --- >>>>>>>> block/qcow2-bitmap.c | 17 ++++++++++++++++- >>>>>>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>>>>>> Don't you want to make use of this flag? It would appear useful to >>>>>>> me if >>>>>>> you just marked autoload bitmaps as in_use instead of deleting them >>>>>>> from >>>>>>> the image when it's opened and then overwrite them when the image is >>>>>>> closed. >>>>>> And what is the use of it? >>>>> You don't need to free any bitmaps when opening the file, and you don't >>>>> need to allocate any new bitmap space when closing it. >>>> As bitmaps are sparce in file, I need to allocate new space when >>>> closing. Or free it... >>> >>> Is it a real problem to reallocate space in qcow2? If so, to minimaze >>> allocations, I will have to make a list of clusters of in_use bitmaps on >>> close, and then save current bitmaps to these clusters (and allocated >>> some additional clusters, or free extra clusters). >> It's not a real problem, but it does take time, and I maintain that it's >> time it doesn't need to take because you can just use the in_use flag. >> >> I wouldn't worry about reusing clusters of other bitmaps. Of course we >> can do that later on in some optimization but not now. >> >> I just mean the basic case of some auto-loaded bitmap that is not being >> deleted while the VM is running and is just saved back to the image file >> once it is closed. I don't expect that users will always consume all of >> the auto-loaded bitmaps while the VM is active... >> >>> Also, if I don't free them on open, I'll have to free them on remove of >>> bdrv dirty bitmap.. >> Yes, so? That takes time, but that is something the user will probably >> expect. I wouldn't expect opening of the file to take that time. >> >> Overall, it doesn't matter time-wise whether you free the bitmap data >> when opening the image file or when the dirty bitmap is actually deleted >> by the user or when the image file is closed. Just setting the single >> in_use flag for all of the auto-loaded bitmaps will basically not take >> any time. >> >> On the other hand, as soon as just a single auto-loaded bitmap survives >> a VM (or qemu-img) invocation, you will very, very likely safe at least >> some time because writing the bitmap to the disk can reuse at least some >> of the existing clusters. > By the way, dealing with removal of bitmaps when they are deleted by the > user is also positive when it comes to migration or read-only disks that > are later reopened R/W: Currently, as far as I can see, you just keep > the auto-load bitmaps in the image if the image is part of an incoming > migration or opened read-only, but then you continue under the > assumption that they are removed from the image. That's not very good. You are right, I need to handle reopening more carefully. In my way, I need to delete bitmaps when reopening R/W and in yours - set in_use bit. > > If you delete the bitmaps only when the user asks you to, then you can > just return an error that the bitmap cannot be removed at this time > because the image file is read-only or used in migration (and I don't > think the latter can even happen when it's the user who has requested > removal of the bitmap). > > Max > Ok, I'll rewrite this way, using 'in_use' bit and trying to avoid allocate/free overhead.
25.10.2016 13:53, Vladimir Sementsov-Ogievskiy wrote: > 24.10.2016 20:18, Max Reitz wrote: >> On 24.10.2016 19:08, Max Reitz wrote: >>> On 24.10.2016 13:35, Vladimir Sementsov-Ogievskiy wrote: >>>> 24.10.2016 13:32, Vladimir Sementsov-Ogievskiy пишет: >>>>> 21.10.2016 22:58, Max Reitz пишет: >>>>>> On 21.10.2016 17:34, Vladimir Sementsov-Ogievskiy wrote: >>>>>>> 07.10.2016 22:44, Max Reitz пишет: >>>>>>>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>>>>>>>> This flag means that the bitmap is now in use by the software or >>>>>>>>> was not >>>>>>>>> successfully saved. In any way, with this flag set the bitmap >>>>>>>>> data >>>>>>>>> must >>>>>>>>> be considered inconsistent and should not be loaded. >>>>>>>>> >>>>>>>>> With current implementation this flag is never set, as we just >>>>>>>>> remove >>>>>>>>> bitmaps from the image after loading. But it defined in qcow2 >>>>>>>>> spec >>>>>>>>> and >>>>>>>>> must be handled. Also, it can be used in future, if async >>>>>>>>> schemes of >>>>>>>>> bitmap loading/saving are implemented. >>>>>>>>> >>>>>>>>> We also remove in-use bitmaps from the image on open. >>>>>>>>> >>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>>>>>> <vsementsov@virtuozzo.com> >>>>>>>>> --- >>>>>>>>> block/qcow2-bitmap.c | 17 ++++++++++++++++- >>>>>>>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>>>>>>> Don't you want to make use of this flag? It would appear useful to >>>>>>>> me if >>>>>>>> you just marked autoload bitmaps as in_use instead of deleting >>>>>>>> them >>>>>>>> from >>>>>>>> the image when it's opened and then overwrite them when the >>>>>>>> image is >>>>>>>> closed. >>>>>>> And what is the use of it? >>>>>> You don't need to free any bitmaps when opening the file, and you >>>>>> don't >>>>>> need to allocate any new bitmap space when closing it. >>>>> As bitmaps are sparce in file, I need to allocate new space when >>>>> closing. Or free it... >>>> >>>> Is it a real problem to reallocate space in qcow2? If so, to minimaze >>>> allocations, I will have to make a list of clusters of in_use >>>> bitmaps on >>>> close, and then save current bitmaps to these clusters (and allocated >>>> some additional clusters, or free extra clusters). >>> It's not a real problem, but it does take time, and I maintain that >>> it's >>> time it doesn't need to take because you can just use the in_use flag. >>> >>> I wouldn't worry about reusing clusters of other bitmaps. Of course we >>> can do that later on in some optimization but not now. >>> >>> I just mean the basic case of some auto-loaded bitmap that is not being >>> deleted while the VM is running and is just saved back to the image >>> file >>> once it is closed. I don't expect that users will always consume all of >>> the auto-loaded bitmaps while the VM is active... >>> >>>> Also, if I don't free them on open, I'll have to free them on >>>> remove of >>>> bdrv dirty bitmap.. >>> Yes, so? That takes time, but that is something the user will probably >>> expect. I wouldn't expect opening of the file to take that time. >>> >>> Overall, it doesn't matter time-wise whether you free the bitmap data >>> when opening the image file or when the dirty bitmap is actually >>> deleted >>> by the user or when the image file is closed. Just setting the single >>> in_use flag for all of the auto-loaded bitmaps will basically not take >>> any time. >>> >>> On the other hand, as soon as just a single auto-loaded bitmap survives >>> a VM (or qemu-img) invocation, you will very, very likely safe at least >>> some time because writing the bitmap to the disk can reuse at least >>> some >>> of the existing clusters. >> By the way, dealing with removal of bitmaps when they are deleted by the >> user is also positive when it comes to migration or read-only disks that >> are later reopened R/W: Currently, as far as I can see, you just keep >> the auto-load bitmaps in the image if the image is part of an incoming >> migration or opened read-only, but then you continue under the >> assumption that they are removed from the image. That's not very good. > > You are right, I need to handle reopening more carefully. In my way, I > need to delete bitmaps when reopening R/W and in yours - set in_use bit. > >> >> If you delete the bitmaps only when the user asks you to, then you can >> just return an error that the bitmap cannot be removed at this time >> because the image file is read-only or used in migration (and I don't >> think the latter can even happen when it's the user who has requested >> removal of the bitmap). >> >> Max >> > > Ok, I'll rewrite this way, using 'in_use' bit and trying to avoid > allocate/free overhead. > > Trying to reuse clusters of in_use bitmaps (including contiguous allocations for bitmap tables) will complicate things a lot.. We can use extra clusters of one in_use bitmap to save another one, the same is for bitmap tables. Extra clusters of old bitmap table (in case of resize) can be used for saving other bitmap data, etc..
26.10.2016 12:04, Vladimir Sementsov-Ogievskiy wrote: > 25.10.2016 13:53, Vladimir Sementsov-Ogievskiy wrote: >> 24.10.2016 20:18, Max Reitz wrote: >>> On 24.10.2016 19:08, Max Reitz wrote: >>>> On 24.10.2016 13:35, Vladimir Sementsov-Ogievskiy wrote: >>>>> 24.10.2016 13:32, Vladimir Sementsov-Ogievskiy пишет: >>>>>> 21.10.2016 22:58, Max Reitz пишет: >>>>>>> On 21.10.2016 17:34, Vladimir Sementsov-Ogievskiy wrote: >>>>>>>> 07.10.2016 22:44, Max Reitz пишет: >>>>>>>>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>>>>>>>>> This flag means that the bitmap is now in use by the software or >>>>>>>>>> was not >>>>>>>>>> successfully saved. In any way, with this flag set the bitmap >>>>>>>>>> data >>>>>>>>>> must >>>>>>>>>> be considered inconsistent and should not be loaded. >>>>>>>>>> >>>>>>>>>> With current implementation this flag is never set, as we >>>>>>>>>> just remove >>>>>>>>>> bitmaps from the image after loading. But it defined in qcow2 >>>>>>>>>> spec >>>>>>>>>> and >>>>>>>>>> must be handled. Also, it can be used in future, if async >>>>>>>>>> schemes of >>>>>>>>>> bitmap loading/saving are implemented. >>>>>>>>>> >>>>>>>>>> We also remove in-use bitmaps from the image on open. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>>>>>>> <vsementsov@virtuozzo.com> >>>>>>>>>> --- >>>>>>>>>> block/qcow2-bitmap.c | 17 ++++++++++++++++- >>>>>>>>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>>>>>>>> Don't you want to make use of this flag? It would appear >>>>>>>>> useful to >>>>>>>>> me if >>>>>>>>> you just marked autoload bitmaps as in_use instead of deleting >>>>>>>>> them >>>>>>>>> from >>>>>>>>> the image when it's opened and then overwrite them when the >>>>>>>>> image is >>>>>>>>> closed. >>>>>>>> And what is the use of it? >>>>>>> You don't need to free any bitmaps when opening the file, and >>>>>>> you don't >>>>>>> need to allocate any new bitmap space when closing it. >>>>>> As bitmaps are sparce in file, I need to allocate new space when >>>>>> closing. Or free it... >>>>> >>>>> Is it a real problem to reallocate space in qcow2? If so, to minimaze >>>>> allocations, I will have to make a list of clusters of in_use >>>>> bitmaps on >>>>> close, and then save current bitmaps to these clusters (and allocated >>>>> some additional clusters, or free extra clusters). >>>> It's not a real problem, but it does take time, and I maintain that >>>> it's >>>> time it doesn't need to take because you can just use the in_use flag. >>>> >>>> I wouldn't worry about reusing clusters of other bitmaps. Of course we >>>> can do that later on in some optimization but not now. >>>> >>>> I just mean the basic case of some auto-loaded bitmap that is not >>>> being >>>> deleted while the VM is running and is just saved back to the image >>>> file >>>> once it is closed. I don't expect that users will always consume >>>> all of >>>> the auto-loaded bitmaps while the VM is active... >>>> >>>>> Also, if I don't free them on open, I'll have to free them on >>>>> remove of >>>>> bdrv dirty bitmap.. >>>> Yes, so? That takes time, but that is something the user will probably >>>> expect. I wouldn't expect opening of the file to take that time. >>>> >>>> Overall, it doesn't matter time-wise whether you free the bitmap data >>>> when opening the image file or when the dirty bitmap is actually >>>> deleted >>>> by the user or when the image file is closed. Just setting the single >>>> in_use flag for all of the auto-loaded bitmaps will basically not take >>>> any time. >>>> >>>> On the other hand, as soon as just a single auto-loaded bitmap >>>> survives >>>> a VM (or qemu-img) invocation, you will very, very likely safe at >>>> least >>>> some time because writing the bitmap to the disk can reuse at least >>>> some >>>> of the existing clusters. >>> By the way, dealing with removal of bitmaps when they are deleted by >>> the >>> user is also positive when it comes to migration or read-only disks >>> that >>> are later reopened R/W: Currently, as far as I can see, you just keep >>> the auto-load bitmaps in the image if the image is part of an incoming >>> migration or opened read-only, but then you continue under the >>> assumption that they are removed from the image. That's not very good. >> >> You are right, I need to handle reopening more carefully. In my way, >> I need to delete bitmaps when reopening R/W and in yours - set in_use >> bit. >> >>> >>> If you delete the bitmaps only when the user asks you to, then you can >>> just return an error that the bitmap cannot be removed at this time >>> because the image file is read-only or used in migration (and I don't >>> think the latter can even happen when it's the user who has requested >>> removal of the bitmap). >>> >>> Max >>> >> >> Ok, I'll rewrite this way, using 'in_use' bit and trying to avoid >> allocate/free overhead. >> >> > > Trying to reuse clusters of in_use bitmaps (including contiguous > allocations for bitmap tables) will complicate things a lot.. We can > use extra clusters of one in_use bitmap to save another one, the same > is for bitmap tables. Extra clusters of old bitmap table (in case of > resize) can be used for saving other bitmap data, etc.. > > A compromise strategy of reusing: 1. accumulate all data clusters of in_use bitmaps, which was loaded by this qcow2 instance (we will not touch other old in_use bitmaps) to free_clusters list 2. for each persistent BdrvDirtyBitmap: If there is corresponding in_use bitmap in the image, and its table size == needed table size (there was no resizes), then let's reuse bitmap table. else, move old bitmap table clusters to free_clusters and allocate new table. 3. for each persistent BdrvDirtyBitmap: For bitmap data clusters, take them from free_clusters list, and if it is empty - allocate new clusters. 4. free extra clusters from free_clusters list if any. This strategy is not optimal, but not bad I thing. Is it ok for you? (I'm not sure that this all is not premature optimization, and may be true way is to just free all old staff and reallocate it, as I do.)
26.10.2016 12:21, Vladimir Sementsov-Ogievskiy wrote: > 26.10.2016 12:04, Vladimir Sementsov-Ogievskiy wrote: >> 25.10.2016 13:53, Vladimir Sementsov-Ogievskiy wrote: >>> 24.10.2016 20:18, Max Reitz wrote: >>>> On 24.10.2016 19:08, Max Reitz wrote: >>>>> On 24.10.2016 13:35, Vladimir Sementsov-Ogievskiy wrote: >>>>>> 24.10.2016 13:32, Vladimir Sementsov-Ogievskiy пишет: >>>>>>> 21.10.2016 22:58, Max Reitz пишет: >>>>>>>> On 21.10.2016 17:34, Vladimir Sementsov-Ogievskiy wrote: >>>>>>>>> 07.10.2016 22:44, Max Reitz пишет: >>>>>>>>>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>>>>>>>>>> This flag means that the bitmap is now in use by the >>>>>>>>>>> software or >>>>>>>>>>> was not >>>>>>>>>>> successfully saved. In any way, with this flag set the >>>>>>>>>>> bitmap data >>>>>>>>>>> must >>>>>>>>>>> be considered inconsistent and should not be loaded. >>>>>>>>>>> >>>>>>>>>>> With current implementation this flag is never set, as we >>>>>>>>>>> just remove >>>>>>>>>>> bitmaps from the image after loading. But it defined in >>>>>>>>>>> qcow2 spec >>>>>>>>>>> and >>>>>>>>>>> must be handled. Also, it can be used in future, if async >>>>>>>>>>> schemes of >>>>>>>>>>> bitmap loading/saving are implemented. >>>>>>>>>>> >>>>>>>>>>> We also remove in-use bitmaps from the image on open. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>>>>>>>> <vsementsov@virtuozzo.com> >>>>>>>>>>> --- >>>>>>>>>>> block/qcow2-bitmap.c | 17 ++++++++++++++++- >>>>>>>>>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>>>>>>>>> Don't you want to make use of this flag? It would appear >>>>>>>>>> useful to >>>>>>>>>> me if >>>>>>>>>> you just marked autoload bitmaps as in_use instead of >>>>>>>>>> deleting them >>>>>>>>>> from >>>>>>>>>> the image when it's opened and then overwrite them when the >>>>>>>>>> image is >>>>>>>>>> closed. >>>>>>>>> And what is the use of it? >>>>>>>> You don't need to free any bitmaps when opening the file, and >>>>>>>> you don't >>>>>>>> need to allocate any new bitmap space when closing it. >>>>>>> As bitmaps are sparce in file, I need to allocate new space when >>>>>>> closing. Or free it... >>>>>> >>>>>> Is it a real problem to reallocate space in qcow2? If so, to >>>>>> minimaze >>>>>> allocations, I will have to make a list of clusters of in_use >>>>>> bitmaps on >>>>>> close, and then save current bitmaps to these clusters (and >>>>>> allocated >>>>>> some additional clusters, or free extra clusters). >>>>> It's not a real problem, but it does take time, and I maintain >>>>> that it's >>>>> time it doesn't need to take because you can just use the in_use >>>>> flag. >>>>> >>>>> I wouldn't worry about reusing clusters of other bitmaps. Of >>>>> course we >>>>> can do that later on in some optimization but not now. >>>>> >>>>> I just mean the basic case of some auto-loaded bitmap that is not >>>>> being >>>>> deleted while the VM is running and is just saved back to the >>>>> image file >>>>> once it is closed. I don't expect that users will always consume >>>>> all of >>>>> the auto-loaded bitmaps while the VM is active... >>>>> >>>>>> Also, if I don't free them on open, I'll have to free them on >>>>>> remove of >>>>>> bdrv dirty bitmap.. >>>>> Yes, so? That takes time, but that is something the user will >>>>> probably >>>>> expect. I wouldn't expect opening of the file to take that time. >>>>> >>>>> Overall, it doesn't matter time-wise whether you free the bitmap data >>>>> when opening the image file or when the dirty bitmap is actually >>>>> deleted >>>>> by the user or when the image file is closed. Just setting the single >>>>> in_use flag for all of the auto-loaded bitmaps will basically not >>>>> take >>>>> any time. >>>>> >>>>> On the other hand, as soon as just a single auto-loaded bitmap >>>>> survives >>>>> a VM (or qemu-img) invocation, you will very, very likely safe at >>>>> least >>>>> some time because writing the bitmap to the disk can reuse at >>>>> least some >>>>> of the existing clusters. >>>> By the way, dealing with removal of bitmaps when they are deleted >>>> by the >>>> user is also positive when it comes to migration or read-only disks >>>> that >>>> are later reopened R/W: Currently, as far as I can see, you just keep >>>> the auto-load bitmaps in the image if the image is part of an incoming >>>> migration or opened read-only, but then you continue under the >>>> assumption that they are removed from the image. That's not very good. >>> >>> You are right, I need to handle reopening more carefully. In my way, >>> I need to delete bitmaps when reopening R/W and in yours - set >>> in_use bit. >>> >>>> >>>> If you delete the bitmaps only when the user asks you to, then you can >>>> just return an error that the bitmap cannot be removed at this time >>>> because the image file is read-only or used in migration (and I don't >>>> think the latter can even happen when it's the user who has requested >>>> removal of the bitmap). >>>> >>>> Max >>>> >>> >>> Ok, I'll rewrite this way, using 'in_use' bit and trying to avoid >>> allocate/free overhead. >>> >>> >> >> Trying to reuse clusters of in_use bitmaps (including contiguous >> allocations for bitmap tables) will complicate things a lot.. We can >> use extra clusters of one in_use bitmap to save another one, the same >> is for bitmap tables. Extra clusters of old bitmap table (in case of >> resize) can be used for saving other bitmap data, etc.. >> >> > > A compromise strategy of reusing: > > 1. accumulate all data clusters of in_use bitmaps, which was loaded by > this qcow2 instance (we will not touch other old in_use bitmaps) to > free_clusters list > 2. for each persistent BdrvDirtyBitmap: > If there is corresponding in_use bitmap in the image, and its > table size == needed table size (there was no resizes), then let's > reuse bitmap table. > else, move old bitmap table clusters to free_clusters and > allocate new table. > 3. for each persistent BdrvDirtyBitmap: > For bitmap data clusters, take them from free_clusters list, > and if it is empty - allocate new clusters. > 4. free extra clusters from free_clusters list if any. > > > This strategy is not optimal, but not bad I thing. Is it ok for you? > (I'm not sure that this all is not premature optimization, and may be > true way is to just free all old staff and reallocate it, as I do.) > > With something like this it is not trivial to maintain consistency in qcow2 image in case of fail on reusing (we can get double-linked clusters, or dead links in the image on disk).. Simple solution will be to remove all in_use bitmaps from header extension, or reallocate for them bitmap tables with all zeroes. So, finally: target: we want to reuse clusters of in_use bitmaps, which was loaded by current qcow2 driver (let's call them our in_use bitmaps. sequence: 1. save bitmap directory in memory 2. remove our in_use bitmaps from bitmap directory in disk. from this point, in case of fail we will finish with some leaking clusters and no corruptions. 3. free_clusters list = all data clusters of our in_use bitmaps 4. for each our in_use bitmap: If table size is not appropriate move bitmap table clusters to free_clusters 5. save bitmaps, using free_clusters list for data clusters, old existing bitmap tables and allocating new space when needed. 6. free extra clusters from free_clusters list if any
26.10.2016 15:13, Vladimir Sementsov-Ogievskiy wrote: > 26.10.2016 12:21, Vladimir Sementsov-Ogievskiy wrote: >> 26.10.2016 12:04, Vladimir Sementsov-Ogievskiy wrote: >>> 25.10.2016 13:53, Vladimir Sementsov-Ogievskiy wrote: >>>> 24.10.2016 20:18, Max Reitz wrote: >>>>> On 24.10.2016 19:08, Max Reitz wrote: >>>>>> On 24.10.2016 13:35, Vladimir Sementsov-Ogievskiy wrote: >>>>>>> 24.10.2016 13:32, Vladimir Sementsov-Ogievskiy пишет: >>>>>>>> 21.10.2016 22:58, Max Reitz пишет: >>>>>>>>> On 21.10.2016 17:34, Vladimir Sementsov-Ogievskiy wrote: >>>>>>>>>> 07.10.2016 22:44, Max Reitz пишет: >>>>>>>>>>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>>>>>>>>>>> This flag means that the bitmap is now in use by the >>>>>>>>>>>> software or >>>>>>>>>>>> was not >>>>>>>>>>>> successfully saved. In any way, with this flag set the >>>>>>>>>>>> bitmap data >>>>>>>>>>>> must >>>>>>>>>>>> be considered inconsistent and should not be loaded. >>>>>>>>>>>> >>>>>>>>>>>> With current implementation this flag is never set, as we >>>>>>>>>>>> just remove >>>>>>>>>>>> bitmaps from the image after loading. But it defined in >>>>>>>>>>>> qcow2 spec >>>>>>>>>>>> and >>>>>>>>>>>> must be handled. Also, it can be used in future, if async >>>>>>>>>>>> schemes of >>>>>>>>>>>> bitmap loading/saving are implemented. >>>>>>>>>>>> >>>>>>>>>>>> We also remove in-use bitmaps from the image on open. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>>>>>>>>> <vsementsov@virtuozzo.com> >>>>>>>>>>>> --- >>>>>>>>>>>> block/qcow2-bitmap.c | 17 ++++++++++++++++- >>>>>>>>>>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>>>>>>>>>> Don't you want to make use of this flag? It would appear >>>>>>>>>>> useful to >>>>>>>>>>> me if >>>>>>>>>>> you just marked autoload bitmaps as in_use instead of >>>>>>>>>>> deleting them >>>>>>>>>>> from >>>>>>>>>>> the image when it's opened and then overwrite them when the >>>>>>>>>>> image is >>>>>>>>>>> closed. >>>>>>>>>> And what is the use of it? >>>>>>>>> You don't need to free any bitmaps when opening the file, and >>>>>>>>> you don't >>>>>>>>> need to allocate any new bitmap space when closing it. >>>>>>>> As bitmaps are sparce in file, I need to allocate new space when >>>>>>>> closing. Or free it... >>>>>>> >>>>>>> Is it a real problem to reallocate space in qcow2? If so, to >>>>>>> minimaze >>>>>>> allocations, I will have to make a list of clusters of in_use >>>>>>> bitmaps on >>>>>>> close, and then save current bitmaps to these clusters (and >>>>>>> allocated >>>>>>> some additional clusters, or free extra clusters). >>>>>> It's not a real problem, but it does take time, and I maintain >>>>>> that it's >>>>>> time it doesn't need to take because you can just use the in_use >>>>>> flag. >>>>>> >>>>>> I wouldn't worry about reusing clusters of other bitmaps. Of >>>>>> course we >>>>>> can do that later on in some optimization but not now. >>>>>> >>>>>> I just mean the basic case of some auto-loaded bitmap that is not >>>>>> being >>>>>> deleted while the VM is running and is just saved back to the >>>>>> image file >>>>>> once it is closed. I don't expect that users will always consume >>>>>> all of >>>>>> the auto-loaded bitmaps while the VM is active... >>>>>> >>>>>>> Also, if I don't free them on open, I'll have to free them on >>>>>>> remove of >>>>>>> bdrv dirty bitmap.. >>>>>> Yes, so? That takes time, but that is something the user will >>>>>> probably >>>>>> expect. I wouldn't expect opening of the file to take that time. >>>>>> >>>>>> Overall, it doesn't matter time-wise whether you free the bitmap >>>>>> data >>>>>> when opening the image file or when the dirty bitmap is actually >>>>>> deleted >>>>>> by the user or when the image file is closed. Just setting the >>>>>> single >>>>>> in_use flag for all of the auto-loaded bitmaps will basically not >>>>>> take >>>>>> any time. >>>>>> >>>>>> On the other hand, as soon as just a single auto-loaded bitmap >>>>>> survives >>>>>> a VM (or qemu-img) invocation, you will very, very likely safe at >>>>>> least >>>>>> some time because writing the bitmap to the disk can reuse at >>>>>> least some >>>>>> of the existing clusters. >>>>> By the way, dealing with removal of bitmaps when they are deleted >>>>> by the >>>>> user is also positive when it comes to migration or read-only >>>>> disks that >>>>> are later reopened R/W: Currently, as far as I can see, you just keep >>>>> the auto-load bitmaps in the image if the image is part of an >>>>> incoming >>>>> migration or opened read-only, but then you continue under the >>>>> assumption that they are removed from the image. That's not very >>>>> good. >>>> >>>> You are right, I need to handle reopening more carefully. In my >>>> way, I need to delete bitmaps when reopening R/W and in yours - set >>>> in_use bit. >>>> >>>>> >>>>> If you delete the bitmaps only when the user asks you to, then you >>>>> can >>>>> just return an error that the bitmap cannot be removed at this time >>>>> because the image file is read-only or used in migration (and I don't >>>>> think the latter can even happen when it's the user who has requested >>>>> removal of the bitmap). >>>>> >>>>> Max >>>>> >>>> >>>> Ok, I'll rewrite this way, using 'in_use' bit and trying to avoid >>>> allocate/free overhead. >>>> >>>> >>> >>> Trying to reuse clusters of in_use bitmaps (including contiguous >>> allocations for bitmap tables) will complicate things a lot.. We can >>> use extra clusters of one in_use bitmap to save another one, the >>> same is for bitmap tables. Extra clusters of old bitmap table (in >>> case of resize) can be used for saving other bitmap data, etc.. >>> >>> >> >> A compromise strategy of reusing: >> >> 1. accumulate all data clusters of in_use bitmaps, which was loaded >> by this qcow2 instance (we will not touch other old in_use bitmaps) >> to free_clusters list >> 2. for each persistent BdrvDirtyBitmap: >> If there is corresponding in_use bitmap in the image, and its >> table size == needed table size (there was no resizes), then let's >> reuse bitmap table. >> else, move old bitmap table clusters to free_clusters and >> allocate new table. >> 3. for each persistent BdrvDirtyBitmap: >> For bitmap data clusters, take them from free_clusters list, >> and if it is empty - allocate new clusters. >> 4. free extra clusters from free_clusters list if any. >> >> >> This strategy is not optimal, but not bad I thing. Is it ok for you? >> (I'm not sure that this all is not premature optimization, and may be >> true way is to just free all old staff and reallocate it, as I do.) >> >> > > With something like this it is not trivial to maintain consistency in > qcow2 image in case of fail on reusing (we can get double-linked > clusters, or dead links in the image on disk).. Simple solution will > be to remove all in_use bitmaps from header extension, or reallocate > for them bitmap tables with all zeroes. So, finally: > > target: we want to reuse clusters of in_use bitmaps, which was loaded > by current qcow2 driver (let's call them our in_use bitmaps. > > sequence: > 1. save bitmap directory in memory > 2. remove our in_use bitmaps from bitmap directory in disk. from this > point, in case of fail we will finish with some leaking clusters and > no corruptions. > 3. free_clusters list = all data clusters of our in_use bitmaps > 4. for each our in_use bitmap: If table size is not appropriate move > bitmap table clusters to free_clusters better is to free it, to not deceiveqcow2 allocator.. Can I allocate several clusters by alloc(size), then assume that I have (size + cluster_size - 1)/cluster_size clusters and that I can free these clusters separately? > 5. save bitmaps, using free_clusters list for data clusters, old > existing bitmap tables and allocating new space when needed. > 6. free extra clusters from free_clusters list if any > >
On 26.10.2016 11:04, Vladimir Sementsov-Ogievskiy wrote: > 25.10.2016 13:53, Vladimir Sementsov-Ogievskiy wrote: >> 24.10.2016 20:18, Max Reitz wrote: >>> On 24.10.2016 19:08, Max Reitz wrote: >>>> On 24.10.2016 13:35, Vladimir Sementsov-Ogievskiy wrote: >>>>> 24.10.2016 13:32, Vladimir Sementsov-Ogievskiy пишет: >>>>>> 21.10.2016 22:58, Max Reitz пишет: >>>>>>> On 21.10.2016 17:34, Vladimir Sementsov-Ogievskiy wrote: >>>>>>>> 07.10.2016 22:44, Max Reitz пишет: >>>>>>>>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>>>>>>>>> This flag means that the bitmap is now in use by the software or >>>>>>>>>> was not >>>>>>>>>> successfully saved. In any way, with this flag set the bitmap >>>>>>>>>> data >>>>>>>>>> must >>>>>>>>>> be considered inconsistent and should not be loaded. >>>>>>>>>> >>>>>>>>>> With current implementation this flag is never set, as we just >>>>>>>>>> remove >>>>>>>>>> bitmaps from the image after loading. But it defined in qcow2 >>>>>>>>>> spec >>>>>>>>>> and >>>>>>>>>> must be handled. Also, it can be used in future, if async >>>>>>>>>> schemes of >>>>>>>>>> bitmap loading/saving are implemented. >>>>>>>>>> >>>>>>>>>> We also remove in-use bitmaps from the image on open. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>>>>>>> <vsementsov@virtuozzo.com> >>>>>>>>>> --- >>>>>>>>>> block/qcow2-bitmap.c | 17 ++++++++++++++++- >>>>>>>>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>>>>>>>> Don't you want to make use of this flag? It would appear useful to >>>>>>>>> me if >>>>>>>>> you just marked autoload bitmaps as in_use instead of deleting >>>>>>>>> them >>>>>>>>> from >>>>>>>>> the image when it's opened and then overwrite them when the >>>>>>>>> image is >>>>>>>>> closed. >>>>>>>> And what is the use of it? >>>>>>> You don't need to free any bitmaps when opening the file, and you >>>>>>> don't >>>>>>> need to allocate any new bitmap space when closing it. >>>>>> As bitmaps are sparce in file, I need to allocate new space when >>>>>> closing. Or free it... >>>>> >>>>> Is it a real problem to reallocate space in qcow2? If so, to minimaze >>>>> allocations, I will have to make a list of clusters of in_use >>>>> bitmaps on >>>>> close, and then save current bitmaps to these clusters (and allocated >>>>> some additional clusters, or free extra clusters). >>>> It's not a real problem, but it does take time, and I maintain that >>>> it's >>>> time it doesn't need to take because you can just use the in_use flag. >>>> >>>> I wouldn't worry about reusing clusters of other bitmaps. Of course we >>>> can do that later on in some optimization but not now. >>>> >>>> I just mean the basic case of some auto-loaded bitmap that is not being >>>> deleted while the VM is running and is just saved back to the image >>>> file >>>> once it is closed. I don't expect that users will always consume all of >>>> the auto-loaded bitmaps while the VM is active... >>>> >>>>> Also, if I don't free them on open, I'll have to free them on >>>>> remove of >>>>> bdrv dirty bitmap.. >>>> Yes, so? That takes time, but that is something the user will probably >>>> expect. I wouldn't expect opening of the file to take that time. >>>> >>>> Overall, it doesn't matter time-wise whether you free the bitmap data >>>> when opening the image file or when the dirty bitmap is actually >>>> deleted >>>> by the user or when the image file is closed. Just setting the single >>>> in_use flag for all of the auto-loaded bitmaps will basically not take >>>> any time. >>>> >>>> On the other hand, as soon as just a single auto-loaded bitmap survives >>>> a VM (or qemu-img) invocation, you will very, very likely safe at least >>>> some time because writing the bitmap to the disk can reuse at least >>>> some >>>> of the existing clusters. >>> By the way, dealing with removal of bitmaps when they are deleted by the >>> user is also positive when it comes to migration or read-only disks that >>> are later reopened R/W: Currently, as far as I can see, you just keep >>> the auto-load bitmaps in the image if the image is part of an incoming >>> migration or opened read-only, but then you continue under the >>> assumption that they are removed from the image. That's not very good. >> >> You are right, I need to handle reopening more carefully. In my way, I >> need to delete bitmaps when reopening R/W and in yours - set in_use bit. >> >>> >>> If you delete the bitmaps only when the user asks you to, then you can >>> just return an error that the bitmap cannot be removed at this time >>> because the image file is read-only or used in migration (and I don't >>> think the latter can even happen when it's the user who has requested >>> removal of the bitmap). >>> >>> Max >>> >> >> Ok, I'll rewrite this way, using 'in_use' bit and trying to avoid >> allocate/free overhead. >> >> > > Trying to reuse clusters of in_use bitmaps (including contiguous > allocations for bitmap tables) will complicate things a lot.. We can use > extra clusters of one in_use bitmap to save another one, the same is for > bitmap tables. Extra clusters of old bitmap table (in case of resize) > can be used for saving other bitmap data, etc.. I feel like you're trying to optimize too much. When rewriting the bitmap data, reuse only the clusters you have to write to anyway. Say the bitmap table looks like this when you open the image: [sparse 0, cluster 42, cluster 45, sparse 0, cluster 46] ("sparse 0" means no cluster is allocated and the bitmap table entry is 0, thus creating a sparse bitmap where that part is supposed to be read as 0.) Now during runtime, you set some bits, maybe you even clear the whole bitmap at some point because you're doing a backup, and then you set some other bits, so when you close the image, the bitmap table would have to look like this: [sparse 0, sparse 0, data, data, data] Now, when you want to save that bitmap data, you just walk through the existing bitmap table. The first entry is a sparse 0, and the bitmap is sparse there, too, so that can stay as it is. The second entry points to data but the bitmap is now sparse. You can then free the cluster (cluster 42) and write "sparse 0" into the bitmap table entry. The third entry points to data and the bitmap contains data there, so you just write the data to cluster 45. The fourth entry is sparse but your bitmap contains data. So now you need to allocate a cluster, maybe that will cluster 67, and write your data there, and make the bitmap table entry point there. The fifth entry finally is handled just like the third entry. So afterwards, your bitmap table would look like this: [sparse 0, sparse 0, cluster 45, cluster 67, cluster 46] It sounds as if you're trying to reuse all the clusters, i.e. you'd ideally get: [sparse 0, sparse 0, cluster 45, cluster 42, cluster 46] May be possible, but I really wouldn't worry about that. I'd consider it much too difficult. Just reuse the existing bitmap table. Max
25.10.2016 13:53, Vladimir Sementsov-Ogievskiy wrote: > 24.10.2016 20:18, Max Reitz wrote: >> On 24.10.2016 19:08, Max Reitz wrote: >>> On 24.10.2016 13:35, Vladimir Sementsov-Ogievskiy wrote: >>>> 24.10.2016 13:32, Vladimir Sementsov-Ogievskiy пишет: >>>>> 21.10.2016 22:58, Max Reitz пишет: >>>>>> On 21.10.2016 17:34, Vladimir Sementsov-Ogievskiy wrote: >>>>>>> 07.10.2016 22:44, Max Reitz пишет: >>>>>>>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>>>>>>>> This flag means that the bitmap is now in use by the software or >>>>>>>>> was not >>>>>>>>> successfully saved. In any way, with this flag set the bitmap >>>>>>>>> data >>>>>>>>> must >>>>>>>>> be considered inconsistent and should not be loaded. >>>>>>>>> >>>>>>>>> With current implementation this flag is never set, as we just >>>>>>>>> remove >>>>>>>>> bitmaps from the image after loading. But it defined in qcow2 >>>>>>>>> spec >>>>>>>>> and >>>>>>>>> must be handled. Also, it can be used in future, if async >>>>>>>>> schemes of >>>>>>>>> bitmap loading/saving are implemented. >>>>>>>>> >>>>>>>>> We also remove in-use bitmaps from the image on open. >>>>>>>>> >>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>>>>>> <vsementsov@virtuozzo.com> >>>>>>>>> --- >>>>>>>>> block/qcow2-bitmap.c | 17 ++++++++++++++++- >>>>>>>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>>>>>>> Don't you want to make use of this flag? It would appear useful to >>>>>>>> me if >>>>>>>> you just marked autoload bitmaps as in_use instead of deleting >>>>>>>> them >>>>>>>> from >>>>>>>> the image when it's opened and then overwrite them when the >>>>>>>> image is >>>>>>>> closed. >>>>>>> And what is the use of it? >>>>>> You don't need to free any bitmaps when opening the file, and you >>>>>> don't >>>>>> need to allocate any new bitmap space when closing it. >>>>> As bitmaps are sparce in file, I need to allocate new space when >>>>> closing. Or free it... >>>> >>>> Is it a real problem to reallocate space in qcow2? If so, to minimaze >>>> allocations, I will have to make a list of clusters of in_use >>>> bitmaps on >>>> close, and then save current bitmaps to these clusters (and allocated >>>> some additional clusters, or free extra clusters). >>> It's not a real problem, but it does take time, and I maintain that >>> it's >>> time it doesn't need to take because you can just use the in_use flag. >>> >>> I wouldn't worry about reusing clusters of other bitmaps. Of course we >>> can do that later on in some optimization but not now. >>> >>> I just mean the basic case of some auto-loaded bitmap that is not being >>> deleted while the VM is running and is just saved back to the image >>> file >>> once it is closed. I don't expect that users will always consume all of >>> the auto-loaded bitmaps while the VM is active... >>> >>>> Also, if I don't free them on open, I'll have to free them on >>>> remove of >>>> bdrv dirty bitmap.. >>> Yes, so? That takes time, but that is something the user will probably >>> expect. I wouldn't expect opening of the file to take that time. >>> >>> Overall, it doesn't matter time-wise whether you free the bitmap data >>> when opening the image file or when the dirty bitmap is actually >>> deleted >>> by the user or when the image file is closed. Just setting the single >>> in_use flag for all of the auto-loaded bitmaps will basically not take >>> any time. >>> >>> On the other hand, as soon as just a single auto-loaded bitmap survives >>> a VM (or qemu-img) invocation, you will very, very likely safe at least >>> some time because writing the bitmap to the disk can reuse at least >>> some >>> of the existing clusters. >> By the way, dealing with removal of bitmaps when they are deleted by the >> user is also positive when it comes to migration or read-only disks that >> are later reopened R/W: Currently, as far as I can see, you just keep >> the auto-load bitmaps in the image if the image is part of an incoming >> migration or opened read-only, but then you continue under the >> assumption that they are removed from the image. That's not very good. > > You are right, I need to handle reopening more carefully. In my way, I > need to delete bitmaps when reopening R/W and in yours - set in_use bit. Now I think, that loading auto_loading bitmaps in qcow2_open is wrong. I should load them only in bdrv_open, to avoid reloading bitmaps on drv->bdrv_open/drv->bdrv_close (they are called from bdrv_snapshot_goto). So, it would be like this: on bdrv_open, after drv->bdrv_open call drv->load_autoloading_bitmaps, which will load bitmaps, mark them in_use in the image (if it is writable), create corresponding BdrvDirtyBitmaps. If there _any_ conflicts with existing BdrvDirtyBitmaps then fail. on bdrv_close, before drv->bdrv_close call drv->store_persitstent_bitmaps, which will store persistent bitmaps, set in_use to false and _delete_ corresponding BdrvDirtyBitmaps. Also, in qcow2_reopen_prepare in case of changing write-ability from false to true we need to mark corresponding bitmaps in the image as in_use.. And something like this for incoming migration too. qcow2_open will only load header extension fields to bs->opaque, and check these fields. It will not load bitmap directory. > >> >> If you delete the bitmaps only when the user asks you to, then you can >> just return an error that the bitmap cannot be removed at this time >> because the image file is read-only or used in migration (and I don't >> think the latter can even happen when it's the user who has requested >> removal of the bitmap). >> >> Max >> > > Ok, I'll rewrite this way, using 'in_use' bit and trying to avoid > allocate/free overhead. > >
On 07.11.2016 17:12, Vladimir Sementsov-Ogievskiy wrote: > 25.10.2016 13:53, Vladimir Sementsov-Ogievskiy wrote: >> 24.10.2016 20:18, Max Reitz wrote: >>> On 24.10.2016 19:08, Max Reitz wrote: >>>> On 24.10.2016 13:35, Vladimir Sementsov-Ogievskiy wrote: >>>>> 24.10.2016 13:32, Vladimir Sementsov-Ogievskiy пишет: >>>>>> 21.10.2016 22:58, Max Reitz пишет: >>>>>>> On 21.10.2016 17:34, Vladimir Sementsov-Ogievskiy wrote: >>>>>>>> 07.10.2016 22:44, Max Reitz пишет: >>>>>>>>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>>>>>>>>> This flag means that the bitmap is now in use by the software or >>>>>>>>>> was not >>>>>>>>>> successfully saved. In any way, with this flag set the bitmap >>>>>>>>>> data >>>>>>>>>> must >>>>>>>>>> be considered inconsistent and should not be loaded. >>>>>>>>>> >>>>>>>>>> With current implementation this flag is never set, as we just >>>>>>>>>> remove >>>>>>>>>> bitmaps from the image after loading. But it defined in qcow2 >>>>>>>>>> spec >>>>>>>>>> and >>>>>>>>>> must be handled. Also, it can be used in future, if async >>>>>>>>>> schemes of >>>>>>>>>> bitmap loading/saving are implemented. >>>>>>>>>> >>>>>>>>>> We also remove in-use bitmaps from the image on open. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>>>>>>> <vsementsov@virtuozzo.com> >>>>>>>>>> --- >>>>>>>>>> block/qcow2-bitmap.c | 17 ++++++++++++++++- >>>>>>>>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>>>>>>>> Don't you want to make use of this flag? It would appear useful to >>>>>>>>> me if >>>>>>>>> you just marked autoload bitmaps as in_use instead of deleting >>>>>>>>> them >>>>>>>>> from >>>>>>>>> the image when it's opened and then overwrite them when the >>>>>>>>> image is >>>>>>>>> closed. >>>>>>>> And what is the use of it? >>>>>>> You don't need to free any bitmaps when opening the file, and you >>>>>>> don't >>>>>>> need to allocate any new bitmap space when closing it. >>>>>> As bitmaps are sparce in file, I need to allocate new space when >>>>>> closing. Or free it... >>>>> >>>>> Is it a real problem to reallocate space in qcow2? If so, to minimaze >>>>> allocations, I will have to make a list of clusters of in_use >>>>> bitmaps on >>>>> close, and then save current bitmaps to these clusters (and allocated >>>>> some additional clusters, or free extra clusters). >>>> It's not a real problem, but it does take time, and I maintain that >>>> it's >>>> time it doesn't need to take because you can just use the in_use flag. >>>> >>>> I wouldn't worry about reusing clusters of other bitmaps. Of course we >>>> can do that later on in some optimization but not now. >>>> >>>> I just mean the basic case of some auto-loaded bitmap that is not being >>>> deleted while the VM is running and is just saved back to the image >>>> file >>>> once it is closed. I don't expect that users will always consume all of >>>> the auto-loaded bitmaps while the VM is active... >>>> >>>>> Also, if I don't free them on open, I'll have to free them on >>>>> remove of >>>>> bdrv dirty bitmap.. >>>> Yes, so? That takes time, but that is something the user will probably >>>> expect. I wouldn't expect opening of the file to take that time. >>>> >>>> Overall, it doesn't matter time-wise whether you free the bitmap data >>>> when opening the image file or when the dirty bitmap is actually >>>> deleted >>>> by the user or when the image file is closed. Just setting the single >>>> in_use flag for all of the auto-loaded bitmaps will basically not take >>>> any time. >>>> >>>> On the other hand, as soon as just a single auto-loaded bitmap survives >>>> a VM (or qemu-img) invocation, you will very, very likely safe at least >>>> some time because writing the bitmap to the disk can reuse at least >>>> some >>>> of the existing clusters. >>> By the way, dealing with removal of bitmaps when they are deleted by the >>> user is also positive when it comes to migration or read-only disks that >>> are later reopened R/W: Currently, as far as I can see, you just keep >>> the auto-load bitmaps in the image if the image is part of an incoming >>> migration or opened read-only, but then you continue under the >>> assumption that they are removed from the image. That's not very good. >> >> You are right, I need to handle reopening more carefully. In my way, I >> need to delete bitmaps when reopening R/W and in yours - set in_use bit. > > Now I think, that loading auto_loading bitmaps in qcow2_open is wrong. I > should load them only in bdrv_open, to avoid reloading bitmaps on > drv->bdrv_open/drv->bdrv_close (they are called from bdrv_snapshot_goto). > > So, it would be like this: > > on bdrv_open, after drv->bdrv_open call drv->load_autoloading_bitmaps, > which will load bitmaps, mark them in_use in the image (if it is > writable), create corresponding BdrvDirtyBitmaps. If there _any_ > conflicts with existing BdrvDirtyBitmaps then fail. > > on bdrv_close, before drv->bdrv_close call drv->store_persitstent_bitmaps, > which will store persistent bitmaps, set in_use to false and _delete_ > corresponding BdrvDirtyBitmaps. Sounds good. > Also, in qcow2_reopen_prepare in case of changing write-ability from > false to true we need to mark corresponding bitmaps in the image as > in_use.. And something like this for incoming migration too. Right. > qcow2_open will only load header extension fields to bs->opaque, and > check these fields. It will not load bitmap directory. Yes, that sounds good. Max
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index a5be25a..8cf40f0 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -28,6 +28,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "qemu/cutils.h" +#include "exec/log.h" #include "block/block_int.h" #include "block/qcow2.h" @@ -44,7 +45,8 @@ #define BME_MAX_NAME_SIZE 1023 /* Bitmap directory entry flags */ -#define BME_RESERVED_FLAGS 0xfffffffd +#define BME_RESERVED_FLAGS 0xfffffffc +#define BME_FLAG_IN_USE 1 #define BME_FLAG_AUTO (1U << 1) /* bits [1, 8] U [56, 63] are reserved */ @@ -287,6 +289,11 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap = NULL; char *name = g_strndup(dir_entry_name_notcstr(entry), entry->name_size); + if (entry->flags & BME_FLAG_IN_USE) { + error_setg(errp, "Bitmap '%s' is in use", name); + goto fail; + } + ret = bitmap_table_load(bs, entry, &bitmap_table); if (ret < 0) { error_setg_errno(errp, -ret, @@ -533,6 +540,14 @@ int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp) for_each_bitmap_dir_entry(e, dir, dir_size) { uint32_t fl = e->flags; + if (fl & BME_FLAG_IN_USE) { + qemu_log("qcow2 warning: " + "removing in_use bitmap '%.*s' for image %s.\n", + e->name_size, (char *)(e + 1), bdrv_get_device_or_node_name(bs)); + + continue; + } + if (fl & BME_FLAG_AUTO) { BdrvDirtyBitmap *bitmap = load_bitmap(bs, e, errp); if (bitmap == NULL) {
This flag means that the bitmap is now in use by the software or was not successfully saved. In any way, with this flag set the bitmap data must be considered inconsistent and should not be loaded. With current implementation this flag is never set, as we just remove bitmaps from the image after loading. But it defined in qcow2 spec and must be handled. Also, it can be used in future, if async schemes of bitmap loading/saving are implemented. We also remove in-use bitmaps from the image on open. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/qcow2-bitmap.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)