23.01.2019 17:36, Eric Blake wrote: > On 1/23/19 2:20 AM, Vladimir Sementsov-Ogievskiy wrote: > >>>>>>>> + hbitmap_set(job->copy_bitmap, cluster, last_cluster - cluster >>>>>>>> + 1); >>>>>>> >>>>>>> Why the +1? Shouldn't the division for last_cluster round up instead? >>>>>>> >>>>>>>> + >>>>>>>> + offset = (last_cluster + 1) * job->cluster_size; >>>>>>> >>>>>>> Same here. >>>>>> >>>>>> last cluster is not "end", but it's last dirty cluster. so number of >>>>>> dirty clusters is last_cluster - cluster + 1, and next offset is >>>>>> calculated through +1 too. >>>>>> >>>>>> If I round up division result, I'll get last for most cases, but "end" >>>>>> (next after the last), for the case when offset % job->cluster_size == >>>>>> 0, so, how to use it? >>>>> >>>>> Doesn't bdrv_dirty_bitmap_next_dirty_area() return a range [offset, >>>>> offset + bytes), i.e. where "offset + bytes" is the first clean offset? >>>> >>>> oops, you are right. then I need >>>> uint64_t last_cluster = (offset + bytes - 1) / job->cluster_size; >>> >>> That, or you just use a rounding up division and rename it from >>> last_cluster to end_cluster or first_clean_cluster or something (and >>> subsequently drop the +1s). >> >> This will not work, as ((offset + bytes) / job->cluster_size) is not first >> clean cluster >> or end cluster. It's a cluster, where is first clean bit located, but it may >> have dirty >> bits too (or, may not). >> >> So, to rewrite based on end_cluster, it should be calculated as >> >> (offset + bytes - 1) / job->cluster_size + 1 >> >> and, I'm going to do so, one "+1" instead of two, and, may be, a bit more >> understandable. > > Better is to use the macros in osdep.h, such as QEMU_ALIGN_UP/DOWN, to > make the intent of the code easier to read than having to closely check > which operation is being performed to see if it makes sense in context. >
Yes, thanks. Exactly: uint64_t end_cluster = DIV_ROUND_UP(offset + bytes, job->cluster_size); -- Best regards, Vladimir
