| Message ID | 20180808071051.30628-4-lbloch@janustech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qcow2: Make the L2 cache cover the whole image by default | expand |
On Wed 08 Aug 2018 09:10:49 AM CEST, Leonid Bloch wrote: > The caches are now recalculated upon image resizing. This is done > because the new default behavior of assigning a sufficient L2 cache to > cover the entire image implies that the cache will still be sufficient > after an image resizing. This is related to what I mentioned in the previous patch. The default behavior doesn't make the cache try to cover the entire image (or at least it doesn't *extend* the cache, which is what I understand from this paragraph). What it does is *reduce* the cache if the smaller version is enough for the entire image. > Signed-off-by: Leonid Bloch <lbloch@janustech.com> > --- > block/qcow2.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 98cb96aaca..f60cb92169 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -3639,6 +3639,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, > } > } > > + bs->total_sectors = offset / BDRV_SECTOR_SIZE; > + > /* write updated header.size */ > offset = cpu_to_be64(offset); > ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size), > @@ -3649,6 +3651,12 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, > } > > s->l1_vm_state_index = new_l1_size; You could add an empty line here for readability. > + /* Update cache sizes */ > + QDict *options = qdict_clone_shallow(bs->options); C99 allows variable declarations in the middle of a block, but we're still doing it at the beginning (I don't know if there's a good reason for this?). Otherwise the patch looks good to me. Thanks! Berto
On 08/08/2018 05:13 PM, Alberto Garcia wrote: > On Wed 08 Aug 2018 09:10:49 AM CEST, Leonid Bloch wrote: >> The caches are now recalculated upon image resizing. This is done >> because the new default behavior of assigning a sufficient L2 cache to >> cover the entire image implies that the cache will still be sufficient >> after an image resizing. > > This is related to what I mentioned in the previous patch. The default > behavior doesn't make the cache try to cover the entire image (or at > least it doesn't *extend* the cache, which is what I understand from > this paragraph). What it does is *reduce* the cache if the smaller > version is enough for the entire image. But it doesn't say that it extends the cache. It says that it *adapts* the cache to the image size, and therefore it should be resized when the image is resized. At least I understand it this way. That said, I'd mention the limit there, instead of just "sufficient". > >> Signed-off-by: Leonid Bloch <lbloch@janustech.com> >> --- >> block/qcow2.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 98cb96aaca..f60cb92169 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -3639,6 +3639,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >> } >> } >> >> + bs->total_sectors = offset / BDRV_SECTOR_SIZE; >> + >> /* write updated header.size */ >> offset = cpu_to_be64(offset); >> ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size), >> @@ -3649,6 +3651,12 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >> } >> >> s->l1_vm_state_index = new_l1_size; > > You could add an empty line here for readability. Yes, definitely, I will. Thanks. > >> + /* Update cache sizes */ >> + QDict *options = qdict_clone_shallow(bs->options); > > C99 allows variable declarations in the middle of a block, but we're > still doing it at the beginning (I don't know if there's a good reason > for this?). I did it for readability, and I didn't see a style directive for this. But if the style requires it - no problem. :) > > Otherwise the patch looks good to me. Thanks! Thanks! Leonid. > > Berto >
diff --git a/block/qcow2.c b/block/qcow2.c index 98cb96aaca..f60cb92169 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -3639,6 +3639,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, } } + bs->total_sectors = offset / BDRV_SECTOR_SIZE; + /* write updated header.size */ offset = cpu_to_be64(offset); ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size), @@ -3649,6 +3651,12 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, } s->l1_vm_state_index = new_l1_size; + /* Update cache sizes */ + QDict *options = qdict_clone_shallow(bs->options); + ret = qcow2_update_options(bs, options, s->flags, errp); + if (ret < 0) { + goto fail; + } ret = 0; fail: qemu_co_mutex_unlock(&s->lock);
The caches are now recalculated upon image resizing. This is done because the new default behavior of assigning a sufficient L2 cache to cover the entire image implies that the cache will still be sufficient after an image resizing. Signed-off-by: Leonid Bloch <lbloch@janustech.com> --- block/qcow2.c | 8 ++++++++ 1 file changed, 8 insertions(+)