On 05/04/2024 09:50, Christoph Hellwig wrote:
The spare lock context tracking warns about the mutex unlock in
queue_limits_commit_update despite the __releases annoation:
block/blk-settings.c:263:9: warning: context imbalance in 'queue_limits_commit_update' - wrong
count at exit
As far as I can tell that is because the sparse lock tracking code is
busted for inline functions. Work around it by splitting an inline
wrapper out of queue_limits_commit_update and doing the unlock there.
I find that just removing the __releases() from
queue_limits_commit_update makes it go away.
I have been playing around with this and I can't see why.
I'm not convinced that mutexes are handled properly by sparse.
Here is a similar issue for net code:
john@localhost:~/mnt_sda4/john/mkp-scsi> make C=2 net/core/sock.o
CHECK scripts/mod/empty.c
CALL scripts/checksyscalls.sh
DESCEND objtool
INSTALL libsubcmd_headers
CHECK net/core/sock.c
net/core/sock.c:2393:9: warning: context imbalance in 'sk_clone_lock' -
wrong count at exit
net/core/sock.c:2397:6: warning: context imbalance in
'sk_free_unlock_clone' - unexpected unlock
net/core/sock.c:4034:13: warning: context imbalance in 'proto_seq_start'
- wrong count at exit
net/core/sock.c:4046:13: warning: context imbalance in 'proto_seq_stop'
- wrong count at exit
john@localhost:~/mnt_sda4/john/mkp-scsi>
static void proto_seq_stop(struct seq_file *seq, void *v)
__releases(proto_list_mutex)
{
mutex_unlock(&proto_list_mutex);
}
That seems similar to the queue_limits_commit_update problem, but no
inlining.
Changing the q->limits_lock to a semaphore seems to make the issue go
away; but how to annotate queue_limits_set() is tricky regardless, as it
acquires and then releases silently.
Anyway, changing the code, below, for sparse when it seems somewhat
broken/unreliable may not be the best approach.
Thanks,
John
Reported-by: John Garry <john.g.garry@xxxxxxxxxx>
Signed-off-by: Christoph Hellwig <hch@xxxxxx>
---
block/blk-settings.c | 40 +++++++++++++++++-----------------------
include/linux/blkdev.h | 32 +++++++++++++++++++++++++++++---
2 files changed, 46 insertions(+), 26 deletions(-)
diff --git a/block/blk-settings.c b/block/blk-settings.c
index cdbaef159c4bc3..9ef52b80965dc1 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -239,31 +239,20 @@ int blk_set_default_limits(struct queue_limits *lim)
return blk_validate_limits(lim);
}
-/**
- * queue_limits_commit_update - commit an atomic update of queue limits
- * @q: queue to update
- * @lim: limits to apply
- *
- * Apply the limits in @lim that were obtained from queue_limits_start_update()
- * and updated by the caller to @q.
- *
- * Returns 0 if successful, else a negative error code.
- */
-int queue_limits_commit_update(struct request_queue *q,
+int __queue_limits_commit_update(struct request_queue *q,
struct queue_limits *lim)
- __releases(q->limits_lock)
{
- int error = blk_validate_limits(lim);
-
- if (!error) {
- q->limits = *lim;
- if (q->disk)
- blk_apply_bdi_limits(q->disk->bdi, lim);
- }
- mutex_unlock(&q->limits_lock);
- return error;
+ int error;
+
+ error = blk_validate_limits(lim);
+ if (error)
+ return error;
+ q->limits = *lim;
+ if (q->disk)
+ blk_apply_bdi_limits(q->disk->bdi, lim);
+ return 0;
}
-EXPORT_SYMBOL_GPL(queue_limits_commit_update);
+EXPORT_SYMBOL_GPL(__queue_limits_commit_update);
/**
* queue_limits_set - apply queue limits to queue
@@ -278,8 +267,13 @@ EXPORT_SYMBOL_GPL(queue_limits_commit_update);
*/
int queue_limits_set(struct request_queue *q, struct queue_limits *lim)
{
+ int error;
+
mutex_lock(&q->limits_lock);
- return queue_limits_commit_update(q, lim);
+ error = __queue_limits_commit_update(q, lim);
+ mutex_unlock(&q->limits_lock);
+
+ return error;
}
EXPORT_SYMBOL_GPL(queue_limits_set);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c3e8f7cf96be9e..99f1d2fcec4a2a 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -869,6 +869,15 @@ static inline unsigned int blk_chunk_sectors_left(sector_t offset,
return chunk_sectors - (offset & (chunk_sectors - 1));
}
+/*
+ * Note: we want queue_limits_start_update to be inline to avoid passing a huge
+ * strut by value. This in turn requires the part of queue_limits_commit_update
+ * that unlocks the mutex to be inline as well to not confuse the sparse lock
+ * context tracking. Never use __queue_limits_commit_update directly.
+ */
+int __queue_limits_commit_update(struct request_queue *q,
+ struct queue_limits *lim);
+
/**
* queue_limits_start_update - start an atomic update of queue limits
* @q: queue to update
@@ -883,13 +892,30 @@ 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;
}
-int queue_limits_commit_update(struct request_queue *q,
- struct queue_limits *lim);
+
+/**
+ * queue_limits_commit_update - commit an atomic update of queue limits
+ * @q: queue to update
+ * @lim: limits to apply
+ *
+ * Apply the limits in @lim that were obtained from queue_limits_start_update()
+ * and updated by the caller to @q.
+ *
+ * Returns 0 if successful, else a negative error code.
+ */
+static inline int queue_limits_commit_update(struct request_queue *q,
+ struct queue_limits *lim)
+{
+ int error = __queue_limits_commit_update(q, lim);
+
+ mutex_unlock(&q->limits_lock);
+ return error;
+}
+
int queue_limits_set(struct request_queue *q, struct queue_limits *lim);
/*
