From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753473AbdF0TgV (ORCPT ); Tue, 27 Jun 2017 15:36:21 -0400 Received: from mail-qk0-f195.google.com ([209.85.220.195]:35983 "EHLO mail-qk0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753278AbdF0TgO (ORCPT ); Tue, 27 Jun 2017 15:36:14 -0400 Message-ID: <1498592170.4830.8.camel@poochiereds.net> Subject: Re: [PATCH 3/3] staging/lustre, 9p, ceph, cifs, dlm: negate remote pids for F_GETLK From: Jeff Layton To: Benjamin Coddington , Oleg Drokin , Andreas Dilger , James Simmons , Greg Kroah-Hartman , Eric Van Hensbergen , Ron Minnich , Latchesar Ionkov , "Yan, Zheng" , Sage Weil , Ilya Dryomov , Steve French , Christine Caulfield , David Teigland , Miklos Szeredi , Alexander Viro , "J. Bruce Fields" , Vitaly Fertman , "John L. Hammond" , Andriy Skulysh , Emoly Liu Cc: lustre-devel@lists.lustre.org, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org, v9fs-developer@lists.sourceforge.net, ceph-devel@vger.kernel.org, linux-cifs@vger.kernel.org, samba-technical@lists.samba.org, cluster-devel@redhat.com, linux-fsdevel@vger.kernel.org Date: Tue, 27 Jun 2017 15:36:10 -0400 In-Reply-To: <3154a78290017da7bbbcb920456b860dbfe9ba26.1498572504.git.bcodding@redhat.com> References: <3154a78290017da7bbbcb920456b860dbfe9ba26.1498572504.git.bcodding@redhat.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.24.3 (3.24.3-1.fc26) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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