[PATCH] POSIX_FADV_{DONTNEED,NOREUSE} defines on s390-64.

[PATCH] POSIX_FADV_{DONTNEED,NOREUSE} defines on s390-64.

[Martin Schwidefsky]

Hi,
our testers noticed that the definitions of POSIX_FADV_DONTNEED and
POSIX_FADV_NOREUSE of the glibc and the linux kernel differ. There
are two options to fix this problem: in the glibc or in the kernel.
I considered this for a while and came to the conclusion that this
is better fixed in the glibc. To fix this in the kernel I'd need to
analyse the arguments of the compat fadvise system call and change
the 31 bit defines to the 64 bit defines. The 64 bit applications
that are currently using DONTNEED and NOREUSE always get -EINVAL.
Nobody complained so far.

blue skies,
  Martin.

2005-07-11  sky  <sky@localhost.localdomain>

	* sysdeps/unix/sysv/linux/s390/bits/fcntl.h (POSIX_FADV_DONTNEED,
	POSIX_FADV_NOREUSE): Use correct values for s390-64.

diff -urpN libc/sysdeps/unix/sysv/linux/s390/bits/fcntl.h libc-s390/sysdeps/unix/sysv/linux/s390/bits/fcntl.h
--- libc/sysdeps/unix/sysv/linux/s390/bits/fcntl.h	2004-08-23 09:28:45.000000000 +0200
+++ libc-s390/sysdeps/unix/sysv/linux/s390/bits/fcntl.h	2005-07-11 12:34:36.000000000 +0200
@@ -190,13 +190,8 @@ struct flock64
 # define POSIX_FADV_RANDOM	1 /* Expect random page references.  */
 # define POSIX_FADV_SEQUENTIAL	2 /* Expect sequential page references.	 */
 # define POSIX_FADV_WILLNEED	3 /* Will need these pages.  */
-# if __WORDSIZE == 64
-#  define POSIX_FADV_DONTNEED	6 /* Don't need these pages.  */
-#  define POSIX_FADV_NOREUSE	7 /* Data will be accessed once.  */
-# else
-#  define POSIX_FADV_DONTNEED	4 /* Don't need these pages.  */
-#  define POSIX_FADV_NOREUSE	5 /* Data will be accessed once.  */
-# endif
+# define POSIX_FADV_DONTNEED	4 /* Don't need these pages.  */
+# define POSIX_FADV_NOREUSE	5 /* Data will be accessed once.  */
 #endif
 
 __BEGIN_DECLS

Re: [PATCH] POSIX_FADV_{DONTNEED,NOREUSE} defines on s390-64.

[Ulrich Drepper]

[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]

Martin Schwidefsky wrote:
> our testers noticed that the definitions of POSIX_FADV_DONTNEED and
> POSIX_FADV_NOREUSE of the glibc and the linux kernel differ.

And how did this happen?  You created the unified fcntl.h file.


> There
> are two options to fix this problem: in the glibc or in the kernel.
> I considered this for a while and came to the conclusion that this
> is better fixed in the glibc. To fix this in the kernel I'd need to
> analyse the arguments of the compat fadvise system call and change
> the 31 bit defines to the 64 bit defines. The 64 bit applications
> that are currently using DONTNEED and NOREUSE always get -EINVAL.
> Nobody complained so far.

The latter doesn't mean much since the behavior is not really visible.
Fixing it in glibc means that the values currently used are forever
taboo.  And it means existing binaries are broken.  If you change the
kernel, existing binaries magically start working and the only drawback
is that a program tested on a new kernel might perform differently on an
old kernel.

Changing glibc might be the simpler solution but it the least friendly
to users.  Can you reconsider it?

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

Re: [PATCH] POSIX_FADV_{DONTNEED,NOREUSE} defines on s390-64.

[Martin Schwidefsky]

libc-hacker-owner@sources.redhat.com wrote on 07/11/2005 06:51:44 PM:

> Martin Schwidefsky wrote:
> > our testers noticed that the definitions of POSIX_FADV_DONTNEED and
> > POSIX_FADV_NOREUSE of the glibc and the linux kernel differ.
>
> And how did this happen?  You created the unified fcntl.h file.

It did not happen with the fcntl.h unification. The "wrong" values
for DONTNEED and NOREUSE have been present before the unification
as well.

> > There
> > are two options to fix this problem: in the glibc or in the kernel.
> > I considered this for a while and came to the conclusion that this
> > is better fixed in the glibc. To fix this in the kernel I'd need to
> > analyse the arguments of the compat fadvise system call and change
> > the 31 bit defines to the 64 bit defines. The 64 bit applications
> > that are currently using DONTNEED and NOREUSE always get -EINVAL.
> > Nobody complained so far.
>
> The latter doesn't mean much since the behavior is not really visible.
> Fixing it in glibc means that the values currently used are forever
> taboo.  And it means existing binaries are broken.  If you change the
> kernel, existing binaries magically start working and the only drawback
> is that a program tested on a new kernel might perform differently on an
> old kernel.
>
> Changing glibc might be the simpler solution but it the least friendly
> to users.  Can you reconsider it?

I'll try to come up with a kernel patch. But it won't be a nice patch.
I have the feeling that the kernel guys won't like it either ..

blue skies,
   Martin

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

Re: [PATCH] POSIX_FADV_{DONTNEED,NOREUSE} defines on s390-64.

[Martin Schwidefsky]

> Changing glibc might be the simpler solution but it the least friendly
> to users.  Can you reconsider it?

The patch to fix this problem in the kernel has hit the git repository.
The solution in the kernel doesn't comply with my sense of nice, good
looking code but I must admit it is the better solution for the customer.

blue skies,
   Martin

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

Re: [PATCH] POSIX_FADV_{DONTNEED,NOREUSE} defines on s390-64.

[Ulrich Drepper]

[-- Attachment #1: Type: text/plain, Size: 202 bytes --]

Martin Schwidefsky wrote:
> The patch to fix this problem in the kernel has hit the git repository.

Good.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

Re: [PATCH] POSIX_FADV_{DONTNEED,NOREUSE} defines on s390-64.

[Martin Schwidefsky]

> > Changing glibc might be the simpler solution but it the least friendly
> > to users.  Can you reconsider it?
> 
> I'll try to come up with a kernel patch. But it won't be a nice patch.
> I have the feeling that the kernel guys won't like it either ..

Hmm, the patch doesn't look as bad as expected. Still can't say that
I like the solution. I'll ask the kernel guys what they think about it.

blue skies,
  Martin.


diffstat:
 arch/s390/kernel/compat_linux.c   |   38 ++++++++++++++++++++++++++++++++++++++
 arch/s390/kernel/compat_wrapper.S |    4 ++--
 include/linux/fadvise.h           |   10 ++++++++++
 3 files changed, 50 insertions(+), 2 deletions(-)

diff -urpN linux-2.6/arch/s390/kernel/compat_linux.c linux-2.6-patched/arch/s390/kernel/compat_linux.c
--- linux-2.6/arch/s390/kernel/compat_linux.c	2005-06-17 21:48:29.000000000 +0200
+++ linux-2.6-patched/arch/s390/kernel/compat_linux.c	2005-07-12 13:28:31.000000000 +0200
@@ -58,6 +58,7 @@
 #include <linux/compat.h>
 #include <linux/vfs.h>
 #include <linux/ptrace.h>
+#include <linux/fadvise.h>
 
 #include <asm/types.h>
 #include <asm/ipc.h>
@@ -1043,3 +1044,40 @@ sys32_timer_create(clockid_t which_clock
 
 	return ret;
 }
+
+/*
+ * 31 bit emulation wrapper functions for sys_fadvise64/fadvise64_64.
+ * These need to rewrite the advise values for POSIX_FADV_{DONTNEED,NOREUSE}
+ * because the 31 bit values differ from the 64 bit values.
+ */
+
+asmlinkage long
+sys32_fadvise64(int fd, loff_t offset, size_t len, int advise)
+{
+	if (advise == 4)
+		advise = POSIX_FADV_DONTNEED;
+	else if (advise == 5)
+		advise = POSIX_FADV_NOREUSE;
+	return sys_fadvise64(fd, offset, len, advise);
+}
+
+struct fadvise64_64_args {
+	int fd;
+	long long offset;
+	long long len;
+	int advice;
+};
+
+asmlinkage long
+sys32_fadvise64_64(struct fadvise64_64_args __user *args)
+{
+	struct fadvise64_64_args a;
+
+	if ( copy_from_user(&a, args, sizeof(a)) )
+		return -EFAULT;
+	if (a.advice == 4)
+		a.advice = POSIX_FADV_DONTNEED;
+	else if (a.advice == 5)
+		a.advice = POSIX_FADV_NOREUSE;
+	return sys_fadvise64_64(a.fd, a.offset, a.len, a.advice);
+}
diff -urpN linux-2.6/arch/s390/kernel/compat_wrapper.S linux-2.6-patched/arch/s390/kernel/compat_wrapper.S
--- linux-2.6/arch/s390/kernel/compat_wrapper.S	2005-07-12 13:27:13.000000000 +0200
+++ linux-2.6-patched/arch/s390/kernel/compat_wrapper.S	2005-07-12 13:28:36.000000000 +0200
@@ -1251,12 +1251,12 @@ sys32_fadvise64_wrapper:
 	or	%r3,%r4			# get low word of 64bit loff_t
 	llgfr	%r4,%r5			# size_t (unsigned long)
 	lgfr	%r5,%r6			# int
-	jg	sys_fadvise64
+	jg	sys32_fadvise64
 
 	.globl	sys32_fadvise64_64_wrapper
 sys32_fadvise64_64_wrapper:
 	llgtr	%r2,%r2			# struct fadvise64_64_args *
-	jg	s390_fadvise64_64
+	jg	sys32_fadvise64_64
 
 	.globl	sys32_clock_settime_wrapper
 sys32_clock_settime_wrapper:
diff -urpN linux-2.6/include/linux/fadvise.h linux-2.6-patched/include/linux/fadvise.h
--- linux-2.6/include/linux/fadvise.h	2005-06-17 21:48:29.000000000 +0200
+++ linux-2.6-patched/include/linux/fadvise.h	2005-07-12 13:28:40.000000000 +0200
@@ -5,7 +5,17 @@
 #define POSIX_FADV_RANDOM	1 /* Expect random page references.  */
 #define POSIX_FADV_SEQUENTIAL	2 /* Expect sequential page references.  */
 #define POSIX_FADV_WILLNEED	3 /* Will need these pages.  */
+
+/*
+ * The advise values for POSIX_FADV_DONTNEED and POSIX_ADV_NOREUSE
+ * for s390-64 differ from the values for the rest of the world.
+ */
+#if defined(__s390x__)
+#define POSIX_FADV_DONTNEED	6 /* Don't need these pages.  */
+#define POSIX_FADV_NOREUSE	7 /* Data will be accessed once.  */
+#else
 #define POSIX_FADV_DONTNEED	4 /* Don't need these pages.  */
 #define POSIX_FADV_NOREUSE	5 /* Data will be accessed once.  */
+#endif
 
 #endif	/* FADVISE_H_INCLUDED */