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

[Sergio Lopez]

On Mon, Feb 25, 2019 at 03:01:22PM +0100, Kevin Wolf wrote:
> 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.

Thanks for the hint. I'm going to give it a try and I'll send an early
RFC patchset to put it in common and confirm we're moving in the right
direction.

Sergio.