Re: [PATCH] e4defrag: choose the best available posix_fadvise variant

[Baruch Siach <baruch@tkos.co.il>]

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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
--
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