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

[Jeff Layton <jlayton@kernel.org>]

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>