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

[Jan Kara]

On Fri 24-07-26 19:14:21, Mateusz Guzik wrote:
> 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@xxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  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
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR