[PATCH 0/2] btrfs: Annotate transaction wait events with

[PATCH 0/2] btrfs: Annotate transaction wait events with

[Ioannis Angelakopoulos]

With this patch series we annotate wait events in btrfs transactions
with lockdep to catch deadlocks involving these wait events.

Recently the btrfs developers fixed a non trivial deadlock involving
wait events (https://lore.kernel.org/linux-btrfs/20220614131413.GJ20633@twin.jikos.cz/)

Currently lockdep is unable to catch these deadlocks since it does not
support wait events by default.

With our lockdep annotations we train lockdep to track these wait events
and catch more potential deadlocks.

Specifically, we annotate two wait events in fs/btrfs/transaction.c:
  1) The num_writers wait event
  2) The num_extwriters wait event

Ioannis Angelakopoulos (2):
  btrfs: Add a lockdep model for the num_writers wait event
  btrfs: Add a lockdep annotation for the num_extwriters wait event

 fs/btrfs/ctree.h       | 15 ++++++++++++
 fs/btrfs/disk-io.c     | 10 ++++++++
 fs/btrfs/transaction.c | 55 +++++++++++++++++++++++++++++++++++++++---
 3 files changed, 76 insertions(+), 4 deletions(-)

-- 
2.30.2

[PATCH 1/2] btrfs: Add a lockdep model for the num_writers wait event

[Ioannis Angelakopoulos]

Annotate the num_writers wait event in fs/btrfs/transaction.c with lockdep
in order to catch deadlocks involving this wait event.

Use a read/write lockdep map for the annotation. A thread starting/joining
the transaction acquires the map as a reader when it increments
cur_trans->num_writers and it acquires the map as a writer before it
blocks on the wait event.


Signed-off-by: Ioannis Angelakopoulos <iangelak@fb.com>
---
 fs/btrfs/ctree.h       | 14 ++++++++++++++
 fs/btrfs/disk-io.c     |  6 ++++++
 fs/btrfs/transaction.c | 39 +++++++++++++++++++++++++++++++++++----
 3 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 4e2569f84aab..160c22391cb5 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1091,6 +1091,8 @@ struct btrfs_fs_info {
 	/* Updates are not protected by any lock */
 	struct btrfs_commit_stats commit_stats;
 
+	struct lockdep_map btrfs_trans_num_writers_map;
+
 #ifdef CONFIG_BTRFS_FS_REF_VERIFY
 	spinlock_t ref_verify_lock;
 	struct rb_root block_tree;
@@ -1172,6 +1174,18 @@ enum {
 	BTRFS_ROOT_UNFINISHED_DROP,
 };
 
+#define btrfs_might_wait_for_event(b, lock) \
+	do { \
+		rwsem_acquire(&b->lock##_map, 0, 0, _THIS_IP_); \
+		rwsem_release(&b->lock##_map, _THIS_IP_); \
+	} while (0)
+
+#define btrfs_lockdep_acquire(b, lock) \
+	rwsem_acquire_read(&b->lock##_map, 0, 0, _THIS_IP_)
+
+#define btrfs_lockdep_release(b, lock) \
+	rwsem_release(&b->lock##_map, _THIS_IP_)
+
 static inline void btrfs_wake_unfinished_drop(struct btrfs_fs_info *fs_info)
 {
 	clear_and_wake_up_bit(BTRFS_FS_UNFINISHED_DROPS, &fs_info->flags);
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 76835394a61b..4061886024de 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3029,6 +3029,8 @@ static int __cold init_tree_roots(struct btrfs_fs_info *fs_info)
 
 void btrfs_init_fs_info(struct btrfs_fs_info *fs_info)
 {
+	static struct lock_class_key btrfs_trans_num_writers_key;
+
 	xa_init_flags(&fs_info->fs_roots, GFP_ATOMIC);
 	xa_init_flags(&fs_info->extent_buffers, GFP_ATOMIC);
 	INIT_LIST_HEAD(&fs_info->trans_list);
@@ -3057,6 +3059,10 @@ void btrfs_init_fs_info(struct btrfs_fs_info *fs_info)
 	mutex_init(&fs_info->zoned_data_reloc_io_lock);
 	seqlock_init(&fs_info->profiles_lock);
 
+	lockdep_init_map(&fs_info->btrfs_trans_num_writers_map,
+					 "btrfs_trans_num_writers",
+					 &btrfs_trans_num_writers_key, 0);
+
 	INIT_LIST_HEAD(&fs_info->dirty_cowonly_roots);
 	INIT_LIST_HEAD(&fs_info->space_info);
 	INIT_LIST_HEAD(&fs_info->tree_mod_seq_list);
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 0a50d5746f6f..7ef24e1f1446 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -313,6 +313,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
 		atomic_inc(&cur_trans->num_writers);
 		extwriter_counter_inc(cur_trans, type);
 		spin_unlock(&fs_info->trans_lock);
+		btrfs_lockdep_acquire(fs_info, btrfs_trans_num_writers);
 		return 0;
 	}
 	spin_unlock(&fs_info->trans_lock);
@@ -334,16 +335,20 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
 	if (!cur_trans)
 		return -ENOMEM;
 
+	btrfs_lockdep_acquire(fs_info, btrfs_trans_num_writers);
+
 	spin_lock(&fs_info->trans_lock);
 	if (fs_info->running_transaction) {
 		/*
 		 * someone started a transaction after we unlocked.  Make sure
 		 * to redo the checks above
 		 */
+		btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
 		kfree(cur_trans);
 		goto loop;
 	} else if (BTRFS_FS_ERROR(fs_info)) {
 		spin_unlock(&fs_info->trans_lock);
+		btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
 		kfree(cur_trans);
 		return -EROFS;
 	}
@@ -1020,6 +1025,9 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
 	extwriter_counter_dec(cur_trans, trans->type);
 
 	cond_wake_up(&cur_trans->writer_wait);
+
+	btrfs_lockdep_release(info, btrfs_trans_num_writers);
+
 	btrfs_put_transaction(cur_trans);
 
 	if (current->journal_info == trans)
@@ -1980,6 +1988,12 @@ static void cleanup_transaction(struct btrfs_trans_handle *trans, int err)
 	if (cur_trans == fs_info->running_transaction) {
 		cur_trans->state = TRANS_STATE_COMMIT_DOING;
 		spin_unlock(&fs_info->trans_lock);
+
+		/*
+		 * The thread has already released the lockdep map as reader already in
+		 * btrfs_commit_transaction().
+		 */
+		btrfs_might_wait_for_event(fs_info, btrfs_trans_num_writers);
 		wait_event(cur_trans->writer_wait,
 			   atomic_read(&cur_trans->num_writers) == 1);
 
@@ -2207,8 +2221,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 			ret = READ_ONCE(prev_trans->aborted);
 
 			btrfs_put_transaction(prev_trans);
-			if (ret)
+			if (ret) {
+				btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
 				goto cleanup_transaction;
+			}
 		} else {
 			spin_unlock(&fs_info->trans_lock);
 		}
@@ -2222,6 +2238,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 		 */
 		if (BTRFS_FS_ERROR(fs_info)) {
 			ret = -EROFS;
+			btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
 			goto cleanup_transaction;
 		}
 	}
@@ -2235,20 +2252,26 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	extwriter_counter_dec(cur_trans, trans->type);
 
 	ret = btrfs_start_delalloc_flush(fs_info);
-	if (ret)
+	if (ret) {
+		btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
 		goto cleanup_transaction;
+	}
 
 	ret = btrfs_run_delayed_items(trans);
-	if (ret)
+	if (ret) {
+		btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
 		goto cleanup_transaction;
+	}
 
 	wait_event(cur_trans->writer_wait,
 		   extwriter_counter_read(cur_trans) == 0);
 
 	/* some pending stuffs might be added after the previous flush. */
 	ret = btrfs_run_delayed_items(trans);
-	if (ret)
+	if (ret) {
+		btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
 		goto cleanup_transaction;
+	}
 
 	btrfs_wait_delalloc_flush(fs_info);
 
@@ -2270,6 +2293,14 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	add_pending_snapshot(trans);
 	cur_trans->state = TRANS_STATE_COMMIT_DOING;
 	spin_unlock(&fs_info->trans_lock);
+
+	/*
+	 * The thread has started/joined the transaction thus it holds the lockdep
+	 * map as a reader. It has to release it before acquiring the lockdep map
+	 * as a writer.
+	 */
+	btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
+	btrfs_might_wait_for_event(fs_info, btrfs_trans_num_writers);
 	wait_event(cur_trans->writer_wait,
 		   atomic_read(&cur_trans->num_writers) == 1);
 
-- 
2.30.2

[PATCH 2/2] btrfs: Add a lockdep model for the num_extwriters wait event

[Ioannis Angelakopoulos]

Similarly to the num_writers wait event in fs/btrfs/transaction.c add a
lockdep annotation for the num_extwriters wait event.

Use a read/write lockdep map for the annotation. A thread starting/joining
the transaction acquires the map as a reader when it increments
cur_trans->num_writers and it acquires the map as a writer before it
blocks on the wait event.

Signed-off-by: Ioannis Angelakopoulos <iangelak@fb.com>
---
 fs/btrfs/ctree.h       |  1 +
 fs/btrfs/disk-io.c     |  4 ++++
 fs/btrfs/transaction.c | 16 ++++++++++++++++
 3 files changed, 21 insertions(+)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 160c22391cb5..0d94786a2c43 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1092,6 +1092,7 @@ struct btrfs_fs_info {
 	struct btrfs_commit_stats commit_stats;
 
 	struct lockdep_map btrfs_trans_num_writers_map;
+	struct lockdep_map btrfs_trans_num_extwriters_map;
 
 #ifdef CONFIG_BTRFS_FS_REF_VERIFY
 	spinlock_t ref_verify_lock;
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 4061886024de..cd47fb170b05 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3030,6 +3030,7 @@ static int __cold init_tree_roots(struct btrfs_fs_info *fs_info)
 void btrfs_init_fs_info(struct btrfs_fs_info *fs_info)
 {
 	static struct lock_class_key btrfs_trans_num_writers_key;
+	static struct lock_class_key btrfs_trans_num_extwriters_key;
 
 	xa_init_flags(&fs_info->fs_roots, GFP_ATOMIC);
 	xa_init_flags(&fs_info->extent_buffers, GFP_ATOMIC);
@@ -3062,6 +3063,9 @@ void btrfs_init_fs_info(struct btrfs_fs_info *fs_info)
 	lockdep_init_map(&fs_info->btrfs_trans_num_writers_map,
 					 "btrfs_trans_num_writers",
 					 &btrfs_trans_num_writers_key, 0);
+	lockdep_init_map(&fs_info->btrfs_trans_num_extwriters_map,
+					 "btrfs_trans_num_extwriters",
+					 &btrfs_trans_num_extwriters_key, 0);
 
 	INIT_LIST_HEAD(&fs_info->dirty_cowonly_roots);
 	INIT_LIST_HEAD(&fs_info->space_info);
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 7ef24e1f1446..972ef959f58f 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -314,6 +314,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
 		extwriter_counter_inc(cur_trans, type);
 		spin_unlock(&fs_info->trans_lock);
 		btrfs_lockdep_acquire(fs_info, btrfs_trans_num_writers);
+		btrfs_lockdep_acquire(fs_info, btrfs_trans_num_extwriters);
 		return 0;
 	}
 	spin_unlock(&fs_info->trans_lock);
@@ -336,6 +337,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
 		return -ENOMEM;
 
 	btrfs_lockdep_acquire(fs_info, btrfs_trans_num_writers);
+	btrfs_lockdep_acquire(fs_info, btrfs_trans_num_extwriters);
 
 	spin_lock(&fs_info->trans_lock);
 	if (fs_info->running_transaction) {
@@ -343,11 +345,13 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
 		 * someone started a transaction after we unlocked.  Make sure
 		 * to redo the checks above
 		 */
+		btrfs_lockdep_release(fs_info, btrfs_trans_num_extwriters);
 		btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
 		kfree(cur_trans);
 		goto loop;
 	} else if (BTRFS_FS_ERROR(fs_info)) {
 		spin_unlock(&fs_info->trans_lock);
+		btrfs_lockdep_release(fs_info, btrfs_trans_num_extwriters);
 		btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
 		kfree(cur_trans);
 		return -EROFS;
@@ -1026,6 +1030,7 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
 
 	cond_wake_up(&cur_trans->writer_wait);
 
+	btrfs_lockdep_release(info, btrfs_trans_num_extwriters);
 	btrfs_lockdep_release(info, btrfs_trans_num_writers);
 
 	btrfs_put_transaction(cur_trans);
@@ -2222,6 +2227,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 
 			btrfs_put_transaction(prev_trans);
 			if (ret) {
+				btrfs_lockdep_release(fs_info, btrfs_trans_num_extwriters);
 				btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
 				goto cleanup_transaction;
 			}
@@ -2238,6 +2244,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 		 */
 		if (BTRFS_FS_ERROR(fs_info)) {
 			ret = -EROFS;
+			btrfs_lockdep_release(fs_info, btrfs_trans_num_extwriters);
 			btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
 			goto cleanup_transaction;
 		}
@@ -2253,16 +2260,25 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 
 	ret = btrfs_start_delalloc_flush(fs_info);
 	if (ret) {
+		btrfs_lockdep_release(fs_info, btrfs_trans_num_extwriters);
 		btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
 		goto cleanup_transaction;
 	}
 
 	ret = btrfs_run_delayed_items(trans);
 	if (ret) {
+		btrfs_lockdep_release(fs_info, btrfs_trans_num_extwriters);
 		btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
 		goto cleanup_transaction;
 	}
 
+	/*
+	 * The thread has started/joined the transaction thus it holds the lockdep
+	 * map as a reader. It has to release it before acquiring the lockdep map
+	 * as a writer.
+	 */
+	btrfs_lockdep_release(fs_info, btrfs_trans_num_extwriters);
+	btrfs_might_wait_for_event(fs_info, btrfs_trans_num_extwriters);
 	wait_event(cur_trans->writer_wait,
 		   extwriter_counter_read(cur_trans) == 0);
 
-- 
2.30.2

Re: [PATCH 1/2] btrfs: Add a lockdep model for the num_writers wait event

[Nikolay Borisov]

On 8.07.22 г. 23:08 ч., Ioannis Angelakopoulos wrote:
> Annotate the num_writers wait event in fs/btrfs/transaction.c with lockdep
> in order to catch deadlocks involving this wait event.
> 
> Use a read/write lockdep map for the annotation. A thread starting/joining
> the transaction acquires the map as a reader when it increments
> cur_trans->num_writers and it acquires the map as a writer before it
> blocks on the wait event.
> 
> 
> Signed-off-by: Ioannis Angelakopoulos <iangelak@fb.com>
> ---
>   fs/btrfs/ctree.h       | 14 ++++++++++++++
>   fs/btrfs/disk-io.c     |  6 ++++++
>   fs/btrfs/transaction.c | 39 +++++++++++++++++++++++++++++++++++----
>   3 files changed, 55 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 4e2569f84aab..160c22391cb5 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1091,6 +1091,8 @@ struct btrfs_fs_info {
>   	/* Updates are not protected by any lock */
>   	struct btrfs_commit_stats commit_stats;
>   
> +	struct lockdep_map btrfs_trans_num_writers_map;
> +
>   #ifdef CONFIG_BTRFS_FS_REF_VERIFY
>   	spinlock_t ref_verify_lock;
>   	struct rb_root block_tree;
> @@ -1172,6 +1174,18 @@ enum {
>   	BTRFS_ROOT_UNFINISHED_DROP,
>   };
>   
> +#define btrfs_might_wait_for_event(b, lock) \
> +	do { \
> +		rwsem_acquire(&b->lock##_map, 0, 0, _THIS_IP_); \
> +		rwsem_release(&b->lock##_map, _THIS_IP_); \
> +	} while (0)
> +
> +#define btrfs_lockdep_acquire(b, lock) \
> +	rwsem_acquire_read(&b->lock##_map, 0, 0, _THIS_IP_)
> +
> +#define btrfs_lockdep_release(b, lock) \
> +	rwsem_release(&b->lock##_map, _THIS_IP_)
> +
>   static inline void btrfs_wake_unfinished_drop(struct btrfs_fs_info *fs_info)
>   {
>   	clear_and_wake_up_bit(BTRFS_FS_UNFINISHED_DROPS, &fs_info->flags);
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 76835394a61b..4061886024de 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3029,6 +3029,8 @@ static int __cold init_tree_roots(struct btrfs_fs_info *fs_info)
>   
>   void btrfs_init_fs_info(struct btrfs_fs_info *fs_info)
>   {
> +	static struct lock_class_key btrfs_trans_num_writers_key;

Shouldn't this variable and its initialization be defined in one of 
the__init functions (i.e init_btrfs_fs() )for the btrfs' kernel module? 
As it stands this would be called on every mount of any instance of a 
btrfs filesystem?

<snip>

Re: [PATCH 1/2] btrfs: Add a lockdep model for the num_writers wait event

[Johannes Thumshirn]

On 08.07.22 22:10, Ioannis Angelakopoulos wrote:
> +#define btrfs_might_wait_for_event(b, lock) \
> +	do { \
> +		rwsem_acquire(&b->lock##_map, 0, 0, _THIS_IP_); \
> +		rwsem_release(&b->lock##_map, _THIS_IP_); \
> +	} while (0)
> +
> +#define btrfs_lockdep_acquire(b, lock) \
> +	rwsem_acquire_read(&b->lock##_map, 0, 0, _THIS_IP_)
> +
> +#define btrfs_lockdep_release(b, lock) \
> +	rwsem_release(&b->lock##_map, _THIS_IP_)

Shouldn't this be only defined for CONFIG_LOCKDEP=y and be
stubbed out for CONFIG_LOCKDEP=n?

Re: [PATCH 1/2] btrfs: Add a lockdep model for the num_writers wait event

[David Sterba]

On Mon, Jul 11, 2022 at 04:04:01PM +0300, Nikolay Borisov wrote:
> > +	rwsem_release(&b->lock##_map, _THIS_IP_)
> > +
> >   static inline void btrfs_wake_unfinished_drop(struct btrfs_fs_info *fs_info)
> >   {
> >   	clear_and_wake_up_bit(BTRFS_FS_UNFINISHED_DROPS, &fs_info->flags);
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index 76835394a61b..4061886024de 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -3029,6 +3029,8 @@ static int __cold init_tree_roots(struct btrfs_fs_info *fs_info)
> >   
> >   void btrfs_init_fs_info(struct btrfs_fs_info *fs_info)
> >   {
> > +	static struct lock_class_key btrfs_trans_num_writers_key;
> 
> Shouldn't this variable and its initialization be defined in one of 
> the__init functions (i.e init_btrfs_fs() )for the btrfs' kernel module? 
> As it stands this would be called on every mount of any instance of a 
> btrfs filesystem?

Yeah I think it should be initialized once for the whole module, however
a static variable in __init function is not safe as the whole section
gets removed from memory once all functions get called (module vs
built-in), either an __initdata annotation or a static variable outside
of a function (eg. like fs_uuids in super.c).

Re: [PATCH 1/2] btrfs: Add a lockdep model for the num_writers wait event

[David Sterba]

On Wed, Jul 13, 2022 at 03:43:14PM +0000, Johannes Thumshirn wrote:
> On 08.07.22 22:10, Ioannis Angelakopoulos wrote:
> > +#define btrfs_might_wait_for_event(b, lock) \
> > +	do { \
> > +		rwsem_acquire(&b->lock##_map, 0, 0, _THIS_IP_); \
> > +		rwsem_release(&b->lock##_map, _THIS_IP_); \
> > +	} while (0)
> > +
> > +#define btrfs_lockdep_acquire(b, lock) \
> > +	rwsem_acquire_read(&b->lock##_map, 0, 0, _THIS_IP_)
> > +
> > +#define btrfs_lockdep_release(b, lock) \
> > +	rwsem_release(&b->lock##_map, _THIS_IP_)
> 
> Shouldn't this be only defined for CONFIG_LOCKDEP=y and be
> stubbed out for CONFIG_LOCKDEP=n?

Yes, all related code should be under the lockdep ifdef, struct
lockdep_map btrfs_trans_num_writers_map too.

Re: [PATCH 1/2] btrfs: Add a lockdep model for the num_writers wait event

[David Sterba]

On Fri, Jul 08, 2022 at 01:08:30PM -0700, Ioannis Angelakopoulos wrote:
> @@ -2207,8 +2221,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>  			ret = READ_ONCE(prev_trans->aborted);
>  
>  			btrfs_put_transaction(prev_trans);
> -			if (ret)
> +			if (ret) {
> +				btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
>  				goto cleanup_transaction;

Please move the call btrfs_lockdep_release to the cleanup block before
the cleanup_transaction label so it's not repeated everywhere.

> +			btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
>  			goto cleanup_transaction;

> +		btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
>  		goto cleanup_transaction;

> +		btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
>  		goto cleanup_transaction;

> +		btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
>  		goto cleanup_transaction;

Re: [PATCH 1/2] btrfs: Add a lockdep model for the num_writers wait event

[Nikolay Borisov]

On 13.07.22 г. 18:43 ч., Johannes Thumshirn wrote:
> On 08.07.22 22:10, Ioannis Angelakopoulos wrote:
>> +#define btrfs_might_wait_for_event(b, lock) \
>> +	do { \
>> +		rwsem_acquire(&b->lock##_map, 0, 0, _THIS_IP_); \
>> +		rwsem_release(&b->lock##_map, _THIS_IP_); \
>> +	} while (0)
>> +
>> +#define btrfs_lockdep_acquire(b, lock) \
>> +	rwsem_acquire_read(&b->lock##_map, 0, 0, _THIS_IP_)
>> +
>> +#define btrfs_lockdep_release(b, lock) \
>> +	rwsem_release(&b->lock##_map, _THIS_IP_)
> 
> Shouldn't this be only defined for CONFIG_LOCKDEP=y and be
> stubbed out for CONFIG_LOCKDEP=n?


rwsem_acquire/rwsem_release is actually stubbed when lockdep is disabled 
i.e check lock_acquire/lock_release. So in effect this is not a problem.

Re: [PATCH 1/2] btrfs: Add a lockdep model for the num_writers wait event

[Johannes Thumshirn]

On 14.07.22 15:28, Nikolay Borisov wrote:
> 
> 
> On 13.07.22 г. 18:43 ч., Johannes Thumshirn wrote:
>> On 08.07.22 22:10, Ioannis Angelakopoulos wrote:
>>> +#define btrfs_might_wait_for_event(b, lock) \
>>> +	do { \
>>> +		rwsem_acquire(&b->lock##_map, 0, 0, _THIS_IP_); \
>>> +		rwsem_release(&b->lock##_map, _THIS_IP_); \
>>> +	} while (0)
>>> +
>>> +#define btrfs_lockdep_acquire(b, lock) \
>>> +	rwsem_acquire_read(&b->lock##_map, 0, 0, _THIS_IP_)
>>> +
>>> +#define btrfs_lockdep_release(b, lock) \
>>> +	rwsem_release(&b->lock##_map, _THIS_IP_)
>>
>> Shouldn't this be only defined for CONFIG_LOCKDEP=y and be
>> stubbed out for CONFIG_LOCKDEP=n?
> 
> 
> rwsem_acquire/rwsem_release is actually stubbed when lockdep is disabled 
> i.e check lock_acquire/lock_release. So in effect this is not a problem.
> 

Ah yeah its

rwsem_acquire()
`-> lock_acquire_exclusive()
    `-> lock_acquire()

With lock_acquire() being stubbed out.

Re: [PATCH 1/2] btrfs: Add a lockdep model for the num_writers wait event

[Nikolay Borisov]

On 14.07.22 г. 16:13 ч., David Sterba wrote:
> On Mon, Jul 11, 2022 at 04:04:01PM +0300, Nikolay Borisov wrote:
>>> +	rwsem_release(&b->lock##_map, _THIS_IP_)
>>> +
>>>    static inline void btrfs_wake_unfinished_drop(struct btrfs_fs_info *fs_info)
>>>    {
>>>    	clear_and_wake_up_bit(BTRFS_FS_UNFINISHED_DROPS, &fs_info->flags);
>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>> index 76835394a61b..4061886024de 100644
>>> --- a/fs/btrfs/disk-io.c
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -3029,6 +3029,8 @@ static int __cold init_tree_roots(struct btrfs_fs_info *fs_info)
>>>    
>>>    void btrfs_init_fs_info(struct btrfs_fs_info *fs_info)
>>>    {
>>> +	static struct lock_class_key btrfs_trans_num_writers_key;
>>
>> Shouldn't this variable and its initialization be defined in one of
>> the__init functions (i.e init_btrfs_fs() )for the btrfs' kernel module?
>> As it stands this would be called on every mount of any instance of a
>> btrfs filesystem?
> 
> Yeah I think it should be initialized once for the whole module, however
> a static variable in __init function is not safe as the whole section
> gets removed from memory once all functions get called (module vs
> built-in), either an __initdata annotation or a static variable outside
> of a function (eg. like fs_uuids in super.c).


Actually the code as is makes sense, since the initialization is about 
the lockdep map and not the lock_class_key, as such it needs to be 
called in a place where we have a reference to btrfs_fs_info and since 
this is per-fs, it makes sense to be in btrfs_init_fs_info.

Re: [PATCH 1/2] btrfs: Add a lockdep model for the num_writers wait event

[Ioannis Angelakopoulos]

On 7/14/22 6:21 AM, David Sterba wrote:
> On Fri, Jul 08, 2022 at 01:08:30PM -0700, Ioannis Angelakopoulos wrote:
>> @@ -2207,8 +2221,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>>   			ret = READ_ONCE(prev_trans->aborted);
>>   
>>   			btrfs_put_transaction(prev_trans);
>> -			if (ret)
>> +			if (ret) {
>> +				btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
>>   				goto cleanup_transaction;
> 
> Please move the call btrfs_lockdep_release to the cleanup block before
> the cleanup_transaction label so it's not repeated everywhere.
>
Unfortunately, I cannot do this with the code as it is. If the commit 
thread releases the btrfs_num_writers lock as reader before the 
wait_event and then jumps to either unlock_reloc or scrub_continue 
labels later in the execution, it will try to release the lock again in 
cleanup_transaction resulting in a double release.
>> +			btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
>>   			goto cleanup_transaction;
> 
>> +		btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
>>   		goto cleanup_transaction;
> 
>> +		btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
>>   		goto cleanup_transaction;
> 
>> +		btrfs_lockdep_release(fs_info, btrfs_trans_num_writers);
>>   		goto cleanup_transaction;