| Message ID | 20230801175220.1558342-2-stsp2@yandex.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t_ofd_locks: ipc semaphore fixes | expand |
On Tue, 2023-08-01 at 22:52 +0500, Stas Sergeev wrote: > Currently IPC_RMID was attempted on a semid returned after failed > semget() with flags=IPC_CREAT|IPC_EXCL. So nothing was actually removed. > > This patch introduces the much more reliable scheme where the wrapper > script creates and removes semaphores, passing a sem key to the test > binary via new -K option. > > This patch speeds up the test ~5 times by removing the sem-awaiting > loop in a lock-getter process. As the semaphore is now created before > the test process started, there is no need to wait for anything. > > CC: fstests@vger.kernel.org > CC: Murphy Zhou <xzhou@redhat.com> > CC: Jeff Layton <jlayton@kernel.org> > CC: Zorro Lang <zlang@redhat.com> > > Signed-off-by: Stas Sergeev <stsp2@yandex.ru> > --- > src/t_ofd_locks.c | 77 +++++++++++++++-------------------------------- > tests/generic/478 | 37 ++++++++++++++++++++--- > 2 files changed, 58 insertions(+), 56 deletions(-) > > diff --git a/src/t_ofd_locks.c b/src/t_ofd_locks.c > index e77f2659..88ef2690 100644 > --- a/src/t_ofd_locks.c > +++ b/src/t_ofd_locks.c > @@ -87,8 +87,6 @@ static void err_exit(char *op, int errn) > fprintf(stderr, "%s: %s\n", op, strerror(errn)); > if (fd > 0) > close(fd); > - if (semid > 0 && semctl(semid, 2, IPC_RMID) == -1) > - perror("exit rmid"); > exit(errn); > } > > @@ -180,16 +178,16 @@ int main(int argc, char **argv) > int setlk_macro = F_OFD_SETLKW; > int getlk_macro = F_OFD_GETLK; > struct timespec ts; > - key_t semkey; > + key_t semkey = -1; > unsigned short vals[2]; > union semun semu; > struct semid_ds sem_ds; > struct sembuf sop; > - int opt, ret, retry; > + int opt, ret; > > //avoid libcap errno bug > errno = 0; > - while((opt = getopt(argc, argv, "sgrwo:l:PRWtFd")) != -1) { > + while((opt = getopt(argc, argv, "sgrwo:l:PRWtFdK:")) != -1) { > switch(opt) { > case 's': > lock_cmd = 1; > @@ -227,6 +225,9 @@ int main(int argc, char **argv) > case 'd': > use_dup = 1; > break; > + case 'K': > + semkey = strtol(optarg, NULL, 16); > + break; > default: > usage(argv[0]); > return -1; > @@ -276,37 +277,15 @@ int main(int argc, char **argv) > err_exit("test_ofd_getlk", errno); > } > > - if((semkey = ftok(argv[optind], 255)) == -1) > + if (semkey == -1) > + semkey = ftok(argv[optind], 255); > + if (semkey == -1) > err_exit("ftok", errno); > > /* setlk, and always init the semaphore at setlk time */ > if (lock_cmd == 1) { > - /* > - * Init the semaphore, with a key related to the testfile. > - * getlk routine will wait untill this sem has been created and > - * iniialized. > - * > - * We must make sure the semaphore set is newly created, rather > - * then the one left from last run. In which case getlk will > - * exit immediately and left setlk routine waiting forever. > - * Also because newly created semaphore has zero sem_otime, > - * which is used here to sync with getlk routine. > - */ > - retry = 0; > - do { > - semid = semget(semkey, 2, IPC_CREAT|IPC_EXCL); > - if (semid < 0 && errno == EEXIST) { > - /* remove sem set after one round of test */ > - if (semctl(semid, 2, IPC_RMID, semu) == -1) > - err_exit("rmid 0", errno); > - retry++; > - } else if (semid < 0) > - err_exit("semget", errno); > - else > - retry = 10; > - } while (retry < 5); > - /* We can't create a new semaphore set in 5 tries */ > - if (retry == 5) > + semid = semget(semkey, 2, 0); > + if (semid < 0) > err_exit("semget", errno); > > /* Init both new sem to 1 */ > @@ -382,35 +361,29 @@ int main(int argc, char **argv) > ts.tv_nsec = 0; > if (semtimedop(semid, &sop, 1, &ts) == -1) > err_exit("wait sem1 0", errno); > - > - /* remove sem set after one round of test */ > - if (semctl(semid, 2, IPC_RMID, semu) == -1) > - err_exit("rmid", errno); > close(fd); > exit(0); > } > > /* getlck */ > if (lock_cmd == 0) { > - /* wait sem created and initialized */ > - retry = 5; > - do { > - semid = semget(semkey, 2, 0); > - if (semid != -1) > - break; > - if (errno == ENOENT && retry) { > - sleep(1); > - retry--; > - continue; > - } else { > - err_exit("getlk_semget", errno); > - } > - } while (1); > + semid = semget(semkey, 2, 0); > + if (semid < 0) > + err_exit("getlk_semget", errno); > + > + /* > + * Wait initialization complete. > + */ > + ret = -1; > do { > + if (ret != -1) > + usleep(100000); > memset(&sem_ds, 0, sizeof(sem_ds)); > semu.buf = &sem_ds; > - ret = semctl(semid, 0, IPC_STAT, semu); > - } while (!(ret == 0 && sem_ds.sem_otime != 0)); > + ret = semctl(semid, 1, GETVAL, semu); > + if (ret == -1) > + err_exit("wait sem1 1", errno); > + } while (ret != 1); > > /* wait sem0 == 0 (setlk and close fd done) */ > sop.sem_num = 0; > diff --git a/tests/generic/478 b/tests/generic/478 > index 480762d2..419acc94 100755 > --- a/tests/generic/478 > +++ b/tests/generic/478 > @@ -99,33 +99,62 @@ _require_ofd_locks > $XFS_IO_PROG -f -c "pwrite -S 0xFF 0 4096" \ > $TEST_DIR/testfile >> $seqres.full 2>&1 > > +mk_sem() > +{ > + SEMID=$(ipcmk -S 2 | cut -d ":" -f 2 | tr -d '[:space:]') > + if [ -z "$SEMID" ]; then > + echo "ipcmk failed" > + exit 1 > + fi > + SEMKEY=$(ipcs -s | grep $SEMID | cut -d " " -f 1) > + if [ -z "$SEMKEY" ]; then > + echo "ipcs failed" > + exit 1 > + fi > +} > + > +rm_sem() > +{ > + ipcrm -s $SEMID 2>/dev/null > +} > + Given that you're now creating the semaphore before launching the test program, do you need to clean it up if the shell script dies abnormally? IOW, do you need to trap on SIGTERM or something and call rm_sem() ? > do_test() > { > - local soptions="$1" > - local goptions="$2" > + local soptions > + local goptions > # print options and getlk output for debug > echo $* >> $seqres.full 2>&1 > + mk_sem > + soptions="$1 -K $SEMKEY" > + goptions="$2 -K $SEMKEY" > # -s : do setlk > $here/src/t_ofd_locks $soptions $TEST_DIR/testfile & > # -g : do getlk > $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \ > tee -a $seqres.full > wait $! > + rm_sem > > + mk_sem > # add -F to clone with CLONE_FILES > - soptions="$1 -F" > + soptions="$1 -F -K $SEMKEY" > + goptions="$2 -K $SEMKEY" > # with -F, new locks are always file to place > $here/src/t_ofd_locks $soptions $TEST_DIR/testfile & > $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \ > tee -a $seqres.full > wait $! > + rm_sem > > + mk_sem > # add -d to dup and close > - soptions="$1 -d" > + soptions="$1 -d -K $SEMKEY" > + goptions="$2 -K $SEMKEY" > $here/src/t_ofd_locks $soptions $TEST_DIR/testfile & > $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \ > tee -a $seqres.full > wait $! > + rm_sem > } > > # Always setlk at range [0,9], getlk at range [0,9] [5,24] or [20,29]. Seems like a reasonable change overall, and I like not having to micromanage the semaphore inside the C program. The cleanup thing can be fixed after the fact if it's even necessary. Reviwed-by: Jeff Layton <jlayton@kernel.org>
11.08.2023 16:39, Jeff Layton пишет: > Given that you're now creating the semaphore before launching the test > program, do you need to clean it up if the shell script dies > abnormally? I'd rather say "no" or at least I wasn't planning that. Unlike the current impl in C code, ipcmk creates the sem with random ID, so the clash is no longer a threat. As such we don't care about any previous invocations, were they successful or faulty. Which is yet another advantage of this approach. rm_sem() is here just for "politeness", whereas in prev approach you have to deal with the results of the past invocations. > Seems like a reasonable change overall, and I like not having to > micromanage the semaphore inside the C program. The cleanup thing can be > fixed after the fact if it's even necessary. > > Reviwed-by: Jeff Layton <jlayton@kernel.org> Thank you!
diff --git a/src/t_ofd_locks.c b/src/t_ofd_locks.c index e77f2659..88ef2690 100644 --- a/src/t_ofd_locks.c +++ b/src/t_ofd_locks.c @@ -87,8 +87,6 @@ static void err_exit(char *op, int errn) fprintf(stderr, "%s: %s\n", op, strerror(errn)); if (fd > 0) close(fd); - if (semid > 0 && semctl(semid, 2, IPC_RMID) == -1) - perror("exit rmid"); exit(errn); } @@ -180,16 +178,16 @@ int main(int argc, char **argv) int setlk_macro = F_OFD_SETLKW; int getlk_macro = F_OFD_GETLK; struct timespec ts; - key_t semkey; + key_t semkey = -1; unsigned short vals[2]; union semun semu; struct semid_ds sem_ds; struct sembuf sop; - int opt, ret, retry; + int opt, ret; //avoid libcap errno bug errno = 0; - while((opt = getopt(argc, argv, "sgrwo:l:PRWtFd")) != -1) { + while((opt = getopt(argc, argv, "sgrwo:l:PRWtFdK:")) != -1) { switch(opt) { case 's': lock_cmd = 1; @@ -227,6 +225,9 @@ int main(int argc, char **argv) case 'd': use_dup = 1; break; + case 'K': + semkey = strtol(optarg, NULL, 16); + break; default: usage(argv[0]); return -1; @@ -276,37 +277,15 @@ int main(int argc, char **argv) err_exit("test_ofd_getlk", errno); } - if((semkey = ftok(argv[optind], 255)) == -1) + if (semkey == -1) + semkey = ftok(argv[optind], 255); + if (semkey == -1) err_exit("ftok", errno); /* setlk, and always init the semaphore at setlk time */ if (lock_cmd == 1) { - /* - * Init the semaphore, with a key related to the testfile. - * getlk routine will wait untill this sem has been created and - * iniialized. - * - * We must make sure the semaphore set is newly created, rather - * then the one left from last run. In which case getlk will - * exit immediately and left setlk routine waiting forever. - * Also because newly created semaphore has zero sem_otime, - * which is used here to sync with getlk routine. - */ - retry = 0; - do { - semid = semget(semkey, 2, IPC_CREAT|IPC_EXCL); - if (semid < 0 && errno == EEXIST) { - /* remove sem set after one round of test */ - if (semctl(semid, 2, IPC_RMID, semu) == -1) - err_exit("rmid 0", errno); - retry++; - } else if (semid < 0) - err_exit("semget", errno); - else - retry = 10; - } while (retry < 5); - /* We can't create a new semaphore set in 5 tries */ - if (retry == 5) + semid = semget(semkey, 2, 0); + if (semid < 0) err_exit("semget", errno); /* Init both new sem to 1 */ @@ -382,35 +361,29 @@ int main(int argc, char **argv) ts.tv_nsec = 0; if (semtimedop(semid, &sop, 1, &ts) == -1) err_exit("wait sem1 0", errno); - - /* remove sem set after one round of test */ - if (semctl(semid, 2, IPC_RMID, semu) == -1) - err_exit("rmid", errno); close(fd); exit(0); } /* getlck */ if (lock_cmd == 0) { - /* wait sem created and initialized */ - retry = 5; - do { - semid = semget(semkey, 2, 0); - if (semid != -1) - break; - if (errno == ENOENT && retry) { - sleep(1); - retry--; - continue; - } else { - err_exit("getlk_semget", errno); - } - } while (1); + semid = semget(semkey, 2, 0); + if (semid < 0) + err_exit("getlk_semget", errno); + + /* + * Wait initialization complete. + */ + ret = -1; do { + if (ret != -1) + usleep(100000); memset(&sem_ds, 0, sizeof(sem_ds)); semu.buf = &sem_ds; - ret = semctl(semid, 0, IPC_STAT, semu); - } while (!(ret == 0 && sem_ds.sem_otime != 0)); + ret = semctl(semid, 1, GETVAL, semu); + if (ret == -1) + err_exit("wait sem1 1", errno); + } while (ret != 1); /* wait sem0 == 0 (setlk and close fd done) */ sop.sem_num = 0; diff --git a/tests/generic/478 b/tests/generic/478 index 480762d2..419acc94 100755 --- a/tests/generic/478 +++ b/tests/generic/478 @@ -99,33 +99,62 @@ _require_ofd_locks $XFS_IO_PROG -f -c "pwrite -S 0xFF 0 4096" \ $TEST_DIR/testfile >> $seqres.full 2>&1 +mk_sem() +{ + SEMID=$(ipcmk -S 2 | cut -d ":" -f 2 | tr -d '[:space:]') + if [ -z "$SEMID" ]; then + echo "ipcmk failed" + exit 1 + fi + SEMKEY=$(ipcs -s | grep $SEMID | cut -d " " -f 1) + if [ -z "$SEMKEY" ]; then + echo "ipcs failed" + exit 1 + fi +} + +rm_sem() +{ + ipcrm -s $SEMID 2>/dev/null +} + do_test() { - local soptions="$1" - local goptions="$2" + local soptions + local goptions # print options and getlk output for debug echo $* >> $seqres.full 2>&1 + mk_sem + soptions="$1 -K $SEMKEY" + goptions="$2 -K $SEMKEY" # -s : do setlk $here/src/t_ofd_locks $soptions $TEST_DIR/testfile & # -g : do getlk $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \ tee -a $seqres.full wait $! + rm_sem + mk_sem # add -F to clone with CLONE_FILES - soptions="$1 -F" + soptions="$1 -F -K $SEMKEY" + goptions="$2 -K $SEMKEY" # with -F, new locks are always file to place $here/src/t_ofd_locks $soptions $TEST_DIR/testfile & $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \ tee -a $seqres.full wait $! + rm_sem + mk_sem # add -d to dup and close - soptions="$1 -d" + soptions="$1 -d -K $SEMKEY" + goptions="$2 -K $SEMKEY" $here/src/t_ofd_locks $soptions $TEST_DIR/testfile & $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \ tee -a $seqres.full wait $! + rm_sem } # Always setlk at range [0,9], getlk at range [0,9] [5,24] or [20,29].
Currently IPC_RMID was attempted on a semid returned after failed semget() with flags=IPC_CREAT|IPC_EXCL. So nothing was actually removed. This patch introduces the much more reliable scheme where the wrapper script creates and removes semaphores, passing a sem key to the test binary via new -K option. This patch speeds up the test ~5 times by removing the sem-awaiting loop in a lock-getter process. As the semaphore is now created before the test process started, there is no need to wait for anything. CC: fstests@vger.kernel.org CC: Murphy Zhou <xzhou@redhat.com> CC: Jeff Layton <jlayton@kernel.org> CC: Zorro Lang <zlang@redhat.com> Signed-off-by: Stas Sergeev <stsp2@yandex.ru> --- src/t_ofd_locks.c | 77 +++++++++++++++-------------------------------- tests/generic/478 | 37 ++++++++++++++++++++--- 2 files changed, 58 insertions(+), 56 deletions(-)