lockref cleanups

lockref cleanups

[Christoph Hellwig]

Hi all,

this series has a bunch of cosmetic cleanups for the lockref code I came up
with when reading the code in preparation of adding a new user of it.

Diffstat:
 fs/dcache.c             |    3 --
 fs/erofs/zdata.c        |    3 --
 fs/gfs2/quota.c         |    3 --
 include/linux/lockref.h |   26 ++++++++++++++------
 lib/lockref.c           |   60 ++++++++++++------------------------------------
 5 files changed, 36 insertions(+), 59 deletions(-)

[PATCH 1/8] lockref: remove lockref_put_not_zero

[Christoph Hellwig]

lockref_put_not_zero is not used anywhere, and unless I'm missing
something didn't end up being used used at all.  Remove it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/lockref.h |  1 -
 lib/lockref.c           | 28 ----------------------------
 2 files changed, 29 deletions(-)

diff --git a/include/linux/lockref.h b/include/linux/lockref.h
index c3a1f78bc884..e5aa0347f274 100644
--- a/include/linux/lockref.h
+++ b/include/linux/lockref.h
@@ -37,7 +37,6 @@ struct lockref {
 extern void lockref_get(struct lockref *);
 extern int lockref_put_return(struct lockref *);
 extern int lockref_get_not_zero(struct lockref *);
-extern int lockref_put_not_zero(struct lockref *);
 extern int lockref_put_or_lock(struct lockref *);
 
 extern void lockref_mark_dead(struct lockref *);
diff --git a/lib/lockref.c b/lib/lockref.c
index 2afe4c5d8919..a68192c979b3 100644
--- a/lib/lockref.c
+++ b/lib/lockref.c
@@ -81,34 +81,6 @@ int lockref_get_not_zero(struct lockref *lockref)
 }
 EXPORT_SYMBOL(lockref_get_not_zero);
 
-/**
- * lockref_put_not_zero - Decrements count unless count <= 1 before decrement
- * @lockref: pointer to lockref structure
- * Return: 1 if count updated successfully or 0 if count would become zero
- */
-int lockref_put_not_zero(struct lockref *lockref)
-{
-	int retval;
-
-	CMPXCHG_LOOP(
-		new.count--;
-		if (old.count <= 1)
-			return 0;
-	,
-		return 1;
-	);
-
-	spin_lock(&lockref->lock);
-	retval = 0;
-	if (lockref->count > 1) {
-		lockref->count--;
-		retval = 1;
-	}
-	spin_unlock(&lockref->lock);
-	return retval;
-}
-EXPORT_SYMBOL(lockref_put_not_zero);
-
 /**
  * lockref_put_return - Decrement reference count if possible
  * @lockref: pointer to lockref structure
-- 
2.45.2

[PATCH 2/8] lockref: improve the lockref_get_not_zero description

[Christoph Hellwig]

lockref_put_return returns exactly -1 and not "an error" when the lockref
is dead or locked.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 lib/lockref.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/lockref.c b/lib/lockref.c
index a68192c979b3..b1b042a9a6c8 100644
--- a/lib/lockref.c
+++ b/lib/lockref.c
@@ -86,7 +86,7 @@ EXPORT_SYMBOL(lockref_get_not_zero);
  * @lockref: pointer to lockref structure
  *
  * Decrement the reference count and return the new value.
- * If the lockref was dead or locked, return an error.
+ * If the lockref was dead or locked, return -1.
  */
 int lockref_put_return(struct lockref *lockref)
 {
-- 
2.45.2

Re: [PATCH 2/8] lockref: improve the lockref_get_not_zero description

[Andreas Gruenbacher]

On Wed, Jan 15, 2025 at 10:56 AM Christoph Hellwig <hch@lst.de> wrote:
> lockref_put_return returns exactly -1 and not "an error" when the lockref
> is dead or locked.

The function name in the subject needs fixing.

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  lib/lockref.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/lockref.c b/lib/lockref.c
> index a68192c979b3..b1b042a9a6c8 100644
> --- a/lib/lockref.c
> +++ b/lib/lockref.c
> @@ -86,7 +86,7 @@ EXPORT_SYMBOL(lockref_get_not_zero);
>   * @lockref: pointer to lockref structure
>   *
>   * Decrement the reference count and return the new value.
> - * If the lockref was dead or locked, return an error.
> + * If the lockref was dead or locked, return -1.
>   */
>  int lockref_put_return(struct lockref *lockref)
>  {
> --
> 2.45.2

Thanks,
Andreas

[PATCH 3/8] lockref: use bool for false/true returns

[Christoph Hellwig]

Replace int used as bool with the actual bool type for return values that
can only be true or false.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/lockref.h |  6 +++---
 lib/lockref.c           | 30 ++++++++++++++----------------
 2 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/include/linux/lockref.h b/include/linux/lockref.h
index e5aa0347f274..3d770e1bdbad 100644
--- a/include/linux/lockref.h
+++ b/include/linux/lockref.h
@@ -36,11 +36,11 @@ struct lockref {
 
 extern void lockref_get(struct lockref *);
 extern int lockref_put_return(struct lockref *);
-extern int lockref_get_not_zero(struct lockref *);
-extern int lockref_put_or_lock(struct lockref *);
+bool lockref_get_not_zero(struct lockref *lockref);
+bool lockref_put_or_lock(struct lockref *lockref);
 
 extern void lockref_mark_dead(struct lockref *);
-extern int lockref_get_not_dead(struct 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)
diff --git a/lib/lockref.c b/lib/lockref.c
index b1b042a9a6c8..5d8e3ef3860e 100644
--- a/lib/lockref.c
+++ b/lib/lockref.c
@@ -58,23 +58,22 @@ EXPORT_SYMBOL(lockref_get);
  * @lockref: pointer to lockref structure
  * Return: 1 if count updated successfully or 0 if count was zero
  */
-int lockref_get_not_zero(struct lockref *lockref)
+bool lockref_get_not_zero(struct lockref *lockref)
 {
-	int retval;
+	bool retval = false;
 
 	CMPXCHG_LOOP(
 		new.count++;
 		if (old.count <= 0)
-			return 0;
+			return false;
 	,
-		return 1;
+		return true;
 	);
 
 	spin_lock(&lockref->lock);
-	retval = 0;
 	if (lockref->count > 0) {
 		lockref->count++;
-		retval = 1;
+		retval = true;
 	}
 	spin_unlock(&lockref->lock);
 	return retval;
@@ -106,22 +105,22 @@ EXPORT_SYMBOL(lockref_put_return);
  * @lockref: pointer to lockref structure
  * Return: 1 if count updated successfully or 0 if count <= 1 and lock taken
  */
-int lockref_put_or_lock(struct lockref *lockref)
+bool lockref_put_or_lock(struct lockref *lockref)
 {
 	CMPXCHG_LOOP(
 		new.count--;
 		if (old.count <= 1)
 			break;
 	,
-		return 1;
+		return true;
 	);
 
 	spin_lock(&lockref->lock);
 	if (lockref->count <= 1)
-		return 0;
+		return false;
 	lockref->count--;
 	spin_unlock(&lockref->lock);
-	return 1;
+	return true;
 }
 EXPORT_SYMBOL(lockref_put_or_lock);
 
@@ -141,23 +140,22 @@ EXPORT_SYMBOL(lockref_mark_dead);
  * @lockref: pointer to lockref structure
  * Return: 1 if count updated successfully or 0 if lockref was dead
  */
-int lockref_get_not_dead(struct lockref *lockref)
+bool lockref_get_not_dead(struct lockref *lockref)
 {
-	int retval;
+	bool retval = false;
 
 	CMPXCHG_LOOP(
 		new.count++;
 		if (old.count < 0)
-			return 0;
+			return false;
 	,
-		return 1;
+		return true;
 	);
 
 	spin_lock(&lockref->lock);
-	retval = 0;
 	if (lockref->count >= 0) {
 		lockref->count++;
-		retval = 1;
+		retval = true;
 	}
 	spin_unlock(&lockref->lock);
 	return retval;
-- 
2.45.2

Re: [PATCH 3/8] lockref: use bool for false/true returns

[Mateusz Guzik]

On Wed, Jan 15, 2025 at 10:46:39AM +0100, Christoph Hellwig wrote:
> Replace int used as bool with the actual bool type for return values that
> can only be true or false.
> 
[snip]

> -int lockref_get_not_zero(struct lockref *lockref)
> +bool lockref_get_not_zero(struct lockref *lockref)
>  {
> -	int retval;
> +	bool retval = false;
>  
>  	CMPXCHG_LOOP(
>  		new.count++;
>  		if (old.count <= 0)
> -			return 0;
> +			return false;
>  	,
> -		return 1;
> +		return true;
>  	);
>  
>  	spin_lock(&lockref->lock);
> -	retval = 0;
>  	if (lockref->count > 0) {
>  		lockref->count++;
> -		retval = 1;
> +		retval = true;
>  	}
>  	spin_unlock(&lockref->lock);
>  	return retval;

While this looks perfectly sane, it worsens codegen around the atomic
on x86-64 at least with gcc 13.3.0. It bisected to this commit and
confirmed top of next-20250318 with this reverted undoes it.

The expected state looks like this:
       f0 48 0f b1 13          lock cmpxchg %rdx,(%rbx)
       75 0e                   jne    ffffffff81b33626 <lockref_get_not_dead+0x46>

However, with the above patch I see:
       f0 48 0f b1 13          lock cmpxchg %rdx,(%rbx)
       40 0f 94 c5             sete   %bpl
       40 84 ed                test   %bpl,%bpl
       74 09                   je     ffffffff81b33636 <lockref_get_not_dead+0x46>

This is not the end of the world, but also really does not need to be
there.

Given that the patch is merely a cosmetic change, I would suggest I gets
dropped.

Re: [PATCH 3/8] lockref: use bool for false/true returns

[Mateusz Guzik]

On Tue, Mar 18, 2025 at 4:25 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> On Wed, Jan 15, 2025 at 10:46:39AM +0100, Christoph Hellwig wrote:
> > Replace int used as bool with the actual bool type for return values that
> > can only be true or false.
> >
> [snip]
>
> > -int lockref_get_not_zero(struct lockref *lockref)
> > +bool lockref_get_not_zero(struct lockref *lockref)
> >  {
> > -     int retval;
> > +     bool retval = false;
> >
> >       CMPXCHG_LOOP(
> >               new.count++;
> >               if (old.count <= 0)
> > -                     return 0;
> > +                     return false;
> >       ,
> > -             return 1;
> > +             return true;
> >       );
> >
> >       spin_lock(&lockref->lock);
> > -     retval = 0;
> >       if (lockref->count > 0) {
> >               lockref->count++;
> > -             retval = 1;
> > +             retval = true;
> >       }
> >       spin_unlock(&lockref->lock);
> >       return retval;
>
> While this looks perfectly sane, it worsens codegen around the atomic
> on x86-64 at least with gcc 13.3.0. It bisected to this commit and
> confirmed top of next-20250318 with this reverted undoes it.
>
> The expected state looks like this:
>        f0 48 0f b1 13          lock cmpxchg %rdx,(%rbx)
>        75 0e                   jne    ffffffff81b33626 <lockref_get_not_dead+0x46>
>
> However, with the above patch I see:
>        f0 48 0f b1 13          lock cmpxchg %rdx,(%rbx)
>        40 0f 94 c5             sete   %bpl
>        40 84 ed                test   %bpl,%bpl
>        74 09                   je     ffffffff81b33636 <lockref_get_not_dead+0x46>
>
> This is not the end of the world, but also really does not need to be
> there.
>
> Given that the patch is merely a cosmetic change, I would suggest I gets
> dropped.

fwiw I confirmed clang does *not* have the problem, I don't know about gcc 14.

Maybe I'll get around to testing it, but first I'm gonna need to carve
out the custom asm into a standalone testcase.

Regardless, 13 suffering the problem is imo a good enough reason to
whack the change.

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH 3/8] lockref: use bool for false/true returns

[Christoph Hellwig]

On Tue, Mar 18, 2025 at 04:51:27PM +0100, Mateusz Guzik wrote:
> fwiw I confirmed clang does *not* have the problem, I don't know about gcc 14.
> 
> Maybe I'll get around to testing it, but first I'm gonna need to carve
> out the custom asm into a standalone testcase.
> 
> Regardless, 13 suffering the problem is imo a good enough reason to
> whack the change.

Reverting a change because a specific compiler generates sligtly worse
code without even showing it has any real life impact feels like I'm
missing something important.

Re: [PATCH 3/8] lockref: use bool for false/true returns

[Mateusz Guzik]

On Wed, Mar 19, 2025 at 7:29 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Mar 18, 2025 at 04:51:27PM +0100, Mateusz Guzik wrote:
> > fwiw I confirmed clang does *not* have the problem, I don't know about gcc 14.
> >
> > Maybe I'll get around to testing it, but first I'm gonna need to carve
> > out the custom asm into a standalone testcase.
> >
> > Regardless, 13 suffering the problem is imo a good enough reason to
> > whack the change.
>
> Reverting a change because a specific compiler generates sligtly worse
> code without even showing it has any real life impact feels like I'm
> missing something important.
>

The change is cosmetic and has an unintended impact on code gen, which
imo already puts a question mark on it.

Neither the change itself nor the resulting impact are of note and in
that case I would err on just not including it for the time being, but
that's just my $0.03.
-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH 3/8] lockref: use bool for false/true returns

[Christoph Hellwig]

On Wed, Mar 19, 2025 at 01:04:14PM +0100, Mateusz Guzik wrote:
> The change is cosmetic and has an unintended impact on code gen, which
> imo already puts a question mark on it.
> 
> Neither the change itself nor the resulting impact are of note and in
> that case I would err on just not including it for the time being, but
> that's just my $0.03.

Im not sure how you came up with that opinion but as you might have
guessed I don't agree at all.  Having the proper types is more than
just cosmetic, on the other hand the code generation change you
see can easily be argued as cosmetic.  You've also completely ignored
the request to show any indication that it actually matters the
slightest.

[PATCH 4/8] lockref: drop superfluous externs

[Christoph Hellwig]

Drop the superfluous externs from the remaining prototypes in lockref.h.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/lockref.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/lockref.h b/include/linux/lockref.h
index 3d770e1bdbad..f821f46e9fb4 100644
--- a/include/linux/lockref.h
+++ b/include/linux/lockref.h
@@ -34,12 +34,12 @@ struct lockref {
 	};
 };
 
-extern void lockref_get(struct lockref *);
-extern int lockref_put_return(struct lockref *);
+void lockref_get(struct lockref *lockref);
+int lockref_put_return(struct lockref *lockref);
 bool lockref_get_not_zero(struct lockref *lockref);
 bool lockref_put_or_lock(struct lockref *lockref);
 
-extern void lockref_mark_dead(struct lockref *);
+void lockref_mark_dead(struct lockref *lockref);
 bool lockref_get_not_dead(struct lockref *lockref);
 
 /* Must be called under spinlock for reliable results */
-- 
2.45.2

[PATCH 5/8] lockref: add a lockref_init helper

[Christoph Hellwig]

Add a helper to initialize the lockdep, that is initialize the spinlock
and set a value.  Having to open code them isn't a big deal, but having
an initializer feels right for a proper primitive.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/lockref.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/lockref.h b/include/linux/lockref.h
index f821f46e9fb4..c39f119659ba 100644
--- a/include/linux/lockref.h
+++ b/include/linux/lockref.h
@@ -34,6 +34,17 @@ struct lockref {
 	};
 };
 
+/**
+ * lockref_init - Initialize a lockref
+ * @lockref: pointer to lockref structure
+ * @count: initial count
+ */
+static inline void lockref_init(struct lockref *lockref, unsigned int count)
+{
+	spin_lock_init(&lockref->lock);
+	lockref->count = count;
+}
+
 void lockref_get(struct lockref *lockref);
 int lockref_put_return(struct lockref *lockref);
 bool lockref_get_not_zero(struct lockref *lockref);
-- 
2.45.2

[PATCH 6/8] dcache: use lockref_init for d_lockref

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/dcache.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index b4d5e9e1e43d..1a01d7a6a7a9 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1681,9 +1681,8 @@ static struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
 	/* Make sure we always see the terminating NUL character */
 	smp_store_release(&dentry->d_name.name, dname); /* ^^^ */
 
-	dentry->d_lockref.count = 1;
 	dentry->d_flags = 0;
-	spin_lock_init(&dentry->d_lock);
+	lockref_init(&dentry->d_lockref, 1);
 	seqcount_spinlock_init(&dentry->d_seq, &dentry->d_lock);
 	dentry->d_inode = NULL;
 	dentry->d_parent = dentry;
-- 
2.45.2

Re: [PATCH 6/8] dcache: use lockref_init for d_lockref

[Dave Chinner]

On Wed, Jan 15, 2025 at 10:46:42AM +0100, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/dcache.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index b4d5e9e1e43d..1a01d7a6a7a9 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1681,9 +1681,8 @@ static struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
>  	/* Make sure we always see the terminating NUL character */
>  	smp_store_release(&dentry->d_name.name, dname); /* ^^^ */
>  
> -	dentry->d_lockref.count = 1;
>  	dentry->d_flags = 0;
> -	spin_lock_init(&dentry->d_lock);

Looks wrong -  dentry->d_lock is not part of dentry->d_lockref...

-Dave.

> +	lockref_init(&dentry->d_lockref, 1);
>  	seqcount_spinlock_init(&dentry->d_seq, &dentry->d_lock);
>  	dentry->d_inode = NULL;
>  	dentry->d_parent = dentry;
> -- 
> 2.45.2
> 
> 
> 

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 6/8] dcache: use lockref_init for d_lockref

[Al Viro]

On Thu, Jan 16, 2025 at 07:13:23AM +1100, Dave Chinner wrote:
> On Wed, Jan 15, 2025 at 10:46:42AM +0100, Christoph Hellwig wrote:
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/dcache.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index b4d5e9e1e43d..1a01d7a6a7a9 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -1681,9 +1681,8 @@ static struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
> >  	/* Make sure we always see the terminating NUL character */
> >  	smp_store_release(&dentry->d_name.name, dname); /* ^^^ */
> >  
> > -	dentry->d_lockref.count = 1;
> >  	dentry->d_flags = 0;
> > -	spin_lock_init(&dentry->d_lock);
> 
> Looks wrong -  dentry->d_lock is not part of dentry->d_lockref...

include/linux/dcache.h:80:#define d_lock  d_lockref.lock

Re: [PATCH 6/8] dcache: use lockref_init for d_lockref

[Dave Chinner]

On Wed, Jan 15, 2025 at 08:30:24PM +0000, Al Viro wrote:
> On Thu, Jan 16, 2025 at 07:13:23AM +1100, Dave Chinner wrote:
> > On Wed, Jan 15, 2025 at 10:46:42AM +0100, Christoph Hellwig wrote:
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >  fs/dcache.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/dcache.c b/fs/dcache.c
> > > index b4d5e9e1e43d..1a01d7a6a7a9 100644
> > > --- a/fs/dcache.c
> > > +++ b/fs/dcache.c
> > > @@ -1681,9 +1681,8 @@ static struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
> > >  	/* Make sure we always see the terminating NUL character */
> > >  	smp_store_release(&dentry->d_name.name, dname); /* ^^^ */
> > >  
> > > -	dentry->d_lockref.count = 1;
> > >  	dentry->d_flags = 0;
> > > -	spin_lock_init(&dentry->d_lock);
> > 
> > Looks wrong -  dentry->d_lock is not part of dentry->d_lockref...
> 
> include/linux/dcache.h:80:#define d_lock  d_lockref.lock

Ah, I missed that subtlety (obviously). Carry on!

-Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 7/8] erofs: use lockref_init for pcl->lockref

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/erofs/zdata.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index 19ef4ff2a134..254f6ad2c336 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -747,8 +747,7 @@ static int z_erofs_register_pcluster(struct z_erofs_decompress_frontend *fe)
 	if (IS_ERR(pcl))
 		return PTR_ERR(pcl);
 
-	spin_lock_init(&pcl->lockref.lock);
-	pcl->lockref.count = 1;		/* one ref for this request */
+	lockref_init(&pcl->lockref, 1); /* one ref for this request */
 	pcl->algorithmformat = map->m_algorithmformat;
 	pcl->length = 0;
 	pcl->partial = true;
-- 
2.45.2

Re: [PATCH 7/8] erofs: use lockref_init for pcl->lockref

[Gao Xiang]

On 2025/1/15 17:46, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Thanks,
Gao Xiang

[PATCH 8/8] gfs2: use lockref_init for qd_lockref

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/gfs2/quota.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index 72b48f6f5561..58bc5013ca49 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -236,8 +236,7 @@ static struct gfs2_quota_data *qd_alloc(unsigned hash, struct gfs2_sbd *sdp, str
 		return NULL;
 
 	qd->qd_sbd = sdp;
-	qd->qd_lockref.count = 0;
-	spin_lock_init(&qd->qd_lockref.lock);
+	lockref_init(&qd->qd_lockref, 0);
 	qd->qd_id = qid;
 	qd->qd_slot = -1;
 	INIT_LIST_HEAD(&qd->qd_lru);
-- 
2.45.2

Re: [PATCH 8/8] gfs2: use lockref_init for qd_lockref

[Andreas Gruenbacher]

On Wed, Jan 15, 2025 at 10:56 AM Christoph Hellwig <hch@lst.de> wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/gfs2/quota.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> index 72b48f6f5561..58bc5013ca49 100644
> --- a/fs/gfs2/quota.c
> +++ b/fs/gfs2/quota.c
> @@ -236,8 +236,7 @@ static struct gfs2_quota_data *qd_alloc(unsigned hash, struct gfs2_sbd *sdp, str
>                 return NULL;
>
>         qd->qd_sbd = sdp;
> -       qd->qd_lockref.count = 0;
> -       spin_lock_init(&qd->qd_lockref.lock);
> +       lockref_init(&qd->qd_lockref, 0);

Hmm, initializing count to 0 seems to be the odd case and it's fairly
simple to change gfs2 to work with an initial value of 1. I wonder if
lockref_init() should really have a count argument.

>         qd->qd_id = qid;
>         qd->qd_slot = -1;
>         INIT_LIST_HEAD(&qd->qd_lru);
> --
> 2.45.2

Thanks,
Andreas

Re: [PATCH 8/8] gfs2: use lockref_init for qd_lockref

[Christoph Hellwig]

On Wed, Jan 15, 2025 at 02:35:03PM +0100, Andreas Gruenbacher wrote:
> > +++ b/fs/gfs2/quota.c
> > @@ -236,8 +236,7 @@ static struct gfs2_quota_data *qd_alloc(unsigned hash, struct gfs2_sbd *sdp, str
> >                 return NULL;
> >
> >         qd->qd_sbd = sdp;
> > -       qd->qd_lockref.count = 0;
> > -       spin_lock_init(&qd->qd_lockref.lock);
> > +       lockref_init(&qd->qd_lockref, 0);
> 
> Hmm, initializing count to 0 seems to be the odd case and it's fairly
> simple to change gfs2 to work with an initial value of 1. I wonder if
> lockref_init() should really have a count argument.

Well, if you can fix it to start with 1 we could start out with 1
as the default.  FYI, I also didn't touch the other gfs2 lockref
because it initialize the lock in the slab init_once callback and
the count on every initialization.

Re: [PATCH 8/8] gfs2: use lockref_init for qd_lockref

[Andreas Gruenbacher]

On Thu, 16 Jan 2025 05:32:26 +0100, Christoph Hellwig <hch@lst.de> wrote:
> Well, if you can fix it to start with 1 we could start out with 1
> as the default.  FYI, I also didn't touch the other gfs2 lockref
> because it initialize the lock in the slab init_once callback and
> the count on every initialization.

Sure, can you add the below patch before the lockref_init conversion?

Thanks,
Andreas

--

gfs2: Prepare for converting to lockref_init

First, move initializing the glock lockref spin lock from
gfs2_init_glock_once() to gfs2_glock_get().

Second, in qd_alloc(), initialize the lockref count to 1 to cover the
common case.  Compensate for that in gfs2_quota_init() by adjusting the
count back down to 0; this case occurs only when mounting the filesystem
rw.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/glock.c | 3 ++-
 fs/gfs2/main.c  | 1 -
 fs/gfs2/quota.c | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 8c4c1f871a88..22ff77cdd827 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -1201,8 +1201,9 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number,
 	if (glops->go_instantiate)
 		gl->gl_flags |= BIT(GLF_INSTANTIATE_NEEDED);
 	gl->gl_name = name;
-	lockdep_set_subclass(&gl->gl_lockref.lock, glops->go_subclass);
 	gl->gl_lockref.count = 1;
+	spin_lock_init(&gl->gl_lockref.lock);
+	lockdep_set_subclass(&gl->gl_lockref.lock, glops->go_subclass);
 	gl->gl_state = LM_ST_UNLOCKED;
 	gl->gl_target = LM_ST_UNLOCKED;
 	gl->gl_demote_state = LM_ST_EXCLUSIVE;
diff --git a/fs/gfs2/main.c b/fs/gfs2/main.c
index 04cadc02e5a6..0727f60ad028 100644
--- a/fs/gfs2/main.c
+++ b/fs/gfs2/main.c
@@ -51,7 +51,6 @@ static void gfs2_init_glock_once(void *foo)
 {
 	struct gfs2_glock *gl = foo;
 
-	spin_lock_init(&gl->gl_lockref.lock);
 	INIT_LIST_HEAD(&gl->gl_holders);
 	INIT_LIST_HEAD(&gl->gl_lru);
 	INIT_LIST_HEAD(&gl->gl_ail_list);
diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index 72b48f6f5561..df78eb8f35f9 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -236,7 +236,7 @@ static struct gfs2_quota_data *qd_alloc(unsigned hash, struct gfs2_sbd *sdp, str
 		return NULL;
 
 	qd->qd_sbd = sdp;
-	qd->qd_lockref.count = 0;
+	qd->qd_lockref.count = 1;
 	spin_lock_init(&qd->qd_lockref.lock);
 	qd->qd_id = qid;
 	qd->qd_slot = -1;
@@ -298,7 +298,6 @@ static int qd_get(struct gfs2_sbd *sdp, struct kqid qid,
 	spin_lock_bucket(hash);
 	*qdp = qd = gfs2_qd_search_bucket(hash, sdp, qid);
 	if (qd == NULL) {
-		new_qd->qd_lockref.count++;
 		*qdp = new_qd;
 		list_add(&new_qd->qd_list, &sdp->sd_quota_list);
 		hlist_bl_add_head_rcu(&new_qd->qd_hlist, &qd_hash_table[hash]);
@@ -1451,6 +1450,7 @@ int gfs2_quota_init(struct gfs2_sbd *sdp)
 			if (qd == NULL)
 				goto fail_brelse;
 
+			qd->qd_lockref.count = 0;
 			set_bit(QDF_CHANGE, &qd->qd_flags);
 			qd->qd_change = qc_change;
 			qd->qd_slot = slot;
-- 
2.47.1

Re: [PATCH 8/8] gfs2: use lockref_init for qd_lockref

[Christian Brauner]

On Fri, Jan 17, 2025 at 05:03:51PM +0100, Andreas Gruenbacher wrote:
> On Thu, 16 Jan 2025 05:32:26 +0100, Christoph Hellwig <hch@lst.de> wrote:
> > Well, if you can fix it to start with 1 we could start out with 1
> > as the default.  FYI, I also didn't touch the other gfs2 lockref
> > because it initialize the lock in the slab init_once callback and
> > the count on every initialization.
> 
> Sure, can you add the below patch before the lockref_init conversion?
> 
> Thanks,
> Andreas
> 
> --
> 
> gfs2: Prepare for converting to lockref_init
> 
> First, move initializing the glock lockref spin lock from
> gfs2_init_glock_once() to gfs2_glock_get().
> 
> Second, in qd_alloc(), initialize the lockref count to 1 to cover the
> common case.  Compensate for that in gfs2_quota_init() by adjusting the
> count back down to 0; this case occurs only when mounting the filesystem
> rw.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---

Can you send this as a proper separae patch, please?

Re: [PATCH 8/8] gfs2: use lockref_init for qd_lockref

[Andreas Gruenbacher]

On Mon, Jan 20, 2025 at 4:25 PM Christian Brauner <brauner@kernel.org> wrote:
> On Fri, Jan 17, 2025 at 05:03:51PM +0100, Andreas Gruenbacher wrote:
> > On Thu, 16 Jan 2025 05:32:26 +0100, Christoph Hellwig <hch@lst.de> wrote:
> > > Well, if you can fix it to start with 1 we could start out with 1
> > > as the default.  FYI, I also didn't touch the other gfs2 lockref
> > > because it initialize the lock in the slab init_once callback and
> > > the count on every initialization.
> >
> > Sure, can you add the below patch before the lockref_init conversion?
> >
> > Thanks,
> > Andreas
> >
> > --
> >
> > gfs2: Prepare for converting to lockref_init
> >
> > First, move initializing the glock lockref spin lock from
> > gfs2_init_glock_once() to gfs2_glock_get().
> >
> > Second, in qd_alloc(), initialize the lockref count to 1 to cover the
> > common case.  Compensate for that in gfs2_quota_init() by adjusting the
> > count back down to 0; this case occurs only when mounting the filesystem
> > rw.
> >
> > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> > ---
>
> Can you send this as a proper separae patch, please?

Do you want this particular version which applies before Christoph's
patches or something that applies on top of Christoph's patches?

Thanks,
Andreas

Re: [PATCH 8/8] gfs2: use lockref_init for qd_lockref

[Christian Brauner]

On Mon, Jan 20, 2025 at 04:44:59PM +0100, Andreas Gruenbacher wrote:
> On Mon, Jan 20, 2025 at 4:25 PM Christian Brauner <brauner@kernel.org> wrote:
> > On Fri, Jan 17, 2025 at 05:03:51PM +0100, Andreas Gruenbacher wrote:
> > > On Thu, 16 Jan 2025 05:32:26 +0100, Christoph Hellwig <hch@lst.de> wrote:
> > > > Well, if you can fix it to start with 1 we could start out with 1
> > > > as the default.  FYI, I also didn't touch the other gfs2 lockref
> > > > because it initialize the lock in the slab init_once callback and
> > > > the count on every initialization.
> > >
> > > Sure, can you add the below patch before the lockref_init conversion?
> > >
> > > Thanks,
> > > Andreas
> > >
> > > --
> > >
> > > gfs2: Prepare for converting to lockref_init
> > >
> > > First, move initializing the glock lockref spin lock from
> > > gfs2_init_glock_once() to gfs2_glock_get().
> > >
> > > Second, in qd_alloc(), initialize the lockref count to 1 to cover the
> > > common case.  Compensate for that in gfs2_quota_init() by adjusting the
> > > count back down to 0; this case occurs only when mounting the filesystem
> > > rw.
> > >
> > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> > > ---
> >
> > Can you send this as a proper separae patch, please?
> 
> Do you want this particular version which applies before Christoph's
> patches or something that applies on top of Christoph's patches?

On top, if you don't mind. Thank you!

Re: lockref cleanups

[Christian Brauner]

On Wed, 15 Jan 2025 10:46:36 +0100, Christoph Hellwig wrote:
> this series has a bunch of cosmetic cleanups for the lockref code I came up
> with when reading the code in preparation of adding a new user of it.
> 
> Diffstat:
>  fs/dcache.c             |    3 --
>  fs/erofs/zdata.c        |    3 --
>  fs/gfs2/quota.c         |    3 --
>  include/linux/lockref.h |   26 ++++++++++++++------
>  lib/lockref.c           |   60 ++++++++++++------------------------------------
>  5 files changed, 36 insertions(+), 59 deletions(-)
> 
> [...]

Looks good, thanks!

---

Applied to the vfs-6.14.misc branch of the vfs/vfs.git tree.
Patches in the vfs-6.14.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.14.misc

[1/8] lockref: remove lockref_put_not_zero
      https://git.kernel.org/vfs/vfs/c/74b5da771c89
[2/8] lockref: improve the lockref_get_not_zero description
      https://git.kernel.org/vfs/vfs/c/8c7568356d74
[3/8] lockref: use bool for false/true returns
      https://git.kernel.org/vfs/vfs/c/57bd981b2db7
[4/8] lockref: drop superfluous externs
      https://git.kernel.org/vfs/vfs/c/80e2823cbe59
[5/8] lockref: add a lockref_init helper
      https://git.kernel.org/vfs/vfs/c/5f0c395edf59
[6/8] dcache: use lockref_init for d_lockref
      https://git.kernel.org/vfs/vfs/c/24706068b7b6
[7/8] erofs: use lockref_init for pcl->lockref
      https://git.kernel.org/vfs/vfs/c/160a93170d53
[8/8] gfs2: use lockref_init for qd_lockref
      https://git.kernel.org/vfs/vfs/c/0ef3858b15e3

Re: lockref cleanups

[Jeff Layton]

On Wed, 2025-01-15 at 10:46 +0100, Christoph Hellwig wrote:
> Hi all,
> 
> this series has a bunch of cosmetic cleanups for the lockref code I came up
> with when reading the code in preparation of adding a new user of it.
> 
> Diffstat:
>  fs/dcache.c             |    3 --
>  fs/erofs/zdata.c        |    3 --
>  fs/gfs2/quota.c         |    3 --
>  include/linux/lockref.h |   26 ++++++++++++++------
>  lib/lockref.c           |   60 ++++++++++++------------------------------------
>  5 files changed, 36 insertions(+), 59 deletions(-)
> 

Aside from the commit log problem Andreas pointed out in patch #2, this
looks good. Assuming that's fixed:

Reviewed-by: Jeff Layton <jlayton@kernel.org>