On Thu 09 Apr 2020 12:59:30 PM CEST, Vladimir Sementsov-Ogievskiy wrote: >> static void calculate_l2_meta(BlockDriverState *bs, >> uint64_t host_cluster_offset, >> uint64_t guest_offset, unsigned bytes, >> - QCowL2Meta **m, bool keep_old) >> + uint64_t *l2_slice, QCowL2Meta **m, bool >> keep_old) >> { >> BDRVQcow2State *s = bs->opaque; >> - unsigned cow_start_from = 0; >> + int l2_index = offset_to_l2_slice_index(s, guest_offset); >> + uint64_t l2_entry; >> + unsigned cow_start_from, cow_end_to; >> unsigned cow_start_to = offset_into_cluster(s, guest_offset); >> unsigned cow_end_from = cow_start_to + bytes; >> - unsigned cow_end_to = ROUND_UP(cow_end_from, s->cluster_size); >> unsigned nb_clusters = size_to_clusters(s, cow_end_from); >> QCowL2Meta *old_m = *m; >> + QCow2ClusterType type; >> + >> + assert(nb_clusters <= s->l2_slice_size - l2_index); >> + >> + /* Return if there's no COW (all clusters are normal and we keep them) >> */ >> + if (keep_old) { >> + int i; >> + for (i = 0; i < nb_clusters; i++) { >> + l2_entry = be64_to_cpu(l2_slice[l2_index + i]); >> + if (qcow2_get_cluster_type(bs, l2_entry) != >> QCOW2_CLUSTER_NORMAL) { > > Could we also allow full ZERO_ALLOC clusters here?
No, because the L2 entry needs to be modified (in order to remove the 'all zeroes' bit) and we need to create a QCowL2Meta entry for that (see qcow2_handle_l2meta()). >> + /* Get the L2 entry of the first cluster */ >> + l2_entry = be64_to_cpu(l2_slice[l2_index]); >> + type = qcow2_get_cluster_type(bs, l2_entry); >> + >> + if (type == QCOW2_CLUSTER_NORMAL && keep_old) { >> + cow_start_from = cow_start_to; >> + } else { >> + cow_start_from = 0; >> + } >> + >> + /* Get the L2 entry of the last cluster */ >> + l2_entry = be64_to_cpu(l2_slice[l2_index + nb_clusters - 1]); >> + type = qcow2_get_cluster_type(bs, l2_entry); >> + >> + if (type == QCOW2_CLUSTER_NORMAL && keep_old) { >> + cow_end_to = cow_end_from; >> + } else { >> + cow_end_to = ROUND_UP(cow_end_from, s->cluster_size); >> + } > > These two ifs may be moved into if (keep_old), and drop "&& keep_old" > from conditions. This also will allow to drop extra calculations, move > new variables to if (keep_old) {} block and allow to pass > l2_slice=NULL together with keep_old=false. In subsequent patches we're going to have more cases than just QCOW2_CLUSTER_NORMAL so I don't think it makes sense to move the keep_old check around. >> @@ -1239,7 +1304,8 @@ static int handle_copied(BlockDriverState *bs, >> uint64_t guest_offset, >> >> l2_index = offset_to_l2_slice_index(s, guest_offset); >> nb_clusters = MIN(nb_clusters, s->l2_slice_size - l2_index); >> - assert(nb_clusters <= INT_MAX); >> + /* Limit total byte count to BDRV_REQUEST_MAX_BYTES */ >> + nb_clusters = MIN(nb_clusters, BDRV_REQUEST_MAX_BYTES >> >> s->cluster_bits); >> >> /* Find L2 entry for the first involved cluster */ >> ret = get_cluster_table(bs, guest_offset, &l2_slice, &l2_index); >> @@ -1249,18 +1315,17 @@ static int handle_copied(BlockDriverState *bs, >> uint64_t guest_offset, >> >> cluster_offset = be64_to_cpu(l2_slice[l2_index]); > > It would be good to s/cluster_offset/l2_entry/ > > And, "cluster_offset & L2E_OFFSET_MASK" is used so many times, so, I'd > not substitute, but keep both variables: l2_entry and cluster_offset. Sounds good, I can change that. >> + /* Allocate at a given offset in the image file */ >> + alloc_cluster_offset = *host_offset == INV_OFFSET ? INV_OFFSET : >> + start_of_cluster(s, *host_offset); >> + ret = do_alloc_cluster_offset(bs, guest_offset, &alloc_cluster_offset, >> + &nb_clusters); >> + if (ret < 0) { >> + goto out; >> } >> >> - qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice); > > actually we don't need l2_slice for keep_old=false in > calculate_l2_meta, so if calculate_l2_meta modified a bit, change of > function tail is not needed.. > > Still, may be l2_slice will be used in calculate_l2_meta() in further > patches? Will see.. We'll need it in a later patch. >> -fail: >> - if (*m && (*m)->nb_clusters > 0) { >> +out: >> + qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice); >> + if (ret < 0 && *m && (*m)->nb_clusters > 0) { >> QLIST_REMOVE(*m, next_in_flight); >> } > > Hmm, unrelated to the patch, but why do we remove meta, which we > didn't create? Not sure actually, I would need to check further... Berto