* Re: [Qemu-devel] [PATCH] backup: Fail early if cannot determine cluster size
2016-05-18 5:53 [Qemu-devel] [PATCH] backup: Fail early if cannot determine cluster size Fam Zheng
@ 2016-05-18 13:34 ` Jeff Cody
2016-05-18 14:00 ` John Snow
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Jeff Cody @ 2016-05-18 13:34 UTC (permalink / raw)
To: Fam Zheng
Cc: qemu-devel, Kevin Wolf, Max Reitz, qemu-block, jsnow, qemu-stable
On Wed, May 18, 2016 at 01:53:27PM +0800, Fam Zheng wrote:
> Otherwise the job is orphaned and block_job_cancel_sync in
> bdrv_close_all() when quiting will hang.
>
> A simple reproducer is running blockdev-backup from null-co:// to
> null-co://.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
> ---
> block/backup.c | 34 ++++++++++++++++++----------------
> 1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 491fd14..3ff48c6 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -505,6 +505,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
> int64_t len;
> BlockDriverInfo bdi;
> int ret;
> + int64_t cluster_size;
>
> assert(bs);
> assert(target);
> @@ -568,23 +569,10 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
> goto error;
> }
>
> - BackupBlockJob *job = block_job_create(&backup_job_driver, bs, speed,
> - cb, opaque, errp);
> - if (!job) {
> - goto error;
> - }
> -
> - job->on_source_error = on_source_error;
> - job->on_target_error = on_target_error;
> - job->target = target;
> - job->sync_mode = sync_mode;
> - job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
> - sync_bitmap : NULL;
> -
> /* If there is no backing file on the target, we cannot rely on COW if our
> * backup cluster size is smaller than the target cluster size. Even for
> * targets with a backing file, try to avoid COW if possible. */
> - ret = bdrv_get_info(job->target, &bdi);
> + ret = bdrv_get_info(target, &bdi);
> if (ret < 0 && !target->backing) {
> error_setg_errno(errp, -ret,
> "Couldn't determine the cluster size of the target image, "
> @@ -594,11 +582,25 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
> goto error;
> } else if (ret < 0 && target->backing) {
> /* Not fatal; just trudge on ahead. */
> - job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
> + cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
> } else {
> - job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
> + cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
> }
>
> + BackupBlockJob *job = block_job_create(&backup_job_driver, bs, speed,
> + cb, opaque, errp);
> + if (!job) {
> + goto error;
> + }
> +
> + job->on_source_error = on_source_error;
> + job->on_target_error = on_target_error;
> + job->target = target;
> + job->sync_mode = sync_mode;
> + job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
> + sync_bitmap : NULL;
> + job->cluster_size = cluster_size;
> +
> bdrv_op_block_all(target, job->common.blocker);
> job->common.len = len;
> job->common.co = qemu_coroutine_create(backup_run);
> --
> 2.8.2
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] backup: Fail early if cannot determine cluster size
2016-05-18 5:53 [Qemu-devel] [PATCH] backup: Fail early if cannot determine cluster size Fam Zheng
2016-05-18 13:34 ` Jeff Cody
@ 2016-05-18 14:00 ` John Snow
2016-05-18 14:25 ` Eric Blake
2016-06-14 14:49 ` Max Reitz
3 siblings, 0 replies; 8+ messages in thread
From: John Snow @ 2016-05-18 14:00 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-stable, Max Reitz
On 05/18/2016 01:53 AM, Fam Zheng wrote:
> Otherwise the job is orphaned and block_job_cancel_sync in
> bdrv_close_all() when quiting will hang.
>
Tch, mea culpa.
> A simple reproducer is running blockdev-backup from null-co:// to
> null-co://.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block/backup.c | 34 ++++++++++++++++++----------------
> 1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 491fd14..3ff48c6 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -505,6 +505,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
> int64_t len;
> BlockDriverInfo bdi;
> int ret;
> + int64_t cluster_size;
>
> assert(bs);
> assert(target);
> @@ -568,23 +569,10 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
> goto error;
> }
>
> - BackupBlockJob *job = block_job_create(&backup_job_driver, bs, speed,
> - cb, opaque, errp);
> - if (!job) {
> - goto error;
> - }
> -
> - job->on_source_error = on_source_error;
> - job->on_target_error = on_target_error;
> - job->target = target;
> - job->sync_mode = sync_mode;
> - job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
> - sync_bitmap : NULL;
> -
> /* If there is no backing file on the target, we cannot rely on COW if our
> * backup cluster size is smaller than the target cluster size. Even for
> * targets with a backing file, try to avoid COW if possible. */
> - ret = bdrv_get_info(job->target, &bdi);
> + ret = bdrv_get_info(target, &bdi);
> if (ret < 0 && !target->backing) {
> error_setg_errno(errp, -ret,
> "Couldn't determine the cluster size of the target image, "
> @@ -594,11 +582,25 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
> goto error;
> } else if (ret < 0 && target->backing) {
> /* Not fatal; just trudge on ahead. */
> - job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
> + cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
> } else {
> - job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
> + cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
> }
>
> + BackupBlockJob *job = block_job_create(&backup_job_driver, bs, speed,
> + cb, opaque, errp);
> + if (!job) {
> + goto error;
> + }
> +
> + job->on_source_error = on_source_error;
> + job->on_target_error = on_target_error;
> + job->target = target;
> + job->sync_mode = sync_mode;
> + job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
> + sync_bitmap : NULL;
> + job->cluster_size = cluster_size;
> +
> bdrv_op_block_all(target, job->common.blocker);
> job->common.len = len;
> job->common.co = qemu_coroutine_create(backup_run);
>
Thank you.
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] backup: Fail early if cannot determine cluster size
2016-05-18 5:53 [Qemu-devel] [PATCH] backup: Fail early if cannot determine cluster size Fam Zheng
2016-05-18 13:34 ` Jeff Cody
2016-05-18 14:00 ` John Snow
@ 2016-05-18 14:25 ` Eric Blake
2016-05-19 0:54 ` Fam Zheng
2016-06-14 14:49 ` Max Reitz
3 siblings, 1 reply; 8+ messages in thread
From: Eric Blake @ 2016-05-18 14:25 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-stable, Max Reitz, jsnow
[-- Attachment #1: Type: text/plain, Size: 612 bytes --]
On 05/17/2016 11:53 PM, Fam Zheng wrote:
> Otherwise the job is orphaned and block_job_cancel_sync in
> bdrv_close_all() when quiting will hang.
s/quiting/quitting/
>
> A simple reproducer is running blockdev-backup from null-co:// to
> null-co://.
Worth a qemu-iotests addition?
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block/backup.c | 34 ++++++++++++++++++----------------
> 1 file changed, 18 insertions(+), 16 deletions(-)
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] backup: Fail early if cannot determine cluster size
2016-05-18 14:25 ` Eric Blake
@ 2016-05-19 0:54 ` Fam Zheng
0 siblings, 0 replies; 8+ messages in thread
From: Fam Zheng @ 2016-05-19 0:54 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, Kevin Wolf, qemu-block, Jeff Cody, qemu-stable,
Max Reitz, jsnow
On Wed, 05/18 08:25, Eric Blake wrote:
> On 05/17/2016 11:53 PM, Fam Zheng wrote:
> > Otherwise the job is orphaned and block_job_cancel_sync in
> > bdrv_close_all() when quiting will hang.
>
> s/quiting/quitting/
>
> >
> > A simple reproducer is running blockdev-backup from null-co:// to
> > null-co://.
>
> Worth a qemu-iotests addition?
Sure, as you asked, will send one in addition.
Fam
>
> >
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> > block/backup.c | 34 ++++++++++++++++++----------------
> > 1 file changed, 18 insertions(+), 16 deletions(-)
> >
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] backup: Fail early if cannot determine cluster size
2016-05-18 5:53 [Qemu-devel] [PATCH] backup: Fail early if cannot determine cluster size Fam Zheng
` (2 preceding siblings ...)
2016-05-18 14:25 ` Eric Blake
@ 2016-06-14 14:49 ` Max Reitz
2016-06-15 1:53 ` Fam Zheng
2016-06-15 2:05 ` Fam Zheng
3 siblings, 2 replies; 8+ messages in thread
From: Max Reitz @ 2016-06-14 14:49 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Jeff Cody, Kevin Wolf, qemu-block, jsnow, qemu-stable
[-- Attachment #1: Type: text/plain, Size: 703 bytes --]
On 18.05.2016 07:53, Fam Zheng wrote:
> Otherwise the job is orphaned and block_job_cancel_sync in
> bdrv_close_all() when quiting will hang.
>
> A simple reproducer is running blockdev-backup from null-co:// to
> null-co://.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block/backup.c | 34 ++++++++++++++++++----------------
> 1 file changed, 18 insertions(+), 16 deletions(-)
Sorry for having waited so long (I was thinking that maybe Jeff wanted
to take this patch), but now the patch no longer applies and I don't
feel comfortable with just fixing it up myself; git-backport-diff tells
me the required changes are "[0015] [FC]".
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] backup: Fail early if cannot determine cluster size
2016-06-14 14:49 ` Max Reitz
@ 2016-06-15 1:53 ` Fam Zheng
2016-06-15 2:05 ` Fam Zheng
1 sibling, 0 replies; 8+ messages in thread
From: Fam Zheng @ 2016-06-15 1:53 UTC (permalink / raw)
To: Max Reitz
Cc: qemu-devel, Jeff Cody, Kevin Wolf, qemu-block, jsnow, qemu-stable
On Tue, 06/14 16:49, Max Reitz wrote:
> On 18.05.2016 07:53, Fam Zheng wrote:
> > Otherwise the job is orphaned and block_job_cancel_sync in
> > bdrv_close_all() when quiting will hang.
> >
> > A simple reproducer is running blockdev-backup from null-co:// to
> > null-co://.
> >
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> > block/backup.c | 34 ++++++++++++++++++----------------
> > 1 file changed, 18 insertions(+), 16 deletions(-)
>
> Sorry for having waited so long (I was thinking that maybe Jeff wanted
> to take this patch), but now the patch no longer applies and I don't
> feel comfortable with just fixing it up myself; git-backport-diff tells
> me the required changes are "[0015] [FC]".
No problem! I'll rebase it and submit again.
Fam
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] backup: Fail early if cannot determine cluster size
2016-06-14 14:49 ` Max Reitz
2016-06-15 1:53 ` Fam Zheng
@ 2016-06-15 2:05 ` Fam Zheng
1 sibling, 0 replies; 8+ messages in thread
From: Fam Zheng @ 2016-06-15 2:05 UTC (permalink / raw)
To: Max Reitz
Cc: qemu-devel, Jeff Cody, Kevin Wolf, qemu-block, jsnow, qemu-stable
On Tue, 06/14 16:49, Max Reitz wrote:
> On 18.05.2016 07:53, Fam Zheng wrote:
> > Otherwise the job is orphaned and block_job_cancel_sync in
> > bdrv_close_all() when quiting will hang.
> >
> > A simple reproducer is running blockdev-backup from null-co:// to
> > null-co://.
> >
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> > block/backup.c | 34 ++++++++++++++++++----------------
> > 1 file changed, 18 insertions(+), 16 deletions(-)
>
> Sorry for having waited so long (I was thinking that maybe Jeff wanted
> to take this patch), but now the patch no longer applies and I don't
> feel comfortable with just fixing it up myself; git-backport-diff tells
> me the required changes are "[0015] [FC]".
Oh it turns out this patch is superseded by
commit 91ab68837933232bcef99da7c968e6d41900419b
Author: Kevin Wolf <kwolf@redhat.com>
Date: Thu Apr 14 12:59:55 2016 +0200
backup: Don't leak BackupBlockJob in error path
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
So let's just drop it.
Fam
^ permalink raw reply [flat|nested] 8+ messages in thread