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

[Jeff Layton <jlayton@kernel.org>]

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>