[PATCH 0/3] Some block sparse fixes

[PATCH 0/3] Some block sparse fixes

[John Garry]

Branches for-6.11/block and block-6.10 both have the following sparse
build issues:

block/bdev.c:388:17: warning: symbol 'blockdev_mnt' was not declared.
Should it be static?
block/blk-settings.c:266:9: warning: context imbalance in
'queue_limits_commit_update' - wrong count at exit
block/blk-cgroup.c:834:5: warning: context imbalance in
'blkg_conf_prep' - wrong count at exit
block/blk-cgroup.c:965:9: warning: context imbalance in
'blkg_conf_exit' - wrong count at exit
block/blk-iocost.c:732:9: warning: context imbalance in 'iocg_lock' -
wrong count at exit
block/blk-iocost.c:743:28: warning: context imbalance in 'iocg_unlock'
- unexpected unlock
block/blk-zoned.c:595:30: warning: context imbalance in
'disk_get_and_lock_zone_wplug' - wrong count at exit
block/blk-zoned.c: note: in included file (through include/linux/blkdev.h):
./include/linux/bio.h:592:9: warning: context imbalance in
'blk_zone_wplug_handle_write' - unexpected unlock
block/blk-zoned.c:1747:31: warning: context imbalance in
'blk_revalidate_seq_zone' - unexpected unlock
block/bfq-iosched.c:5498:9: warning: context imbalance in
'bfq_exit_icq' - different lock contexts for basic block

This series contains changes to resolve some of those issues. Some issues
are not addressed, as follows:
- for zoned stuff, as mentioned by Damien at [0], they are not worth
  solving
- for blk-iocost.c issues, at [1] Tejun says that lockdep does a better
  job (than sparse)
- for blk-cgroup.c issues, at [2] again Tejun says that lockdep does a
  better job

I am reposting the patch from Jiapeng Chong, as it was never picked up.

[0] https://lore.kernel.org/linux-block/5e04760e-1334-4514-b2d0-be0d7df33865@kernel.org/
[1] https://lore.kernel.org/linux-block/ZmtwqDsMnTJHQB6o@slm.duckdns.org/
[2] https://lore.kernel.org/lkml/Y7x9t+4EwXFl7OwS@slm.duckdns.org/

Jiapeng Chong (1):
  bdev: make blockdev_mnt static

John Garry (2):
  block: Drop locking annotation for limits_lock
  block: BFQ: Refactor bfq_exit_icq() to silence sparse warning

 block/bdev.c           |  2 +-
 block/bfq-iosched.c    | 38 ++++++++++++++++++++------------------
 block/blk-settings.c   |  1 -
 include/linux/blkdev.h |  1 -
 4 files changed, 21 insertions(+), 21 deletions(-)

-- 
2.31.1

[PATCH 1/3] bdev: make blockdev_mnt static

[John Garry]

From: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>

The blockdev_mnt are not used outside the file bdev.c, so the modification
is defined as static.

block/bdev.c:377:17: warning: symbol 'blockdev_mnt' was not declared. Should it be static?

Reported-by: Abaci Robot <abaci@linux.alibaba.com>
jpg: Remove closes bugzilla link
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 block/bdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/bdev.c b/block/bdev.c
index 353677ac49b3..ced4ac990ec8 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -385,7 +385,7 @@ static struct file_system_type bd_type = {
 };
 
 struct super_block *blockdev_superblock __ro_after_init;
-struct vfsmount *blockdev_mnt __ro_after_init;
+static struct vfsmount *blockdev_mnt __ro_after_init;
 EXPORT_SYMBOL_GPL(blockdev_superblock);
 
 void __init bdev_cache_init(void)
-- 
2.31.1

Re: [PATCH 1/3] bdev: make blockdev_mnt static

[Bart Van Assche]

On 6/14/24 2:03 AM, John Garry wrote:
> The blockdev_mnt are not used outside the file bdev.c, so the modification
> is defined as static.

If this patch would be reposted, please rewrite the above description such that
it becomes comprehensible.

> jpg: Remove closes bugzilla link

The above text should be surrounded with square brackets.

Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH 2/3] block: Drop locking annotation for limits_lock

[John Garry]

Currently compiling block/blk-settings.c with C=1 gives the following
warning:
block/blk-settings.c:262:9: warning: context imbalance in 'queue_limits_commit_update' - wrong count at exit

request_queue.limits_lock is a mutex. Sparse locking annotation for
mutexes are currently not supported - see [0] - so drop that locking
annotation.

[0] https://lore.kernel.org/lkml/cover.1579893447.git.jbi.octave@gmail.com/T/#mbb8bda6c0a7ca7ce19f46df976a8e3b489745488

Fixes: d690cb8ae14bd ("block: add an API to atomically update queue limits")
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 block/blk-settings.c   | 1 -
 include/linux/blkdev.h | 1 -
 2 files changed, 2 deletions(-)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index effeb9a639bb..cdc7845260b9 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -254,7 +254,6 @@ int blk_set_default_limits(struct queue_limits *lim)
  */
 int queue_limits_commit_update(struct request_queue *q,
 		struct queue_limits *lim)
-	__releases(q->limits_lock)
 {
 	int error = blk_validate_limits(lim);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 24c36929920b..f798c9c6eb30 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -904,7 +904,6 @@ static inline unsigned int blk_chunk_sectors_left(sector_t offset,
  */
 static inline struct queue_limits
 queue_limits_start_update(struct request_queue *q)
-	__acquires(q->limits_lock)
 {
 	mutex_lock(&q->limits_lock);
 	return q->limits;
-- 
2.31.1

Re: [PATCH 2/3] block: Drop locking annotation for limits_lock

[Bart Van Assche]

On 6/14/24 2:03 AM, John Garry wrote:
> Currently compiling block/blk-settings.c with C=1 gives the following
> warning:
> block/blk-settings.c:262:9: warning: context imbalance in 'queue_limits_commit_update' - wrong count at exit
> 
> request_queue.limits_lock is a mutex. Sparse locking annotation for
> mutexes are currently not supported - see [0] - so drop that locking
> annotation.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 2/3] block: Drop locking annotation for limits_lock

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 3/3] block: BFQ: Refactor bfq_exit_icq() to silence sparse warning

[John Garry]

Currently building for C=1 generates the following warning:
block/bfq-iosched.c:5498:9: warning: context imbalance in 'bfq_exit_icq' - different lock contexts for basic block

Refactor bfq_exit_icq() into a core part which loops for the actuators,
and only lock calling this routine when necessary.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 block/bfq-iosched.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 4b88a54a9b76..36a4998c4b37 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -5463,40 +5463,42 @@ static void bfq_exit_icq_bfqq(struct bfq_io_cq *bic, bool is_sync,
 	}
 }
 
+static void _bfq_exit_icq(struct bfq_io_cq *bic, unsigned int num_actuators)
+{
+	struct bfq_iocq_bfqq_data *bfqq_data = bic->bfqq_data;
+	unsigned int act_idx;
+
+	for (act_idx = 0; act_idx < num_actuators; act_idx++) {
+		if (bfqq_data[act_idx].stable_merge_bfqq)
+			bfq_put_stable_ref(bfqq_data[act_idx].stable_merge_bfqq);
+
+		bfq_exit_icq_bfqq(bic, true, act_idx);
+		bfq_exit_icq_bfqq(bic, false, act_idx);
+	}
+}
+
 static void bfq_exit_icq(struct io_cq *icq)
 {
 	struct bfq_io_cq *bic = icq_to_bic(icq);
 	struct bfq_data *bfqd = bic_to_bfqd(bic);
 	unsigned long flags;
-	unsigned int act_idx;
+
 	/*
 	 * If bfqd and thus bfqd->num_actuators is not available any
 	 * longer, then cycle over all possible per-actuator bfqqs in
 	 * next loop. We rely on bic being zeroed on creation, and
 	 * therefore on its unused per-actuator fields being NULL.
-	 */
-	unsigned int num_actuators = BFQ_MAX_ACTUATORS;
-	struct bfq_iocq_bfqq_data *bfqq_data = bic->bfqq_data;
-
-	/*
+	 *
 	 * bfqd is NULL if scheduler already exited, and in that case
 	 * this is the last time these queues are accessed.
 	 */
 	if (bfqd) {
 		spin_lock_irqsave(&bfqd->lock, flags);
-		num_actuators = bfqd->num_actuators;
-	}
-
-	for (act_idx = 0; act_idx < num_actuators; act_idx++) {
-		if (bfqq_data[act_idx].stable_merge_bfqq)
-			bfq_put_stable_ref(bfqq_data[act_idx].stable_merge_bfqq);
-
-		bfq_exit_icq_bfqq(bic, true, act_idx);
-		bfq_exit_icq_bfqq(bic, false, act_idx);
-	}
-
-	if (bfqd)
+		_bfq_exit_icq(bic, bfqd->num_actuators);
 		spin_unlock_irqrestore(&bfqd->lock, flags);
+	} else {
+		_bfq_exit_icq(bic, BFQ_MAX_ACTUATORS);
+	}
 }
 
 /*
-- 
2.31.1

Re: [PATCH 3/3] block: BFQ: Refactor bfq_exit_icq() to silence sparse warning

[Bart Van Assche]

On 6/14/24 2:03 AM, John Garry wrote:
> Currently building for C=1 generates the following warning:
> block/bfq-iosched.c:5498:9: warning: context imbalance in 'bfq_exit_icq' - different lock contexts for basic block
> 
> Refactor bfq_exit_icq() into a core part which loops for the actuators,
> and only lock calling this routine when necessary.

I like this patch because it makes the code easier to read.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 0/3] Some block sparse fixes

[Jens Axboe]

On Fri, 14 Jun 2024 09:03:42 +0000, John Garry wrote:
> Branches for-6.11/block and block-6.10 both have the following sparse
> build issues:
> 
> block/bdev.c:388:17: warning: symbol 'blockdev_mnt' was not declared.
> Should it be static?
> block/blk-settings.c:266:9: warning: context imbalance in
> 'queue_limits_commit_update' - wrong count at exit
> block/blk-cgroup.c:834:5: warning: context imbalance in
> 'blkg_conf_prep' - wrong count at exit
> block/blk-cgroup.c:965:9: warning: context imbalance in
> 'blkg_conf_exit' - wrong count at exit
> block/blk-iocost.c:732:9: warning: context imbalance in 'iocg_lock' -
> wrong count at exit
> block/blk-iocost.c:743:28: warning: context imbalance in 'iocg_unlock'
> - unexpected unlock
> block/blk-zoned.c:595:30: warning: context imbalance in
> 'disk_get_and_lock_zone_wplug' - wrong count at exit
> block/blk-zoned.c: note: in included file (through include/linux/blkdev.h):
> ./include/linux/bio.h:592:9: warning: context imbalance in
> 'blk_zone_wplug_handle_write' - unexpected unlock
> block/blk-zoned.c:1747:31: warning: context imbalance in
> 'blk_revalidate_seq_zone' - unexpected unlock
> block/bfq-iosched.c:5498:9: warning: context imbalance in
> 'bfq_exit_icq' - different lock contexts for basic block
> 
> [...]

Applied, thanks!

[1/3] bdev: make blockdev_mnt static
      commit: d9c2332199d073c5edd7163d64fbdee6224d8c08
[2/3] block: Drop locking annotation for limits_lock
      commit: c3042a5403ef2be622023fcc3b11fc1aa08ba7fa
[3/3] block: BFQ: Refactor bfq_exit_icq() to silence sparse warning
      commit: 66088084fdabb6e5075cd19e8ffe15b8bc7e3708

Best regards,
-- 
Jens Axboe