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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
>
