From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:59309)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <jsnow@redhat.com>) id 1YdiPR-0001RI-NM
	for qemu-devel@nongnu.org; Thu, 02 Apr 2015 12:56:02 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <jsnow@redhat.com>) id 1YdiPP-0002wH-16
	for qemu-devel@nongnu.org; Thu, 02 Apr 2015 12:56:01 -0400
Message-ID: <551D7498.2050100@redhat.com>
Date: Thu, 02 Apr 2015 12:55:52 -0400
From: John Snow <jsnow@redhat.com>
MIME-Version: 1.0
References: <1426879023-18151-1-git-send-email-jsnow@redhat.com>
	<1426879023-18151-11-git-send-email-jsnow@redhat.com>
	<20150402124413.GE25244@stefanha-thinkpad.redhat.com>
In-Reply-To: <20150402124413.GE25244@stefanha-thinkpad.redhat.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v4 10/20] qmp: Add support of
 "dirty-bitmap" sync mode for drive-backup
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Stefan Hajnoczi <stefanha@gmail.com>
Cc: famz@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org, armbru@redhat.com, vsementsov@parallels.com, stefanha@redhat.com, mreitz@redhat.com



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.