[PATCH 00/11] Make pinned extents tracking per-transaction

[PATCH 00/11] Make pinned extents tracking per-transaction

[Nikolay Borisov]

I've finally managed to finish and test the pinned extents rework. So here it is.

The idea is to move pinnex extents tracking from the global fs_info->pinned_extents,
which is just a pointer to fs_info->freed_extents[] array members to a per
transaction structure. This will allow to cleanup pinned extents information
during transaction abort.

The bulk of the changes are necessary to ensure a valid transaction handle
is passed to every function that utilizes fs_info->pinned_extents. Also it
was necessary to account for the peculiarities of excluded extents which are
also tracked in ->freed_extents array but with a different flag (yuck).

First 9 patches propagate btrfs_trans_handle where it's necessary. Patch 10
factors out pinned extent clean up to make Patch 11 more readable , alternatively
the changes in patch 10 had to be in patch 11 which would have made it messier
for review.

I believe this series doesn't bring any user facing changes it (hopefully)
streamlines the code and makes it apparent what the lifetime of pinned extents
is and paves the way for further cleanups of BUG_ON.

Nikolay Borisov (11):
  btrfs: Perform pinned cleanup directly in btrfs_destroy_delayed_refs
  btrfs: Make btrfs_pin_extent take trans handle
  btrfs: Introduce unaccount_log_buffer
  btrfs: Call btrfs_pin_reserved_extent only during active transaction
  btrfs: Make btrfs_pin_reserved_extent take transaction
  btrfs: Make btrfs_pin_extent_for_log_replay take transaction handle
  btrfs: Make pin_down_extent take btrfs_trans_handle
  btrfs: Pass trans handle to write_pinned_extent_entries
  btrfs: Mark pinned log extents as excluded
  btrfs: Factor out pinned extent clean up in btrfs_delete_unused_bgs
  btrfs: Use btrfs_transaction::pinned_extents

 fs/btrfs/block-group.c       | 85 +++++++++++++++++++++++-------------
 fs/btrfs/ctree.h             | 12 ++---
 fs/btrfs/disk-io.c           | 56 ++++++++++++------------
 fs/btrfs/extent-io-tree.h    |  3 +-
 fs/btrfs/extent-tree.c       | 70 +++++++++++++----------------
 fs/btrfs/free-space-cache.c  |  5 ++-
 fs/btrfs/tests/btrfs-tests.c |  7 +--
 fs/btrfs/transaction.c       |  1 +
 fs/btrfs/transaction.h       |  1 +
 fs/btrfs/tree-log.c          | 70 +++++++++++++++++++----------
 include/trace/events/btrfs.h |  3 +-
 11 files changed, 174 insertions(+), 139 deletions(-)

--
2.17.1

[PATCH 01/11] btrfs: Perform pinned cleanup directly in btrfs_destroy_delayed_refs

[Nikolay Borisov]

Having btrfs_destroy_delayed_refs call btrfs_pin_extent is problematic
for making pinned extents tracking per-transaction since
btrfs_trans_handle cannot be passed to btrfs_pin_extent in this context.
Additionally delayed refs heads pinned in btrfs_destroy_delayed_refs
are going to be handled very closely, in btrfs_destroy_pinned_extent.

To enable btrfs_pin_extent to take btrfs_trans_handle simply open code
it in btrfs_destroy_delayed_refs and call btrfs_error_unpin_extent_range
on the range. This enables us to do less work in
btrfs_destroy_pinned_extent and leaves btrfs_pin_extent being called in
contexts which have a valid btrfs_trans_handle.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 5ce2801f8388..9209c7b0997c 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -42,6 +42,7 @@
 #include "ref-verify.h"
 #include "block-group.h"
 #include "discard.h"
+#include "space-info.h"
 
 #define BTRFS_SUPER_FLAG_SUPP	(BTRFS_HEADER_FLAG_WRITTEN |\
 				 BTRFS_HEADER_FLAG_RELOC |\
@@ -4261,9 +4262,28 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
 		spin_unlock(&delayed_refs->lock);
 		mutex_unlock(&head->mutex);
 
-		if (pin_bytes)
-			btrfs_pin_extent(fs_info, head->bytenr,
-					 head->num_bytes, 1);
+		if (pin_bytes) {
+			struct btrfs_block_group *cache;
+			cache = btrfs_lookup_block_group(fs_info, head->bytenr);
+			BUG_ON(!cache);
+
+			spin_lock(&cache->space_info->lock);
+			spin_lock(&cache->lock);
+			cache->pinned += head->num_bytes;
+			btrfs_space_info_update_bytes_pinned(fs_info,
+					cache->space_info, head->num_bytes);
+			cache->reserved -= head->num_bytes;
+			cache->space_info->bytes_reserved -= head->num_bytes;
+			spin_unlock(&cache->lock);
+			spin_unlock(&cache->space_info->lock);
+			percpu_counter_add_batch(&cache->space_info->total_bytes_pinned,
+				    head->num_bytes, BTRFS_TOTAL_BYTES_PINNED_BATCH);
+
+			btrfs_put_block_group(cache);
+
+			btrfs_error_unpin_extent_range(fs_info, head->bytenr,
+						       head->bytenr + head->num_bytes - 1);
+		}
 		btrfs_cleanup_ref_head_accounting(fs_info, delayed_refs, head);
 		btrfs_put_delayed_ref_head(head);
 		cond_resched();
-- 
2.17.1

[PATCH 02/11] btrfs: Make btrfs_pin_extent take trans handle

[Nikolay Borisov]

Preparation for switching pinned extent tracking to a per-transaction
basis.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.h       |  4 ++--
 fs/btrfs/extent-tree.c | 18 +++++++-----------
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index f90b82050d2d..11eaabe104b8 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2460,8 +2460,8 @@ int btrfs_lookup_data_extent(struct btrfs_fs_info *fs_info, u64 start, u64 len);
 int btrfs_lookup_extent_info(struct btrfs_trans_handle *trans,
 			     struct btrfs_fs_info *fs_info, u64 bytenr,
 			     u64 offset, int metadata, u64 *refs, u64 *flags);
-int btrfs_pin_extent(struct btrfs_fs_info *fs_info,
-		     u64 bytenr, u64 num, int reserved);
+int btrfs_pin_extent(struct btrfs_trans_handle *trans, u64 bytenr, u64 num,
+		     int reserved);
 int btrfs_pin_extent_for_log_replay(struct btrfs_fs_info *fs_info,
 				    u64 bytenr, u64 num_bytes);
 int btrfs_exclude_logged_extents(struct extent_buffer *eb);
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 0163fdd59f8f..31ae94a7591c 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -1705,8 +1705,8 @@ static int run_one_delayed_ref(struct btrfs_trans_handle *trans,
 
 	if (trans->aborted) {
 		if (insert_reserved)
-			btrfs_pin_extent(trans->fs_info, node->bytenr,
-					 node->num_bytes, 1);
+			btrfs_pin_extent(trans, node->bytenr, node->num_bytes,
+					 1);
 		return 0;
 	}
 
@@ -1721,8 +1721,7 @@ static int run_one_delayed_ref(struct btrfs_trans_handle *trans,
 	else
 		BUG();
 	if (ret && insert_reserved)
-		btrfs_pin_extent(trans->fs_info, node->bytenr,
-				 node->num_bytes, 1);
+		btrfs_pin_extent(trans, node->bytenr, node->num_bytes, 1);
 	return ret;
 }
 
@@ -1867,8 +1866,7 @@ static int cleanup_ref_head(struct btrfs_trans_handle *trans,
 	spin_unlock(&delayed_refs->lock);
 
 	if (head->must_insert_reserved) {
-		btrfs_pin_extent(fs_info, head->bytenr,
-				 head->num_bytes, 1);
+		btrfs_pin_extent(trans, head->bytenr, head->num_bytes, 1);
 		if (head->is_data) {
 			ret = btrfs_del_csums(trans, fs_info->csum_root,
 					      head->bytenr, head->num_bytes);
@@ -2612,14 +2610,12 @@ static int pin_down_extent(struct btrfs_block_group *cache,
 	return 0;
 }
 
-int btrfs_pin_extent(struct btrfs_fs_info *fs_info,
+int btrfs_pin_extent(struct btrfs_trans_handle *trans,
 		     u64 bytenr, u64 num_bytes, int reserved)
 {
 	struct btrfs_block_group *cache;
 
-	ASSERT(fs_info->running_transaction);
-
-	cache = btrfs_lookup_block_group(fs_info, bytenr);
+	cache = btrfs_lookup_block_group(trans->fs_info, bytenr);
 	BUG_ON(!cache); /* Logic error */
 
 	pin_down_extent(cache, bytenr, num_bytes, reserved);
@@ -3345,7 +3341,7 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans, struct btrfs_ref *ref)
 	    (ref->type == BTRFS_REF_DATA &&
 	     ref->data_ref.ref_root == BTRFS_TREE_LOG_OBJECTID)) {
 		/* unlocks the pinned mutex */
-		btrfs_pin_extent(fs_info, ref->bytenr, ref->len, 1);
+		btrfs_pin_extent(trans, ref->bytenr, ref->len, 1);
 		old_ref_mod = new_ref_mod = 0;
 		ret = 0;
 	} else if (ref->type == BTRFS_REF_METADATA) {
-- 
2.17.1

[PATCH 03/11] btrfs: Introduce unaccount_log_buffer

[Nikolay Borisov]

This function correctly adjusts the reserved bytes occupied by a
log tree extent buffer. It will be used instead of calling
btrfs_pin_reserved_extent.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/tree-log.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index a2bae5c230e1..30e7d96a69fe 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -18,6 +18,8 @@
 #include "compression.h"
 #include "qgroup.h"
 #include "inode-map.h"
+#include "block-group.h"
+#include "space-info.h"
 
 /* magic values for the inode_only field in btrfs_log_inode:
  *
@@ -2659,6 +2661,27 @@ static int replay_one_buffer(struct btrfs_root *log, struct extent_buffer *eb,
 	return ret;
 }
 
+static void unaccount_log_buffer(struct btrfs_fs_info *fs_info, u64 start)
+{
+	struct btrfs_block_group *cache;
+
+	cache = btrfs_lookup_block_group(fs_info, start);
+	if (!cache) {
+		btrfs_err(fs_info, "unable to find block group for %llu", start);
+		return;
+	}
+
+	spin_lock(&cache->space_info->lock);
+	spin_lock(&cache->lock);
+	cache->reserved -= fs_info->nodesize;
+	cache->space_info->bytes_reserved -= fs_info->nodesize;
+	spin_unlock(&cache->lock);
+	spin_unlock(&cache->space_info->lock);
+
+	btrfs_put_block_group(cache);
+	return;
+}
+
 static noinline int walk_down_log_tree(struct btrfs_trans_handle *trans,
 				   struct btrfs_root *root,
 				   struct btrfs_path *path, int *level,
-- 
2.17.1

[PATCH 04/11] btrfs: Call btrfs_pin_reserved_extent only during active transaction

[Nikolay Borisov]

Calling btrfs_pin_reserved_extent makes sense only with a valid
transaction since pinned extents are processed from transaction commit
in btrfs_finish_extent_commit. In case of error it's sufficient to
adjust the reserved counter to account for log tree extents allocated in
the last transaction.

This commit moves btrfs_pin_reserved_extent to be called only with
valid transaction handle and otherwise uses the newly introduced
unaccount_log_buffer to adjust "reserved". If this is not done if a
failure occurs before transaction is committed WARN_ON are going to
be triggered on unmount. This was especially pronounced with generic/475
test.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/tree-log.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 30e7d96a69fe..4c550579e621 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -2743,18 +2743,16 @@ static noinline int walk_down_log_tree(struct btrfs_trans_handle *trans,
 					btrfs_clean_tree_block(next);
 					btrfs_wait_tree_block_writeback(next);
 					btrfs_tree_unlock(next);
+					ret = btrfs_pin_reserved_extent(fs_info,
+								bytenr, blocksize);
+					if (ret) {
+						free_extent_buffer(next);
+						return ret;
+					}
 				} else {
 					if (test_and_clear_bit(EXTENT_BUFFER_DIRTY, &next->bflags))
 						clear_extent_buffer_dirty(next);
-				}
-
-				WARN_ON(root_owner !=
-					BTRFS_TREE_LOG_OBJECTID);
-				ret = btrfs_pin_reserved_extent(fs_info,
-							bytenr, blocksize);
-				if (ret) {
-					free_extent_buffer(next);
-					return ret;
+					unaccount_log_buffer(fs_info, bytenr);
 				}
 			}
 			free_extent_buffer(next);
@@ -2822,17 +2820,17 @@ static noinline int walk_up_log_tree(struct btrfs_trans_handle *trans,
 					btrfs_clean_tree_block(next);
 					btrfs_wait_tree_block_writeback(next);
 					btrfs_tree_unlock(next);
+					ret = btrfs_pin_reserved_extent(fs_info,
+							path->nodes[*level]->start,
+							path->nodes[*level]->len);
+					if (ret)
+						return ret;
 				} else {
 					if (test_and_clear_bit(EXTENT_BUFFER_DIRTY, &next->bflags))
 						clear_extent_buffer_dirty(next);
+					unaccount_log_buffer(fs_info,
+							     path->nodes[*level]->start);
 				}
-
-				WARN_ON(root_owner != BTRFS_TREE_LOG_OBJECTID);
-				ret = btrfs_pin_reserved_extent(fs_info,
-						path->nodes[*level]->start,
-						path->nodes[*level]->len);
-				if (ret)
-					return ret;
 			}
 			free_extent_buffer(path->nodes[*level]);
 			path->nodes[*level] = NULL;
@@ -2903,15 +2901,16 @@ static int walk_log_tree(struct btrfs_trans_handle *trans,
 				btrfs_clean_tree_block(next);
 				btrfs_wait_tree_block_writeback(next);
 				btrfs_tree_unlock(next);
+				ret = btrfs_pin_reserved_extent(fs_info, next->start,
+								next->len);
+				if (ret)
+					goto out;
 			} else {
 				if (test_and_clear_bit(EXTENT_BUFFER_DIRTY, &next->bflags))
 					clear_extent_buffer_dirty(next);
+				unaccount_log_buffer(fs_info,
+						     next->start);
 			}
-
-			ret = btrfs_pin_reserved_extent(fs_info, next->start,
-							next->len);
-			if (ret)
-				goto out;
 		}
 	}
 
-- 
2.17.1

[PATCH 05/11] btrfs: Make btrfs_pin_reserved_extent take transaction

[Nikolay Borisov]

btrfs_pin_reserved_extent is now only called with a valid transaction so
exploit the fact to take a transaction. This is preparation for tracking
pinned extents on a per-transaction basis.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.h       | 2 +-
 fs/btrfs/extent-tree.c | 8 +++++---
 fs/btrfs/tree-log.c    | 6 +++---
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 11eaabe104b8..1fe1cbe20bba 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2498,7 +2498,7 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans, struct btrfs_ref *ref);
 
 int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
 			       u64 start, u64 len, int delalloc);
-int btrfs_pin_reserved_extent(struct btrfs_fs_info *fs_info, u64 start,
+int btrfs_pin_reserved_extent(struct btrfs_trans_handle *trans, u64 start,
 			      u64 len);
 void btrfs_prepare_extent_commit(struct btrfs_fs_info *fs_info);
 int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans);
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 31ae94a7591c..cf6048fa6e9a 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4185,14 +4185,16 @@ int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
-int btrfs_pin_reserved_extent(struct btrfs_fs_info *fs_info, u64 start, u64 len)
+int btrfs_pin_reserved_extent(struct btrfs_trans_handle *trans, u64 start,
+			      u64 len)
 {
 	struct btrfs_block_group *cache;
 	int ret = 0;
 
-	cache = btrfs_lookup_block_group(fs_info, start);
+	cache = btrfs_lookup_block_group(trans->fs_info, start);
 	if (!cache) {
-		btrfs_err(fs_info, "unable to find block group for %llu", start);
+		btrfs_err(trans->fs_info, "unable to find block group for %llu",
+			  start);
 		return -ENOSPC;
 	}
 
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 4c550579e621..ccbac7663d3b 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -2743,7 +2743,7 @@ static noinline int walk_down_log_tree(struct btrfs_trans_handle *trans,
 					btrfs_clean_tree_block(next);
 					btrfs_wait_tree_block_writeback(next);
 					btrfs_tree_unlock(next);
-					ret = btrfs_pin_reserved_extent(fs_info,
+					ret = btrfs_pin_reserved_extent(trans,
 								bytenr, blocksize);
 					if (ret) {
 						free_extent_buffer(next);
@@ -2820,7 +2820,7 @@ static noinline int walk_up_log_tree(struct btrfs_trans_handle *trans,
 					btrfs_clean_tree_block(next);
 					btrfs_wait_tree_block_writeback(next);
 					btrfs_tree_unlock(next);
-					ret = btrfs_pin_reserved_extent(fs_info,
+					ret = btrfs_pin_reserved_extent(trans,
 							path->nodes[*level]->start,
 							path->nodes[*level]->len);
 					if (ret)
@@ -2901,7 +2901,7 @@ static int walk_log_tree(struct btrfs_trans_handle *trans,
 				btrfs_clean_tree_block(next);
 				btrfs_wait_tree_block_writeback(next);
 				btrfs_tree_unlock(next);
-				ret = btrfs_pin_reserved_extent(fs_info, next->start,
+				ret = btrfs_pin_reserved_extent(trans, next->start,
 								next->len);
 				if (ret)
 					goto out;
-- 
2.17.1

[PATCH 06/11] btrfs: Make btrfs_pin_extent_for_log_replay take transaction handle

[Nikolay Borisov]

Preparation for refactoring pinned extents tracking.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.h       | 2 +-
 fs/btrfs/extent-tree.c | 4 ++--
 fs/btrfs/tree-log.c    | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 1fe1cbe20bba..c6ce8c047814 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2462,7 +2462,7 @@ int btrfs_lookup_extent_info(struct btrfs_trans_handle *trans,
 			     u64 offset, int metadata, u64 *refs, u64 *flags);
 int btrfs_pin_extent(struct btrfs_trans_handle *trans, u64 bytenr, u64 num,
 		     int reserved);
-int btrfs_pin_extent_for_log_replay(struct btrfs_fs_info *fs_info,
+int btrfs_pin_extent_for_log_replay(struct btrfs_trans_handle *trans,
 				    u64 bytenr, u64 num_bytes);
 int btrfs_exclude_logged_extents(struct extent_buffer *eb);
 int btrfs_cross_ref_exist(struct btrfs_root *root,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index cf6048fa6e9a..a9a54b7ae15a 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2627,13 +2627,13 @@ int btrfs_pin_extent(struct btrfs_trans_handle *trans,
 /*
  * this function must be called within transaction
  */
-int btrfs_pin_extent_for_log_replay(struct btrfs_fs_info *fs_info,
+int btrfs_pin_extent_for_log_replay(struct btrfs_trans_handle *trans,
 				    u64 bytenr, u64 num_bytes)
 {
 	struct btrfs_block_group *cache;
 	int ret;

-	cache = btrfs_lookup_block_group(fs_info, bytenr);
+	cache = btrfs_lookup_block_group(trans->fs_info, bytenr);
 	if (!cache)
 		return -EINVAL;

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index ccbac7663d3b..b535d409a728 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -313,7 +313,7 @@ static int process_one_buffer(struct btrfs_root *log,
 	}

 	if (wc->pin)
-		ret = btrfs_pin_extent_for_log_replay(fs_info, eb->start,
+		ret = btrfs_pin_extent_for_log_replay(wc->trans, eb->start,
 						      eb->len);

 	if (!ret && btrfs_buffer_uptodate(eb, gen, 0)) {
@@ -6151,7 +6151,7 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
 			 * each subsequent pass.
 			 */
 			if (ret == -ENOENT)
-				ret = btrfs_pin_extent_for_log_replay(fs_info,
+				ret = btrfs_pin_extent_for_log_replay(trans,
 							log->node->start,
 							log->node->len);
 			free_extent_buffer(log->node);
--
2.17.1

[PATCH 07/11] btrfs: Make pin_down_extent take btrfs_trans_handle

[Nikolay Borisov]

All callers have a reference to a transaction handle so pass it to
pin_down_extent. This is the final step before switching pinned extent
tracking to a per-transaction basis.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent-tree.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a9a54b7ae15a..7dcf9217a622 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2586,7 +2586,8 @@ static u64 first_logical_byte(struct btrfs_fs_info *fs_info, u64 search_start)
 	return bytenr;
 }
 
-static int pin_down_extent(struct btrfs_block_group *cache,
+static int pin_down_extent(struct btrfs_trans_handle *trans,
+			   struct btrfs_block_group *cache,
 			   u64 bytenr, u64 num_bytes, int reserved)
 {
 	struct btrfs_fs_info *fs_info = cache->fs_info;
@@ -2618,7 +2619,7 @@ int btrfs_pin_extent(struct btrfs_trans_handle *trans,
 	cache = btrfs_lookup_block_group(trans->fs_info, bytenr);
 	BUG_ON(!cache); /* Logic error */
 
-	pin_down_extent(cache, bytenr, num_bytes, reserved);
+	pin_down_extent(trans, cache, bytenr, num_bytes, reserved);
 
 	btrfs_put_block_group(cache);
 	return 0;
@@ -2645,7 +2646,7 @@ int btrfs_pin_extent_for_log_replay(struct btrfs_trans_handle *trans,
 	 */
 	btrfs_cache_block_group(cache, 1);
 
-	pin_down_extent(cache, bytenr, num_bytes, 0);
+	pin_down_extent(trans, cache, bytenr, num_bytes, 0);
 
 	/* remove us from the free space cache (if we're there at all) */
 	ret = btrfs_remove_free_space(cache, bytenr, num_bytes);
@@ -3297,7 +3298,7 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
 		cache = btrfs_lookup_block_group(fs_info, buf->start);
 
 		if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
-			pin_down_extent(cache, buf->start, buf->len, 1);
+			pin_down_extent(trans, cache, buf->start, buf->len, 1);
 			btrfs_put_block_group(cache);
 			goto out;
 		}
@@ -4198,7 +4199,7 @@ int btrfs_pin_reserved_extent(struct btrfs_trans_handle *trans, u64 start,
 		return -ENOSPC;
 	}
 
-	ret = pin_down_extent(cache, start, len, 1);
+	ret = pin_down_extent(trans, cache, start, len, 1);
 	btrfs_put_block_group(cache);
 	return ret;
 }
-- 
2.17.1

[PATCH 08/11] btrfs: Pass trans handle to write_pinned_extent_entries

[Nikolay Borisov]

Preparation for refactoring pinned extents tracking.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/free-space-cache.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 0598fd3c6e3f..9d6372139547 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -1067,6 +1067,7 @@ update_cache_item(struct btrfs_trans_handle *trans,
 }

 static noinline_for_stack int write_pinned_extent_entries(
+			    struct btrfs_trans_handle *trans,
 			    struct btrfs_block_group *block_group,
 			    struct btrfs_io_ctl *io_ctl,
 			    int *entries)
@@ -1317,7 +1318,7 @@ static int __btrfs_write_out_cache(struct btrfs_root *root, struct inode *inode,
 	 * If this changes while we are working we'll get added back to
 	 * the dirty list and redo it.  No locking needed
 	 */
-	ret = write_pinned_extent_entries(block_group, io_ctl, &entries);
+	ret = write_pinned_extent_entries(trans, block_group, io_ctl, &entries);
 	if (ret)
 		goto out_nospc_locked;

--
2.17.1

[PATCH 09/11] btrfs: Mark pinned log extents as excluded

[Nikolay Borisov]

In preparation to making pinned extents per-transaction ensure that
log such extents are always excluded from caching. To achieve this in
addition to marking them via btrfs_pin_extent_for_log_replay they also
need to be marked with btrfs_add_excluded_extent to prevent log tree
extent buffer being loaded by the free space caching thread. That's
required since log treeblocks are not recorded in the extent tree, hence
they always look free.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent-tree.c | 8 ++++++++
 fs/btrfs/tree-log.c    | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 7dcf9217a622..d680f2ac336b 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2634,6 +2634,8 @@ int btrfs_pin_extent_for_log_replay(struct btrfs_trans_handle *trans,
 	struct btrfs_block_group *cache;
 	int ret;
 
+	btrfs_add_excluded_extent(trans->fs_info, bytenr, num_bytes);
+
 	cache = btrfs_lookup_block_group(trans->fs_info, bytenr);
 	if (!cache)
 		return -EINVAL;
@@ -2920,6 +2922,12 @@ int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans)
 			mutex_unlock(&fs_info->unused_bg_unpin_mutex);
 			break;
 		}
+		if (test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags)) {
+			clear_extent_bits(&fs_info->freed_extents[0], start,
+					  end, EXTENT_UPTODATE);
+			clear_extent_bits(&fs_info->freed_extents[1], start,
+					  end, EXTENT_UPTODATE);
+		}
 
 		if (btrfs_test_opt(fs_info, DISCARD_SYNC))
 			ret = btrfs_discard_extent(fs_info, start,
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index b535d409a728..f89de24838d5 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -2994,7 +2994,7 @@ static inline void btrfs_remove_log_ctx(struct btrfs_root *root,
 	mutex_unlock(&root->log_mutex);
 }
 
-/* 
+/*
  * Invoked in log mutex context, or be sure there is no other task which
  * can access the list.
  */
-- 
2.17.1

[PATCH 10/11] btrfs: Factor out pinned extent clean up in btrfs_delete_unused_bgs

[Nikolay Borisov]

Next patch is going to refactor how pinned extents are tracked which
will necessitate changing this code. To ease that work and contain the
changes factor the code now in preparation, this will also help review.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/block-group.c | 69 ++++++++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 29 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 14851584e245..48bb9e08f2e8 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1233,6 +1233,45 @@ static int inc_block_group_ro(struct btrfs_block_group *cache, int force)
 	return ret;
 }

+static bool clean_pinned_extents(struct btrfs_block_group *bg)
+{
+	struct btrfs_fs_info *fs_info = bg->fs_info;
+	u64 start = bg->start;
+	u64 end = start + bg->length - 1;
+	int ret;
+
+	/*
+	 * Hold the unused_bg_unpin_mutex lock to avoid racing with
+	 * btrfs_finish_extent_commit(). If we are at transaction N,
+	 * another task might be running finish_extent_commit() for the
+	 * previous transaction N - 1, and have seen a range belonging
+	 * to the block group in freed_extents[] before we were able to
+	 * clear the whole block group range from freed_extents[]. This
+	 * means that task can lookup for the block group after we
+	 * unpinned it from freed_extents[] and removed it, leading to
+	 * a BUG_ON() at unpin_extent_range().
+	 */
+	mutex_lock(&fs_info->unused_bg_unpin_mutex);
+	ret = clear_extent_bits(&fs_info->freed_extents[0], start, end,
+			  EXTENT_DIRTY);
+	if (ret)
+		goto failure;
+
+	ret = clear_extent_bits(&fs_info->freed_extents[1], start, end,
+			  EXTENT_DIRTY);
+	if (ret)
+		goto failure;
+	mutex_unlock(&fs_info->unused_bg_unpin_mutex);
+
+	return true;
+
+failure:
+	mutex_unlock(&fs_info->unused_bg_unpin_mutex);
+	btrfs_dec_block_group_ro(bg);
+	return false;
+
+}
+
 /*
  * Process the unused_bgs list and remove any that don't have any allocated
  * space inside of them.
@@ -1250,7 +1289,6 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)

 	spin_lock(&fs_info->unused_bgs_lock);
 	while (!list_empty(&fs_info->unused_bgs)) {
-		u64 start, end;
 		int trimming;

 		block_group = list_first_entry(&fs_info->unused_bgs,
@@ -1329,35 +1367,8 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 		 * We could have pending pinned extents for this block group,
 		 * just delete them, we don't care about them anymore.
 		 */
-		start = block_group->start;
-		end = start + block_group->length - 1;
-		/*
-		 * Hold the unused_bg_unpin_mutex lock to avoid racing with
-		 * btrfs_finish_extent_commit(). If we are at transaction N,
-		 * another task might be running finish_extent_commit() for the
-		 * previous transaction N - 1, and have seen a range belonging
-		 * to the block group in freed_extents[] before we were able to
-		 * clear the whole block group range from freed_extents[]. This
-		 * means that task can lookup for the block group after we
-		 * unpinned it from freed_extents[] and removed it, leading to
-		 * a BUG_ON() at btrfs_unpin_extent_range().
-		 */
-		mutex_lock(&fs_info->unused_bg_unpin_mutex);
-		ret = clear_extent_bits(&fs_info->freed_extents[0], start, end,
-				  EXTENT_DIRTY);
-		if (ret) {
-			mutex_unlock(&fs_info->unused_bg_unpin_mutex);
-			btrfs_dec_block_group_ro(block_group);
+		if (!clean_pinned_extents(block_group))
 			goto end_trans;
-		}
-		ret = clear_extent_bits(&fs_info->freed_extents[1], start, end,
-				  EXTENT_DIRTY);
-		if (ret) {
-			mutex_unlock(&fs_info->unused_bg_unpin_mutex);
-			btrfs_dec_block_group_ro(block_group);
-			goto end_trans;
-		}
-		mutex_unlock(&fs_info->unused_bg_unpin_mutex);

 		/*
 		 * At this point, the block_group is read only and should fail
--
2.17.1

[PATCH 11/11] btrfs: Use btrfs_transaction::pinned_extents

[Nikolay Borisov]

This commit flips the switch to start tracking/processing pinned
extents on a per-transaction basis. It mostly replaces all references
from btrfs_fs_info::(pinned_extents|freed_extents[]) to
btrfs_transaction::pinned_extents. Two notable modifications that
warrant explicit mention are changing clean_pinned_extents to get a
reference to the previously running transaction. The other one is
removal of call to btrfs_destroy_pinned_extent since transactions are
going to be cleaned in btrfs_cleanup_one_transaction.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/block-group.c       | 38 ++++++++++++++++++++++++------------
 fs/btrfs/ctree.h             |  4 ++--
 fs/btrfs/disk-io.c           | 30 +++++-----------------------
 fs/btrfs/extent-io-tree.h    |  3 +--
 fs/btrfs/extent-tree.c       | 31 ++++++++---------------------
 fs/btrfs/free-space-cache.c  |  2 +-
 fs/btrfs/tests/btrfs-tests.c |  7 ++-----
 fs/btrfs/transaction.c       |  1 +
 fs/btrfs/transaction.h       |  1 +
 include/trace/events/btrfs.h |  3 +--
 10 files changed, 47 insertions(+), 73 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 48bb9e08f2e8..562dfb7dc77f 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -460,7 +460,7 @@ u64 add_new_free_space(struct btrfs_block_group *block_group, u64 start, u64 end
 	int ret;
 
 	while (start < end) {
-		ret = find_first_extent_bit(info->pinned_extents, start,
+		ret = find_first_extent_bit(&info->excluded_extents, start,
 					    &extent_start, &extent_end,
 					    EXTENT_DIRTY | EXTENT_UPTODATE,
 					    NULL);
@@ -1233,32 +1233,44 @@ static int inc_block_group_ro(struct btrfs_block_group *cache, int force)
 	return ret;
 }
 
-static bool clean_pinned_extents(struct btrfs_block_group *bg)
+static bool clean_pinned_extents(struct btrfs_trans_handle *trans,
+				 struct btrfs_block_group *bg)
 {
 	struct btrfs_fs_info *fs_info = bg->fs_info;
+	struct btrfs_transaction *prev_trans = NULL;
 	u64 start = bg->start;
 	u64 end = start + bg->length - 1;
 	int ret;
 
+	spin_lock(&fs_info->trans_lock);
+	if (trans->transaction->list.prev != &fs_info->trans_list) {
+		prev_trans = list_entry(trans->transaction->list.prev,
+					struct btrfs_transaction, list);
+		refcount_inc(&prev_trans->use_count);
+	}
+	spin_unlock(&fs_info->trans_lock);
+
 	/*
 	 * Hold the unused_bg_unpin_mutex lock to avoid racing with
 	 * btrfs_finish_extent_commit(). If we are at transaction N,
 	 * another task might be running finish_extent_commit() for the
 	 * previous transaction N - 1, and have seen a range belonging
-	 * to the block group in freed_extents[] before we were able to
-	 * clear the whole block group range from freed_extents[]. This
+	 * to the block group in pinned_extents before we were able to
+	 * clear the whole block group range from pinned_extents. This
 	 * means that task can lookup for the block group after we
-	 * unpinned it from freed_extents[] and removed it, leading to
+	 * unpinned it from pinned_extents[] and removed it, leading to
 	 * a BUG_ON() at unpin_extent_range().
 	 */
 	mutex_lock(&fs_info->unused_bg_unpin_mutex);
-	ret = clear_extent_bits(&fs_info->freed_extents[0], start, end,
-			  EXTENT_DIRTY);
-	if (ret)
-		goto failure;
+	if (prev_trans) {
+		ret = clear_extent_bits(&prev_trans->pinned_extents, start, end,
+					EXTENT_DIRTY);
+		if (ret)
+			goto failure;
+	}
 
-	ret = clear_extent_bits(&fs_info->freed_extents[1], start, end,
-			  EXTENT_DIRTY);
+	ret = clear_extent_bits(&trans->transaction->pinned_extents, start, end,
+				EXTENT_DIRTY);
 	if (ret)
 		goto failure;
 	mutex_unlock(&fs_info->unused_bg_unpin_mutex);
@@ -1367,7 +1379,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 		 * We could have pending pinned extents for this block group,
 		 * just delete them, we don't care about them anymore.
 		 */
-		if (!clean_pinned_extents(block_group))
+		if (!clean_pinned_extents(trans, block_group))
 			goto end_trans;
 
 		/*
@@ -2877,7 +2889,7 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans,
 					&cache->space_info->total_bytes_pinned,
 					num_bytes,
 					BTRFS_TOTAL_BYTES_PINNED_BATCH);
-			set_extent_dirty(info->pinned_extents,
+			set_extent_dirty(&trans->transaction->pinned_extents,
 					 bytenr, bytenr + num_bytes - 1,
 					 GFP_NOFS | __GFP_NOFAIL);
 		}
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index c6ce8c047814..a84072fbb4e7 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -596,8 +596,8 @@ struct btrfs_fs_info {
 	/* keep track of unallocated space */
 	atomic64_t free_chunk_space;
 
-	struct extent_io_tree freed_extents[2];
-	struct extent_io_tree *pinned_extents;
+	/* Tracks ranges which used by log trees blocks/logged data extents */
+	struct extent_io_tree excluded_extents;
 
 	/* logical->physical extent mapping */
 	struct extent_map_tree mapping_tree;
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 9209c7b0997c..3cb786463eb2 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2021,10 +2021,8 @@ void btrfs_free_fs_roots(struct btrfs_fs_info *fs_info)
 			btrfs_drop_and_free_fs_root(fs_info, gang[i]);
 	}
 
-	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
+	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
 		btrfs_free_log_root_tree(NULL, fs_info);
-		btrfs_destroy_pinned_extent(fs_info, fs_info->pinned_extents);
-	}
 }
 
 static void btrfs_init_scrub(struct btrfs_fs_info *fs_info)
@@ -2779,11 +2777,8 @@ int __cold open_ctree(struct super_block *sb,
 	fs_info->block_group_cache_tree = RB_ROOT;
 	fs_info->first_logical_byte = (u64)-1;
 
-	extent_io_tree_init(fs_info, &fs_info->freed_extents[0],
-			    IO_TREE_FS_INFO_FREED_EXTENTS0, NULL);
-	extent_io_tree_init(fs_info, &fs_info->freed_extents[1],
-			    IO_TREE_FS_INFO_FREED_EXTENTS1, NULL);
-	fs_info->pinned_extents = &fs_info->freed_extents[0];
+	extent_io_tree_init(fs_info, &fs_info->excluded_extents,
+			    IO_TREE_FS_INFO_EXCLUDED_EXTENTS, NULL);
 	set_bit(BTRFS_FS_BARRIER, &fs_info->flags);
 
 	mutex_init(&fs_info->ordered_operations_mutex);
@@ -4384,16 +4379,12 @@ static int btrfs_destroy_marked_extents(struct btrfs_fs_info *fs_info,
 }
 
 static int btrfs_destroy_pinned_extent(struct btrfs_fs_info *fs_info,
-				       struct extent_io_tree *pinned_extents)
+				       struct extent_io_tree *unpin)
 {
-	struct extent_io_tree *unpin;
 	u64 start;
 	u64 end;
 	int ret;
-	bool loop = true;
 
-	unpin = pinned_extents;
-again:
 	while (1) {
 		struct extent_state *cached_state = NULL;
 
@@ -4418,15 +4409,6 @@ static int btrfs_destroy_pinned_extent(struct btrfs_fs_info *fs_info,
 		cond_resched();
 	}
 
-	if (loop) {
-		if (unpin == &fs_info->freed_extents[0])
-			unpin = &fs_info->freed_extents[1];
-		else
-			unpin = &fs_info->freed_extents[0];
-		loop = false;
-		goto again;
-	}
-
 	return 0;
 }
 
@@ -4518,8 +4500,7 @@ void btrfs_cleanup_one_transaction(struct btrfs_transaction *cur_trans,
 
 	btrfs_destroy_marked_extents(fs_info, &cur_trans->dirty_pages,
 				     EXTENT_DIRTY);
-	btrfs_destroy_pinned_extent(fs_info,
-				    fs_info->pinned_extents);
+	btrfs_destroy_pinned_extent(fs_info, &cur_trans->pinned_extents);
 
 	cur_trans->state =TRANS_STATE_COMPLETED;
 	wake_up(&cur_trans->commit_wait);
@@ -4571,7 +4552,6 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
 	btrfs_destroy_all_ordered_extents(fs_info);
 	btrfs_destroy_delayed_inodes(fs_info);
 	btrfs_assert_delayed_root_empty(fs_info);
-	btrfs_destroy_pinned_extent(fs_info, fs_info->pinned_extents);
 	btrfs_destroy_all_delalloc_inodes(fs_info);
 	mutex_unlock(&fs_info->transaction_kthread_mutex);
 
diff --git a/fs/btrfs/extent-io-tree.h b/fs/btrfs/extent-io-tree.h
index a3febe746c79..02c2e4e711bf 100644
--- a/fs/btrfs/extent-io-tree.h
+++ b/fs/btrfs/extent-io-tree.h
@@ -36,8 +36,7 @@ struct io_failure_record;
 #define CHUNK_TRIMMED				EXTENT_DEFRAG
 
 enum {
-	IO_TREE_FS_INFO_FREED_EXTENTS0,
-	IO_TREE_FS_INFO_FREED_EXTENTS1,
+	IO_TREE_FS_INFO_EXCLUDED_EXTENTS,
 	IO_TREE_INODE_IO,
 	IO_TREE_INODE_IO_FAILURE,
 	IO_TREE_RELOC_BLOCKS,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index d680f2ac336b..f7b89ab921e1 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -64,10 +64,8 @@ int btrfs_add_excluded_extent(struct btrfs_fs_info *fs_info,
 			      u64 start, u64 num_bytes)
 {
 	u64 end = start + num_bytes - 1;
-	set_extent_bits(&fs_info->freed_extents[0],
-			start, end, EXTENT_UPTODATE);
-	set_extent_bits(&fs_info->freed_extents[1],
-			start, end, EXTENT_UPTODATE);
+	set_extent_bits(&fs_info->excluded_extents, start, end,
+			EXTENT_UPTODATE);
 	return 0;
 }
 
@@ -79,10 +77,8 @@ void btrfs_free_excluded_extents(struct btrfs_block_group *cache)
 	start = cache->start;
 	end = start + cache->length - 1;
 
-	clear_extent_bits(&fs_info->freed_extents[0],
-			  start, end, EXTENT_UPTODATE);
-	clear_extent_bits(&fs_info->freed_extents[1],
-			  start, end, EXTENT_UPTODATE);
+	clear_extent_bits(&fs_info->excluded_extents, start, end,
+			  EXTENT_UPTODATE);
 }
 
 static u64 generic_ref_to_space_flags(struct btrfs_ref *ref)
@@ -2606,7 +2602,7 @@ static int pin_down_extent(struct btrfs_trans_handle *trans,
 
 	percpu_counter_add_batch(&cache->space_info->total_bytes_pinned,
 		    num_bytes, BTRFS_TOTAL_BYTES_PINNED_BATCH);
-	set_extent_dirty(fs_info->pinned_extents, bytenr,
+	set_extent_dirty(&trans->transaction->pinned_extents, bytenr,
 			 bytenr + num_bytes - 1, GFP_NOFS | __GFP_NOFAIL);
 	return 0;
 }
@@ -2762,11 +2758,6 @@ void btrfs_prepare_extent_commit(struct btrfs_fs_info *fs_info)
 		}
 	}
 
-	if (fs_info->pinned_extents == &fs_info->freed_extents[0])
-		fs_info->pinned_extents = &fs_info->freed_extents[1];
-	else
-		fs_info->pinned_extents = &fs_info->freed_extents[0];
-
 	up_write(&fs_info->commit_root_sem);
 
 	btrfs_update_global_block_rsv(fs_info);
@@ -2907,10 +2898,7 @@ int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans)
 	u64 end;
 	int ret;
 
-	if (fs_info->pinned_extents == &fs_info->freed_extents[0])
-		unpin = &fs_info->freed_extents[1];
-	else
-		unpin = &fs_info->freed_extents[0];
+	unpin = &trans->transaction->pinned_extents;
 
 	while (!trans->aborted) {
 		struct extent_state *cached_state = NULL;
@@ -2922,12 +2910,9 @@ int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans)
 			mutex_unlock(&fs_info->unused_bg_unpin_mutex);
 			break;
 		}
-		if (test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags)) {
-			clear_extent_bits(&fs_info->freed_extents[0], start,
+		if (test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags))
+			clear_extent_bits(&fs_info->excluded_extents, start,
 					  end, EXTENT_UPTODATE);
-			clear_extent_bits(&fs_info->freed_extents[1], start,
-					  end, EXTENT_UPTODATE);
-		}
 
 		if (btrfs_test_opt(fs_info, DISCARD_SYNC))
 			ret = btrfs_discard_extent(fs_info, start,
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 9d6372139547..bd9c4b5da549 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -1086,7 +1086,7 @@ static noinline_for_stack int write_pinned_extent_entries(
 	 * We shouldn't have switched the pinned extents yet so this is the
 	 * right one
 	 */
-	unpin = block_group->fs_info->pinned_extents;
+	unpin = &trans->transaction->pinned_extents;
 
 	start = block_group->start;
 
diff --git a/fs/btrfs/tests/btrfs-tests.c b/fs/btrfs/tests/btrfs-tests.c
index c12b91ff5f56..2f7514068c3f 100644
--- a/fs/btrfs/tests/btrfs-tests.c
+++ b/fs/btrfs/tests/btrfs-tests.c
@@ -156,12 +156,9 @@ struct btrfs_fs_info *btrfs_alloc_dummy_fs_info(u32 nodesize, u32 sectorsize)
 	INIT_LIST_HEAD(&fs_info->fs_devices->devices);
 	INIT_RADIX_TREE(&fs_info->buffer_radix, GFP_ATOMIC);
 	INIT_RADIX_TREE(&fs_info->fs_roots_radix, GFP_ATOMIC);
-	extent_io_tree_init(fs_info, &fs_info->freed_extents[0],
-			    IO_TREE_FS_INFO_FREED_EXTENTS0, NULL);
-	extent_io_tree_init(fs_info, &fs_info->freed_extents[1],
-			    IO_TREE_FS_INFO_FREED_EXTENTS1, NULL);
+	extent_io_tree_init(fs_info, &fs_info->excluded_extents,
+			    IO_TREE_FS_INFO_EXCLUDED_EXTENTS, NULL);
 	extent_map_tree_init(&fs_info->mapping_tree);
-	fs_info->pinned_extents = &fs_info->freed_extents[0];
 	set_bit(BTRFS_FS_STATE_DUMMY_FS_INFO, &fs_info->fs_state);
 
 	test_mnt->mnt_sb->s_fs_info = fs_info;
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 55d8fd68775a..4b9091582ec4 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -334,6 +334,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
 	list_add_tail(&cur_trans->list, &fs_info->trans_list);
 	extent_io_tree_init(fs_info, &cur_trans->dirty_pages,
 			IO_TREE_TRANS_DIRTY_PAGES, fs_info->btree_inode);
+	extent_io_tree_init(fs_info, &cur_trans->pinned_extents, 0, NULL);
 	fs_info->generation++;
 	cur_trans->transid = fs_info->generation;
 	fs_info->running_transaction = cur_trans;
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index 49f7196368f5..2a046a0155c1 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -71,6 +71,7 @@ struct btrfs_transaction {
 	 */
 	struct list_head io_bgs;
 	struct list_head dropped_roots;
+	struct extent_io_tree pinned_extents;
 
 	/*
 	 * we need to make sure block group deletion doesn't race with
diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
index 17088a112ed0..f4fa3e15297d 100644
--- a/include/trace/events/btrfs.h
+++ b/include/trace/events/btrfs.h
@@ -81,8 +81,7 @@ TRACE_DEFINE_ENUM(COMMIT_TRANS);
 
 #define show_extent_io_tree_owner(owner)				       \
 	__print_symbolic(owner,						       \
-		{ IO_TREE_FS_INFO_FREED_EXTENTS0, "FREED_EXTENTS0" },	       \
-		{ IO_TREE_FS_INFO_FREED_EXTENTS1, "FREED_EXTENTS1" },	       \
+		{ IO_TREE_FS_INFO_EXCLUDED_EXTENTS, "EXCLUDED_EXTENTS" }, \
 		{ IO_TREE_INODE_IO,		  "INODE_IO" },		       \
 		{ IO_TREE_INODE_IO_FAILURE,	  "INODE_IO_FAILURE" },	       \
 		{ IO_TREE_RELOC_BLOCKS,		  "RELOC_BLOCKS" },	       \
-- 
2.17.1

Re: [PATCH 01/11] btrfs: Perform pinned cleanup directly in btrfs_destroy_delayed_refs

[Josef Bacik]

On 1/20/20 9:09 AM, Nikolay Borisov wrote:
> Having btrfs_destroy_delayed_refs call btrfs_pin_extent is problematic
> for making pinned extents tracking per-transaction since
> btrfs_trans_handle cannot be passed to btrfs_pin_extent in this context.
> Additionally delayed refs heads pinned in btrfs_destroy_delayed_refs
> are going to be handled very closely, in btrfs_destroy_pinned_extent.
> 
> To enable btrfs_pin_extent to take btrfs_trans_handle simply open code
> it in btrfs_destroy_delayed_refs and call btrfs_error_unpin_extent_range
> on the range. This enables us to do less work in
> btrfs_destroy_pinned_extent and leaves btrfs_pin_extent being called in
> contexts which have a valid btrfs_trans_handle.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 02/11] btrfs: Make btrfs_pin_extent take trans handle

[Josef Bacik]

On 1/20/20 9:09 AM, Nikolay Borisov wrote:
> Preparation for switching pinned extent tracking to a per-transaction
> basis.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 03/11] btrfs: Introduce unaccount_log_buffer

[Josef Bacik]

On 1/20/20 9:09 AM, Nikolay Borisov wrote:
> This function correctly adjusts the reserved bytes occupied by a
> log tree extent buffer. It will be used instead of calling
> btrfs_pin_reserved_extent.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 04/11] btrfs: Call btrfs_pin_reserved_extent only during active transaction

[Josef Bacik]

On 1/20/20 9:09 AM, Nikolay Borisov wrote:
> Calling btrfs_pin_reserved_extent makes sense only with a valid
> transaction since pinned extents are processed from transaction commit
> in btrfs_finish_extent_commit. In case of error it's sufficient to
> adjust the reserved counter to account for log tree extents allocated in
> the last transaction.
> 
> This commit moves btrfs_pin_reserved_extent to be called only with
> valid transaction handle and otherwise uses the newly introduced
> unaccount_log_buffer to adjust "reserved". If this is not done if a
> failure occurs before transaction is committed WARN_ON are going to
> be triggered on unmount. This was especially pronounced with generic/475
> test.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 05/11] btrfs: Make btrfs_pin_reserved_extent take transaction

[Josef Bacik]

On 1/20/20 9:09 AM, Nikolay Borisov wrote:
> btrfs_pin_reserved_extent is now only called with a valid transaction so
> exploit the fact to take a transaction. This is preparation for tracking
> pinned extents on a per-transaction basis.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 06/11] btrfs: Make btrfs_pin_extent_for_log_replay take transaction handle

[Josef Bacik]

On 1/20/20 9:09 AM, Nikolay Borisov wrote:
> Preparation for refactoring pinned extents tracking.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 07/11] btrfs: Make pin_down_extent take btrfs_trans_handle

[Josef Bacik]

On 1/20/20 9:09 AM, Nikolay Borisov wrote:
> All callers have a reference to a transaction handle so pass it to
> pin_down_extent. This is the final step before switching pinned extent
> tracking to a per-transaction basis.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 08/11] btrfs: Pass trans handle to write_pinned_extent_entries

[Josef Bacik]

On 1/20/20 9:09 AM, Nikolay Borisov wrote:
> Preparation for refactoring pinned extents tracking.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 09/11] btrfs: Mark pinned log extents as excluded

[Josef Bacik]

On 1/20/20 9:09 AM, Nikolay Borisov wrote:
> In preparation to making pinned extents per-transaction ensure that
> log such extents are always excluded from caching. To achieve this in
> addition to marking them via btrfs_pin_extent_for_log_replay they also
> need to be marked with btrfs_add_excluded_extent to prevent log tree
> extent buffer being loaded by the free space caching thread. That's
> required since log treeblocks are not recorded in the extent tree, hence
> they always look free.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>   fs/btrfs/extent-tree.c | 8 ++++++++
>   fs/btrfs/tree-log.c    | 2 +-
>   2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 7dcf9217a622..d680f2ac336b 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2634,6 +2634,8 @@ int btrfs_pin_extent_for_log_replay(struct btrfs_trans_handle *trans,
>   	struct btrfs_block_group *cache;
>   	int ret;
>   
> +	btrfs_add_excluded_extent(trans->fs_info, bytenr, num_bytes);
> +
>   	cache = btrfs_lookup_block_group(trans->fs_info, bytenr);
>   	if (!cache)
>   		return -EINVAL;
> @@ -2920,6 +2922,12 @@ int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans)
>   			mutex_unlock(&fs_info->unused_bg_unpin_mutex);
>   			break;
>   		}
> +		if (test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags)) {
> +			clear_extent_bits(&fs_info->freed_extents[0], start,
> +					  end, EXTENT_UPTODATE);
> +			clear_extent_bits(&fs_info->freed_extents[1], start,
> +					  end, EXTENT_UPTODATE);
> +		}
>   
>   		if (btrfs_test_opt(fs_info, DISCARD_SYNC))
>   			ret = btrfs_discard_extent(fs_info, start,
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index b535d409a728..f89de24838d5 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -2994,7 +2994,7 @@ static inline void btrfs_remove_log_ctx(struct btrfs_root *root,
>   	mutex_unlock(&root->log_mutex);
>   }
>   
> -/*
> +/*

nit: this part needs to be dropped.  Other than that

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 10/11] btrfs: Factor out pinned extent clean up in btrfs_delete_unused_bgs

[Josef Bacik]

On 1/20/20 9:09 AM, Nikolay Borisov wrote:
> Next patch is going to refactor how pinned extents are tracked which
> will necessitate changing this code. To ease that work and contain the
> changes factor the code now in preparation, this will also help review.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 11/11] btrfs: Use btrfs_transaction::pinned_extents

[Josef Bacik]

On 1/20/20 9:09 AM, Nikolay Borisov wrote:
> This commit flips the switch to start tracking/processing pinned
> extents on a per-transaction basis. It mostly replaces all references
> from btrfs_fs_info::(pinned_extents|freed_extents[]) to
> btrfs_transaction::pinned_extents. Two notable modifications that
> warrant explicit mention are changing clean_pinned_extents to get a
> reference to the previously running transaction. The other one is
> removal of call to btrfs_destroy_pinned_extent since transactions are
> going to be cleaned in btrfs_cleanup_one_transaction.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

I'd prefer if the excluded extent changes were separate from the pinned extent 
changes.

> ---
>   fs/btrfs/block-group.c       | 38 ++++++++++++++++++++++++------------
>   fs/btrfs/ctree.h             |  4 ++--
>   fs/btrfs/disk-io.c           | 30 +++++-----------------------
>   fs/btrfs/extent-io-tree.h    |  3 +--
>   fs/btrfs/extent-tree.c       | 31 ++++++++---------------------
>   fs/btrfs/free-space-cache.c  |  2 +-
>   fs/btrfs/tests/btrfs-tests.c |  7 ++-----
>   fs/btrfs/transaction.c       |  1 +
>   fs/btrfs/transaction.h       |  1 +
>   include/trace/events/btrfs.h |  3 +--
>   10 files changed, 47 insertions(+), 73 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 48bb9e08f2e8..562dfb7dc77f 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -460,7 +460,7 @@ u64 add_new_free_space(struct btrfs_block_group *block_group, u64 start, u64 end
>   	int ret;
>   
>   	while (start < end) {
> -		ret = find_first_extent_bit(info->pinned_extents, start,
> +		ret = find_first_extent_bit(&info->excluded_extents, start,
>   					    &extent_start, &extent_end,
>   					    EXTENT_DIRTY | EXTENT_UPTODATE,
>   					    NULL);

We're no longer doing EXTENT_DIRTY in excluded_extents, so we don't need this part.

> @@ -1233,32 +1233,44 @@ static int inc_block_group_ro(struct btrfs_block_group *cache, int force)
>   	return ret;
>   }
>   
> -static bool clean_pinned_extents(struct btrfs_block_group *bg)
> +static bool clean_pinned_extents(struct btrfs_trans_handle *trans,
> +				 struct btrfs_block_group *bg)
>   {
>   	struct btrfs_fs_info *fs_info = bg->fs_info;
> +	struct btrfs_transaction *prev_trans = NULL;
>   	u64 start = bg->start;
>   	u64 end = start + bg->length - 1;
>   	int ret;
>   
> +	spin_lock(&fs_info->trans_lock);
> +	if (trans->transaction->list.prev != &fs_info->trans_list) {
> +		prev_trans = list_entry(trans->transaction->list.prev,
> +					struct btrfs_transaction, list);
> +		refcount_inc(&prev_trans->use_count);
> +	}
> +	spin_unlock(&fs_info->trans_lock);
> +
>   	/*
>   	 * Hold the unused_bg_unpin_mutex lock to avoid racing with
>   	 * btrfs_finish_extent_commit(). If we are at transaction N,
>   	 * another task might be running finish_extent_commit() for the
>   	 * previous transaction N - 1, and have seen a range belonging
> -	 * to the block group in freed_extents[] before we were able to
> -	 * clear the whole block group range from freed_extents[]. This
> +	 * to the block group in pinned_extents before we were able to
> +	 * clear the whole block group range from pinned_extents. This
>   	 * means that task can lookup for the block group after we
> -	 * unpinned it from freed_extents[] and removed it, leading to
> +	 * unpinned it from pinned_extents[] and removed it, leading to
>   	 * a BUG_ON() at unpin_extent_range().
>   	 */
>   	mutex_lock(&fs_info->unused_bg_unpin_mutex);
> -	ret = clear_extent_bits(&fs_info->freed_extents[0], start, end,
> -			  EXTENT_DIRTY);
> -	if (ret)
> -		goto failure;
> +	if (prev_trans) {
> +		ret = clear_extent_bits(&prev_trans->pinned_extents, start, end,
> +					EXTENT_DIRTY);
> +		if (ret)
> +			goto failure;
> +	}

You are leaking a ref to prev_trans here.

<snip>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 9209c7b0997c..3cb786463eb2 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2021,10 +2021,8 @@ void btrfs_free_fs_roots(struct btrfs_fs_info *fs_info)
>   			btrfs_drop_and_free_fs_root(fs_info, gang[i]);
>   	}
>   
> -	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
> +	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
>   		btrfs_free_log_root_tree(NULL, fs_info);
> -		btrfs_destroy_pinned_extent(fs_info, fs_info->pinned_extents);
> -	}

What about the excluded extents?  We may never cache the block group with one of 
the super mirrors in it, and thus we would leak the excluded extent for it.  Thanks,

Josef

Re: [PATCH 11/11] btrfs: Use btrfs_transaction::pinned_extents

[Nikolay Borisov]

On 22.01.20 г. 22:21 ч., Josef Bacik wrote:
> On 1/20/20 9:09 AM, Nikolay Borisov wrote:
>> This commit flips the switch to start tracking/processing pinned
>> extents on a per-transaction basis. It mostly replaces all references
>> from btrfs_fs_info::(pinned_extents|freed_extents[]) to
>> btrfs_transaction::pinned_extents. Two notable modifications that
>> warrant explicit mention are changing clean_pinned_extents to get a
>> reference to the previously running transaction. The other one is
>> removal of call to btrfs_destroy_pinned_extent since transactions are
>> going to be cleaned in btrfs_cleanup_one_transaction.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> 
> I'd prefer if the excluded extent changes were separate from the pinned
> extent changes.
> 
>> ---
>>   fs/btrfs/block-group.c       | 38 ++++++++++++++++++++++++------------
>>   fs/btrfs/ctree.h             |  4 ++--
>>   fs/btrfs/disk-io.c           | 30 +++++-----------------------
>>   fs/btrfs/extent-io-tree.h    |  3 +--
>>   fs/btrfs/extent-tree.c       | 31 ++++++++---------------------
>>   fs/btrfs/free-space-cache.c  |  2 +-
>>   fs/btrfs/tests/btrfs-tests.c |  7 ++-----
>>   fs/btrfs/transaction.c       |  1 +
>>   fs/btrfs/transaction.h       |  1 +
>>   include/trace/events/btrfs.h |  3 +--
>>   10 files changed, 47 insertions(+), 73 deletions(-)
>>
>> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
>> index 48bb9e08f2e8..562dfb7dc77f 100644
>> --- a/fs/btrfs/block-group.c
>> +++ b/fs/btrfs/block-group.c
>> @@ -460,7 +460,7 @@ u64 add_new_free_space(struct btrfs_block_group
>> *block_group, u64 start, u64 end
>>       int ret;
>>         while (start < end) {
>> -        ret = find_first_extent_bit(info->pinned_extents, start,
>> +        ret = find_first_extent_bit(&info->excluded_extents, start,
>>                           &extent_start, &extent_end,
>>                           EXTENT_DIRTY | EXTENT_UPTODATE,
>>                           NULL);
> 
> We're no longer doing EXTENT_DIRTY in excluded_extents, so we don't need
> this part.
> 
>> @@ -1233,32 +1233,44 @@ static int inc_block_group_ro(struct
>> btrfs_block_group *cache, int force)
>>       return ret;
>>   }
>>   -static bool clean_pinned_extents(struct btrfs_block_group *bg)
>> +static bool clean_pinned_extents(struct btrfs_trans_handle *trans,
>> +                 struct btrfs_block_group *bg)
>>   {
>>       struct btrfs_fs_info *fs_info = bg->fs_info;
>> +    struct btrfs_transaction *prev_trans = NULL;
>>       u64 start = bg->start;
>>       u64 end = start + bg->length - 1;
>>       int ret;
>>   +    spin_lock(&fs_info->trans_lock);
>> +    if (trans->transaction->list.prev != &fs_info->trans_list) {
>> +        prev_trans = list_entry(trans->transaction->list.prev,
>> +                    struct btrfs_transaction, list);
>> +        refcount_inc(&prev_trans->use_count);
>> +    }
>> +    spin_unlock(&fs_info->trans_lock);
>> +
>>       /*
>>        * Hold the unused_bg_unpin_mutex lock to avoid racing with
>>        * btrfs_finish_extent_commit(). If we are at transaction N,
>>        * another task might be running finish_extent_commit() for the
>>        * previous transaction N - 1, and have seen a range belonging
>> -     * to the block group in freed_extents[] before we were able to
>> -     * clear the whole block group range from freed_extents[]. This
>> +     * to the block group in pinned_extents before we were able to
>> +     * clear the whole block group range from pinned_extents. This
>>        * means that task can lookup for the block group after we
>> -     * unpinned it from freed_extents[] and removed it, leading to
>> +     * unpinned it from pinned_extents[] and removed it, leading to
>>        * a BUG_ON() at unpin_extent_range().
>>        */
>>       mutex_lock(&fs_info->unused_bg_unpin_mutex);
>> -    ret = clear_extent_bits(&fs_info->freed_extents[0], start, end,
>> -              EXTENT_DIRTY);
>> -    if (ret)
>> -        goto failure;
>> +    if (prev_trans) {
>> +        ret = clear_extent_bits(&prev_trans->pinned_extents, start, end,
>> +                    EXTENT_DIRTY);
>> +        if (ret)
>> +            goto failure;
>> +    }
> 
> You are leaking a ref to prev_trans here.
> 
> <snip>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 9209c7b0997c..3cb786463eb2 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -2021,10 +2021,8 @@ void btrfs_free_fs_roots(struct btrfs_fs_info
>> *fs_info)
>>               btrfs_drop_and_free_fs_root(fs_info, gang[i]);
>>       }
>>   -    if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
>> +    if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
>>           btrfs_free_log_root_tree(NULL, fs_info);
>> -        btrfs_destroy_pinned_extent(fs_info, fs_info->pinned_extents);
>> -    }
> 
> What about the excluded extents?  We may never cache the block group
> with one of the super mirrors in it, and thus we would leak the excluded
> extent for it.  Thanks,

btrfs_destroy_pinned_extent didn't touch EXTENT_UPDATE (excluded
extents) before so my removing this call doesn't change that. E.g. if
there is a bug here where excluded extents are not cleaned up then it's
not due to my code.

On the other hand I don't quite understand your concern w.r.t pinned
extents. Can you elaborate?

> 
> Josef

Re: [PATCH 11/11] btrfs: Use btrfs_transaction::pinned_extents

[Josef Bacik]

On 1/23/20 3:54 AM, Nikolay Borisov wrote:
> 
> 
> On 22.01.20 г. 22:21 ч., Josef Bacik wrote:
>> On 1/20/20 9:09 AM, Nikolay Borisov wrote:
>>> This commit flips the switch to start tracking/processing pinned
>>> extents on a per-transaction basis. It mostly replaces all references
>>> from btrfs_fs_info::(pinned_extents|freed_extents[]) to
>>> btrfs_transaction::pinned_extents. Two notable modifications that
>>> warrant explicit mention are changing clean_pinned_extents to get a
>>> reference to the previously running transaction. The other one is
>>> removal of call to btrfs_destroy_pinned_extent since transactions are
>>> going to be cleaned in btrfs_cleanup_one_transaction.
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>
>> I'd prefer if the excluded extent changes were separate from the pinned
>> extent changes.
>>
>>> ---
>>>    fs/btrfs/block-group.c       | 38 ++++++++++++++++++++++++------------
>>>    fs/btrfs/ctree.h             |  4 ++--
>>>    fs/btrfs/disk-io.c           | 30 +++++-----------------------
>>>    fs/btrfs/extent-io-tree.h    |  3 +--
>>>    fs/btrfs/extent-tree.c       | 31 ++++++++---------------------
>>>    fs/btrfs/free-space-cache.c  |  2 +-
>>>    fs/btrfs/tests/btrfs-tests.c |  7 ++-----
>>>    fs/btrfs/transaction.c       |  1 +
>>>    fs/btrfs/transaction.h       |  1 +
>>>    include/trace/events/btrfs.h |  3 +--
>>>    10 files changed, 47 insertions(+), 73 deletions(-)
>>>
>>> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
>>> index 48bb9e08f2e8..562dfb7dc77f 100644
>>> --- a/fs/btrfs/block-group.c
>>> +++ b/fs/btrfs/block-group.c
>>> @@ -460,7 +460,7 @@ u64 add_new_free_space(struct btrfs_block_group
>>> *block_group, u64 start, u64 end
>>>        int ret;
>>>          while (start < end) {
>>> -        ret = find_first_extent_bit(info->pinned_extents, start,
>>> +        ret = find_first_extent_bit(&info->excluded_extents, start,
>>>                            &extent_start, &extent_end,
>>>                            EXTENT_DIRTY | EXTENT_UPTODATE,
>>>                            NULL);
>>
>> We're no longer doing EXTENT_DIRTY in excluded_extents, so we don't need
>> this part.
>>
>>> @@ -1233,32 +1233,44 @@ static int inc_block_group_ro(struct
>>> btrfs_block_group *cache, int force)
>>>        return ret;
>>>    }
>>>    -static bool clean_pinned_extents(struct btrfs_block_group *bg)
>>> +static bool clean_pinned_extents(struct btrfs_trans_handle *trans,
>>> +                 struct btrfs_block_group *bg)
>>>    {
>>>        struct btrfs_fs_info *fs_info = bg->fs_info;
>>> +    struct btrfs_transaction *prev_trans = NULL;
>>>        u64 start = bg->start;
>>>        u64 end = start + bg->length - 1;
>>>        int ret;
>>>    +    spin_lock(&fs_info->trans_lock);
>>> +    if (trans->transaction->list.prev != &fs_info->trans_list) {
>>> +        prev_trans = list_entry(trans->transaction->list.prev,
>>> +                    struct btrfs_transaction, list);
>>> +        refcount_inc(&prev_trans->use_count);
>>> +    }
>>> +    spin_unlock(&fs_info->trans_lock);
>>> +
>>>        /*
>>>         * Hold the unused_bg_unpin_mutex lock to avoid racing with
>>>         * btrfs_finish_extent_commit(). If we are at transaction N,
>>>         * another task might be running finish_extent_commit() for the
>>>         * previous transaction N - 1, and have seen a range belonging
>>> -     * to the block group in freed_extents[] before we were able to
>>> -     * clear the whole block group range from freed_extents[]. This
>>> +     * to the block group in pinned_extents before we were able to
>>> +     * clear the whole block group range from pinned_extents. This
>>>         * means that task can lookup for the block group after we
>>> -     * unpinned it from freed_extents[] and removed it, leading to
>>> +     * unpinned it from pinned_extents[] and removed it, leading to
>>>         * a BUG_ON() at unpin_extent_range().
>>>         */
>>>        mutex_lock(&fs_info->unused_bg_unpin_mutex);
>>> -    ret = clear_extent_bits(&fs_info->freed_extents[0], start, end,
>>> -              EXTENT_DIRTY);
>>> -    if (ret)
>>> -        goto failure;
>>> +    if (prev_trans) {
>>> +        ret = clear_extent_bits(&prev_trans->pinned_extents, start, end,
>>> +                    EXTENT_DIRTY);
>>> +        if (ret)
>>> +            goto failure;
>>> +    }
>>
>> You are leaking a ref to prev_trans here.
>>
>> <snip>
>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>> index 9209c7b0997c..3cb786463eb2 100644
>>> --- a/fs/btrfs/disk-io.c
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -2021,10 +2021,8 @@ void btrfs_free_fs_roots(struct btrfs_fs_info
>>> *fs_info)
>>>                btrfs_drop_and_free_fs_root(fs_info, gang[i]);
>>>        }
>>>    -    if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
>>> +    if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
>>>            btrfs_free_log_root_tree(NULL, fs_info);
>>> -        btrfs_destroy_pinned_extent(fs_info, fs_info->pinned_extents);
>>> -    }
>>
>> What about the excluded extents?  We may never cache the block group
>> with one of the super mirrors in it, and thus we would leak the excluded
>> extent for it.  Thanks,
> 
> btrfs_destroy_pinned_extent didn't touch EXTENT_UPDATE (excluded
> extents) before so my removing this call doesn't change that. E.g. if
> there is a bug here where excluded extents are not cleaned up then it's
> not due to my code.
> 
> On the other hand I don't quite understand your concern w.r.t pinned
> extents. Can you elaborate?
> 

Sorry thunderbird ate my followup, we drop the excluded extents in 
btrfs_free_block_groups() if they are never cached, so you are fine here.  Thanks,

Josef

[PATCH v2] btrfs: Use btrfs_transaction::pinned_extents

[Nikolay Borisov]

This commit flips the switch to start tracking/processing pinned
extents on a per-transaction basis. It mostly replaces all references
from btrfs_fs_info::(pinned_extents|freed_extents[]) to
btrfs_transaction::pinned_extents. Two notable modifications that
warrant explicit mention are changing clean_pinned_extents to get a
reference to the previously running transaction. The other one is
removal of call to btrfs_destroy_pinned_extent since transactions are
going to be cleaned in btrfs_cleanup_one_transaction.

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

V2:
 * Properly release transaction in clean_pinned_extents [Josef]
 * Only check for EXTENT_UPTODATE in add_new_free_space [Josef]

 fs/btrfs/block-group.c       | 43 +++++++++++++++++++++++-------------
 fs/btrfs/ctree.h             |  4 ++--
 fs/btrfs/disk-io.c           | 30 +++++--------------------
 fs/btrfs/extent-io-tree.h    |  3 +--
 fs/btrfs/extent-tree.c       | 31 +++++++-------------------
 fs/btrfs/free-space-cache.c  |  2 +-
 fs/btrfs/tests/btrfs-tests.c |  7 ++----
 fs/btrfs/transaction.c       |  1 +
 fs/btrfs/transaction.h       |  1 +
 include/trace/events/btrfs.h |  3 +--
 10 files changed, 50 insertions(+), 75 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 48bb9e08f2e8..91054c32d88a 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -460,10 +460,9 @@ u64 add_new_free_space(struct btrfs_block_group *block_group, u64 start, u64 end
 	int ret;

 	while (start < end) {
-		ret = find_first_extent_bit(info->pinned_extents, start,
+		ret = find_first_extent_bit(&info->excluded_extents, start,
 					    &extent_start, &extent_end,
-					    EXTENT_DIRTY | EXTENT_UPTODATE,
-					    NULL);
+					    EXTENT_UPTODATE, NULL);
 		if (ret)
 			break;

@@ -1233,32 +1232,46 @@ static int inc_block_group_ro(struct btrfs_block_group *cache, int force)
 	return ret;
 }

-static bool clean_pinned_extents(struct btrfs_block_group *bg)
+static bool clean_pinned_extents(struct btrfs_trans_handle *trans,
+				 struct btrfs_block_group *bg)
 {
 	struct btrfs_fs_info *fs_info = bg->fs_info;
+	struct btrfs_transaction *prev_trans = NULL;
 	u64 start = bg->start;
 	u64 end = start + bg->length - 1;
 	int ret;

+	spin_lock(&fs_info->trans_lock);
+	if (trans->transaction->list.prev != &fs_info->trans_list) {
+		prev_trans = list_entry(trans->transaction->list.prev,
+					struct btrfs_transaction, list);
+		refcount_inc(&prev_trans->use_count);
+	}
+	spin_unlock(&fs_info->trans_lock);
+
 	/*
 	 * Hold the unused_bg_unpin_mutex lock to avoid racing with
 	 * btrfs_finish_extent_commit(). If we are at transaction N,
 	 * another task might be running finish_extent_commit() for the
 	 * previous transaction N - 1, and have seen a range belonging
-	 * to the block group in freed_extents[] before we were able to
-	 * clear the whole block group range from freed_extents[]. This
+	 * to the block group in pinned_extents before we were able to
+	 * clear the whole block group range from pinned_extents. This
 	 * means that task can lookup for the block group after we
-	 * unpinned it from freed_extents[] and removed it, leading to
+	 * unpinned it from pinned_extents[] and removed it, leading to
 	 * a BUG_ON() at unpin_extent_range().
 	 */
 	mutex_lock(&fs_info->unused_bg_unpin_mutex);
-	ret = clear_extent_bits(&fs_info->freed_extents[0], start, end,
-			  EXTENT_DIRTY);
-	if (ret)
-		goto failure;
+	if (prev_trans) {
+		ret = clear_extent_bits(&prev_trans->pinned_extents, start, end,
+					EXTENT_DIRTY);
+		if (ret)
+			goto failure;
+	}
+
+	btrfs_put_transaction(prev_trans);

-	ret = clear_extent_bits(&fs_info->freed_extents[1], start, end,
-			  EXTENT_DIRTY);
+	ret = clear_extent_bits(&trans->transaction->pinned_extents, start, end,
+				EXTENT_DIRTY);
 	if (ret)
 		goto failure;
 	mutex_unlock(&fs_info->unused_bg_unpin_mutex);
@@ -1367,7 +1380,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 		 * We could have pending pinned extents for this block group,
 		 * just delete them, we don't care about them anymore.
 		 */
-		if (!clean_pinned_extents(block_group))
+		if (!clean_pinned_extents(trans, block_group))
 			goto end_trans;

 		/*
@@ -2877,7 +2890,7 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans,
 					&cache->space_info->total_bytes_pinned,
 					num_bytes,
 					BTRFS_TOTAL_BYTES_PINNED_BATCH);
-			set_extent_dirty(info->pinned_extents,
+			set_extent_dirty(&trans->transaction->pinned_extents,
 					 bytenr, bytenr + num_bytes - 1,
 					 GFP_NOFS | __GFP_NOFAIL);
 		}
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index c6ce8c047814..a84072fbb4e7 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -596,8 +596,8 @@ struct btrfs_fs_info {
 	/* keep track of unallocated space */
 	atomic64_t free_chunk_space;

-	struct extent_io_tree freed_extents[2];
-	struct extent_io_tree *pinned_extents;
+	/* Tracks ranges which used by log trees blocks/logged data extents */
+	struct extent_io_tree excluded_extents;

 	/* logical->physical extent mapping */
 	struct extent_map_tree mapping_tree;
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 9209c7b0997c..3cb786463eb2 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2021,10 +2021,8 @@ void btrfs_free_fs_roots(struct btrfs_fs_info *fs_info)
 			btrfs_drop_and_free_fs_root(fs_info, gang[i]);
 	}

-	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
+	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
 		btrfs_free_log_root_tree(NULL, fs_info);
-		btrfs_destroy_pinned_extent(fs_info, fs_info->pinned_extents);
-	}
 }

 static void btrfs_init_scrub(struct btrfs_fs_info *fs_info)
@@ -2779,11 +2777,8 @@ int __cold open_ctree(struct super_block *sb,
 	fs_info->block_group_cache_tree = RB_ROOT;
 	fs_info->first_logical_byte = (u64)-1;

-	extent_io_tree_init(fs_info, &fs_info->freed_extents[0],
-			    IO_TREE_FS_INFO_FREED_EXTENTS0, NULL);
-	extent_io_tree_init(fs_info, &fs_info->freed_extents[1],
-			    IO_TREE_FS_INFO_FREED_EXTENTS1, NULL);
-	fs_info->pinned_extents = &fs_info->freed_extents[0];
+	extent_io_tree_init(fs_info, &fs_info->excluded_extents,
+			    IO_TREE_FS_INFO_EXCLUDED_EXTENTS, NULL);
 	set_bit(BTRFS_FS_BARRIER, &fs_info->flags);

 	mutex_init(&fs_info->ordered_operations_mutex);
@@ -4384,16 +4379,12 @@ static int btrfs_destroy_marked_extents(struct btrfs_fs_info *fs_info,
 }

 static int btrfs_destroy_pinned_extent(struct btrfs_fs_info *fs_info,
-				       struct extent_io_tree *pinned_extents)
+				       struct extent_io_tree *unpin)
 {
-	struct extent_io_tree *unpin;
 	u64 start;
 	u64 end;
 	int ret;
-	bool loop = true;

-	unpin = pinned_extents;
-again:
 	while (1) {
 		struct extent_state *cached_state = NULL;

@@ -4418,15 +4409,6 @@ static int btrfs_destroy_pinned_extent(struct btrfs_fs_info *fs_info,
 		cond_resched();
 	}

-	if (loop) {
-		if (unpin == &fs_info->freed_extents[0])
-			unpin = &fs_info->freed_extents[1];
-		else
-			unpin = &fs_info->freed_extents[0];
-		loop = false;
-		goto again;
-	}
-
 	return 0;
 }

@@ -4518,8 +4500,7 @@ void btrfs_cleanup_one_transaction(struct btrfs_transaction *cur_trans,

 	btrfs_destroy_marked_extents(fs_info, &cur_trans->dirty_pages,
 				     EXTENT_DIRTY);
-	btrfs_destroy_pinned_extent(fs_info,
-				    fs_info->pinned_extents);
+	btrfs_destroy_pinned_extent(fs_info, &cur_trans->pinned_extents);

 	cur_trans->state =TRANS_STATE_COMPLETED;
 	wake_up(&cur_trans->commit_wait);
@@ -4571,7 +4552,6 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
 	btrfs_destroy_all_ordered_extents(fs_info);
 	btrfs_destroy_delayed_inodes(fs_info);
 	btrfs_assert_delayed_root_empty(fs_info);
-	btrfs_destroy_pinned_extent(fs_info, fs_info->pinned_extents);
 	btrfs_destroy_all_delalloc_inodes(fs_info);
 	mutex_unlock(&fs_info->transaction_kthread_mutex);

diff --git a/fs/btrfs/extent-io-tree.h b/fs/btrfs/extent-io-tree.h
index a3febe746c79..02c2e4e711bf 100644
--- a/fs/btrfs/extent-io-tree.h
+++ b/fs/btrfs/extent-io-tree.h
@@ -36,8 +36,7 @@ struct io_failure_record;
 #define CHUNK_TRIMMED				EXTENT_DEFRAG

 enum {
-	IO_TREE_FS_INFO_FREED_EXTENTS0,
-	IO_TREE_FS_INFO_FREED_EXTENTS1,
+	IO_TREE_FS_INFO_EXCLUDED_EXTENTS,
 	IO_TREE_INODE_IO,
 	IO_TREE_INODE_IO_FAILURE,
 	IO_TREE_RELOC_BLOCKS,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index d680f2ac336b..f7b89ab921e1 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -64,10 +64,8 @@ int btrfs_add_excluded_extent(struct btrfs_fs_info *fs_info,
 			      u64 start, u64 num_bytes)
 {
 	u64 end = start + num_bytes - 1;
-	set_extent_bits(&fs_info->freed_extents[0],
-			start, end, EXTENT_UPTODATE);
-	set_extent_bits(&fs_info->freed_extents[1],
-			start, end, EXTENT_UPTODATE);
+	set_extent_bits(&fs_info->excluded_extents, start, end,
+			EXTENT_UPTODATE);
 	return 0;
 }

@@ -79,10 +77,8 @@ void btrfs_free_excluded_extents(struct btrfs_block_group *cache)
 	start = cache->start;
 	end = start + cache->length - 1;

-	clear_extent_bits(&fs_info->freed_extents[0],
-			  start, end, EXTENT_UPTODATE);
-	clear_extent_bits(&fs_info->freed_extents[1],
-			  start, end, EXTENT_UPTODATE);
+	clear_extent_bits(&fs_info->excluded_extents, start, end,
+			  EXTENT_UPTODATE);
 }

 static u64 generic_ref_to_space_flags(struct btrfs_ref *ref)
@@ -2606,7 +2602,7 @@ static int pin_down_extent(struct btrfs_trans_handle *trans,

 	percpu_counter_add_batch(&cache->space_info->total_bytes_pinned,
 		    num_bytes, BTRFS_TOTAL_BYTES_PINNED_BATCH);
-	set_extent_dirty(fs_info->pinned_extents, bytenr,
+	set_extent_dirty(&trans->transaction->pinned_extents, bytenr,
 			 bytenr + num_bytes - 1, GFP_NOFS | __GFP_NOFAIL);
 	return 0;
 }
@@ -2762,11 +2758,6 @@ void btrfs_prepare_extent_commit(struct btrfs_fs_info *fs_info)
 		}
 	}

-	if (fs_info->pinned_extents == &fs_info->freed_extents[0])
-		fs_info->pinned_extents = &fs_info->freed_extents[1];
-	else
-		fs_info->pinned_extents = &fs_info->freed_extents[0];
-
 	up_write(&fs_info->commit_root_sem);

 	btrfs_update_global_block_rsv(fs_info);
@@ -2907,10 +2898,7 @@ int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans)
 	u64 end;
 	int ret;

-	if (fs_info->pinned_extents == &fs_info->freed_extents[0])
-		unpin = &fs_info->freed_extents[1];
-	else
-		unpin = &fs_info->freed_extents[0];
+	unpin = &trans->transaction->pinned_extents;

 	while (!trans->aborted) {
 		struct extent_state *cached_state = NULL;
@@ -2922,12 +2910,9 @@ int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans)
 			mutex_unlock(&fs_info->unused_bg_unpin_mutex);
 			break;
 		}
-		if (test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags)) {
-			clear_extent_bits(&fs_info->freed_extents[0], start,
+		if (test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags))
+			clear_extent_bits(&fs_info->excluded_extents, start,
 					  end, EXTENT_UPTODATE);
-			clear_extent_bits(&fs_info->freed_extents[1], start,
-					  end, EXTENT_UPTODATE);
-		}

 		if (btrfs_test_opt(fs_info, DISCARD_SYNC))
 			ret = btrfs_discard_extent(fs_info, start,
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 9d6372139547..bd9c4b5da549 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -1086,7 +1086,7 @@ static noinline_for_stack int write_pinned_extent_entries(
 	 * We shouldn't have switched the pinned extents yet so this is the
 	 * right one
 	 */
-	unpin = block_group->fs_info->pinned_extents;
+	unpin = &trans->transaction->pinned_extents;

 	start = block_group->start;

diff --git a/fs/btrfs/tests/btrfs-tests.c b/fs/btrfs/tests/btrfs-tests.c
index c12b91ff5f56..2f7514068c3f 100644
--- a/fs/btrfs/tests/btrfs-tests.c
+++ b/fs/btrfs/tests/btrfs-tests.c
@@ -156,12 +156,9 @@ struct btrfs_fs_info *btrfs_alloc_dummy_fs_info(u32 nodesize, u32 sectorsize)
 	INIT_LIST_HEAD(&fs_info->fs_devices->devices);
 	INIT_RADIX_TREE(&fs_info->buffer_radix, GFP_ATOMIC);
 	INIT_RADIX_TREE(&fs_info->fs_roots_radix, GFP_ATOMIC);
-	extent_io_tree_init(fs_info, &fs_info->freed_extents[0],
-			    IO_TREE_FS_INFO_FREED_EXTENTS0, NULL);
-	extent_io_tree_init(fs_info, &fs_info->freed_extents[1],
-			    IO_TREE_FS_INFO_FREED_EXTENTS1, NULL);
+	extent_io_tree_init(fs_info, &fs_info->excluded_extents,
+			    IO_TREE_FS_INFO_EXCLUDED_EXTENTS, NULL);
 	extent_map_tree_init(&fs_info->mapping_tree);
-	fs_info->pinned_extents = &fs_info->freed_extents[0];
 	set_bit(BTRFS_FS_STATE_DUMMY_FS_INFO, &fs_info->fs_state);

 	test_mnt->mnt_sb->s_fs_info = fs_info;
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 55d8fd68775a..4b9091582ec4 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -334,6 +334,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
 	list_add_tail(&cur_trans->list, &fs_info->trans_list);
 	extent_io_tree_init(fs_info, &cur_trans->dirty_pages,
 			IO_TREE_TRANS_DIRTY_PAGES, fs_info->btree_inode);
+	extent_io_tree_init(fs_info, &cur_trans->pinned_extents, 0, NULL);
 	fs_info->generation++;
 	cur_trans->transid = fs_info->generation;
 	fs_info->running_transaction = cur_trans;
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index 49f7196368f5..2a046a0155c1 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -71,6 +71,7 @@ struct btrfs_transaction {
 	 */
 	struct list_head io_bgs;
 	struct list_head dropped_roots;
+	struct extent_io_tree pinned_extents;

 	/*
 	 * we need to make sure block group deletion doesn't race with
diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
index 17088a112ed0..f4fa3e15297d 100644
--- a/include/trace/events/btrfs.h
+++ b/include/trace/events/btrfs.h
@@ -81,8 +81,7 @@ TRACE_DEFINE_ENUM(COMMIT_TRANS);

 #define show_extent_io_tree_owner(owner)				       \
 	__print_symbolic(owner,						       \
-		{ IO_TREE_FS_INFO_FREED_EXTENTS0, "FREED_EXTENTS0" },	       \
-		{ IO_TREE_FS_INFO_FREED_EXTENTS1, "FREED_EXTENTS1" },	       \
+		{ IO_TREE_FS_INFO_EXCLUDED_EXTENTS, "EXCLUDED_EXTENTS" }, \
 		{ IO_TREE_INODE_IO,		  "INODE_IO" },		       \
 		{ IO_TREE_INODE_IO_FAILURE,	  "INODE_IO_FAILURE" },	       \
 		{ IO_TREE_RELOC_BLOCKS,		  "RELOC_BLOCKS" },	       \
--
2.17.1

Re: [PATCH v2] btrfs: Use btrfs_transaction::pinned_extents

[Josef Bacik]

On 1/24/20 5:35 AM, Nikolay Borisov wrote:
> This commit flips the switch to start tracking/processing pinned
> extents on a per-transaction basis. It mostly replaces all references
> from btrfs_fs_info::(pinned_extents|freed_extents[]) to
> btrfs_transaction::pinned_extents. Two notable modifications that
> warrant explicit mention are changing clean_pinned_extents to get a
> reference to the previously running transaction. The other one is
> removal of call to btrfs_destroy_pinned_extent since transactions are
> going to be cleaned in btrfs_cleanup_one_transaction.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> V2:
>   * Properly release transaction in clean_pinned_extents [Josef]
>   * Only check for EXTENT_UPTODATE in add_new_free_space [Josef]
> 
>   fs/btrfs/block-group.c       | 43 +++++++++++++++++++++++-------------
>   fs/btrfs/ctree.h             |  4 ++--
>   fs/btrfs/disk-io.c           | 30 +++++--------------------
>   fs/btrfs/extent-io-tree.h    |  3 +--
>   fs/btrfs/extent-tree.c       | 31 +++++++-------------------
>   fs/btrfs/free-space-cache.c  |  2 +-
>   fs/btrfs/tests/btrfs-tests.c |  7 ++----
>   fs/btrfs/transaction.c       |  1 +
>   fs/btrfs/transaction.h       |  1 +
>   include/trace/events/btrfs.h |  3 +--
>   10 files changed, 50 insertions(+), 75 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 48bb9e08f2e8..91054c32d88a 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -460,10 +460,9 @@ u64 add_new_free_space(struct btrfs_block_group *block_group, u64 start, u64 end
>   	int ret;
> 
>   	while (start < end) {
> -		ret = find_first_extent_bit(info->pinned_extents, start,
> +		ret = find_first_extent_bit(&info->excluded_extents, start,
>   					    &extent_start, &extent_end,
> -					    EXTENT_DIRTY | EXTENT_UPTODATE,
> -					    NULL);
> +					    EXTENT_UPTODATE, NULL);
>   		if (ret)
>   			break;
> 
> @@ -1233,32 +1232,46 @@ static int inc_block_group_ro(struct btrfs_block_group *cache, int force)
>   	return ret;
>   }
> 
> -static bool clean_pinned_extents(struct btrfs_block_group *bg)
> +static bool clean_pinned_extents(struct btrfs_trans_handle *trans,
> +				 struct btrfs_block_group *bg)
>   {
>   	struct btrfs_fs_info *fs_info = bg->fs_info;
> +	struct btrfs_transaction *prev_trans = NULL;
>   	u64 start = bg->start;
>   	u64 end = start + bg->length - 1;
>   	int ret;
> 
> +	spin_lock(&fs_info->trans_lock);
> +	if (trans->transaction->list.prev != &fs_info->trans_list) {
> +		prev_trans = list_entry(trans->transaction->list.prev,
> +					struct btrfs_transaction, list);
> +		refcount_inc(&prev_trans->use_count);
> +	}
> +	spin_unlock(&fs_info->trans_lock);
> +
>   	/*
>   	 * Hold the unused_bg_unpin_mutex lock to avoid racing with
>   	 * btrfs_finish_extent_commit(). If we are at transaction N,
>   	 * another task might be running finish_extent_commit() for the
>   	 * previous transaction N - 1, and have seen a range belonging
> -	 * to the block group in freed_extents[] before we were able to
> -	 * clear the whole block group range from freed_extents[]. This
> +	 * to the block group in pinned_extents before we were able to
> +	 * clear the whole block group range from pinned_extents. This
>   	 * means that task can lookup for the block group after we
> -	 * unpinned it from freed_extents[] and removed it, leading to
> +	 * unpinned it from pinned_extents[] and removed it, leading to
>   	 * a BUG_ON() at unpin_extent_range().
>   	 */
>   	mutex_lock(&fs_info->unused_bg_unpin_mutex);
> -	ret = clear_extent_bits(&fs_info->freed_extents[0], start, end,
> -			  EXTENT_DIRTY);
> -	if (ret)
> -		goto failure;
> +	if (prev_trans) {
> +		ret = clear_extent_bits(&prev_trans->pinned_extents, start, end,
> +					EXTENT_DIRTY);
> +		if (ret)
> +			goto failure;
> +	}
> +
> +	btrfs_put_transaction(prev_trans);

The failure case is missing the put.  Thanks,

Josef

[PATCH v3] btrfs: Use btrfs_transaction::pinned_extents

[Nikolay Borisov]

This commit flips the switch to start tracking/processing pinned
extents on a per-transaction basis. It mostly replaces all references
from btrfs_fs_info::(pinned_extents|freed_extents[]) to
btrfs_transaction::pinned_extents. Two notable modifications that
warrant explicit mention are changing clean_pinned_extents to get a
reference to the previously running transaction. The other one is
removal of call to btrfs_destroy_pinned_extent since transactions are
going to be cleaned in btrfs_cleanup_one_transaction.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
V3:
 * Added missing btrfs_put_transaction under failure label
 in clean_pinned_extents [Josef]

V2:
 * Properly release transaction in clean_pinned_extents [Josef]
 * Only check for EXTENT_UPTODATE in add_new_free_space [Josef]
 fs/btrfs/block-group.c       | 44 ++++++++++++++++++++++++------------
 fs/btrfs/ctree.h             |  4 ++--
 fs/btrfs/disk-io.c           | 30 ++++--------------------
 fs/btrfs/extent-io-tree.h    |  3 +--
 fs/btrfs/extent-tree.c       | 31 +++++++------------------
 fs/btrfs/free-space-cache.c  |  2 +-
 fs/btrfs/tests/btrfs-tests.c |  7 ++----
 fs/btrfs/transaction.c       |  1 +
 fs/btrfs/transaction.h       |  1 +
 include/trace/events/btrfs.h |  3 +--
 10 files changed, 51 insertions(+), 75 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 48bb9e08f2e8..c711b4585a61 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -460,10 +460,9 @@ u64 add_new_free_space(struct btrfs_block_group *block_group, u64 start, u64 end
 	int ret;

 	while (start < end) {
-		ret = find_first_extent_bit(info->pinned_extents, start,
+		ret = find_first_extent_bit(&info->excluded_extents, start,
 					    &extent_start, &extent_end,
-					    EXTENT_DIRTY | EXTENT_UPTODATE,
-					    NULL);
+					    EXTENT_UPTODATE, NULL);
 		if (ret)
 			break;

@@ -1233,32 +1232,46 @@ static int inc_block_group_ro(struct btrfs_block_group *cache, int force)
 	return ret;
 }

-static bool clean_pinned_extents(struct btrfs_block_group *bg)
+static bool clean_pinned_extents(struct btrfs_trans_handle *trans,
+				 struct btrfs_block_group *bg)
 {
 	struct btrfs_fs_info *fs_info = bg->fs_info;
+	struct btrfs_transaction *prev_trans = NULL;
 	u64 start = bg->start;
 	u64 end = start + bg->length - 1;
 	int ret;

+	spin_lock(&fs_info->trans_lock);
+	if (trans->transaction->list.prev != &fs_info->trans_list) {
+		prev_trans = list_entry(trans->transaction->list.prev,
+					struct btrfs_transaction, list);
+		refcount_inc(&prev_trans->use_count);
+	}
+	spin_unlock(&fs_info->trans_lock);
+
 	/*
 	 * Hold the unused_bg_unpin_mutex lock to avoid racing with
 	 * btrfs_finish_extent_commit(). If we are at transaction N,
 	 * another task might be running finish_extent_commit() for the
 	 * previous transaction N - 1, and have seen a range belonging
-	 * to the block group in freed_extents[] before we were able to
-	 * clear the whole block group range from freed_extents[]. This
+	 * to the block group in pinned_extents before we were able to
+	 * clear the whole block group range from pinned_extents. This
 	 * means that task can lookup for the block group after we
-	 * unpinned it from freed_extents[] and removed it, leading to
+	 * unpinned it from pinned_extents[] and removed it, leading to
 	 * a BUG_ON() at unpin_extent_range().
 	 */
 	mutex_lock(&fs_info->unused_bg_unpin_mutex);
-	ret = clear_extent_bits(&fs_info->freed_extents[0], start, end,
-			  EXTENT_DIRTY);
-	if (ret)
-		goto failure;
+	if (prev_trans) {
+		ret = clear_extent_bits(&prev_trans->pinned_extents, start, end,
+					EXTENT_DIRTY);
+		if (ret)
+			goto failure;
+	}
+
+	btrfs_put_transaction(prev_trans);

-	ret = clear_extent_bits(&fs_info->freed_extents[1], start, end,
-			  EXTENT_DIRTY);
+	ret = clear_extent_bits(&trans->transaction->pinned_extents, start, end,
+				EXTENT_DIRTY);
 	if (ret)
 		goto failure;
 	mutex_unlock(&fs_info->unused_bg_unpin_mutex);
@@ -1266,6 +1279,7 @@ static bool clean_pinned_extents(struct btrfs_block_group *bg)
 	return true;

 failure:
+	btrfs_put_transaction(prev_trans);
 	mutex_unlock(&fs_info->unused_bg_unpin_mutex);
 	btrfs_dec_block_group_ro(bg);
 	return false;
@@ -1367,7 +1381,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 		 * We could have pending pinned extents for this block group,
 		 * just delete them, we don't care about them anymore.
 		 */
-		if (!clean_pinned_extents(block_group))
+		if (!clean_pinned_extents(trans, block_group))
 			goto end_trans;

 		/*
@@ -2877,7 +2891,7 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans,
 					&cache->space_info->total_bytes_pinned,
 					num_bytes,
 					BTRFS_TOTAL_BYTES_PINNED_BATCH);
-			set_extent_dirty(info->pinned_extents,
+			set_extent_dirty(&trans->transaction->pinned_extents,
 					 bytenr, bytenr + num_bytes - 1,
 					 GFP_NOFS | __GFP_NOFAIL);
 		}
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index c6ce8c047814..a84072fbb4e7 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -596,8 +596,8 @@ struct btrfs_fs_info {
 	/* keep track of unallocated space */
 	atomic64_t free_chunk_space;

-	struct extent_io_tree freed_extents[2];
-	struct extent_io_tree *pinned_extents;
+	/* Tracks ranges which used by log trees blocks/logged data extents */
+	struct extent_io_tree excluded_extents;

 	/* logical->physical extent mapping */
 	struct extent_map_tree mapping_tree;
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index d4567f00799d..07dbb1924026 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2021,10 +2021,8 @@ void btrfs_free_fs_roots(struct btrfs_fs_info *fs_info)
 			btrfs_drop_and_free_fs_root(fs_info, gang[i]);
 	}

-	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
+	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
 		btrfs_free_log_root_tree(NULL, fs_info);
-		btrfs_destroy_pinned_extent(fs_info, fs_info->pinned_extents);
-	}
 }

 static void btrfs_init_scrub(struct btrfs_fs_info *fs_info)
@@ -2779,11 +2777,8 @@ int __cold open_ctree(struct super_block *sb,
 	fs_info->block_group_cache_tree = RB_ROOT;
 	fs_info->first_logical_byte = (u64)-1;

-	extent_io_tree_init(fs_info, &fs_info->freed_extents[0],
-			    IO_TREE_FS_INFO_FREED_EXTENTS0, NULL);
-	extent_io_tree_init(fs_info, &fs_info->freed_extents[1],
-			    IO_TREE_FS_INFO_FREED_EXTENTS1, NULL);
-	fs_info->pinned_extents = &fs_info->freed_extents[0];
+	extent_io_tree_init(fs_info, &fs_info->excluded_extents,
+			    IO_TREE_FS_INFO_EXCLUDED_EXTENTS, NULL);
 	set_bit(BTRFS_FS_BARRIER, &fs_info->flags);

 	mutex_init(&fs_info->ordered_operations_mutex);
@@ -4391,16 +4386,12 @@ static int btrfs_destroy_marked_extents(struct btrfs_fs_info *fs_info,
 }

 static int btrfs_destroy_pinned_extent(struct btrfs_fs_info *fs_info,
-				       struct extent_io_tree *pinned_extents)
+				       struct extent_io_tree *unpin)
 {
-	struct extent_io_tree *unpin;
 	u64 start;
 	u64 end;
 	int ret;
-	bool loop = true;

-	unpin = pinned_extents;
-again:
 	while (1) {
 		struct extent_state *cached_state = NULL;

@@ -4425,15 +4416,6 @@ static int btrfs_destroy_pinned_extent(struct btrfs_fs_info *fs_info,
 		cond_resched();
 	}

-	if (loop) {
-		if (unpin == &fs_info->freed_extents[0])
-			unpin = &fs_info->freed_extents[1];
-		else
-			unpin = &fs_info->freed_extents[0];
-		loop = false;
-		goto again;
-	}
-
 	return 0;
 }

@@ -4525,8 +4507,7 @@ void btrfs_cleanup_one_transaction(struct btrfs_transaction *cur_trans,

 	btrfs_destroy_marked_extents(fs_info, &cur_trans->dirty_pages,
 				     EXTENT_DIRTY);
-	btrfs_destroy_pinned_extent(fs_info,
-				    fs_info->pinned_extents);
+	btrfs_destroy_pinned_extent(fs_info, &cur_trans->pinned_extents);

 	cur_trans->state =TRANS_STATE_COMPLETED;
 	wake_up(&cur_trans->commit_wait);
@@ -4578,7 +4559,6 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
 	btrfs_destroy_all_ordered_extents(fs_info);
 	btrfs_destroy_delayed_inodes(fs_info);
 	btrfs_assert_delayed_root_empty(fs_info);
-	btrfs_destroy_pinned_extent(fs_info, fs_info->pinned_extents);
 	btrfs_destroy_all_delalloc_inodes(fs_info);
 	mutex_unlock(&fs_info->transaction_kthread_mutex);

diff --git a/fs/btrfs/extent-io-tree.h b/fs/btrfs/extent-io-tree.h
index a3febe746c79..02c2e4e711bf 100644
--- a/fs/btrfs/extent-io-tree.h
+++ b/fs/btrfs/extent-io-tree.h
@@ -36,8 +36,7 @@ struct io_failure_record;
 #define CHUNK_TRIMMED				EXTENT_DEFRAG

 enum {
-	IO_TREE_FS_INFO_FREED_EXTENTS0,
-	IO_TREE_FS_INFO_FREED_EXTENTS1,
+	IO_TREE_FS_INFO_EXCLUDED_EXTENTS,
 	IO_TREE_INODE_IO,
 	IO_TREE_INODE_IO_FAILURE,
 	IO_TREE_RELOC_BLOCKS,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index d680f2ac336b..f7b89ab921e1 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -64,10 +64,8 @@ int btrfs_add_excluded_extent(struct btrfs_fs_info *fs_info,
 			      u64 start, u64 num_bytes)
 {
 	u64 end = start + num_bytes - 1;
-	set_extent_bits(&fs_info->freed_extents[0],
-			start, end, EXTENT_UPTODATE);
-	set_extent_bits(&fs_info->freed_extents[1],
-			start, end, EXTENT_UPTODATE);
+	set_extent_bits(&fs_info->excluded_extents, start, end,
+			EXTENT_UPTODATE);
 	return 0;
 }

@@ -79,10 +77,8 @@ void btrfs_free_excluded_extents(struct btrfs_block_group *cache)
 	start = cache->start;
 	end = start + cache->length - 1;

-	clear_extent_bits(&fs_info->freed_extents[0],
-			  start, end, EXTENT_UPTODATE);
-	clear_extent_bits(&fs_info->freed_extents[1],
-			  start, end, EXTENT_UPTODATE);
+	clear_extent_bits(&fs_info->excluded_extents, start, end,
+			  EXTENT_UPTODATE);
 }

 static u64 generic_ref_to_space_flags(struct btrfs_ref *ref)
@@ -2606,7 +2602,7 @@ static int pin_down_extent(struct btrfs_trans_handle *trans,

 	percpu_counter_add_batch(&cache->space_info->total_bytes_pinned,
 		    num_bytes, BTRFS_TOTAL_BYTES_PINNED_BATCH);
-	set_extent_dirty(fs_info->pinned_extents, bytenr,
+	set_extent_dirty(&trans->transaction->pinned_extents, bytenr,
 			 bytenr + num_bytes - 1, GFP_NOFS | __GFP_NOFAIL);
 	return 0;
 }
@@ -2762,11 +2758,6 @@ void btrfs_prepare_extent_commit(struct btrfs_fs_info *fs_info)
 		}
 	}

-	if (fs_info->pinned_extents == &fs_info->freed_extents[0])
-		fs_info->pinned_extents = &fs_info->freed_extents[1];
-	else
-		fs_info->pinned_extents = &fs_info->freed_extents[0];
-
 	up_write(&fs_info->commit_root_sem);

 	btrfs_update_global_block_rsv(fs_info);
@@ -2907,10 +2898,7 @@ int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans)
 	u64 end;
 	int ret;

-	if (fs_info->pinned_extents == &fs_info->freed_extents[0])
-		unpin = &fs_info->freed_extents[1];
-	else
-		unpin = &fs_info->freed_extents[0];
+	unpin = &trans->transaction->pinned_extents;

 	while (!trans->aborted) {
 		struct extent_state *cached_state = NULL;
@@ -2922,12 +2910,9 @@ int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans)
 			mutex_unlock(&fs_info->unused_bg_unpin_mutex);
 			break;
 		}
-		if (test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags)) {
-			clear_extent_bits(&fs_info->freed_extents[0], start,
+		if (test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags))
+			clear_extent_bits(&fs_info->excluded_extents, start,
 					  end, EXTENT_UPTODATE);
-			clear_extent_bits(&fs_info->freed_extents[1], start,
-					  end, EXTENT_UPTODATE);
-		}

 		if (btrfs_test_opt(fs_info, DISCARD_SYNC))
 			ret = btrfs_discard_extent(fs_info, start,
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 9d6372139547..bd9c4b5da549 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -1086,7 +1086,7 @@ static noinline_for_stack int write_pinned_extent_entries(
 	 * We shouldn't have switched the pinned extents yet so this is the
 	 * right one
 	 */
-	unpin = block_group->fs_info->pinned_extents;
+	unpin = &trans->transaction->pinned_extents;

 	start = block_group->start;

diff --git a/fs/btrfs/tests/btrfs-tests.c b/fs/btrfs/tests/btrfs-tests.c
index c12b91ff5f56..2f7514068c3f 100644
--- a/fs/btrfs/tests/btrfs-tests.c
+++ b/fs/btrfs/tests/btrfs-tests.c
@@ -156,12 +156,9 @@ struct btrfs_fs_info *btrfs_alloc_dummy_fs_info(u32 nodesize, u32 sectorsize)
 	INIT_LIST_HEAD(&fs_info->fs_devices->devices);
 	INIT_RADIX_TREE(&fs_info->buffer_radix, GFP_ATOMIC);
 	INIT_RADIX_TREE(&fs_info->fs_roots_radix, GFP_ATOMIC);
-	extent_io_tree_init(fs_info, &fs_info->freed_extents[0],
-			    IO_TREE_FS_INFO_FREED_EXTENTS0, NULL);
-	extent_io_tree_init(fs_info, &fs_info->freed_extents[1],
-			    IO_TREE_FS_INFO_FREED_EXTENTS1, NULL);
+	extent_io_tree_init(fs_info, &fs_info->excluded_extents,
+			    IO_TREE_FS_INFO_EXCLUDED_EXTENTS, NULL);
 	extent_map_tree_init(&fs_info->mapping_tree);
-	fs_info->pinned_extents = &fs_info->freed_extents[0];
 	set_bit(BTRFS_FS_STATE_DUMMY_FS_INFO, &fs_info->fs_state);

 	test_mnt->mnt_sb->s_fs_info = fs_info;
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 33dcc88b428a..8ed3d9ecbc4f 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -334,6 +334,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
 	list_add_tail(&cur_trans->list, &fs_info->trans_list);
 	extent_io_tree_init(fs_info, &cur_trans->dirty_pages,
 			IO_TREE_TRANS_DIRTY_PAGES, fs_info->btree_inode);
+	extent_io_tree_init(fs_info, &cur_trans->pinned_extents, 0, NULL);
 	fs_info->generation++;
 	cur_trans->transid = fs_info->generation;
 	fs_info->running_transaction = cur_trans;
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index 49f7196368f5..2a046a0155c1 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -71,6 +71,7 @@ struct btrfs_transaction {
 	 */
 	struct list_head io_bgs;
 	struct list_head dropped_roots;
+	struct extent_io_tree pinned_extents;

 	/*
 	 * we need to make sure block group deletion doesn't race with
diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
index 17088a112ed0..f4fa3e15297d 100644
--- a/include/trace/events/btrfs.h
+++ b/include/trace/events/btrfs.h
@@ -81,8 +81,7 @@ TRACE_DEFINE_ENUM(COMMIT_TRANS);

 #define show_extent_io_tree_owner(owner)				       \
 	__print_symbolic(owner,						       \
-		{ IO_TREE_FS_INFO_FREED_EXTENTS0, "FREED_EXTENTS0" },	       \
-		{ IO_TREE_FS_INFO_FREED_EXTENTS1, "FREED_EXTENTS1" },	       \
+		{ IO_TREE_FS_INFO_EXCLUDED_EXTENTS, "EXCLUDED_EXTENTS" }, \
 		{ IO_TREE_INODE_IO,		  "INODE_IO" },		       \
 		{ IO_TREE_INODE_IO_FAILURE,	  "INODE_IO_FAILURE" },	       \
 		{ IO_TREE_RELOC_BLOCKS,		  "RELOC_BLOCKS" },	       \
--
2.17.1

Re: [PATCH v3] btrfs: Use btrfs_transaction::pinned_extents

[Josef Bacik]

On 1/24/20 10:18 AM, Nikolay Borisov wrote:
> This commit flips the switch to start tracking/processing pinned
> extents on a per-transaction basis. It mostly replaces all references
> from btrfs_fs_info::(pinned_extents|freed_extents[]) to
> btrfs_transaction::pinned_extents. Two notable modifications that
> warrant explicit mention are changing clean_pinned_extents to get a
> reference to the previously running transaction. The other one is
> removal of call to btrfs_destroy_pinned_extent since transactions are
> going to be cleaned in btrfs_cleanup_one_transaction.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 01/11] btrfs: Perform pinned cleanup directly in btrfs_destroy_delayed_refs

[David Sterba]

On Mon, Jan 20, 2020 at 04:09:08PM +0200, Nikolay Borisov wrote:
> Having btrfs_destroy_delayed_refs call btrfs_pin_extent is problematic
> for making pinned extents tracking per-transaction since
> btrfs_trans_handle cannot be passed to btrfs_pin_extent in this context.
> Additionally delayed refs heads pinned in btrfs_destroy_delayed_refs
> are going to be handled very closely, in btrfs_destroy_pinned_extent.
> 
> To enable btrfs_pin_extent to take btrfs_trans_handle simply open code
> it in btrfs_destroy_delayed_refs and call btrfs_error_unpin_extent_range
> on the range. This enables us to do less work in
> btrfs_destroy_pinned_extent and leaves btrfs_pin_extent being called in
> contexts which have a valid btrfs_trans_handle.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/disk-io.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 5ce2801f8388..9209c7b0997c 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -42,6 +42,7 @@
>  #include "ref-verify.h"
>  #include "block-group.h"
>  #include "discard.h"
> +#include "space-info.h"
>  
>  #define BTRFS_SUPER_FLAG_SUPP	(BTRFS_HEADER_FLAG_WRITTEN |\
>  				 BTRFS_HEADER_FLAG_RELOC |\
> @@ -4261,9 +4262,28 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
>  		spin_unlock(&delayed_refs->lock);
>  		mutex_unlock(&head->mutex);
>  
> -		if (pin_bytes)
> -			btrfs_pin_extent(fs_info, head->bytenr,
> -					 head->num_bytes, 1);
> +		if (pin_bytes) {
> +			struct btrfs_block_group *cache;
> +			cache = btrfs_lookup_block_group(fs_info, head->bytenr);
> +			BUG_ON(!cache);

So this BUG_ON is propagated from btrfs_pin_extent but not turned into
proper error handling in any of the followup patches.

> +
> +			spin_lock(&cache->space_info->lock);
> +			spin_lock(&cache->lock);
> +			cache->pinned += head->num_bytes;
> +			btrfs_space_info_update_bytes_pinned(fs_info,
> +					cache->space_info, head->num_bytes);
> +			cache->reserved -= head->num_bytes;
> +			cache->space_info->bytes_reserved -= head->num_bytes;
> +			spin_unlock(&cache->lock);
> +			spin_unlock(&cache->space_info->lock);
> +			percpu_counter_add_batch(&cache->space_info->total_bytes_pinned,
> +				    head->num_bytes, BTRFS_TOTAL_BYTES_PINNED_BATCH);
> +
> +			btrfs_put_block_group(cache);
> +
> +			btrfs_error_unpin_extent_range(fs_info, head->bytenr,
> +						       head->bytenr + head->num_bytes - 1);

This should also handle errors

> +		}
>  		btrfs_cleanup_ref_head_accounting(fs_info, delayed_refs, head);
>  		btrfs_put_delayed_ref_head(head);
>  		cond_resched();
> -- 
> 2.17.1

Re: [PATCH 09/11] btrfs: Mark pinned log extents as excluded

[David Sterba]

On Mon, Jan 20, 2020 at 04:09:16PM +0200, Nikolay Borisov wrote:
> In preparation to making pinned extents per-transaction ensure that
> log such extents are always excluded from caching. To achieve this in
> addition to marking them via btrfs_pin_extent_for_log_replay they also
> need to be marked with btrfs_add_excluded_extent to prevent log tree
> extent buffer being loaded by the free space caching thread. That's
> required since log treeblocks are not recorded in the extent tree, hence
> they always look free.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/extent-tree.c | 8 ++++++++
>  fs/btrfs/tree-log.c    | 2 +-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 7dcf9217a622..d680f2ac336b 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2634,6 +2634,8 @@ int btrfs_pin_extent_for_log_replay(struct btrfs_trans_handle *trans,
>  	struct btrfs_block_group *cache;
>  	int ret;
>  
> +	btrfs_add_excluded_extent(trans->fs_info, bytenr, num_bytes);

Again lack of error handling, I thought that untangling and cleaning up
the extent pinning was motivated to actually handle the errors.

> +
>  	cache = btrfs_lookup_block_group(trans->fs_info, bytenr);
>  	if (!cache)
>  		return -EINVAL;
> @@ -2920,6 +2922,12 @@ int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans)
>  			mutex_unlock(&fs_info->unused_bg_unpin_mutex);
>  			break;
>  		}
> +		if (test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags)) {
> +			clear_extent_bits(&fs_info->freed_extents[0], start,
> +					  end, EXTENT_UPTODATE);
> +			clear_extent_bits(&fs_info->freed_extents[1], start,
> +					  end, EXTENT_UPTODATE);
> +		}
>  
>  		if (btrfs_test_opt(fs_info, DISCARD_SYNC))
>  			ret = btrfs_discard_extent(fs_info, start,
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index b535d409a728..f89de24838d5 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -2994,7 +2994,7 @@ static inline void btrfs_remove_log_ctx(struct btrfs_root *root,
>  	mutex_unlock(&root->log_mutex);
>  }
>  
> -/* 
> +/*
>   * Invoked in log mutex context, or be sure there is no other task which
>   * can access the list.
>   */
> -- 
> 2.17.1

Re: [PATCH v3] btrfs: Use btrfs_transaction::pinned_extents

[David Sterba]

On Fri, Jan 24, 2020 at 05:18:30PM +0200, Nikolay Borisov wrote:
> This commit flips the switch to start tracking/processing pinned
> extents on a per-transaction basis. It mostly replaces all references
> from btrfs_fs_info::(pinned_extents|freed_extents[]) to
> btrfs_transaction::pinned_extents. Two notable modifications that
> warrant explicit mention are changing clean_pinned_extents to get a
> reference to the previously running transaction. The other one is
> removal of call to btrfs_destroy_pinned_extent since transactions are
> going to be cleaned in btrfs_cleanup_one_transaction.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

The subject could tell me what's the summmary of the change, I can see
in the code that btrfs_transaction::pinned_extents is used but for
what? If this is the main change of the patchset after cleanups, then
the subject is close to the cover letter, so something like "switch to
per-transaction pinned extents".

Re: [PATCH 09/11] btrfs: Mark pinned log extents as excluded

[Nikolay Borisov]

On 30.01.20 г. 15:53 ч., David Sterba wrote:
> On Mon, Jan 20, 2020 at 04:09:16PM +0200, Nikolay Borisov wrote:
>> In preparation to making pinned extents per-transaction ensure that
>> log such extents are always excluded from caching. To achieve this in
>> addition to marking them via btrfs_pin_extent_for_log_replay they also
>> need to be marked with btrfs_add_excluded_extent to prevent log tree
>> extent buffer being loaded by the free space caching thread. That's
>> required since log treeblocks are not recorded in the extent tree, hence
>> they always look free.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  fs/btrfs/extent-tree.c | 8 ++++++++
>>  fs/btrfs/tree-log.c    | 2 +-
>>  2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 7dcf9217a622..d680f2ac336b 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -2634,6 +2634,8 @@ int btrfs_pin_extent_for_log_replay(struct btrfs_trans_handle *trans,
>>  	struct btrfs_block_group *cache;
>>  	int ret;
>>  
>> +	btrfs_add_excluded_extent(trans->fs_info, bytenr, num_bytes);
> 
> Again lack of error handling, I thought that untangling and cleaning up
> the extent pinning was motivated to actually handle the errors.

It is but it will be done in a follow up series. For now I want to have
the pinned extents be per-transaction and not be more broken (e.g. no
error handling) than we currently are.

> 
>> +
>>  	cache = btrfs_lookup_block_group(trans->fs_info, bytenr);
>>  	if (!cache)
>>  		return -EINVAL;
>> @@ -2920,6 +2922,12 @@ int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans)
>>  			mutex_unlock(&fs_info->unused_bg_unpin_mutex);
>>  			break;
>>  		}
>> +		if (test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags)) {
>> +			clear_extent_bits(&fs_info->freed_extents[0], start,
>> +					  end, EXTENT_UPTODATE);
>> +			clear_extent_bits(&fs_info->freed_extents[1], start,
>> +					  end, EXTENT_UPTODATE);
>> +		}
>>  
>>  		if (btrfs_test_opt(fs_info, DISCARD_SYNC))
>>  			ret = btrfs_discard_extent(fs_info, start,
>> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
>> index b535d409a728..f89de24838d5 100644
>> --- a/fs/btrfs/tree-log.c
>> +++ b/fs/btrfs/tree-log.c
>> @@ -2994,7 +2994,7 @@ static inline void btrfs_remove_log_ctx(struct btrfs_root *root,
>>  	mutex_unlock(&root->log_mutex);
>>  }
>>  
>> -/* 
>> +/*
>>   * Invoked in log mutex context, or be sure there is no other task which
>>   * can access the list.
>>   */
>> -- 
>> 2.17.1

Re: [PATCH 01/11] btrfs: Perform pinned cleanup directly in btrfs_destroy_delayed_refs

[Nikolay Borisov]

On 30.01.20 г. 15:51 ч., David Sterba wrote:
> On Mon, Jan 20, 2020 at 04:09:08PM +0200, Nikolay Borisov wrote:
>> Having btrfs_destroy_delayed_refs call btrfs_pin_extent is problematic
>> for making pinned extents tracking per-transaction since
>> btrfs_trans_handle cannot be passed to btrfs_pin_extent in this context.
>> Additionally delayed refs heads pinned in btrfs_destroy_delayed_refs
>> are going to be handled very closely, in btrfs_destroy_pinned_extent.
>>
>> To enable btrfs_pin_extent to take btrfs_trans_handle simply open code
>> it in btrfs_destroy_delayed_refs and call btrfs_error_unpin_extent_range
>> on the range. This enables us to do less work in
>> btrfs_destroy_pinned_extent and leaves btrfs_pin_extent being called in
>> contexts which have a valid btrfs_trans_handle.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  fs/btrfs/disk-io.c | 26 +++++++++++++++++++++++---
>>  1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 5ce2801f8388..9209c7b0997c 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -42,6 +42,7 @@
>>  #include "ref-verify.h"
>>  #include "block-group.h"
>>  #include "discard.h"
>> +#include "space-info.h"
>>  
>>  #define BTRFS_SUPER_FLAG_SUPP	(BTRFS_HEADER_FLAG_WRITTEN |\
>>  				 BTRFS_HEADER_FLAG_RELOC |\
>> @@ -4261,9 +4262,28 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
>>  		spin_unlock(&delayed_refs->lock);
>>  		mutex_unlock(&head->mutex);
>>  
>> -		if (pin_bytes)
>> -			btrfs_pin_extent(fs_info, head->bytenr,
>> -					 head->num_bytes, 1);
>> +		if (pin_bytes) {
>> +			struct btrfs_block_group *cache;
>> +			cache = btrfs_lookup_block_group(fs_info, head->bytenr);
>> +			BUG_ON(!cache);
> 
> So this BUG_ON is propagated from btrfs_pin_extent but not turned into
> proper error handling in any of the followup patches.

This BUGON should be there or it could be turned into an ASSERT. It's
used to catch racing block group freeing i.e if it triggers it's a
logical error, a real bug.

> 

<snip>

>> +			btrfs_error_unpin_extent_range(fs_info, head->bytenr,
>> +						       head->bytenr + head->num_bytes - 1);
> 
> This should also handle errors

Turns out unpin_extent_range cane just return void, since it cannot fail
so there's nothing to handle.

> 
>> +		}
>>  		btrfs_cleanup_ref_head_accounting(fs_info, delayed_refs, head);
>>  		btrfs_put_delayed_ref_head(head);
>>  		cond_resched();
>> -- 
>> 2.17.1

Re: [PATCH 09/11] btrfs: Mark pinned log extents as excluded

[Nikolay Borisov]

On 30.01.20 г. 15:53 ч., David Sterba wrote:
> On Mon, Jan 20, 2020 at 04:09:16PM +0200, Nikolay Borisov wrote:
>> In preparation to making pinned extents per-transaction ensure that
>> log such extents are always excluded from caching. To achieve this in
>> addition to marking them via btrfs_pin_extent_for_log_replay they also
>> need to be marked with btrfs_add_excluded_extent to prevent log tree
>> extent buffer being loaded by the free space caching thread. That's
>> required since log treeblocks are not recorded in the extent tree, hence
>> they always look free.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  fs/btrfs/extent-tree.c | 8 ++++++++
>>  fs/btrfs/tree-log.c    | 2 +-
>>  2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 7dcf9217a622..d680f2ac336b 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -2634,6 +2634,8 @@ int btrfs_pin_extent_for_log_replay(struct btrfs_trans_handle *trans,
>>  	struct btrfs_block_group *cache;
>>  	int ret;
>>  
>> +	btrfs_add_excluded_extent(trans->fs_info, bytenr, num_bytes);
> 
> Again lack of error handling, I thought that untangling and cleaning up
> the extent pinning was motivated to actually handle the errors.

Currently btrfs_add_excluded_extent always returns 0. The possible err
values from set_extent_bit are not propagated. I think this is a cleanup
for another series.

> 

<snip>

Re: [PATCH 11/11 v3] btrfs: Use btrfs_transaction::pinned_extents

[David Sterba]

On Fri, Jan 24, 2020 at 05:18:30PM +0200, Nikolay Borisov wrote:
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -334,6 +334,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
>  	list_add_tail(&cur_trans->list, &fs_info->trans_list);
>  	extent_io_tree_init(fs_info, &cur_trans->dirty_pages,
>  			IO_TREE_TRANS_DIRTY_PAGES, fs_info->btree_inode);
> +	extent_io_tree_init(fs_info, &cur_trans->pinned_extents, 0, NULL);

What's the reason there's no symbolic name for pinned_extents? Also 0
matches IO_TREE_FS_INFO_EXCLUDED_EXTENTS because it's first in the enum
list.

Re: [PATCH 11/11 v3] btrfs: Use btrfs_transaction::pinned_extents

[Nikolay Borisov]

On 6.02.20 г. 20:10 ч., David Sterba wrote:
> On Fri, Jan 24, 2020 at 05:18:30PM +0200, Nikolay Borisov wrote:
>> --- a/fs/btrfs/transaction.c
>> +++ b/fs/btrfs/transaction.c
>> @@ -334,6 +334,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
>>  	list_add_tail(&cur_trans->list, &fs_info->trans_list);
>>  	extent_io_tree_init(fs_info, &cur_trans->dirty_pages,
>>  			IO_TREE_TRANS_DIRTY_PAGES, fs_info->btree_inode);
>> +	extent_io_tree_init(fs_info, &cur_trans->pinned_extents, 0, NULL);
> 
> What's the reason there's no symbolic name for pinned_extents? Also 0
> matches IO_TREE_FS_INFO_EXCLUDED_EXTENTS because it's first in the enum
> list.
> 

No reason, I'll change it in next version :)

Re: [PATCH 00/11] Make pinned extents tracking per-transaction

[David Sterba]

On Mon, Jan 20, 2020 at 04:09:07PM +0200, Nikolay Borisov wrote:
> I've finally managed to finish and test the pinned extents rework. So here it is.
> 
> The idea is to move pinnex extents tracking from the global fs_info->pinned_extents,
> which is just a pointer to fs_info->freed_extents[] array members to a per
> transaction structure. This will allow to cleanup pinned extents information
> during transaction abort.
> 
> The bulk of the changes are necessary to ensure a valid transaction handle
> is passed to every function that utilizes fs_info->pinned_extents. Also it
> was necessary to account for the peculiarities of excluded extents which are
> also tracked in ->freed_extents array but with a different flag (yuck).
> 
> First 9 patches propagate btrfs_trans_handle where it's necessary. Patch 10
> factors out pinned extent clean up to make Patch 11 more readable , alternatively
> the changes in patch 10 had to be in patch 11 which would have made it messier
> for review.
> 
> I believe this series doesn't bring any user facing changes it (hopefully)
> streamlines the code and makes it apparent what the lifetime of pinned extents
> is and paves the way for further cleanups of BUG_ON.
> 
> Nikolay Borisov (11):
>   btrfs: Perform pinned cleanup directly in btrfs_destroy_delayed_refs
>   btrfs: Make btrfs_pin_extent take trans handle
>   btrfs: Introduce unaccount_log_buffer
>   btrfs: Call btrfs_pin_reserved_extent only during active transaction
>   btrfs: Make btrfs_pin_reserved_extent take transaction
>   btrfs: Make btrfs_pin_extent_for_log_replay take transaction handle
>   btrfs: Make pin_down_extent take btrfs_trans_handle
>   btrfs: Pass trans handle to write_pinned_extent_entries
>   btrfs: Mark pinned log extents as excluded
>   btrfs: Factor out pinned extent clean up in btrfs_delete_unused_bgs
>   btrfs: Use btrfs_transaction::pinned_extents

So far I found test btrfs/139 to fail, nothing in the log, only the
golden output does not match.

Re: [PATCH 11/11 v3] btrfs: Use btrfs_transaction::pinned_extents

[David Sterba]

On Thu, Feb 06, 2020 at 09:40:35PM +0200, Nikolay Borisov wrote:
> 
> 
> On 6.02.20 г. 20:10 ч., David Sterba wrote:
> > On Fri, Jan 24, 2020 at 05:18:30PM +0200, Nikolay Borisov wrote:
> >> --- a/fs/btrfs/transaction.c
> >> +++ b/fs/btrfs/transaction.c
> >> @@ -334,6 +334,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
> >>  	list_add_tail(&cur_trans->list, &fs_info->trans_list);
> >>  	extent_io_tree_init(fs_info, &cur_trans->dirty_pages,
> >>  			IO_TREE_TRANS_DIRTY_PAGES, fs_info->btree_inode);
> >> +	extent_io_tree_init(fs_info, &cur_trans->pinned_extents, 0, NULL);
> > 
> > What's the reason there's no symbolic name for pinned_extents? Also 0
> > matches IO_TREE_FS_INFO_EXCLUDED_EXTENTS because it's first in the enum
> > list.
> > 
> 
> No reason, I'll change it in next version :)

Patchset going to misc-next with the following fixup:

--- a/fs/btrfs/extent-io-tree.h
+++ b/fs/btrfs/extent-io-tree.h
@@ -36,6 +36,7 @@ struct io_failure_record;
 #define CHUNK_TRIMMED                          EXTENT_DEFRAG
 
 enum {
+       IO_TREE_FS_PINNED_EXTENTS,
        IO_TREE_FS_EXCLUDED_EXTENTS,
        IO_TREE_INODE_IO,
        IO_TREE_INODE_IO_FAILURE,
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index e39cc15646a4..559a7a38d5a8 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -334,7 +334,8 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
        list_add_tail(&cur_trans->list, &fs_info->trans_list);
        extent_io_tree_init(fs_info, &cur_trans->dirty_pages,
                        IO_TREE_TRANS_DIRTY_PAGES, fs_info->btree_inode);
-       extent_io_tree_init(fs_info, &cur_trans->pinned_extents, 0, NULL);
+       extent_io_tree_init(fs_info, &cur_trans->pinned_extents,
+                       IO_TREE_FS_PINNED_EXTENTS, NULL);
        fs_info->generation++;
        cur_trans->transid = fs_info->generation;
        fs_info->running_transaction = cur_trans;
diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
index 0f11f1fb982d..bcbc763b8814 100644
--- a/include/trace/events/btrfs.h
+++ b/include/trace/events/btrfs.h
@@ -81,6 +81,7 @@ TRACE_DEFINE_ENUM(COMMIT_TRANS);
 
 #define show_extent_io_tree_owner(owner)                                      \
        __print_symbolic(owner,                                                \
+               { IO_TREE_FS_PINNED_EXTENTS,      "PINNED_EXTENTS" },          \
                { IO_TREE_FS_EXCLUDED_EXTENTS,    "EXCLUDED_EXTENTS" },        \
                { IO_TREE_INODE_IO,               "INODE_IO" },                \
                { IO_TREE_INODE_IO_FAILURE,       "INODE_IO_FAILURE" },        \