thr3ads.net - Libguestfs - Re: [Libguestfs] [PATCH] file: Normalize errno value for ENODEV [Jul 2018]

If this information is useful, please help other people find it:
Share via:

Nir Soffer

2018-Jul-28 11:42 UTC

[Libguestfs] [PATCH] file: Normalize errno value for ENODEV

Fix issues Eric found in the original patch:
https://www.redhat.com/archives/libguestfs/2018-July/msg00072.html

- When handling ENODEV, the caller is expecting ENOTSUPP to trigger
  fallback.
- ENODEV should be ignored in file_trim.

Tested only on Fedora 28.
---
 plugins/file/file.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/plugins/file/file.c b/plugins/file/file.c
index a7c07fb..4210adb 100644
--- a/plugins/file/file.c
+++ b/plugins/file/file.c
@@ -50,6 +50,21 @@
 
 static char *filename = NULL;
 
+#if defined(FALLOC_FL_PUNCH_HOLE) || defined(FALLOC_FL_ZERO_RANGE)
+static int
+do_fallocate(int fd, int mode, off_t offset, off_t len)
+{
+  int r = -1;
+  r = fallocate (fd, mode, offset, len);
+  /* kernel 3.10 fails with ENODEV for block device. Kernel >= 4.9 fails
+     with EOPNOTSUPP in this case. Normlize errno to simplify callers. */
+  if (r == -1 && errno == ENODEV) {
+    errno = EOPNOTSUPP;
+  }
+  return r;
+}
+#endif
+
 static void
 file_unload (void)
 {
@@ -241,9 +256,9 @@ file_zero (void *handle, uint32_t count, uint64_t offset,
int may_trim)
 
 #ifdef FALLOC_FL_PUNCH_HOLE
   if (may_trim) {
-    r = fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
-		   offset, count);
-    if (r == -1 && errno != EOPNOTSUPP && errno != ENODEV) {
+    r = do_fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
+                      offset, count);
+    if (r == -1 && errno != EOPNOTSUPP) {
       nbdkit_error ("zero: %m");
     }
     /* PUNCH_HOLE is older; if it is not supported, it is likely that
@@ -253,8 +268,8 @@ file_zero (void *handle, uint32_t count, uint64_t offset,
int may_trim)
 #endif
 
 #ifdef FALLOC_FL_ZERO_RANGE
-  r = fallocate (h->fd, FALLOC_FL_ZERO_RANGE, offset, count);
-  if (r == -1 && errno != EOPNOTSUPP && errno != ENODEV) {
+  r = do_fallocate (h->fd, FALLOC_FL_ZERO_RANGE, offset, count);
+  if (r == -1 && errno != EOPNOTSUPP) {
     nbdkit_error ("zero: %m");
   }
 #else
@@ -288,11 +303,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, EPERM, or ENODEV (kernel 3.10) */
-  r = fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
-                 offset, count);
+   * than EIO or EPERM. */
+  r = do_fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
+                    offset, count);
   if (r < 0) {
-    if (errno != EPERM && errno != EIO && errno != ENODEV) {
+    if (errno != EPERM && errno != EIO) {
       nbdkit_debug ("ignoring failed fallocate during trim: %m");
       r = 0;
     }
-- 
2.17.1

Richard W.M. Jones

2018-Jul-28 19:14 UTC

head link

Re: [Libguestfs] [PATCH] file: Normalize errno value for ENODEV

On Sat, Jul 28, 2018 at 02:42:56PM +0300, Nir Soffer
wrote:> Fix issues Eric found in the original patch:
> https://www.redhat.com/archives/libguestfs/2018-July/msg00072.html
> 
> - When handling ENODEV, the caller is expecting ENOTSUPP to trigger
>   fallback.
> - ENODEV should be ignored in file_trim.
> 
> Tested only on Fedora 28.
> ---
>  plugins/file/file.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/plugins/file/file.c b/plugins/file/file.c
> index a7c07fb..4210adb 100644
> --- a/plugins/file/file.c
> +++ b/plugins/file/file.c
> @@ -50,6 +50,21 @@
>  
>  static char *filename = NULL;
>  
> +#if defined(FALLOC_FL_PUNCH_HOLE) || defined(FALLOC_FL_ZERO_RANGE)
> +static int
> +do_fallocate(int fd, int mode, off_t offset, off_t len)
> +{
> +  int r = -1;
> +  r = fallocate (fd, mode, offset, len);
> +  /* kernel 3.10 fails with ENODEV for block device. Kernel >= 4.9
fails
> +     with EOPNOTSUPP in this case. Normlize errno to simplify callers. */
> +  if (r == -1 && errno == ENODEV) {
> +    errno = EOPNOTSUPP;
> +  }
> +  return r;
> +}
> +#endif
> +
>  static void
>  file_unload (void)
>  {
> @@ -241,9 +256,9 @@ file_zero (void *handle, uint32_t count, uint64_t
offset, int may_trim)
>  
>  #ifdef FALLOC_FL_PUNCH_HOLE
>    if (may_trim) {
> -    r = fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> -		   offset, count);
> -    if (r == -1 && errno != EOPNOTSUPP && errno != ENODEV)
{
> +    r = do_fallocate (h->fd, FALLOC_FL_PUNCH_HOLE |
FALLOC_FL_KEEP_SIZE,
> +                      offset, count);
> +    if (r == -1 && errno != EOPNOTSUPP) {
>        nbdkit_error ("zero: %m");
>      }
>      /* PUNCH_HOLE is older; if it is not supported, it is likely that
> @@ -253,8 +268,8 @@ file_zero (void *handle, uint32_t count, uint64_t
offset, int may_trim)
>  #endif
>  
>  #ifdef FALLOC_FL_ZERO_RANGE
> -  r = fallocate (h->fd, FALLOC_FL_ZERO_RANGE, offset, count);
> -  if (r == -1 && errno != EOPNOTSUPP && errno != ENODEV) {
> +  r = do_fallocate (h->fd, FALLOC_FL_ZERO_RANGE, offset, count);
> +  if (r == -1 && errno != EOPNOTSUPP) {
>      nbdkit_error ("zero: %m");
>    }
>  #else
> @@ -288,11 +303,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, EPERM, or ENODEV (kernel 3.10) */
> -  r = fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> -                 offset, count);
> +   * than EIO or EPERM. */
> +  r = do_fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> +                    offset, count);
>    if (r < 0) {
> -    if (errno != EPERM && errno != EIO && errno != ENODEV)
{
> +    if (errno != EPERM && errno != EIO) {
>        nbdkit_debug ("ignoring failed fallocate during trim:
%m");
>        r = 0;
>      }
Seems reasonable enough to me.  Eric what do you think?

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/

Eric Blake

2018-Jul-30 14:45 UTC

head link

Re: [Libguestfs] [PATCH] file: Normalize errno value for ENODEV

On 07/28/2018 06:42 AM, Nir Soffer wrote:> Fix issues Eric found in the original patch:
> https://www.redhat.com/archives/libguestfs/2018-July/msg00072.html
> 
> - When handling ENODEV, the caller is expecting ENOTSUPP to trigger
>    fallback.
> - ENODEV should be ignored in file_trim.
> 
> Tested only on Fedora 28.
> ---
>   plugins/file/file.c | 33 ++++++++++++++++++++++++---------
>   1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/plugins/file/file.c b/plugins/file/file.c
> index a7c07fb..4210adb 100644
> --- a/plugins/file/file.c
> +++ b/plugins/file/file.c
> @@ -50,6 +50,21 @@
>   
>   static char *filename = NULL;
>   
> +#if defined(FALLOC_FL_PUNCH_HOLE) || defined(FALLOC_FL_ZERO_RANGE)
> +static int
> +do_fallocate(int fd, int mode, off_t offset, off_t len)
> +{
Similar name to the normalizing wrapper that qemu uses, but enough 
differences that I think you're safe here. (Be careful that you aren't 
directly copying code from that project to this one, due to the 
difference in licensing).
> +  int r = -1;
> +  r = fallocate (fd, mode, offset, len);
> +  /* kernel 3.10 fails with ENODEV for block device. Kernel >= 4.9
fails
> +     with EOPNOTSUPP in this case. Normlize errno to simplify callers. */
s/Normlize/Normalize/
> +  if (r == -1 && errno == ENODEV) {
> +    errno = EOPNOTSUPP;
> +  }
Question - do we care about retrying on EINTR?  That would also fit well 
in this normalizing wrapper.  But that can be a separate patch.

With the typo fix, I'm okay with this patch fixing up the immediate 
issues. There's still the bigger question of whether we need to revisit 
the logic anyways (using _just_ PUNCH_HOLE when we want to guarantee 
zeroes and permit unmap, and _just_ ZERO_RANGE when we want to guarantee 
zeroes without discarding) - but that was pre-existing and didn't 
regress as a result of the immediate issue that this is trying to fix.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Nir Soffer

2018-Jul-30 16:16 UTC

head link

Re: [Libguestfs] [PATCH] file: Normalize errno value for ENODEV

On Mon, Jul 30, 2018 at 5:45 PM Eric Blake <eblake@redhat.com> wrote:
> On 07/28/2018 06:42 AM, Nir Soffer wrote:
> > Fix issues Eric found in the original patch:
> > https://www.redhat.com/archives/libguestfs/2018-July/msg00072.html
> >
> > - When handling ENODEV, the caller is expecting ENOTSUPP to trigger
> >    fallback.
> > - ENODEV should be ignored in file_trim.
> >
> > Tested only on Fedora 28.
> > ---
> >   plugins/file/file.c | 33 ++++++++++++++++++++++++---------
> >   1 file changed, 24 insertions(+), 9 deletions(-)
> >
> > diff --git a/plugins/file/file.c b/plugins/file/file.c
> > index a7c07fb..4210adb 100644
> > --- a/plugins/file/file.c
> > +++ b/plugins/file/file.c
> > @@ -50,6 +50,21 @@
> >
> >   static char *filename = NULL;
> >
> > +#if defined(FALLOC_FL_PUNCH_HOLE) || defined(FALLOC_FL_ZERO_RANGE)
> > +static int
> > +do_fallocate(int fd, int mode, off_t offset, off_t len)
> > +{
>
> Similar name to the normalizing wrapper that qemu uses, but enough
> differences that I think you're safe here. (Be careful that you
aren't
> directly copying code from that project to this one, due to the
> difference in licensing).
>
> > +  int r = -1;
> > +  r = fallocate (fd, mode, offset, len);
> > +  /* kernel 3.10 fails with ENODEV for block device. Kernel >= 4.9
fails
> > +     with EOPNOTSUPP in this case. Normlize errno to simplify
callers.
> */
>
> s/Normlize/Normalize/
> > +  if (r == -1 && errno == ENODEV) {
> > +    errno = EOPNOTSUPP;
> > +  }
>
> Question - do we care about retrying on EINTR?  That would also fit well
> in this normalizing wrapper.  But that can be a separate patch.
>
I agree.

> With the typo fix, I'm okay with this patch fixing up the immediate
> issues. There's still the bigger question of whether we need to revisit
> the logic anyways (using _just_ PUNCH_HOLE when we want to guarantee
> zeroes and permit unmap, and _just_ ZERO_RANGE when we want to guarantee
> zeroes without discarding) - but that was pre-existing and didn't
> regress as a result of the immediate issue that this is trying to fix.
>
I'm not sure that we can use _just_ something because of the compatibility
issues with older kernel and block devices. I hope this is improved in
https://www.redhat.com/archives/libguestfs/2018-July/msg00084.html

I'll send v2 with the typo fix, we need to fix the regression in the
previous
patch first.

Nir

Maybe Matching Threads

Search for more maybe matching threads

Libguestfs - Jul 2018 - Re: [PATCH] file: Normalize errno value for ENODEV

[Libguestfs] [PATCH] file: Normalize errno value for ENODEV

Re: [Libguestfs] [PATCH] file: Normalize errno value for ENODEV

Re: [Libguestfs] [PATCH] file: Normalize errno value for ENODEV

Re: [Libguestfs] [PATCH] file: Normalize errno value for ENODEV

Maybe Matching Threads