Re: [PATCH 3/3] staging/lustre, 9p, ceph, cifs, dlm: negate remote pids for F_GETLK

[Jeff Layton <jlayton@poochiereds.net>]

On Tue, 2017-06-27 at 11:18 -0400, Benjamin Coddington wrote:
> In the previous patch, the locks API will expect that if a filesystem
> returns a remote pid as opposed to a local pid for F_GETLK, that remote pid
> will be <= 0.  This signifies that the pid is remote, and the locks API
> will forego translating that pid into the pid namespace of the local
> calling process.  Since local pids will never be larger than PID_MAX_LIMIT
> (which is currently defined as <= 4 million), but pid_t is an unsigned int,
> we should have plenty of room to represent remote pids with negative
> numbers if we assume that remote pid numbers are similarly limited.  If
> this is not the case, then we run the risk of having a remote pid returned
> for which there is also a corresponding local pid.  This is a problem we
> have now, but this patch should reduce the chances of that occurring, while
> also returning those remote pid numbers, for whatever that may be worth.
>
> This patch updates lustre, 9p, ceph, cifs, and dlm to negate the remote pid
> returned for F_GETLK lock requests.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  drivers/staging/lustre/lustre/ldlm/ldlm_flock.c | 2 +-
>  fs/9p/vfs_file.c                                | 2 +-
>  fs/ceph/locks.c                                 | 2 +-
>  fs/cifs/cifssmb.c                               | 2 +-
>  fs/dlm/plock.c                                  | 2 +-
>  5 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> index b7f28b39c7b3..abcbf075acc0 100644
> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> @@ -596,7 +596,7 @@ ldlm_flock_completion_ast(struct ldlm_lock *lock, __u64 flags, void *data)
>  		default:
>  			getlk->fl_type = F_UNLCK;
>  		}
> -		getlk->fl_pid = (pid_t)lock->l_policy_data.l_flock.pid;
> +		getlk->fl_pid = -(pid_t)lock->l_policy_data.l_flock.pid;
>  		getlk->fl_start = (loff_t)lock->l_policy_data.l_flock.start;
>  		getlk->fl_end = (loff_t)lock->l_policy_data.l_flock.end;
>  	} else {
> diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
> index 3de3b4a89d89..43c242e17132 100644
> --- a/fs/9p/vfs_file.c
> +++ b/fs/9p/vfs_file.c
> @@ -288,7 +288,7 @@ static int v9fs_file_getlock(struct file *filp, struct file_lock *fl)
>  			fl->fl_end = OFFSET_MAX;
>  		else
>  			fl->fl_end = glock.start + glock.length - 1;
> -		fl->fl_pid = glock.proc_id;
> +		fl->fl_pid = -glock.proc_id;
>  	}
>  	kfree(glock.client_id);
>  	return res;
> diff --git a/fs/ceph/locks.c b/fs/ceph/locks.c
> index 6806dbeaee19..0fd5c288ce4e 100644
> --- a/fs/ceph/locks.c
> +++ b/fs/ceph/locks.c
> @@ -79,7 +79,7 @@ static int ceph_lock_message(u8 lock_type, u16 operation, struct file *file,
>  	err = ceph_mdsc_do_request(mdsc, inode, req);
>  
>  	if (operation == CEPH_MDS_OP_GETFILELOCK) {
> -		fl->fl_pid = le64_to_cpu(req->r_reply_info.filelock_reply->pid);
> +		fl->fl_pid = -le64_to_cpu(req->r_reply_info.filelock_reply->pid);
>  		if (CEPH_LOCK_SHARED == req->r_reply_info.filelock_reply->type)
>  			fl->fl_type = F_RDLCK;
>  		else if (CEPH_LOCK_EXCL == req->r_reply_info.filelock_reply->type)
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index fbb0d4cbda41..cb367050f972 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -2515,7 +2515,7 @@ CIFSSMBPosixLock(const unsigned int xid, struct cifs_tcon *tcon,
>  			pLockData->fl_start = le64_to_cpu(parm_data->start);
>  			pLockData->fl_end = pLockData->fl_start +
>  					le64_to_cpu(parm_data->length) - 1;
> -			pLockData->fl_pid = le32_to_cpu(parm_data->pid);
> +			pLockData->fl_pid = -le32_to_cpu(parm_data->pid);
>  		}
>  	}
>  
> diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
> index d401425f602a..e631b1689228 100644
> --- a/fs/dlm/plock.c
> +++ b/fs/dlm/plock.c
> @@ -367,7 +367,7 @@ int dlm_posix_get(dlm_lockspace_t *lockspace, u64 number, struct file *file,
>  		locks_init_lock(fl);
>  		fl->fl_type = (op->info.ex) ? F_WRLCK : F_RDLCK;
>  		fl->fl_flags = FL_POSIX;
> -		fl->fl_pid = op->info.pid;
> +		fl->fl_pid = -op->info.pid;
>  		fl->fl_start = op->info.start;
>  		fl->fl_end = op->info.end;
>  		rv = 0;


I think this is probably a reasonable thing to do, given that we also
report OFD locks today with an l_pid of -1. The pid on any sort of
distributed fs is pretty meaningless anyway.

I think this all looks good. I'll plan to merge it for -next in a bit
and do some testing with it.

Thanks!
-- 
Jeff Layton <jlayton@poochiereds.net>