| Message ID | 20250115094702.504610-9-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/8] lockref: remove lockref_put_not_zero | expand |
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
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.
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;
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?
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
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!
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);
Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/gfs2/quota.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)