Re: [PATCH v2 07/11] block: add x-blockdev-amend qmp command

[Maxim Levitsky]

On Fri, 2019-11-08 at 11:36 +0100, Max Reitz wrote:
> On 08.11.19 10:26, Maxim Levitsky wrote:
> > On Fri, 2019-10-04 at 20:53 +0200, Max Reitz wrote:
> > > On 13.09.19 00:30, Maxim Levitsky wrote:
> > > > Signed-off-by: Maxim Levitsky <address@hidden>
> > > > ---
> > > >  block/Makefile.objs       |   2 +-
> > > >  block/amend.c             | 116 ++++++++++++++++++++++++++++++++++++++
> > > >  include/block/block_int.h |  23 ++++++--
> > > >  qapi/block-core.json      |  26 +++++++++
> > > >  qapi/job.json             |   4 +-
> > > >  5 files changed, 163 insertions(+), 8 deletions(-)
> > > >  create mode 100644 block/amend.c
> 
> [...]
> 
> > > > +static int coroutine_fn blockdev_amend_run(Job *job, Error **errp)
> > > > +{
> > > > +    BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common);
> > > > +    int ret;
> > > > +
> > > > +    job_progress_set_remaining(&s->common, 1);
> > > > +    ret = s->bs->drv->bdrv_co_amend(s->bs, s->opts, s->force, errp);
> > > > +    job_progress_update(&s->common, 1);
> > > 
> > > It would be nice if the amend job could make use of the progress
> > > reporting that we have in place for amend.
> > 
> > I also thought about it, but is it worth it?
> > 
> > I looked through the status reporting of the qcow2 amend
> > code (which doesn't really allowed to be run through
> > qmp blockdev-amend, due to complexity of changing 
> > the qcow2 format on the fly).
> 
> True, and we could always add it later.
> 
> I suppose I was mostly wondering because bdrv_amend_options already has
> all of that infrastructure and I was assuming that qcow2's bdrv_co_amend
> implementation would make some use of the existing function.  Well, it
> doesn’t, so *shrug*
> 
> [...]
> 
> > > > +    /*
> > > > +     * Create the block job
> > > > +     * TODO Running in the main context. Block drivers need to error 
> > > > out or add
> > > > +     * locking when they use a BDS in a different AioContext.
> > > 
> > > Why shouldn’t the job just run in the node’s context?
> > 
> > This is shameless copy&pasta from the blockdev-create code
> > (which I did note in the copyright of the file)
> 
> Well, you noted that it’s heavily based on it, not that it’s just C&P.

'heavily based' is a polite way to say that file is copied and then changed
to fit new purpose, isn't it :-)


> 
> So I suppose the comment is just wrong here?
Yes, I absolutely missed this part since I don't know the block layer well 
enough.
Thanks for explaining this on IRC, that blockdev_create job is special
in a sense that block device state doesn't exit yet for it (I also was already 
bitten
by this in luks block driver), thus the job runs in main AIO context.
No need to do so in amend, and thus I'll remove that wrongly copy&pasta'ed 
comment.

Thanks a lot!!

Best regards,
        Maxim Levitsky


> 
> Max
>