[Libguestfs] [PATCH] file: Fix zero/trim with block device

[Nir Soffer]

On Sat, Jul 28, 2018 at 1:11 PM Richard W.M. Jones <rjones at redhat.com>
wrote:

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

I hope the issues Eric found are fixed in this version:
https://www.redhat.com/archives/libguestfs/2018-July/msg00077.html

Sorry about the bad patch.

Nir
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20180728/37ce5b54/attachment.htm>