| Message ID | 23c2b99441922fe311235e241eca52e9701e54d4.1388645034.git.baruch@tkos.co.il |
|---|---|
| State | Superseded, archived |
| Headers | show |
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 <baruch@tkos.co.il> Thanks, applied. - 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
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 <baruch@tkos.co.il> > > 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
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 -- 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
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 */
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 <baruch@tkos.co.il> --- configure.in | 1 + misc/e4defrag.c | 27 +++++++++++++++++++++------ 2 files changed, 22 insertions(+), 6 deletions(-)