Libguestfs - Jul 2018 - Re: [PATCH] file: Fix zero/trim with block device

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.)

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
@@ -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
@@ -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) {
       nbdkit_debug ("ignoring failed fallocate during trim: %m");
       r = 0;
     }
-- 
2.17.1

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).
>         nbdkit_debug ("ignoring failed fallocate during trim:
%m");
>         r = 0;
>       }
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Sat, Jul 28, 2018 at 12:23 AM Eric Blake <eblake@redhat.com> 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.
>
Probably not, I'l check this.

> > @@ -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'll fix this on the next version.

>
> >         nbdkit_debug ("ignoring failed fallocate during trim:
%m");
> >         r = 0;
> >       }
> >
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266 <(919)%20301-3266>
> Virtualization:  qemu.org | libvirt.org
>
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
>

On Sat, Jul 28, 2018 at 12:03:04AM +0300, 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.)
> 
> 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
> @@ -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
> @@ -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)
{
>        nbdkit_debug ("ignoring failed fallocate during trim:
%m");
>        r = 0;
>      }
Thanks - I've pushed it.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

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.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/