[PATCH 0/2] lockref tidy ups + touch ups to it's usage by dcache

[PATCH 0/2] lockref tidy ups + touch ups to it's usage by dcache

[Mateusz Guzik]

The open-coded check for < 0 indicating a dead object uses information
it should not know, so to speak.

This is a preliminary clean up for a longer term goal of moving dcache
away from lockref into an approach which can atomic_add both ways
instead of suffering the cmpxchg loop.

Mateusz Guzik (2):
  lockref: tidy up dead count handling
  dcache: use lockref routines for dead count checks

 fs/ceph/dir.c           |  2 +-
 fs/dcache.c             | 16 ++++++++--------
 fs/erofs/zdata.c        |  4 ++--
 fs/gfs2/glock.c         |  6 +++---
 fs/gfs2/lock_dlm.c      |  6 +++---
 fs/gfs2/quota.c         |  4 ++--
 fs/xfs/xfs_buf.c        |  4 ++--
 fs/xfs/xfs_qm.c         |  4 ++--
 include/linux/lockref.h | 12 ++++++++++--
 lib/lockref.c           |  2 +-
 10 files changed, 34 insertions(+), 26 deletions(-)

-- 
2.48.1

[PATCH 1/2] lockref: tidy up dead count handling

[Mateusz Guzik]

1. put the dead val into a macro so that it can be used in other places
2. __lockref_is_dead():
- drop the __ suffix, this is not an internal routine
- drop the spurious cast, the value is already a signed int
- use READ_ONCE to prevent any compile shenanigans
3. provide lockref_is_dead_or_zero()

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
 fs/ceph/dir.c           |  2 +-
 fs/erofs/zdata.c        |  4 ++--
 fs/gfs2/glock.c         |  6 +++---
 fs/gfs2/lock_dlm.c      |  6 +++---
 fs/gfs2/quota.c         |  4 ++--
 fs/xfs/xfs_buf.c        |  4 ++--
 fs/xfs/xfs_qm.c         |  4 ++--
 include/linux/lockref.h | 12 ++++++++++--
 lib/lockref.c           |  2 +-
 9 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index b957149abe5a..f39856ff3ea1 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1667,7 +1667,7 @@ __dentry_leases_walk(struct ceph_mds_client *mdsc,
 		if (!spin_trylock(&dentry->d_lock))
 			continue;
 
-		if (__lockref_is_dead(&dentry->d_lockref)) {
+		if (lockref_is_dead(&dentry->d_lockref)) {
 			list_del_init(&di->lease_list);
 			goto next;
 		}
diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index 74520e910259..d022d1dff5a1 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -725,7 +725,7 @@ static bool z_erofs_get_pcluster(struct z_erofs_pcluster *pcl)
 		return true;
 
 	spin_lock(&pcl->lockref.lock);
-	if (__lockref_is_dead(&pcl->lockref)) {
+	if (lockref_is_dead(&pcl->lockref)) {
 		spin_unlock(&pcl->lockref.lock);
 		return false;
 	}
@@ -945,7 +945,7 @@ static void z_erofs_put_pcluster(struct erofs_sb_info *sbi,
 	if (lockref_put_or_lock(&pcl->lockref))
 		return;
 
-	DBG_BUGON(__lockref_is_dead(&pcl->lockref));
+	DBG_BUGON(lockref_is_dead(&pcl->lockref));
 	if (!--pcl->lockref.count) {
 		if (try_free && xa_trylock(&sbi->managed_pslots)) {
 			free = __erofs_try_to_release_pcluster(sbi, pcl);
diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index b8a144d3a73b..eaa2980051ed 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -2080,7 +2080,7 @@ static void clear_glock(struct gfs2_glock *gl)
 	gfs2_glock_remove_from_lru(gl);
 
 	spin_lock(&gl->gl_lockref.lock);
-	if (!__lockref_is_dead(&gl->gl_lockref)) {
+	if (!lockref_is_dead(&gl->gl_lockref)) {
 		gl->gl_lockref.count++;
 		if (gl->gl_state != LM_ST_UNLOCKED)
 			request_demote(gl, LM_ST_UNLOCKED, 0, false);
@@ -2115,7 +2115,7 @@ static void dump_glock_func(struct gfs2_glock *gl)
 static void withdraw_glock(struct gfs2_glock *gl)
 {
 	spin_lock(&gl->gl_lockref.lock);
-	if (!__lockref_is_dead(&gl->gl_lockref)) {
+	if (!lockref_is_dead(&gl->gl_lockref)) {
 		/*
 		 * We don't want to write back any more dirty data.  Unlock the
 		 * remaining inode and resource group glocks; this will cause
@@ -2483,7 +2483,7 @@ static void gfs2_glock_iter_next(struct gfs2_glock_iter *gi, loff_t n)
 				continue;
 			break;
 		} else {
-			if (__lockref_is_dead(&gl->gl_lockref))
+			if (lockref_is_dead(&gl->gl_lockref))
 				continue;
 			n--;
 		}
diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c
index 7828ad0b6f5a..cc901fb97da0 100644
--- a/fs/gfs2/lock_dlm.c
+++ b/fs/gfs2/lock_dlm.c
@@ -126,7 +126,7 @@ static void gdlm_ast(void *arg)
 	clear_bit(GLF_BLOCKING, &gl->gl_flags);
 
 	/* If the glock is dead, we only react to a dlm_unlock() reply. */
-	if (__lockref_is_dead(&gl->gl_lockref) &&
+	if (lockref_is_dead(&gl->gl_lockref) &&
 	    gl->gl_lksb.sb_status != -DLM_EUNLOCK)
 		return;
 
@@ -182,7 +182,7 @@ static void gdlm_bast(void *arg, int mode)
 {
 	struct gfs2_glock *gl = arg;
 
-	if (__lockref_is_dead(&gl->gl_lockref))
+	if (lockref_is_dead(&gl->gl_lockref))
 		return;
 
 	switch (mode) {
@@ -329,7 +329,7 @@ static void gdlm_put_lock(struct gfs2_glock *gl)
 	uint32_t flags = 0;
 	int error;
 
-	BUG_ON(!__lockref_is_dead(&gl->gl_lockref));
+	BUG_ON(!lockref_is_dead(&gl->gl_lockref));
 
 	if (test_bit(GLF_INITIAL, &gl->gl_flags)) {
 		gfs2_glock_free(gl);
diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index 91e9975d25e8..001c8b39ca55 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -342,7 +342,7 @@ static void qd_put(struct gfs2_quota_data *qd)
 	if (lockref_put_or_lock(&qd->qd_lockref))
 		return;
 
-	BUG_ON(__lockref_is_dead(&qd->qd_lockref));
+	BUG_ON(lockref_is_dead(&qd->qd_lockref));
 	sdp = qd->qd_sbd;
 	if (unlikely(!test_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags))) {
 		lockref_mark_dead(&qd->qd_lockref);
@@ -486,7 +486,7 @@ static bool qd_grab_sync(struct gfs2_sbd *sdp, struct gfs2_quota_data *qd,
 	    qd->qd_sync_gen >= sync_gen)
 		goto out;
 
-	if (__lockref_is_dead(&qd->qd_lockref))
+	if (lockref_is_dead(&qd->qd_lockref))
 		goto out;
 	qd->qd_lockref.count++;
 
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 21d67af781da..17b9d643e1a8 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -80,7 +80,7 @@ xfs_buf_stale(
 
 	spin_lock(&bp->b_lockref.lock);
 	atomic_set(&bp->b_lru_ref, 0);
-	if (!__lockref_is_dead(&bp->b_lockref))
+	if (!lockref_is_dead(&bp->b_lockref))
 		list_lru_del_obj(&bp->b_target->bt_lru, &bp->b_lru);
 	spin_unlock(&bp->b_lockref.lock);
 }
@@ -841,7 +841,7 @@ static void
 xfs_buf_destroy(
 	struct xfs_buf		*bp)
 {
-	ASSERT(__lockref_is_dead(&bp->b_lockref));
+	ASSERT(lockref_is_dead(&bp->b_lockref));
 	ASSERT(!(bp->b_flags & _XBF_DELWRI_Q));
 
 	if (bp->b_pag)
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 896b24f87ac9..99a82107b8e6 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -128,7 +128,7 @@ xfs_qm_dqpurge(
 	struct xfs_quotainfo	*qi = dqp->q_mount->m_quotainfo;
 
 	spin_lock(&dqp->q_lockref.lock);
-	if (dqp->q_lockref.count > 0 || __lockref_is_dead(&dqp->q_lockref)) {
+	if (dqp->q_lockref.count > 0 || lockref_is_dead(&dqp->q_lockref)) {
 		spin_unlock(&dqp->q_lockref.lock);
 		return -EAGAIN;
 	}
@@ -429,7 +429,7 @@ xfs_qm_dquot_isolate(
 	 * from the LRU, leave it for the freeing task to complete the freeing
 	 * process rather than risk it being free from under us here.
 	 */
-	if (__lockref_is_dead(&dqp->q_lockref))
+	if (lockref_is_dead(&dqp->q_lockref))
 		goto out_miss_unlock;
 
 	/*
diff --git a/include/linux/lockref.h b/include/linux/lockref.h
index 6ded24cdb4a8..ddfb7d3b8cec 100644
--- a/include/linux/lockref.h
+++ b/include/linux/lockref.h
@@ -34,6 +34,8 @@ struct lockref {
 	};
 };
 
+#define __LOCKREF_DEAD_VAL	-128
+
 /**
  * lockref_init - Initialize a lockref
  * @lockref: pointer to lockref structure
@@ -55,9 +57,15 @@ void lockref_mark_dead(struct lockref *lockref);
 bool lockref_get_not_dead(struct lockref *lockref);
 
 /* Must be called under spinlock for reliable results */
-static inline bool __lockref_is_dead(const struct lockref *l)
+static inline bool lockref_is_dead(const struct lockref *l)
+{
+	return (READ_ONCE(l->count) == __LOCKREF_DEAD_VAL);
+}
+
+static inline bool lockref_is_dead_or_zero(const struct lockref *l)
 {
-	return ((int)l->count < 0);
+	int count = READ_ONCE(l->count);
+	return (count == __LOCKREF_DEAD_VAL || count == 0);
 }
 
 #endif /* __LINUX_LOCKREF_H */
diff --git a/lib/lockref.c b/lib/lockref.c
index 5d8e3ef3860e..9b3dd688d8cd 100644
--- a/lib/lockref.c
+++ b/lib/lockref.c
@@ -131,7 +131,7 @@ EXPORT_SYMBOL(lockref_put_or_lock);
 void lockref_mark_dead(struct lockref *lockref)
 {
 	assert_spin_locked(&lockref->lock);
-	lockref->count = -128;
+	lockref->count = __LOCKREF_DEAD_VAL;
 }
 EXPORT_SYMBOL(lockref_mark_dead);
 
-- 
2.48.1

[PATCH 2/2] dcache: use lockref routines for dead count checks

[Mateusz Guzik]

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
 fs/dcache.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 3e9af9de7074..2aee85f3fbaa 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -434,7 +434,7 @@ static inline void __d_clear_type_and_inode(struct dentry *dentry)
 static void dentry_free(struct dentry *dentry)
 {
 	DENTRY_WARN_ONCE(d_really_is_positive(dentry), dentry);
-	DENTRY_WARN_ONCE(dentry->d_lockref.count >= 0, dentry);
+	DENTRY_WARN_ONCE(!lockref_is_dead(&dentry->d_lockref), dentry);
 	D_FLAG_VERIFY(dentry, 0);
 	if (unlikely(dname_external(dentry))) {
 		struct external_name *p = external_name(dentry);
@@ -782,7 +782,7 @@ static bool lock_for_kill(struct dentry *dentry)
  *
  * If @dentry is idle and remains such after we assemble the full
  * locking environment for eviction (see lock_for_kill() for details)
- * we mark it doomed (->d_lockref.count < 0) and proceed to detaching
+ * we mark it doomed (see lockref_mark_dead()) and proceed to detaching
  * it from any filesystem objects.  Otherwise we drop ->d_lock and
  * return %NULL.
  *
@@ -946,7 +946,7 @@ static inline bool fast_dput(struct dentry *dentry)
 	if (unlikely(ret < 0)) {
 		spin_lock(&dentry->d_lock);
 		rcu_read_unlock();
-		if (WARN_ON_ONCE(dentry->d_lockref.count <= 0)) {
+		if (WARN_ON_ONCE(lockref_is_dead_or_zero(&dentry->d_lockref))) {
 			spin_unlock(&dentry->d_lock);
 			return true;
 		}
@@ -1644,7 +1644,7 @@ static enum d_walk_ret select_collect(void *_data, struct dentry *dentry)
 	if (data->start == dentry)
 		goto out;
 
-	if (dentry->d_lockref.count <= 0) {
+	if (lockref_is_dead_or_zero(&dentry->d_lockref)) {
 		__move_to_shrink_list(dentry, &data->dispose);
 		data->found++;
 	}
@@ -1676,7 +1676,7 @@ static enum d_walk_ret select_collect2(void *_data, struct dentry *dentry)
 	if (data->start == dentry)
 		goto out;
 
-	if (dentry->d_lockref.count <= 0) {
+	if (lockref_is_dead_or_zero(&dentry->d_lockref)) {
 		if (!__move_to_shrink_list(dentry, &data->dispose)) {
 			/*
 			 * We need an enter RCU read-side critical area that
@@ -1747,7 +1747,7 @@ static void shrink_dcache_tree(struct dentry *parent, bool for_umount)
 			spin_lock(&v->d_lock);
 			rcu_read_unlock();
 
-			if (unlikely(v->d_lockref.count < 0)) {
+			if (unlikely(lockref_is_dead(&v->d_lockref))) {
 				// It's doomed; if it isn't dead yet, notify us
 				// once it becomes invisible to d_walk().
 				need_wait = d_add_waiter(v, &wait);
@@ -1823,7 +1823,7 @@ void shrink_dcache_for_umount(struct super_block *sb)
 		spin_unlock(&sb->s_roots_lock);
 		spin_lock(&dentry->d_lock);
 		rcu_read_unlock();
-		if (unlikely(dentry->d_lockref.count < 0)) {
+		if (unlikely(lockref_is_dead(&dentry->d_lockref))) {
 			struct completion_list wait;
 			bool need_wait = d_add_waiter(dentry, &wait);
 
@@ -2822,7 +2822,7 @@ struct dentry *d_alloc_parallel(struct dentry *parent,
 		spin_lock(&dentry->d_lock);
 		rcu_read_unlock();
 		/* now we can try to grab a reference */
-		if (unlikely(dentry->d_lockref.count < 0)) {
+		if (unlikely(lockref_is_dead(&dentry->d_lockref))) {
 			spin_unlock(&dentry->d_lock);
 			goto retry;
 		}
-- 
2.48.1