Re: [LTP] [PATCH 1/5] fcntl40: test for owner values on classic posix lock

[Petr Vorel <pvorel@suse.cz>]

Hi Alexander,

> This patch adds fcntl40 to test similar owner values for classical owner
> locks. There was an issue been found in the gfs2 filesystem because
> there can be collisions with identical owner values.

Thanks for your work!

...
> +++ b/testcases/kernel/syscalls/fcntl/fcntl40.c
> @@ -0,0 +1,188 @@
There is no SPDX and copyright, see other files:

// SPDX-License-Identifier: GPL-2.0-or-later
/*
 * Copyright (c) 2023 ...
 */

> +/*
NOTE: this should be /*\
to be able to get the description in our automatically generated documentation

https://github.com/linux-test-project/ltp/releases/download/20230516/metadata.20230516.html

> + * [Description]
> + * Tests gfs2 dlm posix op queue handling in the kernel.
> + * It is recommended to run watch -n 0.1 "dlm_tool plocks $LS"
> + * aside to monitor dlm plock handling.
> + *
> + * [How to use it]
> + * Call it with TMPDIR=/mnt ./fcntl40 where TMPDIR is a gfs2 mountpoint.
I wonder if we could check for GFS2_MAGIC (we'd need to add it to
include/tst_fs.h => 0x01161970) and quit the test with tst_brk(TCONF) if TMPDIR
is not on gfs2.

ATM we don't have any helper in struct tst_test, which would do it.

> + * Try it on other filesystems to compare results.
> + *
> + * [What's it doing]
nit: I'd replace this with [Algorithm].

...
> +void do_child(void)
This should be static (in all files).

make check (or make check-fcntl40) is your friend.

> +{
> +	pthread_t t1, t2;
> +
> +	SAFE_PTHREAD_CREATE(&t1, NULL, do_thread1, NULL);
> +	SAFE_PTHREAD_CREATE(&t2, NULL, do_thread2, NULL);
> +
> +	SAFE_PTHREAD_JOIN(t1, NULL);
> +	SAFE_PTHREAD_JOIN(t2, NULL);
> +
> +	tst_res(TPASS, "Child passed!");
> +}
> +
> +void do_parent(void)
This should also be static.

> +{
> +	struct flock fl = {
> +		.l_whence = SEEK_SET,
> +	};
> +
> +	/* wait for 1 seconds so thread2 lock 1-1 tries to acquires at first
> +	 * than thread1 lock 0-0 tries to acquired to have a specific waiting
> +	 * order in dlm posix handling.
> +	 */
> +	sleep(1);

I wonder if there could be some proactive check instead of sleep.
FYI we have undocumented TST_RETRY_FUNC() in C API.

> +	/* tell thread2 to call SETLKW for lock 0-0 */
> +	TST_CHECKPOINT_WAKE(1);
> +	/* wait 3 seconds for thread 1 and 2 being in waiting state */
> +	sleep(3);
> +
> +	/* unlock 0-1, should be successful */
> +	fl.l_type = F_UNLCK;
> +	fl.l_start = 1;
> +	fl.l_len = 1;
> +	tst_res(TINFO, "unlock region 1-1 thread2");
> +	SAFE_FCNTL(fd, F_SETLK, &fl);
> +
> +	/* wait until thread 2 got acquired and leave waiting */
> +	TST_CHECKPOINT_WAIT(2);
> +
> +	fl.l_start = 0;
> +	fl.l_len = 1;
> +	fl.l_type = F_UNLCK;
> +	tst_res(TINFO, "unlock region 0-0 thread2");
> +	SAFE_FCNTL(fd, F_SETLK, &fl);
> +}
> +
> +static void fcntl40_test(void)
> +{
> +	struct flock fl = {
> +		.l_type = F_WRLCK,
> +		.l_whence = SEEK_SET,
> +		.l_start = 0L,
> +		.l_len = 2L,
> +	};
> +	pid_t pid;
> +
> +	tst_res(TINFO, "parent lock region 0-1 - should be successful");
> +	SAFE_FCNTL(fd, F_SETLK, &fl);
> +	tst_res(TINFO, "parent region 0-1 locked");
> +
> +	pid = SAFE_FORK();
> +	if (pid == 0) {
> +		do_child();
> +		return;
> +	}
> +
> +	do_parent();
> +	wait(NULL);

waitpid() should be replaced by tst_reap_children(), see
https://github.com/linux-test-project/ltp/wiki/C-Test-API#18-doing-the-test-in-the-child-process

> +
> +	tst_res(TPASS, "Parent passed!");
There is TPASS in child, does it really need to be in the parent as well?
> +}
> +
> +static void setup(void)
> +{
> +	fd = SAFE_OPEN("filename", O_RDWR | O_CREAT, 0700);
> +}
> +
> +static void cleanup(void)
> +{
> +	if (fd > -1)
> +		SAFE_CLOSE(fd);
> +}
> +
> +static struct tst_test test = {
> +	.forks_child = 1,
> +	.needs_checkpoints = 1,
> +	.test_all = fcntl40_test,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +};

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp