Re: [Qemu-block] [PATCH v4 10/20] qmp: Add support of dirty-bitmap sync mode for drive-backup
On 04/02/2015 08:44 AM, Stefan Hajnoczi wrote:
On Fri, Mar 20, 2015 at 03:16:53PM -0400, John Snow wrote:
+} else if (job-sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+/* Dirty Bitmap sync has a slightly different iteration method */
+HBitmapIter hbi;
+int64_t sector;
+int64_t cluster;
+int64_t last_cluster = -1;
+bool polyrhythmic;
+
+bdrv_dirty_iter_init(bs, job-sync_bitmap, hbi);
+/* Does the granularity happen to match our backup cluster size? */
+polyrhythmic = (bdrv_dirty_bitmap_granularity(job-sync_bitmap) !=
+BACKUP_CLUSTER_SIZE);
+
+/* Find the next dirty /sector/ and copy that /cluster/ */
+while ((sector = hbitmap_iter_next(hbi)) != -1) {
+cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
+
+/* Fake progress updates for any clusters we skipped,
+ * excluding this current cluster. */
+if (cluster != last_cluster + 1) {
+job-common.offset += ((cluster - last_cluster - 1) *
+ BACKUP_CLUSTER_SIZE);
+}
+
+if (yield_and_check(job)) {
+goto leave;
+}
+
+do {
+ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER,
+BACKUP_SECTORS_PER_CLUSTER,
error_is_read);
+if ((ret 0)
+backup_error_action(job, error_is_read, -ret) ==
+BLOCK_ERROR_ACTION_REPORT) {
+goto leave;
+}
+} while (ret 0);
+
+/* Advance (or rewind) our iterator if we need to. */
+if (polyrhythmic) {
+bdrv_set_dirty_iter(hbi,
+(cluster + 1) *
BACKUP_SECTORS_PER_CLUSTER);
+}
+
+last_cluster = cluster;
+}
What happens when the dirty bitmap granularity is larger than
BACKUP_SECTORS_PER_CLUSTER?
|-bitmap granularity-|
|---backup cluster---|
~~~
Will these sectors ever be copied?
I think this case causes an infinite loop:
cluster = hbitmap_iter_next(hbi) / BACKUP_SECTORS_PER_CLUSTER
The iterator is reset:
bdrv_set_dirty_iter(hbi, (cluster + 1) * BACKUP_SECTORS_PER_CLUSTER);
So we get the same cluster ever time and never advance?
I had mistakenly thought that if I advanced to the middle of a
granularity grouping that the iterator might return the next (virtual)
index to me, instead of the beginning of the current group.
Anyway, I've fixed this up a bit and added in some granularity variance
tests for the iotests to test what happens if the granularity is larger,
equal, or smaller.
v5 is ready to send out, but I need to test it first, so I will probably
send that out Wednesday night.
Thanks,
--js
Re: [Qemu-block] [PATCH v4 10/20] qmp: Add support of dirty-bitmap sync mode for drive-backup
On Fri, Mar 20, 2015 at 03:16:53PM -0400, John Snow wrote:
+} else if (job-sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+/* Dirty Bitmap sync has a slightly different iteration method */
+HBitmapIter hbi;
+int64_t sector;
+int64_t cluster;
+int64_t last_cluster = -1;
+bool polyrhythmic;
+
+bdrv_dirty_iter_init(bs, job-sync_bitmap, hbi);
+/* Does the granularity happen to match our backup cluster size? */
+polyrhythmic = (bdrv_dirty_bitmap_granularity(job-sync_bitmap) !=
+BACKUP_CLUSTER_SIZE);
+
+/* Find the next dirty /sector/ and copy that /cluster/ */
+while ((sector = hbitmap_iter_next(hbi)) != -1) {
+cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
+
+/* Fake progress updates for any clusters we skipped,
+ * excluding this current cluster. */
+if (cluster != last_cluster + 1) {
+job-common.offset += ((cluster - last_cluster - 1) *
+ BACKUP_CLUSTER_SIZE);
+}
+
+if (yield_and_check(job)) {
+goto leave;
+}
+
+do {
+ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER,
+BACKUP_SECTORS_PER_CLUSTER,
error_is_read);
+if ((ret 0)
+backup_error_action(job, error_is_read, -ret) ==
+BLOCK_ERROR_ACTION_REPORT) {
+goto leave;
+}
+} while (ret 0);
+
+/* Advance (or rewind) our iterator if we need to. */
+if (polyrhythmic) {
+bdrv_set_dirty_iter(hbi,
+(cluster + 1) *
BACKUP_SECTORS_PER_CLUSTER);
+}
+
+last_cluster = cluster;
+}
What happens when the dirty bitmap granularity is larger than
BACKUP_SECTORS_PER_CLUSTER?
|-bitmap granularity-|
|---backup cluster---|
~~~
Will these sectors ever be copied?
I think this case causes an infinite loop:
cluster = hbitmap_iter_next(hbi) / BACKUP_SECTORS_PER_CLUSTER
The iterator is reset:
bdrv_set_dirty_iter(hbi, (cluster + 1) * BACKUP_SECTORS_PER_CLUSTER);
So we get the same cluster ever time and never advance?
pgpDXGmrHmKck.pgp
Description: PGP signature
Re: [Qemu-block] [PATCH v4 10/20] qmp: Add support of dirty-bitmap sync mode for drive-backup
On 04/02/2015 08:44 AM, Stefan Hajnoczi wrote:
On Fri, Mar 20, 2015 at 03:16:53PM -0400, John Snow wrote:
+} else if (job-sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+/* Dirty Bitmap sync has a slightly different iteration method */
+HBitmapIter hbi;
+int64_t sector;
+int64_t cluster;
+int64_t last_cluster = -1;
+bool polyrhythmic;
+
+bdrv_dirty_iter_init(bs, job-sync_bitmap, hbi);
+/* Does the granularity happen to match our backup cluster size? */
+polyrhythmic = (bdrv_dirty_bitmap_granularity(job-sync_bitmap) !=
+BACKUP_CLUSTER_SIZE);
+
+/* Find the next dirty /sector/ and copy that /cluster/ */
+while ((sector = hbitmap_iter_next(hbi)) != -1) {
+cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
+
+/* Fake progress updates for any clusters we skipped,
+ * excluding this current cluster. */
+if (cluster != last_cluster + 1) {
+job-common.offset += ((cluster - last_cluster - 1) *
+ BACKUP_CLUSTER_SIZE);
+}
+
+if (yield_and_check(job)) {
+goto leave;
+}
+
+do {
+ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER,
+BACKUP_SECTORS_PER_CLUSTER,
error_is_read);
+if ((ret 0)
+backup_error_action(job, error_is_read, -ret) ==
+BLOCK_ERROR_ACTION_REPORT) {
+goto leave;
+}
+} while (ret 0);
+
+/* Advance (or rewind) our iterator if we need to. */
+if (polyrhythmic) {
+bdrv_set_dirty_iter(hbi,
+(cluster + 1) *
BACKUP_SECTORS_PER_CLUSTER);
+}
+
+last_cluster = cluster;
+}
What happens when the dirty bitmap granularity is larger than
BACKUP_SECTORS_PER_CLUSTER?
|-bitmap granularity-|
|---backup cluster---|
~~~
Will these sectors ever be copied?
I think this case causes an infinite loop:
cluster = hbitmap_iter_next(hbi) / BACKUP_SECTORS_PER_CLUSTER
The iterator is reset:
bdrv_set_dirty_iter(hbi, (cluster + 1) * BACKUP_SECTORS_PER_CLUSTER);
So we get the same cluster ever time and never advance?
That is indeed a bug. Tracking to the middle of a granularity group will
return the index of the group you're in the middle of, not the next group.
Thanks for catching this.
