[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/3] qapi: blockdev-backup: add discard-source parameter
From: |
Fiona Ebner |
Subject: |
Re: [PATCH 2/3] qapi: blockdev-backup: add discard-source parameter |
Date: |
Thu, 11 Jan 2024 16:19:52 +0100 |
User-agent: |
Mozilla Thunderbird |
Hi Vladimir,
hope I didn't miss a newer version of this series. I'm currently
evaluating fleecing backup for Proxmox downstream, so I pulled in this
series and wanted to let you know about two issues I encountered while
testing. We are still based on 8.1, but if I'm not mistaken, they are
still relevant:
Am 31.03.22 um 21:57 schrieb Vladimir Sementsov-Ogievskiy:
> @@ -575,6 +577,10 @@ static coroutine_fn int block_copy_task_entry(AioTask
> *task)
> co_put_to_shres(s->mem, t->req.bytes);
> block_copy_task_end(t, ret);
>
> + if (s->discard_source && ret == 0) {
> + bdrv_co_pdiscard(s->source, t->req.offset, t->req.bytes);
> + }
> +
> return ret;
> }
>
If the image size is not aligned to the cluster size, passing
t->req.bytes when calling bdrv_co_pdiscard() can lead to an assertion
failure at the end of the image:
> kvm: ../block/io.c:1982: bdrv_co_write_req_prepare: Assertion `offset + bytes
> <= bs->total_sectors * BDRV_SECTOR_SIZE || child->perm & BLK_PERM_RESIZE'
> failed.
block_copy_do_copy() does have a line to clamp down:
> int64_t nbytes = MIN(offset + bytes, s->len) - offset;
If I do the same before calling bdrv_co_pdiscard(), the failure goes away.
For the second one, the following code saw some changes since the series
was sent:
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index 79cf12380e..3e77313a9a 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -319,7 +319,7 @@ static void cbw_child_perm(BlockDriverState *bs,
> BdrvChild *c,
> bdrv_default_perms(bs, c, role, reopen_queue,
> perm, shared, nperm, nshared);
>
> - *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
> + *nperm = *nperm | BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE;
> *nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> }
> }
It's now:
> bdrv_default_perms(bs, c, role, reopen_queue,
> perm, shared, nperm, nshared);
>
> if (!QLIST_EMPTY(&bs->parents)) {
> if (perm & BLK_PERM_WRITE) {
> *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
> }
> *nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> }
So I wasn't sure how to adapt the patch:
- If setting BLK_PERM_WRITE unconditionally, it seems to break usual
drive-backup (with no fleecing set up):
> permissions 'write' are both required by node '#block691' (uses node
> '#block151' as 'file' child) and unshared by block device 'drive-scsi0' (uses
> node '#block151' as 'root' child).
- If I only do it within the if block, it doesn't work when I try to set
up fleecing, because bs->parents is empty for me, i.e. when passing the
snapshot-access node to backup_job_create() while the usual cbw for
backup is appended. I should note I'm doing it manually in a custom QMP
command, not in a transaction (which requires the not-yet-merged
blockdev-replace AFAIU).
Not sure if I'm doing something wrong, but maybe what you wrote in the
commit message is necessary after all?
> Alternative is to pass
> an option to bdrv_cbw_append(), add some internal open-option for
> copy-before-write filter to require WRITE permission only for backup
> with discard-source=true. But I'm not sure it worth the complexity.
Best Regards,
Fiona
- Re: [PATCH 2/3] qapi: blockdev-backup: add discard-source parameter,
Fiona Ebner <=