| Message ID | 20230801175220.1558342-3-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: > The locker was waiting for sem_otime on sem0 to became non-zero after > incrementing sem0 himself. So sem_otime was never 0 at the time of > checking it, so the check was redundant/wrong. > > This patch: > - moves the increment of sem1 to the lock-tester site > - lock-setter waits for that sem1 event, for which this patch replaces > the wait loop on sem_otime with GETVAL loop, adding a small sleep > - increment of sem0 to 2 moved past that sem1 event. That sem0 event > is currently not used/waited. > > This guarantees that the lock-setter is working only after lock-getter > is fully initialized. > > 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 | 44 +++++++++++++++++++++++++------------------- > 1 file changed, 25 insertions(+), 19 deletions(-) > > diff --git a/src/t_ofd_locks.c b/src/t_ofd_locks.c > index 88ef2690..58cb0959 100644 > --- a/src/t_ofd_locks.c > +++ b/src/t_ofd_locks.c > @@ -294,32 +294,29 @@ int main(int argc, char **argv) > semu.array = vals; > if (semctl(semid, 2, SETALL, semu) == -1) > err_exit("init sem", errno); > - /* Inc both new sem to 2 */ > - sop.sem_num = 0; > - sop.sem_op = 1; > - sop.sem_flg = 0; > - ts.tv_sec = 5; > - ts.tv_nsec = 0; > - if (semtimedop(semid, &sop, 1, &ts) == -1) > - err_exit("inc sem0 2", errno); > - sop.sem_num = 1; > - sop.sem_op = 1; > - sop.sem_flg = 0; > - ts.tv_sec = 5; > - ts.tv_nsec = 0; > - if (semtimedop(semid, &sop, 1, &ts) == -1) > - err_exit("inc sem1 2", errno); > > /* > - * Wait initialization complete. semctl(2) only update > - * sem_ctime, semop(2) will update sem_otime. > + * 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 2", errno); > + } while (ret != 2); > + > + /* Inc sem0 to 2 */ > + sop.sem_num = 0; > + sop.sem_op = 1; > + sop.sem_flg = 0; > + ts.tv_sec = 5; > + ts.tv_nsec = 0; > + if (semtimedop(semid, &sop, 1, &ts) == -1) > + err_exit("inc sem0 2", errno); > > /* place the lock */ > if (fcntl(fd, setlk_macro, &flk) < 0) > @@ -385,6 +382,15 @@ int main(int argc, char **argv) > err_exit("wait sem1 1", errno); > } while (ret != 1); > > + /* inc sem1 to 2 (initialization completed) */ > + sop.sem_num = 1; > + sop.sem_op = 1; > + sop.sem_flg = 0; > + ts.tv_sec = 5; > + ts.tv_nsec = 0; > + if (semtimedop(semid, &sop, 1, &ts) == -1) > + err_exit("inc sem1 2", errno); > + > /* wait sem0 == 0 (setlk and close fd done) */ > sop.sem_num = 0; > sop.sem_op = 0; Reviewed-by: Jeff Layton <jlayton@kernel.org>
diff --git a/src/t_ofd_locks.c b/src/t_ofd_locks.c index 88ef2690..58cb0959 100644 --- a/src/t_ofd_locks.c +++ b/src/t_ofd_locks.c @@ -294,32 +294,29 @@ int main(int argc, char **argv) semu.array = vals; if (semctl(semid, 2, SETALL, semu) == -1) err_exit("init sem", errno); - /* Inc both new sem to 2 */ - sop.sem_num = 0; - sop.sem_op = 1; - sop.sem_flg = 0; - ts.tv_sec = 5; - ts.tv_nsec = 0; - if (semtimedop(semid, &sop, 1, &ts) == -1) - err_exit("inc sem0 2", errno); - sop.sem_num = 1; - sop.sem_op = 1; - sop.sem_flg = 0; - ts.tv_sec = 5; - ts.tv_nsec = 0; - if (semtimedop(semid, &sop, 1, &ts) == -1) - err_exit("inc sem1 2", errno); /* - * Wait initialization complete. semctl(2) only update - * sem_ctime, semop(2) will update sem_otime. + * 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 2", errno); + } while (ret != 2); + + /* Inc sem0 to 2 */ + sop.sem_num = 0; + sop.sem_op = 1; + sop.sem_flg = 0; + ts.tv_sec = 5; + ts.tv_nsec = 0; + if (semtimedop(semid, &sop, 1, &ts) == -1) + err_exit("inc sem0 2", errno); /* place the lock */ if (fcntl(fd, setlk_macro, &flk) < 0) @@ -385,6 +382,15 @@ int main(int argc, char **argv) err_exit("wait sem1 1", errno); } while (ret != 1); + /* inc sem1 to 2 (initialization completed) */ + sop.sem_num = 1; + sop.sem_op = 1; + sop.sem_flg = 0; + ts.tv_sec = 5; + ts.tv_nsec = 0; + if (semtimedop(semid, &sop, 1, &ts) == -1) + err_exit("inc sem1 2", errno); + /* wait sem0 == 0 (setlk and close fd done) */ sop.sem_num = 0; sop.sem_op = 0;
The locker was waiting for sem_otime on sem0 to became non-zero after incrementing sem0 himself. So sem_otime was never 0 at the time of checking it, so the check was redundant/wrong. This patch: - moves the increment of sem1 to the lock-tester site - lock-setter waits for that sem1 event, for which this patch replaces the wait loop on sem_otime with GETVAL loop, adding a small sleep - increment of sem0 to 2 moved past that sem1 event. That sem0 event is currently not used/waited. This guarantees that the lock-setter is working only after lock-getter is fully initialized. 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 | 44 +++++++++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 19 deletions(-)