* new lockdep warning about registering a non-static key
@ 2023-01-19 15:19 Christoph Hellwig
2023-01-19 16:15 ` Filipe Manana
0 siblings, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2023-01-19 15:19 UTC (permalink / raw)
To: linux-btrfs
xfstests btrfs/168 on current misc-next:
btrfs/168 1s ... [ 32.860171] run fstests btrfs/168 at 2023-01-19 15:18:17
[ 33.224255] BTRFS info (device vdb): using crc32c (crc32c-intel) checksum
alm
[ 33.224835] BTRFS info (device vdb): using free space tree
[ 33.229739] BTRFS info (device vdb): auto enabling async discard
[ 33.442266] BTRFS: device fsid 62b088c0-2f9a-490e-a6d8-e661c167c616 devid 1
)
)[ 33.489483] BTRFS info (device vdc): using crc32c (crc32c-intel) checksum
)alm
)[ 33.490135] BTRFS info (device vdc): using free space tree
)[ 33.494829] BTRFS info (device vdc): auto enabling async discard
)[ 33.495469] BTRFS info (device vdc): checking UUID tree
)[ 33.566003] INFO: trying to register non-static key.
)[ 33.566400] The code is fine but needs lockdep annotation, or maybe
)[ 33.566813] you didn't initialize this object before use?
)[ 33.567172] turning off the locking correctness validator.
)[ 33.567527] CPU: 0 PID: 7480 Comm: btrfs Not tainted 6.2.0-rc4+ #331
)[ 33.567930] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
)rel-4
)[ 33.568665] Call Trace:
)[ 33.568830] <TASK>
)[ 33.568996] dump_stack_lvl+0x5b/0x73
)[ 33.569256] register_lock_class+0x48d/0x4a0
)[ 33.569541] ? __kmem_cache_alloc_node+0x28c/0x340
)[ 33.569854] ? kmalloc_trace+0x21/0x50
)[ 33.569871] ? btrfs_lru_cache_store+0x3e/0x1c0
)[ 33.569871] ? cache_dir_utimes+0xac/0xe0
)[ 33.569871] ? finish_inode_if_needed+0x14d1/0x17c0
)[ 33.569871] ? changed_cb+0x1c1/0xbd0
)[ 33.569871] ? btrfs_ioctl_send+0x1d6e/0x2080
)[ 33.569871] ? _btrfs_ioctl_send+0xd9/0x120
)[ 33.569871] __lock_acquire+0x6d/0x1ef0
)[ 33.569871] lock_acquire+0xd2/0x2e0
)[ 33.569871] ? mtree_insert_range+0x86/0x170
)[ 33.569871] ? btrfs_lru_cache_store+0x3e/0x1c0
)[ 33.569871] _raw_spin_lock+0x2a/0x40
)[ 33.569871] ? mtree_insert_range+0x86/0x170
)[ 33.569871] mtree_insert_range+0x86/0x170
)[ 33.569871] btrfs_lru_cache_store+0x5f/0x1c0
)[ 33.569871] cache_dir_utimes+0xac/0xe0
)[ 33.569871] finish_inode_if_needed+0x14d1/0x17c0
)[ 33.569871] ? lock_is_held_type+0xe3/0x140
)[ 33.569871] ? find_held_lock+0x2b/0x80
)[ 33.569871] changed_cb+0x1c1/0xbd0
)[ 33.569871] ? lock_is_held_type+0xe3/0x140
)[ 33.569871] ? find_held_lock+0x2b/0x80
)[ 33.569871] ? lock_release+0x145/0x2f0
)[ 33.569871] ? read_extent_buffer+0x92/0xb0
)[ 33.569871] btrfs_ioctl_send+0x1d6e/0x2080
)[ 33.569871] ? _btrfs_ioctl_send+0xf3/0x120
)[ 33.569871] ? rcu_read_lock_sched_held+0x36/0x70
)[ 33.569871] _btrfs_ioctl_send+0xd9/0x120
)[ 33.569871] ? __lock_acquire+0x397/0x1ef0
)[ 33.569871] btrfs_ioctl+0x11c1/0x3230
)[ 33.569871] ? lock_is_held_type+0xe3/0x140
)[ 33.569871] ? find_held_lock+0x2b/0x80
)[ 33.569871] ? lock_release+0x145/0x2f0
)[ 33.569871] __x64_sys_ioctl+0x80/0xb0
)[ 33.569871] do_syscall_64+0x34/0x80
)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: new lockdep warning about registering a non-static key
2023-01-19 15:19 new lockdep warning about registering a non-static key Christoph Hellwig
@ 2023-01-19 16:15 ` Filipe Manana
2023-01-19 18:51 ` Liam R. Howlett
0 siblings, 1 reply; 5+ messages in thread
From: Filipe Manana @ 2023-01-19 16:15 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-btrfs, Liam.Howlett
On Thu, Jan 19, 2023 at 04:19:29PM +0100, Christoph Hellwig wrote:
> xfstests btrfs/168 on current misc-next:
>
> btrfs/168 1s ... [ 32.860171] run fstests btrfs/168 at 2023-01-19 15:18:17
> [ 33.224255] BTRFS info (device vdb): using crc32c (crc32c-intel) checksum
> alm
> [ 33.224835] BTRFS info (device vdb): using free space tree
> [ 33.229739] BTRFS info (device vdb): auto enabling async discard
> [ 33.442266] BTRFS: device fsid 62b088c0-2f9a-490e-a6d8-e661c167c616 devid 1
> )
> )[ 33.489483] BTRFS info (device vdc): using crc32c (crc32c-intel) checksum
> )alm
> )[ 33.490135] BTRFS info (device vdc): using free space tree
> )[ 33.494829] BTRFS info (device vdc): auto enabling async discard
> )[ 33.495469] BTRFS info (device vdc): checking UUID tree
> )[ 33.566003] INFO: trying to register non-static key.
> )[ 33.566400] The code is fine but needs lockdep annotation, or maybe
> )[ 33.566813] you didn't initialize this object before use?
> )[ 33.567172] turning off the locking correctness validator.
> )[ 33.567527] CPU: 0 PID: 7480 Comm: btrfs Not tainted 6.2.0-rc4+ #331
> )[ 33.567930] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> )rel-4
> )[ 33.568665] Call Trace:
> )[ 33.568830] <TASK>
> )[ 33.568996] dump_stack_lvl+0x5b/0x73
> )[ 33.569256] register_lock_class+0x48d/0x4a0
> )[ 33.569541] ? __kmem_cache_alloc_node+0x28c/0x340
> )[ 33.569854] ? kmalloc_trace+0x21/0x50
> )[ 33.569871] ? btrfs_lru_cache_store+0x3e/0x1c0
> )[ 33.569871] ? cache_dir_utimes+0xac/0xe0
> )[ 33.569871] ? finish_inode_if_needed+0x14d1/0x17c0
> )[ 33.569871] ? changed_cb+0x1c1/0xbd0
> )[ 33.569871] ? btrfs_ioctl_send+0x1d6e/0x2080
> )[ 33.569871] ? _btrfs_ioctl_send+0xd9/0x120
> )[ 33.569871] __lock_acquire+0x6d/0x1ef0
> )[ 33.569871] lock_acquire+0xd2/0x2e0
> )[ 33.569871] ? mtree_insert_range+0x86/0x170
> )[ 33.569871] ? btrfs_lru_cache_store+0x3e/0x1c0
> )[ 33.569871] _raw_spin_lock+0x2a/0x40
> )[ 33.569871] ? mtree_insert_range+0x86/0x170
> )[ 33.569871] mtree_insert_range+0x86/0x170
> )[ 33.569871] btrfs_lru_cache_store+0x5f/0x1c0
> )[ 33.569871] cache_dir_utimes+0xac/0xe0
> )[ 33.569871] finish_inode_if_needed+0x14d1/0x17c0
> )[ 33.569871] ? lock_is_held_type+0xe3/0x140
> )[ 33.569871] ? find_held_lock+0x2b/0x80
> )[ 33.569871] changed_cb+0x1c1/0xbd0
> )[ 33.569871] ? lock_is_held_type+0xe3/0x140
> )[ 33.569871] ? find_held_lock+0x2b/0x80
> )[ 33.569871] ? lock_release+0x145/0x2f0
> )[ 33.569871] ? read_extent_buffer+0x92/0xb0
> )[ 33.569871] btrfs_ioctl_send+0x1d6e/0x2080
> )[ 33.569871] ? _btrfs_ioctl_send+0xf3/0x120
> )[ 33.569871] ? rcu_read_lock_sched_held+0x36/0x70
> )[ 33.569871] _btrfs_ioctl_send+0xd9/0x120
> )[ 33.569871] ? __lock_acquire+0x397/0x1ef0
> )[ 33.569871] btrfs_ioctl+0x11c1/0x3230
> )[ 33.569871] ? lock_is_held_type+0xe3/0x140
> )[ 33.569871] ? find_held_lock+0x2b/0x80
> )[ 33.569871] ? lock_release+0x145/0x2f0
> )[ 33.569871] __x64_sys_ioctl+0x80/0xb0
> )[ 33.569871] do_syscall_64+0x34/0x80
Oddly I don't get anything reported here (and lockdep is enabled), but it's
easy to see why it happens.
We're passing MT_FLAGS_LOCK_EXTERN to mt_init_flags(), as we don't care
about locking in btrfs' send, and because of that mt_init_flags() does not
initialize the spinlock:
(maple_tree.h)
static inline void mt_init_flags(struct maple_tree *mt, unsigned int flags)
{
mt->ma_flags = flags;
if (!mt_external_lock(mt))
spin_lock_init(&mt->ma_lock);
rcu_assign_pointer(mt->ma_root, NULL);
}
However at mtree_insert_range(), and other mtree_* functions, mtree_lock()
is called unconditionally, which always tries to lock the spin lock, as it's
defined as:
#define mtree_lock(mt) spin_lock((&(mt)->ma_lock))
But the spinlock was not initialized, as the whole point of MT_FLAGS_LOCK_EXTERN
is to skip locking (unless I misunderstood its purpose).
It seems mtree_lock() should be something like:
static inline void mtree_lock(struct maple_tree *mt)
{
if (!mt_external_lock(mt))
spin_lock((&mt->ma_lock));
}
And the equivalent for mtree_unlock() as well.
So cc'ing Liam.
Thanks.
> )
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: new lockdep warning about registering a non-static key
2023-01-19 16:15 ` Filipe Manana
@ 2023-01-19 18:51 ` Liam R. Howlett
2023-01-19 19:26 ` Filipe Manana
0 siblings, 1 reply; 5+ messages in thread
From: Liam R. Howlett @ 2023-01-19 18:51 UTC (permalink / raw)
To: Filipe Manana; +Cc: Christoph Hellwig, linux-btrfs, Matthew Wilcox (Oracle)
* Filipe Manana <fdmanana@kernel.org> [230119 11:15]:
> On Thu, Jan 19, 2023 at 04:19:29PM +0100, Christoph Hellwig wrote:
> > xfstests btrfs/168 on current misc-next:
> >
> > btrfs/168 1s ... [ 32.860171] run fstests btrfs/168 at 2023-01-19 15:18:17
> > [ 33.224255] BTRFS info (device vdb): using crc32c (crc32c-intel) checksum
> > alm
> > [ 33.224835] BTRFS info (device vdb): using free space tree
> > [ 33.229739] BTRFS info (device vdb): auto enabling async discard
> > [ 33.442266] BTRFS: device fsid 62b088c0-2f9a-490e-a6d8-e661c167c616 devid 1
> > )
> > )[ 33.489483] BTRFS info (device vdc): using crc32c (crc32c-intel) checksum
> > )alm
> > )[ 33.490135] BTRFS info (device vdc): using free space tree
> > )[ 33.494829] BTRFS info (device vdc): auto enabling async discard
> > )[ 33.495469] BTRFS info (device vdc): checking UUID tree
> > )[ 33.566003] INFO: trying to register non-static key.
> > )[ 33.566400] The code is fine but needs lockdep annotation, or maybe
> > )[ 33.566813] you didn't initialize this object before use?
> > )[ 33.567172] turning off the locking correctness validator.
> > )[ 33.567527] CPU: 0 PID: 7480 Comm: btrfs Not tainted 6.2.0-rc4+ #331
> > )[ 33.567930] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> > )rel-4
> > )[ 33.568665] Call Trace:
> > )[ 33.568830] <TASK>
> > )[ 33.568996] dump_stack_lvl+0x5b/0x73
> > )[ 33.569256] register_lock_class+0x48d/0x4a0
> > )[ 33.569541] ? __kmem_cache_alloc_node+0x28c/0x340
> > )[ 33.569854] ? kmalloc_trace+0x21/0x50
> > )[ 33.569871] ? btrfs_lru_cache_store+0x3e/0x1c0
> > )[ 33.569871] ? cache_dir_utimes+0xac/0xe0
> > )[ 33.569871] ? finish_inode_if_needed+0x14d1/0x17c0
> > )[ 33.569871] ? changed_cb+0x1c1/0xbd0
> > )[ 33.569871] ? btrfs_ioctl_send+0x1d6e/0x2080
> > )[ 33.569871] ? _btrfs_ioctl_send+0xd9/0x120
> > )[ 33.569871] __lock_acquire+0x6d/0x1ef0
> > )[ 33.569871] lock_acquire+0xd2/0x2e0
> > )[ 33.569871] ? mtree_insert_range+0x86/0x170
> > )[ 33.569871] ? btrfs_lru_cache_store+0x3e/0x1c0
> > )[ 33.569871] _raw_spin_lock+0x2a/0x40
> > )[ 33.569871] ? mtree_insert_range+0x86/0x170
> > )[ 33.569871] mtree_insert_range+0x86/0x170
> > )[ 33.569871] btrfs_lru_cache_store+0x5f/0x1c0
> > )[ 33.569871] cache_dir_utimes+0xac/0xe0
> > )[ 33.569871] finish_inode_if_needed+0x14d1/0x17c0
> > )[ 33.569871] ? lock_is_held_type+0xe3/0x140
> > )[ 33.569871] ? find_held_lock+0x2b/0x80
> > )[ 33.569871] changed_cb+0x1c1/0xbd0
> > )[ 33.569871] ? lock_is_held_type+0xe3/0x140
> > )[ 33.569871] ? find_held_lock+0x2b/0x80
> > )[ 33.569871] ? lock_release+0x145/0x2f0
> > )[ 33.569871] ? read_extent_buffer+0x92/0xb0
> > )[ 33.569871] btrfs_ioctl_send+0x1d6e/0x2080
> > )[ 33.569871] ? _btrfs_ioctl_send+0xf3/0x120
> > )[ 33.569871] ? rcu_read_lock_sched_held+0x36/0x70
> > )[ 33.569871] _btrfs_ioctl_send+0xd9/0x120
> > )[ 33.569871] ? __lock_acquire+0x397/0x1ef0
> > )[ 33.569871] btrfs_ioctl+0x11c1/0x3230
> > )[ 33.569871] ? lock_is_held_type+0xe3/0x140
> > )[ 33.569871] ? find_held_lock+0x2b/0x80
> > )[ 33.569871] ? lock_release+0x145/0x2f0
> > )[ 33.569871] __x64_sys_ioctl+0x80/0xb0
> > )[ 33.569871] do_syscall_64+0x34/0x80
>
> Oddly I don't get anything reported here (and lockdep is enabled), but it's
> easy to see why it happens.
>
> We're passing MT_FLAGS_LOCK_EXTERN to mt_init_flags(), as we don't care
> about locking in btrfs' send, and because of that mt_init_flags() does not
> initialize the spinlock:
>
> (maple_tree.h)
>
> static inline void mt_init_flags(struct maple_tree *mt, unsigned int flags)
> {
> mt->ma_flags = flags;
> if (!mt_external_lock(mt))
> spin_lock_init(&mt->ma_lock);
> rcu_assign_pointer(mt->ma_root, NULL);
> }
>
> However at mtree_insert_range(), and other mtree_* functions, mtree_lock()
> is called unconditionally, which always tries to lock the spin lock, as it's
> defined as:
>
> #define mtree_lock(mt) spin_lock((&(mt)->ma_lock))
>
> But the spinlock was not initialized, as the whole point of MT_FLAGS_LOCK_EXTERN
> is to skip locking (unless I misunderstood its purpose).
It appears that there is a misunderstanding here. The
MT_FLAGS_LOCK_EXTERN flag is to specify an external flag, not that you
don't care about or won't be locking.
> It seems mtree_lock() should be something like:
>
> static inline void mtree_lock(struct maple_tree *mt)
> {
> if (!mt_external_lock(mt))
> spin_lock((&mt->ma_lock));
> }
>
> And the equivalent for mtree_unlock() as well.
The mtree_ family of function is part of the simple interface which
handles the locking for you. If you are going to use an external lock,
then you should use the advanced interface.. but you have to use a lock.
Perhaps the mtree_lock/unlock() pair should be expanded to WARN_ON()
MT_FLAGS_LOCK_EXTERN to catch this issue?
Where did you get this idea about how the locking works? Perhaps a
documentation change is also in order.
Thanks,
Liam
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: new lockdep warning about registering a non-static key
2023-01-19 18:51 ` Liam R. Howlett
@ 2023-01-19 19:26 ` Filipe Manana
2023-01-19 20:26 ` Liam R. Howlett
0 siblings, 1 reply; 5+ messages in thread
From: Filipe Manana @ 2023-01-19 19:26 UTC (permalink / raw)
To: Liam R. Howlett; +Cc: Christoph Hellwig, linux-btrfs, Matthew Wilcox (Oracle)
On Thu, Jan 19, 2023 at 6:51 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> * Filipe Manana <fdmanana@kernel.org> [230119 11:15]:
> > On Thu, Jan 19, 2023 at 04:19:29PM +0100, Christoph Hellwig wrote:
> > > xfstests btrfs/168 on current misc-next:
> > >
> > > btrfs/168 1s ... [ 32.860171] run fstests btrfs/168 at 2023-01-19 15:18:17
> > > [ 33.224255] BTRFS info (device vdb): using crc32c (crc32c-intel) checksum
> > > alm
> > > [ 33.224835] BTRFS info (device vdb): using free space tree
> > > [ 33.229739] BTRFS info (device vdb): auto enabling async discard
> > > [ 33.442266] BTRFS: device fsid 62b088c0-2f9a-490e-a6d8-e661c167c616 devid 1
> > > )
> > > )[ 33.489483] BTRFS info (device vdc): using crc32c (crc32c-intel) checksum
> > > )alm
> > > )[ 33.490135] BTRFS info (device vdc): using free space tree
> > > )[ 33.494829] BTRFS info (device vdc): auto enabling async discard
> > > )[ 33.495469] BTRFS info (device vdc): checking UUID tree
> > > )[ 33.566003] INFO: trying to register non-static key.
> > > )[ 33.566400] The code is fine but needs lockdep annotation, or maybe
> > > )[ 33.566813] you didn't initialize this object before use?
> > > )[ 33.567172] turning off the locking correctness validator.
> > > )[ 33.567527] CPU: 0 PID: 7480 Comm: btrfs Not tainted 6.2.0-rc4+ #331
> > > )[ 33.567930] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> > > )rel-4
> > > )[ 33.568665] Call Trace:
> > > )[ 33.568830] <TASK>
> > > )[ 33.568996] dump_stack_lvl+0x5b/0x73
> > > )[ 33.569256] register_lock_class+0x48d/0x4a0
> > > )[ 33.569541] ? __kmem_cache_alloc_node+0x28c/0x340
> > > )[ 33.569854] ? kmalloc_trace+0x21/0x50
> > > )[ 33.569871] ? btrfs_lru_cache_store+0x3e/0x1c0
> > > )[ 33.569871] ? cache_dir_utimes+0xac/0xe0
> > > )[ 33.569871] ? finish_inode_if_needed+0x14d1/0x17c0
> > > )[ 33.569871] ? changed_cb+0x1c1/0xbd0
> > > )[ 33.569871] ? btrfs_ioctl_send+0x1d6e/0x2080
> > > )[ 33.569871] ? _btrfs_ioctl_send+0xd9/0x120
> > > )[ 33.569871] __lock_acquire+0x6d/0x1ef0
> > > )[ 33.569871] lock_acquire+0xd2/0x2e0
> > > )[ 33.569871] ? mtree_insert_range+0x86/0x170
> > > )[ 33.569871] ? btrfs_lru_cache_store+0x3e/0x1c0
> > > )[ 33.569871] _raw_spin_lock+0x2a/0x40
> > > )[ 33.569871] ? mtree_insert_range+0x86/0x170
> > > )[ 33.569871] mtree_insert_range+0x86/0x170
> > > )[ 33.569871] btrfs_lru_cache_store+0x5f/0x1c0
> > > )[ 33.569871] cache_dir_utimes+0xac/0xe0
> > > )[ 33.569871] finish_inode_if_needed+0x14d1/0x17c0
> > > )[ 33.569871] ? lock_is_held_type+0xe3/0x140
> > > )[ 33.569871] ? find_held_lock+0x2b/0x80
> > > )[ 33.569871] changed_cb+0x1c1/0xbd0
> > > )[ 33.569871] ? lock_is_held_type+0xe3/0x140
> > > )[ 33.569871] ? find_held_lock+0x2b/0x80
> > > )[ 33.569871] ? lock_release+0x145/0x2f0
> > > )[ 33.569871] ? read_extent_buffer+0x92/0xb0
> > > )[ 33.569871] btrfs_ioctl_send+0x1d6e/0x2080
> > > )[ 33.569871] ? _btrfs_ioctl_send+0xf3/0x120
> > > )[ 33.569871] ? rcu_read_lock_sched_held+0x36/0x70
> > > )[ 33.569871] _btrfs_ioctl_send+0xd9/0x120
> > > )[ 33.569871] ? __lock_acquire+0x397/0x1ef0
> > > )[ 33.569871] btrfs_ioctl+0x11c1/0x3230
> > > )[ 33.569871] ? lock_is_held_type+0xe3/0x140
> > > )[ 33.569871] ? find_held_lock+0x2b/0x80
> > > )[ 33.569871] ? lock_release+0x145/0x2f0
> > > )[ 33.569871] __x64_sys_ioctl+0x80/0xb0
> > > )[ 33.569871] do_syscall_64+0x34/0x80
> >
> > Oddly I don't get anything reported here (and lockdep is enabled), but it's
> > easy to see why it happens.
> >
> > We're passing MT_FLAGS_LOCK_EXTERN to mt_init_flags(), as we don't care
> > about locking in btrfs' send, and because of that mt_init_flags() does not
> > initialize the spinlock:
> >
> > (maple_tree.h)
> >
> > static inline void mt_init_flags(struct maple_tree *mt, unsigned int flags)
> > {
> > mt->ma_flags = flags;
> > if (!mt_external_lock(mt))
> > spin_lock_init(&mt->ma_lock);
> > rcu_assign_pointer(mt->ma_root, NULL);
> > }
> >
> > However at mtree_insert_range(), and other mtree_* functions, mtree_lock()
> > is called unconditionally, which always tries to lock the spin lock, as it's
> > defined as:
> >
> > #define mtree_lock(mt) spin_lock((&(mt)->ma_lock))
> >
> > But the spinlock was not initialized, as the whole point of MT_FLAGS_LOCK_EXTERN
> > is to skip locking (unless I misunderstood its purpose).
>
> It appears that there is a misunderstanding here. The
> MT_FLAGS_LOCK_EXTERN flag is to specify an external flag, not that you
> don't care about or won't be locking.
>
>
> > It seems mtree_lock() should be something like:
> >
> > static inline void mtree_lock(struct maple_tree *mt)
> > {
> > if (!mt_external_lock(mt))
> > spin_lock((&mt->ma_lock));
> > }
> >
> > And the equivalent for mtree_unlock() as well.
>
> The mtree_ family of function is part of the simple interface which
> handles the locking for you. If you are going to use an external lock,
> then you should use the advanced interface.. but you have to use a lock.
>
> Perhaps the mtree_lock/unlock() pair should be expanded to WARN_ON()
> MT_FLAGS_LOCK_EXTERN to catch this issue?
>
> Where did you get this idea about how the locking works? Perhaps a
> documentation change is also in order.
Well, I don't remember exactly where I got the idea, but I suppose it
was from reading maple_tree.h:
1) the definition of mt_init_flags() doesn't initialize the spin lock
if MT_FLAGS_LOCK_EXTERN is set;
2) the comment for MT_FLAGS_LOCK_EXTERN says "mt_lock is not used"
So I guess I inferred that the use of that flag implied the maple tree
code would not use the spin lock at all.
Ok, I'll drop the incorrect use of the flag in btrfs then. Thanks.
>
> Thanks,
> Liam
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: new lockdep warning about registering a non-static key
2023-01-19 19:26 ` Filipe Manana
@ 2023-01-19 20:26 ` Liam R. Howlett
0 siblings, 0 replies; 5+ messages in thread
From: Liam R. Howlett @ 2023-01-19 20:26 UTC (permalink / raw)
To: Filipe Manana; +Cc: Christoph Hellwig, linux-btrfs, Matthew Wilcox (Oracle)
...
> > > > )[ 33.566003] INFO: trying to register non-static key.
> > > > )[ 33.566400] The code is fine but needs lockdep annotation, or maybe
> > > > )[ 33.566813] you didn't initialize this object before use?
> > > > )[ 33.567172] turning off the locking correctness validator.
> > > > )[ 33.567527] CPU: 0 PID: 7480 Comm: btrfs Not tainted 6.2.0-rc4+ #331
> > > > )[ 33.567930] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> > > > )rel-4
> > > > )[ 33.568665] Call Trace:
> > > > )[ 33.568830] <TASK>
> > > > )[ 33.568996] dump_stack_lvl+0x5b/0x73
> > > > )[ 33.569256] register_lock_class+0x48d/0x4a0
> > > > )[ 33.569541] ? __kmem_cache_alloc_node+0x28c/0x340
> > > > )[ 33.569854] ? kmalloc_trace+0x21/0x50
> > > > )[ 33.569871] ? btrfs_lru_cache_store+0x3e/0x1c0
> > > > )[ 33.569871] ? cache_dir_utimes+0xac/0xe0
> > > > )[ 33.569871] ? finish_inode_if_needed+0x14d1/0x17c0
> > > > )[ 33.569871] ? changed_cb+0x1c1/0xbd0
> > > > )[ 33.569871] ? btrfs_ioctl_send+0x1d6e/0x2080
> > > > )[ 33.569871] ? _btrfs_ioctl_send+0xd9/0x120
> > > > )[ 33.569871] __lock_acquire+0x6d/0x1ef0
> > > > )[ 33.569871] lock_acquire+0xd2/0x2e0
> > > > )[ 33.569871] ? mtree_insert_range+0x86/0x170
> > > > )[ 33.569871] ? btrfs_lru_cache_store+0x3e/0x1c0
> > > > )[ 33.569871] _raw_spin_lock+0x2a/0x40
> > > > )[ 33.569871] ? mtree_insert_range+0x86/0x170
> > > > )[ 33.569871] mtree_insert_range+0x86/0x170
> > > > )[ 33.569871] btrfs_lru_cache_store+0x5f/0x1c0
> > > > )[ 33.569871] cache_dir_utimes+0xac/0xe0
> > > > )[ 33.569871] finish_inode_if_needed+0x14d1/0x17c0
> > > > )[ 33.569871] ? lock_is_held_type+0xe3/0x140
> > > > )[ 33.569871] ? find_held_lock+0x2b/0x80
> > > > )[ 33.569871] changed_cb+0x1c1/0xbd0
> > > > )[ 33.569871] ? lock_is_held_type+0xe3/0x140
> > > > )[ 33.569871] ? find_held_lock+0x2b/0x80
> > > > )[ 33.569871] ? lock_release+0x145/0x2f0
> > > > )[ 33.569871] ? read_extent_buffer+0x92/0xb0
> > > > )[ 33.569871] btrfs_ioctl_send+0x1d6e/0x2080
> > > > )[ 33.569871] ? _btrfs_ioctl_send+0xf3/0x120
> > > > )[ 33.569871] ? rcu_read_lock_sched_held+0x36/0x70
> > > > )[ 33.569871] _btrfs_ioctl_send+0xd9/0x120
> > > > )[ 33.569871] ? __lock_acquire+0x397/0x1ef0
> > > > )[ 33.569871] btrfs_ioctl+0x11c1/0x3230
> > > > )[ 33.569871] ? lock_is_held_type+0xe3/0x140
> > > > )[ 33.569871] ? find_held_lock+0x2b/0x80
> > > > )[ 33.569871] ? lock_release+0x145/0x2f0
> > > > )[ 33.569871] __x64_sys_ioctl+0x80/0xb0
> > > > )[ 33.569871] do_syscall_64+0x34/0x80
> > >
> > > Oddly I don't get anything reported here (and lockdep is enabled), but it's
> > > easy to see why it happens.
...
> > > But the spinlock was not initialized, as the whole point of MT_FLAGS_LOCK_EXTERN
> > > is to skip locking (unless I misunderstood its purpose).
> >
> > It appears that there is a misunderstanding here. The
> > MT_FLAGS_LOCK_EXTERN flag is to specify an external flag, not that you
> > don't care about or won't be locking.
> >
> >
...
> >
> > The mtree_ family of function is part of the simple interface which
> > handles the locking for you. If you are going to use an external lock,
> > then you should use the advanced interface.. but you have to use a lock.
> >
> > Perhaps the mtree_lock/unlock() pair should be expanded to WARN_ON()
> > MT_FLAGS_LOCK_EXTERN to catch this issue?
The more concerning part is that you were able to call these functions
without an obvious issue on your end. I suspect that the maple tree
mt->ma_lock was set to something lockdep didn't mind seeing, but was
NULL or, at least, something lockdep did not like for Christoph.
> >
> > Where did you get this idea about how the locking works? Perhaps a
> > documentation change is also in order.
>
> Well, I don't remember exactly where I got the idea, but I suppose it
> was from reading maple_tree.h:
>
> 1) the definition of mt_init_flags() doesn't initialize the spin lock
> if MT_FLAGS_LOCK_EXTERN is set;
> 2) the comment for MT_FLAGS_LOCK_EXTERN says "mt_lock is not used"
>
> So I guess I inferred that the use of that flag implied the maple tree
> code would not use the spin lock at all.
>
> Ok, I'll drop the incorrect use of the flag in btrfs then. Thanks.
Thanks. It's often hard to see such things as the author. I will look
into how to avoid this from going undetected and update the header
comments.
Regards,
Liam
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-01-20 5:23 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-19 15:19 new lockdep warning about registering a non-static key Christoph Hellwig
2023-01-19 16:15 ` Filipe Manana
2023-01-19 18:51 ` Liam R. Howlett
2023-01-19 19:26 ` Filipe Manana
2023-01-19 20:26 ` Liam R. Howlett
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.