[Libguestfs] [PATCH v6] v2v: Add -o rhv-upload output mode.

[Richard W.M. Jones]

On Wed, Mar 21, 2018 at 08:08:37PM +0000, Nir Soffer wrote:
> > +    http.putheader("Authorization", transfer.signed_ticket)
> > +    # The oVirt server only uses the first part of the range, and the
> > +    # content-length.
> > +    http.putheader("Content-Range", "bytes %d-%d/*" % (offset,
> > offset+count-1))
> > +    http.putheader("Content-Length", str(count))
> > +    http.endheaders()
> > +    http.send(buf)
> >
> 
> What is the size of the typical buffers? sending new request for every
> small buffer is not efficient.

There is a performance issue currently, although I suspect it's not to
do with this since we send quite large buffers.  In any case combining
requests is going to complicate the code and I'm not very confident I
can do it without screwing up somewhere.

> > +
> > +    r = http.getresponse()
> > +    if r.status != 200:
> > +        transfer_service.pause()
> > +        h['failed'] = True
> > +        raise RuntimeError("could not write sector (%d, %d): %d: %s" %
> > +                           (offset, count, r.status, r.reason))
> 
> +
> > +def zero(h, count, offset, may_trim):
> >
> 
> What is may_trim? trim can never replace zero.

As Eric said.  We're ignoring this parameter which seems like the
right thing to do unless we can guarantee that trimmed data is read
back as zeroes.

> > +    # so nbdkit could call this even if the server doesn't support
> > +    # zeroing.  If this is the case we must emulate.
> > +    if not h['can_zero']:
> > +        emulate_zero_with_pwrite(h, count, offset)
> > +        return
> > +
> > +    # Construct the JSON request for zeroing.
> > +    buf = json.dumps({'op': "zero",
> > +                      'offset': offset,
> > +                      'size': count,
> > +                      'flush': False})
> >
> 
> Is this intended? in cover letter you worte that no flush or trim
> are done. Does qemu-img flush at the end?

Cover letter refers to the current behaviour where we don't have an
imageio server that supports zero, trim or flush.

> > +# qemu-img convert starts by trying to zero/trim the whole device.
> > +# Since we've just created a new disk it's safe to ignore these
> > +# requests as long as they are smaller than the highest write seen.
> > +# After that we must emulate them with writes.
> >
> 
> Isn't this needed also for the real zero? In the first version it will not
> be very efficient. We don't want to zero the entire image.

Can't the server use ioctl(BLKZEROOUT) to implement fast zeroing?

> If qemu-img wants to trim, better call trim - it will be very fast because
> it will
> do nothing in the first version :-)

We don't control what qemu-img does now or in the future.  Also
different output drivers and formats could behave differently.

> > +def flush(h):
> > +    http = h['http']
> > +    transfer=h['transfer']
> > +    transfer_service=h['transfer_service']
> > +
> > +    # Construct the JSON request for flushing.  Note the offset
> > +    # and count must be present but are ignored by imageio.
> > +    buf = json.dumps({'op': "flush",
> > +                      'offset': 0,
> > +                      'size': 0,
> > +                      'flush': True})
> >
> 
> flush does not accept any arguments - it will not fail, but better not have
> code that has no effect.

I didn't understand this.  Do you mean ‘'flush'’ key doesn't have a
parameter?  The documentation seems to indicate that what I'm doing
above is correct so maybe the docs are wrong or unclear.

> > +def close(h):
> > +    http = h['http']
> > +    connection = h['connection']
> > +
> > +    http.close()
> >
> 
> If qemu-img does not flush at the end, we can ensure flush here.
> Maybe keep h["dirty"] flag, setting to True after very write/zero/trim,
> and setting the False after flush?

This is more of a philosophical question.  The nbdkit plugin just
follows flush requests issued by the client (ie. qemu-img).  qemu-img
probably won't issue its own flush request.  Should it issue a flush
or is that the job of oVirt?

> > +    # Otherwise if we did fail then we should delete the disk.
> > +    else:
> > +        disk_service = h['disk_service']
> > +        disk_service.remove()
> >
> 
> This should be done only if we created the disk - is this always the case?

Yes.  If we failed to create a disk, open() fails, and close() is
never called.

I made all the other changes you suggested, see upcoming v7 patch.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v