Re: [PATCH] btrfs: Silence a static checker locking warning

[David Sterba <dsterba@suse.cz>]

On Sat, Feb 09, 2019 at 12:02:55PM +0300, Dan Carpenter wrote:
> Back in the day, before commit 0b246afa62b0 ("btrfs: root->fs_info
> cleanup, add fs_info convenience variables") then we used to take
> different locks.

Nope, it's the same per-filesystem lock, just the old code got there
in two different ways (ie. two subvolume roots).

> But now it's just one lock and the static checkers
> think we can call down_read(&fs_info->subvol_sem); twice in a row which
> would lead to a deadlock.

Why? It's read side of a semaphore.

> That code is several years old now so presumably both (old_ino =
> BTRFS_FIRST_FREE_OBJECTID) and (new_ino = BTRFS_FIRST_FREE_OBJECTID)
> conditions can't be true at the same time or the bug would have showed
> up in testing.

Why do you think it's a bug? If you are sure that there's a bug we've
overlooked, please state it in the changelog, the rationale you've
provided is very vague.

And I believe also wrong. The rename-exchange cannot work between two
subvolumes, but we still can cross-rename two subvolumes. In this
example hierarchy:

/
- subvol1 (inode number 256, ie. BTRFS_FIRST_FREE_OBJECTID)
  - file1
- subvol2 (inode number 256, ie. BTRFS_FIRST_FREE_OBJECTID)
  - file2

btrfs_rename_exchange leads to this:

/
- subvol1
  - file2
- subvol2
  - file1

There's no common tool that supports renameat2, so I'm using the one
from fstests/src/renameat2.c to verify that, and it does indeed work as
expected.

> I have re-written the code though to make it cleaner and
> to silence the static checkers.

Maybe there's something new the static checker needs to learn.