<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Sat, Jul 28, 2018 at 1:11 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 Fri, Jul 27, 2018 at 04:23:10PM -0500, Eric Blake wrote:<br>
> On 07/27/2018 04:03 PM, Nir Soffer wrote:<br>
> >When using block device on RHEL 7.5, file plugin fails to zero with this<br>
> >error (copied from strace):<br>
> ><br>
> >[pid 39551] fallocate(8, FALLOC_FL_ZERO_RANGE, 1536, 64000) = -1 ENODEV (No such device)<br>
> ><br>
> >This is expected error according to the manual:<br>
> ><br>
> >ENODEV fd does not refer to a regular file or a directory.  (If fd is a<br>
> >pipe or FIFO, a different error results.)<br>
> <br>
> The man page is out-of-date; newer kernels support<br>
> FALLOC_FL_ZERO_RANGE on block devices, in a manner that leaves the<br>
> pages marked allocated (that is, no discard happens). The kernel<br>
> also supports FALLOC_FL_PUNCH_HOLE to guarantee a read-zeroes result<br>
> (using discard, if that works), and<br>
> FALLOC_FL_PUNCH_HOLE|FALLOC_FL_NO_HIDE_STALE (which only discards,<br>
> and no longer guarantees a read-zeroes result).  But you are also<br>
> right that in all cases, the errno returned when the operation<br>
> cannot be supported is not always EOPNOSUPP.<br>
> <br>
> ><br>
> >Treat this error as EOPNOSUPP.<br>
> ><br>
> >Tested only on Fedora 28; I don't know how to build nbdkit on RHEL, but<br>
> >the change is pretty trivial.<br>
> >---<br>
> >  plugins/file/file.c | 8 ++++----<br>
> >  1 file changed, 4 insertions(+), 4 deletions(-)<br>
> ><br>
> >diff --git a/plugins/file/file.c b/plugins/file/file.c<br>
> >index b6e33de..a7c07fb 100644<br>
> >--- a/plugins/file/file.c<br>
> >+++ b/plugins/file/file.c<br>
> >@@ -243,7 +243,7 @@ file_zero (void *handle, uint32_t count, uint64_t offset, int may_trim)<br>
> >    if (may_trim) {<br>
> >      r = fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,<br>
> >                offset, count);<br>
> >-    if (r == -1 && errno != EOPNOTSUPP) {<br>
> >+    if (r == -1 && errno != EOPNOTSUPP && errno != ENODEV) {<br>
> >        nbdkit_error ("zero: %m");<br>
> >      }<br>
> >      /* PUNCH_HOLE is older; if it is not supported, it is likely that<br>
> <br>
> Given the recent thread on qemu about the different fallocate flags<br>
> in relation to block devices, we may need to revisit the logic in<br>
> this function to more closely follow the flags (use<br>
> FALLOC_FL_ZERO_RANGE when may_trim is false, and<br>
> FALLOC_FL_PUNCH_HOLE when it is true; as well as tweak the<br>
> .discard() callback to use<br>
> FALLOC_FL_PUNCH_HOLE|FALLOC_FL_NO_HIDE_STALE).<br>
> <br>
> <a href="https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg05135.html" rel="noreferrer" target="_blank">https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg05135.html</a><br>
> <br>
> >@@ -254,7 +254,7 @@ file_zero (void *handle, uint32_t count, uint64_t offset, int may_trim)<br>
> >  #ifdef FALLOC_FL_ZERO_RANGE<br>
> >    r = fallocate (h->fd, FALLOC_FL_ZERO_RANGE, offset, count);<br>
> >-  if (r == -1 && errno != EOPNOTSUPP) {<br>
> >+  if (r == -1 && errno != EOPNOTSUPP && errno != ENODEV) {<br>
> >      nbdkit_error ("zero: %m");<br>
> >    }<br>
> >  #else<br>
> <br>
> Are we sure that the caller still sees the correct EOPNOTSUPP errno<br>
> value that it uses in deciding whether to attempt the manual<br>
> fallbacks? You may need to explicitly set errno.<br>
> <br>
> >@@ -288,11 +288,11 @@ file_trim (void *handle, uint32_t count, uint64_t offset)<br>
> >    struct handle *h = handle;<br>
> >    /* Trim is advisory; we don't care if it fails for anything other<br>
> >-   * than EIO or EPERM. */<br>
> >+   * than EIO, EPERM, or ENODEV (kernel 3.10) */<br>
> >    r = fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,<br>
> >                   offset, count);<br>
> >    if (r < 0) {<br>
> >-    if (errno != EPERM && errno != EIO) {<br>
> >+    if (errno != EPERM && errno != EIO && errno != ENODEV) {<br>
> <br>
> This last hunk looks wrong.  We want to ignore ENODEV errors in this<br>
> code path (the same way we are already ignoring EOPNOTSUPP errors).<br>
<br>
I should have read the mailing list first ...<br>
<br>
I pushed Nir's initial patch.  Let me know if additional<br>
changes are required.<br></blockquote><div><br></div><div>I hope the issues Eric found are fixed in this version:</div><div><a href="https://www.redhat.com/archives/libguestfs/2018-July/msg00077.html">https://www.redhat.com/archives/libguestfs/2018-July/msg00077.html</a><br></div><div> </div><div>Sorry about the bad patch.</div><div><br></div><div>Nir</div></div></div>