Re: [Qemu-devel] [PATCH] qemu-img: implement copy offload (-C) for dd

[Kevin Wolf]

Am 25.02.2019 um 14:40 hat Sergio Lopez geschrieben:
> On Fri, Feb 22, 2019 at 11:04:25AM +0100, Kevin Wolf wrote:
> > Am 21.02.2019 um 20:32 hat Sergio Lopez geschrieben:
> > > On Thu, Feb 21, 2019 at 12:08:12PM -0600, Eric Blake wrote:
> > > > On 2/21/19 11:37 AM, Sergio Lopez wrote:
> > > > > This parameter is analogous to convert's "-C", making use of
> > > > > bdrv_co_copy_range().
> > > > 
> > > > The last time I tried to patch 'qemu-img dd', it was pointed out that it
> > > > already has several bugs (where it is not on feature-parity with real
> > > > dd), and that we REALLY want to make it a syntactic sugar wrapper around
> > > > 'qemu-img convert', rather than duplicating code (which means that
> > > > qemu-img convert needs to make it easier to do arbitrary offsets and
> > > > subsets - although to some extent you can already do that with
> > > > --image-opts and appropriate raw driver wrappers).
> > > > 
> > > > https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg02618.html
> > > 
> > > Interesting, I wasn't aware of that conversation. It might a little
> > > late to go again through it, but while I don't a strong opinion about
> > > it, I do have some reservations about the idea of making 'dd' a
> > > frontend for 'convert'.
> > > 
> > > While I do see the functional similarity of both commands, to me they
> > > are quite different at a semantical level. For 'convert', I do expect
> > > it to do "the right thing" and use the optimal settings (i.e. choosing
> > > the best transfer size) by default, while 'dd' is more of "do whatever
> > > the user told you to do no matter how wrong it is".
> > 
> > I think that's what defaults are for. It would make sense to allow the
> > user to specify the buffer size even for 'qemu-img convert'. In fact, we
> > already have a variable for this, we'd just have to add an option to set
> > it explicitly instead of just relying on what the output block node
> > tells us.
> > 
> > > Due to this differences, I think turning 'convert' code into something
> > > able to deal with 'dd' semantics would imply adding a considerable
> > > number of conditionals, possibly making it harder to maintain than
> > > keeping it separate.
> > 
> > 'qemu-img dd' currently supports five options:
> > 
> > * if and of. These are obviously supported for convert, too.
> > * count and skip. We don't have these in convert yet. We could either
> >   add the functionality there or add a raw layer in the 'dd'
> >   implementation before we call into the common code.
> > * bs. The buffer size is already configurable in ImgConvertState.
> > 
> > So getting this functionality into 'convert' should be easy.
> > 
> > There are more differences between 'convert' and 'dd' in how exactly the
> > copy is done. I'm not sure whether there is actually a good use for the
> > dumb 'dd' copying or whether it was just implemented this way because it
> > was easier.
> > 
> > Currently we have features like copying only a given range only in 'dd',
> > and most other features like zero detection, dealing with backing files,
> > compression, copy offloading or parallel out-of-order copy only in
> > 'convert'.
> > 
> > Actually, we have more than just these two places that copy images: We
> > also have the mirror block job and for other special cases also the
> > other block jobs that copy data, each with its own list of features and
> > missing options.
> > 
> > What I really want to see eventually is a way to have all features
> > available in all of these instead of only a random selection where
> > you're out of luck if you want to copy part of an image with compressed
> > output while the VM is running because these three are features
> > supported in three different copy implementations and you can't get more
> > than one of the features at the same time.
> 
> OK, makes sense. Is someone already working on this?

I don't think anyone has found the time yet.

If you want to start work on it, maybe the best way to begin
incrementally would be to factor out some common functionality from the
block jobs into a new block/copy.c, from which we can later create an
actual unified 'copy' block job.

Or it might turn out that another way works better. I'm only talking
theory here, I haven't actually tried how well it works.

Kevin