[PATCHv2] generic: add fcntl corner cases tests

Commit Message

Alexander Aring Sept. 25, 2023, 8:18 p.m. UTC

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 <[e-mail]>
---
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

Comments

Alexander Aring Nov. 2, 2023, 2:37 p.m. UTC | #1

Hi,

On Mon, Sep 25, 2023 at 4:18 PM Alexander Aring <[e-mail]> 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 <[e-mail]>
> ---
> 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

Alexander Aring Feb. 1, 2024, 3:03 p.m. UTC | #2

Hi,

On Thu, Nov 2, 2023 at 10:37 AM Alexander Aring <[e-mail]> wrote:
>
> Hi,
>
> On Mon, Sep 25, 2023 at 4:18 PM Alexander Aring <[e-mail]> 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 <[e-mail]>
> > ---
> > 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

Jeffrey Layton Feb. 1, 2024, 5:10 p.m. UTC | #3

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 <[e-mail]>
> ---
> 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 <[e-mail]>

Zorro Lang Feb. 2, 2024, 12:04 p.m. UTC | #4

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 <[e-mail]>
> > ---
> > 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 <[e-mail]>
>

Alexander Aring Feb. 2, 2024, 12:19 p.m. UTC | #5

Hi Zorro,

On Fri, Feb 2, 2024 at 7:04 AM Zorro Lang <[e-mail]> 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 <[e-mail]>
> > > ---
> > > 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

Zorro Lang Feb. 2, 2024, 12:27 p.m. UTC | #6

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 <[e-mail]> 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 <[e-mail]>
> > > > ---
> > > > 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
>

Jeffrey Layton Feb. 2, 2024, 12:36 p.m. UTC | #7

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 <[e-mail]> 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 <[e-mail]>
> > > > > ---
> > > > > 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.

Zorro Lang Feb. 2, 2024, 12:46 p.m. UTC | #8

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 <[e-mail]> 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 <[e-mail]>
> > > > > > ---
> > > > > > 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 <[e-mail]>
>

Zorro Lang Feb. 9, 2024, 5:26 a.m. UTC | #9

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 <[e-mail]>
> ---

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
>

Steve French Feb. 9, 2024, 5:35 a.m. UTC | #10

Could you forward the changeset for this so we could try it?

On Thu, Feb 8, 2024 at 11:26 PM Zorro Lang <[e-mail]> 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 <[e-mail]>
> > ---
>
> 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
> >
>
>

Zorro Lang Feb. 9, 2024, 11:43 a.m. UTC | #11

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 <[e-mail]> 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 <[e-mail]>
> > > ---
> >
> > 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
>

Zorro Lang March 1, 2024, 10:38 a.m. UTC | #12

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 <[e-mail]> 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 <[e-mail]>
> > > > ---
> > >
> > > 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
> >

Alexander Aring March 1, 2024, 2:08 p.m. UTC | #13

Hi,

On Fri, Mar 1, 2024 at 5:38 AM Zorro Lang <[e-mail]> 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

Paulo Alcantara March 1, 2024, 4:25 p.m. UTC | #14

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.

Alexander Aring March 1, 2024, 5:23 p.m. UTC | #15

Hi,

On Fri, Mar 1, 2024 at 11:25 AM Paulo Alcantara <[e-mail]> 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

Paulo Alcantara March 1, 2024, 11:59 p.m. UTC | #16

Alexander Aring <[e-mail]> writes:

> Hi,
>
> On Fri, Mar 1, 2024 at 11:25 AM Paulo Alcantara <[e-mail]> 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?

Zorro Lang March 3, 2024, 5:08 a.m. UTC | #17

On Fri, Mar 01, 2024 at 08:59:01PM -0300, Paulo Alcantara wrote:
> Alexander Aring <[e-mail]> writes:
> 
> > Hi,
> >
> > On Fri, Mar 1, 2024 at 11:25 AM Paulo Alcantara <[e-mail]> 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?
>

Anand Jain March 7, 2024, 3:58 p.m. UTC | #18

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 <[e-mail]>
> ---
> 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

Zorro Lang March 30, 2024, 7:25 a.m. UTC | #19

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 <[e-mail]>
> ---
> 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
>

Alexander Aring April 2, 2024, 2:56 p.m. UTC | #20

Hi,

On Sat, Mar 30, 2024 at 3:25 AM Zorro Lang <[e-mail]> 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 <[e-mail]>
> > ---
> > 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

Patch

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