[PATCH 0/4] locks: locks related cleanups for v4.1

[PATCH 0/4] locks: locks related cleanups for v4.1

[Jeff Layton]

Nothing terribly earth-shattering here. Just a few locking-related
cleanup patches that I'm planning to queue up for v4.1. The only one
that really warrants close attention is the 4th one that gets rid of the
i_lock protection around the i_flctx pointer.

One of the earlier patchsets that added the i_flctx field used cmpxchg
to assign it, but I ended up taking that out due to a problem that Sasha
Levin reported. I now think that I misunderstood the problem and that
using cmpxchg for that should be ok.

Jeff Layton (4):
  locks: don't allocate a lock context for an F_UNLCK request
  locks: change lm_get_owner and lm_put_owner prototypes
  locks: get rid of WE_CAN_BREAK_LSLK_NOW dead code
  locks: use cmpxchg to assign i_flctx pointer

 fs/locks.c          | 45 +++++++++++++++++++--------------------------
 fs/nfsd/nfs4state.c | 18 ++++++++++--------
 include/linux/fs.h  |  4 ++--
 3 files changed, 31 insertions(+), 36 deletions(-)

-- 
2.1.0

[PATCH 1/4] locks: don't allocate a lock context for an F_UNLCK request

[Jeff Layton]

In the event that we get an F_UNLCK request on an inode that has no lock
context, there is no reason to allocate one. Change
locks_get_lock_context to take a "type" pointer and avoid allocating a
new context if it's F_UNLCK.

Then, fix the callers to return appropriately if that function returns
NULL.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/locks.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 0915a3ead897..7be49ad1c902 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -203,11 +203,11 @@ static struct kmem_cache *flctx_cache __read_mostly;
 static struct kmem_cache *filelock_cache __read_mostly;
 
 static struct file_lock_context *
-locks_get_lock_context(struct inode *inode)
+locks_get_lock_context(struct inode *inode, int type)
 {
 	struct file_lock_context *new;
 
-	if (likely(inode->i_flctx))
+	if (likely(inode->i_flctx) || type == F_UNLCK)
 		goto out;
 
 	new = kmem_cache_alloc(flctx_cache, GFP_KERNEL);
@@ -877,9 +877,12 @@ static int flock_lock_file(struct file *filp, struct file_lock *request)
 	bool found = false;
 	LIST_HEAD(dispose);
 
-	ctx = locks_get_lock_context(inode);
-	if (!ctx)
-		return -ENOMEM;
+	ctx = locks_get_lock_context(inode, request->fl_type);
+	if (!ctx) {
+		if (request->fl_type != F_UNLCK)
+			return -ENOMEM;
+		return (request->fl_flags & FL_EXISTS) ? -ENOENT : 0;
+	}
 
 	if (!(request->fl_flags & FL_ACCESS) && (request->fl_type != F_UNLCK)) {
 		new_fl = locks_alloc_lock();
@@ -945,9 +948,9 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
 	bool added = false;
 	LIST_HEAD(dispose);
 
-	ctx = locks_get_lock_context(inode);
+	ctx = locks_get_lock_context(inode, request->fl_type);
 	if (!ctx)
-		return -ENOMEM;
+		return (request->fl_type == F_UNLCK) ? 0 : -ENOMEM;
 
 	/*
 	 * We may need two file_lock structures for this operation,
@@ -1610,9 +1613,9 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
 	lease = *flp;
 	trace_generic_add_lease(inode, lease);
 
-	ctx = locks_get_lock_context(inode);
+	ctx = locks_get_lock_context(inode, arg);
 	if (!ctx)
-		return -ENOMEM;
+		return (arg == F_UNLCK) ? 0 : -ENOMEM;
 
 	/*
 	 * In the delegation case we need mutual exclusion with
-- 
2.1.0

[PATCH 2/4] locks: change lm_get_owner and lm_put_owner prototypes

[Jeff Layton]

The current prototypes for these operations are somewhat awkward as they
deal with fl_owners but take struct file_lock arguments. In the future,
we'll want to be able to take references without necessarily dealing
with a struct file_lock.

Change them to take fl_owner_t arguments instead and have the callers
deal with assigning the values to the file_lock structs.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/locks.c          |  8 +++++---
 fs/nfsd/nfs4state.c | 18 ++++++++++--------
 include/linux/fs.h  |  4 ++--
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 7be49ad1c902..49d240874d4e 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -276,8 +276,10 @@ void locks_release_private(struct file_lock *fl)
 	}
 
 	if (fl->fl_lmops) {
-		if (fl->fl_lmops->lm_put_owner)
-			fl->fl_lmops->lm_put_owner(fl);
+		if (fl->fl_lmops->lm_put_owner) {
+			fl->fl_lmops->lm_put_owner(fl->fl_owner);
+			fl->fl_owner = NULL;
+		}
 		fl->fl_lmops = NULL;
 	}
 }
@@ -333,7 +335,7 @@ void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
 
 	if (fl->fl_lmops) {
 		if (fl->fl_lmops->lm_get_owner)
-			fl->fl_lmops->lm_get_owner(new, fl);
+			fl->fl_lmops->lm_get_owner(fl->fl_owner);
 	}
 }
 EXPORT_SYMBOL(locks_copy_conflock);
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d2f2c37dc2db..132a0270dff3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4932,20 +4932,22 @@ nfs4_transform_lock_offset(struct file_lock *lock)
 		lock->fl_end = OFFSET_MAX;
 }
 
-static void nfsd4_fl_get_owner(struct file_lock *dst, struct file_lock *src)
+static fl_owner_t
+nfsd4_fl_get_owner(fl_owner_t owner)
 {
-	struct nfs4_lockowner *lo = (struct nfs4_lockowner *)src->fl_owner;
-	dst->fl_owner = (fl_owner_t)lockowner(nfs4_get_stateowner(&lo->lo_owner));
+	struct nfs4_lockowner *lo = (struct nfs4_lockowner *)owner;
+
+	nfs4_get_stateowner(&lo->lo_owner);
+	return owner;
 }
 
-static void nfsd4_fl_put_owner(struct file_lock *fl)
+static void
+nfsd4_fl_put_owner(fl_owner_t owner)
 {
-	struct nfs4_lockowner *lo = (struct nfs4_lockowner *)fl->fl_owner;
+	struct nfs4_lockowner *lo = (struct nfs4_lockowner *)owner;
 
-	if (lo) {
+	if (lo)
 		nfs4_put_stateowner(&lo->lo_owner);
-		fl->fl_owner = NULL;
-	}
 }
 
 static const struct lock_manager_operations nfsd_posix_mng_ops  = {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b4d71b5e1ff2..fe1cef7bb677 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -892,8 +892,8 @@ struct file_lock_operations {
 struct lock_manager_operations {
 	int (*lm_compare_owner)(struct file_lock *, struct file_lock *);
 	unsigned long (*lm_owner_key)(struct file_lock *);
-	void (*lm_get_owner)(struct file_lock *, struct file_lock *);
-	void (*lm_put_owner)(struct file_lock *);
+	fl_owner_t (*lm_get_owner)(fl_owner_t);
+	void (*lm_put_owner)(fl_owner_t);
 	void (*lm_notify)(struct file_lock *);	/* unblock callback */
 	int (*lm_grant)(struct file_lock *, int);
 	bool (*lm_break)(struct file_lock *);
-- 
2.1.0

[PATCH 3/4] locks: get rid of WE_CAN_BREAK_LSLK_NOW dead code

[Jeff Layton]

As Bruce points out, there's no compelling reason to change /proc/locks
output at this point. If we did want to do this, then we'd almost
certainly want to introduce a new file to display this info (maybe via
debugfs?).

Let's remove the dead WE_CAN_BREAK_LSLK_NOW ifdef here and just plan to
stay with the legacy format.

Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
---
 fs/locks.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 49d240874d4e..4347f3dc17cc 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2565,15 +2565,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 			       : (fl->fl_type == F_WRLCK) ? "WRITE" : "READ ");
 	}
 	if (inode) {
-#ifdef WE_CAN_BREAK_LSLK_NOW
-		seq_printf(f, "%d %s:%ld ", fl_pid,
-				inode->i_sb->s_id, inode->i_ino);
-#else
-		/* userspace relies on this representation of dev_t ;-( */
+		/* userspace relies on this representation of dev_t */
 		seq_printf(f, "%d %02x:%02x:%ld ", fl_pid,
 				MAJOR(inode->i_sb->s_dev),
 				MINOR(inode->i_sb->s_dev), inode->i_ino);
-#endif
 	} else {
 		seq_printf(f, "%d <none>:0 ", fl_pid);
 	}
-- 
2.1.0

[PATCH 4/4] locks: use cmpxchg to assign i_flctx pointer

[Jeff Layton]

During the v3.20/v4.0 cycle, I had originally had the code manage the
inode->i_flctx pointer using a compare-and-swap operation instead of
the i_lock.

Sasha Levin though hit a problem while testing trinity that made me
believe that that wasn't safe. I now think though that I completely
misread the problem, even though it seemed like it went away when
we started using the i_lock to protect this pointer.

The issue was likely the same race that Kirill Shutemov hit while
testing the pre-rc1 v4.0 kernel and that Linus spotted. Due to the
way that the spinlock was dropped in the middle of flock_lock_file,
you could end up with multiple flock locks for the same struct file
on the inode.

Reinstate the use of a CAS operation to assign this pointer since it's
likely to be more efficient and gets the i_lock completely out of the
file locking business.

Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
---
 fs/locks.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 4347f3dc17cc..22c0785c00ed 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -223,14 +223,7 @@ locks_get_lock_context(struct inode *inode, int type)
 	 * Assign the pointer if it's not already assigned. If it is, then
 	 * free the context we just allocated.
 	 */
-	spin_lock(&inode->i_lock);
-	if (likely(!inode->i_flctx)) {
-		inode->i_flctx = new;
-		new = NULL;
-	}
-	spin_unlock(&inode->i_lock);
-
-	if (new)
+	if (cmpxchg(&inode->i_flctx, NULL, new))
 		kmem_cache_free(flctx_cache, new);
 out:
 	return inode->i_flctx;
-- 
2.1.0