[libvirt] [PATCH option 1] Use fallocate directly if possible

[Eric Blake]

According to Jiri Denemark on 3/2/2010 8:26 AM:
> If fallocate() is present, use it directly instead of posix_allocate().
> If it is not support by the kernel or filesystem, emulate it using
> mmap() or write().
> 
> This change is to work around slow fallocate emulation done by glibc's
> posix_allocate() when used on files opened with O_DSYNC.

I'm personally undecided between the two approaches at the moment, but
here's some stylistic review in the meantime.

>  dnl Availability of various common functions (non-fatal if missing).
> -AC_CHECK_FUNCS([cfmakeraw regexec uname sched_getaffinity getuid getgid posix_fallocate mmap])
> +AC_CHECK_FUNCS([cfmakeraw regexec uname sched_getaffinity getuid getgid posix_fallocate mmap fallocate])

This is already a long line, and you made it longer.  Autoconf will let
you line-wrap the list to AC_CHECK_FUNCS; it splits on whitespace,
including newlines.  And maybe it makes more sense to use
AC_CHECK_FUNCS_ONCE instead of AC_CHECK_FUNCS (oh right, 2.59 didn't have
the former).

>  
> -#ifdef HAVE_POSIX_FALLOCATE
> +#if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE)

Redundant parenthesis.  I know upstream gnulib's maint.mk warns about
them; did we turn them off in libvirt's 'make syntax-check'?

>  #ifdef HAVE_MMAP
> -int safezero(int fd, int flags ATTRIBUTE_UNUSED, off_t offset, off_t len)
> +static int safezero_mmap(int fd, int flags ATTRIBUTE_UNUSED, off_t offset, off_t len)

Already there before your patch, but formatting this as:

static int
safezero_mmap(int...

makes life easier for emacs, and for 'grep "^safezero"' to quickly find
the implementation.

> @@ -196,6 +196,24 @@ int safezero(int fd, int flags ATTRIBUTE_UNUSED, off_t offset, off_t len)
>      return 0;
>  }
>  #endif /* HAVE_MMAP */
> +
> +int safezero(int fd, int flags ATTRIBUTE_UNUSED, off_t offset, off_t len)

Likewise, for:

int
safezero(int...

> +{
> +#ifdef HAVE_FALLOCATE
> +    if (fallocate(fd, 0, offset, len) < 0) {
> +        if (errno != ENOSYS && errno != EOPNOTSUPP)
> +            return -1;
> +    } else
> +        return 0;
> +#endif
> +
> +#ifdef HAVE_MMAP
> +    return safezero_mmap(fd, 0, offset, len);
> +#else
> +    return safezero_write(fd, 0, offset, len);
> +#endif

Looks reasonable; but why not name both fallbacks safezero_fallback at the
points where they are declared (HAVE_MMAP/!HAVE_MMAP), so that you can get
rid of the in-function #ifdef HAVE_MMAP here?

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 320 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100302/34ae647d/attachment-0001.sig>