<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Apr 10, 2018 at 5:50 PM Richard W.M. Jones <<a href="mailto:rjones@redhat.com">rjones@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, Apr 10, 2018 at 02:07:33PM +0000, Nir Soffer wrote:<br>
> This makes sense if the device is backed by a block device on oVirt side,<br>
> and the NBD support efficient zeroing. But in this case the device is backed<br>
> by an empty sparse file on NFS, and oVirt does not support yet efficient<br>
> zeroing, we just write zeros manually.<br>
><br>
> I think should be handled on virt-v2v plugin side. When zeroing a file raw<br>
> image,<br>
> you can ignore zero requests after the highest write offset, since the<br>
> plugin<br>
> created a new image, and we know that the image is empty.<br>
><br>
> When the destination is a block device we cannot avoid zeroing since a block<br>
> device may contain junk data (we usually get dirty empty images from our<br>
> local<br>
> xtremio server).<br>
<br>
(Off topic for qemu-block but ...)  We don't have enough information<br>
at our end to know about any of this.<br></blockquote><div><br></div><div>Can't use use this logic in the oVirt plugin?</div><div><br></div><div>file based storage -> skip initial zeroing</div><div>block based storage -> use initial zeroing</div><div><br></div><div>Do you think that publishing disk capabilities in the sdk will solve this?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> > The problem is that the NBD block driver has max_pwrite_zeroes = 32 MB,<br>
> > so it's not that efficient after all. I'm not sure if there is a real<br>
> > reason for this, but Eric should know.<br>
> ><br>
><br>
> We support zero with unlimited size without sending any payload to oVirt,<br>
> so<br>
> there is no reason to limit zero request by max_pwrite_zeros. This limit may<br>
> make sense when zero is emulated using pwrite.<br>
<br>
Yes, this seems wrong, but I'd want Eric to comment.<br>
<br>
> > > However, since you suggest that we could use "trim" request for these<br>
> > > requests, it means that these requests are advisory (since trim is), and<br>
> > > we can just ignore them if the server does not support trim.<br>
> ><br>
> > What qemu-img sends shouldn't be a NBD_CMD_TRIM request (which is indeed<br>
> > advisory), but a NBD_CMD_WRITE_ZEROES request. qemu-img relies on the<br>
> > image actually being zeroed after this.<br>
> ><br>
><br>
> So it seems that may_trim=1 is wrong, since trim cannot replace zero.<br>
<br>
Note that the current plugin ignores may_trim.  It is not used at all,<br>
so it's not relevant to this problem.<br>
<br>
However this flag actually corresponds to the inverse of<br>
NBD_CMD_FLAG_NO_HOLE which is defined by the NBD spec as:<br>
<br>
    bit 1, NBD_CMD_FLAG_NO_HOLE; valid during<br>
    NBD_CMD_WRITE_ZEROES. SHOULD be set to 1 if the client wants to<br>
    ensure that the server does not create a hole. The client MAY send<br>
    NBD_CMD_FLAG_NO_HOLE even if NBD_FLAG_SEND_TRIM was not set in the<br>
    transmission flags field. The server MUST support the use of this<br>
    flag if it advertises NBD_FLAG_SEND_WRITE_ZEROES. *<br>
<br>
qemu-img convert uses NBD_CMD_WRITE_ZEROES and does NOT set this flag<br>
(hence in the plugin we see may_trim=1), and I believe that qemu-img<br>
is correct because it doesn't want to force preallocation.<br></blockquote><div><br></div><div>So once oVirt will support efficient zeroing, this flag may be translated to</div><div>(for file based storage):</div><div><br></div><div>may_trim=1 -> fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE)</div><div>may_trim=0 -> fallocate(FALLOC_FL_ZERO_RANGE | FALLOC_FL_KEEP_SIZE)</div><div><br></div><div>We planed to choose this by default on oVirt side, based on disk type. For preallocated</div><div>disk we never want to use FALLOC_FL_PUNCH_HOLE, and for sparse disk we always</div><div>want to use FALLOC_FL_PUNCH_HOLE unless it is not supported.</div><div><br></div><div>Seems that we need to add a "trim" or "punch_hole" flag to the PATCH/zero request,</div><div>so you can hint oVirt how do you want to zero. oVirt will choose what to do based</div><div>on storage type (file/block), user request(trim/notrim), and disk type (thin/preallocated).</div><div><br></div><div>I think we can start the use this flag when we publish the "trim" feature.</div><div><br></div><div>Nir</div></div></div>