Re: [PATCH] Consolidate Linux sync_file_range implementations

[Yury Norov <ynorov at caviumnetworks dot com>]

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