| Message ID | fd0f93353a218ff4518f34ebdbca05c2fc0f1085.1584468723.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add subcluster allocation to qcow2 | expand |
17.03.2020 21:16, Alberto Garcia wrote: > qcow2 images with subclusters have 128-bit L2 entries. The first 64 > bits contain the same information as traditional images and the last > 64 bits form a bitmap with the status of each individual subcluster. > > Because of that we cannot assume that L2 entries are sizeof(uint64_t) > anymore. This function returns the proper value for the image. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > Reviewed-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2.h | 9 +++++++++ > block/qcow2-cluster.c | 12 ++++++------ > block/qcow2-refcount.c | 14 ++++++++------ > block/qcow2.c | 8 ++++---- > 4 files changed, 27 insertions(+), 16 deletions(-) > > diff --git a/block/qcow2.h b/block/qcow2.h > index 06929072d2..1eb4b46807 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -80,6 +80,10 @@ > > #define QCOW_EXTL2_SUBCLUSTERS_PER_CLUSTER 32 > > +/* Size of normal and extended L2 entries */ > +#define L2E_SIZE_NORMAL (sizeof(uint64_t)) > +#define L2E_SIZE_EXTENDED (sizeof(uint64_t) * 2) > + > #define MIN_CLUSTER_BITS 9 > #define MAX_CLUSTER_BITS 21 > > @@ -506,6 +510,11 @@ static inline bool has_subclusters(BDRVQcow2State *s) > return false; > } > > +static inline size_t l2_entry_size(BDRVQcow2State *s) > +{ > + return has_subclusters(s) ? L2E_SIZE_EXTENDED : L2E_SIZE_NORMAL; > +} > + > static inline uint64_t get_l2_entry(BDRVQcow2State *s, uint64_t *l2_slice, > int idx) > { > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index cd48ab0223..41a23c5305 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -208,7 +208,7 @@ static int l2_load(BlockDriverState *bs, uint64_t offset, > uint64_t l2_offset, uint64_t **l2_slice) > { > BDRVQcow2State *s = bs->opaque; > - int start_of_slice = sizeof(uint64_t) * > + int start_of_slice = l2_entry_size(s) * > (offset_to_l2_index(s, offset) - offset_to_l2_slice_index(s, offset)); > > return qcow2_cache_get(bs, s->l2_table_cache, l2_offset + start_of_slice, > @@ -281,7 +281,7 @@ static int l2_allocate(BlockDriverState *bs, int l1_index) > > /* allocate a new l2 entry */ > > - l2_offset = qcow2_alloc_clusters(bs, s->l2_size * sizeof(uint64_t)); > + l2_offset = qcow2_alloc_clusters(bs, s->l2_size * l2_entry_size(s)); hmm. s->l2_size * l2_entry_size, isn't it just s->cluster_size always? Maybe, just refactor these things? > if (l2_offset < 0) { > ret = l2_offset; > goto fail; > @@ -305,7 +305,7 @@ static int l2_allocate(BlockDriverState *bs, int l1_index) [...] > @@ -1425,7 +1425,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, > bs->encrypted = true; > } > > - s->l2_bits = s->cluster_bits - 3; /* L2 is always one cluster */ > + s->l2_bits = s->cluster_bits - ctz32(l2_entry_size(s)); > s->l2_size = 1 << s->l2_bits; > /* 2^(s->refcount_order - 3) is the refcount width in bytes */ > s->refcount_block_bits = s->cluster_bits - (s->refcount_order - 3); > @@ -4104,7 +4104,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, > * preallocation. All that matters is that we will not have to allocate > * new refcount structures for them.) */ > nb_new_l2_tables = DIV_ROUND_UP(nb_new_data_clusters, > - s->cluster_size / sizeof(uint64_t)); > + s->cluster_size / l2_entry_size(s)); Isn't it just s->l2_size ? > /* The cluster range may not be aligned to L2 boundaries, so add one L2 > * table for a potential head/tail */ > nb_new_l2_tables++; > Conversions looks correct, but how to check that we have converted everything? Trying at least cd block; git grep 'sizeof(uint64_t)' qcow2* | grep -v 'l1_size \*' | grep -v 'l1_sz \*' | grep -v refcount | grep -v reftable I found this not converted chunk: /* total size of L2 tables */ nl2e = aligned_total_size / cluster_size; nl2e = ROUND_UP(nl2e, cluster_size / sizeof(uint64_t)); meta_size += nl2e * sizeof(uint64_t); Hmm. How to avoid it? Maybe, at least, refactor the code, to drop all sizeof(uint64_t), converting them to L2_ENTRY_SIZE, L1_ENTRY_SIZE, REFTABLE_ENTRY_SIZE etc? And all occurrences of pure '8' (not many of them exist)
On Tue 14 Apr 2020 11:44:57 AM CEST, Vladimir Sementsov-Ogievskiy wrote: >> /* allocate a new l2 entry */ >> >> - l2_offset = qcow2_alloc_clusters(bs, s->l2_size * sizeof(uint64_t)); >> + l2_offset = qcow2_alloc_clusters(bs, s->l2_size * l2_entry_size(s)); > > hmm. s->l2_size * l2_entry_size, isn't it just s->cluster_size always? > Maybe, just refactor these things? I think the patch is simpler to follow if I only do the strictly necessary changes and don't mix them with other things. >> nb_new_l2_tables = DIV_ROUND_UP(nb_new_data_clusters, >> - s->cluster_size / sizeof(uint64_t)); >> + s->cluster_size / l2_entry_size(s)); > > Isn't it just s->l2_size ? Yes, same as before. >> /* The cluster range may not be aligned to L2 boundaries, so add one L2 >> * table for a potential head/tail */ >> nb_new_l2_tables++; > > Conversions looks correct, but how to check that we have converted > everything? I went through all cases, I think I didn't miss any! > I found this not converted chunk: > > /* total size of L2 tables */ > nl2e = aligned_total_size / cluster_size; > nl2e = ROUND_UP(nl2e, cluster_size / sizeof(uint64_t)); > meta_size += nl2e * sizeof(uint64_t); This is used by qcow2_measure() and is fixed on a later patch because, unlike all other cases, it does not use a BlockDriverState to determine the size of an L2 entry. > Hmm. How to avoid it? Maybe, at least, refactor the code, to drop all > sizeof(uint64_t), converting them to L2_ENTRY_SIZE, L1_ENTRY_SIZE, > REFTABLE_ENTRY_SIZE etc? That wouldn't be a bad thing I guess but, again, for a separate patch or series. > And all occurrences of pure '8' (not many of them exist) I think most/all nowadays only refer to the number of bits per byte. Maybe there's a couple that still need to be fixed, but we have been removing a lot of numeric literals from the qcow2 code (see for example b6c246942b, 3afea40243 or a35f87f50d). Berto
14.04.2020 15:20, Alberto Garcia wrote: > On Tue 14 Apr 2020 11:44:57 AM CEST, Vladimir Sementsov-Ogievskiy wrote: >>> /* allocate a new l2 entry */ >>> >>> - l2_offset = qcow2_alloc_clusters(bs, s->l2_size * sizeof(uint64_t)); >>> + l2_offset = qcow2_alloc_clusters(bs, s->l2_size * l2_entry_size(s)); >> >> hmm. s->l2_size * l2_entry_size, isn't it just s->cluster_size always? >> Maybe, just refactor these things? > > I think the patch is simpler to follow if I only do the strictly > necessary changes and don't mix them with other things. > >>> nb_new_l2_tables = DIV_ROUND_UP(nb_new_data_clusters, >>> - s->cluster_size / sizeof(uint64_t)); >>> + s->cluster_size / l2_entry_size(s)); >> >> Isn't it just s->l2_size ? > > Yes, same as before. > >>> /* The cluster range may not be aligned to L2 boundaries, so add one L2 >>> * table for a potential head/tail */ >>> nb_new_l2_tables++; >> >> Conversions looks correct, but how to check that we have converted >> everything? > > I went through all cases, I think I didn't miss any! > >> I found this not converted chunk: >> >> /* total size of L2 tables */ >> nl2e = aligned_total_size / cluster_size; >> nl2e = ROUND_UP(nl2e, cluster_size / sizeof(uint64_t)); >> meta_size += nl2e * sizeof(uint64_t); > > This is used by qcow2_measure() and is fixed on a later patch because, > unlike all other cases, it does not use a BlockDriverState to determine > the size of an L2 entry. > >> Hmm. How to avoid it? Maybe, at least, refactor the code, to drop all >> sizeof(uint64_t), converting them to L2_ENTRY_SIZE, L1_ENTRY_SIZE, >> REFTABLE_ENTRY_SIZE etc? > > That wouldn't be a bad thing I guess but, again, for a separate patch or > series. > >> And all occurrences of pure '8' (not many of them exist) > > I think most/all nowadays only refer to the number of bits per byte. > > Maybe there's a couple that still need to be fixed, but we have been > removing a lot of numeric literals from the qcow2 code (see for example > b6c246942b, 3afea40243 or a35f87f50d). > git grep '\<8\>' block/qcow2* shows at least qcow2-cluster.c: s->l1_table_offset + 8 * l1_start_index, bufsize, false); qcow2-cluster.c: s->l1_table_offset + 8 * l1_start_index,
On Tue 14 Apr 2020 02:29:13 PM CEST, Vladimir Sementsov-Ogievskiy wrote: >>> Hmm. How to avoid it? Maybe, at least, refactor the code, to drop all >>> sizeof(uint64_t), converting them to L2_ENTRY_SIZE, L1_ENTRY_SIZE, >>> REFTABLE_ENTRY_SIZE etc? >> >> That wouldn't be a bad thing I guess but, again, for a separate patch or >> series. >> >>> And all occurrences of pure '8' (not many of them exist) >> >> I think most/all nowadays only refer to the number of bits per byte. >> >> Maybe there's a couple that still need to be fixed, but we have been >> removing a lot of numeric literals from the qcow2 code (see for example >> b6c246942b, 3afea40243 or a35f87f50d). >> > > > git grep '\<8\>' block/qcow2* > > shows at least > > qcow2-cluster.c: s->l1_table_offset + 8 * l1_start_index, bufsize, false); > qcow2-cluster.c: s->l1_table_offset + 8 * l1_start_index, I see, worth replacing with L1_ENTRY_SIZE as you suggest. I can take of writing the patches if you want. Berto
14.04.2020 15:33, Alberto Garcia wrote: > On Tue 14 Apr 2020 02:29:13 PM CEST, Vladimir Sementsov-Ogievskiy wrote: >>>> Hmm. How to avoid it? Maybe, at least, refactor the code, to drop all >>>> sizeof(uint64_t), converting them to L2_ENTRY_SIZE, L1_ENTRY_SIZE, >>>> REFTABLE_ENTRY_SIZE etc? >>> >>> That wouldn't be a bad thing I guess but, again, for a separate patch or >>> series. >>> >>>> And all occurrences of pure '8' (not many of them exist) >>> >>> I think most/all nowadays only refer to the number of bits per byte. >>> >>> Maybe there's a couple that still need to be fixed, but we have been >>> removing a lot of numeric literals from the qcow2 code (see for example >>> b6c246942b, 3afea40243 or a35f87f50d). >>> >> >> >> git grep '\<8\>' block/qcow2* >> >> shows at least >> >> qcow2-cluster.c: s->l1_table_offset + 8 * l1_start_index, bufsize, false); >> qcow2-cluster.c: s->l1_table_offset + 8 * l1_start_index, > > I see, worth replacing with L1_ENTRY_SIZE as you suggest. I can take of > writing the patches if you want. > That would be great, if not too burdensome :)
On 4/14/20 7:20 AM, Alberto Garcia wrote: >> Hmm. How to avoid it? Maybe, at least, refactor the code, to drop all >> sizeof(uint64_t), converting them to L2_ENTRY_SIZE, L1_ENTRY_SIZE, >> REFTABLE_ENTRY_SIZE etc? > > That wouldn't be a bad thing I guess but, again, for a separate patch or > series. > >> And all occurrences of pure '8' (not many of them exist) > > I think most/all nowadays only refer to the number of bits per byte. CHAR_BIT (from <limits.h>) is good for that. > > Maybe there's a couple that still need to be fixed, but we have been > removing a lot of numeric literals from the qcow2 code (see for example > b6c246942b, 3afea40243 or a35f87f50d). > > Berto >
On Tue 14 Apr 2020 06:01:42 PM CEST, Eric Blake <eblake@redhat.com> wrote: >>> And all occurrences of pure '8' (not many of them exist) >> >> I think most/all nowadays only refer to the number of bits per byte. > > CHAR_BIT (from <limits.h>) is good for that. Wow, ok, I wonder if that actually makes the code more readable, but I'll take it into account when writing the patch, thanks. Berto
diff --git a/block/qcow2.h b/block/qcow2.h index 06929072d2..1eb4b46807 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -80,6 +80,10 @@ #define QCOW_EXTL2_SUBCLUSTERS_PER_CLUSTER 32 +/* Size of normal and extended L2 entries */ +#define L2E_SIZE_NORMAL (sizeof(uint64_t)) +#define L2E_SIZE_EXTENDED (sizeof(uint64_t) * 2) + #define MIN_CLUSTER_BITS 9 #define MAX_CLUSTER_BITS 21 @@ -506,6 +510,11 @@ static inline bool has_subclusters(BDRVQcow2State *s) return false; } +static inline size_t l2_entry_size(BDRVQcow2State *s) +{ + return has_subclusters(s) ? L2E_SIZE_EXTENDED : L2E_SIZE_NORMAL; +} + static inline uint64_t get_l2_entry(BDRVQcow2State *s, uint64_t *l2_slice, int idx) { diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index cd48ab0223..41a23c5305 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -208,7 +208,7 @@ static int l2_load(BlockDriverState *bs, uint64_t offset, uint64_t l2_offset, uint64_t **l2_slice) { BDRVQcow2State *s = bs->opaque; - int start_of_slice = sizeof(uint64_t) * + int start_of_slice = l2_entry_size(s) * (offset_to_l2_index(s, offset) - offset_to_l2_slice_index(s, offset)); return qcow2_cache_get(bs, s->l2_table_cache, l2_offset + start_of_slice, @@ -281,7 +281,7 @@ static int l2_allocate(BlockDriverState *bs, int l1_index) /* allocate a new l2 entry */ - l2_offset = qcow2_alloc_clusters(bs, s->l2_size * sizeof(uint64_t)); + l2_offset = qcow2_alloc_clusters(bs, s->l2_size * l2_entry_size(s)); if (l2_offset < 0) { ret = l2_offset; goto fail; @@ -305,7 +305,7 @@ static int l2_allocate(BlockDriverState *bs, int l1_index) /* allocate a new entry in the l2 cache */ - slice_size2 = s->l2_slice_size * sizeof(uint64_t); + slice_size2 = s->l2_slice_size * l2_entry_size(s); n_slices = s->cluster_size / slice_size2; trace_qcow2_l2_allocate_get_empty(bs, l1_index); @@ -369,7 +369,7 @@ fail: } s->l1_table[l1_index] = old_l2_offset; if (l2_offset > 0) { - qcow2_free_clusters(bs, l2_offset, s->l2_size * sizeof(uint64_t), + qcow2_free_clusters(bs, l2_offset, s->l2_size * l2_entry_size(s), QCOW2_DISCARD_ALWAYS); } return ret; @@ -718,7 +718,7 @@ static int get_cluster_table(BlockDriverState *bs, uint64_t offset, /* Then decrease the refcount of the old table */ if (l2_offset) { - qcow2_free_clusters(bs, l2_offset, s->l2_size * sizeof(uint64_t), + qcow2_free_clusters(bs, l2_offset, s->l2_size * l2_entry_size(s), QCOW2_DISCARD_OTHER); } @@ -1919,7 +1919,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table, int ret; int i, j; - slice_size2 = s->l2_slice_size * sizeof(uint64_t); + slice_size2 = s->l2_slice_size * l2_entry_size(s); n_slices = s->cluster_size / slice_size2; if (!is_active_l1) { diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 141e4fdcb1..3b89a97fd0 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -1254,7 +1254,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, l2_slice = NULL; l1_table = NULL; l1_size2 = l1_size * sizeof(uint64_t); - slice_size2 = s->l2_slice_size * sizeof(uint64_t); + slice_size2 = s->l2_slice_size * l2_entry_size(s); n_slices = s->cluster_size / slice_size2; s->cache_discards = true; @@ -1605,7 +1605,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res, int i, l2_size, nb_csectors, ret; /* Read L2 table from disk */ - l2_size = s->l2_size * sizeof(uint64_t); + l2_size = s->l2_size * l2_entry_size(s); l2_table = g_malloc(l2_size); ret = bdrv_pread(bs->file, l2_offset, l2_table, l2_size); @@ -1680,15 +1680,16 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res, fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", offset); if (fix & BDRV_FIX_ERRORS) { + int idx = i * (l2_entry_size(s) / sizeof(uint64_t)); uint64_t l2e_offset = - l2_offset + (uint64_t)i * sizeof(uint64_t); + l2_offset + (uint64_t)i * l2_entry_size(s); int ign = active ? QCOW2_OL_ACTIVE_L2 : QCOW2_OL_INACTIVE_L2; l2_entry = QCOW_OFLAG_ZERO; set_l2_entry(s, l2_table, i, l2_entry); ret = qcow2_pre_write_overlap_check(bs, ign, - l2e_offset, sizeof(uint64_t), false); + l2e_offset, l2_entry_size(s), false); if (ret < 0) { fprintf(stderr, "ERROR: Overlap check failed\n"); res->check_errors++; @@ -1698,7 +1699,8 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res, } ret = bdrv_pwrite_sync(bs->file, l2e_offset, - &l2_table[i], sizeof(uint64_t)); + &l2_table[idx], + l2_entry_size(s)); if (ret < 0) { fprintf(stderr, "ERROR: Failed to overwrite L2 " "table entry: %s\n", strerror(-ret)); @@ -1905,7 +1907,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res, } ret = bdrv_pread(bs->file, l2_offset, l2_table, - s->l2_size * sizeof(uint64_t)); + s->l2_size * l2_entry_size(s)); if (ret < 0) { fprintf(stderr, "ERROR: Could not read L2 table: %s\n", strerror(-ret)); diff --git a/block/qcow2.c b/block/qcow2.c index 239e0ad3d9..d3b8581aed 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -870,7 +870,7 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts, uint64_t max_l2_entries = DIV_ROUND_UP(virtual_disk_size, s->cluster_size); /* An L2 table is always one cluster in size so the max cache size * should be a multiple of the cluster size. */ - uint64_t max_l2_cache = ROUND_UP(max_l2_entries * sizeof(uint64_t), + uint64_t max_l2_cache = ROUND_UP(max_l2_entries * l2_entry_size(s), s->cluster_size); combined_cache_size_set = qemu_opt_get(opts, QCOW2_OPT_CACHE_SIZE); @@ -1031,7 +1031,7 @@ static int qcow2_update_options_prepare(BlockDriverState *bs, } } - r->l2_slice_size = l2_cache_entry_size / sizeof(uint64_t); + r->l2_slice_size = l2_cache_entry_size / l2_entry_size(s); r->l2_table_cache = qcow2_cache_create(bs, l2_cache_size, l2_cache_entry_size); r->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size, @@ -1425,7 +1425,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, bs->encrypted = true; } - s->l2_bits = s->cluster_bits - 3; /* L2 is always one cluster */ + s->l2_bits = s->cluster_bits - ctz32(l2_entry_size(s)); s->l2_size = 1 << s->l2_bits; /* 2^(s->refcount_order - 3) is the refcount width in bytes */ s->refcount_block_bits = s->cluster_bits - (s->refcount_order - 3); @@ -4104,7 +4104,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, * preallocation. All that matters is that we will not have to allocate * new refcount structures for them.) */ nb_new_l2_tables = DIV_ROUND_UP(nb_new_data_clusters, - s->cluster_size / sizeof(uint64_t)); + s->cluster_size / l2_entry_size(s)); /* The cluster range may not be aligned to L2 boundaries, so add one L2 * table for a potential head/tail */ nb_new_l2_tables++;