On Thu, 2018-04-12 at 12:09 -0700, t...@kernel.org wrote:
> On Thu, Apr 12, 2018 at 06:56:26PM +0000, Bart Van Assche wrote:
> > On Thu, 2018-04-12 at 11:11 -0700, t...@kernel.org 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 <t...@kernel.org>
Signed-off-by: Bart Van Assche <bart.vanass...@wdc.com>
Cc: Tejun Heo <t...@kernel.org>
---
 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;

Reply via email to