Consolidate Linux sync_file_range implementations

  This patch consolidates all the sync_file_Range implementation for Linux
in only one (sysdeps/unix/sysv/linux/sync_file_range.c).  It also removes
the syscall from the auto-generation using assembly macros (except for
x86_64 due x32 [1]).

For current minimum supported kernel (2.6.32 for x86_64 and 3.2 for all
other architectures) either sync_file_range or sync_file_range2 is supported
and it is expected that any future Linux ABI will provide either of one
syscall.  So the code path that returns ENOSYS in the case of missing
syscall is removed.

Checked on x86_64, i386, powerpc64le, aarch64, and armhf.

	* sysdeps/unix/sysv/linux/Makefile (tests): Add tst-sync_file_range.
	* sysdeps/unix/sysv/linux/mips/mips32/sync_file_range.c: Remove file.
	* sysdeps/sysv/linux/powerpc/powerpc64/sync_file_range.c: Likewise.
	* sysdeps/unix/sysv/linux/sync_file_range.c: New file.
	* sysdeps/unix/sysv/linux/tst-sync_file_range.c (sync_file_range):
	Consolidate all Linux implementations.

[1] https://patchwork.ozlabs.org/patch/659794/
---
 ChangeLog                                          |   7 ++
 sysdeps/unix/sysv/linux/Makefile                   |   2 +-
 .../unix/sysv/linux/mips/mips32/sync_file_range.c  |  33 ------
 .../unix/sysv/linux/mips/mips64/n32/syscalls.list  |   1 -
 .../unix/sysv/linux/mips/mips64/n64/syscalls.list  |   2 -
 .../sysv/linux/powerpc/powerpc64/sync_file_range.c |  30 -----
 sysdeps/unix/sysv/linux/sync_file_range.c          |  35 ++----
 sysdeps/unix/sysv/linux/tst-sync_file_range.c      | 131 +++++++++++++++++++++
 8 files changed, 147 insertions(+), 94 deletions(-)
 delete mode 100644 sysdeps/unix/sysv/linux/mips/mips32/sync_file_range.c
 delete mode 100644 sysdeps/unix/sysv/linux/powerpc/powerpc64/sync_file_range.c
 create mode 100644 sysdeps/unix/sysv/linux/tst-sync_file_range.c
  

On Mon, 2016-10-10 at 17:58 -0300, Adhemerval Zanella wrote:
> 
	* sysdeps/unix/sysv/linux/sync_file_range.c: New file.

Is this really a new file?  Your diff seems to indicate otherwise.

p.
  

Hi,

Tested on aarch64/lp64 and aarch64/ilp32

Few minor comments below

Yury.

On Mon, Oct 10, 2016 at 05:58:02PM -0300, Adhemerval Zanella wrote:
> This patch consolidates all the sync_file_Range implementation for Linux

sync_file_Range - typo?

> in only one (sysdeps/unix/sysv/linux/sync_file_range.c).  It also removes
> the syscall from the auto-generation using assembly macros (except for
> x86_64 due x32 [1]).
> 
> For current minimum supported kernel (2.6.32 for x86_64 and 3.2 for all
> other architectures) either sync_file_range or sync_file_range2 is supported
> and it is expected that any future Linux ABI will provide either of one
> syscall.  So the code path that returns ENOSYS in the case of missing
> syscall is removed.
> 
> Checked on x86_64, i386, powerpc64le, aarch64, and armhf.
> 
> 	* sysdeps/unix/sysv/linux/Makefile (tests): Add tst-sync_file_range.
> 	* sysdeps/unix/sysv/linux/mips/mips32/sync_file_range.c: Remove file.
> 	* sysdeps/sysv/linux/powerpc/powerpc64/sync_file_range.c: Likewise.
> 	* sysdeps/unix/sysv/linux/sync_file_range.c: New file.
> 	* sysdeps/unix/sysv/linux/tst-sync_file_range.c (sync_file_range):
> 	Consolidate all Linux implementations.
> 
> [1] https://patchwork.ozlabs.org/patch/659794/
> ---
>  ChangeLog                                          |   7 ++
>  sysdeps/unix/sysv/linux/Makefile                   |   2 +-
>  .../unix/sysv/linux/mips/mips32/sync_file_range.c  |  33 ------
>  .../unix/sysv/linux/mips/mips64/n32/syscalls.list  |   1 -
>  .../unix/sysv/linux/mips/mips64/n64/syscalls.list  |   2 -
>  .../sysv/linux/powerpc/powerpc64/sync_file_range.c |  30 -----
>  sysdeps/unix/sysv/linux/sync_file_range.c          |  35 ++----
>  sysdeps/unix/sysv/linux/tst-sync_file_range.c      | 131 +++++++++++++++++++++
>  8 files changed, 147 insertions(+), 94 deletions(-)
>  delete mode 100644 sysdeps/unix/sysv/linux/mips/mips32/sync_file_range.c
>  delete mode 100644 sysdeps/unix/sysv/linux/powerpc/powerpc64/sync_file_range.c
>  create mode 100644 sysdeps/unix/sysv/linux/tst-sync_file_range.c
> 
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 718b3b9..d6a8106 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -43,7 +43,7 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \
>  		  bits/mman-linux.h
>  
>  tests += tst-clone tst-clone2 tst-fanotify tst-personality tst-quota \
> -	 tst-fallocate tst-fallocate64
> +	 tst-fallocate tst-fallocate64 tst-sync_file_range
>  
>  # Generate the list of SYS_* macros for the system calls (__NR_* macros).
>  
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/sync_file_range.c b/sysdeps/unix/sysv/linux/mips/mips32/sync_file_range.c
> deleted file mode 100644
> index de831c7..0000000
> --- a/sysdeps/unix/sysv/linux/mips/mips32/sync_file_range.c
> +++ /dev/null
> @@ -1,33 +0,0 @@
> -/* Selective file content synch'ing.
> -   Copyright (C) 2006-2016 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -
> -   The GNU C Library is free software; you can redistribute it and/or
> -   modify it under the terms of the GNU Lesser General Public
> -   License as published by the Free Software Foundation; either
> -   version 2.1 of the License, or (at your option) any later version.
> -
> -   The GNU C Library is distributed in the hope that it will be useful,
> -   but WITHOUT ANY WARRANTY; without even the implied warranty of
> -   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -   Lesser General Public License for more details.
> -
> -   You should have received a copy of the GNU Lesser General Public
> -   License along with the GNU C Library.  If not, see
> -   <http://www.gnu.org/licenses/>.  */
> -
> -#include <errno.h>
> -#include <fcntl.h>
> -#include <sys/types.h>
> -
> -#include <sysdep-cancel.h>
> -#include <sys/syscall.h>
> -
> -int
> -sync_file_range (int fd, __off64_t from, __off64_t to, unsigned int flags)
> -{
> -  return SYSCALL_CANCEL (sync_file_range, fd, 0,
> -			 __LONG_LONG_PAIR ((long) (from >> 32), (long) from),
> -			 __LONG_LONG_PAIR ((long) (to >> 32), (long) to),
> -			 flags);
> -}
> diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n32/syscalls.list b/sysdeps/unix/sysv/linux/mips/mips64/n32/syscalls.list
> index f55a94a..58fd46a 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips64/n32/syscalls.list
> +++ b/sysdeps/unix/sysv/linux/mips/mips64/n32/syscalls.list
> @@ -3,7 +3,6 @@
>  mmap64		-	mmap		b:aniiii __mmap64	mmap64
>  
>  readahead	-	readahead	i:iii	__readahead	readahead
> -sync_file_range	-	sync_file_range	Ci: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 890a744..533f19d 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips64/n64/syscalls.list
> +++ b/sysdeps/unix/sysv/linux/mips/mips64/n64/syscalls.list
> @@ -2,8 +2,6 @@
>  
>  mmap		-	mmap		b:aniiii __mmap		mmap __mmap64 mmap64
>  
> -sync_file_range	-	sync_file_range	Ci:iiii	sync_file_range
> -
>  prlimit		EXTRA	prlimit64	i:iipp	prlimit		prlimit64
>  
>  fanotify_mark	EXTRA	fanotify_mark	i:iiiis	fanotify_mark
> diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/sync_file_range.c b/sysdeps/unix/sysv/linux/powerpc/powerpc64/sync_file_range.c
> deleted file mode 100644
> index d76a2ff..0000000
> --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/sync_file_range.c
> +++ /dev/null
> @@ -1,30 +0,0 @@
> -/* Selective file content synch'ing.
> -   Copyright (C) 2006-2016 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -
> -   The GNU C Library is free software; you can redistribute it and/or
> -   modify it under the terms of the GNU Lesser General Public
> -   License as published by the Free Software Foundation; either
> -   version 2.1 of the License, or (at your option) any later version.
> -
> -   The GNU C Library is distributed in the hope that it will be useful,
> -   but WITHOUT ANY WARRANTY; without even the implied warranty of
> -   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -   Lesser General Public License for more details.
> -
> -   You should have received a copy of the GNU Lesser General Public
> -   License along with the GNU C Library; if not, see
> -   <http://www.gnu.org/licenses/>.  */
> -
> -#include <errno.h>
> -#include <fcntl.h>
> -#include <sys/types.h>
> -
> -#include <sysdep-cancel.h>
> -#include <sys/syscall.h>
> -
> -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);
> -}
> diff --git a/sysdeps/unix/sysv/linux/sync_file_range.c b/sysdeps/unix/sysv/linux/sync_file_range.c
> index a8f4214..6797488 100644
> --- a/sysdeps/unix/sysv/linux/sync_file_range.c
> +++ b/sysdeps/unix/sysv/linux/sync_file_range.c
> @@ -16,37 +16,18 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> -#include <errno.h>
>  #include <fcntl.h>
> -#include <sys/types.h>
> -
>  #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)
> +sync_file_range (int fd, __off64_t offset, __off64_t len, unsigned int flags)
>  {
> +#if defined(__NR_sync_file_range2)

Whitespace before brace?

> +  return SYSCALL_CANCEL (sync_file_range2, fd, flags, SYSCALL_LL64 (offset),
> +			 SYSCALL_LL64 (len));
> +#elif defined(__NR_sync_file_range)

And here.

>    return SYSCALL_CANCEL (sync_file_range, fd,
> -			 __LONG_LONG_PAIR ((long) (from >> 32), (long) from),
> -			 __LONG_LONG_PAIR ((long) (to >> 32), (long) to),
> -			 flags);
> -}
> -#elif 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,
> -			 __LONG_LONG_PAIR ((long) (from >> 32), (long) from),
> -			 __LONG_LONG_PAIR ((long) (to >> 32), (long) to));
> -}
> -#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)
> +			 __ALIGNMENT_ARG SYSCALL_LL64 (offset),
> +			 SYSCALL_LL64 (len), flags);
>  #endif
> +}
> diff --git a/sysdeps/unix/sysv/linux/tst-sync_file_range.c b/sysdeps/unix/sysv/linux/tst-sync_file_range.c
> new file mode 100644
> index 0000000..499a234
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-sync_file_range.c
> @@ -0,0 +1,131 @@
> +/* Basic sync_file_range (not specific flag is checked).
> +   Copyright (C) 2016 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* sync_file_range is only define for LFS.  */
> +#define _FILE_OFFSET_BITS 64
> +#include <fcntl.h>
> +#include <errno.h>
> +
> +static void do_prepare (void);
> +#define PREPARE(argc, argv)     do_prepare ()
> +static int do_test (void);
> +#define TEST_FUNCTION           do_test ()
> +
> +#define TIMEOUT 20 /* sec.  */
> +
> +#define XSTR(s) STR(S)
> +#define STR(s)  #s
> +
> +#include <test-skeleton.c>
> +
> +static char *temp_filename;
> +static int temp_fd;
> +
> +static char fifoname[] = "/tmp/tst-posix_fadvise-fifo-XXXXXX";
> +static int fifofd;
> +
> +void
> +do_prepare (void)
> +{
> +  temp_fd = create_temp_file ("tst-file_sync_range.", &temp_filename);
> +  if (temp_fd == -1)
> +    FAIL_EXIT1 ("cannot create temporary file: %m");
> +
> +  if (mktemp (fifoname) == NULL)
> +    FAIL_EXIT1 ("cannot generate temp file name: %m");
> +  add_temp_file (fifoname);
> +
> +  if (mkfifo (fifoname, S_IWUSR | S_IRUSR) != 0)
> +    FAIL_EXIT1 ("cannot create fifo: %m");
> +
> +  fifofd = open (fifoname, O_RDONLY | O_NONBLOCK);
> +  if (fifofd == -1)
> +    FAIL_EXIT1 ("cannot open fifo: %m");
> +}
> +
> +static int
> +do_test (void)
> +{
> +  int ret;
> +
> +  /* This tests first check for some invalid usage and then check for
> +     a simple usage.  It does not cover for all possible issue since for
> +     EIO/ENOMEM/ENOSPC would require to create very specific scenarios that
> +     are outside the current test coverage (basically correct kernel argument
> +     passing.  */
> +
> +  /* Check for invalid file descriptor.  */
> +  if ((ret = sync_file_range (-1, 0, 0, 0)) != -1)
> +    FAIL_EXIT1 ("sync_file_range did not fail on an invalid descriptor "
> +		"(returned %d, expected -1)", ret);
> +  if (errno != EBADF)
> +    FAIL_EXIT1 ("sync_file_range on an invalid descriptor did not set errno to "
> +		"EBADF (%d)", errno);
> +
> +  if ((ret = sync_file_range (fifofd, 0, 0, 0)) != -1)
> +    FAIL_EXIT1 ("sync_file_range did not fail on an invalid descriptor "
> +		"(returned %d, expected -1)", ret);
> +  if (errno != ESPIPE)
> +    FAIL_EXIT1 ("sync_file_range on an invalid descriptor did not set errno to "
> +		"EBADF (%d)", errno);
> +
> +  /* Check for invalid flags (it must be
> +     SYNC_FILE_RANGE_{WAIT_BEFORE,WRITE,WAIT_AFTER) or a 'or' combination of
> +     them.  */
> +  if ((ret = sync_file_range (temp_fd, 0, 0, -1)) != -1)
> +    FAIL_EXIT1 ("sync_file_range did not failed with invalid flags "
> +		"(returned %d, " "expected -1)", ret);
> +  if (errno != EINVAL)
> +    FAIL_EXIT1 ("sync_file_range with invalid flag did not set errno to "
> +		"EINVAL (%d)", errno);
> +
> +  /* Check for negative offset.  */
> +  if ((ret = sync_file_range (temp_fd, -1, 1, 0)) != -1)
> +    FAIL_EXIT1 ("sync_file_range did not failed with invalid offset "
> +		"(returned %d, expected -1)", ret);
> +  if (errno != EINVAL)
> +    FAIL_EXIT1 ("sync_file_range with invalid offset did not set errno to "
> +		"EINVAL (%d)", errno);
> +
> +  /* offset + nbytes must be a positive value.  */
> +  if ((ret = sync_file_range (temp_fd, 1024, -2048, 0)) != -1)
> +    FAIL_EXIT1 ("sync_file_range did not failed with invalid nbytes (returned %d, "
> +	  "expected -1)", ret);
> +  if (errno != EINVAL)
> +    FAIL_EXIT1 ("sync_file_range with invalid offset did not set errno to "
> +		"EINVAL (%d)", errno);
> +
> +  /* offset + nbytes must be larger or equal than offset */
> +  if ((ret = sync_file_range (temp_fd, -1024, 1024, 0)) != -1)
> +    FAIL_EXIT1 ("sync_file_range did not failed with invalid offset "
> +		"(returned %d, expected -1)", ret);
> +  if (errno != EINVAL)
> +    FAIL_EXIT1 ("sync_file_range with invalid offset did not set errno to "
> +		"EINVAL (%d)", errno);
> +
> +  /* Check simple successful case.  */
> +  if ((ret = sync_file_range (temp_fd, 0, 1024, 0)) == -1)
> +    FAIL_EXIT1 ("sync_file_range failed (errno = %d)", errno);
> +
> +  /* Finally check also a successful case with a 64-bit offset.  */
> +  off_t large_offset = UINT32_MAX + 2048LL;
> +  if ((ret = sync_file_range (temp_fd, large_offset, 1024, 0)) == -1)
> +    FAIL_EXIT1 ("sync_file_range failed (errno = %d)", errno);
> +
> +  return 0;
> +}
> -- 
> 2.7.4
  

On 11/10/2016 08:16, Yury Norov wrote:
> Hi,
> 
> Tested on aarch64/lp64 and aarch64/ilp32
> 
> Few minor comments below
> 
> Yury.
> 
> On Mon, Oct 10, 2016 at 05:58:02PM -0300, Adhemerval Zanella wrote:
>> This patch consolidates all the sync_file_Range implementation for Linux
> 
> sync_file_Range - typo?

Oops, I will fix it.

>> diff --git a/sysdeps/unix/sysv/linux/sync_file_range.c b/sysdeps/unix/sysv/linux/sync_file_range.c
>> index a8f4214..6797488 100644
>> --- a/sysdeps/unix/sysv/linux/sync_file_range.c
>> +++ b/sysdeps/unix/sysv/linux/sync_file_range.c
>> @@ -16,37 +16,18 @@
>>     License along with the GNU C Library; if not, see
>>     <http://www.gnu.org/licenses/>.  */
>>  
>> -#include <errno.h>
>>  #include <fcntl.h>
>> -#include <sys/types.h>
>> -
>>  #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)
>> +sync_file_range (int fd, __off64_t offset, __off64_t len, unsigned int flags)
>>  {
>> +#if defined(__NR_sync_file_range2)
> 
> Whitespace before brace?

Ack.

> 
>> +  return SYSCALL_CANCEL (sync_file_range2, fd, flags, SYSCALL_LL64 (offset),
>> +			 SYSCALL_LL64 (len));
>> +#elif defined(__NR_sync_file_range)
> 
> And here.

Ack.
  

On 10/10/2016 18:52, Phil Blundell wrote:
> On Mon, 2016-10-10 at 17:58 -0300, Adhemerval Zanella wrote:
>>
> 	* sysdeps/unix/sysv/linux/sync_file_range.c: New file.
> 
> Is this really a new file?  Your diff seems to indicate otherwise.

Indeed, I will add a proper CL comment.