[PATCH 0/5] More lockdep annotations

[PATCH 0/5] More lockdep annotations

[David Sterba]

Lockdep annotations are better than comments about necessary locks.

David Sterba (5):
  btrfs: tests: add locks around add_extent_mapping
  btrfs: assert extent map tree lock in add_extent_mapping
  btrfs: assert tree mod log lock in __tree_mod_log_insert
  btrfs: assert delayed ref lock in btrfs_find_delayed_ref_head
  btrfs: assert bio list lock in merge_rbio

 fs/btrfs/ctree.c                  |  4 ++--
 fs/btrfs/delayed-ref.c            |  7 ++++---
 fs/btrfs/extent_map.c             |  2 ++
 fs/btrfs/raid56.c                 |  4 ++--
 fs/btrfs/tests/extent-map-tests.c | 22 ++++++++++++++++++++++
 5 files changed, 32 insertions(+), 7 deletions(-)

-- 
2.21.0

[PATCH 1/5] btrfs: tests: add locks around add_extent_mapping

[David Sterba]

There are no concerns about locking during the selftests so the locks
are not necessary, but following patches will add lockdep assertions to
add_extent_mapping so this is needed in tests too.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/tests/extent-map-tests.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/fs/btrfs/tests/extent-map-tests.c b/fs/btrfs/tests/extent-map-tests.c
index 87aeabe9d610..4a7f796c9900 100644
--- a/fs/btrfs/tests/extent-map-tests.c
+++ b/fs/btrfs/tests/extent-map-tests.c
@@ -66,7 +66,9 @@ static int test_case_1(struct btrfs_fs_info *fs_info,
 	em->len = SZ_16K;
 	em->block_start = 0;
 	em->block_len = SZ_16K;
+	write_lock(&em_tree->lock);
 	ret = add_extent_mapping(em_tree, em, 0);
+	write_unlock(&em_tree->lock);
 	if (ret < 0) {
 		test_err("cannot add extent range [0, 16K)");
 		goto out;
@@ -85,7 +87,9 @@ static int test_case_1(struct btrfs_fs_info *fs_info,
 	em->len = SZ_4K;
 	em->block_start = SZ_32K; /* avoid merging */
 	em->block_len = SZ_4K;
+	write_lock(&em_tree->lock);
 	ret = add_extent_mapping(em_tree, em, 0);
+	write_unlock(&em_tree->lock);
 	if (ret < 0) {
 		test_err("cannot add extent range [16K, 20K)");
 		goto out;
@@ -104,7 +108,9 @@ static int test_case_1(struct btrfs_fs_info *fs_info,
 	em->len = len;
 	em->block_start = start;
 	em->block_len = len;
+	write_lock(&em_tree->lock);
 	ret = btrfs_add_extent_mapping(fs_info, em_tree, &em, em->start, em->len);
+	write_unlock(&em_tree->lock);
 	if (ret) {
 		test_err("case1 [%llu %llu]: ret %d", start, start + len, ret);
 		goto out;
@@ -148,7 +154,9 @@ static int test_case_2(struct btrfs_fs_info *fs_info,
 	em->len = SZ_1K;
 	em->block_start = EXTENT_MAP_INLINE;
 	em->block_len = (u64)-1;
+	write_lock(&em_tree->lock);
 	ret = add_extent_mapping(em_tree, em, 0);
+	write_unlock(&em_tree->lock);
 	if (ret < 0) {
 		test_err("cannot add extent range [0, 1K)");
 		goto out;
@@ -167,7 +175,9 @@ static int test_case_2(struct btrfs_fs_info *fs_info,
 	em->len = SZ_4K;
 	em->block_start = SZ_4K;
 	em->block_len = SZ_4K;
+	write_lock(&em_tree->lock);
 	ret = add_extent_mapping(em_tree, em, 0);
+	write_unlock(&em_tree->lock);
 	if (ret < 0) {
 		test_err("cannot add extent range [4K, 8K)");
 		goto out;
@@ -186,7 +196,9 @@ static int test_case_2(struct btrfs_fs_info *fs_info,
 	em->len = SZ_1K;
 	em->block_start = EXTENT_MAP_INLINE;
 	em->block_len = (u64)-1;
+	write_lock(&em_tree->lock);
 	ret = btrfs_add_extent_mapping(fs_info, em_tree, &em, em->start, em->len);
+	write_unlock(&em_tree->lock);
 	if (ret) {
 		test_err("case2 [0 1K]: ret %d", ret);
 		goto out;
@@ -225,7 +237,9 @@ static int __test_case_3(struct btrfs_fs_info *fs_info,
 	em->len = SZ_4K;
 	em->block_start = SZ_4K;
 	em->block_len = SZ_4K;
+	write_lock(&em_tree->lock);
 	ret = add_extent_mapping(em_tree, em, 0);
+	write_unlock(&em_tree->lock);
 	if (ret < 0) {
 		test_err("cannot add extent range [4K, 8K)");
 		goto out;
@@ -244,7 +258,9 @@ static int __test_case_3(struct btrfs_fs_info *fs_info,
 	em->len = SZ_16K;
 	em->block_start = 0;
 	em->block_len = SZ_16K;
+	write_lock(&em_tree->lock);
 	ret = btrfs_add_extent_mapping(fs_info, em_tree, &em, start, len);
+	write_unlock(&em_tree->lock);
 	if (ret) {
 		test_err("case3 [0x%llx 0x%llx): ret %d",
 			 start, start + len, ret);
@@ -320,7 +336,9 @@ static int __test_case_4(struct btrfs_fs_info *fs_info,
 	em->len = SZ_8K;
 	em->block_start = 0;
 	em->block_len = SZ_8K;
+	write_lock(&em_tree->lock);
 	ret = add_extent_mapping(em_tree, em, 0);
+	write_unlock(&em_tree->lock);
 	if (ret < 0) {
 		test_err("cannot add extent range [0, 8K)");
 		goto out;
@@ -339,7 +357,9 @@ static int __test_case_4(struct btrfs_fs_info *fs_info,
 	em->len = 24 * SZ_1K;
 	em->block_start = SZ_16K; /* avoid merging */
 	em->block_len = 24 * SZ_1K;
+	write_lock(&em_tree->lock);
 	ret = add_extent_mapping(em_tree, em, 0);
+	write_unlock(&em_tree->lock);
 	if (ret < 0) {
 		test_err("cannot add extent range [8K, 32K)");
 		goto out;
@@ -357,7 +377,9 @@ static int __test_case_4(struct btrfs_fs_info *fs_info,
 	em->len = SZ_32K;
 	em->block_start = 0;
 	em->block_len = SZ_32K;
+	write_lock(&em_tree->lock);
 	ret = btrfs_add_extent_mapping(fs_info, em_tree, &em, start, len);
+	write_unlock(&em_tree->lock);
 	if (ret) {
 		test_err("case4 [0x%llx 0x%llx): ret %d",
 			 start, len, ret);
-- 
2.21.0

[PATCH 2/5] btrfs: assert extent map tree lock in add_extent_mapping

[David Sterba]

As add_extent_mapping is called from several functions, let's add the
lock annotation. The tree is going to be modified so it must be the
exclusive lock.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent_map.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index 9558d79faf1e..a73af4159495 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -384,6 +384,8 @@ int add_extent_mapping(struct extent_map_tree *tree,
 {
 	int ret = 0;
 
+	lockdep_assert_held_exclusive(&tree->lock);
+
 	ret = tree_insert(&tree->map, em);
 	if (ret)
 		goto out;
-- 
2.21.0

[PATCH 3/5] btrfs: assert tree mod log lock in __tree_mod_log_insert

[David Sterba]

The tree is going to be modified so it must be the exclusive lock.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 5df76c17775a..99a585ede79d 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -376,8 +376,6 @@ void btrfs_put_tree_mod_seq(struct btrfs_fs_info *fs_info,
  * The 'start address' is the logical address of the *new* root node
  * for root replace operations, or the logical address of the affected
  * block for all other operations.
- *
- * Note: must be called with write lock for fs_info::tree_mod_log_lock.
  */
 static noinline int
 __tree_mod_log_insert(struct btrfs_fs_info *fs_info, struct tree_mod_elem *tm)
@@ -387,6 +385,8 @@ __tree_mod_log_insert(struct btrfs_fs_info *fs_info, struct tree_mod_elem *tm)
 	struct rb_node *parent = NULL;
 	struct tree_mod_elem *cur;
 
+	lockdep_assert_held_exclusive(&fs_info->tree_mod_log_lock);
+
 	tm->seq = btrfs_inc_tree_mod_seq(fs_info);
 
 	tm_root = &fs_info->tree_mod_log;
-- 
2.21.0

[PATCH 4/5] btrfs: assert delayed ref lock in btrfs_find_delayed_ref_head

[David Sterba]

Turn the comment about required lock into an assertion.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/delayed-ref.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index a73fc23e2961..a94fae897b3f 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -957,13 +957,14 @@ int btrfs_add_delayed_extent_op(struct btrfs_trans_handle *trans,
 }
 
 /*
- * this does a simple search for the head node for a given extent.
- * It must be called with the delayed ref spinlock held, and it returns
- * the head node if any where found, or NULL if not.
+ * This does a simple search for the head node for a given extent.  Returns the
+ * head node if found, or NULL if not.
  */
 struct btrfs_delayed_ref_head *
 btrfs_find_delayed_ref_head(struct btrfs_delayed_ref_root *delayed_refs, u64 bytenr)
 {
+	lockdep_assert_held(&delayed_refs->lock);
+
 	return find_ref_head(delayed_refs, bytenr, false);
 }
 
-- 
2.21.0

[PATCH 5/5] btrfs: assert bio list lock in merge_rbio

[David Sterba]

Turn the comment about required lock into an assertion.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/raid56.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 67a6f7d47402..505979d867e0 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -310,12 +310,12 @@ static void steal_rbio(struct btrfs_raid_bio *src, struct btrfs_raid_bio *dest)
  * merging means we take the bio_list from the victim and
  * splice it into the destination.  The victim should
  * be discarded afterwards.
- *
- * must be called with dest->rbio_list_lock held
  */
 static void merge_rbio(struct btrfs_raid_bio *dest,
 		       struct btrfs_raid_bio *victim)
 {
+	lockdep_assert_held(&dest->bio_list_lock);
+
 	bio_list_merge(&dest->bio_list, &victim->bio_list);
 	dest->bio_list_bytes += victim->bio_list_bytes;
 	dest->generic_bio_cnt += victim->generic_bio_cnt;
-- 
2.21.0

Re: [PATCH 0/5] More lockdep annotations

[Nikolay Borisov]

On 31.05.19 г. 20:01 ч., David Sterba wrote:
> Lockdep annotations are better than comments about necessary locks.
> 
> David Sterba (5):
>   btrfs: tests: add locks around add_extent_mapping
>   btrfs: assert extent map tree lock in add_extent_mapping
>   btrfs: assert tree mod log lock in __tree_mod_log_insert
>   btrfs: assert delayed ref lock in btrfs_find_delayed_ref_head
>   btrfs: assert bio list lock in merge_rbio
> 
>  fs/btrfs/ctree.c                  |  4 ++--
>  fs/btrfs/delayed-ref.c            |  7 ++++---
>  fs/btrfs/extent_map.c             |  2 ++
>  fs/btrfs/raid56.c                 |  4 ++--
>  fs/btrfs/tests/extent-map-tests.c | 22 ++++++++++++++++++++++
>  5 files changed, 32 insertions(+), 7 deletions(-)
> 


For the whole series :

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH 0/5] More lockdep annotations

[David Sterba]

On Wed, Jun 05, 2019 at 10:34:24AM +0300, Nikolay Borisov wrote:
> 
> 
> On 31.05.19 г. 20:01 ч., David Sterba wrote:
> > Lockdep annotations are better than comments about necessary locks.
> > 
> > David Sterba (5):
> >   btrfs: tests: add locks around add_extent_mapping
> >   btrfs: assert extent map tree lock in add_extent_mapping
> >   btrfs: assert tree mod log lock in __tree_mod_log_insert
> >   btrfs: assert delayed ref lock in btrfs_find_delayed_ref_head
> >   btrfs: assert bio list lock in merge_rbio
> > 
> >  fs/btrfs/ctree.c                  |  4 ++--
> >  fs/btrfs/delayed-ref.c            |  7 ++++---
> >  fs/btrfs/extent_map.c             |  2 ++
> >  fs/btrfs/raid56.c                 |  4 ++--
> >  fs/btrfs/tests/extent-map-tests.c | 22 ++++++++++++++++++++++
> >  5 files changed, 32 insertions(+), 7 deletions(-)
> > 
> 
> 
> For the whole series :
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Thanks, series moved to misc-next.