Libguestfs - Jul 2018 - Re: [PATCH v2] file: Add missing include for FALLOC_FL_*

On RHEL 7.5 we need to include <linux/falloc.h> for FALLOC_FL_* macros.
Without the macros, fallocate is never used and we fall back to manual
zeroing.

Here are examples runs with this change with a local file on ext4:

$ export SOCK=/tmp/nbd.sock
$ export FILE=/var/tmp/nbd.img
$ export BLOCK=/dev/loop2

$ src/nbdkit -f plugins/file/.libs/nbdkit-file-plugin.so file=$FILE -U $SOCK
$ time qemu-img convert -n -f raw -O raw /var/tmp/fedora-27.img nbd:unix:$SOCK

real    0m13.361s
user    0m0.127s
sys     0m0.668s

$ src/nbdkit -f plugins/file/.libs/nbdkit-file-plugin.so file=$BLOCK -U $SOCK
$ time qemu-img convert -n -f raw -O raw /var/tmp/fedora-27.img nbd:unix:$SOCK

real    0m13.491s
user    0m0.129s
sys     0m0.612s

Tested on Fedora 28 and RHEL 7.5.
---

v2:
- Include <linux/falloc.h> only on Linux

v1 was here:
https://www.redhat.com/archives/libguestfs/2018-July/msg00083.html

 plugins/file/file.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/plugins/file/file.c b/plugins/file/file.c
index a8a6253..0345115 100644
--- a/plugins/file/file.c
+++ b/plugins/file/file.c
@@ -42,6 +42,10 @@
 #include <sys/stat.h>
 #include <errno.h>
 
+#if defined(__linux__)
+#include <linux/falloc.h>   /* For FALLOC_FL_* on RHEL, glibc < 2.18
*/
+#endif
+
 #include <nbdkit-plugin.h>
 
 #ifndef O_CLOEXEC
-- 
2.17.1

On 07/30/2018 01:04 PM, Nir Soffer wrote:
> On RHEL 7.5 we need to include <linux/falloc.h> for FALLOC_FL_*
macros.
Rather, on any Linux system that pre-dates glibc 2.18, where the flags 
were finally supported directly in <fcntl.h>
> Without the macros, fallocate is never used and we fall back to manual
> zeroing.
> 
> +++ b/plugins/file/file.c
> @@ -42,6 +42,10 @@
>   #include <sys/stat.h>
>   #include <errno.h>
>   
> +#if defined(__linux__)
> +#include <linux/falloc.h>   /* For FALLOC_FL_* on RHEL, glibc <
2.18 */
Doesn't mention which version of RHEL, nor the fact that non-RHEL 
systems may also be impacted (it is the glibc version that matters here, 
rather than the distro).
> +#endif
> +
ACK.

Perhaps could be made tighter, with a less ambiguous comment, as:

#if defined(__linux__) && !defined(FALLOC_FL_PUNCH_HOLE)
# include <linux/falloc.h>  /* For FALLOC_FL_*, on glibc < 2.18 */
#endif

but let's see if Rich has any preference between the two.

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

On Mon, Jul 30, 2018 at 9:16 PM Eric Blake <eblake@redhat.com> wrote:
> On 07/30/2018 01:04 PM, Nir Soffer wrote:
> > On RHEL 7.5 we need to include <linux/falloc.h> for FALLOC_FL_*
macros.
>
> Rather, on any Linux system that pre-dates glibc 2.18, where the flags
> were finally supported directly in <fcntl.h>
>
> > Without the macros, fallocate is never used and we fall back to manual
> > zeroing.
> >
>
> > +++ b/plugins/file/file.c
> > @@ -42,6 +42,10 @@
> >   #include <sys/stat.h>
> >   #include <errno.h>
> >
> > +#if defined(__linux__)
> > +#include <linux/falloc.h>   /* For FALLOC_FL_* on RHEL, glibc
< 2.18 */
>
> Doesn't mention which version of RHEL, nor the fact that non-RHEL
> systems may also be impacted (it is the glibc version that matters here,
> rather than the distro).
>
> > +#endif
> > +
>
> ACK.
>
> Perhaps could be made tighter, with a less ambiguous comment, as:
>
> #if defined(__linux__) && !defined(FALLOC_FL_PUNCH_HOLE)
> # include <linux/falloc.h>  /* For FALLOC_FL_*, on glibc < 2.18 */
> #endif
>
> but let's see if Rich has any preference between the two.
>
Both changes are better, I'll post v3 if Rich prefer this.

Nir

On Mon, Jul 30, 2018 at 01:16:10PM -0500, Eric Blake
wrote:
> On 07/30/2018 01:04 PM, Nir Soffer wrote:
> >On RHEL 7.5 we need to include <linux/falloc.h> for FALLOC_FL_*
macros.
> 
> Rather, on any Linux system that pre-dates glibc 2.18, where the
> flags were finally supported directly in <fcntl.h>
> 
> >Without the macros, fallocate is never used and we fall back to manual
> >zeroing.
> >
> 
> >+++ b/plugins/file/file.c
> >@@ -42,6 +42,10 @@
> >  #include <sys/stat.h>
> >  #include <errno.h>
> >+#if defined(__linux__)
> >+#include <linux/falloc.h>   /* For FALLOC_FL_* on RHEL, glibc
< 2.18 */
> 
> Doesn't mention which version of RHEL, nor the fact that non-RHEL
> systems may also be impacted (it is the glibc version that matters
> here, rather than the distro).
> 
> >+#endif
> >+
> 
> ACK.
> 
> Perhaps could be made tighter, with a less ambiguous comment, as:
> 
> #if defined(__linux__) && !defined(FALLOC_FL_PUNCH_HOLE)
> # include <linux/falloc.h>  /* For FALLOC_FL_*, on glibc < 2.18 */
> #endif
> 
> but let's see if Rich has any preference between the two.
Your version is better.  Upstream hat on we don't care about the
weirdness of those enterprise distros :-)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW