Re: [PATCH 2/3] qapi: blockdev-backup: add discard-source parameter

[Vladimir Sementsov-Ogievskiy]

Hi Fiona!

That was the only version, because it was based on "[PATCH v5 00/45] Transactional 
block-graph modifying API", as written in commit message.

And "[PATCH v5 00/45] Transactional block-graph modifying API" was not merged, instead I decided to 
send it part-by-part, some were already merged. Now the current "in-flight" part is "[PATCH v8 
0/7] blockdev-replace".

So, probably something from that old big series is still needed for "backup: 
discard-source parameter" to work. Or, may be everything is prepared now.

I'll look at it closely next week and try to make a v2. Thanks for testing and 
debugging!

On 11.01.24 18:19, Fiona Ebner wrote:
> 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

--
Best regards,
Vladimir