diff mbox

Change sync_file_range to be non-cancellable

Message ID 1445430841-25953-1-git-send-email-adhemerval.zanella@linaro.com
State New
Headers show

Commit Message

Adhemerval Zanella Netto Oct. 21, 2015, 12:34 p.m. UTC 
This patch changes the linux specific sync_file_range syscalls to be
non-cancellable.  The rationale is:

1. This is a Linux specific syscall that is not mentioned in POSIX
   cancellable entrypoints [1] and the standard states and implementation
   shall not introduce cancellation points into any other functions
   specified.

2. For mips it requires 7 arguments, which will make the new
   cancellation code require very specific handling for the platform.

3. It aligns with other implementations (musl) which does not set
   sync_file_range as cancellable.

Also since GLIBC requires a minimum 2.6.32 kernel, the patch also
cleanups the mips code to assume __NR_sync_file_range and the
powerpc one to either assume __NR_sync_file_range2 or __NR_sync_file_range.

Checked on x86_64, i386, powerpc64le and build for mips (ABIO32, ABIN32,
and ABI64).

[1] http://pubs.opengroup.org/onlinepubs/9699919799/

	* sysdeps/unix/sysv/linux/powerpc/powerpc64/sync_file_range.c
	(__NR_sync_file_range2): Assume it is always defined.
	(sync_file_range): Make syscall non-cancellable.
	* sysdeps/unix/sysv/linux/mips/mips32/sync_file_range.c
	(__NR_sync_file_range): Assume it is always defined.
	(sync_file_range): Make syscall non-cancellable.
	* sysdeps/unix/sysv/linux/mips/mips64/n32/syscalls.list
	(sync_file_range): Likewise.
	* sysdeps/unix/sysv/linux/mips/mips64/n64/syscalls.list
	(sync_file_range): Likewise.
	* sysdeps/unix/sysv/linux/sync_file_range.c (sync_file_range):
	Likewise.
	* sysdeps/unix/sysv/linux/wordsize-64/syscalls.list (sync_file_range):
	Likewise.
---
 ChangeLog                                                 | 15 +++++++++++++++
 sysdeps/unix/sysv/linux/mips/mips32/sync_file_range.c     | 13 +------------
 sysdeps/unix/sysv/linux/mips/mips64/n32/syscalls.list     |  2 +-
 sysdeps/unix/sysv/linux/mips/mips64/n64/syscalls.list     |  2 +-
 .../unix/sysv/linux/powerpc/powerpc64/sync_file_range.c   | 13 +------------
 sysdeps/unix/sysv/linux/sync_file_range.c                 |  4 ++--
 sysdeps/unix/sysv/linux/wordsize-64/syscalls.list         |  2 +-
 7 files changed, 22 insertions(+), 29 deletions(-)

Comments

Florian Weimer Oct. 21, 2015, 1:24 p.m. UTC | #1 
On 10/21/2015 02:34 PM, Adhemerval Zanella wrote:
> This patch changes the linux specific sync_file_range syscalls to be
> non-cancellable.  The rationale is:
> 
> 1. This is a Linux specific syscall that is not mentioned in POSIX
>    cancellable entrypoints [1] and the standard states and implementation
>    shall not introduce cancellation points into any other functions
>    specified.

It says that we must not make any function mentioned in POSIX
cancellable.  We can certainly introduce our own functions which are
cancellable and not port of POSIX.

I think sync_file_range should be cancellable for consistency with fsync
and fdatasync.

Florian
Szabolcs Nagy Oct. 21, 2015, 1:48 p.m. UTC | #2 
On 21/10/15 14:24, Florian Weimer wrote:
> On 10/21/2015 02:34 PM, Adhemerval Zanella wrote:
>> This patch changes the linux specific sync_file_range syscalls to be
>> non-cancellable.  The rationale is:
>>
>> 1. This is a Linux specific syscall that is not mentioned in POSIX
>>     cancellable entrypoints [1] and the standard states and implementation
>>     shall not introduce cancellation points into any other functions
>>     specified.
>
> It says that we must not make any function mentioned in POSIX
> cancellable.  We can certainly introduce our own functions which are
> cancellable and not port of POSIX.
>
> I think sync_file_range should be cancellable for consistency with fsync
> and fdatasync.
>

this is something musl might want too.

one ugliness is that cancellation is either guaranteed
to be acted upon or guaranteed not to, which is bad
for all cases when there is a fast path in libc that
avoids calling the blocking syscall.

e.g. sync_file_range is not a cancellation point now
if it returns ENOSYS in glibc.

> Florian
>
Mike Frysinger Oct. 21, 2015, 2:46 p.m. UTC | #3 
On 21 Oct 2015 10:34, Adhemerval Zanella wrote:
> Also since GLIBC requires a minimum 2.6.32 kernel, the patch also
> cleanups the mips code to assume __NR_sync_file_range and the
> powerpc one to either assume __NR_sync_file_range2 or __NR_sync_file_range.

unless i'm missing something, this is orthogonal to cancellation, so it
should be a sep patch/commit.  it would also allow for quick merging as
it wouldn't get caught up in the cancellation debate.
-mike
Adhemerval Zanella Netto Oct. 21, 2015, 3:24 p.m. UTC | #4 
> Em 21 de out de 2015, às 12:46, Mike Frysinger <vapier@gentoo.org> escreveu:
> 
>> On 21 Oct 2015 10:34, Adhemerval Zanella wrote:
>> Also since GLIBC requires a minimum 2.6.32 kernel, the patch also
>> cleanups the mips code to assume __NR_sync_file_range and the
>> powerpc one to either assume __NR_sync_file_range2 or __NR_sync_file_range.
> 
> unless i'm missing something, this is orthogonal to cancellation, so it
> should be a sep patch/commit.  it would also allow for quick merging as
> it wouldn't get caught up in the cancellation debate.
> -mike

Yes and also I withdrawn the cancellation changes. I will resend a patch with cleanup for syscall definitions.
diff mbox

Patch

diff --git a/sysdeps/unix/sysv/linux/mips/mips32/sync_file_range.c b/sysdeps/unix/sysv/linux/mips/mips32/sync_file_range.c
index b79e44d..ce82f30 100644
--- a/sysdeps/unix/sysv/linux/mips/mips32/sync_file_range.c
+++ b/sysdeps/unix/sysv/linux/mips/mips32/sync_file_range.c
@@ -23,22 +23,11 @@ 
 #include <sysdep-cancel.h>
 #include <sys/syscall.h>
 
-
-#ifdef __NR_sync_file_range
 int
 sync_file_range (int fd, __off64_t from, __off64_t to, unsigned int flags)
 {
-  return SYSCALL_CANCEL (sync_file_range, fd, 0,
+  return INLINE_SYSCALL (sync_file_range, 7, fd, 0,
 			 __LONG_LONG_PAIR ((long) (from >> 32), (long) from),
 			 __LONG_LONG_PAIR ((long) (to >> 32), (long) to),
 			 flags);
 }
-#else
-int
-sync_file_range (int fd, __off64_t from, __off64_t to, unsigned int flags)
-{
-  __set_errno (ENOSYS);
-  return -1;
-}
-stub_warning (sync_file_range)
-#endif
diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n32/syscalls.list b/sysdeps/unix/sysv/linux/mips/mips64/n32/syscalls.list
index 7ad5523..0148e42 100644
--- a/sysdeps/unix/sysv/linux/mips/mips64/n32/syscalls.list
+++ b/sysdeps/unix/sysv/linux/mips/mips64/n32/syscalls.list
@@ -1,7 +1,7 @@ 
 # File name	Caller	Syscall name	# args	Strong name	Weak names
 
 readahead	-	readahead	i:iii	__readahead	readahead
-sync_file_range	-	sync_file_range	Ci:iiii	sync_file_range
+sync_file_range	-	sync_file_range	i:iiii	sync_file_range
 
 prlimit64	EXTRA	prlimit64	i:iipp	prlimit64
 
diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n64/syscalls.list b/sysdeps/unix/sysv/linux/mips/mips64/n64/syscalls.list
index b23a2a1..e4fac08 100644
--- a/sysdeps/unix/sysv/linux/mips/mips64/n64/syscalls.list
+++ b/sysdeps/unix/sysv/linux/mips/mips64/n64/syscalls.list
@@ -1,6 +1,6 @@ 
 # File name	Caller	Syscall name	# args	Strong name	Weak names
 
-sync_file_range	-	sync_file_range	Ci:iiii	sync_file_range
+sync_file_range	-	sync_file_range	i:iiii	sync_file_range
 
 prlimit		EXTRA	prlimit64	i:iipp	prlimit		prlimit64
 
diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/sync_file_range.c b/sysdeps/unix/sysv/linux/powerpc/powerpc64/sync_file_range.c
index 9f46458..d3d54ef 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/sync_file_range.c
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/sync_file_range.c
@@ -23,19 +23,8 @@ 
 #include <sysdep-cancel.h>
 #include <sys/syscall.h>
 
-
-#if defined __NR_sync_file_range2
-int
-sync_file_range (int fd, __off64_t from, __off64_t to, unsigned int flags)
-{
-  return SYSCALL_CANCEL (sync_file_range2, fd, flags, from, to);
-}
-#else
 int
 sync_file_range (int fd, __off64_t from, __off64_t to, unsigned int flags)
 {
-  __set_errno (ENOSYS);
-  return -1;
+  return INLINE_SYSCALL (sync_file_range2, 4, fd, flags, from, to);
 }
-stub_warning (sync_file_range)
-#endif
diff --git a/sysdeps/unix/sysv/linux/sync_file_range.c b/sysdeps/unix/sysv/linux/sync_file_range.c
index 2ea6dcf..69b128a 100644
--- a/sysdeps/unix/sysv/linux/sync_file_range.c
+++ b/sysdeps/unix/sysv/linux/sync_file_range.c
@@ -28,7 +28,7 @@ 
 int
 sync_file_range (int fd, __off64_t from, __off64_t to, unsigned int flags)
 {
-  return SYSCALL_CANCEL (sync_file_range, fd,
+  return INLINE_SYSCALL (sync_file_range, 6, fd,
 			 __LONG_LONG_PAIR ((long) (from >> 32), (long) from),
 			 __LONG_LONG_PAIR ((long) (to >> 32), (long) to),
 			 flags);
@@ -37,7 +37,7 @@  sync_file_range (int fd, __off64_t from, __off64_t to, unsigned int flags)
 int
 sync_file_range (int fd, __off64_t from, __off64_t to, unsigned int flags)
 {
-  return SYSCALL_CANCEL (sync_file_range2, fd, flags,
+  return INLINE_SYSCALL (sync_file_range2, 6, fd, flags,
 			 __LONG_LONG_PAIR ((long) (from >> 32), (long) from),
 			 __LONG_LONG_PAIR ((long) (to >> 32), (long) to));
 }
diff --git a/sysdeps/unix/sysv/linux/wordsize-64/syscalls.list b/sysdeps/unix/sysv/linux/wordsize-64/syscalls.list
index 51ee8d8..55e62f3 100644
--- a/sysdeps/unix/sysv/linux/wordsize-64/syscalls.list
+++ b/sysdeps/unix/sysv/linux/wordsize-64/syscalls.list
@@ -14,7 +14,7 @@  getrlimit	-	getrlimit	i:ip	__getrlimit	getrlimit getrlimit64 __getrlimit64
 setrlimit	-	setrlimit	i:ip	__setrlimit	setrlimit setrlimit64
 readahead	-	readahead	i:iii	__readahead	readahead
 sendfile	-	sendfile	i:iipi	sendfile	sendfile64
-sync_file_range	-	sync_file_range	Ci:iiii	sync_file_range
+sync_file_range	-	sync_file_range	i:iiii	sync_file_range
 creat		-	creat		Ci:si	creat		creat64
 open		-	open		Ci:siv	__libc_open	__open open __open64 open64
 prlimit		EXTRA	prlimit64	i:iipp	prlimit		prlimit64