Re: new lockdep warning about registering a non-static key

[Filipe Manana <fdmanana@kernel.org>]

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.


> )