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