| Message ID | 20190209090254.GC4865@kadam (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Silence a static checker locking warning | expand |
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.
On Mon, Feb 11, 2019 at 05:36:13PM +0100, David Sterba wrote: > 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. Lockdep was forgiving and did not deadlock, that I would notice while running the test. There's a warning in the log about the recursive locking. So we need to add the lock annotation or merge them to a single location as you suggest. And also add a test to fstests, as the subvolume related testcases are completely missing.
On Mon, Feb 11, 2019 at 05:36:13PM +0100, David Sterba wrote: > > 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. Gar. Yes. You're right. I hadn't thought about that read locks could nest. regards, dan carpenter
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 9b0e3e2d589c..039a12f51cd7 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -9423,9 +9423,8 @@ static int btrfs_rename_exchange(struct inode *old_dir, btrfs_init_log_ctx(&ctx_dest, new_inode); /* close the race window with snapshot create/destroy ioctl */ - if (old_ino == BTRFS_FIRST_FREE_OBJECTID) - down_read(&fs_info->subvol_sem); - if (new_ino == BTRFS_FIRST_FREE_OBJECTID) + if (old_ino == BTRFS_FIRST_FREE_OBJECTID || + new_ino == BTRFS_FIRST_FREE_OBJECTID) down_read(&fs_info->subvol_sem); /* @@ -9644,9 +9643,8 @@ static int btrfs_rename_exchange(struct inode *old_dir, ret = ret ? ret : ret2; } out_notrans: - if (new_ino == BTRFS_FIRST_FREE_OBJECTID) - up_read(&fs_info->subvol_sem); - if (old_ino == BTRFS_FIRST_FREE_OBJECTID) + if (new_ino == BTRFS_FIRST_FREE_OBJECTID || + old_ino == BTRFS_FIRST_FREE_OBJECTID) up_read(&fs_info->subvol_sem); return ret;
Back in the day, before commit 0b246afa62b0 ("btrfs: root->fs_info cleanup, add fs_info convenience variables") then we used to take different locks. 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. 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. I have re-written the code though to make it cleaner and to silence the static checkers. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- fs/btrfs/inode.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)