[PATCH 2/2] t_ofd_locks: fix sem initialization sequence

[PATCH 2/2] t_ofd_locks: fix sem initialization sequence

[Stas Sergeev]

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 95739dc7..b7b2022d 100644
--- a/src/t_ofd_locks.c
+++ b/src/t_ofd_locks.c
@@ -296,32 +296,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)
@@ -388,6 +385,15 @@ int main(int argc, char **argv)
 			}
 		} while (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;
-- 
2.39.2

[PATCH v3 0/2] t_ofd_locks: ipc semaphore fixes

[Stas Sergeev]

This patch set provides 2 semaphore fixes for t_ofd_locks.
First patch fixes the handling of stalled semaphore.
Second patch fixes the init sequence, making sure the lock-setter
process actually waits for the lock-getter to initialize.

Changes in v3: sem is now created by the wrapper script, rather than
by the lock-setter process. This allowed to remove the sem-await loop
from the lock-getter, speeding up the test ~5 times.

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>

Stas Sergeev (2):
  t_ofd_locks: fix stalled semaphore handling
  t_ofd_locks: fix sem initialization sequence

 src/t_ofd_locks.c | 92 +++++++++++++++++++----------------------------
 tests/generic/478 | 14 +++++---
 2 files changed, 47 insertions(+), 59 deletions(-)

-- 
2.39.2

[PATCH 1/2] t_ofd_locks: fix stalled semaphore handling

[Stas Sergeev]

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
+}
+
 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].
-- 
2.39.2

[PATCH 2/2] t_ofd_locks: fix sem initialization sequence

[Stas Sergeev]

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;
-- 
2.39.2

Re: [PATCH 1/2] t_ofd_locks: fix stalled semaphore handling

[Jeff Layton]

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>

Re: [PATCH 2/2] t_ofd_locks: fix sem initialization sequence

[Jeff Layton]

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>

Re: [PATCH 1/2] t_ofd_locks: fix stalled semaphore handling

[stsp]

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!