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

[Alexander Aring <aahringo@redhat.com>]

Hi,

On Wed, Jun 21, 2023 at 5:10 AM Petr Vorel <pvorel@suse.cz> wrote:
>
> 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
>

ok

> > + * [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.
>

I will mention that gfs2 is only an example here. It becomes
interesting when a file system implements its own .lock() callback OR
if somebody wants to test file system core, when a filesystem does not
implement its own .lock().

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

ok.

> ...
> > +void do_child(void)
> This should be static (in all files).
>
> make check (or make check-fcntl40) is your friend.
>

ok. Thanks for telling me about make check.

> > +{
> > +     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.
>

ok.

> > +{
> > +     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.
>

I try to look into it.

> > +     /* 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
>

ok. Thanks.

> > +
> > +     tst_res(TPASS, "Parent passed!");
> There is TPASS in child, does it really need to be in the parent as well?

no.

- Alex


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