syscalls/fcntl: add new test for open file description locks
PATCH 1/1 syscalls/fcntl: add new test for open file description locks
OFD lock is available since Linux 3.15 and new F_OFD_* macros added to lapi/fcntl.h
Test is based on [1] article and checks that OFD locks can syncronise access to a file between multiple threads.
[1] https://lwn.net/Articles/586904
--- include/lapi/fcntl.h | 6 + runtest/syscalls | 2 + testcases/kernel/syscalls/.gitignore | 2 + testcases/kernel/syscalls/fcntl/Makefile | 3 + testcases/kernel/syscalls/fcntl/fcntl34.c | 172 +++++++++++++++++++++++++++++ 5 files changed, 185 insertions(+), 0 deletions(-) create mode 100644 testcases/kernel/syscalls/fcntl/fcntl34.c
include/lapi/fcntl.h +6 -0
@@ -43,6 +43,12 @@# define F_OWNER_PGRP 2#endif +#ifndef F_OFD_GETLK +#define F_OFD_GETLK 36 +#define F_OFD_SETLK 37 +#define F_OFD_SETLKW 38 +#endif +#ifndef AT_FDCWD # define AT_FDCWD -100#endif
runtest/syscalls +2 -0
@@ -265,6 +265,8 @@ fcntl32 fcntl32 fcntl32_64 fcntl32_64 fcntl33 fcntl33 fcntl33_64 fcntl33_64 +fcntl34 fcntl34 +fcntl34_64 fcntl34_64 fdatasync01 fdatasync01 fdatasync02 fdatasync02
testcases/kernel/syscalls/.gitignore +2 -0
@@ -228,6 +228,8 @@ /fcntl/fcntl32_64 /fcntl/fcntl33 /fcntl/fcntl33_64 +/fcntl/fcntl34 +/fcntl/fcntl34_64 /fdatasync/fdatasync01 /fdatasync/fdatasync02 /flock/flock01
testcases/kernel/syscalls/fcntl/Makefile +3 -0
@@ -21,6 +21,9 @@ top_srcdir ?= ../../../.. fcntl33: LDLIBS+=-lrt fcntl33_64: LDLIBS+=-lrt +fcntl34: LDLIBS += -lpthread +fcntl34_64: LDLIBS += -lpthread + include $(top_srcdir)/include/mk/testcases.mk include $(abs_srcdir)/../utils/newer_64.mk
testcases/kernel/syscalls/fcntl/fcntl34.c new file +172 -0
172 lines not on this page · raw message
Signature · 1 line
1.7.1
PATCH 1/1 syscalls/fcntl: add new test for open file description locks
Hi,
Quoted 7 lines · Alexey Kodanev
OFD lock is available since Linux 3.15 and new F_OFD_* macros added to
lapi/fcntl.hTest is based on [1] article and checks that OFD locks can syncronise access to a file between multiple threads.
Any comments?
Thanks,
Alexey
PATCH 1/1 syscalls/fcntl: add new test for open file description locks
Hi!
Quoted 9 lines
Quoted 7 lines
OFD lock is available since Linux 3.15 and new F_OFD_* macros added to
lapi/fcntl.hTest is based on [1] article and checks that OFD locks can syncronise access to a file between multiple threads.
Any comments?
Sorry I've missed this one, will have a look asap.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/1 syscalls/fcntl: add new test for open file description locks
In reply to Alexey KodanevHi!
include/lapi/fcntl.h line 50
Quoted 5 lines
include/lapi/fcntl.h +5
-0+#ifndef F_OFD_GETLK +#define F_OFD_GETLK 36 +#define F_OFD_SETLK 37 +#define F_OFD_SETLKW 38 +#endif
I prefer having the ifdef/define/endif for each of the values even when it's unlikely that SETLK is defined and GETLK is undefined.
It would be also better to include space after the # in the inner define, but that is very minor.
Quoted 75 lines
#ifndef
AT_FDCWD# define AT_FDCWD -100 #endifruntest/syscalls +2
-0@@ -265,6 +265,8 @@ fcntl32 fcntl32 fcntl32_64 fcntl32_64 fcntl33 fcntl33 fcntl33_64 fcntl33_64 +fcntl34 fcntl34 +fcntl34_64 fcntl34_64 fdatasync01 fdatasync01 fdatasync02 fdatasync02testcases/kernel/syscalls/.gitignore +2
-0@@ -228,6 +228,8 @@ /fcntl/fcntl32_64 /fcntl/fcntl33 /fcntl/fcntl33_64 +/fcntl/fcntl34 +/fcntl/fcntl34_64 /fdatasync/fdatasync01 /fdatasync/fdatasync02 /flock/flock01testcases/kernel/syscalls/fcntl/Makefile +3
-0@@ -21,6 +21,9 @@ top_srcdir ?= ../../../.. fcntl33: LDLIBS+=-lrt fcntl33_64: LDLIBS+=-lrt +fcntl34: LDLIBS += -lpthread +fcntl34_64: LDLIBS += -lpthread + include $(top_srcdir)/include/mk/testcases.mk include $(abs_srcdir)/../utils/newer_64.mktestcases/kernel/syscalls/fcntl/fcntl34.c new file +26
-0@@ -0,0 +1,172 @@ +/* +* Copyright (c) 2016 Oracle and/or its affiliates. All Rights Reserved. +* +* This program is free software; you can redistribute it and/or +* modify it under the terms of the GNU General Public License as +* published by the Free Software Foundation; either version 2 of +* the License, or (at your option) any later version. +* +* This program is distributed in the hope that it would be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program. If not, see <http://www.gnu.org/licenses/>. +* +* Author: Alexey Kodanev <[email protected]> +* +*/ + +#include <sys/types.h> +#include <sys/stat.h> +#include <unistd.h> +#include <fcntl.h> +#include <pthread.h> +#include <errno.h>
testcases/kernel/syscalls/fcntl/fcntl34.c line 36
Quoted 10 lines
testcases/kernel/syscalls/fcntl/fcntl34.c new file +10
-0+ +#include "lapi/fcntl.h" +#include "test.h" +#include "safe_macros.h" + +char *TCID ="fcntl34" ; +int TST_TOTAL =1 ; + +static int thread_size; +static const int max_thread_size =32 ;
These should be renamed to threads and max_threads or thread_cnt and max_thread_cnt, since size describes something that could be measured (like distance) and count describes something that could be counted.
testcases/kernel/syscalls/fcntl/fcntl34.c line 46
Quoted 10 lines
testcases/kernel/syscalls/fcntl/fcntl34.c new file +10
-0+static const char fname[] ="tst_ofd_locks" ; +const int writes_num =100 ; +const int bytes_num =4096 ; + +TST_DECLARE_ONCE_FN(cleanup, tst_rmdir) + +static void setup (void ) +{ +tst_tmpdir (); + thread_size =SAFE_SYSCONF (cleanup, _SC_NPROCESSORS_CONF) *3 ;
We have tst_ncpus_conf() in the test library, I guess that we should use it instead.
testcases/kernel/syscalls/fcntl/fcntl34.c line 51
Quoted 5 lines
testcases/kernel/syscalls/fcntl/fcntl34.c new file +5
-0+if (thread_size > max_thread_size) + thread_size = max_thread_size; +} + +static void spawn_threads(pthread_t *id, int size,
^
same here should be renamed to threads or something similar, size is really confusing
testcases/kernel/syscalls/fcntl/fcntl34.c line 60
Quoted 9 lines
testcases/kernel/syscalls/fcntl/fcntl34.c new file +9
-0+ void *(*thread_fn)(void *)) +{ +intptr_t i; + +tst_resm (TINFO ,"spawning '%d' threads" , size); +for (i =0 ; i < size; ++i) { +if (pthread_create (id + i,NULL , thread_fn, (void *)i)) { +tst_brkm (TBROK |TERRNO , cleanup, +"pthread_create() failed" );
Using TERRNO is wrong here. The error value is returned by the pthread_create() instead.
You should do:
ret = pthread_create(...);
if (ret) {
tst_brkm(TBROK, cleanup, "pthread_create(): %s",
tst_strerrno(ret));
}testcases/kernel/syscalls/fcntl/fcntl34.c line 65
Quoted 5 lines
testcases/kernel/syscalls/fcntl/fcntl34.c new file +5
-0+ } + } +} + +static void wait_threads (pthread_t *id,int size)
^
Here as well.
testcases/kernel/syscalls/fcntl/fcntl34.c line 72
Quoted 7 lines
testcases/kernel/syscalls/fcntl/fcntl34.c new file +7
-0+{ +int i; + +tst_resm (TINFO ,"waiting for '%d' threads" , size); +for (i =0 ; i < size; ++i) { +if (pthread_join (id[i],NULL ) !=0 ) +tst_brkm (TBROK , cleanup,"pthread_join() failed" );
Here as well, we should use return value from the pthread_join().
Maybe we should create safe_pthread.h header as well...
testcases/kernel/syscalls/fcntl/fcntl34.c line 79
Quoted 7 lines
testcases/kernel/syscalls/fcntl/fcntl34.c new file +7
-0+ } +} + +void *thread_fn_01 (void *arg) +{ +int i; +char buf[bytes_num];
^
On the other hand this can be renamed to write_size as byte buffers have size.
testcases/kernel/syscalls/fcntl/fcntl34.c line 80
Quoted 1 line
testcases/kernel/syscalls/fcntl/fcntl34.c new file +1
-0+int fd =SAFE_OPEN (cleanup, fname,O_RDWR ,0600 );
^
No need for the mode here if O_CREAT is not in flags.
Quoted 29 lines
testcases/kernel/syscalls/fcntl/fcntl34.c new file +29
-0+memset (buf, (intptr_t )arg, bytes_num); + +struct flock lck = { + .l_whence =SEEK_SET , + .l_start =0 , + .l_len =1 , + }; + +for (i =0 ; i < writes_num; ++i) { + lck.l_type =F_WRLCK ; +if (fcntl (fd,F_OFD_SETLKW , &lck) ==-1 ) +tst_brkm (TBROK |TERRNO , cleanup,"fcntl() failed" ); + +SAFE_LSEEK (cleanup, fd,0 ,SEEK_END ); +SAFE_WRITE (cleanup,1 , fd, buf, bytes_num); + + lck.l_type =F_UNLCK ; +if (fcntl (fd,F_OFD_SETLKW , &lck) ==-1 ) +tst_brkm (TBROK |TERRNO , cleanup,"fcntl() failed" ); + +pthread_yield (); + } + +SAFE_CLOSE (cleanup, fd); + +return NULL ; +} + +void test01 (void )
testcases/kernel/syscalls/fcntl/fcntl34.c line 120
Quoted 10 lines
testcases/kernel/syscalls/fcntl/fcntl34.c new file +10
-0+{ +intptr_t i; +int k; +pthread_t id[thread_size]; +int res[thread_size]; +char buf[bytes_num]; + +tst_resm (TINFO ,"write to a file inside threads with OFD locks" ); + +int fd =SAFE_OPEN (cleanup, fname,O_CREAT |O_RDONLY ,0600 );
Shouldn't we truncate the file here in case that the test is looping with -i option?
testcases/kernel/syscalls/fcntl/fcntl34.c line 133
Quoted 12 lines
testcases/kernel/syscalls/fcntl/fcntl34.c new file +12
-0+memset (res,0 ,sizeof (res)); + +spawn_threads (id, thread_size, thread_fn_01); +wait_threads (id, thread_size); + +tst_resm (TINFO ,"verifying file's data" ); +SAFE_LSEEK (cleanup, fd,0 ,SEEK_SET ); +for (i =0 ; i < writes_num * thread_size; ++i) { +SAFE_READ (cleanup,1 , fd, buf, bytes_num); + +if (buf[0 ] <0 || buf[0 ] > thread_size) +tst_brkm (TFAIL , cleanup,"unexpected data read" );
I would rather do tst_resm(TFAIL, ...) followed by return here and in the rest of the checks below as well.
testcases/kernel/syscalls/fcntl/fcntl34.c line 135
Quoted 1 line
testcases/kernel/syscalls/fcntl/fcntl34.c new file +1
-0+ ++res[(int )buf[0 ]];
^
Do we really need to cast it to int here?
Quoted 26 lines
testcases/kernel/syscalls/fcntl/fcntl34.c new file +26
-0+for (k =1 ; k < bytes_num; ++k) { +if (buf[0 ] != buf[k]) { +tst_brkm (TFAIL , cleanup, +"unexpected data read" ); + } + } + } + +for (i =0 ; i < thread_size; ++i) { +if (res[i] != writes_num) +tst_brkm (TFAIL , cleanup,"corrupted data found" ); + } +SAFE_CLOSE (cleanup, fd); + +tst_resm (TPASS ,"OFD locks synchronized access between threads" ); +} + +int main (int ac,char *av[]) +{ +int lc; + +tst_parse_opts (ac, av,NULL ,NULL ); + +if (tst_kvercmp (3 ,15 ,0 ) <0 ) +tst_brkm (TCONF ,NULL ,"Test must be run with kernel 3.15+" ); +
testcases/kernel/syscalls/fcntl/fcntl34.c line 172
Quoted 10 lines
testcases/kernel/syscalls/fcntl/fcntl34.c new file +10
-0+setup (); + +for (lc =0 ;TEST_LOOPING (lc); ++lc) { + tst_count =0 ; +test01 (); + } + +cleanup (); +tst_exit (); +}
Otherwise it looks good.
I would be happier if the code would get converted to the new test library but given that I've managed to ignore it for quite some time I'm in no position to force it.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/1 syscalls/fcntl: add new test for open file description locks
Hi,
Quoted 12 lines · Cyril Hrubis
Hi!
testcases/kernel/syscalls/fcntl/fcntl34.c line 60
Quoted 8 lines
testcases/kernel/syscalls/fcntl/fcntl34.c new file +8
-0+#ifndef F_OFD_GETLK +intptr_t i; + +tst_resm (TINFO ,"spawning '%d' threads" , size); +for (i =0 ; i < size; ++i) { +if (pthread_create (id + i,NULL , thread_fn, (void *)i)) { +tst_brkm (TBROK |TERRNO , cleanup, +"pthread_create() failed" );Using TERRNO is wrong here. The error value is returned by the
pthread_create()instead.
Quoted 7 lines
You should do:
ret = pthread_create(...); if (ret) { tst_brkm(TBROK, cleanup, "pthread_create(): %s", tst_strerrno(ret)); }
Oh, that wasn't obvious, I should have read man-page more carefully. Thanks.
Quoted 3 lines
testcases/kernel/syscalls/fcntl/fcntl34.c line 135
Quoted 1 line
testcases/kernel/syscalls/fcntl/fcntl34.c new file +1
-0+ ++res[(int )buf[0 ]];^
Do we really need to cast it to int here?
Otherwise gcc would complain "warning: array subscript has type ‘char’"
Quoted 9 lines
testcases/kernel/syscalls/fcntl/fcntl34.c line 172
Quoted 4 lines
testcases/kernel/syscalls/fcntl/fcntl34.c new file +4
-0+ +cleanup (); +tst_exit (); +}Otherwise it looks good.
I would be happier if the code would get converted to the new test library but given that I've managed to ignore it for quite some time I'm in no position to force it.
I'll send a new patch with new API usage and the one for safe_pthread.
Thanks,
Alexey
PATCH 1/1 syscalls/fcntl: add new test for open file description locks
Hi!
Quoted 6 lines
Quoted 3 lines
testcases/kernel/syscalls/fcntl/fcntl34.c line 135
Quoted 1 line
testcases/kernel/syscalls/fcntl/fcntl34.c new file +1
-0+ ++res[(int )buf[0 ]];^
Do we really need to cast it to int here?Otherwise gcc would complain "warning: array subscript has type ?char?"
Hmm, perhaps the compiler tries to warn you that char may be signed or unsigned depending on a architecture and compiler implementation. Does the warning goes away if you declare the buf as a unsigned char?
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/1 syscalls/fcntl: add new test for open file description locks
Hi,
Quoted 9 lines · Cyril Hrubis
Hi!
Quoted 5 lines
Quoted 3 lines
testcases/kernel/syscalls/fcntl/fcntl34.c line 135
Quoted 1 line
testcases/kernel/syscalls/fcntl/fcntl34.c new file +1
-0+ ++res[(int )buf[0 ]];^
Do we really need to cast it to int here?Otherwise gcc would complain "warning: array subscript has type ?char?"
Hmm, perhaps the compiler tries to warn you that char may be signed or unsigned depending on a architecture and compiler implementation. Does the warning goes away if you declare the buf as a unsigned char?
Correct, it went away with unsigned char... more safe this way.
Thanks,
Alexey