e4defrag: choose the best available posix_fadvise variant

On Fri, Jan 03, 2014 at 11:23:44AM -0500, Theodore Ts'o wrote:
> 
> Oh, oops.  I'll find the other version of the patch and fix it up.  

Here is the revised version of your patch which I currently have in my
sources.  Let me know what you think.

				- Ted

commit e47e7100a357ed9ba1575a6a8caca9258b1aab77
Author: Baruch Siach <baruch@tkos.co.il>
Date:   Thu Jan 2 13:05:37 2014 -0500

    e4defrag: choose the best available posix_fadvise variant
    
    Use posix_fadvise64() when available. This allows 64bit offsets on 32bit
    systems.
    
    Rename the internal posix_fadvise() implementation to avoid collision with the
    C library signature that is sometimes present event when the implementation
    itself is not. This fixes build errors like:
    
    e4defrag.c:189:2: warning: #warning Using locally defined posix_fadvise interface. [-Wcpp
    e4defrag.c:203:12: error: conflicting types for ‘posix_fadvise’
    
    [ Modified by tytso to try to use fadvise64 as well, and to remove the
      attempt to use the syscall directly, since between fadvise64,
      fadvise64_64, and complexities caused by required dummy arguments on
      some architectures, it's not worth the hair.  ]
    
    Signed-off-by: Baruch Siach <baruch@tkos.co.il>
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Ted,

On Sun, Jan 05, 2014 at 01:12:33AM -0500, Theodore Ts'o wrote:
> On Fri, Jan 03, 2014 at 11:23:44AM -0500, Theodore Ts'o wrote:
> > 
> > Oh, oops.  I'll find the other version of the patch and fix it up.  
> 
> Here is the revised version of your patch which I currently have in my
> sources.  Let me know what you think.

Thanks. A few comments below.

> commit e47e7100a357ed9ba1575a6a8caca9258b1aab77
> Author: Baruch Siach <baruch@tkos.co.il>
> Date:   Thu Jan 2 13:05:37 2014 -0500
> 
>     e4defrag: choose the best available posix_fadvise variant
>     
>     Use posix_fadvise64() when available. This allows 64bit offsets on 32bit
>     systems.
>     
>     Rename the internal posix_fadvise() implementation to avoid collision with the
>     C library signature that is sometimes present event when the implementation
>     itself is not. This fixes build errors like:

This paragraph is not needed anymore, since the internal posix_fadvise is 
completely removed.

>     
>     e4defrag.c:189:2: warning: #warning Using locally defined posix_fadvise interface. [-Wcpp
>     e4defrag.c:203:12: error: conflicting types for ‘posix_fadvise’
>     
>     [ Modified by tytso to try to use fadvise64 as well, and to remove the
>       attempt to use the syscall directly, since between fadvise64,
>       fadvise64_64, and complexities caused by required dummy arguments on
>       some architectures, it's not worth the hair.  ]
>     
>     Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>     Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> 
> diff --git a/configure b/configure
> index 6f8f1ab..5943849 100755
> --- a/configure
> +++ b/configure
> @@ -11051,7 +11051,7 @@ if test "$ac_res" != no; then :
>  fi
>  
>  fi
> -for ac_func in  	__secure_getenv 	backtrace 	blkid_probe_get_topology 	chflags 	fallocate 	fallocate64 	fchown 	fdatasync 	fstat64 	ftruncate64 	futimes 	getdtablesize 	getmntinfo 	getpwuid_r 	getrlimit 	getrusage 	jrand48 	llseek 	lseek64 	mallinfo 	mbstowcs 	memalign 	mmap 	msync 	nanosleep 	open64 	pathconf 	posix_fadvise 	posix_memalign 	prctl 	secure_getenv 	setmntent 	setresgid 	setresuid 	srandom 	strcasecmp 	strdup 	strnlen 	strptime 	strtoull 	sync_file_range 	sysconf 	usleep 	utime 	valloc
> +for ac_func in  	__secure_getenv 	backtrace 	blkid_probe_get_topology 	chflags 	fadvise64 	fallocate 	fallocate64 	fchown 	fdatasync 	fstat64 	ftruncate64 	futimes 	getdtablesize 	getmntinfo 	getpwuid_r 	getrlimit 	getrusage 	jrand48 	llseek 	lseek64 	mallinfo 	mbstowcs 	memalign 	mmap 	msync 	nanosleep 	open64 	pathconf 	posix_fadvise 	posix_fadvise64 	posix_memalign 	prctl 	secure_getenv 	setmntent 	setresgid 	setresuid 	srandom 	strcasecmp 	strdup 	strnlen 	strptime 	strtoull 	sync_file_range 	sysconf 	usleep 	utime 	valloc
>  do :
>    as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
>  ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
> diff --git a/configure.in b/configure.in
> index b70a3f9..214d3f6 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -1026,6 +1026,7 @@ AC_CHECK_FUNCS(m4_flatten([
>  	backtrace
>  	blkid_probe_get_topology
>  	chflags
> +	fadvise64

There is no such symbol in any C library that I know of (unlike fallocate64). 
Where have you see this?

>  	fallocate
>  	fallocate64
>  	fchown
> @@ -1050,6 +1051,7 @@ AC_CHECK_FUNCS(m4_flatten([
>  	open64
>  	pathconf
>  	posix_fadvise
> +	posix_fadvise64
>  	posix_memalign
>  	prctl
>  	secure_getenv
> diff --git a/lib/config.h.in b/lib/config.h.in
> index ef2e664..0197b56 100644
> --- a/lib/config.h.in
> +++ b/lib/config.h.in
> @@ -103,6 +103,9 @@
>  /* Define to 1 if Ext2 ioctls present */
>  #undef HAVE_EXT2_IOCTLS
>  
> +/* Define to 1 if you have the `fadvise64' function. */
> +#undef HAVE_FADVISE64
> +
>  /* Define to 1 if you have the `fallocate' function. */
>  #undef HAVE_FALLOCATE
>  
> @@ -287,6 +290,9 @@
>  /* Define to 1 if you have the `posix_fadvise' function. */
>  #undef HAVE_POSIX_FADVISE
>  
> +/* Define to 1 if you have the `posix_fadvise64' function. */
> +#undef HAVE_POSIX_FADVISE64
> +
>  /* Define to 1 if you have the `posix_memalign' function. */
>  #undef HAVE_POSIX_MEMALIGN
>  
> diff --git a/misc/e4defrag.c b/misc/e4defrag.c
> index c6a5f0d..65933b1 100644
> --- a/misc/e4defrag.c
> +++ b/misc/e4defrag.c
> @@ -34,13 +34,11 @@
>  #include <unistd.h>
>  #include <ext2fs/ext2_types.h>
>  #include <ext2fs/ext2fs.h>
> -#include <linux/fs.h>
>  #include <sys/ioctl.h>
>  #include <ext2fs/fiemap.h>
>  #include <sys/mman.h>
>  #include <sys/stat.h>
>  #include <sys/statfs.h>
> -#include <sys/syscall.h>
>  #include <sys/vfs.h>
>  
>  /* A relatively new ioctl interface ... */
> @@ -183,28 +181,21 @@ static ext4_fsblk_t	files_block_count;
>  static struct frag_statistic_ino	frag_rank[SHOW_FRAG_FILES];
>  
>  
> -/* Local definitions of some syscalls glibc may not yet have */
> -
> -#ifndef HAVE_POSIX_FADVISE
> -#warning Using locally defined posix_fadvise interface.
> -
> -#ifndef __NR_fadvise64_64
> -#error Your kernel headers dont define __NR_fadvise64_64
> -#endif
> -
> -/*
> - * fadvise() -		Give advice about file access.
> +/* Local definitions of some syscalls glibc may not yet have

This comment becomes redundant. We now rely on the C library to provide the 
system call.

>   *
> - * @fd:			defrag target file's descriptor.
> - * @offset:		file offset.
> - * @len:		area length.
> - * @advise:		process flag.
> + * We prefer posix_fadvise64 when available, as it allows 64bit offset on
> + * 32bit systems
>   */
> -static int posix_fadvise(int fd, loff_t offset, size_t len, int advise)
> -{
> -	return syscall(__NR_fadvise64_64, fd, offset, len, advise);
> -}
> -#endif /* ! HAVE_FADVISE64_64 */
> +
> +#if defined(HAVE_POSIX_FADVISE64)
> +#define posix_fadvise	posix_fadvise64
> +#elif defined(HAVE_FADVISE64)
> +#define posix_fadvise	fadvise64
> +#elif defined(HAVE_POSIX_FADVISE)
> +#define posix_fadvise	posix_fadvise

Also not needed.

> +#else
> +#error posix_fadvise not available!
> +#endif
>  
>  #ifndef HAVE_SYNC_FILE_RANGE
>  #warning Using locally defined sync_file_range interface.

baruch

On Sun, Jan 05, 2014 at 08:46:31AM +0200, Baruch Siach wrote:
> >     Rename the internal posix_fadvise() implementation to avoid collision with the
> >     C library signature that is sometimes present event when the implementation
> >     itself is not. This fixes build errors like:
> 
> This paragraph is not needed anymore, since the internal posix_fadvise is 
> completely removed.

Good point, thanks.  I'll remove it.

> > +	fadvise64
> 
> There is no such symbol in any C library that I know of (unlike fallocate64). 
> Where have you see this?

Dietlibc has it.  It's a little moot since at the moment e4defrag uses
a number of other functions which aren't supported by dietlibc.
(Including nftw64 and FTW_MOUNT/FTW_PHYS; nftw64 isn't a part of any
standard, but FTW_MOUNT and FTW_PHYS is guaranteed by the Single Unix
Specification. We could rewrite nftw64 in terms of nftw and stat64,
which would be supported by dietlibc, but that still leaves FTW_MOUNT
and FTW_PHYS.  But since that's missing standards-compliant
functionality, and it shouldn't be that hard to implement, I'm hopeful
the dietlibc maintianer will accept a patch to fix this at some point.)

The main reason why I like dietlibc is because it allows me to build a
stripped e2fsck.static in 433k on x86_64, instead of 1312k doing a
static build using glibc.  It's also useful as a convenient
"mini-libc", which makes it much more likely that e2fsprogs can be
compiled on non-glibc libc's, such as uClibc and Android's Bionic.
(I'd consider patches to the build system for other mini-libc's, but
dietlibc has the advantage of being available on almost all Debian
architectures, and it is very convenient for me to use.)

The other benefit of dietlibc, along with work such as Andreas' test
builds e2fsprogs on MacOS X, is that it helps ensure e2fsprogs's
portability across other operating systems.  So for example, although
I don't do any testing trying to build e2fsprogs for use with
Android/Cyanogen, I was pleased to find out that 1.42.9 built cleanly
against Bionic.

It turns out dietlibc support was silently broken a while back due to
changes in autoconf, so there were a couple fix ups I needed to make
e2fsprogs work well on dietlibc again.

> > - * fadvise() -		Give advice about file access.
> > +/* Local definitions of some syscalls glibc may not yet have
> 
> This comment becomes redundant. We now rely on the C library to provide the 
> system call.

Good point, I'll remove it.

> > +#define posix_fadvise	posix_fadvise
> 
> Also not needed.

Yup, will fix.

Thanks for the review!

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html