Linux-BTRFS Archive on lore.kernel.org
 help / Atom feed
* [PATCH] btrfs: Silence a static checker locking warning
@ 2019-02-09  9:02 Dan Carpenter
  2019-02-11 16:36 ` David Sterba
  0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2019-02-09  9:02 UTC (permalink / raw)
  To: Chris Mason, Jeff Mahoney
  Cc: Josef Bacik, David Sterba, linux-btrfs, kernel-janitors

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(-)

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;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] btrfs: Silence a static checker locking warning
  2019-02-09  9:02 [PATCH] btrfs: Silence a static checker locking warning Dan Carpenter
@ 2019-02-11 16:36 ` David Sterba
  2019-02-11 17:07   ` David Sterba
  2019-02-11 18:42   ` Dan Carpenter
  0 siblings, 2 replies; 4+ messages in thread
From: David Sterba @ 2019-02-11 16:36 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Chris Mason, Jeff Mahoney, Josef Bacik, kernel-janitors, linux-btrfs

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] btrfs: Silence a static checker locking warning
  2019-02-11 16:36 ` David Sterba
@ 2019-02-11 17:07   ` David Sterba
  2019-02-11 18:42   ` Dan Carpenter
  1 sibling, 0 replies; 4+ messages in thread
From: David Sterba @ 2019-02-11 17:07 UTC (permalink / raw)
  To: dsterba, Dan Carpenter, Chris Mason, Jeff Mahoney, Josef Bacik,
	kernel-janitors, linux-btrfs

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] btrfs: Silence a static checker locking warning
  2019-02-11 16:36 ` David Sterba
  2019-02-11 17:07   ` David Sterba
@ 2019-02-11 18:42   ` Dan Carpenter
  1 sibling, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2019-02-11 18:42 UTC (permalink / raw)
  To: dsterba, Chris Mason, Jeff Mahoney, Josef Bacik, kernel-janitors,
	linux-btrfs

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-09  9:02 [PATCH] btrfs: Silence a static checker locking warning Dan Carpenter
2019-02-11 16:36 ` David Sterba
2019-02-11 17:07   ` David Sterba
2019-02-11 18:42   ` Dan Carpenter

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org linux-btrfs@archiver.kernel.org
	public-inbox-index linux-btrfs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox