[PATCH 0/3] blk-cgroup: three small fixes

[PATCH 0/3] blk-cgroup: three small fixes

[Ming Lei]

Hello,

The first 2 fixes lockdep warnings.

The last one tries to deactivate policy after blkgs are destroyed.


Ming Lei (3):
  blk-throttle: fix lockdep warning of "cgroup_mutex or RCU read lock
    required!"
  blk-cgroup: avoid to warn !rcu_read_lock_held() in blkg_lookup()
  blk-cgroup: bypass blkcg_deactivate_policy after destroying

 block/blk-cgroup.c   | 13 +++++++++++++
 block/blk-cgroup.h   |  2 --
 block/blk-throttle.c |  2 ++
 3 files changed, 15 insertions(+), 2 deletions(-)

-- 
2.41.0

[PATCH 1/3] blk-throttle: fix lockdep warning of "cgroup_mutex or RCU read lock required!"

[Ming Lei]

Inside blkg_for_each_descendant_pre(), both
css_for_each_descendant_pre() and blkg_lookup() requires RCU read lock,
and either cgroup_assert_mutex_or_rcu_locked() or rcu_read_lock_held()
is called.

Fix the warning by adding rcu read lock.

Reported-by: Changhui Zhong <czhong@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-throttle.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 38a881cf97d0..050f8ac6fcb1 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1314,6 +1314,7 @@ static void tg_conf_updated(struct throtl_grp *tg, bool global)
 		   tg_bps_limit(tg, READ), tg_bps_limit(tg, WRITE),
 		   tg_iops_limit(tg, READ), tg_iops_limit(tg, WRITE));
 
+	rcu_read_lock();
 	/*
 	 * Update has_rules[] flags for the updated tg's subtree.  A tg is
 	 * considered to have rules if either the tg itself or any of its
@@ -1341,6 +1342,7 @@ static void tg_conf_updated(struct throtl_grp *tg, bool global)
 		this_tg->latency_target = max(this_tg->latency_target,
 				parent_tg->latency_target);
 	}
+	rcu_read_unlock();
 
 	/*
 	 * We're already holding queue_lock and know @tg is valid.  Let's
-- 
2.41.0

[PATCH 2/3] blk-cgroup: avoid to warn !rcu_read_lock_held() in blkg_lookup()

[Ming Lei]

So far, all callers either holds spin lock or rcu read explicitly, and
most of the caller has added WARN_ON_ONCE(!rcu_read_lock_held()) or
lockdep_assert_held(&disk->queue->queue_lock).

Remove WARN_ON_ONCE(!rcu_read_lock_held()) from blkg_lookup() for
killing the false positive warning from blkg_conf_prep().

Reported-by: Changhui Zhong <czhong@redhat.com>
Fixes: 83462a6c971c ("blkcg: Drop unnecessary RCU read [un]locks from blkg_conf_prep/finish()")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-cgroup.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
index 624c03c8fe64..fd482439afbc 100644
--- a/block/blk-cgroup.h
+++ b/block/blk-cgroup.h
@@ -249,8 +249,6 @@ static inline struct blkcg_gq *blkg_lookup(struct blkcg *blkcg,
 {
 	struct blkcg_gq *blkg;
 
-	WARN_ON_ONCE(!rcu_read_lock_held());
-
 	if (blkcg == &blkcg_root)
 		return q->root_blkg;
 
-- 
2.41.0

[PATCH 3/3] blk-cgroup: bypass blkcg_deactivate_policy after destroying

[Ming Lei]

blkcg_deactivate_policy() can be called after blkg_destroy_all()
returns, and it isn't necessary since blkg_destroy_all has covered
policy deactivation.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-cgroup.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 4a42ea2972ad..4b48c2c44098 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -577,6 +577,7 @@ static void blkg_destroy_all(struct gendisk *disk)
 	struct request_queue *q = disk->queue;
 	struct blkcg_gq *blkg, *n;
 	int count = BLKG_DESTROY_BATCH_SIZE;
+	int i;
 
 restart:
 	spin_lock_irq(&q->queue_lock);
@@ -602,6 +603,18 @@ static void blkg_destroy_all(struct gendisk *disk)
 		}
 	}
 
+	/*
+	 * Mark policy deactivated since policy offline has been done, and
+	 * the free is scheduled, so future blkcg_deactivate_policy() can
+	 * be bypassed
+	 */
+	for (i = 0; i < BLKCG_MAX_POLS; i++) {
+		struct blkcg_policy *pol = blkcg_policy[i];
+
+		if (pol)
+			__clear_bit(pol->plid, q->blkcg_pols);
+	}
+
 	q->root_blkg = NULL;
 	spin_unlock_irq(&q->queue_lock);
 }
-- 
2.41.0

Re: [PATCH 0/3] blk-cgroup: three small fixes

[Jens Axboe]

On Fri, 17 Nov 2023 10:35:21 +0800, Ming Lei wrote:
> The first 2 fixes lockdep warnings.
> 
> The last one tries to deactivate policy after blkgs are destroyed.
> 
> 
> Ming Lei (3):
>   blk-throttle: fix lockdep warning of "cgroup_mutex or RCU read lock
>     required!"
>   blk-cgroup: avoid to warn !rcu_read_lock_held() in blkg_lookup()
>   blk-cgroup: bypass blkcg_deactivate_policy after destroying
> 
> [...]

Applied, thanks!

[1/3] blk-throttle: fix lockdep warning of "cgroup_mutex or RCU read lock required!"
      commit: 27b13e209ddca5979847a1b57890e0372c1edcee
[2/3] blk-cgroup: avoid to warn !rcu_read_lock_held() in blkg_lookup()
      commit: 35a99d6557cacbc177314735342f77a2dda41872
[3/3] blk-cgroup: bypass blkcg_deactivate_policy after destroying
      commit: e63a57303599b17290cd8bc48e6f20b24289a8bc

Best regards,
-- 
Jens Axboe

Re: [PATCH 3/3] blk-cgroup: bypass blkcg_deactivate_policy after destroying

[Tejun Heo]

Hello, Ming.

On Fri, Nov 17, 2023 at 10:35:24AM +0800, Ming Lei wrote:
> blkcg_deactivate_policy() can be called after blkg_destroy_all()
> returns, and it isn't necessary since blkg_destroy_all has covered
> policy deactivation.

Does this actually affect anything? This would matter iff a policy is
deactivated for the queue between blkg_destroy_all() and the rest of the
queue destruction, right? That's a really small window against very rare
operations. The patch is already in and I'm not necessarily against the
change but I'm curious what the motivation is.

Thanks.

-- 
tejun

Re: [PATCH 3/3] blk-cgroup: bypass blkcg_deactivate_policy after destroying

[Ming Lei]

On Sun, Nov 19, 2023 at 04:57:46AM -1000, Tejun Heo wrote:
> Hello, Ming.
> 
> On Fri, Nov 17, 2023 at 10:35:24AM +0800, Ming Lei wrote:
> > blkcg_deactivate_policy() can be called after blkg_destroy_all()
> > returns, and it isn't necessary since blkg_destroy_all has covered
> > policy deactivation.
> 
> Does this actually affect anything? This would matter iff a policy is
> deactivated for the queue between blkg_destroy_all() and the rest of the
> queue destruction, right?

Yeah, it is true actually for throttle code.



thanks,
Ming