<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Thu, Mar 22, 2018 at 5:06 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">> > +<br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> > + r = http.getresponse()<br>
> > + if r.status != 200:<br>
> > + transfer_service.pause()<br>
> > + h['failed'] = True<br>
> > + raise RuntimeError("could not write sector (%d, %d): %d: %s" %<br>
> > + (offset, count, r.status, r.reason))<br>
><br>
> +<br>
> > +def zero(h, count, offset, may_trim):<br>
> ><br>
><br>
> What is may_trim? trim can never replace zero.<br>
<br>
As Eric said. We're ignoring this parameter which seems like the<br>
right thing to do unless we can guarantee that trimmed data is read<br>
back as zeroes.<br></blockquote><div><br></div><div>I don't think we will be able to provide this guarantee for trim, only best</div><div>effort trim that may also do nothing on some images.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> > +# qemu-img convert starts by trying to zero/trim the whole device.<br>
> > +# Since we've just created a new disk it's safe to ignore these<br>
> > +# requests as long as they are smaller than the highest write seen.<br>
> > +# After that we must emulate them with writes.<br>
> ><br>
><br>
> Isn't this needed also for the real zero? In the first version it will not<br>
> be very efficient. We don't want to zero the entire image.<br>
<br>
Can't the server use ioctl(BLKZEROOUT) to implement fast zeroing?<br></blockquote><div><br></div><div>It will but I cannot promise it for 4.2.3. My goal is complete API with </div><div>minimal implementation, and improving later as needed.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> If qemu-img wants to trim, better call trim - it will be very fast because<br>
> it will<br>
> do nothing in the first version :-)<br>
<br>
We don't control what qemu-img does now or in the future. Also<br>
different output drivers and formats could behave differently.<br></blockquote><div><br></div><div>Sure, but if qemu wants to trim, why use zero?</div><div><br></div><div>Since BLKDISCARD may or may not cause future reads to return zeros,</div><div>I assume that qemu-img is not using trim as a replacement for zeroing</div><div>data.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> > +def flush(h):<br>
> > + http = h['http']<br>
> > + transfer=h['transfer']<br>
> > + transfer_service=h['transfer_service']<br>
> > +<br>
> > + # Construct the JSON request for flushing. Note the offset<br>
> > + # and count must be present but are ignored by imageio.<br>
> > + buf = json.dumps({'op': "flush",<br>
> > + 'offset': 0,<br>
> > + 'size': 0,<br>
> > + 'flush': True})<br>
> ><br>
><br>
> flush does not accept any arguments - it will not fail, but better not have<br>
> code that has no effect.<br>
<br>
I didn't understand this. Do you mean ‘'flush'’ key doesn't have a<br>
parameter? The documentation seems to indicate that what I'm doing<br>
above is correct so maybe the docs are wrong or unclear.<br></blockquote><div><br></div><div>I mean the flush operation expects {"op": "flush"}. The docs are not clear</div><div>enough.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> > +def close(h):<br>
> > + http = h['http']<br>
> > + connection = h['connection']<br>
> > +<br>
> > + http.close()<br>
> ><br>
><br>
> If qemu-img does not flush at the end, we can ensure flush here.<br>
> Maybe keep h["dirty"] flag, setting to True after very write/zero/trim,<br>
> and setting the False after flush?<br>
<br>
This is more of a philosophical question. The nbdkit plugin just<br>
follows flush requests issued by the client (ie. qemu-img). qemu-img<br>
probably won't issue its own flush request. Should it issue a flush<br>
or is that the job of oVirt?<br></blockquote><div><br></div><div>oVirt cannot flush since it does not have open/close semantics.</div><div><br></div><div>The client also need to know if flush was successful, so we cannot flush</div><div>automatically when deleting a ticket. If you choose to optimize by</div><div>avoid flushing in PUT/PATCH, you must flush explicitly.</div><div><br></div><div>Eric wrote this in</div><div><a href="https://www.redhat.com/archives/libguestfs/2018-March/msg00151.html">https://www.redhat.com/archives/libguestfs/2018-March/msg00151.html</a>:</div><div><pre>+Remember that most of the feature check functions return merely a
+boolean success value, while C<.can_fua> has three success values.
+The difference between values may affect choices made in the filter:
+when splitting a write request that requested FUA from the client, if
+C<next_ops-E<gt>can_fua> returns C<NBDKIT_FUA_NATIVE>, then the filter
+should pass the FUA flag on to each sub-request; while if it is known
+that FUA is emulated by a flush because of a return of
+C<NBDKIT_FUA_EMULATE>, it is more efficient to only flush once after
+all sub-requests have completed (often by passing C<NBDKIT_FLAG_FUA>
+on to only the final sub-request, or by dropping the flag and ending
+with a direct call to C<next_ops-E<gt>flush>).
</pre></div><div>If qemu-img does not flush on the last request you must flush, and close()</div><div>seems to be the best place, assuming that qemu-img checks close() return value</div><div>and will fail the convert it flush fails.</div><div><br></div><div>Thanks for the issues you raised here, I'll update the docs to make them</div><div>clear.</div><div><br></div><div>Nir</div></div></div>