* [PATCH] block: add lockdep to queue_limits_commit_update()
@ 2025-11-09 7:44 Chaitanya Kulkarni
2025-11-11 8:19 ` Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Chaitanya Kulkarni @ 2025-11-09 7:44 UTC (permalink / raw)
To: hch; +Cc: axboe, linux-block, Chaitanya Kulkarni
queue_limits_commit_update() expects q->limits_lock to be held by
the caller (via queue_limits_start_update()).
The API pattern is:
lim = queue_limits_start_update(q); /* acquires lock */
/* modify lim */
queue_limits_commit_update(q, &lim); /* releases lock */
OR
queue_limits_commit_update_frozen(q, &lim);
lim = queue_limits_start_update(q); /* acquires lock */
queue_limits_commit_update(q, &lim); /* releases lock */
Add lockdep_assert_held() to report incorrect API usage.
Signed-off-by: Chaitanya Kulkarni <ckulkarnilinux@gmail.com>
---
With this patch incorrect calls to queue_limits_commit_update() will
report following :-
[ 800.055674] ------------[ cut here ]------------
[ 800.055676] WARNING: CPU: 36 PID: 5507 at block/blk-settings.c:559 queue_limits_commit_update+0xc2/0xd0
[ 800.055691] Modules linked in: test_lockdep(O+) snd_seq_dummy snd_hrtimer snd_seq snd_seq_device snd_timer snd soundcore bridge stp llc ip_set nf_tables rfkill nfnetlink tun sunrpc xfs squashfs loop intel_rapl_msr intel_rapl_common kvm_amd ccp kvm ppdev parport_pc parport irqbypass i2c_piix4 joydev pcspkr i2c_smbus qxl drm_exec drm_ttm_helper bochs ttm drm_shmem_helper drm_client_lib virtio_net drm_kms_helper net_failover ghash_clmulni_intel virtio_blk failover drm ata_generic serio_raw pata_acpi qemu_fw_cfg ipmi_devintf ipmi_msghandler fuse [last unloaded: test_lockdep(O)]
[ 800.055785] CPU: 36 UID: 0 PID: 5507 Comm: insmod Tainted: G O N 6.18.0-rc4lblk+ #103 PREEMPT(voluntary)
[ 800.055792] Tainted: [O]=OOT_MODULE, [N]=TEST
[ 800.055794] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
[ 800.055797] RIP: 0010:queue_limits_commit_update+0xc2/0xd0
[ 800.055801] Code: e8 f3 b8 89 00 44 89 e0 5b 5d 41 5c c3 cc cc cc cc 48 8d bf f8 06 00 00 be ff ff ff ff e8 06 2f 89 00 85 c0 0f 85 5b ff ff ff <0f> 0b e9 54 ff ff ff 0f 1f 80 00 00 00 00 90 90 90 90 90 90 90 90
[ 800.055805] RSP: 0018:ffffc90007e37b30 EFLAGS: 00010246
[ 800.055809] RAX: 0000000000000000 RBX: ffff88815afaae40 RCX: 0000000000000001
[ 800.055812] RDX: 0000000000000000 RSI: ffffffff8294deb9 RDI: ffffffff829e7716
[ 800.055815] RBP: ffffc90007e37b50 R08: ffffc90007e37b50 R09: ffff88815afaae40
[ 800.055818] R10: ffff8897df0a0000 R11: ffff88983fed3bc0 R12: 0000000000000000
[ 800.055820] R13: 000055d8c85e62a0 R14: ffff8881761b24a8 R15: ffffffff8457b4e0
[ 800.055825] FS: 00007faf1f802b80(0000) GS:ffff88985c0e7000(0000) knlGS:0000000000000000
[ 800.055834] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 800.055837] CR2: 000055d8c85e9eb8 CR3: 000000021fc44000 CR4: 0000000000350ef0
[ 800.055842] DR0: ffffffff845fdc70 DR1: ffffffff845fdc71 DR2: ffffffff845fdc72
[ 800.055844] DR3: ffffffff845fdc73 DR6: 00000000ffff0ff0 DR7: 0000000000000600
[ 800.055847] Call Trace:
[ 800.055850] <TASK>
[ 800.055853] ? __pfx_test_lockdep_init+0x10/0x10 [test_lockdep]
[ 800.055860] test_lockdep_init+0x1f1/0xff0 [test_lockdep]
[ 800.055881] ? lock_acquire+0xe3/0x2f0
[ 800.055889] ? find_held_lock+0x2b/0x80
[ 800.055895] ? __kmalloc_cache_noprof+0x5d/0x770
[ 800.055906] ? lock_release+0x14a/0x2b0
[ 800.055911] ? fs_reclaim_acquire+0x48/0xd0
[ 800.055923] ? __pfx_test_lockdep_init+0x10/0x10 [test_lockdep]
[ 800.055929] do_one_initcall+0x58/0x2b0
[ 800.055941] do_init_module+0x64/0x260
[ 800.055952] init_module_from_file+0x8a/0xd0
[ 800.055969] idempotent_init_module+0x17b/0x280
[ 800.055980] ? netif_queue_set_napi+0xe0/0x150
[ 800.056007] __x64_sys_finit_module+0x66/0xd0
[ 800.056012] ? do_syscall_64+0x38/0xb00
[ 800.056064] do_syscall_64+0x76/0xb00
[ 800.056072] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 800.056076] RIP: 0033:0x7faf1f32bddd
---
block/blk-settings.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 78dfef117623..51401f08ce05 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -556,6 +556,8 @@ int queue_limits_commit_update(struct request_queue *q,
{
int error;
+ lockdep_assert_held(&q->limits_lock);
+
error = blk_validate_limits(lim);
if (error)
goto out_unlock;
--
2.40.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] block: add lockdep to queue_limits_commit_update()
2025-11-09 7:44 [PATCH] block: add lockdep to queue_limits_commit_update() Chaitanya Kulkarni
@ 2025-11-11 8:19 ` Christoph Hellwig
2025-11-11 8:51 ` John Garry
2025-11-11 14:55 ` Jens Axboe
2 siblings, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2025-11-11 8:19 UTC (permalink / raw)
To: Chaitanya Kulkarni; +Cc: hch, axboe, linux-block
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] block: add lockdep to queue_limits_commit_update()
2025-11-09 7:44 [PATCH] block: add lockdep to queue_limits_commit_update() Chaitanya Kulkarni
2025-11-11 8:19 ` Christoph Hellwig
@ 2025-11-11 8:51 ` John Garry
2025-11-11 19:00 ` Bart Van Assche
2025-11-11 14:55 ` Jens Axboe
2 siblings, 1 reply; 5+ messages in thread
From: John Garry @ 2025-11-11 8:51 UTC (permalink / raw)
To: Chaitanya Kulkarni, hch; +Cc: axboe, linux-block
On 09/11/2025 07:44, Chaitanya Kulkarni wrote:
> queue_limits_commit_update() expects q->limits_lock to be held by
> the caller (via queue_limits_start_update()).
>
> The API pattern is:
>
> lim = queue_limits_start_update(q); /* acquires lock */
> /* modify lim */
> queue_limits_commit_update(q, &lim); /* releases lock */
>
> OR
>
> queue_limits_commit_update_frozen(q, &lim);
> lim = queue_limits_start_update(q); /* acquires lock */
> queue_limits_commit_update(q, &lim); /* releases lock */
>
> Add lockdep_assert_held() to report incorrect API usage.
>
> Signed-off-by: Chaitanya Kulkarni <ckulkarnilinux@gmail.com>
> ---
>
> With this patch incorrect calls to queue_limits_commit_update() will
> report following :-
>
> [ 800.055674] ------------[ cut here ]------------
> [ 800.055676] WARNING: CPU: 36 PID: 5507 at block/blk-settings.c:559 queue_limits_commit_update+0xc2/0xd0
> [ 800.055691] Modules linked in: test_lockdep(O+) snd_seq_dummy snd_hrtimer snd_seq snd_seq_device snd_timer snd soundcore bridge stp llc ip_set nf_tables rfkill nfnetlink tun sunrpc xfs squashfs loop intel_rapl_msr intel_rapl_common kvm_amd ccp kvm ppdev parport_pc parport irqbypass i2c_piix4 joydev pcspkr i2c_smbus qxl drm_exec drm_ttm_helper bochs ttm drm_shmem_helper drm_client_lib virtio_net drm_kms_helper net_failover ghash_clmulni_intel virtio_blk failover drm ata_generic serio_raw pata_acpi qemu_fw_cfg ipmi_devintf ipmi_msghandler fuse [last unloaded: test_lockdep(O)]
> [ 800.055785] CPU: 36 UID: 0 PID: 5507 Comm: insmod Tainted: G O N 6.18.0-rc4lblk+ #103 PREEMPT(voluntary)
> [ 800.055792] Tainted: [O]=OOT_MODULE, [N]=TEST
> [ 800.055794] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
> [ 800.055797] RIP: 0010:queue_limits_commit_update+0xc2/0xd0
> [ 800.055801] Code: e8 f3 b8 89 00 44 89 e0 5b 5d 41 5c c3 cc cc cc cc 48 8d bf f8 06 00 00 be ff ff ff ff e8 06 2f 89 00 85 c0 0f 85 5b ff ff ff <0f> 0b e9 54 ff ff ff 0f 1f 80 00 00 00 00 90 90 90 90 90 90 90 90
> [ 800.055805] RSP: 0018:ffffc90007e37b30 EFLAGS: 00010246
> [ 800.055809] RAX: 0000000000000000 RBX: ffff88815afaae40 RCX: 0000000000000001
> [ 800.055812] RDX: 0000000000000000 RSI: ffffffff8294deb9 RDI: ffffffff829e7716
> [ 800.055815] RBP: ffffc90007e37b50 R08: ffffc90007e37b50 R09: ffff88815afaae40
> [ 800.055818] R10: ffff8897df0a0000 R11: ffff88983fed3bc0 R12: 0000000000000000
> [ 800.055820] R13: 000055d8c85e62a0 R14: ffff8881761b24a8 R15: ffffffff8457b4e0
> [ 800.055825] FS: 00007faf1f802b80(0000) GS:ffff88985c0e7000(0000) knlGS:0000000000000000
> [ 800.055834] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 800.055837] CR2: 000055d8c85e9eb8 CR3: 000000021fc44000 CR4: 0000000000350ef0
> [ 800.055842] DR0: ffffffff845fdc70 DR1: ffffffff845fdc71 DR2: ffffffff845fdc72
> [ 800.055844] DR3: ffffffff845fdc73 DR6: 00000000ffff0ff0 DR7: 0000000000000600
> [ 800.055847] Call Trace:
> [ 800.055850] <TASK>
> [ 800.055853] ? __pfx_test_lockdep_init+0x10/0x10 [test_lockdep]
> [ 800.055860] test_lockdep_init+0x1f1/0xff0 [test_lockdep]
> [ 800.055881] ? lock_acquire+0xe3/0x2f0
> [ 800.055889] ? find_held_lock+0x2b/0x80
> [ 800.055895] ? __kmalloc_cache_noprof+0x5d/0x770
> [ 800.055906] ? lock_release+0x14a/0x2b0
> [ 800.055911] ? fs_reclaim_acquire+0x48/0xd0
> [ 800.055923] ? __pfx_test_lockdep_init+0x10/0x10 [test_lockdep]
> [ 800.055929] do_one_initcall+0x58/0x2b0
> [ 800.055941] do_init_module+0x64/0x260
> [ 800.055952] init_module_from_file+0x8a/0xd0
> [ 800.055969] idempotent_init_module+0x17b/0x280
> [ 800.055980] ? netif_queue_set_napi+0xe0/0x150
> [ 800.056007] __x64_sys_finit_module+0x66/0xd0
> [ 800.056012] ? do_syscall_64+0x38/0xb00
> [ 800.056064] do_syscall_64+0x76/0xb00
> [ 800.056072] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [ 800.056076] RIP: 0033:0x7faf1f32bddd
>
> ---
> block/blk-settings.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index 78dfef117623..51401f08ce05 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -556,6 +556,8 @@ int queue_limits_commit_update(struct request_queue *q,
> {
> int error;
>
> + lockdep_assert_held(&q->limits_lock);
> +
We always unlock the mutex in this function (not shown).
With your change, if CONFIG_LOCKDEP is enabled then we get the above
warning. However, if CONFIG_DEBUG_MUTEXES was enabled, we would already
be getting a warning.
Maybe using LOCKDEP is much more preferred than DEBUG_MUTEXES.
> error = blk_validate_limits(lim);
> if (error)
> goto out_unlock;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] block: add lockdep to queue_limits_commit_update()
2025-11-09 7:44 [PATCH] block: add lockdep to queue_limits_commit_update() Chaitanya Kulkarni
2025-11-11 8:19 ` Christoph Hellwig
2025-11-11 8:51 ` John Garry
@ 2025-11-11 14:55 ` Jens Axboe
2 siblings, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2025-11-11 14:55 UTC (permalink / raw)
To: hch, Chaitanya Kulkarni; +Cc: linux-block
On Sat, 08 Nov 2025 23:44:26 -0800, Chaitanya Kulkarni wrote:
> queue_limits_commit_update() expects q->limits_lock to be held by
> the caller (via queue_limits_start_update()).
>
> The API pattern is:
>
> lim = queue_limits_start_update(q); /* acquires lock */
> /* modify lim */
> queue_limits_commit_update(q, &lim); /* releases lock */
>
> [...]
Applied, thanks!
[1/1] block: add lockdep to queue_limits_commit_update()
commit: 86afb1cdc28f4332c6e0a1937244e0a80d4d63b1
Best regards,
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] block: add lockdep to queue_limits_commit_update()
2025-11-11 8:51 ` John Garry
@ 2025-11-11 19:00 ` Bart Van Assche
0 siblings, 0 replies; 5+ messages in thread
From: Bart Van Assche @ 2025-11-11 19:00 UTC (permalink / raw)
To: John Garry, Chaitanya Kulkarni, hch; +Cc: axboe, linux-block
On 11/11/25 12:51 AM, John Garry wrote:
> On 09/11/2025 07:44, Chaitanya Kulkarni wrote:
>> block/blk-settings.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/block/blk-settings.c b/block/blk-settings.c
>> index 78dfef117623..51401f08ce05 100644
>> --- a/block/blk-settings.c
>> +++ b/block/blk-settings.c
>> @@ -556,6 +556,8 @@ int queue_limits_commit_update(struct
>> request_queue *q,
>> {
>> int error;
>> + lockdep_assert_held(&q->limits_lock);
>> +
>
> We always unlock the mutex in this function (not shown).
>
> With your change, if CONFIG_LOCKDEP is enabled then we get the above
> warning. However, if CONFIG_DEBUG_MUTEXES was enabled, we would already
> be getting a warning.
>
> Maybe using LOCKDEP is much more preferred than DEBUG_MUTEXES.
If all goes well soon any calls to queue_limits_commit_update() without
holding q->limits_lock will result in a failed build even if
CONFIG_DEBUG_MUTEXES and CONFIG_LOCKDEP are disabled. See also
"[PATCH v3 00/35] Compiler-Based Capability- and Locking-Analysis"
(https://lore.kernel.org/lkml/20250918140451.1289454-1-elver@google.com/).
Thanks,
Bart.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-11-11 19:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-09 7:44 [PATCH] block: add lockdep to queue_limits_commit_update() Chaitanya Kulkarni
2025-11-11 8:19 ` Christoph Hellwig
2025-11-11 8:51 ` John Garry
2025-11-11 19:00 ` Bart Van Assche
2025-11-11 14:55 ` Jens Axboe
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.