On Thu, 2018-04-12 at 12:09 -0700, tj@xxxxxxxxxx wrote:
> On Thu, Apr 12, 2018 at 06:56:26PM +0000, Bart Van Assche wrote:
> > On Thu, 2018-04-12 at 11:11 -0700, tj@xxxxxxxxxx wrote:
> > > * Right now, blk_queue_enter/exit() doesn't have any annotations.
> > > IOW, there's no way for paths which need enter locked to actually
> > > assert that. If we're gonna protect more things with queue
> > > enter/exit, it gotta be annotated.
> >
> > Hello Tejun,
> >
> > One possibility is to check the count for the local CPU of q->q_usage_counter
> > at runtime, e.g. using WARN_ON_ONCE(). However, that might have a runtime
> > overhead. Another possibility is to follow the example of kernfs and to use
> > a lockdep map and lockdep_assert_held(). There might be other alternatives I
> > have not thought of.
>
> Oh, I'd just do straight-forward lockdep annotation on
> queue_enter/exit functions and provide the necessary assert helpers.
Hello Tejun,
Is something like the patch below perhaps what you had in mind?
Thanks,
Bart.
Subject: [PATCH] block: Use lockdep to verify whether blk_queue_enter() is
used when necessary
Suggested-by: Tejun Heo <tj@xxxxxxxxxx>
Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxx>
Cc: Tejun Heo <tj@xxxxxxxxxx>
---
block/blk-cgroup.c | 2 ++
block/blk-core.c | 13 ++++++++++++-
block/blk.h | 1 +
include/linux/blk-cgroup.h | 2 ++
include/linux/blkdev.h | 1 +
5 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 1c16694ae145..c34e13e76f90 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -145,6 +145,8 @@ struct blkcg_gq *blkg_lookup_slowpath(struct blkcg *blkcg,
{
struct blkcg_gq *blkg;
+ lock_is_held(&q->q_enter_map);
+
/*
* Hint didn't match. Look up from the radix tree. Note that the
* hint can only be updated under queue_lock as otherwise @blkg
diff --git a/block/blk-core.c b/block/blk-core.c
index 39308e874ffa..a61dbe7f24a5 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -931,8 +931,13 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags)
}
rcu_read_unlock();
- if (success)
+ if (success) {
+ mutex_acquire_nest(&q->q_enter_map, 0, 0,
+ lock_is_held(&q->q_enter_map) ?
+ &q->q_enter_map : NULL,
+ _RET_IP_);
return 0;
+ }
if (flags & BLK_MQ_REQ_NOWAIT)
return -EBUSY;
@@ -959,6 +964,7 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags)
void blk_queue_exit(struct request_queue *q)
{
+ mutex_release(&q->q_enter_map, 0, _RET_IP_);
percpu_ref_put(&q->q_usage_counter);
}
@@ -994,12 +1000,15 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id,
spinlock_t *lock)
{
struct request_queue *q;
+ static struct lock_class_key __key;
q = kmem_cache_alloc_node(blk_requestq_cachep,
gfp_mask | __GFP_ZERO, node_id);
if (!q)
return NULL;
+ lockdep_init_map(&q->q_enter_map, "q_enter_map", &__key, 0);
+
q->id = ida_simple_get(&blk_queue_ida, 0, 0, gfp_mask);
if (q->id < 0)
goto fail_q;
@@ -2264,6 +2273,8 @@ generic_make_request_checks(struct bio *bio)
goto end_io;
}
+ lock_is_held(&q->q_enter_map);
+
/*
* For a REQ_NOWAIT based request, return -EOPNOTSUPP
* if queue is not a request based queue.
diff --git a/block/blk.h b/block/blk.h
index 7cd64f533a46..26f313331b13 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -145,6 +145,7 @@ static inline void blk_queue_enter_live(struct request_queue *q)
* been established, further references under that same context
* need not check that the queue has been frozen (marked dead).
*/
+ mutex_acquire_nest(&q->q_enter_map, 0, 0, &q->q_enter_map, _RET_IP_);
percpu_ref_get(&q->q_usage_counter);
}
diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
index 6c666fd7de3c..52e2e2d9971e 100644
--- a/include/linux/blk-cgroup.h
+++ b/include/linux/blk-cgroup.h
@@ -266,6 +266,8 @@ static inline struct blkcg_gq *__blkg_lookup(struct blkcg *blkcg,
{
struct blkcg_gq *blkg;
+ lock_is_held(&q->q_enter_map);
+
if (blkcg == &blkcg_root)
return q->root_blkg;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 0d22f351a74b..a0b1adbd4406 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -631,6 +631,7 @@ struct request_queue {
struct rcu_head rcu_head;
wait_queue_head_t mq_freeze_wq;
struct percpu_ref q_usage_counter;
+ struct lockdep_map q_enter_map;
struct list_head all_q_node;
struct blk_mq_tag_set *tag_set;
