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@xxxxxxxx>
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,
