[Cluster-devel] [RFCv2 5/7] dlm: use fl_owner from lockd

[Jeff Layton]

On Mon, 2023-08-14 at 17:11 -0400, Alexander Aring wrote:
> This patch is changing the fl_owner value in case of an nfs lock request
> to not be the pid of lockd. Instead this patch changes it to be the
> owner value that nfs is giving us.
> 
> Currently there exists proved problems with this behaviour. One nfsd
> server was created to export a gfs2 filesystem mount. Two nfs clients
> doing a nfs mount of this export. Those two clients should conflict each
> other operating on the same nfs file.
> 
> A small test program was written:
> 
> int main(int argc, const char *argv[])
> {
> 	struct flock fl = {
> 		.l_type = F_WRLCK,
> 		.l_whence = SEEK_SET,
> 		.l_start = 1L,
> 		.l_len = 1L,
> 	};
> 	int fd;
> 
> 	fd = open("filename", O_RDWR | O_CREAT, 0700);
> 	printf("try to lock...\n");
> 	fcntl(fd, F_SETLKW, &fl);
> 	printf("locked!\n");
> 	getc(stdin);
> 
> 	return 0;
> }
> 
> Running on both clients at the same time and don't interrupting by
> pressing any key. It will show that both clients are able to acquire the
> lock which shouldn't be the case. The issue is here that the fl_owner
> value is the same and the lock context of both clients should be
> separated.
> 
> This patch lets lockd define how to deal with lock contexts and chose
> hopefully the right fl_owner value. A test after this patch was made and
> the locks conflicts each other which should be the case.
> 
> Signed-off-by: Alexander Aring <aahringo at redhat.com>
> ---
>  fs/dlm/plock.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
> index 00e1d802a81c..0094fa4004cc 100644
> --- a/fs/dlm/plock.c
> +++ b/fs/dlm/plock.c
> @@ -145,6 +145,7 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file,
>  	op->info.number		= number;
>  	op->info.start		= fl->fl_start;
>  	op->info.end		= fl->fl_end;
> +	op->info.owner = (__u64)(long)fl->fl_owner;
>  	/* async handling */
>  	if (fl->fl_lmops && fl->fl_lmops->lm_grant) {
>  		op_data = kzalloc(sizeof(*op_data), GFP_NOFS);
> @@ -154,9 +155,6 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file,
>  			goto out;
>  		}
>  
> -		/* fl_owner is lockd which doesn't distinguish
> -		   processes on the nfs client */
> -		op->info.owner	= (__u64) fl->fl_pid;
>  		op_data->callback = fl->fl_lmops->lm_grant;
>  		locks_init_lock(&op_data->flc);
>  		locks_copy_lock(&op_data->flc, fl);
> @@ -168,8 +166,6 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file,
>  		send_op(op);
>  		rv = FILE_LOCK_DEFERRED;
>  		goto out;
> -	} else {
> -		op->info.owner	= (__u64)(long) fl->fl_owner;
>  	}
>  
>  	send_op(op);
> @@ -326,10 +322,7 @@ int dlm_posix_unlock(dlm_lockspace_t *lockspace, u64 number, struct file *file,
>  	op->info.number		= number;
>  	op->info.start		= fl->fl_start;
>  	op->info.end		= fl->fl_end;
> -	if (fl->fl_lmops && fl->fl_lmops->lm_grant)
> -		op->info.owner	= (__u64) fl->fl_pid;
> -	else
> -		op->info.owner	= (__u64)(long) fl->fl_owner;
> +	op->info.owner = (__u64)(long)fl->fl_owner;
>  
>  	if (fl->fl_flags & FL_CLOSE) {
>  		op->info.flags |= DLM_PLOCK_FL_CLOSE;
> @@ -389,7 +382,7 @@ int dlm_posix_cancel(dlm_lockspace_t *lockspace, u64 number, struct file *file,
>  	info.number = number;
>  	info.start = fl->fl_start;
>  	info.end = fl->fl_end;
> -	info.owner = (__u64)fl->fl_pid;
> +	info.owner = (__u64)(long)fl->fl_owner;
>  
>  	rv = do_lock_cancel(&info);
>  	switch (rv) {
> @@ -450,10 +443,7 @@ int dlm_posix_get(dlm_lockspace_t *lockspace, u64 number, struct file *file,
>  	op->info.number		= number;
>  	op->info.start		= fl->fl_start;
>  	op->info.end		= fl->fl_end;
> -	if (fl->fl_lmops && fl->fl_lmops->lm_grant)
> -		op->info.owner	= (__u64) fl->fl_pid;
> -	else
> -		op->info.owner	= (__u64)(long) fl->fl_owner;
> +	op->info.owner = (__u64)(long)fl->fl_owner;
>  
>  	send_op(op);
>  	wait_event(recv_wq, (op->done != 0));

This is the way.

Acked-by: Jeff Layton <jlayton at kernel.org>