| Message ID | 20230925201827.1703857-1-aahringo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv2] generic: add fcntl corner cases tests | expand |
Hi, On Mon, Sep 25, 2023 at 4:18 PM Alexander Aring <aahringo@redhat.com> wrote: > > This patch adds fcntl corner cases that was being used to confirm issues > on a GFS2 filesystem. The GFS2 filesystem has it's own ->lock() > implementation and in those corner cases issues was being found and > fixed. > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > --- > changes since v2: > - move fcntl tests into one fcntl c file > - remove ofd and same owner tests, should be reflected by only one test > - simplify commit message (remove testname out of it) > - add error messages in fcntl.c to give more information if an error > occur ping, any comments? Would for me nice to have these fcntl() tests upstream and have a starting point to add more tests regarding dlm plock implementation. Thanks. - Alex
Hi, On Thu, Nov 2, 2023 at 10:37 AM Alexander Aring <aahringo@redhat.com> wrote: > > Hi, > > On Mon, Sep 25, 2023 at 4:18 PM Alexander Aring <aahringo@redhat.com> wrote: > > > > This patch adds fcntl corner cases that was being used to confirm issues > > on a GFS2 filesystem. The GFS2 filesystem has it's own ->lock() > > implementation and in those corner cases issues was being found and > > fixed. > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > --- > > changes since v2: > > - move fcntl tests into one fcntl c file > > - remove ofd and same owner tests, should be reflected by only one test > > - simplify commit message (remove testname out of it) > > - add error messages in fcntl.c to give more information if an error > > occur > > ping, any comments? Would for me nice to have these fcntl() tests > upstream and have a starting point to add more tests regarding dlm > plock implementation. > > Thanks. ping? - Alex
On Mon, 2023-09-25 at 16:18 -0400, Alexander Aring wrote: > This patch adds fcntl corner cases that was being used to confirm issues > on a GFS2 filesystem. The GFS2 filesystem has it's own ->lock() > implementation and in those corner cases issues was being found and > fixed. > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > --- > changes since v2: > - move fcntl tests into one fcntl c file > - remove ofd and same owner tests, should be reflected by only one test > - simplify commit message (remove testname out of it) > - add error messages in fcntl.c to give more information if an error > occur > > src/Makefile | 3 +- > src/fcntl.c | 322 ++++++++++++++++++++++++++++++++++++++++++ > tests/generic/732 | 32 +++++ > tests/generic/732.out | 2 + > 4 files changed, 358 insertions(+), 1 deletion(-) > create mode 100644 src/fcntl.c > create mode 100755 tests/generic/732 > create mode 100644 tests/generic/732.out > > diff --git a/src/Makefile b/src/Makefile > index 2815f919..67f936d3 100644 > --- a/src/Makefile > +++ b/src/Makefile > @@ -19,7 +19,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \ > t_ofd_locks t_mmap_collision mmap-write-concurrent \ > t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \ > t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \ > - t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test > + t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test \ > + fcntl I think this function needs a less-generic name than "fcntl". Other the lock tests themselves look like a nice addition though. > > LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ > preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \ > diff --git a/src/fcntl.c b/src/fcntl.c > new file mode 100644 > index 00000000..8e375357 > --- /dev/null > +++ b/src/fcntl.c > @@ -0,0 +1,322 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* Copyright (c) 2023 Alexander Aring. All Rights Reserved. > + */ > + > +#include <pthread.h> > +#include <unistd.h> > +#include <string.h> > +#include <limits.h> > +#include <stdlib.h> > +#include <errno.h> > +#include <fcntl.h> > +#include <stdio.h> > +#include <wait.h> > + > +static char filename[PATH_MAX + 1]; > +static int fd; > + > +static void usage(char *name, const char *msg) > +{ > + printf("Fatal: %s\nUsage:\n" > + "%s <file for fcntl>\n", msg, name); > + _exit(1); > +} > + > +static void *do_equal_file_lock_thread(void *arg) > +{ > + struct flock fl = { > + .l_type = F_WRLCK, > + .l_whence = SEEK_SET, > + .l_start = 0L, > + .l_len = 1L, > + }; > + int rv; > + > + rv = fcntl(fd, F_SETLKW, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + return NULL; > +} > + > +static void do_test_equal_file_lock(void) > +{ > + struct flock fl; > + pthread_t t[2]; > + int pid, rv; > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0L; > + fl.l_len = 1L; > + > + /* acquire range 0-0 */ > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + pid = fork(); > + if (pid == 0) { > + rv = pthread_create(&t[0], NULL, do_equal_file_lock_thread, NULL); > + if (rv != 0) { > + fprintf(stderr, "failed to create pthread\n"); > + _exit(1); > + } > + > + rv = pthread_create(&t[1], NULL, do_equal_file_lock_thread, NULL); > + if (rv != 0) { > + fprintf(stderr, "failed to create pthread\n"); > + _exit(1); > + } > + > + pthread_join(t[0], NULL); > + pthread_join(t[1], NULL); > + > + _exit(0); > + } > + > + /* wait until threads block on 0-0 */ > + sleep(3); > + > + fl.l_type = F_UNLCK; > + fl.l_start = 0; > + fl.l_len = 1; > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + sleep(3); > + > + /* check if the ->lock() implementation got the > + * right locks granted because two waiter with the > + * same file_lock fields are waiting > + */ > + fl.l_type = F_WRLCK; > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1 && errno == EAGAIN) { > + fprintf(stderr, "deadlock, pthread not cleaned up correctly\n"); > + _exit(1); > + } > + > + wait(NULL); > +} > + > +static void catch_alarm(int num) { } > + > +static void do_test_signal_interrupt_child(int *pfd) > +{ > + struct sigaction act; > + unsigned char m; > + struct flock fl; > + int rv; > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 1; > + fl.l_len = 1; > + > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + memset(&act, 0, sizeof(act)); > + act.sa_handler = catch_alarm; > + sigemptyset(&act.sa_mask); > + sigaddset(&act.sa_mask, SIGALRM); > + sigaction(SIGALRM, &act, NULL); > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + /* interrupt SETLKW by signal in 3 secs */ > + alarm(3); > + rv = fcntl(fd, F_SETLKW, &fl); > + if (rv == 0) { > + fprintf(stderr, "fcntl interrupt successful but should fail with EINTR\n"); > + _exit(1); > + } > + > + /* synchronize to move parent to test region 1-1 */ > + write(pfd[1], &m, sizeof(m)); > + > + /* keep child alive */ > + read(pfd[1], &m, sizeof(m)); > +} > + > +static void do_test_signal_interrupt(void) > +{ > + struct flock fl; > + unsigned char m; > + int pid, rv; > + int pfd[2]; > + > + rv = pipe(pfd); > + if (rv == -1) { > + perror("pipe"); > + _exit(1); > + } > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + pid = fork(); > + if (pid == 0) { > + do_test_signal_interrupt_child(pfd); > + _exit(0); > + } > + > + /* wait until child writes */ > + read(pfd[0], &m, sizeof(m)); > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 1; > + fl.l_len = 1; > + /* parent testing childs region, the child will think > + * it has region 1-1 locked because it was interrupted > + * by region 0-0. Due bugs the interruption also unlocked > + * region 1-1. > + */ > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == 0) { > + fprintf(stderr, "fcntl trylock successful but should fail because child still acquires region\n"); > + _exit(1); > + } > + > + write(pfd[0], &m, sizeof(m)); > + > + wait(NULL); > + > + close(pfd[0]); > + close(pfd[1]); > + > + /* cleanup everything */ > + fl.l_type = F_UNLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 2; > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > +} > + > +static void do_test_kill_child(void) > +{ > + struct flock fl; > + int rv; > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + rv = fcntl(fd, F_SETLKW, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > +} > + > +static void do_test_kill(void) > +{ > + struct flock fl; > + int pid_to_kill; > + int rv; > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + pid_to_kill = fork(); > + if (pid_to_kill == 0) { > + do_test_kill_child(); > + _exit(0); > + } > + > + /* wait until child blocks */ > + sleep(3); > + > + kill(pid_to_kill, SIGKILL); > + > + /* wait until Linux did plock cleanup */ > + sleep(3); > + > + fl.l_type = F_UNLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + /* cleanup parent lock */ > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + /* check if the child still holds the lock > + * and killing the child was not cleaning > + * up locks. > + */ > + rv = fcntl(fd, F_SETLK, &fl); > + if ((rv == -1 && errno == EAGAIN)) { > + fprintf(stderr, "fcntl trylock failed but should be successful because killing child should cleanup acquired lock\n"); > + _exit(1); > + } > +} > + > +int main(int argc, char * const argv[]) > +{ > + if (optind != argc - 1) > + usage(argv[0], "<file for fcntl> is mandatory to tell the file where to run fcntl() on it"); > + > + strncpy(filename, argv[1], PATH_MAX); > + > + fd = open(filename, O_RDWR | O_CREAT, 0700); > + if (fd == -1) { > + perror("open"); > + _exit(1); > + } > + > + /* test to have to equal struct file_lock requests in ->lock() */ > + do_test_equal_file_lock(); > + /* test to interrupt F_SETLKW by a signal and cleanup only canceled the pending interrupted request */ > + do_test_signal_interrupt(); > + /* test if cleanup is correct if a child gets killed while being blocked in F_SETLKW */ > + do_test_kill(); > + > + close(fd); > + > + return 0; > +} > diff --git a/tests/generic/732 b/tests/generic/732 > new file mode 100755 > index 00000000..d77f9fc2 > --- /dev/null > +++ b/tests/generic/732 > @@ -0,0 +1,32 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2023 Alexander Aring. All Rights Reserved. > +# > +# FS QA Test 732 > +# > +# This tests performs some fcntl() corner cases. See fcntl test > +# program for more details. > +# > +. ./common/preamble > +_begin_fstest auto > + > +# Import common functions. > +. ./common/filter > + > +# real QA test starts here > + > +# Modify as appropriate. > +_supported_fs generic > +_require_test > +_require_test_program fcntl > + > +echo "Silence is golden" > + > +$here/src/fcntl $TEST_DIR/testfile > +if [ $? -ne 0 ] > +then > + exit > +fi > + > +status=0 > +exit > diff --git a/tests/generic/732.out b/tests/generic/732.out > new file mode 100644 > index 00000000..451f82ce > --- /dev/null > +++ b/tests/generic/732.out > @@ -0,0 +1,2 @@ > +QA output created by 732 > +Silence is golden Assuming you change the name of "fcntl" program, you can add: Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Thu, Feb 01, 2024 at 12:10:44PM -0500, Jeff Layton wrote: > On Mon, 2023-09-25 at 16:18 -0400, Alexander Aring wrote: > > This patch adds fcntl corner cases that was being used to confirm issues > > on a GFS2 filesystem. The GFS2 filesystem has it's own ->lock() > > implementation and in those corner cases issues was being found and > > fixed. > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > --- > > changes since v2: > > - move fcntl tests into one fcntl c file > > - remove ofd and same owner tests, should be reflected by only one test > > - simplify commit message (remove testname out of it) > > - add error messages in fcntl.c to give more information if an error > > occur > > > > src/Makefile | 3 +- > > src/fcntl.c | 322 ++++++++++++++++++++++++++++++++++++++++++ > > tests/generic/732 | 32 +++++ > > tests/generic/732.out | 2 + > > 4 files changed, 358 insertions(+), 1 deletion(-) > > create mode 100644 src/fcntl.c > > create mode 100755 tests/generic/732 > > create mode 100644 tests/generic/732.out > > > > diff --git a/src/Makefile b/src/Makefile > > index 2815f919..67f936d3 100644 > > --- a/src/Makefile > > +++ b/src/Makefile > > @@ -19,7 +19,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \ > > t_ofd_locks t_mmap_collision mmap-write-concurrent \ > > t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \ > > t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \ > > - t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test > > + t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test \ > > + fcntl > > I think this function needs a less-generic name than "fcntl". Other the > lock tests themselves look like a nice addition though. Yeah, the "fcntl" name is too "big", please rename it to show what it trys to test. Hi Alexander, you can tell me the new name you'd like to change, if you don't want to send a V2. That might help to catch the fstests release of this weekend :) Thanks, Zorro > > > > > LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ > > preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \ > > diff --git a/src/fcntl.c b/src/fcntl.c > > new file mode 100644 > > index 00000000..8e375357 > > --- /dev/null > > +++ b/src/fcntl.c > > @@ -0,0 +1,322 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* Copyright (c) 2023 Alexander Aring. All Rights Reserved. > > + */ > > + > > +#include <pthread.h> > > +#include <unistd.h> > > +#include <string.h> > > +#include <limits.h> > > +#include <stdlib.h> > > +#include <errno.h> > > +#include <fcntl.h> > > +#include <stdio.h> > > +#include <wait.h> > > + > > +static char filename[PATH_MAX + 1]; > > +static int fd; > > + > > +static void usage(char *name, const char *msg) > > +{ > > + printf("Fatal: %s\nUsage:\n" > > + "%s <file for fcntl>\n", msg, name); > > + _exit(1); > > +} > > + > > +static void *do_equal_file_lock_thread(void *arg) > > +{ > > + struct flock fl = { > > + .l_type = F_WRLCK, > > + .l_whence = SEEK_SET, > > + .l_start = 0L, > > + .l_len = 1L, > > + }; > > + int rv; > > + > > + rv = fcntl(fd, F_SETLKW, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > + > > + return NULL; > > +} > > + > > +static void do_test_equal_file_lock(void) > > +{ > > + struct flock fl; > > + pthread_t t[2]; > > + int pid, rv; > > + > > + fl.l_type = F_WRLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 0L; > > + fl.l_len = 1L; > > + > > + /* acquire range 0-0 */ > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > + > > + pid = fork(); > > + if (pid == 0) { > > + rv = pthread_create(&t[0], NULL, do_equal_file_lock_thread, NULL); > > + if (rv != 0) { > > + fprintf(stderr, "failed to create pthread\n"); > > + _exit(1); > > + } > > + > > + rv = pthread_create(&t[1], NULL, do_equal_file_lock_thread, NULL); > > + if (rv != 0) { > > + fprintf(stderr, "failed to create pthread\n"); > > + _exit(1); > > + } > > + > > + pthread_join(t[0], NULL); > > + pthread_join(t[1], NULL); > > + > > + _exit(0); > > + } > > + > > + /* wait until threads block on 0-0 */ > > + sleep(3); > > + > > + fl.l_type = F_UNLCK; > > + fl.l_start = 0; > > + fl.l_len = 1; > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > + > > + sleep(3); > > + > > + /* check if the ->lock() implementation got the > > + * right locks granted because two waiter with the > > + * same file_lock fields are waiting > > + */ > > + fl.l_type = F_WRLCK; > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == -1 && errno == EAGAIN) { > > + fprintf(stderr, "deadlock, pthread not cleaned up correctly\n"); > > + _exit(1); > > + } > > + > > + wait(NULL); > > +} > > + > > +static void catch_alarm(int num) { } > > + > > +static void do_test_signal_interrupt_child(int *pfd) > > +{ > > + struct sigaction act; > > + unsigned char m; > > + struct flock fl; > > + int rv; > > + > > + fl.l_type = F_WRLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 1; > > + fl.l_len = 1; > > + > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > + > > + memset(&act, 0, sizeof(act)); > > + act.sa_handler = catch_alarm; > > + sigemptyset(&act.sa_mask); > > + sigaddset(&act.sa_mask, SIGALRM); > > + sigaction(SIGALRM, &act, NULL); > > + > > + fl.l_type = F_WRLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 0; > > + fl.l_len = 1; > > + > > + /* interrupt SETLKW by signal in 3 secs */ > > + alarm(3); > > + rv = fcntl(fd, F_SETLKW, &fl); > > + if (rv == 0) { > > + fprintf(stderr, "fcntl interrupt successful but should fail with EINTR\n"); > > + _exit(1); > > + } > > + > > + /* synchronize to move parent to test region 1-1 */ > > + write(pfd[1], &m, sizeof(m)); > > + > > + /* keep child alive */ > > + read(pfd[1], &m, sizeof(m)); > > +} > > + > > +static void do_test_signal_interrupt(void) > > +{ > > + struct flock fl; > > + unsigned char m; > > + int pid, rv; > > + int pfd[2]; > > + > > + rv = pipe(pfd); > > + if (rv == -1) { > > + perror("pipe"); > > + _exit(1); > > + } > > + > > + fl.l_type = F_WRLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 0; > > + fl.l_len = 1; > > + > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > + > > + pid = fork(); > > + if (pid == 0) { > > + do_test_signal_interrupt_child(pfd); > > + _exit(0); > > + } > > + > > + /* wait until child writes */ > > + read(pfd[0], &m, sizeof(m)); > > + > > + fl.l_type = F_WRLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 1; > > + fl.l_len = 1; > > + /* parent testing childs region, the child will think > > + * it has region 1-1 locked because it was interrupted > > + * by region 0-0. Due bugs the interruption also unlocked > > + * region 1-1. > > + */ > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == 0) { > > + fprintf(stderr, "fcntl trylock successful but should fail because child still acquires region\n"); > > + _exit(1); > > + } > > + > > + write(pfd[0], &m, sizeof(m)); > > + > > + wait(NULL); > > + > > + close(pfd[0]); > > + close(pfd[1]); > > + > > + /* cleanup everything */ > > + fl.l_type = F_UNLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 0; > > + fl.l_len = 2; > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > +} > > + > > +static void do_test_kill_child(void) > > +{ > > + struct flock fl; > > + int rv; > > + > > + fl.l_type = F_WRLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 0; > > + fl.l_len = 1; > > + > > + rv = fcntl(fd, F_SETLKW, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > +} > > + > > +static void do_test_kill(void) > > +{ > > + struct flock fl; > > + int pid_to_kill; > > + int rv; > > + > > + fl.l_type = F_WRLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 0; > > + fl.l_len = 1; > > + > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > + > > + pid_to_kill = fork(); > > + if (pid_to_kill == 0) { > > + do_test_kill_child(); > > + _exit(0); > > + } > > + > > + /* wait until child blocks */ > > + sleep(3); > > + > > + kill(pid_to_kill, SIGKILL); > > + > > + /* wait until Linux did plock cleanup */ > > + sleep(3); > > + > > + fl.l_type = F_UNLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 0; > > + fl.l_len = 1; > > + > > + /* cleanup parent lock */ > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > + > > + fl.l_type = F_WRLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 0; > > + fl.l_len = 1; > > + > > + /* check if the child still holds the lock > > + * and killing the child was not cleaning > > + * up locks. > > + */ > > + rv = fcntl(fd, F_SETLK, &fl); > > + if ((rv == -1 && errno == EAGAIN)) { > > + fprintf(stderr, "fcntl trylock failed but should be successful because killing child should cleanup acquired lock\n"); > > + _exit(1); > > + } > > +} > > + > > +int main(int argc, char * const argv[]) > > +{ > > + if (optind != argc - 1) > > + usage(argv[0], "<file for fcntl> is mandatory to tell the file where to run fcntl() on it"); > > + > > + strncpy(filename, argv[1], PATH_MAX); > > + > > + fd = open(filename, O_RDWR | O_CREAT, 0700); > > + if (fd == -1) { > > + perror("open"); > > + _exit(1); > > + } > > + > > + /* test to have to equal struct file_lock requests in ->lock() */ > > + do_test_equal_file_lock(); > > + /* test to interrupt F_SETLKW by a signal and cleanup only canceled the pending interrupted request */ > > + do_test_signal_interrupt(); > > + /* test if cleanup is correct if a child gets killed while being blocked in F_SETLKW */ > > + do_test_kill(); > > + > > + close(fd); > > + > > + return 0; > > +} > > diff --git a/tests/generic/732 b/tests/generic/732 > > new file mode 100755 > > index 00000000..d77f9fc2 > > --- /dev/null > > +++ b/tests/generic/732 > > @@ -0,0 +1,32 @@ > > +#! /bin/bash > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright (c) 2023 Alexander Aring. All Rights Reserved. > > +# > > +# FS QA Test 732 > > +# > > +# This tests performs some fcntl() corner cases. See fcntl test > > +# program for more details. > > +# > > +. ./common/preamble > > +_begin_fstest auto > > + > > +# Import common functions. > > +. ./common/filter > > + > > +# real QA test starts here > > + > > +# Modify as appropriate. > > +_supported_fs generic > > +_require_test > > +_require_test_program fcntl > > + > > +echo "Silence is golden" > > + > > +$here/src/fcntl $TEST_DIR/testfile > > +if [ $? -ne 0 ] > > +then > > + exit > > +fi > > + > > +status=0 > > +exit > > diff --git a/tests/generic/732.out b/tests/generic/732.out > > new file mode 100644 > > index 00000000..451f82ce > > --- /dev/null > > +++ b/tests/generic/732.out > > @@ -0,0 +1,2 @@ > > +QA output created by 732 > > +Silence is golden > > > Assuming you change the name of "fcntl" program, you can add: > > Reviewed-by: Jeff Layton <jlayton@kernel.org> >
Hi Zorro, On Fri, Feb 2, 2024 at 7:04 AM Zorro Lang <zlang@redhat.com> wrote: > > On Thu, Feb 01, 2024 at 12:10:44PM -0500, Jeff Layton wrote: > > On Mon, 2023-09-25 at 16:18 -0400, Alexander Aring wrote: > > > This patch adds fcntl corner cases that was being used to confirm issues > > > on a GFS2 filesystem. The GFS2 filesystem has it's own ->lock() > > > implementation and in those corner cases issues was being found and > > > fixed. > > > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > > --- > > > changes since v2: > > > - move fcntl tests into one fcntl c file > > > - remove ofd and same owner tests, should be reflected by only one test > > > - simplify commit message (remove testname out of it) > > > - add error messages in fcntl.c to give more information if an error > > > occur > > > > > > src/Makefile | 3 +- > > > src/fcntl.c | 322 ++++++++++++++++++++++++++++++++++++++++++ > > > tests/generic/732 | 32 +++++ > > > tests/generic/732.out | 2 + > > > 4 files changed, 358 insertions(+), 1 deletion(-) > > > create mode 100644 src/fcntl.c > > > create mode 100755 tests/generic/732 > > > create mode 100644 tests/generic/732.out > > > > > > diff --git a/src/Makefile b/src/Makefile > > > index 2815f919..67f936d3 100644 > > > --- a/src/Makefile > > > +++ b/src/Makefile > > > @@ -19,7 +19,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \ > > > t_ofd_locks t_mmap_collision mmap-write-concurrent \ > > > t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \ > > > t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \ > > > - t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test > > > + t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test \ > > > + fcntl > > > > I think this function needs a less-generic name than "fcntl". Other the > > lock tests themselves look like a nice addition though. > > Yeah, the "fcntl" name is too "big", please rename it to show what it trys > to test. Hi Alexander, you can tell me the new name you'd like to change, > if you don't want to send a V2. That might help to catch the fstests release > of this weekend :) > I was thinking about "fcntl_corner_tests", if this is okay. Thanks. - Alex
On Fri, Feb 02, 2024 at 07:19:53AM -0500, Alexander Aring wrote: > Hi Zorro, > > On Fri, Feb 2, 2024 at 7:04 AM Zorro Lang <zlang@redhat.com> wrote: > > > > On Thu, Feb 01, 2024 at 12:10:44PM -0500, Jeff Layton wrote: > > > On Mon, 2023-09-25 at 16:18 -0400, Alexander Aring wrote: > > > > This patch adds fcntl corner cases that was being used to confirm issues > > > > on a GFS2 filesystem. The GFS2 filesystem has it's own ->lock() > > > > implementation and in those corner cases issues was being found and > > > > fixed. > > > > > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > > > --- > > > > changes since v2: > > > > - move fcntl tests into one fcntl c file > > > > - remove ofd and same owner tests, should be reflected by only one test > > > > - simplify commit message (remove testname out of it) > > > > - add error messages in fcntl.c to give more information if an error > > > > occur > > > > > > > > src/Makefile | 3 +- > > > > src/fcntl.c | 322 ++++++++++++++++++++++++++++++++++++++++++ > > > > tests/generic/732 | 32 +++++ > > > > tests/generic/732.out | 2 + > > > > 4 files changed, 358 insertions(+), 1 deletion(-) > > > > create mode 100644 src/fcntl.c > > > > create mode 100755 tests/generic/732 > > > > create mode 100644 tests/generic/732.out > > > > > > > > diff --git a/src/Makefile b/src/Makefile > > > > index 2815f919..67f936d3 100644 > > > > --- a/src/Makefile > > > > +++ b/src/Makefile > > > > @@ -19,7 +19,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \ > > > > t_ofd_locks t_mmap_collision mmap-write-concurrent \ > > > > t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \ > > > > t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \ > > > > - t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test > > > > + t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test \ > > > > + fcntl > > > > > > I think this function needs a less-generic name than "fcntl". Other the > > > lock tests themselves look like a nice addition though. > > > > Yeah, the "fcntl" name is too "big", please rename it to show what it trys > > to test. Hi Alexander, you can tell me the new name you'd like to change, > > if you don't want to send a V2. That might help to catch the fstests release > > of this weekend :) > > > > I was thinking about "fcntl_corner_tests", if this is okay. Good to me, I'm rename to that if no more objection :) > > Thanks. > > - Alex >
On Fri, 2024-02-02 at 20:27 +0800, Zorro Lang wrote: > On Fri, Feb 02, 2024 at 07:19:53AM -0500, Alexander Aring wrote: > > Hi Zorro, > > > > On Fri, Feb 2, 2024 at 7:04 AM Zorro Lang <zlang@redhat.com> wrote: > > > > > > On Thu, Feb 01, 2024 at 12:10:44PM -0500, Jeff Layton wrote: > > > > On Mon, 2023-09-25 at 16:18 -0400, Alexander Aring wrote: > > > > > This patch adds fcntl corner cases that was being used to confirm issues > > > > > on a GFS2 filesystem. The GFS2 filesystem has it's own ->lock() > > > > > implementation and in those corner cases issues was being found and > > > > > fixed. > > > > > > > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > > > > --- > > > > > changes since v2: > > > > > - move fcntl tests into one fcntl c file > > > > > - remove ofd and same owner tests, should be reflected by only one test > > > > > - simplify commit message (remove testname out of it) > > > > > - add error messages in fcntl.c to give more information if an error > > > > > occur > > > > > > > > > > src/Makefile | 3 +- > > > > > src/fcntl.c | 322 ++++++++++++++++++++++++++++++++++++++++++ > > > > > tests/generic/732 | 32 +++++ > > > > > tests/generic/732.out | 2 + > > > > > 4 files changed, 358 insertions(+), 1 deletion(-) > > > > > create mode 100644 src/fcntl.c > > > > > create mode 100755 tests/generic/732 > > > > > create mode 100644 tests/generic/732.out > > > > > > > > > > diff --git a/src/Makefile b/src/Makefile > > > > > index 2815f919..67f936d3 100644 > > > > > --- a/src/Makefile > > > > > +++ b/src/Makefile > > > > > @@ -19,7 +19,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \ > > > > > t_ofd_locks t_mmap_collision mmap-write-concurrent \ > > > > > t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \ > > > > > t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \ > > > > > - t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test > > > > > + t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test \ > > > > > + fcntl > > > > > > > > I think this function needs a less-generic name than "fcntl". Other the > > > > lock tests themselves look like a nice addition though. > > > > > > Yeah, the "fcntl" name is too "big", please rename it to show what it trys > > > to test. Hi Alexander, you can tell me the new name you'd like to change, > > > if you don't want to send a V2. That might help to catch the fstests release > > > of this weekend :) > > > > > > > I was thinking about "fcntl_corner_tests", if this is okay. > > Good to me, I'm rename to that if no more objection :) > > Can we make it "fcntl_lock_corner_tests"? These tests are all about file locking, but fcntl() is a generic system call that does a lot of different things besides that.
On Fri, Feb 02, 2024 at 07:36:11AM -0500, Jeff Layton wrote: > On Fri, 2024-02-02 at 20:27 +0800, Zorro Lang wrote: > > On Fri, Feb 02, 2024 at 07:19:53AM -0500, Alexander Aring wrote: > > > Hi Zorro, > > > > > > On Fri, Feb 2, 2024 at 7:04 AM Zorro Lang <zlang@redhat.com> wrote: > > > > > > > > On Thu, Feb 01, 2024 at 12:10:44PM -0500, Jeff Layton wrote: > > > > > On Mon, 2023-09-25 at 16:18 -0400, Alexander Aring wrote: > > > > > > This patch adds fcntl corner cases that was being used to confirm issues > > > > > > on a GFS2 filesystem. The GFS2 filesystem has it's own ->lock() > > > > > > implementation and in those corner cases issues was being found and > > > > > > fixed. > > > > > > > > > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > > > > > --- > > > > > > changes since v2: > > > > > > - move fcntl tests into one fcntl c file > > > > > > - remove ofd and same owner tests, should be reflected by only one test > > > > > > - simplify commit message (remove testname out of it) > > > > > > - add error messages in fcntl.c to give more information if an error > > > > > > occur > > > > > > > > > > > > src/Makefile | 3 +- > > > > > > src/fcntl.c | 322 ++++++++++++++++++++++++++++++++++++++++++ > > > > > > tests/generic/732 | 32 +++++ > > > > > > tests/generic/732.out | 2 + > > > > > > 4 files changed, 358 insertions(+), 1 deletion(-) > > > > > > create mode 100644 src/fcntl.c > > > > > > create mode 100755 tests/generic/732 > > > > > > create mode 100644 tests/generic/732.out > > > > > > > > > > > > diff --git a/src/Makefile b/src/Makefile > > > > > > index 2815f919..67f936d3 100644 > > > > > > --- a/src/Makefile > > > > > > +++ b/src/Makefile > > > > > > @@ -19,7 +19,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \ > > > > > > t_ofd_locks t_mmap_collision mmap-write-concurrent \ > > > > > > t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \ > > > > > > t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \ > > > > > > - t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test > > > > > > + t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test \ > > > > > > + fcntl > > > > > > > > > > I think this function needs a less-generic name than "fcntl". Other the > > > > > lock tests themselves look like a nice addition though. > > > > > > > > Yeah, the "fcntl" name is too "big", please rename it to show what it trys > > > > to test. Hi Alexander, you can tell me the new name you'd like to change, > > > > if you don't want to send a V2. That might help to catch the fstests release > > > > of this weekend :) > > > > > > > > > > I was thinking about "fcntl_corner_tests", if this is okay. > > > > Good to me, I'm rename to that if no more objection :) > > > > > > Can we make it "fcntl_lock_corner_tests"? > > These tests are all about file locking, but fcntl() is a generic system > call that does a lot of different things besides that. Oh, you remind me, looks like we should add this case into "locks" group. Any more suggestions? > -- > Jeff Layton <jlayton@kernel.org> >
On Mon, Sep 25, 2023 at 04:18:27PM -0400, Alexander Aring wrote: > This patch adds fcntl corner cases that was being used to confirm issues > on a GFS2 filesystem. The GFS2 filesystem has it's own ->lock() > implementation and in those corner cases issues was being found and > fixed. > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > --- Hi Alexander, This test case directly hang on CIFS testing. Actually it's not a real hang, the fcntl_lock_corner_tests process can be killed, but before killing it manually, it was blocked there. Please check if it's a case issue or cifs bug, or it's not suitable for cifs? I'll try to delay this patch merging to next release, leave some time to have a discission. CC cifs list to get more reviewing. Thanks, Zorro FSTYP -- cifs PLATFORM -- Linux/ppc64le ibm-xxx-xxx-xxxx 6.8.0-rc3+ #1 SMP Wed Feb 7 01:38:35 EST 2024 MKFS_OPTIONS -- //xxx-xxx-xx-xxxx.xxxx.xxx.com/SCRATCH_dev MOUNT_OPTIONS -- -o vers=3.11,mfsymlinks,username=root,password=redhat -o context=system_u:object_r:root_t:s0 //xxx-xxx-xx-xxxx.xxxx.xxx.com/SCRATCH_dev /mnt/xfstests/scratch/cifs-client generic/740 ... ... # ps axu|grep fcntl root 1138909 0.0 0.0 5056 0 ? S Feb07 0:00 /var/lib/xfstests/src/fcntl_lock_corner_tests /mnt/xfstests/test/cifs-client/testfile root 1138910 0.0 0.0 21760 0 ? Sl Feb07 0:00 /var/lib/xfstests/src/fcntl_lock_corner_tests /mnt/xfstests/test/cifs-client/testfile # cat /proc/1138909/stack [<0>] 0xc00000008d068a00 [<0>] __switch_to+0x13c/0x228 [<0>] do_wait+0x15c/0x224 [<0>] kernel_wait4+0xb8/0x2c8 [<0>] system_call_exception+0x134/0x390 [<0>] system_call_vectored_common+0x15c/0x2ec # cat /proc/1138910/stack [<0>] 0xc00000008de94400 [<0>] __switch_to+0x13c/0x228 [<0>] futex_wait_queue+0xa8/0x134 [<0>] __futex_wait+0xb4/0x15c [<0>] futex_wait+0x94/0x150 [<0>] do_futex+0xe8/0x374 [<0>] sys_futex+0xa4/0x234 [<0>] system_call_exception+0x134/0x390 [<0>] system_call_vectored_common+0x15c/0x2ec # strace -p 1138909 strace: Process 1138909 attached wait4(-1, (nothing more output) strace: Process 1138909 detached ^C <detached ...> # strace -p 1138910 strace: Process 1138910 attached futex(0x7fff97a8f110, FUTEX_WAIT_BITSET|FUTEX_CLOCK_REALTIME, 1138912, NULL, FUTEX_BITSET_MATCH_ANY (nothing more output) ^Cstrace: Process 1138910 detached <detached ...> > changes since v2: > - move fcntl tests into one fcntl c file > - remove ofd and same owner tests, should be reflected by only one test > - simplify commit message (remove testname out of it) > - add error messages in fcntl.c to give more information if an error > occur > > src/Makefile | 3 +- > src/fcntl.c | 322 ++++++++++++++++++++++++++++++++++++++++++ > tests/generic/732 | 32 +++++ > tests/generic/732.out | 2 + > 4 files changed, 358 insertions(+), 1 deletion(-) > create mode 100644 src/fcntl.c > create mode 100755 tests/generic/732 > create mode 100644 tests/generic/732.out > > diff --git a/src/Makefile b/src/Makefile > index 2815f919..67f936d3 100644 > --- a/src/Makefile > +++ b/src/Makefile > @@ -19,7 +19,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \ > t_ofd_locks t_mmap_collision mmap-write-concurrent \ > t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \ > t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \ > - t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test > + t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test \ > + fcntl > > LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ > preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \ > diff --git a/src/fcntl.c b/src/fcntl.c > new file mode 100644 > index 00000000..8e375357 > --- /dev/null > +++ b/src/fcntl.c > @@ -0,0 +1,322 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* Copyright (c) 2023 Alexander Aring. All Rights Reserved. > + */ > + > +#include <pthread.h> > +#include <unistd.h> > +#include <string.h> > +#include <limits.h> > +#include <stdlib.h> > +#include <errno.h> > +#include <fcntl.h> > +#include <stdio.h> > +#include <wait.h> > + > +static char filename[PATH_MAX + 1]; > +static int fd; > + > +static void usage(char *name, const char *msg) > +{ > + printf("Fatal: %s\nUsage:\n" > + "%s <file for fcntl>\n", msg, name); > + _exit(1); > +} > + > +static void *do_equal_file_lock_thread(void *arg) > +{ > + struct flock fl = { > + .l_type = F_WRLCK, > + .l_whence = SEEK_SET, > + .l_start = 0L, > + .l_len = 1L, > + }; > + int rv; > + > + rv = fcntl(fd, F_SETLKW, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + return NULL; > +} > + > +static void do_test_equal_file_lock(void) > +{ > + struct flock fl; > + pthread_t t[2]; > + int pid, rv; > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0L; > + fl.l_len = 1L; > + > + /* acquire range 0-0 */ > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + pid = fork(); > + if (pid == 0) { > + rv = pthread_create(&t[0], NULL, do_equal_file_lock_thread, NULL); > + if (rv != 0) { > + fprintf(stderr, "failed to create pthread\n"); > + _exit(1); > + } > + > + rv = pthread_create(&t[1], NULL, do_equal_file_lock_thread, NULL); > + if (rv != 0) { > + fprintf(stderr, "failed to create pthread\n"); > + _exit(1); > + } > + > + pthread_join(t[0], NULL); > + pthread_join(t[1], NULL); > + > + _exit(0); > + } > + > + /* wait until threads block on 0-0 */ > + sleep(3); > + > + fl.l_type = F_UNLCK; > + fl.l_start = 0; > + fl.l_len = 1; > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + sleep(3); > + > + /* check if the ->lock() implementation got the > + * right locks granted because two waiter with the > + * same file_lock fields are waiting > + */ > + fl.l_type = F_WRLCK; > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1 && errno == EAGAIN) { > + fprintf(stderr, "deadlock, pthread not cleaned up correctly\n"); > + _exit(1); > + } > + > + wait(NULL); > +} > + > +static void catch_alarm(int num) { } > + > +static void do_test_signal_interrupt_child(int *pfd) > +{ > + struct sigaction act; > + unsigned char m; > + struct flock fl; > + int rv; > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 1; > + fl.l_len = 1; > + > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + memset(&act, 0, sizeof(act)); > + act.sa_handler = catch_alarm; > + sigemptyset(&act.sa_mask); > + sigaddset(&act.sa_mask, SIGALRM); > + sigaction(SIGALRM, &act, NULL); > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + /* interrupt SETLKW by signal in 3 secs */ > + alarm(3); > + rv = fcntl(fd, F_SETLKW, &fl); > + if (rv == 0) { > + fprintf(stderr, "fcntl interrupt successful but should fail with EINTR\n"); > + _exit(1); > + } > + > + /* synchronize to move parent to test region 1-1 */ > + write(pfd[1], &m, sizeof(m)); > + > + /* keep child alive */ > + read(pfd[1], &m, sizeof(m)); > +} > + > +static void do_test_signal_interrupt(void) > +{ > + struct flock fl; > + unsigned char m; > + int pid, rv; > + int pfd[2]; > + > + rv = pipe(pfd); > + if (rv == -1) { > + perror("pipe"); > + _exit(1); > + } > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + pid = fork(); > + if (pid == 0) { > + do_test_signal_interrupt_child(pfd); > + _exit(0); > + } > + > + /* wait until child writes */ > + read(pfd[0], &m, sizeof(m)); > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 1; > + fl.l_len = 1; > + /* parent testing childs region, the child will think > + * it has region 1-1 locked because it was interrupted > + * by region 0-0. Due bugs the interruption also unlocked > + * region 1-1. > + */ > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == 0) { > + fprintf(stderr, "fcntl trylock successful but should fail because child still acquires region\n"); > + _exit(1); > + } > + > + write(pfd[0], &m, sizeof(m)); > + > + wait(NULL); > + > + close(pfd[0]); > + close(pfd[1]); > + > + /* cleanup everything */ > + fl.l_type = F_UNLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 2; > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > +} > + > +static void do_test_kill_child(void) > +{ > + struct flock fl; > + int rv; > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + rv = fcntl(fd, F_SETLKW, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > +} > + > +static void do_test_kill(void) > +{ > + struct flock fl; > + int pid_to_kill; > + int rv; > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + pid_to_kill = fork(); > + if (pid_to_kill == 0) { > + do_test_kill_child(); > + _exit(0); > + } > + > + /* wait until child blocks */ > + sleep(3); > + > + kill(pid_to_kill, SIGKILL); > + > + /* wait until Linux did plock cleanup */ > + sleep(3); > + > + fl.l_type = F_UNLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + /* cleanup parent lock */ > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + /* check if the child still holds the lock > + * and killing the child was not cleaning > + * up locks. > + */ > + rv = fcntl(fd, F_SETLK, &fl); > + if ((rv == -1 && errno == EAGAIN)) { > + fprintf(stderr, "fcntl trylock failed but should be successful because killing child should cleanup acquired lock\n"); > + _exit(1); > + } > +} > + > +int main(int argc, char * const argv[]) > +{ > + if (optind != argc - 1) > + usage(argv[0], "<file for fcntl> is mandatory to tell the file where to run fcntl() on it"); > + > + strncpy(filename, argv[1], PATH_MAX); > + > + fd = open(filename, O_RDWR | O_CREAT, 0700); > + if (fd == -1) { > + perror("open"); > + _exit(1); > + } > + > + /* test to have to equal struct file_lock requests in ->lock() */ > + do_test_equal_file_lock(); > + /* test to interrupt F_SETLKW by a signal and cleanup only canceled the pending interrupted request */ > + do_test_signal_interrupt(); > + /* test if cleanup is correct if a child gets killed while being blocked in F_SETLKW */ > + do_test_kill(); > + > + close(fd); > + > + return 0; > +} > diff --git a/tests/generic/732 b/tests/generic/732 > new file mode 100755 > index 00000000..d77f9fc2 > --- /dev/null > +++ b/tests/generic/732 > @@ -0,0 +1,32 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2023 Alexander Aring. All Rights Reserved. > +# > +# FS QA Test 732 > +# > +# This tests performs some fcntl() corner cases. See fcntl test > +# program for more details. > +# > +. ./common/preamble > +_begin_fstest auto > + > +# Import common functions. > +. ./common/filter > + > +# real QA test starts here > + > +# Modify as appropriate. > +_supported_fs generic > +_require_test > +_require_test_program fcntl > + > +echo "Silence is golden" > + > +$here/src/fcntl $TEST_DIR/testfile > +if [ $? -ne 0 ] > +then > + exit > +fi > + > +status=0 > +exit > diff --git a/tests/generic/732.out b/tests/generic/732.out > new file mode 100644 > index 00000000..451f82ce > --- /dev/null > +++ b/tests/generic/732.out > @@ -0,0 +1,2 @@ > +QA output created by 732 > +Silence is golden > -- > 2.31.1 >
Could you forward the changeset for this so we could try it? On Thu, Feb 8, 2024 at 11:26 PM Zorro Lang <zlang@redhat.com> wrote: > > On Mon, Sep 25, 2023 at 04:18:27PM -0400, Alexander Aring wrote: > > This patch adds fcntl corner cases that was being used to confirm issues > > on a GFS2 filesystem. The GFS2 filesystem has it's own ->lock() > > implementation and in those corner cases issues was being found and > > fixed. > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > --- > > Hi Alexander, > > This test case directly hang on CIFS testing. Actually it's not a > real hang, the fcntl_lock_corner_tests process can be killed, but > before killing it manually, it was blocked there. > > Please check if it's a case issue or cifs bug, or it's not suitable > for cifs? I'll try to delay this patch merging to next release, > leave some time to have a discission. CC cifs list to get more > reviewing. > > Thanks, > Zorro > > FSTYP -- cifs > PLATFORM -- Linux/ppc64le ibm-xxx-xxx-xxxx 6.8.0-rc3+ #1 SMP Wed Feb 7 01:38:35 EST 2024 > MKFS_OPTIONS -- //xxx-xxx-xx-xxxx.xxxx.xxx.com/SCRATCH_dev > MOUNT_OPTIONS -- -o vers=3.11,mfsymlinks,username=root,password=redhat -o context=system_u:object_r:root_t:s0 //xxx-xxx-xx-xxxx.xxxx.xxx.com/SCRATCH_dev /mnt/xfstests/scratch/cifs-client > > generic/740 > ... > ... > > # ps axu|grep fcntl > root 1138909 0.0 0.0 5056 0 ? S Feb07 0:00 /var/lib/xfstests/src/fcntl_lock_corner_tests /mnt/xfstests/test/cifs-client/testfile > root 1138910 0.0 0.0 21760 0 ? Sl Feb07 0:00 /var/lib/xfstests/src/fcntl_lock_corner_tests /mnt/xfstests/test/cifs-client/testfile > # cat /proc/1138909/stack > [<0>] 0xc00000008d068a00 > [<0>] __switch_to+0x13c/0x228 > [<0>] do_wait+0x15c/0x224 > [<0>] kernel_wait4+0xb8/0x2c8 > [<0>] system_call_exception+0x134/0x390 > [<0>] system_call_vectored_common+0x15c/0x2ec > # cat /proc/1138910/stack > [<0>] 0xc00000008de94400 > [<0>] __switch_to+0x13c/0x228 > [<0>] futex_wait_queue+0xa8/0x134 > [<0>] __futex_wait+0xb4/0x15c > [<0>] futex_wait+0x94/0x150 > [<0>] do_futex+0xe8/0x374 > [<0>] sys_futex+0xa4/0x234 > [<0>] system_call_exception+0x134/0x390 > [<0>] system_call_vectored_common+0x15c/0x2ec > > # strace -p 1138909 > strace: Process 1138909 attached > wait4(-1, > (nothing more output) > strace: Process 1138909 detached > ^C <detached ...> > # strace -p 1138910 > strace: Process 1138910 attached > futex(0x7fff97a8f110, FUTEX_WAIT_BITSET|FUTEX_CLOCK_REALTIME, 1138912, NULL, FUTEX_BITSET_MATCH_ANY > (nothing more output) > ^Cstrace: Process 1138910 detached > <detached ...> > > > > changes since v2: > > - move fcntl tests into one fcntl c file > > - remove ofd and same owner tests, should be reflected by only one test > > - simplify commit message (remove testname out of it) > > - add error messages in fcntl.c to give more information if an error > > occur > > > > src/Makefile | 3 +- > > src/fcntl.c | 322 ++++++++++++++++++++++++++++++++++++++++++ > > tests/generic/732 | 32 +++++ > > tests/generic/732.out | 2 + > > 4 files changed, 358 insertions(+), 1 deletion(-) > > create mode 100644 src/fcntl.c > > create mode 100755 tests/generic/732 > > create mode 100644 tests/generic/732.out > > > > diff --git a/src/Makefile b/src/Makefile > > index 2815f919..67f936d3 100644 > > --- a/src/Makefile > > +++ b/src/Makefile > > @@ -19,7 +19,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \ > > t_ofd_locks t_mmap_collision mmap-write-concurrent \ > > t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \ > > t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \ > > - t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test > > + t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test \ > > + fcntl > > > > LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ > > preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \ > > diff --git a/src/fcntl.c b/src/fcntl.c > > new file mode 100644 > > index 00000000..8e375357 > > --- /dev/null > > +++ b/src/fcntl.c > > @@ -0,0 +1,322 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* Copyright (c) 2023 Alexander Aring. All Rights Reserved. > > + */ > > + > > +#include <pthread.h> > > +#include <unistd.h> > > +#include <string.h> > > +#include <limits.h> > > +#include <stdlib.h> > > +#include <errno.h> > > +#include <fcntl.h> > > +#include <stdio.h> > > +#include <wait.h> > > + > > +static char filename[PATH_MAX + 1]; > > +static int fd; > > + > > +static void usage(char *name, const char *msg) > > +{ > > + printf("Fatal: %s\nUsage:\n" > > + "%s <file for fcntl>\n", msg, name); > > + _exit(1); > > +} > > + > > +static void *do_equal_file_lock_thread(void *arg) > > +{ > > + struct flock fl = { > > + .l_type = F_WRLCK, > > + .l_whence = SEEK_SET, > > + .l_start = 0L, > > + .l_len = 1L, > > + }; > > + int rv; > > + > > + rv = fcntl(fd, F_SETLKW, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > + > > + return NULL; > > +} > > + > > +static void do_test_equal_file_lock(void) > > +{ > > + struct flock fl; > > + pthread_t t[2]; > > + int pid, rv; > > + > > + fl.l_type = F_WRLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 0L; > > + fl.l_len = 1L; > > + > > + /* acquire range 0-0 */ > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > + > > + pid = fork(); > > + if (pid == 0) { > > + rv = pthread_create(&t[0], NULL, do_equal_file_lock_thread, NULL); > > + if (rv != 0) { > > + fprintf(stderr, "failed to create pthread\n"); > > + _exit(1); > > + } > > + > > + rv = pthread_create(&t[1], NULL, do_equal_file_lock_thread, NULL); > > + if (rv != 0) { > > + fprintf(stderr, "failed to create pthread\n"); > > + _exit(1); > > + } > > + > > + pthread_join(t[0], NULL); > > + pthread_join(t[1], NULL); > > + > > + _exit(0); > > + } > > + > > + /* wait until threads block on 0-0 */ > > + sleep(3); > > + > > + fl.l_type = F_UNLCK; > > + fl.l_start = 0; > > + fl.l_len = 1; > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > + > > + sleep(3); > > + > > + /* check if the ->lock() implementation got the > > + * right locks granted because two waiter with the > > + * same file_lock fields are waiting > > + */ > > + fl.l_type = F_WRLCK; > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == -1 && errno == EAGAIN) { > > + fprintf(stderr, "deadlock, pthread not cleaned up correctly\n"); > > + _exit(1); > > + } > > + > > + wait(NULL); > > +} > > + > > +static void catch_alarm(int num) { } > > + > > +static void do_test_signal_interrupt_child(int *pfd) > > +{ > > + struct sigaction act; > > + unsigned char m; > > + struct flock fl; > > + int rv; > > + > > + fl.l_type = F_WRLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 1; > > + fl.l_len = 1; > > + > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > + > > + memset(&act, 0, sizeof(act)); > > + act.sa_handler = catch_alarm; > > + sigemptyset(&act.sa_mask); > > + sigaddset(&act.sa_mask, SIGALRM); > > + sigaction(SIGALRM, &act, NULL); > > + > > + fl.l_type = F_WRLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 0; > > + fl.l_len = 1; > > + > > + /* interrupt SETLKW by signal in 3 secs */ > > + alarm(3); > > + rv = fcntl(fd, F_SETLKW, &fl); > > + if (rv == 0) { > > + fprintf(stderr, "fcntl interrupt successful but should fail with EINTR\n"); > > + _exit(1); > > + } > > + > > + /* synchronize to move parent to test region 1-1 */ > > + write(pfd[1], &m, sizeof(m)); > > + > > + /* keep child alive */ > > + read(pfd[1], &m, sizeof(m)); > > +} > > + > > +static void do_test_signal_interrupt(void) > > +{ > > + struct flock fl; > > + unsigned char m; > > + int pid, rv; > > + int pfd[2]; > > + > > + rv = pipe(pfd); > > + if (rv == -1) { > > + perror("pipe"); > > + _exit(1); > > + } > > + > > + fl.l_type = F_WRLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 0; > > + fl.l_len = 1; > > + > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > + > > + pid = fork(); > > + if (pid == 0) { > > + do_test_signal_interrupt_child(pfd); > > + _exit(0); > > + } > > + > > + /* wait until child writes */ > > + read(pfd[0], &m, sizeof(m)); > > + > > + fl.l_type = F_WRLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 1; > > + fl.l_len = 1; > > + /* parent testing childs region, the child will think > > + * it has region 1-1 locked because it was interrupted > > + * by region 0-0. Due bugs the interruption also unlocked > > + * region 1-1. > > + */ > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == 0) { > > + fprintf(stderr, "fcntl trylock successful but should fail because child still acquires region\n"); > > + _exit(1); > > + } > > + > > + write(pfd[0], &m, sizeof(m)); > > + > > + wait(NULL); > > + > > + close(pfd[0]); > > + close(pfd[1]); > > + > > + /* cleanup everything */ > > + fl.l_type = F_UNLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 0; > > + fl.l_len = 2; > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > +} > > + > > +static void do_test_kill_child(void) > > +{ > > + struct flock fl; > > + int rv; > > + > > + fl.l_type = F_WRLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 0; > > + fl.l_len = 1; > > + > > + rv = fcntl(fd, F_SETLKW, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > +} > > + > > +static void do_test_kill(void) > > +{ > > + struct flock fl; > > + int pid_to_kill; > > + int rv; > > + > > + fl.l_type = F_WRLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 0; > > + fl.l_len = 1; > > + > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > + > > + pid_to_kill = fork(); > > + if (pid_to_kill == 0) { > > + do_test_kill_child(); > > + _exit(0); > > + } > > + > > + /* wait until child blocks */ > > + sleep(3); > > + > > + kill(pid_to_kill, SIGKILL); > > + > > + /* wait until Linux did plock cleanup */ > > + sleep(3); > > + > > + fl.l_type = F_UNLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 0; > > + fl.l_len = 1; > > + > > + /* cleanup parent lock */ > > + rv = fcntl(fd, F_SETLK, &fl); > > + if (rv == -1) { > > + perror("fcntl"); > > + _exit(1); > > + } > > + > > + fl.l_type = F_WRLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 0; > > + fl.l_len = 1; > > + > > + /* check if the child still holds the lock > > + * and killing the child was not cleaning > > + * up locks. > > + */ > > + rv = fcntl(fd, F_SETLK, &fl); > > + if ((rv == -1 && errno == EAGAIN)) { > > + fprintf(stderr, "fcntl trylock failed but should be successful because killing child should cleanup acquired lock\n"); > > + _exit(1); > > + } > > +} > > + > > +int main(int argc, char * const argv[]) > > +{ > > + if (optind != argc - 1) > > + usage(argv[0], "<file for fcntl> is mandatory to tell the file where to run fcntl() on it"); > > + > > + strncpy(filename, argv[1], PATH_MAX); > > + > > + fd = open(filename, O_RDWR | O_CREAT, 0700); > > + if (fd == -1) { > > + perror("open"); > > + _exit(1); > > + } > > + > > + /* test to have to equal struct file_lock requests in ->lock() */ > > + do_test_equal_file_lock(); > > + /* test to interrupt F_SETLKW by a signal and cleanup only canceled the pending interrupted request */ > > + do_test_signal_interrupt(); > > + /* test if cleanup is correct if a child gets killed while being blocked in F_SETLKW */ > > + do_test_kill(); > > + > > + close(fd); > > + > > + return 0; > > +} > > diff --git a/tests/generic/732 b/tests/generic/732 > > new file mode 100755 > > index 00000000..d77f9fc2 > > --- /dev/null > > +++ b/tests/generic/732 > > @@ -0,0 +1,32 @@ > > +#! /bin/bash > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright (c) 2023 Alexander Aring. All Rights Reserved. > > +# > > +# FS QA Test 732 > > +# > > +# This tests performs some fcntl() corner cases. See fcntl test > > +# program for more details. > > +# > > +. ./common/preamble > > +_begin_fstest auto > > + > > +# Import common functions. > > +. ./common/filter > > + > > +# real QA test starts here > > + > > +# Modify as appropriate. > > +_supported_fs generic > > +_require_test > > +_require_test_program fcntl > > + > > +echo "Silence is golden" > > + > > +$here/src/fcntl $TEST_DIR/testfile > > +if [ $? -ne 0 ] > > +then > > + exit > > +fi > > + > > +status=0 > > +exit > > diff --git a/tests/generic/732.out b/tests/generic/732.out > > new file mode 100644 > > index 00000000..451f82ce > > --- /dev/null > > +++ b/tests/generic/732.out > > @@ -0,0 +1,2 @@ > > +QA output created by 732 > > +Silence is golden > > -- > > 2.31.1 > > > >
On Thu, Feb 08, 2024 at 11:35:06PM -0600, Steve French wrote: > Could you forward the changeset for this so we could try it? Hi Steve, Thanks for your reply. Sure, there's a temporary branch "patches-in-queue", you can get it by: # git clone -b patches-in-queue git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git Then you can see a generic/740 test case in it, run it on cifs. I tested with cifs 3.11, and wrote the /etc/samba/smb.conf as: [TEST_dev] path = $dir writable = yes ea support = yes strict allocate = yes And set: MOUNT_OPTIONS="-o vers=3.11,mfsymlinks,username=root,password=xxxxxxxxxx" TEST_FS_MOUNT_OPTS="-o vers=3.11,mfsymlinks,username=root,password=xxxxxxxx" I'm not sure if it's a cifs issue, Alexander writes this case for gfs2, it works for other fs, but blocked (until kill it manually) on cifs. So hope to get some suggestions from cifs list. Thanks, Zorro > > On Thu, Feb 8, 2024 at 11:26 PM Zorro Lang <zlang@redhat.com> wrote: > > > > On Mon, Sep 25, 2023 at 04:18:27PM -0400, Alexander Aring wrote: > > > This patch adds fcntl corner cases that was being used to confirm issues > > > on a GFS2 filesystem. The GFS2 filesystem has it's own ->lock() > > > implementation and in those corner cases issues was being found and > > > fixed. > > > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > > --- > > > > Hi Alexander, > > > > This test case directly hang on CIFS testing. Actually it's not a > > real hang, the fcntl_lock_corner_tests process can be killed, but > > before killing it manually, it was blocked there. > > > > Please check if it's a case issue or cifs bug, or it's not suitable > > for cifs? I'll try to delay this patch merging to next release, > > leave some time to have a discission. CC cifs list to get more > > reviewing. > > > > Thanks, > > Zorro > > > > FSTYP -- cifs > > PLATFORM -- Linux/ppc64le ibm-xxx-xxx-xxxx 6.8.0-rc3+ #1 SMP Wed Feb 7 01:38:35 EST 2024 > > MKFS_OPTIONS -- //xxx-xxx-xx-xxxx.xxxx.xxx.com/SCRATCH_dev > > MOUNT_OPTIONS -- -o vers=3.11,mfsymlinks,username=root,password=redhat -o context=system_u:object_r:root_t:s0 //xxx-xxx-xx-xxxx.xxxx.xxx.com/SCRATCH_dev /mnt/xfstests/scratch/cifs-client > > > > generic/740 > > ... > > ... > > > > # ps axu|grep fcntl > > root 1138909 0.0 0.0 5056 0 ? S Feb07 0:00 /var/lib/xfstests/src/fcntl_lock_corner_tests /mnt/xfstests/test/cifs-client/testfile > > root 1138910 0.0 0.0 21760 0 ? Sl Feb07 0:00 /var/lib/xfstests/src/fcntl_lock_corner_tests /mnt/xfstests/test/cifs-client/testfile > > # cat /proc/1138909/stack > > [<0>] 0xc00000008d068a00 > > [<0>] __switch_to+0x13c/0x228 > > [<0>] do_wait+0x15c/0x224 > > [<0>] kernel_wait4+0xb8/0x2c8 > > [<0>] system_call_exception+0x134/0x390 > > [<0>] system_call_vectored_common+0x15c/0x2ec > > # cat /proc/1138910/stack > > [<0>] 0xc00000008de94400 > > [<0>] __switch_to+0x13c/0x228 > > [<0>] futex_wait_queue+0xa8/0x134 > > [<0>] __futex_wait+0xb4/0x15c > > [<0>] futex_wait+0x94/0x150 > > [<0>] do_futex+0xe8/0x374 > > [<0>] sys_futex+0xa4/0x234 > > [<0>] system_call_exception+0x134/0x390 > > [<0>] system_call_vectored_common+0x15c/0x2ec > > > > # strace -p 1138909 > > strace: Process 1138909 attached > > wait4(-1, > > (nothing more output) > > strace: Process 1138909 detached > > ^C <detached ...> > > # strace -p 1138910 > > strace: Process 1138910 attached > > futex(0x7fff97a8f110, FUTEX_WAIT_BITSET|FUTEX_CLOCK_REALTIME, 1138912, NULL, FUTEX_BITSET_MATCH_ANY > > (nothing more output) > > ^Cstrace: Process 1138910 detached > > <detached ...> > > > > > > > changes since v2: > > > - move fcntl tests into one fcntl c file > > > - remove ofd and same owner tests, should be reflected by only one test > > > - simplify commit message (remove testname out of it) > > > - add error messages in fcntl.c to give more information if an error > > > occur > > > > > > src/Makefile | 3 +- > > > src/fcntl.c | 322 ++++++++++++++++++++++++++++++++++++++++++ > > > tests/generic/732 | 32 +++++ > > > tests/generic/732.out | 2 + > > > 4 files changed, 358 insertions(+), 1 deletion(-) > > > create mode 100644 src/fcntl.c > > > create mode 100755 tests/generic/732 > > > create mode 100644 tests/generic/732.out > > > > > > diff --git a/src/Makefile b/src/Makefile > > > index 2815f919..67f936d3 100644 > > > --- a/src/Makefile > > > +++ b/src/Makefile > > > @@ -19,7 +19,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \ > > > t_ofd_locks t_mmap_collision mmap-write-concurrent \ > > > t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \ > > > t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \ > > > - t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test > > > + t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test \ > > > + fcntl > > > > > > LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ > > > preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \ > > > diff --git a/src/fcntl.c b/src/fcntl.c > > > new file mode 100644 > > > index 00000000..8e375357 > > > --- /dev/null > > > +++ b/src/fcntl.c > > > @@ -0,0 +1,322 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > +/* Copyright (c) 2023 Alexander Aring. All Rights Reserved. > > > + */ > > > + > > > +#include <pthread.h> > > > +#include <unistd.h> > > > +#include <string.h> > > > +#include <limits.h> > > > +#include <stdlib.h> > > > +#include <errno.h> > > > +#include <fcntl.h> > > > +#include <stdio.h> > > > +#include <wait.h> > > > + > > > +static char filename[PATH_MAX + 1]; > > > +static int fd; > > > + > > > +static void usage(char *name, const char *msg) > > > +{ > > > + printf("Fatal: %s\nUsage:\n" > > > + "%s <file for fcntl>\n", msg, name); > > > + _exit(1); > > > +} > > > + > > > +static void *do_equal_file_lock_thread(void *arg) > > > +{ > > > + struct flock fl = { > > > + .l_type = F_WRLCK, > > > + .l_whence = SEEK_SET, > > > + .l_start = 0L, > > > + .l_len = 1L, > > > + }; > > > + int rv; > > > + > > > + rv = fcntl(fd, F_SETLKW, &fl); > > > + if (rv == -1) { > > > + perror("fcntl"); > > > + _exit(1); > > > + } > > > + > > > + return NULL; > > > +} > > > + > > > +static void do_test_equal_file_lock(void) > > > +{ > > > + struct flock fl; > > > + pthread_t t[2]; > > > + int pid, rv; > > > + > > > + fl.l_type = F_WRLCK; > > > + fl.l_whence = SEEK_SET; > > > + fl.l_start = 0L; > > > + fl.l_len = 1L; > > > + > > > + /* acquire range 0-0 */ > > > + rv = fcntl(fd, F_SETLK, &fl); > > > + if (rv == -1) { > > > + perror("fcntl"); > > > + _exit(1); > > > + } > > > + > > > + pid = fork(); > > > + if (pid == 0) { > > > + rv = pthread_create(&t[0], NULL, do_equal_file_lock_thread, NULL); > > > + if (rv != 0) { > > > + fprintf(stderr, "failed to create pthread\n"); > > > + _exit(1); > > > + } > > > + > > > + rv = pthread_create(&t[1], NULL, do_equal_file_lock_thread, NULL); > > > + if (rv != 0) { > > > + fprintf(stderr, "failed to create pthread\n"); > > > + _exit(1); > > > + } > > > + > > > + pthread_join(t[0], NULL); > > > + pthread_join(t[1], NULL); > > > + > > > + _exit(0); > > > + } > > > + > > > + /* wait until threads block on 0-0 */ > > > + sleep(3); > > > + > > > + fl.l_type = F_UNLCK; > > > + fl.l_start = 0; > > > + fl.l_len = 1; > > > + rv = fcntl(fd, F_SETLK, &fl); > > > + if (rv == -1) { > > > + perror("fcntl"); > > > + _exit(1); > > > + } > > > + > > > + sleep(3); > > > + > > > + /* check if the ->lock() implementation got the > > > + * right locks granted because two waiter with the > > > + * same file_lock fields are waiting > > > + */ > > > + fl.l_type = F_WRLCK; > > > + rv = fcntl(fd, F_SETLK, &fl); > > > + if (rv == -1 && errno == EAGAIN) { > > > + fprintf(stderr, "deadlock, pthread not cleaned up correctly\n"); > > > + _exit(1); > > > + } > > > + > > > + wait(NULL); > > > +} > > > + > > > +static void catch_alarm(int num) { } > > > + > > > +static void do_test_signal_interrupt_child(int *pfd) > > > +{ > > > + struct sigaction act; > > > + unsigned char m; > > > + struct flock fl; > > > + int rv; > > > + > > > + fl.l_type = F_WRLCK; > > > + fl.l_whence = SEEK_SET; > > > + fl.l_start = 1; > > > + fl.l_len = 1; > > > + > > > + rv = fcntl(fd, F_SETLK, &fl); > > > + if (rv == -1) { > > > + perror("fcntl"); > > > + _exit(1); > > > + } > > > + > > > + memset(&act, 0, sizeof(act)); > > > + act.sa_handler = catch_alarm; > > > + sigemptyset(&act.sa_mask); > > > + sigaddset(&act.sa_mask, SIGALRM); > > > + sigaction(SIGALRM, &act, NULL); > > > + > > > + fl.l_type = F_WRLCK; > > > + fl.l_whence = SEEK_SET; > > > + fl.l_start = 0; > > > + fl.l_len = 1; > > > + > > > + /* interrupt SETLKW by signal in 3 secs */ > > > + alarm(3); > > > + rv = fcntl(fd, F_SETLKW, &fl); > > > + if (rv == 0) { > > > + fprintf(stderr, "fcntl interrupt successful but should fail with EINTR\n"); > > > + _exit(1); > > > + } > > > + > > > + /* synchronize to move parent to test region 1-1 */ > > > + write(pfd[1], &m, sizeof(m)); > > > + > > > + /* keep child alive */ > > > + read(pfd[1], &m, sizeof(m)); > > > +} > > > + > > > +static void do_test_signal_interrupt(void) > > > +{ > > > + struct flock fl; > > > + unsigned char m; > > > + int pid, rv; > > > + int pfd[2]; > > > + > > > + rv = pipe(pfd); > > > + if (rv == -1) { > > > + perror("pipe"); > > > + _exit(1); > > > + } > > > + > > > + fl.l_type = F_WRLCK; > > > + fl.l_whence = SEEK_SET; > > > + fl.l_start = 0; > > > + fl.l_len = 1; > > > + > > > + rv = fcntl(fd, F_SETLK, &fl); > > > + if (rv == -1) { > > > + perror("fcntl"); > > > + _exit(1); > > > + } > > > + > > > + pid = fork(); > > > + if (pid == 0) { > > > + do_test_signal_interrupt_child(pfd); > > > + _exit(0); > > > + } > > > + > > > + /* wait until child writes */ > > > + read(pfd[0], &m, sizeof(m)); > > > + > > > + fl.l_type = F_WRLCK; > > > + fl.l_whence = SEEK_SET; > > > + fl.l_start = 1; > > > + fl.l_len = 1; > > > + /* parent testing childs region, the child will think > > > + * it has region 1-1 locked because it was interrupted > > > + * by region 0-0. Due bugs the interruption also unlocked > > > + * region 1-1. > > > + */ > > > + rv = fcntl(fd, F_SETLK, &fl); > > > + if (rv == 0) { > > > + fprintf(stderr, "fcntl trylock successful but should fail because child still acquires region\n"); > > > + _exit(1); > > > + } > > > + > > > + write(pfd[0], &m, sizeof(m)); > > > + > > > + wait(NULL); > > > + > > > + close(pfd[0]); > > > + close(pfd[1]); > > > + > > > + /* cleanup everything */ > > > + fl.l_type = F_UNLCK; > > > + fl.l_whence = SEEK_SET; > > > + fl.l_start = 0; > > > + fl.l_len = 2; > > > + rv = fcntl(fd, F_SETLK, &fl); > > > + if (rv == -1) { > > > + perror("fcntl"); > > > + _exit(1); > > > + } > > > +} > > > + > > > +static void do_test_kill_child(void) > > > +{ > > > + struct flock fl; > > > + int rv; > > > + > > > + fl.l_type = F_WRLCK; > > > + fl.l_whence = SEEK_SET; > > > + fl.l_start = 0; > > > + fl.l_len = 1; > > > + > > > + rv = fcntl(fd, F_SETLKW, &fl); > > > + if (rv == -1) { > > > + perror("fcntl"); > > > + _exit(1); > > > + } > > > +} > > > + > > > +static void do_test_kill(void) > > > +{ > > > + struct flock fl; > > > + int pid_to_kill; > > > + int rv; > > > + > > > + fl.l_type = F_WRLCK; > > > + fl.l_whence = SEEK_SET; > > > + fl.l_start = 0; > > > + fl.l_len = 1; > > > + > > > + rv = fcntl(fd, F_SETLK, &fl); > > > + if (rv == -1) { > > > + perror("fcntl"); > > > + _exit(1); > > > + } > > > + > > > + pid_to_kill = fork(); > > > + if (pid_to_kill == 0) { > > > + do_test_kill_child(); > > > + _exit(0); > > > + } > > > + > > > + /* wait until child blocks */ > > > + sleep(3); > > > + > > > + kill(pid_to_kill, SIGKILL); > > > + > > > + /* wait until Linux did plock cleanup */ > > > + sleep(3); > > > + > > > + fl.l_type = F_UNLCK; > > > + fl.l_whence = SEEK_SET; > > > + fl.l_start = 0; > > > + fl.l_len = 1; > > > + > > > + /* cleanup parent lock */ > > > + rv = fcntl(fd, F_SETLK, &fl); > > > + if (rv == -1) { > > > + perror("fcntl"); > > > + _exit(1); > > > + } > > > + > > > + fl.l_type = F_WRLCK; > > > + fl.l_whence = SEEK_SET; > > > + fl.l_start = 0; > > > + fl.l_len = 1; > > > + > > > + /* check if the child still holds the lock > > > + * and killing the child was not cleaning > > > + * up locks. > > > + */ > > > + rv = fcntl(fd, F_SETLK, &fl); > > > + if ((rv == -1 && errno == EAGAIN)) { > > > + fprintf(stderr, "fcntl trylock failed but should be successful because killing child should cleanup acquired lock\n"); > > > + _exit(1); > > > + } > > > +} > > > + > > > +int main(int argc, char * const argv[]) > > > +{ > > > + if (optind != argc - 1) > > > + usage(argv[0], "<file for fcntl> is mandatory to tell the file where to run fcntl() on it"); > > > + > > > + strncpy(filename, argv[1], PATH_MAX); > > > + > > > + fd = open(filename, O_RDWR | O_CREAT, 0700); > > > + if (fd == -1) { > > > + perror("open"); > > > + _exit(1); > > > + } > > > + > > > + /* test to have to equal struct file_lock requests in ->lock() */ > > > + do_test_equal_file_lock(); > > > + /* test to interrupt F_SETLKW by a signal and cleanup only canceled the pending interrupted request */ > > > + do_test_signal_interrupt(); > > > + /* test if cleanup is correct if a child gets killed while being blocked in F_SETLKW */ > > > + do_test_kill(); > > > + > > > + close(fd); > > > + > > > + return 0; > > > +} > > > diff --git a/tests/generic/732 b/tests/generic/732 > > > new file mode 100755 > > > index 00000000..d77f9fc2 > > > --- /dev/null > > > +++ b/tests/generic/732 > > > @@ -0,0 +1,32 @@ > > > +#! /bin/bash > > > +# SPDX-License-Identifier: GPL-2.0 > > > +# Copyright (c) 2023 Alexander Aring. All Rights Reserved. > > > +# > > > +# FS QA Test 732 > > > +# > > > +# This tests performs some fcntl() corner cases. See fcntl test > > > +# program for more details. > > > +# > > > +. ./common/preamble > > > +_begin_fstest auto > > > + > > > +# Import common functions. > > > +. ./common/filter > > > + > > > +# real QA test starts here > > > + > > > +# Modify as appropriate. > > > +_supported_fs generic > > > +_require_test > > > +_require_test_program fcntl > > > + > > > +echo "Silence is golden" > > > + > > > +$here/src/fcntl $TEST_DIR/testfile > > > +if [ $? -ne 0 ] > > > +then > > > + exit > > > +fi > > > + > > > +status=0 > > > +exit > > > diff --git a/tests/generic/732.out b/tests/generic/732.out > > > new file mode 100644 > > > index 00000000..451f82ce > > > --- /dev/null > > > +++ b/tests/generic/732.out > > > @@ -0,0 +1,2 @@ > > > +QA output created by 732 > > > +Silence is golden > > > -- > > > 2.31.1 > > > > > > > > > > -- > Thanks, > > Steve >
On Fri, Feb 09, 2024 at 07:43:10PM +0800, Zorro Lang wrote: > On Thu, Feb 08, 2024 at 11:35:06PM -0600, Steve French wrote: > > Could you forward the changeset for this so we could try it? > > Hi Steve, > > Thanks for your reply. Sure, there's a temporary branch "patches-in-queue", > you can get it by: > # git clone -b patches-in-queue git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git > > Then you can see a generic/740 test case in it, run it on cifs. I tested > with cifs 3.11, and wrote the /etc/samba/smb.conf as: > [TEST_dev] > path = $dir > writable = yes > ea support = yes > strict allocate = yes > And set: > MOUNT_OPTIONS="-o vers=3.11,mfsymlinks,username=root,password=xxxxxxxxxx" > TEST_FS_MOUNT_OPTS="-o vers=3.11,mfsymlinks,username=root,password=xxxxxxxx" > > I'm not sure if it's a cifs issue, Alexander writes this case for gfs2, it > works for other fs, but blocked (until kill it manually) on cifs. So hope > to get some suggestions from cifs list. Hi Steve, Any feedback about this? Just to make sure if it's a cifs bug or a case issue. Thanks, Zorro > > Thanks, > Zorro > > > > > On Thu, Feb 8, 2024 at 11:26 PM Zorro Lang <zlang@redhat.com> wrote: > > > > > > On Mon, Sep 25, 2023 at 04:18:27PM -0400, Alexander Aring wrote: > > > > This patch adds fcntl corner cases that was being used to confirm issues > > > > on a GFS2 filesystem. The GFS2 filesystem has it's own ->lock() > > > > implementation and in those corner cases issues was being found and > > > > fixed. > > > > > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > > > --- > > > > > > Hi Alexander, > > > > > > This test case directly hang on CIFS testing. Actually it's not a > > > real hang, the fcntl_lock_corner_tests process can be killed, but > > > before killing it manually, it was blocked there. > > > > > > Please check if it's a case issue or cifs bug, or it's not suitable > > > for cifs? I'll try to delay this patch merging to next release, > > > leave some time to have a discission. CC cifs list to get more > > > reviewing. > > > > > > Thanks, > > > Zorro > > > > > > FSTYP -- cifs > > > PLATFORM -- Linux/ppc64le ibm-xxx-xxx-xxxx 6.8.0-rc3+ #1 SMP Wed Feb 7 01:38:35 EST 2024 > > > MKFS_OPTIONS -- //xxx-xxx-xx-xxxx.xxxx.xxx.com/SCRATCH_dev > > > MOUNT_OPTIONS -- -o vers=3.11,mfsymlinks,username=root,password=redhat -o context=system_u:object_r:root_t:s0 //xxx-xxx-xx-xxxx.xxxx.xxx.com/SCRATCH_dev /mnt/xfstests/scratch/cifs-client > > > > > > generic/740 > > > ... > > > ... > > > > > > # ps axu|grep fcntl > > > root 1138909 0.0 0.0 5056 0 ? S Feb07 0:00 /var/lib/xfstests/src/fcntl_lock_corner_tests /mnt/xfstests/test/cifs-client/testfile > > > root 1138910 0.0 0.0 21760 0 ? Sl Feb07 0:00 /var/lib/xfstests/src/fcntl_lock_corner_tests /mnt/xfstests/test/cifs-client/testfile > > > # cat /proc/1138909/stack > > > [<0>] 0xc00000008d068a00 > > > [<0>] __switch_to+0x13c/0x228 > > > [<0>] do_wait+0x15c/0x224 > > > [<0>] kernel_wait4+0xb8/0x2c8 > > > [<0>] system_call_exception+0x134/0x390 > > > [<0>] system_call_vectored_common+0x15c/0x2ec > > > # cat /proc/1138910/stack > > > [<0>] 0xc00000008de94400 > > > [<0>] __switch_to+0x13c/0x228 > > > [<0>] futex_wait_queue+0xa8/0x134 > > > [<0>] __futex_wait+0xb4/0x15c > > > [<0>] futex_wait+0x94/0x150 > > > [<0>] do_futex+0xe8/0x374 > > > [<0>] sys_futex+0xa4/0x234 > > > [<0>] system_call_exception+0x134/0x390 > > > [<0>] system_call_vectored_common+0x15c/0x2ec > > > > > > # strace -p 1138909 > > > strace: Process 1138909 attached > > > wait4(-1, > > > (nothing more output) > > > strace: Process 1138909 detached > > > ^C <detached ...> > > > # strace -p 1138910 > > > strace: Process 1138910 attached > > > futex(0x7fff97a8f110, FUTEX_WAIT_BITSET|FUTEX_CLOCK_REALTIME, 1138912, NULL, FUTEX_BITSET_MATCH_ANY > > > (nothing more output) > > > ^Cstrace: Process 1138910 detached > > > <detached ...> > > > > > > > > > > changes since v2: > > > > - move fcntl tests into one fcntl c file > > > > - remove ofd and same owner tests, should be reflected by only one test > > > > - simplify commit message (remove testname out of it) > > > > - add error messages in fcntl.c to give more information if an error > > > > occur > > > > > > > > src/Makefile | 3 +- > > > > src/fcntl.c | 322 ++++++++++++++++++++++++++++++++++++++++++ > > > > tests/generic/732 | 32 +++++ > > > > tests/generic/732.out | 2 + > > > > 4 files changed, 358 insertions(+), 1 deletion(-) > > > > create mode 100644 src/fcntl.c > > > > create mode 100755 tests/generic/732 > > > > create mode 100644 tests/generic/732.out > > > > > > > > diff --git a/src/Makefile b/src/Makefile > > > > index 2815f919..67f936d3 100644 > > > > --- a/src/Makefile > > > > +++ b/src/Makefile > > > > @@ -19,7 +19,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \ > > > > t_ofd_locks t_mmap_collision mmap-write-concurrent \ > > > > t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \ > > > > t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \ > > > > - t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test > > > > + t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test \ > > > > + fcntl > > > > > > > > LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ > > > > preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \ > > > > diff --git a/src/fcntl.c b/src/fcntl.c > > > > new file mode 100644 > > > > index 00000000..8e375357 > > > > --- /dev/null > > > > +++ b/src/fcntl.c > > > > @@ -0,0 +1,322 @@ > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > +/* Copyright (c) 2023 Alexander Aring. All Rights Reserved. > > > > + */ > > > > + > > > > +#include <pthread.h> > > > > +#include <unistd.h> > > > > +#include <string.h> > > > > +#include <limits.h> > > > > +#include <stdlib.h> > > > > +#include <errno.h> > > > > +#include <fcntl.h> > > > > +#include <stdio.h> > > > > +#include <wait.h> > > > > + > > > > +static char filename[PATH_MAX + 1]; > > > > +static int fd; > > > > + > > > > +static void usage(char *name, const char *msg) > > > > +{ > > > > + printf("Fatal: %s\nUsage:\n" > > > > + "%s <file for fcntl>\n", msg, name); > > > > + _exit(1); > > > > +} > > > > + > > > > +static void *do_equal_file_lock_thread(void *arg) > > > > +{ > > > > + struct flock fl = { > > > > + .l_type = F_WRLCK, > > > > + .l_whence = SEEK_SET, > > > > + .l_start = 0L, > > > > + .l_len = 1L, > > > > + }; > > > > + int rv; > > > > + > > > > + rv = fcntl(fd, F_SETLKW, &fl); > > > > + if (rv == -1) { > > > > + perror("fcntl"); > > > > + _exit(1); > > > > + } > > > > + > > > > + return NULL; > > > > +} > > > > + > > > > +static void do_test_equal_file_lock(void) > > > > +{ > > > > + struct flock fl; > > > > + pthread_t t[2]; > > > > + int pid, rv; > > > > + > > > > + fl.l_type = F_WRLCK; > > > > + fl.l_whence = SEEK_SET; > > > > + fl.l_start = 0L; > > > > + fl.l_len = 1L; > > > > + > > > > + /* acquire range 0-0 */ > > > > + rv = fcntl(fd, F_SETLK, &fl); > > > > + if (rv == -1) { > > > > + perror("fcntl"); > > > > + _exit(1); > > > > + } > > > > + > > > > + pid = fork(); > > > > + if (pid == 0) { > > > > + rv = pthread_create(&t[0], NULL, do_equal_file_lock_thread, NULL); > > > > + if (rv != 0) { > > > > + fprintf(stderr, "failed to create pthread\n"); > > > > + _exit(1); > > > > + } > > > > + > > > > + rv = pthread_create(&t[1], NULL, do_equal_file_lock_thread, NULL); > > > > + if (rv != 0) { > > > > + fprintf(stderr, "failed to create pthread\n"); > > > > + _exit(1); > > > > + } > > > > + > > > > + pthread_join(t[0], NULL); > > > > + pthread_join(t[1], NULL); > > > > + > > > > + _exit(0); > > > > + } > > > > + > > > > + /* wait until threads block on 0-0 */ > > > > + sleep(3); > > > > + > > > > + fl.l_type = F_UNLCK; > > > > + fl.l_start = 0; > > > > + fl.l_len = 1; > > > > + rv = fcntl(fd, F_SETLK, &fl); > > > > + if (rv == -1) { > > > > + perror("fcntl"); > > > > + _exit(1); > > > > + } > > > > + > > > > + sleep(3); > > > > + > > > > + /* check if the ->lock() implementation got the > > > > + * right locks granted because two waiter with the > > > > + * same file_lock fields are waiting > > > > + */ > > > > + fl.l_type = F_WRLCK; > > > > + rv = fcntl(fd, F_SETLK, &fl); > > > > + if (rv == -1 && errno == EAGAIN) { > > > > + fprintf(stderr, "deadlock, pthread not cleaned up correctly\n"); > > > > + _exit(1); > > > > + } > > > > + > > > > + wait(NULL); > > > > +} > > > > + > > > > +static void catch_alarm(int num) { } > > > > + > > > > +static void do_test_signal_interrupt_child(int *pfd) > > > > +{ > > > > + struct sigaction act; > > > > + unsigned char m; > > > > + struct flock fl; > > > > + int rv; > > > > + > > > > + fl.l_type = F_WRLCK; > > > > + fl.l_whence = SEEK_SET; > > > > + fl.l_start = 1; > > > > + fl.l_len = 1; > > > > + > > > > + rv = fcntl(fd, F_SETLK, &fl); > > > > + if (rv == -1) { > > > > + perror("fcntl"); > > > > + _exit(1); > > > > + } > > > > + > > > > + memset(&act, 0, sizeof(act)); > > > > + act.sa_handler = catch_alarm; > > > > + sigemptyset(&act.sa_mask); > > > > + sigaddset(&act.sa_mask, SIGALRM); > > > > + sigaction(SIGALRM, &act, NULL); > > > > + > > > > + fl.l_type = F_WRLCK; > > > > + fl.l_whence = SEEK_SET; > > > > + fl.l_start = 0; > > > > + fl.l_len = 1; > > > > + > > > > + /* interrupt SETLKW by signal in 3 secs */ > > > > + alarm(3); > > > > + rv = fcntl(fd, F_SETLKW, &fl); > > > > + if (rv == 0) { > > > > + fprintf(stderr, "fcntl interrupt successful but should fail with EINTR\n"); > > > > + _exit(1); > > > > + } > > > > + > > > > + /* synchronize to move parent to test region 1-1 */ > > > > + write(pfd[1], &m, sizeof(m)); > > > > + > > > > + /* keep child alive */ > > > > + read(pfd[1], &m, sizeof(m)); > > > > +} > > > > + > > > > +static void do_test_signal_interrupt(void) > > > > +{ > > > > + struct flock fl; > > > > + unsigned char m; > > > > + int pid, rv; > > > > + int pfd[2]; > > > > + > > > > + rv = pipe(pfd); > > > > + if (rv == -1) { > > > > + perror("pipe"); > > > > + _exit(1); > > > > + } > > > > + > > > > + fl.l_type = F_WRLCK; > > > > + fl.l_whence = SEEK_SET; > > > > + fl.l_start = 0; > > > > + fl.l_len = 1; > > > > + > > > > + rv = fcntl(fd, F_SETLK, &fl); > > > > + if (rv == -1) { > > > > + perror("fcntl"); > > > > + _exit(1); > > > > + } > > > > + > > > > + pid = fork(); > > > > + if (pid == 0) { > > > > + do_test_signal_interrupt_child(pfd); > > > > + _exit(0); > > > > + } > > > > + > > > > + /* wait until child writes */ > > > > + read(pfd[0], &m, sizeof(m)); > > > > + > > > > + fl.l_type = F_WRLCK; > > > > + fl.l_whence = SEEK_SET; > > > > + fl.l_start = 1; > > > > + fl.l_len = 1; > > > > + /* parent testing childs region, the child will think > > > > + * it has region 1-1 locked because it was interrupted > > > > + * by region 0-0. Due bugs the interruption also unlocked > > > > + * region 1-1. > > > > + */ > > > > + rv = fcntl(fd, F_SETLK, &fl); > > > > + if (rv == 0) { > > > > + fprintf(stderr, "fcntl trylock successful but should fail because child still acquires region\n"); > > > > + _exit(1); > > > > + } > > > > + > > > > + write(pfd[0], &m, sizeof(m)); > > > > + > > > > + wait(NULL); > > > > + > > > > + close(pfd[0]); > > > > + close(pfd[1]); > > > > + > > > > + /* cleanup everything */ > > > > + fl.l_type = F_UNLCK; > > > > + fl.l_whence = SEEK_SET; > > > > + fl.l_start = 0; > > > > + fl.l_len = 2; > > > > + rv = fcntl(fd, F_SETLK, &fl); > > > > + if (rv == -1) { > > > > + perror("fcntl"); > > > > + _exit(1); > > > > + } > > > > +} > > > > + > > > > +static void do_test_kill_child(void) > > > > +{ > > > > + struct flock fl; > > > > + int rv; > > > > + > > > > + fl.l_type = F_WRLCK; > > > > + fl.l_whence = SEEK_SET; > > > > + fl.l_start = 0; > > > > + fl.l_len = 1; > > > > + > > > > + rv = fcntl(fd, F_SETLKW, &fl); > > > > + if (rv == -1) { > > > > + perror("fcntl"); > > > > + _exit(1); > > > > + } > > > > +} > > > > + > > > > +static void do_test_kill(void) > > > > +{ > > > > + struct flock fl; > > > > + int pid_to_kill; > > > > + int rv; > > > > + > > > > + fl.l_type = F_WRLCK; > > > > + fl.l_whence = SEEK_SET; > > > > + fl.l_start = 0; > > > > + fl.l_len = 1; > > > > + > > > > + rv = fcntl(fd, F_SETLK, &fl); > > > > + if (rv == -1) { > > > > + perror("fcntl"); > > > > + _exit(1); > > > > + } > > > > + > > > > + pid_to_kill = fork(); > > > > + if (pid_to_kill == 0) { > > > > + do_test_kill_child(); > > > > + _exit(0); > > > > + } > > > > + > > > > + /* wait until child blocks */ > > > > + sleep(3); > > > > + > > > > + kill(pid_to_kill, SIGKILL); > > > > + > > > > + /* wait until Linux did plock cleanup */ > > > > + sleep(3); > > > > + > > > > + fl.l_type = F_UNLCK; > > > > + fl.l_whence = SEEK_SET; > > > > + fl.l_start = 0; > > > > + fl.l_len = 1; > > > > + > > > > + /* cleanup parent lock */ > > > > + rv = fcntl(fd, F_SETLK, &fl); > > > > + if (rv == -1) { > > > > + perror("fcntl"); > > > > + _exit(1); > > > > + } > > > > + > > > > + fl.l_type = F_WRLCK; > > > > + fl.l_whence = SEEK_SET; > > > > + fl.l_start = 0; > > > > + fl.l_len = 1; > > > > + > > > > + /* check if the child still holds the lock > > > > + * and killing the child was not cleaning > > > > + * up locks. > > > > + */ > > > > + rv = fcntl(fd, F_SETLK, &fl); > > > > + if ((rv == -1 && errno == EAGAIN)) { > > > > + fprintf(stderr, "fcntl trylock failed but should be successful because killing child should cleanup acquired lock\n"); > > > > + _exit(1); > > > > + } > > > > +} > > > > + > > > > +int main(int argc, char * const argv[]) > > > > +{ > > > > + if (optind != argc - 1) > > > > + usage(argv[0], "<file for fcntl> is mandatory to tell the file where to run fcntl() on it"); > > > > + > > > > + strncpy(filename, argv[1], PATH_MAX); > > > > + > > > > + fd = open(filename, O_RDWR | O_CREAT, 0700); > > > > + if (fd == -1) { > > > > + perror("open"); > > > > + _exit(1); > > > > + } > > > > + > > > > + /* test to have to equal struct file_lock requests in ->lock() */ > > > > + do_test_equal_file_lock(); > > > > + /* test to interrupt F_SETLKW by a signal and cleanup only canceled the pending interrupted request */ > > > > + do_test_signal_interrupt(); > > > > + /* test if cleanup is correct if a child gets killed while being blocked in F_SETLKW */ > > > > + do_test_kill(); > > > > + > > > > + close(fd); > > > > + > > > > + return 0; > > > > +} > > > > diff --git a/tests/generic/732 b/tests/generic/732 > > > > new file mode 100755 > > > > index 00000000..d77f9fc2 > > > > --- /dev/null > > > > +++ b/tests/generic/732 > > > > @@ -0,0 +1,32 @@ > > > > +#! /bin/bash > > > > +# SPDX-License-Identifier: GPL-2.0 > > > > +# Copyright (c) 2023 Alexander Aring. All Rights Reserved. > > > > +# > > > > +# FS QA Test 732 > > > > +# > > > > +# This tests performs some fcntl() corner cases. See fcntl test > > > > +# program for more details. > > > > +# > > > > +. ./common/preamble > > > > +_begin_fstest auto > > > > + > > > > +# Import common functions. > > > > +. ./common/filter > > > > + > > > > +# real QA test starts here > > > > + > > > > +# Modify as appropriate. > > > > +_supported_fs generic > > > > +_require_test > > > > +_require_test_program fcntl > > > > + > > > > +echo "Silence is golden" > > > > + > > > > +$here/src/fcntl $TEST_DIR/testfile > > > > +if [ $? -ne 0 ] > > > > +then > > > > + exit > > > > +fi > > > > + > > > > +status=0 > > > > +exit > > > > diff --git a/tests/generic/732.out b/tests/generic/732.out > > > > new file mode 100644 > > > > index 00000000..451f82ce > > > > --- /dev/null > > > > +++ b/tests/generic/732.out > > > > @@ -0,0 +1,2 @@ > > > > +QA output created by 732 > > > > +Silence is golden > > > > -- > > > > 2.31.1 > > > > > > > > > > > > > > > > -- > > Thanks, > > > > Steve > >
Hi, On Fri, Mar 1, 2024 at 5:38 AM Zorro Lang <zlang@kernel.org> wrote: > > On Fri, Feb 09, 2024 at 07:43:10PM +0800, Zorro Lang wrote: > > On Thu, Feb 08, 2024 at 11:35:06PM -0600, Steve French wrote: > > > Could you forward the changeset for this so we could try it? > > > > Hi Steve, > > > > Thanks for your reply. Sure, there's a temporary branch "patches-in-queue", > > you can get it by: > > # git clone -b patches-in-queue git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git > > > > Then you can see a generic/740 test case in it, run it on cifs. I tested > > with cifs 3.11, and wrote the /etc/samba/smb.conf as: > > [TEST_dev] > > path = $dir > > writable = yes > > ea support = yes > > strict allocate = yes > > And set: > > MOUNT_OPTIONS="-o vers=3.11,mfsymlinks,username=root,password=xxxxxxxxxx" > > TEST_FS_MOUNT_OPTS="-o vers=3.11,mfsymlinks,username=root,password=xxxxxxxx" > > > > I'm not sure if it's a cifs issue, Alexander writes this case for gfs2, it > > works for other fs, but blocked (until kill it manually) on cifs. So hope > > to get some suggestions from cifs list. > > Hi Steve, > > Any feedback about this? Just to make sure if it's a cifs bug or a case issue. > I talked yesterday with Paulo Alcantara (cifs developer), he is currently looking into it. Paulo, is there any feedback yet? Thanks. Thanks for the ping Zorro. - Alex
Hi Zorro,
The problem is that cifs.ko is returning -EACCES from fcntl(2) called
in do_test_equal_file_lock() but it is expecting -EAGAIN to be
returned, so it hangs in wait4(2):
...
[pid 14846] fcntl(3, F_SETLK, {l_type=F_WRLCK, l_whence=SEEK_SET, l_start=0, l_len=1}) = -1 EACCES (Permission denied)
[pid 14846] wait4(-1,
The man page says:
F_SETLK (struct flock *)
Acquire a lock (when l_type is F_RDLCK or F_WRLCK) or release a
lock (when l_type is F_UNLCK) on the bytes specified by the
l_whence, l_start, and l_len fields of lock. If a conflicting
lock is held by another process, this call returns -1 and sets
errno to EACCES or EAGAIN. (The error returned in this case
differs across implementations, so POSIX requires a portable ap‐
plication to check for both errors.)
so fcntl_lock_corner_tests should also handle -EACCES.
Hi, On Fri, Mar 1, 2024 at 11:25 AM Paulo Alcantara <pc@manguebit.com> wrote: > > Hi Zorro, > > The problem is that cifs.ko is returning -EACCES from fcntl(2) called > in do_test_equal_file_lock() but it is expecting -EAGAIN to be > returned, so it hangs in wait4(2): > > ... > [pid 14846] fcntl(3, F_SETLK, {l_type=F_WRLCK, l_whence=SEEK_SET, l_start=0, l_len=1}) = -1 EACCES (Permission denied) > [pid 14846] wait4(-1, > > The man page says: > > F_SETLK (struct flock *) > Acquire a lock (when l_type is F_RDLCK or F_WRLCK) or release a > lock (when l_type is F_UNLCK) on the bytes specified by the > l_whence, l_start, and l_len fields of lock. If a conflicting > lock is held by another process, this call returns -1 and sets > errno to EACCES or EAGAIN. (The error returned in this case > differs across implementations, so POSIX requires a portable ap‐ > plication to check for both errors.) > > so fcntl_lock_corner_tests should also handle -EACCES. > yes, that is a bug in the test but in my opinion there is still an issue. The mentioned fcntl(F_SETLK) above is just a sanity check to print out if something is not correct and it will print out that something is not correct and fails. The problem is that wait() below, the child processes are not returning and are in a blocking state which should not be the case. What the test is doing is the following: parent: 1. lock(A) # should be successful to acquire child: thread0: 2. lock(A) # should block thread1: 3. lock(A) # should block parent: 5. sleep(3) #wait until child are in blocking state of lock(A) 5. unlock(A) # both threads of the child should unlock and exit ... 6. sleep 3 # wait for pending unlock op (not really sure if it's necessary) ... 7. trylock(A) # mentioned sanity check 8. wait(NULL) # wait for child to exit The unlock(A) should unblock the child threads, it is important to mention that this test does a lock corner test and the lock(A) in both threads ends in a ->lock() call with a "struct file_lock" that has mostly the same fields. We had issues with that in gfs2 and a lookup function to find the right request with an async complete handler of the lock operation. - Alex
Alexander Aring <aahringo@redhat.com> writes: > Hi, > > On Fri, Mar 1, 2024 at 11:25 AM Paulo Alcantara <pc@manguebit.com> wrote: >> >> Hi Zorro, >> >> The problem is that cifs.ko is returning -EACCES from fcntl(2) called >> in do_test_equal_file_lock() but it is expecting -EAGAIN to be >> returned, so it hangs in wait4(2): >> >> ... >> [pid 14846] fcntl(3, F_SETLK, {l_type=F_WRLCK, l_whence=SEEK_SET, l_start=0, l_len=1}) = -1 EACCES (Permission denied) >> [pid 14846] wait4(-1, >> >> The man page says: >> >> F_SETLK (struct flock *) >> Acquire a lock (when l_type is F_RDLCK or F_WRLCK) or release a >> lock (when l_type is F_UNLCK) on the bytes specified by the >> l_whence, l_start, and l_len fields of lock. If a conflicting >> lock is held by another process, this call returns -1 and sets >> errno to EACCES or EAGAIN. (The error returned in this case >> differs across implementations, so POSIX requires a portable ap‐ >> plication to check for both errors.) >> >> so fcntl_lock_corner_tests should also handle -EACCES. >> > > yes, that is a bug in the test but in my opinion there is still an > issue. The mentioned fcntl(F_SETLK) above is just a sanity check to > print out if something is not correct and it will print out that > something is not correct and fails. Yes, I agree it might be a cifs.ko issue. However, it's still important making sure that the test exits gracefully and then report an error rather than hanging. > The problem is that wait() below, the child processes are not > returning and are in a blocking state which should not be the case. > > What the test is doing is the following: > > parent: > > 1. lock(A) # should be successful to acquire Client successfully acquires it. > child: > thread0: > 2. lock(A) # should block > thread1: > 3. lock(A) # should block OK - both threads are blocked. > parent: > > 5. sleep(3) #wait until child are in blocking state of lock(A) OK. > 5. unlock(A) # both threads of the child should unlock and exit At this point, both threads are woken up and one of them acquires the lock and returns. The other thread gets blocked again because it finds a conflicting lock that was taken from the other thread. The child then never exits because it is waiting in pthread_join(). > 6. sleep 3 # wait for pending unlock op (not really sure if it's necessary) > ... > 7. trylock(A) # mentioned sanity check Client returns -EACCES because one of the child threads acquired the lock. > The unlock(A) should unblock the child threads, it is important to > mention that this test does a lock corner test and the lock(A) in both > threads ends in a ->lock() call with a "struct file_lock" that has > mostly the same fields. We had issues with that in gfs2 and a lookup > function to find the right request with an async complete handler of > the lock operation. Alex, thanks for the explanation! As we've talked, there might be a missing check of fl_owner or some sort of protocol limitation while checking for lock conflicts. Steve, any thoughts on this?
On Fri, Mar 01, 2024 at 08:59:01PM -0300, Paulo Alcantara wrote: > Alexander Aring <aahringo@redhat.com> writes: > > > Hi, > > > > On Fri, Mar 1, 2024 at 11:25 AM Paulo Alcantara <pc@manguebit.com> wrote: > >> > >> Hi Zorro, > >> > >> The problem is that cifs.ko is returning -EACCES from fcntl(2) called > >> in do_test_equal_file_lock() but it is expecting -EAGAIN to be > >> returned, so it hangs in wait4(2): > >> > >> ... > >> [pid 14846] fcntl(3, F_SETLK, {l_type=F_WRLCK, l_whence=SEEK_SET, l_start=0, l_len=1}) = -1 EACCES (Permission denied) > >> [pid 14846] wait4(-1, > >> > >> The man page says: > >> > >> F_SETLK (struct flock *) > >> Acquire a lock (when l_type is F_RDLCK or F_WRLCK) or release a > >> lock (when l_type is F_UNLCK) on the bytes specified by the > >> l_whence, l_start, and l_len fields of lock. If a conflicting > >> lock is held by another process, this call returns -1 and sets > >> errno to EACCES or EAGAIN. (The error returned in this case > >> differs across implementations, so POSIX requires a portable ap‐ > >> plication to check for both errors.) > >> > >> so fcntl_lock_corner_tests should also handle -EACCES. > >> > > > > yes, that is a bug in the test but in my opinion there is still an > > issue. The mentioned fcntl(F_SETLK) above is just a sanity check to > > print out if something is not correct and it will print out that > > something is not correct and fails. > > Yes, I agree it might be a cifs.ko issue. However, it's still important > making sure that the test exits gracefully and then report an error > rather than hanging. Thanks for all of you look into it! If the C program can deal with issue (report error rather than hang), that would be good. Or how about give the fcntl testing process a (long enough) timeout number, to avoid it block the whole fstests test running, and report error if it exits unnormally. Thanks, Zorro > > > The problem is that wait() below, the child processes are not > > returning and are in a blocking state which should not be the case. > > > > What the test is doing is the following: > > > > parent: > > > > 1. lock(A) # should be successful to acquire > > Client successfully acquires it. > > > child: > > thread0: > > 2. lock(A) # should block > > thread1: > > 3. lock(A) # should block > > OK - both threads are blocked. > > > parent: > > > > 5. sleep(3) #wait until child are in blocking state of lock(A) > > OK. > > > 5. unlock(A) # both threads of the child should unlock and exit > > At this point, both threads are woken up and one of them acquires the > lock and returns. The other thread gets blocked again because it finds > a conflicting lock that was taken from the other thread. The child then > never exits because it is waiting in pthread_join(). > > > 6. sleep 3 # wait for pending unlock op (not really sure if it's necessary) > > ... > > 7. trylock(A) # mentioned sanity check > > Client returns -EACCES because one of the child threads acquired the > lock. > > > The unlock(A) should unblock the child threads, it is important to > > mention that this test does a lock corner test and the lock(A) in both > > threads ends in a ->lock() call with a "struct file_lock" that has > > mostly the same fields. We had issues with that in gfs2 and a lookup > > function to find the right request with an async complete handler of > > the lock operation. > > Alex, thanks for the explanation! As we've talked, there might be a > missing check of fl_owner or some sort of protocol limitation while > checking for lock conflicts. > > Steve, any thoughts on this? >
On 9/26/23 01:48, Alexander Aring wrote: > This patch adds fcntl corner cases that was being used to confirm issues > on a GFS2 filesystem. The GFS2 filesystem has it's own ->lock() > implementation and in those corner cases issues was being found and > fixed. > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > --- > changes since v2: > - move fcntl tests into one fcntl c file > - remove ofd and same owner tests, should be reflected by only one test > - simplify commit message (remove testname out of it) > - add error messages in fcntl.c to give more information if an error > occur > > src/Makefile | 3 +- > src/fcntl.c | 322 ++++++++++++++++++++++++++++++++++++++++++ > tests/generic/732 | 32 +++++ > tests/generic/732.out | 2 + > 4 files changed, 358 insertions(+), 1 deletion(-) > create mode 100644 src/fcntl.c > create mode 100755 tests/generic/732 > create mode 100644 tests/generic/732.out > > diff --git a/src/Makefile b/src/Makefile > index 2815f919..67f936d3 100644 > --- a/src/Makefile > +++ b/src/Makefile > @@ -19,7 +19,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \ > t_ofd_locks t_mmap_collision mmap-write-concurrent \ > t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \ > t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \ > - t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test > + t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test \ > + fcntl > Please add the binary file to the .gitignore. src/fcntl_lock_corner_tests Thanks, Anand > LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ > preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \ > diff --git a/src/fcntl.c b/src/fcntl.c > new file mode 100644 > index 00000000..8e375357 > --- /dev/null > +++ b/src/fcntl.c > @@ -0,0 +1,322 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* Copyright (c) 2023 Alexander Aring. All Rights Reserved. > + */ > + > +#include <pthread.h> > +#include <unistd.h> > +#include <string.h> > +#include <limits.h> > +#include <stdlib.h> > +#include <errno.h> > +#include <fcntl.h> > +#include <stdio.h> > +#include <wait.h> > + > +static char filename[PATH_MAX + 1]; > +static int fd; > + > +static void usage(char *name, const char *msg) > +{ > + printf("Fatal: %s\nUsage:\n" > + "%s <file for fcntl>\n", msg, name); > + _exit(1); > +} > + > +static void *do_equal_file_lock_thread(void *arg) > +{ > + struct flock fl = { > + .l_type = F_WRLCK, > + .l_whence = SEEK_SET, > + .l_start = 0L, > + .l_len = 1L, > + }; > + int rv; > + > + rv = fcntl(fd, F_SETLKW, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + return NULL; > +} > + > +static void do_test_equal_file_lock(void) > +{ > + struct flock fl; > + pthread_t t[2]; > + int pid, rv; > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0L; > + fl.l_len = 1L; > + > + /* acquire range 0-0 */ > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + pid = fork(); > + if (pid == 0) { > + rv = pthread_create(&t[0], NULL, do_equal_file_lock_thread, NULL); > + if (rv != 0) { > + fprintf(stderr, "failed to create pthread\n"); > + _exit(1); > + } > + > + rv = pthread_create(&t[1], NULL, do_equal_file_lock_thread, NULL); > + if (rv != 0) { > + fprintf(stderr, "failed to create pthread\n"); > + _exit(1); > + } > + > + pthread_join(t[0], NULL); > + pthread_join(t[1], NULL); > + > + _exit(0); > + } > + > + /* wait until threads block on 0-0 */ > + sleep(3); > + > + fl.l_type = F_UNLCK; > + fl.l_start = 0; > + fl.l_len = 1; > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + sleep(3); > + > + /* check if the ->lock() implementation got the > + * right locks granted because two waiter with the > + * same file_lock fields are waiting > + */ > + fl.l_type = F_WRLCK; > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1 && errno == EAGAIN) { > + fprintf(stderr, "deadlock, pthread not cleaned up correctly\n"); > + _exit(1); > + } > + > + wait(NULL); > +} > + > +static void catch_alarm(int num) { } > + > +static void do_test_signal_interrupt_child(int *pfd) > +{ > + struct sigaction act; > + unsigned char m; > + struct flock fl; > + int rv; > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 1; > + fl.l_len = 1; > + > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + memset(&act, 0, sizeof(act)); > + act.sa_handler = catch_alarm; > + sigemptyset(&act.sa_mask); > + sigaddset(&act.sa_mask, SIGALRM); > + sigaction(SIGALRM, &act, NULL); > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + /* interrupt SETLKW by signal in 3 secs */ > + alarm(3); > + rv = fcntl(fd, F_SETLKW, &fl); > + if (rv == 0) { > + fprintf(stderr, "fcntl interrupt successful but should fail with EINTR\n"); > + _exit(1); > + } > + > + /* synchronize to move parent to test region 1-1 */ > + write(pfd[1], &m, sizeof(m)); > + > + /* keep child alive */ > + read(pfd[1], &m, sizeof(m)); > +} > + > +static void do_test_signal_interrupt(void) > +{ > + struct flock fl; > + unsigned char m; > + int pid, rv; > + int pfd[2]; > + > + rv = pipe(pfd); > + if (rv == -1) { > + perror("pipe"); > + _exit(1); > + } > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + pid = fork(); > + if (pid == 0) { > + do_test_signal_interrupt_child(pfd); > + _exit(0); > + } > + > + /* wait until child writes */ > + read(pfd[0], &m, sizeof(m)); > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 1; > + fl.l_len = 1; > + /* parent testing childs region, the child will think > + * it has region 1-1 locked because it was interrupted > + * by region 0-0. Due bugs the interruption also unlocked > + * region 1-1. > + */ > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == 0) { > + fprintf(stderr, "fcntl trylock successful but should fail because child still acquires region\n"); > + _exit(1); > + } > + > + write(pfd[0], &m, sizeof(m)); > + > + wait(NULL); > + > + close(pfd[0]); > + close(pfd[1]); > + > + /* cleanup everything */ > + fl.l_type = F_UNLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 2; > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > +} > + > +static void do_test_kill_child(void) > +{ > + struct flock fl; > + int rv; > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + rv = fcntl(fd, F_SETLKW, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > +} > + > +static void do_test_kill(void) > +{ > + struct flock fl; > + int pid_to_kill; > + int rv; > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + pid_to_kill = fork(); > + if (pid_to_kill == 0) { > + do_test_kill_child(); > + _exit(0); > + } > + > + /* wait until child blocks */ > + sleep(3); > + > + kill(pid_to_kill, SIGKILL); > + > + /* wait until Linux did plock cleanup */ > + sleep(3); > + > + fl.l_type = F_UNLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + /* cleanup parent lock */ > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + /* check if the child still holds the lock > + * and killing the child was not cleaning > + * up locks. > + */ > + rv = fcntl(fd, F_SETLK, &fl); > + if ((rv == -1 && errno == EAGAIN)) { > + fprintf(stderr, "fcntl trylock failed but should be successful because killing child should cleanup acquired lock\n"); > + _exit(1); > + } > +} > + > +int main(int argc, char * const argv[]) > +{ > + if (optind != argc - 1) > + usage(argv[0], "<file for fcntl> is mandatory to tell the file where to run fcntl() on it"); > + > + strncpy(filename, argv[1], PATH_MAX); > + > + fd = open(filename, O_RDWR | O_CREAT, 0700); > + if (fd == -1) { > + perror("open"); > + _exit(1); > + } > + > + /* test to have to equal struct file_lock requests in ->lock() */ > + do_test_equal_file_lock(); > + /* test to interrupt F_SETLKW by a signal and cleanup only canceled the pending interrupted request */ > + do_test_signal_interrupt(); > + /* test if cleanup is correct if a child gets killed while being blocked in F_SETLKW */ > + do_test_kill(); > + > + close(fd); > + > + return 0; > +} > diff --git a/tests/generic/732 b/tests/generic/732 > new file mode 100755 > index 00000000..d77f9fc2 > --- /dev/null > +++ b/tests/generic/732 > @@ -0,0 +1,32 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2023 Alexander Aring. All Rights Reserved. > +# > +# FS QA Test 732 > +# > +# This tests performs some fcntl() corner cases. See fcntl test > +# program for more details. > +# > +. ./common/preamble > +_begin_fstest auto > + > +# Import common functions. > +. ./common/filter > + > +# real QA test starts here > + > +# Modify as appropriate. > +_supported_fs generic > +_require_test > +_require_test_program fcntl > + > +echo "Silence is golden" > + > +$here/src/fcntl $TEST_DIR/testfile > +if [ $? -ne 0 ] > +then > + exit > +fi > + > +status=0 > +exit > diff --git a/tests/generic/732.out b/tests/generic/732.out > new file mode 100644 > index 00000000..451f82ce > --- /dev/null > +++ b/tests/generic/732.out > @@ -0,0 +1,2 @@ > +QA output created by 732 > +Silence is golden
On Mon, Sep 25, 2023 at 04:18:27PM -0400, Alexander Aring wrote: > This patch adds fcntl corner cases that was being used to confirm issues > on a GFS2 filesystem. The GFS2 filesystem has it's own ->lock() > implementation and in those corner cases issues was being found and > fixed. > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > --- > changes since v2: > - move fcntl tests into one fcntl c file > - remove ofd and same owner tests, should be reflected by only one test > - simplify commit message (remove testname out of it) > - add error messages in fcntl.c to give more information if an error > occur Any update? :) It's still in "patches-in-queue" branch. Thanks, Zorro > > src/Makefile | 3 +- > src/fcntl.c | 322 ++++++++++++++++++++++++++++++++++++++++++ > tests/generic/732 | 32 +++++ > tests/generic/732.out | 2 + > 4 files changed, 358 insertions(+), 1 deletion(-) > create mode 100644 src/fcntl.c > create mode 100755 tests/generic/732 > create mode 100644 tests/generic/732.out > > diff --git a/src/Makefile b/src/Makefile > index 2815f919..67f936d3 100644 > --- a/src/Makefile > +++ b/src/Makefile > @@ -19,7 +19,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \ > t_ofd_locks t_mmap_collision mmap-write-concurrent \ > t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \ > t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \ > - t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test > + t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test \ > + fcntl > > LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ > preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \ > diff --git a/src/fcntl.c b/src/fcntl.c > new file mode 100644 > index 00000000..8e375357 > --- /dev/null > +++ b/src/fcntl.c > @@ -0,0 +1,322 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* Copyright (c) 2023 Alexander Aring. All Rights Reserved. > + */ > + > +#include <pthread.h> > +#include <unistd.h> > +#include <string.h> > +#include <limits.h> > +#include <stdlib.h> > +#include <errno.h> > +#include <fcntl.h> > +#include <stdio.h> > +#include <wait.h> > + > +static char filename[PATH_MAX + 1]; > +static int fd; > + > +static void usage(char *name, const char *msg) > +{ > + printf("Fatal: %s\nUsage:\n" > + "%s <file for fcntl>\n", msg, name); > + _exit(1); > +} > + > +static void *do_equal_file_lock_thread(void *arg) > +{ > + struct flock fl = { > + .l_type = F_WRLCK, > + .l_whence = SEEK_SET, > + .l_start = 0L, > + .l_len = 1L, > + }; > + int rv; > + > + rv = fcntl(fd, F_SETLKW, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + return NULL; > +} > + > +static void do_test_equal_file_lock(void) > +{ > + struct flock fl; > + pthread_t t[2]; > + int pid, rv; > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0L; > + fl.l_len = 1L; > + > + /* acquire range 0-0 */ > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + pid = fork(); > + if (pid == 0) { > + rv = pthread_create(&t[0], NULL, do_equal_file_lock_thread, NULL); > + if (rv != 0) { > + fprintf(stderr, "failed to create pthread\n"); > + _exit(1); > + } > + > + rv = pthread_create(&t[1], NULL, do_equal_file_lock_thread, NULL); > + if (rv != 0) { > + fprintf(stderr, "failed to create pthread\n"); > + _exit(1); > + } > + > + pthread_join(t[0], NULL); > + pthread_join(t[1], NULL); > + > + _exit(0); > + } > + > + /* wait until threads block on 0-0 */ > + sleep(3); > + > + fl.l_type = F_UNLCK; > + fl.l_start = 0; > + fl.l_len = 1; > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + sleep(3); > + > + /* check if the ->lock() implementation got the > + * right locks granted because two waiter with the > + * same file_lock fields are waiting > + */ > + fl.l_type = F_WRLCK; > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1 && errno == EAGAIN) { > + fprintf(stderr, "deadlock, pthread not cleaned up correctly\n"); > + _exit(1); > + } > + > + wait(NULL); > +} > + > +static void catch_alarm(int num) { } > + > +static void do_test_signal_interrupt_child(int *pfd) > +{ > + struct sigaction act; > + unsigned char m; > + struct flock fl; > + int rv; > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 1; > + fl.l_len = 1; > + > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + memset(&act, 0, sizeof(act)); > + act.sa_handler = catch_alarm; > + sigemptyset(&act.sa_mask); > + sigaddset(&act.sa_mask, SIGALRM); > + sigaction(SIGALRM, &act, NULL); > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + /* interrupt SETLKW by signal in 3 secs */ > + alarm(3); > + rv = fcntl(fd, F_SETLKW, &fl); > + if (rv == 0) { > + fprintf(stderr, "fcntl interrupt successful but should fail with EINTR\n"); > + _exit(1); > + } > + > + /* synchronize to move parent to test region 1-1 */ > + write(pfd[1], &m, sizeof(m)); > + > + /* keep child alive */ > + read(pfd[1], &m, sizeof(m)); > +} > + > +static void do_test_signal_interrupt(void) > +{ > + struct flock fl; > + unsigned char m; > + int pid, rv; > + int pfd[2]; > + > + rv = pipe(pfd); > + if (rv == -1) { > + perror("pipe"); > + _exit(1); > + } > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + pid = fork(); > + if (pid == 0) { > + do_test_signal_interrupt_child(pfd); > + _exit(0); > + } > + > + /* wait until child writes */ > + read(pfd[0], &m, sizeof(m)); > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 1; > + fl.l_len = 1; > + /* parent testing childs region, the child will think > + * it has region 1-1 locked because it was interrupted > + * by region 0-0. Due bugs the interruption also unlocked > + * region 1-1. > + */ > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == 0) { > + fprintf(stderr, "fcntl trylock successful but should fail because child still acquires region\n"); > + _exit(1); > + } > + > + write(pfd[0], &m, sizeof(m)); > + > + wait(NULL); > + > + close(pfd[0]); > + close(pfd[1]); > + > + /* cleanup everything */ > + fl.l_type = F_UNLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 2; > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > +} > + > +static void do_test_kill_child(void) > +{ > + struct flock fl; > + int rv; > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + rv = fcntl(fd, F_SETLKW, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > +} > + > +static void do_test_kill(void) > +{ > + struct flock fl; > + int pid_to_kill; > + int rv; > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + pid_to_kill = fork(); > + if (pid_to_kill == 0) { > + do_test_kill_child(); > + _exit(0); > + } > + > + /* wait until child blocks */ > + sleep(3); > + > + kill(pid_to_kill, SIGKILL); > + > + /* wait until Linux did plock cleanup */ > + sleep(3); > + > + fl.l_type = F_UNLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + /* cleanup parent lock */ > + rv = fcntl(fd, F_SETLK, &fl); > + if (rv == -1) { > + perror("fcntl"); > + _exit(1); > + } > + > + fl.l_type = F_WRLCK; > + fl.l_whence = SEEK_SET; > + fl.l_start = 0; > + fl.l_len = 1; > + > + /* check if the child still holds the lock > + * and killing the child was not cleaning > + * up locks. > + */ > + rv = fcntl(fd, F_SETLK, &fl); > + if ((rv == -1 && errno == EAGAIN)) { > + fprintf(stderr, "fcntl trylock failed but should be successful because killing child should cleanup acquired lock\n"); > + _exit(1); > + } > +} > + > +int main(int argc, char * const argv[]) > +{ > + if (optind != argc - 1) > + usage(argv[0], "<file for fcntl> is mandatory to tell the file where to run fcntl() on it"); > + > + strncpy(filename, argv[1], PATH_MAX); > + > + fd = open(filename, O_RDWR | O_CREAT, 0700); > + if (fd == -1) { > + perror("open"); > + _exit(1); > + } > + > + /* test to have to equal struct file_lock requests in ->lock() */ > + do_test_equal_file_lock(); > + /* test to interrupt F_SETLKW by a signal and cleanup only canceled the pending interrupted request */ > + do_test_signal_interrupt(); > + /* test if cleanup is correct if a child gets killed while being blocked in F_SETLKW */ > + do_test_kill(); > + > + close(fd); > + > + return 0; > +} > diff --git a/tests/generic/732 b/tests/generic/732 > new file mode 100755 > index 00000000..d77f9fc2 > --- /dev/null > +++ b/tests/generic/732 > @@ -0,0 +1,32 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2023 Alexander Aring. All Rights Reserved. > +# > +# FS QA Test 732 > +# > +# This tests performs some fcntl() corner cases. See fcntl test > +# program for more details. > +# > +. ./common/preamble > +_begin_fstest auto > + > +# Import common functions. > +. ./common/filter > + > +# real QA test starts here > + > +# Modify as appropriate. > +_supported_fs generic > +_require_test > +_require_test_program fcntl > + > +echo "Silence is golden" > + > +$here/src/fcntl $TEST_DIR/testfile > +if [ $? -ne 0 ] > +then > + exit > +fi > + > +status=0 > +exit > diff --git a/tests/generic/732.out b/tests/generic/732.out > new file mode 100644 > index 00000000..451f82ce > --- /dev/null > +++ b/tests/generic/732.out > @@ -0,0 +1,2 @@ > +QA output created by 732 > +Silence is golden > -- > 2.31.1 >
Hi, On Sat, Mar 30, 2024 at 3:25 AM Zorro Lang <zlang@redhat.com> wrote: > > On Mon, Sep 25, 2023 at 04:18:27PM -0400, Alexander Aring wrote: > > This patch adds fcntl corner cases that was being used to confirm issues > > on a GFS2 filesystem. The GFS2 filesystem has it's own ->lock() > > implementation and in those corner cases issues was being found and > > fixed. > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > --- > > changes since v2: > > - move fcntl tests into one fcntl c file > > - remove ofd and same owner tests, should be reflected by only one test > > - simplify commit message (remove testname out of it) > > - add error messages in fcntl.c to give more information if an error > > occur > > Any update? :) It's still in "patches-in-queue" branch. I still have it on my list to work on this. Thanks for all the recommendations for another version. I hope I can provide an update soon. What is about the CIFS issue, we will ignore it for now? From my understanding it is an issue in CIFS but probably rare to hit and not easy to fix for now. (report after hang) I can look if we can add an alarm() type interruption, but this only work if the filesystem really implements that fcntl(..., F_SETLKW, ...) is interruptible which should be the case, otherwise it would be another exception why the test is know to be failing. Thanks. - Alex
diff --git a/src/Makefile b/src/Makefile index 2815f919..67f936d3 100644 --- a/src/Makefile +++ b/src/Makefile @@ -19,7 +19,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \ t_ofd_locks t_mmap_collision mmap-write-concurrent \ t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \ t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \ - t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test + t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test \ + fcntl LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \ diff --git a/src/fcntl.c b/src/fcntl.c new file mode 100644 index 00000000..8e375357 --- /dev/null +++ b/src/fcntl.c @@ -0,0 +1,322 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* Copyright (c) 2023 Alexander Aring. All Rights Reserved. + */ + +#include <pthread.h> +#include <unistd.h> +#include <string.h> +#include <limits.h> +#include <stdlib.h> +#include <errno.h> +#include <fcntl.h> +#include <stdio.h> +#include <wait.h> + +static char filename[PATH_MAX + 1]; +static int fd; + +static void usage(char *name, const char *msg) +{ + printf("Fatal: %s\nUsage:\n" + "%s <file for fcntl>\n", msg, name); + _exit(1); +} + +static void *do_equal_file_lock_thread(void *arg) +{ + struct flock fl = { + .l_type = F_WRLCK, + .l_whence = SEEK_SET, + .l_start = 0L, + .l_len = 1L, + }; + int rv; + + rv = fcntl(fd, F_SETLKW, &fl); + if (rv == -1) { + perror("fcntl"); + _exit(1); + } + + return NULL; +} + +static void do_test_equal_file_lock(void) +{ + struct flock fl; + pthread_t t[2]; + int pid, rv; + + fl.l_type = F_WRLCK; + fl.l_whence = SEEK_SET; + fl.l_start = 0L; + fl.l_len = 1L; + + /* acquire range 0-0 */ + rv = fcntl(fd, F_SETLK, &fl); + if (rv == -1) { + perror("fcntl"); + _exit(1); + } + + pid = fork(); + if (pid == 0) { + rv = pthread_create(&t[0], NULL, do_equal_file_lock_thread, NULL); + if (rv != 0) { + fprintf(stderr, "failed to create pthread\n"); + _exit(1); + } + + rv = pthread_create(&t[1], NULL, do_equal_file_lock_thread, NULL); + if (rv != 0) { + fprintf(stderr, "failed to create pthread\n"); + _exit(1); + } + + pthread_join(t[0], NULL); + pthread_join(t[1], NULL); + + _exit(0); + } + + /* wait until threads block on 0-0 */ + sleep(3); + + fl.l_type = F_UNLCK; + fl.l_start = 0; + fl.l_len = 1; + rv = fcntl(fd, F_SETLK, &fl); + if (rv == -1) { + perror("fcntl"); + _exit(1); + } + + sleep(3); + + /* check if the ->lock() implementation got the + * right locks granted because two waiter with the + * same file_lock fields are waiting + */ + fl.l_type = F_WRLCK; + rv = fcntl(fd, F_SETLK, &fl); + if (rv == -1 && errno == EAGAIN) { + fprintf(stderr, "deadlock, pthread not cleaned up correctly\n"); + _exit(1); + } + + wait(NULL); +} + +static void catch_alarm(int num) { } + +static void do_test_signal_interrupt_child(int *pfd) +{ + struct sigaction act; + unsigned char m; + struct flock fl; + int rv; + + fl.l_type = F_WRLCK; + fl.l_whence = SEEK_SET; + fl.l_start = 1; + fl.l_len = 1; + + rv = fcntl(fd, F_SETLK, &fl); + if (rv == -1) { + perror("fcntl"); + _exit(1); + } + + memset(&act, 0, sizeof(act)); + act.sa_handler = catch_alarm; + sigemptyset(&act.sa_mask); + sigaddset(&act.sa_mask, SIGALRM); + sigaction(SIGALRM, &act, NULL); + + fl.l_type = F_WRLCK; + fl.l_whence = SEEK_SET; + fl.l_start = 0; + fl.l_len = 1; + + /* interrupt SETLKW by signal in 3 secs */ + alarm(3); + rv = fcntl(fd, F_SETLKW, &fl); + if (rv == 0) { + fprintf(stderr, "fcntl interrupt successful but should fail with EINTR\n"); + _exit(1); + } + + /* synchronize to move parent to test region 1-1 */ + write(pfd[1], &m, sizeof(m)); + + /* keep child alive */ + read(pfd[1], &m, sizeof(m)); +} + +static void do_test_signal_interrupt(void) +{ + struct flock fl; + unsigned char m; + int pid, rv; + int pfd[2]; + + rv = pipe(pfd); + if (rv == -1) { + perror("pipe"); + _exit(1); + } + + fl.l_type = F_WRLCK; + fl.l_whence = SEEK_SET; + fl.l_start = 0; + fl.l_len = 1; + + rv = fcntl(fd, F_SETLK, &fl); + if (rv == -1) { + perror("fcntl"); + _exit(1); + } + + pid = fork(); + if (pid == 0) { + do_test_signal_interrupt_child(pfd); + _exit(0); + } + + /* wait until child writes */ + read(pfd[0], &m, sizeof(m)); + + fl.l_type = F_WRLCK; + fl.l_whence = SEEK_SET; + fl.l_start = 1; + fl.l_len = 1; + /* parent testing childs region, the child will think + * it has region 1-1 locked because it was interrupted + * by region 0-0. Due bugs the interruption also unlocked + * region 1-1. + */ + rv = fcntl(fd, F_SETLK, &fl); + if (rv == 0) { + fprintf(stderr, "fcntl trylock successful but should fail because child still acquires region\n"); + _exit(1); + } + + write(pfd[0], &m, sizeof(m)); + + wait(NULL); + + close(pfd[0]); + close(pfd[1]); + + /* cleanup everything */ + fl.l_type = F_UNLCK; + fl.l_whence = SEEK_SET; + fl.l_start = 0; + fl.l_len = 2; + rv = fcntl(fd, F_SETLK, &fl); + if (rv == -1) { + perror("fcntl"); + _exit(1); + } +} + +static void do_test_kill_child(void) +{ + struct flock fl; + int rv; + + fl.l_type = F_WRLCK; + fl.l_whence = SEEK_SET; + fl.l_start = 0; + fl.l_len = 1; + + rv = fcntl(fd, F_SETLKW, &fl); + if (rv == -1) { + perror("fcntl"); + _exit(1); + } +} + +static void do_test_kill(void) +{ + struct flock fl; + int pid_to_kill; + int rv; + + fl.l_type = F_WRLCK; + fl.l_whence = SEEK_SET; + fl.l_start = 0; + fl.l_len = 1; + + rv = fcntl(fd, F_SETLK, &fl); + if (rv == -1) { + perror("fcntl"); + _exit(1); + } + + pid_to_kill = fork(); + if (pid_to_kill == 0) { + do_test_kill_child(); + _exit(0); + } + + /* wait until child blocks */ + sleep(3); + + kill(pid_to_kill, SIGKILL); + + /* wait until Linux did plock cleanup */ + sleep(3); + + fl.l_type = F_UNLCK; + fl.l_whence = SEEK_SET; + fl.l_start = 0; + fl.l_len = 1; + + /* cleanup parent lock */ + rv = fcntl(fd, F_SETLK, &fl); + if (rv == -1) { + perror("fcntl"); + _exit(1); + } + + fl.l_type = F_WRLCK; + fl.l_whence = SEEK_SET; + fl.l_start = 0; + fl.l_len = 1; + + /* check if the child still holds the lock + * and killing the child was not cleaning + * up locks. + */ + rv = fcntl(fd, F_SETLK, &fl); + if ((rv == -1 && errno == EAGAIN)) { + fprintf(stderr, "fcntl trylock failed but should be successful because killing child should cleanup acquired lock\n"); + _exit(1); + } +} + +int main(int argc, char * const argv[]) +{ + if (optind != argc - 1) + usage(argv[0], "<file for fcntl> is mandatory to tell the file where to run fcntl() on it"); + + strncpy(filename, argv[1], PATH_MAX); + + fd = open(filename, O_RDWR | O_CREAT, 0700); + if (fd == -1) { + perror("open"); + _exit(1); + } + + /* test to have to equal struct file_lock requests in ->lock() */ + do_test_equal_file_lock(); + /* test to interrupt F_SETLKW by a signal and cleanup only canceled the pending interrupted request */ + do_test_signal_interrupt(); + /* test if cleanup is correct if a child gets killed while being blocked in F_SETLKW */ + do_test_kill(); + + close(fd); + + return 0; +} diff --git a/tests/generic/732 b/tests/generic/732 new file mode 100755 index 00000000..d77f9fc2 --- /dev/null +++ b/tests/generic/732 @@ -0,0 +1,32 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2023 Alexander Aring. All Rights Reserved. +# +# FS QA Test 732 +# +# This tests performs some fcntl() corner cases. See fcntl test +# program for more details. +# +. ./common/preamble +_begin_fstest auto + +# Import common functions. +. ./common/filter + +# real QA test starts here + +# Modify as appropriate. +_supported_fs generic +_require_test +_require_test_program fcntl + +echo "Silence is golden" + +$here/src/fcntl $TEST_DIR/testfile +if [ $? -ne 0 ] +then + exit +fi + +status=0 +exit diff --git a/tests/generic/732.out b/tests/generic/732.out new file mode 100644 index 00000000..451f82ce --- /dev/null +++ b/tests/generic/732.out @@ -0,0 +1,2 @@ +QA output created by 732 +Silence is golden
This patch adds fcntl corner cases that was being used to confirm issues on a GFS2 filesystem. The GFS2 filesystem has it's own ->lock() implementation and in those corner cases issues was being found and fixed. Signed-off-by: Alexander Aring <aahringo@redhat.com> --- changes since v2: - move fcntl tests into one fcntl c file - remove ofd and same owner tests, should be reflected by only one test - simplify commit message (remove testname out of it) - add error messages in fcntl.c to give more information if an error occur src/Makefile | 3 +- src/fcntl.c | 322 ++++++++++++++++++++++++++++++++++++++++++ tests/generic/732 | 32 +++++ tests/generic/732.out | 2 + 4 files changed, 358 insertions(+), 1 deletion(-) create mode 100644 src/fcntl.c create mode 100755 tests/generic/732 create mode 100644 tests/generic/732.out