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’
Signed-off-by: Baruch Siach <[email protected]>
---
configure.in | 1 +
misc/e4defrag.c | 27 +++++++++++++++++++++------
2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/configure.in b/configure.in
index fef8d9b7b935..9f13a46ac8ae 100644
--- a/configure.in
+++ b/configure.in
@@ -1102,6 +1102,7 @@ AC_CHECK_FUNCS(m4_flatten([
open64
pathconf
posix_fadvise
+ posix_fadvise64
posix_memalign
prctl
secure_getenv
diff --git a/misc/e4defrag.c b/misc/e4defrag.c
index c6a5f0daef96..b28c26538bfc 100644
--- a/misc/e4defrag.c
+++ b/misc/e4defrag.c
@@ -183,13 +183,27 @@ 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 */
+/* Local definitions of some syscalls glibc may not yet have
+ *
+ * We prefer posix_fadvise64 when available, as it allows 64bit offset on
+ * 32bit systems
+ */
+
+#if defined(HAVE_POSIX_FADVISE64)
+#define posix_fadvise posix_fadvise64
+#elif defined(__NR_fadvise64_64)
+#define FADVISE_SYSCALL_NR __NR_fadvise64_64
+#elif defined(HAVE_POSIX_FADVISE)
+#define posix_fadvise posix_fadvise
+#elif defined(__NR_fadvise64)
+#define FADVISE_SYSCALL_NR __NR_fadvise64
+#endif
-#ifndef HAVE_POSIX_FADVISE
+#ifndef posix_fadvise
#warning Using locally defined posix_fadvise interface.
-#ifndef __NR_fadvise64_64
-#error Your kernel headers dont define __NR_fadvise64_64
+#ifndef FADVISE_SYSCALL_NR
+#error Your kernel headers dont define __NR_fadvise64_64 or __NR_fadvise64
#endif
/*
@@ -200,9 +214,10 @@ static struct frag_statistic_ino frag_rank[SHOW_FRAG_FILES];
* @len: area length.
* @advise: process flag.
*/
-static int posix_fadvise(int fd, loff_t offset, size_t len, int advise)
+#define posix_fadvise __posix_fadvise
+static int __posix_fadvise(int fd, loff_t offset, size_t len, int advise)
{
- return syscall(__NR_fadvise64_64, fd, offset, len, advise);
+ return syscall(FADVISE_SYSCALL_NR, fd, offset, len, advise);
}
#endif /* ! HAVE_FADVISE64_64 */
--
1.8.5.2
On Thu, Jan 02, 2014 at 08:43:54AM +0200, Baruch Siach wrote:
> 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’
>
> Signed-off-by: Baruch Siach <[email protected]>
Thanks, applied.
- Ted
Hi Ted,
On Thu, Jan 02, 2014 at 07:47:10PM -0500, Theodore Ts'o wrote:
> On Thu, Jan 02, 2014 at 08:43:54AM +0200, Baruch Siach wrote:
> > 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’
> >
> > Signed-off-by: Baruch Siach <[email protected]>
>
> Thanks, applied.
Thanks. You seem to have applied v1 of this patch without the #else comment
fix. I'll fix this up in a follow up patch. Please push your tree so I can
reference this commit in the log.
Another problem with this patch is that it doesn't takes into account the
problem of passing 64bit values on 32bit architectures. See the discussion
under NOTES in the syscall(2) man page for more information on that. I'll try
to address this in another patch if you think it's worth it. The solution is
likely to look quite ugly when taking the arch specific alignment requirements
also into account. The alternative is to require C library support for
fadvise64_64 on 32bit architectures.
What do you think?
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.2.679.5364, http://www.tkos.co.il -
On Fri, Jan 03, 2014 at 06:57:48AM +0200, Baruch Siach wrote:
>
> Thanks. You seem to have applied v1 of this patch without the #else comment
> fix. I'll fix this up in a follow up patch. Please push your tree so I can
> reference this commit in the log.
Oh, oops. I'll find the other version of the patch and fix it up. I
did notice that you had two different patches sent out, and I thought
I had picked the later one, but obviously I didn't. (Or maybe I
replied to the wrong version. I'll double check.)
In the future, it would be much appreciated if you included an
explicit v2 in the subject line of the e-mail.
> Another problem with this patch is that it doesn't takes into
> account the problem of passing 64bit values on 32bit
> architectures. See the discussion under NOTES in the syscall(2) man
> page for more information on that. I'll try to address this in
> another patch if you think it's worth it. The solution is likely to
> look quite ugly when taking the arch specific alignment requirements
> also into account. The alternative is to require C library support
> for fadvise64_64 on 32bit architectures.
Ugh. I wonder how many libc's don't actually support posix_fadvise at
this point. This hack was put in a at least back in September of
2009. It's been over three years at this point. Maybe it's just time
to blow out the build with the error and tell people who complain to
get an upgraded libc.
Even dietlibc has posix_fadvise at this point. (And fadvise64(),
although interestingly, not posix_fadvise64).
Given how ugly it is to support the syscall, it may be that we're
better off trying to use posix_fadvise64, fadvise64, fadvise, in that
order, and giving up on the direct-call syscall approach. After all,
syscall interfaces is the reason why we have a C library in the first
place.
You have more experience with uClibc --- are there any platforms where
it doesn't have posix_fadvise, posix_fadvise64, or some similarly
named variant?
Thanks,
- Ted
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
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
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
*
- * @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
+#else
+#error posix_fadvise not available!
+#endif
#ifndef HAVE_SYNC_FILE_RANGE
#warning Using locally defined sync_file_range interface.
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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Theodore Ts'o <[email protected]>
>
> 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------------{=
- [email protected] - tel: +972.2.679.5364, http://www.tkos.co.il -
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