This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Consolidate Linux sync_file_range implementations
- From: Yury Norov <ynorov at caviumnetworks dot com>
- To: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- Cc: <libc-alpha at sourceware dot org>
- Date: Tue, 11 Oct 2016 14:16:08 +0300
- Subject: Re: [PATCH] Consolidate Linux sync_file_range implementations
- Authentication-results: sourceware.org; auth=none
- Authentication-results: spf=none (sender IP is ) smtp.mailfrom=Yuri dot Norov at caviumnetworks dot com;
- References: <1476133082-27968-1-git-send-email-adhemerval.zanella@linaro.org>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
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