Re: [Qemu-devel] [PATCH v2 09/11] block/backup: teach TOP to never copy unallocated regions

From: Max Reitz <mreitz@redhat.com>
To: John Snow <jsnow@redhat.com>,
	qemu-block@nongnu.org, qemu-devel@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>,
	vsementsov@virtuozzo.com, Markus Armbruster <armbru@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v2 09/11] block/backup: teach TOP to never copy unallocated regions
Date: Tue, 16 Jul 2019 13:43:52 +0200	[thread overview]
Message-ID: <1e0e9638-6d60-7343-e6db-a1e77b5da932@redhat.com> (raw)
In-Reply-To: <20190716000117.25219-10-jsnow@redhat.com>

[-- Attachment #1.1: Type: text/plain, Size: 1938 bytes --]

On 16.07.19 02:01, John Snow wrote:
> Presently, If sync=TOP is selected, we mark the entire bitmap as dirty.
> In the write notifier handler, we dutifully copy out such regions.
> 
> Fix this in three parts:
> 
> 1. Mark the bitmap as being initialized before the first yield.
> 2. After the first yield but before the backup loop, interrogate the
> allocation status asynchronously and initialize the bitmap.
> 3. Teach the write notifier to interrogate allocation status if it is
> invoked during bitmap initialization.
> 
> As an effect of this patch, the job progress for TOP backups
> now behaves like this:
> 
> - total progress starts at bdrv_length.
> - As allocation status is interrogated, total progress decreases.
> - As blocks are copied, current progress increases.
> 
> Taken together, the floor and ceiling move to meet each other.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/backup.c     | 78 ++++++++++++++++++++++++++++++++++++++++------
>  block/trace-events |  1 +
>  2 files changed, 70 insertions(+), 9 deletions(-)

Looks good to me but for a seemingly unrelated change:

> diff --git a/block/backup.c b/block/backup.c
> index b407d57954..e28fd23f6a 100644
> --- a/block/backup.c
> +++ b/block/backup.c

[...]

> @@ -507,10 +565,12 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
>               * notify callback service CoW requests. */
>              job_yield(job);
>          }
> +        ret = -ECANCELED;

This one.  This doesn’t look like it belongs in this patch, and I’m not
even sure it’s correct.  Being cancelled is the normal state for
sync=none, so I suppose it is correct to just return 0 then.

Max

>      } else {
>          ret = backup_loop(s);
>      }
>  
> + out:
>      notifier_with_return_remove(&s->before_write);
>  
>      /* wait until pending backup_do_cow() calls have completed */

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]