2018-12-19 22:48:05

by Dennis Zhou

[permalink] [raw]
Subject: [PATCH] blkcg: clean up blkg_tryget_closest()

The implementation of blkg_tryget_closest() wasn't super obvious and
became a point of suspicion when debugging [1]. So let's clean it up so
it's obviously not the problem.

[1] https://lore.kernel.org/linux-block/[email protected]/

Signed-off-by: Dennis Zhou <[email protected]>
---
include/linux/blk-cgroup.h | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
index f025fd1e22e6..76c61318fda5 100644
--- a/include/linux/blk-cgroup.h
+++ b/include/linux/blk-cgroup.h
@@ -499,22 +499,33 @@ static inline void blkg_get(struct blkcg_gq *blkg)
*/
static inline bool blkg_tryget(struct blkcg_gq *blkg)
{
- return percpu_ref_tryget(&blkg->refcnt);
+ return blkg && percpu_ref_tryget(&blkg->refcnt);
}

/**
* blkg_tryget_closest - try and get a blkg ref on the closet blkg
* @blkg: blkg to get
*
- * This walks up the blkg tree to find the closest non-dying blkg and returns
- * the blkg that it did association with as it may not be the passed in blkg.
+ * This needs to be called rcu protected. As the failure mode here is to walk
+ * up the blkg tree, this ensure that the blkg->parent pointers are always
+ * valid. This returns the blkg that it ended up taking a reference on or %NULL
+ * if no reference was taken.
*/
static inline struct blkcg_gq *blkg_tryget_closest(struct blkcg_gq *blkg)
{
- while (blkg && !percpu_ref_tryget(&blkg->refcnt))
+ struct blkcg_gq *ret_blkg = NULL;
+
+ WARN_ON_ONCE(!rcu_read_lock_held());
+
+ while (blkg) {
+ if (blkg_tryget(blkg)) {
+ ret_blkg = blkg;
+ break;
+ }
blkg = blkg->parent;
+ }

- return blkg;
+ return ret_blkg;
}

/**
--
2.17.1



2018-12-20 17:22:04

by Jens Axboe

[permalink] [raw]
Subject: Re: [PATCH] blkcg: clean up blkg_tryget_closest()

On 12/19/18 3:43 PM, Dennis Zhou wrote:
> The implementation of blkg_tryget_closest() wasn't super obvious and
> became a point of suspicion when debugging [1]. So let's clean it up so
> it's obviously not the problem.

I like the cleanup - adding to the later pile for the merge window, thanks
Dennis.

--
Jens Axboe