* [PATCH 01/13] btrfs: fix missed extent on fsync after dropping extent maps
2022-09-19 14:06 [PATCH 00/13] btrfs: fixes and cleanups around extent maps fdmanana
@ 2022-09-19 14:06 ` fdmanana
2022-09-20 10:19 ` Anand Jain
2022-09-21 11:11 ` Anand Jain
2022-09-19 14:06 ` [PATCH 02/13] btrfs: move btrfs_drop_extent_cache() to extent_map.c fdmanana
` (12 subsequent siblings)
13 siblings, 2 replies; 20+ messages in thread
From: fdmanana @ 2022-09-19 14:06 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
When dropping extent maps for a range, through btrfs_drop_extent_cache(),
if we find an extent map that starts before our target range and/or ends
before the target range, and we are not able to allocate extent maps for
splitting that extent map, then we don't fail and simply remove the entire
extent map from the inode's extent map tree.
This is generally fine, because in case anyone needs to access the extent
map, it can just load it again later from the respective file extent
item(s) in the subvolume btree. However, if that extent map is new and is
in the list of modified extents, then a fast fsync will miss the parts of
the extent that were outside our range (that needed to be split),
therefore not logging them. Fix that by marking the inode for a full
fsync. This issue was introduced after removing BUG_ON()s triggered when
the split extent map allocations failed, done by commit 7014cdb49305ed
("Btrfs: btrfs_drop_extent_cache should never fail"), back in 2012, and
the fast fsync path already existed but was very recent.
Also, in the case where we could allocate extent maps for the split
operations but then fail to add a split extent map to the tree, mark the
inode for a full fsync as well. This is not supposed to ever fail, and we
assert that, but in case assertions are disabled (CONFIG_BTRFS_ASSERT is
not set), it's the correct thing to do to make sure a fast fsync will not
miss a new extent.
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/file.c | 58 +++++++++++++++++++++++++++++++++++++++----------
1 file changed, 46 insertions(+), 12 deletions(-)
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index fea508a35900..0e52292cf8bc 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -523,6 +523,7 @@ void btrfs_drop_extent_cache(struct btrfs_inode *inode, u64 start, u64 end,
testend = 0;
}
while (1) {
+ bool ends_after_range = false;
int no_splits = 0;
modified = false;
@@ -539,10 +540,12 @@ void btrfs_drop_extent_cache(struct btrfs_inode *inode, u64 start, u64 end,
write_unlock(&em_tree->lock);
break;
}
+ if (testend && em->start + em->len > start + len)
+ ends_after_range = true;
flags = em->flags;
gen = em->generation;
if (skip_pinned && test_bit(EXTENT_FLAG_PINNED, &em->flags)) {
- if (testend && em->start + em->len >= start + len) {
+ if (ends_after_range) {
free_extent_map(em);
write_unlock(&em_tree->lock);
break;
@@ -592,7 +595,7 @@ void btrfs_drop_extent_cache(struct btrfs_inode *inode, u64 start, u64 end,
split = split2;
split2 = NULL;
}
- if (testend && em->start + em->len > start + len) {
+ if (ends_after_range) {
u64 diff = start + len - em->start;
split->start = start + len;
@@ -630,14 +633,42 @@ void btrfs_drop_extent_cache(struct btrfs_inode *inode, u64 start, u64 end,
} else {
ret = add_extent_mapping(em_tree, split,
modified);
- ASSERT(ret == 0); /* Logic error */
+ /* Logic error, shouldn't happen. */
+ ASSERT(ret == 0);
+ if (WARN_ON(ret != 0) && modified)
+ btrfs_set_inode_full_sync(inode);
}
free_extent_map(split);
split = NULL;
}
next:
- if (extent_map_in_tree(em))
+ if (extent_map_in_tree(em)) {
+ /*
+ * If the extent map is still in the tree it means that
+ * either of the following is true:
+ *
+ * 1) It fits entirely in our range (doesn't end beyond
+ * it or starts before it);
+ *
+ * 2) It starts before our range and/or ends after our
+ * range, and we were not able to allocate the extent
+ * maps for split operations, @split and @split2.
+ *
+ * If we are at case 2) then we just remove the entire
+ * extent map - this is fine since if anyone needs it to
+ * access the subranges outside our range, will just
+ * load it again from the subvolume tree's file extent
+ * item. However if the extent map was in the list of
+ * modified extents, then we must mark the inode for a
+ * full fsync, otherwise a fast fsync will miss this
+ * extent if it's new and needs to be logged.
+ */
+ if ((em->start < start || ends_after_range) && modified) {
+ ASSERT(no_splits);
+ btrfs_set_inode_full_sync(inode);
+ }
remove_extent_mapping(em_tree, em);
+ }
write_unlock(&em_tree->lock);
/* once for us */
@@ -2199,14 +2230,6 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
atomic_inc(&root->log_batch);
- /*
- * Always check for the full sync flag while holding the inode's lock,
- * to avoid races with other tasks. The flag must be either set all the
- * time during logging or always off all the time while logging.
- */
- full_sync = test_bit(BTRFS_INODE_NEEDS_FULL_SYNC,
- &BTRFS_I(inode)->runtime_flags);
-
/*
* Before we acquired the inode's lock and the mmap lock, someone may
* have dirtied more pages in the target range. We need to make sure
@@ -2231,6 +2254,17 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
goto out;
}
+ /*
+ * Always check for the full sync flag while holding the inode's lock,
+ * to avoid races with other tasks. The flag must be either set all the
+ * time during logging or always off all the time while logging.
+ * We check the flag here after starting delalloc above, because when
+ * running delalloc the full sync flag may be set if we need to drop
+ * extra extent map ranges due to temporary memory allocation failures.
+ */
+ full_sync = test_bit(BTRFS_INODE_NEEDS_FULL_SYNC,
+ &BTRFS_I(inode)->runtime_flags);
+
/*
* We have to do this here to avoid the priority inversion of waiting on
* IO of a lower priority task while holding a transaction open.
--
2.35.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 01/13] btrfs: fix missed extent on fsync after dropping extent maps
2022-09-19 14:06 ` [PATCH 01/13] btrfs: fix missed extent on fsync after dropping " fdmanana
@ 2022-09-20 10:19 ` Anand Jain
2022-09-20 10:27 ` Filipe Manana
2022-09-21 11:11 ` Anand Jain
1 sibling, 1 reply; 20+ messages in thread
From: Anand Jain @ 2022-09-20 10:19 UTC (permalink / raw)
To: fdmanana, linux-btrfs
On 19/09/2022 22:06, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> When dropping extent maps for a range, through btrfs_drop_extent_cache(),
> if we find an extent map that starts before our target range and/or ends
> before the target range, and we are not able to allocate extent maps for
> splitting that extent map, then we don't fail and simply remove the entire
> extent map from the inode's extent map tree.
> + if (testend && em->start + em->len > start + len)
%len comes from
u64 len = end - start + 1;
IMO >= was correct here; because including %start + %len is already
after the range as in the original code. No?
> + ends_after_range = true;
> flags = em->flags;
> gen = em->generation;
> if (skip_pinned && test_bit(EXTENT_FLAG_PINNED, &em->flags)) {
> - if (testend && em->start + em->len >= start + len) {
> + if (ends_after_range) {
> free_extent_map(em);
> write_unlock(&em_tree->lock);
> break;
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 01/13] btrfs: fix missed extent on fsync after dropping extent maps
2022-09-20 10:19 ` Anand Jain
@ 2022-09-20 10:27 ` Filipe Manana
2022-09-21 11:11 ` Anand Jain
0 siblings, 1 reply; 20+ messages in thread
From: Filipe Manana @ 2022-09-20 10:27 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs
On Tue, Sep 20, 2022 at 11:19 AM Anand Jain <anand.jain@oracle.com> wrote:
>
> On 19/09/2022 22:06, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > When dropping extent maps for a range, through btrfs_drop_extent_cache(),
> > if we find an extent map that starts before our target range and/or ends
> > before the target range, and we are not able to allocate extent maps for
> > splitting that extent map, then we don't fail and simply remove the entire
> > extent map from the inode's extent map tree.
>
>
> > + if (testend && em->start + em->len > start + len)
>
> %len comes from
>
> u64 len = end - start + 1;
>
> IMO >= was correct here; because including %start + %len is already
> after the range as in the original code. No?
No, > is the correct thing to do. It only matters if the extent map's
range ends after our range.
Try the math with a simple example, with a start of 0 and a length of
4096 (end offset if 4095), and you'll see.
Thanks.
>
> > + ends_after_range = true;
> > flags = em->flags;
> > gen = em->generation;
> > if (skip_pinned && test_bit(EXTENT_FLAG_PINNED, &em->flags)) {
> > - if (testend && em->start + em->len >= start + len) {
> > + if (ends_after_range) {
> > free_extent_map(em);
> > write_unlock(&em_tree->lock);
> > break;
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 01/13] btrfs: fix missed extent on fsync after dropping extent maps
2022-09-20 10:27 ` Filipe Manana
@ 2022-09-21 11:11 ` Anand Jain
0 siblings, 0 replies; 20+ messages in thread
From: Anand Jain @ 2022-09-21 11:11 UTC (permalink / raw)
To: Filipe Manana; +Cc: linux-btrfs
On 20/09/2022 18:27, Filipe Manana wrote:
> On Tue, Sep 20, 2022 at 11:19 AM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> On 19/09/2022 22:06, fdmanana@kernel.org wrote:
>>> From: Filipe Manana <fdmanana@suse.com>
>>>
>>> When dropping extent maps for a range, through btrfs_drop_extent_cache(),
>>> if we find an extent map that starts before our target range and/or ends
>>> before the target range, and we are not able to allocate extent maps for
>>> splitting that extent map, then we don't fail and simply remove the entire
>>> extent map from the inode's extent map tree.
>>
>>
>>> + if (testend && em->start + em->len > start + len)
>>
>> %len comes from
>>
>> u64 len = end - start + 1;
>>
>> IMO >= was correct here; because including %start + %len is already
>> after the range as in the original code. No?
>
> No, > is the correct thing to do. It only matters if the extent map's
> range ends after our range.
Got it.
> Try the math with a simple example, with a start of 0 and a length of
> 4096 (end offset if 4095), and you'll see.
Thanks.
> Thanks.
>
>>
>>> + ends_after_range = true;
>>> flags = em->flags;
>>> gen = em->generation;
>>> if (skip_pinned && test_bit(EXTENT_FLAG_PINNED, &em->flags)) {
>>> - if (testend && em->start + em->len >= start + len) {
>>> + if (ends_after_range) {
>>> free_extent_map(em);
>>> write_unlock(&em_tree->lock);
>>> break;
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 01/13] btrfs: fix missed extent on fsync after dropping extent maps
2022-09-19 14:06 ` [PATCH 01/13] btrfs: fix missed extent on fsync after dropping " fdmanana
2022-09-20 10:19 ` Anand Jain
@ 2022-09-21 11:11 ` Anand Jain
1 sibling, 0 replies; 20+ messages in thread
From: Anand Jain @ 2022-09-21 11:11 UTC (permalink / raw)
To: fdmanana, linux-btrfs
On 19/09/2022 22:06, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> When dropping extent maps for a range, through btrfs_drop_extent_cache(),
> if we find an extent map that starts before our target range and/or ends
> before the target range, and we are not able to allocate extent maps for
> splitting that extent map, then we don't fail and simply remove the entire
> extent map from the inode's extent map tree.
>
> This is generally fine, because in case anyone needs to access the extent
> map, it can just load it again later from the respective file extent
> item(s) in the subvolume btree. However, if that extent map is new and is
> in the list of modified extents, then a fast fsync will miss the parts of
> the extent that were outside our range (that needed to be split),
> therefore not logging them. Fix that by marking the inode for a full
> fsync. This issue was introduced after removing BUG_ON()s triggered when
> the split extent map allocations failed, done by commit 7014cdb49305ed
> ("Btrfs: btrfs_drop_extent_cache should never fail"), back in 2012, and
> the fast fsync path already existed but was very recent.
>
> Also, in the case where we could allocate extent maps for the split
> operations but then fail to add a split extent map to the tree, mark the
> inode for a full fsync as well. This is not supposed to ever fail, and we
> assert that, but in case assertions are disabled (CONFIG_BTRFS_ASSERT is
> not set), it's the correct thing to do to make sure a fast fsync will not
> miss a new extent.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
Reviewed-by: Anand Jain <anand.jain@oracle.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 02/13] btrfs: move btrfs_drop_extent_cache() to extent_map.c
2022-09-19 14:06 [PATCH 00/13] btrfs: fixes and cleanups around extent maps fdmanana
2022-09-19 14:06 ` [PATCH 01/13] btrfs: fix missed extent on fsync after dropping " fdmanana
@ 2022-09-19 14:06 ` fdmanana
2022-09-19 14:06 ` [PATCH 03/13] btrfs: use extent_map_end() at btrfs_drop_extent_map_range() fdmanana
` (11 subsequent siblings)
13 siblings, 0 replies; 20+ messages in thread
From: fdmanana @ 2022-09-19 14:06 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
The function btrfs_drop_extent_cache() doesn't really belong at file.c
because what it does is drop a range of extent maps for a file range.
It directly allocates and manipulates extent maps, by dropping, spliting
and replacing them in an extent map tree, so it should be located at
extent_map.c, where all manipulations of an extent map tree and its extent
maps are supposed to be done.
So move it out of file.c and into extent_map.c. Additionally do the
following changes:
1) Rename it into btrfs_drop_extent_map_range(), as this makes it more
clear about what it does. The term "cache" is a bit confusing as it's
not widely used, "extent maps" or "extent mapping" is much more common;
2) Change its 'skip_pinned' argument from int to bool;
3) Turn several of its local variables from int to bool, since they are
used as booleans;
4) Move the declaration of some variables out of the function's main
scope and into the scopes where they are used;
5) Remove pointless assignment of false to 'modified' early in the while
loop, as later that variable is set and it's not used before that
second assignment;
6) Remove checks for NULL before calling free_extent_map().
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/ctree.h | 2 -
fs/btrfs/extent_map.c | 192 +++++++++++++++++++++++++++++++++++
fs/btrfs/extent_map.h | 5 +
fs/btrfs/file.c | 190 +---------------------------------
fs/btrfs/free-space-cache.c | 2 +-
fs/btrfs/inode.c | 59 ++++++-----
fs/btrfs/relocation.c | 8 +-
fs/btrfs/tests/inode-tests.c | 2 +-
8 files changed, 237 insertions(+), 223 deletions(-)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 8b7b7a212da0..691a14fd3335 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3534,8 +3534,6 @@ int btrfs_add_inode_defrag(struct btrfs_trans_handle *trans,
int btrfs_run_defrag_inodes(struct btrfs_fs_info *fs_info);
void btrfs_cleanup_defrag_inodes(struct btrfs_fs_info *fs_info);
int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync);
-void btrfs_drop_extent_cache(struct btrfs_inode *inode, u64 start, u64 end,
- int skip_pinned);
extern const struct file_operations btrfs_file_operations;
int btrfs_drop_extents(struct btrfs_trans_handle *trans,
struct btrfs_root *root, struct btrfs_inode *inode,
diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index d5640e695e6b..587e0298bfab 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -7,6 +7,7 @@
#include "volumes.h"
#include "extent_map.h"
#include "compression.h"
+#include "btrfs_inode.h"
static struct kmem_cache *extent_map_cache;
@@ -658,3 +659,194 @@ int btrfs_add_extent_mapping(struct btrfs_fs_info *fs_info,
ASSERT(ret == 0 || ret == -EEXIST);
return ret;
}
+
+/*
+ * Drop all extent maps in a given range.
+ *
+ * @inode: The target inode.
+ * @start: Start offset of the range.
+ * @end: End offset of the range (inclusive value).
+ * @skip_pinned: Indicate if pinned extent maps should be ignored or not.
+ *
+ * This drops all the extent maps that intersect the given range [@start, @end].
+ * Extent maps that partially overlap the range and extend behind or beyond it,
+ * are split.
+ * The caller should have locked an appropriate file range in the inode's io
+ * tree before calling this function.
+ */
+void btrfs_drop_extent_map_range(struct btrfs_inode *inode, u64 start, u64 end,
+ bool skip_pinned)
+{
+ struct extent_map *split = NULL;
+ struct extent_map *split2 = NULL;
+ struct extent_map_tree *em_tree = &inode->extent_tree;
+ u64 len = end - start + 1;
+ bool testend = true;
+
+ WARN_ON(end < start);
+ if (end == (u64)-1) {
+ len = (u64)-1;
+ testend = false;
+ }
+ while (1) {
+ struct extent_map *em;
+ u64 gen;
+ unsigned long flags;
+ bool ends_after_range = false;
+ bool no_splits = false;
+ bool modified;
+ bool compressed;
+
+ if (!split)
+ split = alloc_extent_map();
+ if (!split2)
+ split2 = alloc_extent_map();
+ if (!split || !split2)
+ no_splits = true;
+
+ write_lock(&em_tree->lock);
+ em = lookup_extent_mapping(em_tree, start, len);
+ if (!em) {
+ write_unlock(&em_tree->lock);
+ break;
+ }
+ if (testend && em->start + em->len > start + len)
+ ends_after_range = true;
+ if (skip_pinned && test_bit(EXTENT_FLAG_PINNED, &em->flags)) {
+ if (ends_after_range) {
+ free_extent_map(em);
+ write_unlock(&em_tree->lock);
+ break;
+ }
+ start = em->start + em->len;
+ if (testend)
+ len = start + len - (em->start + em->len);
+ free_extent_map(em);
+ write_unlock(&em_tree->lock);
+ continue;
+ }
+ flags = em->flags;
+ gen = em->generation;
+ compressed = test_bit(EXTENT_FLAG_COMPRESSED, &em->flags);
+ clear_bit(EXTENT_FLAG_PINNED, &em->flags);
+ clear_bit(EXTENT_FLAG_LOGGING, &flags);
+ modified = !list_empty(&em->list);
+ if (no_splits)
+ goto next;
+
+ if (em->start < start) {
+ split->start = em->start;
+ split->len = start - em->start;
+
+ if (em->block_start < EXTENT_MAP_LAST_BYTE) {
+ split->orig_start = em->orig_start;
+ split->block_start = em->block_start;
+
+ if (compressed)
+ split->block_len = em->block_len;
+ else
+ split->block_len = split->len;
+ split->orig_block_len = max(split->block_len,
+ em->orig_block_len);
+ split->ram_bytes = em->ram_bytes;
+ } else {
+ split->orig_start = split->start;
+ split->block_len = 0;
+ split->block_start = em->block_start;
+ split->orig_block_len = 0;
+ split->ram_bytes = split->len;
+ }
+
+ split->generation = gen;
+ split->flags = flags;
+ split->compress_type = em->compress_type;
+ replace_extent_mapping(em_tree, em, split, modified);
+ free_extent_map(split);
+ split = split2;
+ split2 = NULL;
+ }
+ if (ends_after_range) {
+ split->start = start + len;
+ split->len = em->start + em->len - (start + len);
+ split->block_start = em->block_start;
+ split->flags = flags;
+ split->compress_type = em->compress_type;
+ split->generation = gen;
+
+ if (em->block_start < EXTENT_MAP_LAST_BYTE) {
+ split->orig_block_len = max(em->block_len,
+ em->orig_block_len);
+
+ split->ram_bytes = em->ram_bytes;
+ if (compressed) {
+ split->block_len = em->block_len;
+ split->orig_start = em->orig_start;
+ } else {
+ const u64 diff = start + len - em->start;
+
+ split->block_len = split->len;
+ split->block_start += diff;
+ split->orig_start = em->orig_start;
+ }
+ } else {
+ split->ram_bytes = split->len;
+ split->orig_start = split->start;
+ split->block_len = 0;
+ split->orig_block_len = 0;
+ }
+
+ if (extent_map_in_tree(em)) {
+ replace_extent_mapping(em_tree, em, split,
+ modified);
+ } else {
+ int ret;
+
+ ret = add_extent_mapping(em_tree, split,
+ modified);
+ /* Logic error, shouldn't happen. */
+ ASSERT(ret == 0);
+ if (WARN_ON(ret != 0) && modified)
+ btrfs_set_inode_full_sync(inode);
+ }
+ free_extent_map(split);
+ split = NULL;
+ }
+next:
+ if (extent_map_in_tree(em)) {
+ /*
+ * If the extent map is still in the tree it means that
+ * either of the following is true:
+ *
+ * 1) It fits entirely in our range (doesn't end beyond
+ * it or starts before it);
+ *
+ * 2) It starts before our range and/or ends after our
+ * range, and we were not able to allocate the extent
+ * maps for split operations, @split and @split2.
+ *
+ * If we are at case 2) then we just remove the entire
+ * extent map - this is fine since if anyone needs it to
+ * access the subranges outside our range, will just
+ * load it again from the subvolume tree's file extent
+ * item. However if the extent map was in the list of
+ * modified extents, then we must mark the inode for a
+ * full fsync, otherwise a fast fsync will miss this
+ * extent if it's new and needs to be logged.
+ */
+ if ((em->start < start || ends_after_range) && modified) {
+ ASSERT(no_splits);
+ btrfs_set_inode_full_sync(inode);
+ }
+ remove_extent_mapping(em_tree, em);
+ }
+ write_unlock(&em_tree->lock);
+
+ /* Once for us. */
+ free_extent_map(em);
+ /* And once for the tree. */
+ free_extent_map(em);
+ }
+
+ free_extent_map(split);
+ free_extent_map(split2);
+}
diff --git a/fs/btrfs/extent_map.h b/fs/btrfs/extent_map.h
index d2fa32ffe304..17f4a9bbee7f 100644
--- a/fs/btrfs/extent_map.h
+++ b/fs/btrfs/extent_map.h
@@ -63,6 +63,8 @@ struct extent_map_tree {
rwlock_t lock;
};
+struct btrfs_inode;
+
static inline int extent_map_in_tree(const struct extent_map *em)
{
return !RB_EMPTY_NODE(&em->rb_node);
@@ -104,5 +106,8 @@ struct extent_map *search_extent_mapping(struct extent_map_tree *tree,
int btrfs_add_extent_mapping(struct btrfs_fs_info *fs_info,
struct extent_map_tree *em_tree,
struct extent_map **em_in, u64 start, u64 len);
+void btrfs_drop_extent_map_range(struct btrfs_inode *inode,
+ u64 start, u64 end,
+ bool skip_pinned);
#endif
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 0e52292cf8bc..793c95dc8df5 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -498,190 +498,6 @@ int btrfs_dirty_pages(struct btrfs_inode *inode, struct page **pages,
return 0;
}
-/*
- * this drops all the extents in the cache that intersect the range
- * [start, end]. Existing extents are split as required.
- */
-void btrfs_drop_extent_cache(struct btrfs_inode *inode, u64 start, u64 end,
- int skip_pinned)
-{
- struct extent_map *em;
- struct extent_map *split = NULL;
- struct extent_map *split2 = NULL;
- struct extent_map_tree *em_tree = &inode->extent_tree;
- u64 len = end - start + 1;
- u64 gen;
- int ret;
- int testend = 1;
- unsigned long flags;
- int compressed = 0;
- bool modified;
-
- WARN_ON(end < start);
- if (end == (u64)-1) {
- len = (u64)-1;
- testend = 0;
- }
- while (1) {
- bool ends_after_range = false;
- int no_splits = 0;
-
- modified = false;
- if (!split)
- split = alloc_extent_map();
- if (!split2)
- split2 = alloc_extent_map();
- if (!split || !split2)
- no_splits = 1;
-
- write_lock(&em_tree->lock);
- em = lookup_extent_mapping(em_tree, start, len);
- if (!em) {
- write_unlock(&em_tree->lock);
- break;
- }
- if (testend && em->start + em->len > start + len)
- ends_after_range = true;
- flags = em->flags;
- gen = em->generation;
- if (skip_pinned && test_bit(EXTENT_FLAG_PINNED, &em->flags)) {
- if (ends_after_range) {
- free_extent_map(em);
- write_unlock(&em_tree->lock);
- break;
- }
- start = em->start + em->len;
- if (testend)
- len = start + len - (em->start + em->len);
- free_extent_map(em);
- write_unlock(&em_tree->lock);
- continue;
- }
- compressed = test_bit(EXTENT_FLAG_COMPRESSED, &em->flags);
- clear_bit(EXTENT_FLAG_PINNED, &em->flags);
- clear_bit(EXTENT_FLAG_LOGGING, &flags);
- modified = !list_empty(&em->list);
- if (no_splits)
- goto next;
-
- if (em->start < start) {
- split->start = em->start;
- split->len = start - em->start;
-
- if (em->block_start < EXTENT_MAP_LAST_BYTE) {
- split->orig_start = em->orig_start;
- split->block_start = em->block_start;
-
- if (compressed)
- split->block_len = em->block_len;
- else
- split->block_len = split->len;
- split->orig_block_len = max(split->block_len,
- em->orig_block_len);
- split->ram_bytes = em->ram_bytes;
- } else {
- split->orig_start = split->start;
- split->block_len = 0;
- split->block_start = em->block_start;
- split->orig_block_len = 0;
- split->ram_bytes = split->len;
- }
-
- split->generation = gen;
- split->flags = flags;
- split->compress_type = em->compress_type;
- replace_extent_mapping(em_tree, em, split, modified);
- free_extent_map(split);
- split = split2;
- split2 = NULL;
- }
- if (ends_after_range) {
- u64 diff = start + len - em->start;
-
- split->start = start + len;
- split->len = em->start + em->len - (start + len);
- split->flags = flags;
- split->compress_type = em->compress_type;
- split->generation = gen;
-
- if (em->block_start < EXTENT_MAP_LAST_BYTE) {
- split->orig_block_len = max(em->block_len,
- em->orig_block_len);
-
- split->ram_bytes = em->ram_bytes;
- if (compressed) {
- split->block_len = em->block_len;
- split->block_start = em->block_start;
- split->orig_start = em->orig_start;
- } else {
- split->block_len = split->len;
- split->block_start = em->block_start
- + diff;
- split->orig_start = em->orig_start;
- }
- } else {
- split->ram_bytes = split->len;
- split->orig_start = split->start;
- split->block_len = 0;
- split->block_start = em->block_start;
- split->orig_block_len = 0;
- }
-
- if (extent_map_in_tree(em)) {
- replace_extent_mapping(em_tree, em, split,
- modified);
- } else {
- ret = add_extent_mapping(em_tree, split,
- modified);
- /* Logic error, shouldn't happen. */
- ASSERT(ret == 0);
- if (WARN_ON(ret != 0) && modified)
- btrfs_set_inode_full_sync(inode);
- }
- free_extent_map(split);
- split = NULL;
- }
-next:
- if (extent_map_in_tree(em)) {
- /*
- * If the extent map is still in the tree it means that
- * either of the following is true:
- *
- * 1) It fits entirely in our range (doesn't end beyond
- * it or starts before it);
- *
- * 2) It starts before our range and/or ends after our
- * range, and we were not able to allocate the extent
- * maps for split operations, @split and @split2.
- *
- * If we are at case 2) then we just remove the entire
- * extent map - this is fine since if anyone needs it to
- * access the subranges outside our range, will just
- * load it again from the subvolume tree's file extent
- * item. However if the extent map was in the list of
- * modified extents, then we must mark the inode for a
- * full fsync, otherwise a fast fsync will miss this
- * extent if it's new and needs to be logged.
- */
- if ((em->start < start || ends_after_range) && modified) {
- ASSERT(no_splits);
- btrfs_set_inode_full_sync(inode);
- }
- remove_extent_mapping(em_tree, em);
- }
- write_unlock(&em_tree->lock);
-
- /* once for us */
- free_extent_map(em);
- /* once for the tree*/
- free_extent_map(em);
- }
- if (split)
- free_extent_map(split);
- if (split2)
- free_extent_map(split2);
-}
-
/*
* this is very complex, but the basic idea is to drop all extents
* in the range start - end. hint_block is filled in with a block number
@@ -739,7 +555,7 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
}
if (args->drop_cache)
- btrfs_drop_extent_cache(inode, args->start, args->end - 1, 0);
+ btrfs_drop_extent_map_range(inode, args->start, args->end - 1, false);
if (args->start >= inode->disk_i_size && !args->replace_extent)
modify_tree = 0;
@@ -2549,7 +2365,7 @@ static int fill_holes(struct btrfs_trans_handle *trans,
hole_em = alloc_extent_map();
if (!hole_em) {
- btrfs_drop_extent_cache(inode, offset, end - 1, 0);
+ btrfs_drop_extent_map_range(inode, offset, end - 1, false);
btrfs_set_inode_full_sync(inode);
} else {
hole_em->start = offset;
@@ -2564,7 +2380,7 @@ static int fill_holes(struct btrfs_trans_handle *trans,
hole_em->generation = trans->transid;
do {
- btrfs_drop_extent_cache(inode, offset, end - 1, 0);
+ btrfs_drop_extent_map_range(inode, offset, end - 1, false);
write_lock(&em_tree->lock);
ret = add_extent_mapping(em_tree, hole_em, 1);
write_unlock(&em_tree->lock);
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 7859eeca484c..f4023651dd68 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -349,7 +349,7 @@ int btrfs_truncate_free_space_cache(struct btrfs_trans_handle *trans,
truncate_pagecache(vfs_inode, 0);
lock_extent(&inode->io_tree, 0, (u64)-1, &cached_state);
- btrfs_drop_extent_cache(inode, 0, (u64)-1, 0);
+ btrfs_drop_extent_map_range(inode, 0, (u64)-1, false);
/*
* We skip the throttling logic for free space cache inodes, so we don't
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 6fde13f62c1d..4cb5a00c7533 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1024,7 +1024,7 @@ static int submit_one_async_extent(struct btrfs_inode *inode,
1 << BTRFS_ORDERED_COMPRESSED,
async_extent->compress_type);
if (ret) {
- btrfs_drop_extent_cache(inode, start, end, 0);
+ btrfs_drop_extent_map_range(inode, start, end, false);
goto out_free_reserve;
}
btrfs_dec_block_group_reservations(fs_info, ins.objectid);
@@ -1254,7 +1254,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
}
alloc_hint = get_extent_allocation_hint(inode, start, num_bytes);
- btrfs_drop_extent_cache(inode, start, start + num_bytes - 1, 0);
+ btrfs_drop_extent_map_range(inode, start, start + num_bytes - 1, false);
/*
* Relocation relies on the relocated extents to have exactly the same
@@ -1319,8 +1319,9 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
* skip current ordered extent.
*/
if (ret)
- btrfs_drop_extent_cache(inode, start,
- start + ram_size - 1, 0);
+ btrfs_drop_extent_map_range(inode, start,
+ start + ram_size - 1,
+ false);
}
btrfs_dec_block_group_reservations(fs_info, ins.objectid);
@@ -1360,7 +1361,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
return ret;
out_drop_extent_cache:
- btrfs_drop_extent_cache(inode, start, start + ram_size - 1, 0);
+ btrfs_drop_extent_map_range(inode, start, start + ram_size - 1, false);
out_reserve:
btrfs_dec_block_group_reservations(fs_info, ins.objectid);
btrfs_free_reserved_extent(fs_info, ins.objectid, ins.offset, 1);
@@ -2099,8 +2100,8 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
1 << BTRFS_ORDERED_PREALLOC,
BTRFS_COMPRESS_NONE);
if (ret) {
- btrfs_drop_extent_cache(inode, cur_offset,
- nocow_end, 0);
+ btrfs_drop_extent_map_range(inode, cur_offset,
+ nocow_end, false);
goto error;
}
} else {
@@ -3358,8 +3359,8 @@ int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent)
unwritten_start += logical_len;
clear_extent_uptodate(io_tree, unwritten_start, end, NULL);
- /* Drop the cache for the part of the extent we didn't write. */
- btrfs_drop_extent_cache(inode, unwritten_start, end, 0);
+ /* Drop extent maps for the part of the extent we didn't write. */
+ btrfs_drop_extent_map_range(inode, unwritten_start, end, false);
/*
* If the ordered extent had an IOERR or something else went
@@ -5088,8 +5089,9 @@ int btrfs_cont_expand(struct btrfs_inode *inode, loff_t oldsize, loff_t size)
if (err)
break;
- btrfs_drop_extent_cache(inode, cur_offset,
- cur_offset + hole_size - 1, 0);
+ btrfs_drop_extent_map_range(inode, cur_offset,
+ cur_offset + hole_size - 1,
+ false);
hole_em = alloc_extent_map();
if (!hole_em) {
btrfs_set_inode_full_sync(inode);
@@ -5112,9 +5114,9 @@ int btrfs_cont_expand(struct btrfs_inode *inode, loff_t oldsize, loff_t size)
write_unlock(&em_tree->lock);
if (err != -EEXIST)
break;
- btrfs_drop_extent_cache(inode, cur_offset,
- cur_offset +
- hole_size - 1, 0);
+ btrfs_drop_extent_map_range(inode, cur_offset,
+ cur_offset + hole_size - 1,
+ false);
}
free_extent_map(hole_em);
} else {
@@ -7084,7 +7086,8 @@ static struct extent_map *btrfs_create_dio_extent(struct btrfs_inode *inode,
if (ret) {
if (em) {
free_extent_map(em);
- btrfs_drop_extent_cache(inode, start, start + len - 1, 0);
+ btrfs_drop_extent_map_range(inode, start,
+ start + len - 1, false);
}
em = ERR_PTR(ret);
}
@@ -7382,8 +7385,8 @@ static struct extent_map *create_io_em(struct btrfs_inode *inode, u64 start,
}
do {
- btrfs_drop_extent_cache(inode, em->start,
- em->start + em->len - 1, 0);
+ btrfs_drop_extent_map_range(inode, em->start,
+ em->start + em->len - 1, false);
write_lock(&em_tree->lock);
ret = add_extent_mapping(em_tree, em, 1);
write_unlock(&em_tree->lock);
@@ -8641,9 +8644,9 @@ static int btrfs_truncate(struct inode *inode, bool skip_writeback)
* size is not block aligned since we will be keeping the last
* block of the extent just the way it is.
*/
- btrfs_drop_extent_cache(BTRFS_I(inode),
- ALIGN(new_size, fs_info->sectorsize),
- (u64)-1, 0);
+ btrfs_drop_extent_map_range(BTRFS_I(inode),
+ ALIGN(new_size, fs_info->sectorsize),
+ (u64)-1, false);
ret = btrfs_truncate_inode_items(trans, root, &control);
@@ -8816,7 +8819,7 @@ struct inode *btrfs_alloc_inode(struct super_block *sb)
#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
void btrfs_test_destroy_inode(struct inode *inode)
{
- btrfs_drop_extent_cache(BTRFS_I(inode), 0, (u64)-1, 0);
+ btrfs_drop_extent_map_range(BTRFS_I(inode), 0, (u64)-1, false);
kmem_cache_free(btrfs_inode_cachep, BTRFS_I(inode));
}
#endif
@@ -8878,7 +8881,7 @@ void btrfs_destroy_inode(struct inode *vfs_inode)
}
btrfs_qgroup_check_reserved_leak(inode);
inode_tree_del(inode);
- btrfs_drop_extent_cache(inode, 0, (u64)-1, 0);
+ btrfs_drop_extent_map_range(inode, 0, (u64)-1, false);
btrfs_inode_clear_file_extent_range(inode, 0, (u64)-1);
btrfs_put_root(inode->root);
}
@@ -9947,8 +9950,8 @@ static int __btrfs_prealloc_file_range(struct inode *inode, int mode,
break;
}
- btrfs_drop_extent_cache(BTRFS_I(inode), cur_offset,
- cur_offset + ins.offset -1, 0);
+ btrfs_drop_extent_map_range(BTRFS_I(inode), cur_offset,
+ cur_offset + ins.offset - 1, false);
em = alloc_extent_map();
if (!em) {
@@ -9972,9 +9975,9 @@ static int __btrfs_prealloc_file_range(struct inode *inode, int mode,
write_unlock(&em_tree->lock);
if (ret != -EEXIST)
break;
- btrfs_drop_extent_cache(BTRFS_I(inode), cur_offset,
- cur_offset + ins.offset - 1,
- 0);
+ btrfs_drop_extent_map_range(BTRFS_I(inode), cur_offset,
+ cur_offset + ins.offset - 1,
+ false);
}
free_extent_map(em);
next:
@@ -10802,7 +10805,7 @@ ssize_t btrfs_do_encoded_write(struct kiocb *iocb, struct iov_iter *from,
(1 << BTRFS_ORDERED_COMPRESSED),
compression);
if (ret) {
- btrfs_drop_extent_cache(inode, start, end, 0);
+ btrfs_drop_extent_map_range(inode, start, end, false);
goto out_free_reserved;
}
btrfs_dec_block_group_reservations(fs_info, ins.objectid);
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index d87020ae5810..1b4a61df5b7c 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1124,8 +1124,8 @@ int replace_file_extents(struct btrfs_trans_handle *trans,
if (!ret)
continue;
- btrfs_drop_extent_cache(BTRFS_I(inode),
- key.offset, end, 1);
+ btrfs_drop_extent_map_range(BTRFS_I(inode),
+ key.offset, end, true);
unlock_extent(&BTRFS_I(inode)->io_tree,
key.offset, end, NULL);
}
@@ -1567,7 +1567,7 @@ static int invalidate_extent_cache(struct btrfs_root *root,
/* the lock_extent waits for read_folio to complete */
lock_extent(&BTRFS_I(inode)->io_tree, start, end, NULL);
- btrfs_drop_extent_cache(BTRFS_I(inode), start, end, 1);
+ btrfs_drop_extent_map_range(BTRFS_I(inode), start, end, true);
unlock_extent(&BTRFS_I(inode)->io_tree, start, end, NULL);
}
return 0;
@@ -2913,7 +2913,7 @@ static noinline_for_stack int setup_relocation_extent_mapping(struct inode *inod
free_extent_map(em);
break;
}
- btrfs_drop_extent_cache(BTRFS_I(inode), start, end, 0);
+ btrfs_drop_extent_map_range(BTRFS_I(inode), start, end, false);
}
unlock_extent(&BTRFS_I(inode)->io_tree, start, end, NULL);
return ret;
diff --git a/fs/btrfs/tests/inode-tests.c b/fs/btrfs/tests/inode-tests.c
index b1c88dd187cb..625f7d398368 100644
--- a/fs/btrfs/tests/inode-tests.c
+++ b/fs/btrfs/tests/inode-tests.c
@@ -267,7 +267,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
goto out;
}
free_extent_map(em);
- btrfs_drop_extent_cache(BTRFS_I(inode), 0, (u64)-1, 0);
+ btrfs_drop_extent_map_range(BTRFS_I(inode), 0, (u64)-1, false);
/*
* All of the magic numbers are based on the mapping setup in
--
2.35.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 03/13] btrfs: use extent_map_end() at btrfs_drop_extent_map_range()
2022-09-19 14:06 [PATCH 00/13] btrfs: fixes and cleanups around extent maps fdmanana
2022-09-19 14:06 ` [PATCH 01/13] btrfs: fix missed extent on fsync after dropping " fdmanana
2022-09-19 14:06 ` [PATCH 02/13] btrfs: move btrfs_drop_extent_cache() to extent_map.c fdmanana
@ 2022-09-19 14:06 ` fdmanana
2022-09-19 14:06 ` [PATCH 04/13] btrfs: use cond_resched_rwlock_write() during inode eviction fdmanana
` (10 subsequent siblings)
13 siblings, 0 replies; 20+ messages in thread
From: fdmanana @ 2022-09-19 14:06 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
Instead of open coding the end offset calculation of an extent map, use
the helper extent_map_end() and cache its result in a local variable,
since it's used several times.
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/extent_map.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index 587e0298bfab..28c5e0243adc 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -690,6 +690,7 @@ void btrfs_drop_extent_map_range(struct btrfs_inode *inode, u64 start, u64 end,
}
while (1) {
struct extent_map *em;
+ u64 em_end;
u64 gen;
unsigned long flags;
bool ends_after_range = false;
@@ -710,7 +711,8 @@ void btrfs_drop_extent_map_range(struct btrfs_inode *inode, u64 start, u64 end,
write_unlock(&em_tree->lock);
break;
}
- if (testend && em->start + em->len > start + len)
+ em_end = extent_map_end(em);
+ if (testend && em_end > start + len)
ends_after_range = true;
if (skip_pinned && test_bit(EXTENT_FLAG_PINNED, &em->flags)) {
if (ends_after_range) {
@@ -718,9 +720,9 @@ void btrfs_drop_extent_map_range(struct btrfs_inode *inode, u64 start, u64 end,
write_unlock(&em_tree->lock);
break;
}
- start = em->start + em->len;
+ start = em_end;
if (testend)
- len = start + len - (em->start + em->len);
+ len = start + len - em_end;
free_extent_map(em);
write_unlock(&em_tree->lock);
continue;
@@ -767,7 +769,7 @@ void btrfs_drop_extent_map_range(struct btrfs_inode *inode, u64 start, u64 end,
}
if (ends_after_range) {
split->start = start + len;
- split->len = em->start + em->len - (start + len);
+ split->len = em_end - (start + len);
split->block_start = em->block_start;
split->flags = flags;
split->compress_type = em->compress_type;
--
2.35.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 04/13] btrfs: use cond_resched_rwlock_write() during inode eviction
2022-09-19 14:06 [PATCH 00/13] btrfs: fixes and cleanups around extent maps fdmanana
` (2 preceding siblings ...)
2022-09-19 14:06 ` [PATCH 03/13] btrfs: use extent_map_end() at btrfs_drop_extent_map_range() fdmanana
@ 2022-09-19 14:06 ` fdmanana
2022-09-19 14:06 ` [PATCH 05/13] btrfs: move open coded extent map tree deletion out of " fdmanana
` (9 subsequent siblings)
13 siblings, 0 replies; 20+ messages in thread
From: fdmanana @ 2022-09-19 14:06 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
At evict_inode_truncate_pages(), instead of manually checking if
rescheduling is needed, then unlock the extent map tree, reschedule and
then write lock again the tree, use the helper cond_resched_rwlock_write()
which does all that.
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/inode.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 4cb5a00c7533..32755e2977af 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5295,11 +5295,7 @@ static void evict_inode_truncate_pages(struct inode *inode)
clear_bit(EXTENT_FLAG_LOGGING, &em->flags);
remove_extent_mapping(map_tree, em);
free_extent_map(em);
- if (need_resched()) {
- write_unlock(&map_tree->lock);
- cond_resched();
- write_lock(&map_tree->lock);
- }
+ cond_resched_rwlock_write(&map_tree->lock);
}
write_unlock(&map_tree->lock);
--
2.35.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 05/13] btrfs: move open coded extent map tree deletion out of inode eviction
2022-09-19 14:06 [PATCH 00/13] btrfs: fixes and cleanups around extent maps fdmanana
` (3 preceding siblings ...)
2022-09-19 14:06 ` [PATCH 04/13] btrfs: use cond_resched_rwlock_write() during inode eviction fdmanana
@ 2022-09-19 14:06 ` fdmanana
2022-09-19 14:06 ` [PATCH 06/13] btrfs: add helper to replace extent map range with a new extent map fdmanana
` (8 subsequent siblings)
13 siblings, 0 replies; 20+ messages in thread
From: fdmanana @ 2022-09-19 14:06 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
Move the loop that removes all the extent maps from the inode's extent
map tree during inode eviction out of inode.c and into extent_map.c, to
btrfs_drop_extent_map_range(). Anything manipulating extent maps or the
extent map tree should be in extent_map.c.
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/extent_map.c | 27 +++++++++++++++++++++++++++
fs/btrfs/inode.c | 15 +--------------
2 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index 28c5e0243adc..7376c0aa2bca 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -660,6 +660,29 @@ int btrfs_add_extent_mapping(struct btrfs_fs_info *fs_info,
return ret;
}
+/*
+ * Drop all extent maps from a tree in the fastest possible way, rescheduling
+ * if needed. This avoids searching the tree, from the root down to the first
+ * extent map, before each deletion.
+ */
+static void drop_all_extent_maps_fast(struct extent_map_tree *tree)
+{
+ write_lock(&tree->lock);
+ while (!RB_EMPTY_ROOT(&tree->map.rb_root)) {
+ struct extent_map *em;
+ struct rb_node *node;
+
+ node = rb_first_cached(&tree->map);
+ em = rb_entry(node, struct extent_map, rb_node);
+ clear_bit(EXTENT_FLAG_PINNED, &em->flags);
+ clear_bit(EXTENT_FLAG_LOGGING, &em->flags);
+ remove_extent_mapping(tree, em);
+ free_extent_map(em);
+ cond_resched_rwlock_write(&tree->lock);
+ }
+ write_unlock(&tree->lock);
+}
+
/*
* Drop all extent maps in a given range.
*
@@ -685,6 +708,10 @@ void btrfs_drop_extent_map_range(struct btrfs_inode *inode, u64 start, u64 end,
WARN_ON(end < start);
if (end == (u64)-1) {
+ if (start == 0 && !skip_pinned) {
+ drop_all_extent_maps_fast(em_tree);
+ return;
+ }
len = (u64)-1;
testend = false;
}
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 32755e2977af..f0cfa9ff5ebd 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5279,25 +5279,12 @@ static int btrfs_setattr(struct user_namespace *mnt_userns, struct dentry *dentr
static void evict_inode_truncate_pages(struct inode *inode)
{
struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
- struct extent_map_tree *map_tree = &BTRFS_I(inode)->extent_tree;
struct rb_node *node;
ASSERT(inode->i_state & I_FREEING);
truncate_inode_pages_final(&inode->i_data);
- write_lock(&map_tree->lock);
- while (!RB_EMPTY_ROOT(&map_tree->map.rb_root)) {
- struct extent_map *em;
-
- node = rb_first_cached(&map_tree->map);
- em = rb_entry(node, struct extent_map, rb_node);
- clear_bit(EXTENT_FLAG_PINNED, &em->flags);
- clear_bit(EXTENT_FLAG_LOGGING, &em->flags);
- remove_extent_mapping(map_tree, em);
- free_extent_map(em);
- cond_resched_rwlock_write(&map_tree->lock);
- }
- write_unlock(&map_tree->lock);
+ btrfs_drop_extent_map_range(BTRFS_I(inode), 0, (u64)-1, false);
/*
* Keep looping until we have no more ranges in the io tree.
--
2.35.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 06/13] btrfs: add helper to replace extent map range with a new extent map
2022-09-19 14:06 [PATCH 00/13] btrfs: fixes and cleanups around extent maps fdmanana
` (4 preceding siblings ...)
2022-09-19 14:06 ` [PATCH 05/13] btrfs: move open coded extent map tree deletion out of " fdmanana
@ 2022-09-19 14:06 ` fdmanana
2022-09-19 14:06 ` [PATCH 07/13] btrfs: remove the refcount warning/check at free_extent_map() fdmanana
` (7 subsequent siblings)
13 siblings, 0 replies; 20+ messages in thread
From: fdmanana @ 2022-09-19 14:06 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
We have several places that need to drop all the extent maps in a given
file range and then add a new extent map for that range. Currently they
call btrfs_drop_extent_map_range() to delete all extent maps in the range
and then keep trying to add the new extent map in a loop that keeps
retrying while the insertion of the new extent map fails with -EEXIST.
So instead of repeating this logic, add a helper to extent_map.c that
does these steps and name it btrfs_replace_extent_map_range(). Also add
a comment about why the retry loop is necessary.
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/extent_map.c | 41 +++++++++++++++++++++++++++++++++++
fs/btrfs/extent_map.h | 3 +++
fs/btrfs/file.c | 8 +------
fs/btrfs/inode.c | 50 +++++++------------------------------------
fs/btrfs/relocation.c | 14 +++---------
5 files changed, 56 insertions(+), 60 deletions(-)
diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index 7376c0aa2bca..bef9cc8bfb2a 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -879,3 +879,44 @@ void btrfs_drop_extent_map_range(struct btrfs_inode *inode, u64 start, u64 end,
free_extent_map(split);
free_extent_map(split2);
}
+
+/*
+ * Replace a range in the inode's extent map tree with a new extent map.
+ *
+ * @inode: The target inode.
+ * @new_em: The new extent map to add to the inode's extent map tree.
+ * @modified: Indicate if the new extent map should be added to the list of
+ * modified extents (for fast fsync tracking).
+ *
+ * Drops all the extent maps in the inode's extent map tree that intersect the
+ * range of the new extent map and adds the new extent map to the tree.
+ * The caller should have locked an appropriate file range in the inode's io
+ * tree before calling this function.
+ */
+int btrfs_replace_extent_map_range(struct btrfs_inode *inode,
+ struct extent_map *new_em,
+ bool modified)
+{
+ const u64 end = new_em->start + new_em->len - 1;
+ struct extent_map_tree *tree = &inode->extent_tree;
+ int ret;
+
+ ASSERT(!extent_map_in_tree(new_em));
+
+ /*
+ * The caller has locked an appropriate file range in the inode's io
+ * tree, but getting -EEXIST when adding the new extent map can still
+ * happen in case there are extents that partially cover the range, and
+ * this is due to two tasks operating on different parts of the extent.
+ * See commit 18e83ac75bfe67 ("Btrfs: fix unexpected EEXIST from
+ * btrfs_get_extent") for an example and details.
+ */
+ do {
+ btrfs_drop_extent_map_range(inode, new_em->start, end, false);
+ write_lock(&tree->lock);
+ ret = add_extent_mapping(tree, new_em, modified);
+ write_unlock(&tree->lock);
+ } while (ret == -EEXIST);
+
+ return ret;
+}
diff --git a/fs/btrfs/extent_map.h b/fs/btrfs/extent_map.h
index 17f4a9bbee7f..ad311864272a 100644
--- a/fs/btrfs/extent_map.h
+++ b/fs/btrfs/extent_map.h
@@ -109,5 +109,8 @@ int btrfs_add_extent_mapping(struct btrfs_fs_info *fs_info,
void btrfs_drop_extent_map_range(struct btrfs_inode *inode,
u64 start, u64 end,
bool skip_pinned);
+int btrfs_replace_extent_map_range(struct btrfs_inode *inode,
+ struct extent_map *new_em,
+ bool modified);
#endif
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 793c95dc8df5..b5847111c783 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -2298,7 +2298,6 @@ static int fill_holes(struct btrfs_trans_handle *trans,
struct extent_buffer *leaf;
struct btrfs_file_extent_item *fi;
struct extent_map *hole_em;
- struct extent_map_tree *em_tree = &inode->extent_tree;
struct btrfs_key key;
int ret;
@@ -2379,12 +2378,7 @@ static int fill_holes(struct btrfs_trans_handle *trans,
hole_em->compress_type = BTRFS_COMPRESS_NONE;
hole_em->generation = trans->transid;
- do {
- btrfs_drop_extent_map_range(inode, offset, end - 1, false);
- write_lock(&em_tree->lock);
- ret = add_extent_mapping(em_tree, hole_em, 1);
- write_unlock(&em_tree->lock);
- } while (ret == -EEXIST);
+ ret = btrfs_replace_extent_map_range(inode, hole_em, true);
free_extent_map(hole_em);
if (ret)
btrfs_set_inode_full_sync(inode);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index f0cfa9ff5ebd..024183c7480f 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5041,7 +5041,6 @@ int btrfs_cont_expand(struct btrfs_inode *inode, loff_t oldsize, loff_t size)
struct extent_io_tree *io_tree = &inode->io_tree;
struct extent_map *em = NULL;
struct extent_state *cached_state = NULL;
- struct extent_map_tree *em_tree = &inode->extent_tree;
u64 hole_start = ALIGN(oldsize, fs_info->sectorsize);
u64 block_end = ALIGN(size, fs_info->sectorsize);
u64 last_byte;
@@ -5089,11 +5088,11 @@ int btrfs_cont_expand(struct btrfs_inode *inode, loff_t oldsize, loff_t size)
if (err)
break;
- btrfs_drop_extent_map_range(inode, cur_offset,
- cur_offset + hole_size - 1,
- false);
hole_em = alloc_extent_map();
if (!hole_em) {
+ btrfs_drop_extent_map_range(inode, cur_offset,
+ cur_offset + hole_size - 1,
+ false);
btrfs_set_inode_full_sync(inode);
goto next;
}
@@ -5108,16 +5107,7 @@ int btrfs_cont_expand(struct btrfs_inode *inode, loff_t oldsize, loff_t size)
hole_em->compress_type = BTRFS_COMPRESS_NONE;
hole_em->generation = fs_info->generation;
- while (1) {
- write_lock(&em_tree->lock);
- err = add_extent_mapping(em_tree, hole_em, 1);
- write_unlock(&em_tree->lock);
- if (err != -EEXIST)
- break;
- btrfs_drop_extent_map_range(inode, cur_offset,
- cur_offset + hole_size - 1,
- false);
- }
+ err = btrfs_replace_extent_map_range(inode, hole_em, true);
free_extent_map(hole_em);
} else {
err = btrfs_inode_set_file_extent_range(inode,
@@ -7337,7 +7327,6 @@ static struct extent_map *create_io_em(struct btrfs_inode *inode, u64 start,
u64 ram_bytes, int compress_type,
int type)
{
- struct extent_map_tree *em_tree;
struct extent_map *em;
int ret;
@@ -7346,7 +7335,6 @@ static struct extent_map *create_io_em(struct btrfs_inode *inode, u64 start,
type == BTRFS_ORDERED_NOCOW ||
type == BTRFS_ORDERED_REGULAR);
- em_tree = &inode->extent_tree;
em = alloc_extent_map();
if (!em)
return ERR_PTR(-ENOMEM);
@@ -7367,18 +7355,7 @@ static struct extent_map *create_io_em(struct btrfs_inode *inode, u64 start,
em->compress_type = compress_type;
}
- do {
- btrfs_drop_extent_map_range(inode, em->start,
- em->start + em->len - 1, false);
- write_lock(&em_tree->lock);
- ret = add_extent_mapping(em_tree, em, 1);
- write_unlock(&em_tree->lock);
- /*
- * The caller has taken lock_extent(), who could race with us
- * to add em?
- */
- } while (ret == -EEXIST);
-
+ ret = btrfs_replace_extent_map_range(inode, em, true);
if (ret) {
free_extent_map(em);
return ERR_PTR(ret);
@@ -9877,7 +9854,6 @@ static int __btrfs_prealloc_file_range(struct inode *inode, int mode,
struct btrfs_trans_handle *trans)
{
struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
- struct extent_map_tree *em_tree = &BTRFS_I(inode)->extent_tree;
struct extent_map *em;
struct btrfs_root *root = BTRFS_I(inode)->root;
struct btrfs_key ins;
@@ -9933,11 +9909,10 @@ static int __btrfs_prealloc_file_range(struct inode *inode, int mode,
break;
}
- btrfs_drop_extent_map_range(BTRFS_I(inode), cur_offset,
- cur_offset + ins.offset - 1, false);
-
em = alloc_extent_map();
if (!em) {
+ btrfs_drop_extent_map_range(BTRFS_I(inode), cur_offset,
+ cur_offset + ins.offset - 1, false);
btrfs_set_inode_full_sync(BTRFS_I(inode));
goto next;
}
@@ -9952,16 +9927,7 @@ static int __btrfs_prealloc_file_range(struct inode *inode, int mode,
set_bit(EXTENT_FLAG_PREALLOC, &em->flags);
em->generation = trans->transid;
- while (1) {
- write_lock(&em_tree->lock);
- ret = add_extent_mapping(em_tree, em, 1);
- write_unlock(&em_tree->lock);
- if (ret != -EEXIST)
- break;
- btrfs_drop_extent_map_range(BTRFS_I(inode), cur_offset,
- cur_offset + ins.offset - 1,
- false);
- }
+ ret = btrfs_replace_extent_map_range(BTRFS_I(inode), em, true);
free_extent_map(em);
next:
num_bytes -= ins.offset;
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 1b4a61df5b7c..cd8adb9f6d3f 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2890,7 +2890,6 @@ static noinline_for_stack int prealloc_file_extent_cluster(
static noinline_for_stack int setup_relocation_extent_mapping(struct inode *inode,
u64 start, u64 end, u64 block_start)
{
- struct extent_map_tree *em_tree = &BTRFS_I(inode)->extent_tree;
struct extent_map *em;
int ret = 0;
@@ -2905,17 +2904,10 @@ static noinline_for_stack int setup_relocation_extent_mapping(struct inode *inod
set_bit(EXTENT_FLAG_PINNED, &em->flags);
lock_extent(&BTRFS_I(inode)->io_tree, start, end, NULL);
- while (1) {
- write_lock(&em_tree->lock);
- ret = add_extent_mapping(em_tree, em, 0);
- write_unlock(&em_tree->lock);
- if (ret != -EEXIST) {
- free_extent_map(em);
- break;
- }
- btrfs_drop_extent_map_range(BTRFS_I(inode), start, end, false);
- }
+ ret = btrfs_replace_extent_map_range(BTRFS_I(inode), em, false);
unlock_extent(&BTRFS_I(inode)->io_tree, start, end, NULL);
+ free_extent_map(em);
+
return ret;
}
--
2.35.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 07/13] btrfs: remove the refcount warning/check at free_extent_map()
2022-09-19 14:06 [PATCH 00/13] btrfs: fixes and cleanups around extent maps fdmanana
` (5 preceding siblings ...)
2022-09-19 14:06 ` [PATCH 06/13] btrfs: add helper to replace extent map range with a new extent map fdmanana
@ 2022-09-19 14:06 ` fdmanana
2022-09-19 14:06 ` [PATCH 08/13] btrfs: remove unnecessary extent map initializations fdmanana
` (6 subsequent siblings)
13 siblings, 0 replies; 20+ messages in thread
From: fdmanana @ 2022-09-19 14:06 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
At free_extent_map(), it's pointless to have a WARN_ON() to check if the
refcount of the extent map is zero. Such check is already done by the
refcount_t module and refcount_dec_and_test(), which loudly complains if
we try to decrement a reference count that is currently 0.
The WARN_ON() dates back to the time when used a regular atomic_t type
for the reference counter, before we switched to the refcount_t type.
The main goal of the refcount_t type/module is precisely to catch such
types of bugs and loudly complain if they happen.
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/extent_map.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index bef9cc8bfb2a..2e6dc5a772f4 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -74,7 +74,6 @@ void free_extent_map(struct extent_map *em)
{
if (!em)
return;
- WARN_ON(refcount_read(&em->refs) == 0);
if (refcount_dec_and_test(&em->refs)) {
WARN_ON(extent_map_in_tree(em));
WARN_ON(!list_empty(&em->list));
--
2.35.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 08/13] btrfs: remove unnecessary extent map initializations
2022-09-19 14:06 [PATCH 00/13] btrfs: fixes and cleanups around extent maps fdmanana
` (6 preceding siblings ...)
2022-09-19 14:06 ` [PATCH 07/13] btrfs: remove the refcount warning/check at free_extent_map() fdmanana
@ 2022-09-19 14:06 ` fdmanana
2022-09-19 14:06 ` [PATCH 09/13] btrfs: assert tree is locked when clearing extent map from logging fdmanana
` (5 subsequent siblings)
13 siblings, 0 replies; 20+ messages in thread
From: fdmanana @ 2022-09-19 14:06 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
When allocating an extent map, we use kmem_cache_zalloc() which guarantees
the returned memory is initialized to zeroes, therefore it's pointless
to initialize the generation and flags of the extent map to zero again.
Remove those initializations, as they are pointless and slightly increase
the object text size.
Before removing them:
$ size fs/btrfs/extent_map.o
text data bss dec hex filename
9241 274 24 9539 2543 fs/btrfs/extent_map.o
After removing them:
$ size fs/btrfs/extent_map.o
text data bss dec hex filename
9209 274 24 9507 2523 fs/btrfs/extent_map.o
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/extent_map.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index 2e6dc5a772f4..6b7eee92d981 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -55,9 +55,7 @@ struct extent_map *alloc_extent_map(void)
if (!em)
return NULL;
RB_CLEAR_NODE(&em->rb_node);
- em->flags = 0;
em->compress_type = BTRFS_COMPRESS_NONE;
- em->generation = 0;
refcount_set(&em->refs, 1);
INIT_LIST_HEAD(&em->list);
return em;
--
2.35.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 09/13] btrfs: assert tree is locked when clearing extent map from logging
2022-09-19 14:06 [PATCH 00/13] btrfs: fixes and cleanups around extent maps fdmanana
` (7 preceding siblings ...)
2022-09-19 14:06 ` [PATCH 08/13] btrfs: remove unnecessary extent map initializations fdmanana
@ 2022-09-19 14:06 ` fdmanana
2022-09-19 14:06 ` [PATCH 10/13] btrfs: remove unnecessary NULL pointer checks when searching extent maps fdmanana
` (4 subsequent siblings)
13 siblings, 0 replies; 20+ messages in thread
From: fdmanana @ 2022-09-19 14:06 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
When calling clear_em_logging() we should have a write lock on the extent
map tree, as we will try to merge the extent map with the previous and
next ones in the tree. So assert that we have a write lock.
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/extent_map.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index 6b7eee92d981..f1616aa8d0f5 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -334,6 +334,8 @@ int unpin_extent_cache(struct extent_map_tree *tree, u64 start, u64 len,
void clear_em_logging(struct extent_map_tree *tree, struct extent_map *em)
{
+ lockdep_assert_held_write(&tree->lock);
+
clear_bit(EXTENT_FLAG_LOGGING, &em->flags);
if (extent_map_in_tree(em))
try_merge_map(tree, em);
--
2.35.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 10/13] btrfs: remove unnecessary NULL pointer checks when searching extent maps
2022-09-19 14:06 [PATCH 00/13] btrfs: fixes and cleanups around extent maps fdmanana
` (8 preceding siblings ...)
2022-09-19 14:06 ` [PATCH 09/13] btrfs: assert tree is locked when clearing extent map from logging fdmanana
@ 2022-09-19 14:06 ` fdmanana
2022-09-22 16:04 ` David Sterba
2022-09-19 14:06 ` [PATCH 11/13] btrfs: remove unnecessary next extent map search fdmanana
` (3 subsequent siblings)
13 siblings, 1 reply; 20+ messages in thread
From: fdmanana @ 2022-09-19 14:06 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
The previous and next pointer arguments passed to __tree_search() are
never NULL as the only caller of this function, __lookup_extent_mapping(),
always passes the address of two on stack pointers. So remove the NULL
checks.
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/extent_map.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index f1616aa8d0f5..12538ff04525 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -163,24 +163,21 @@ static struct rb_node *__tree_search(struct rb_root *root, u64 offset,
return n;
}
- if (prev_ret) {
- orig_prev = prev;
- while (prev && offset >= extent_map_end(prev_entry)) {
- prev = rb_next(prev);
- prev_entry = rb_entry(prev, struct extent_map, rb_node);
- }
- *prev_ret = prev;
- prev = orig_prev;
+ orig_prev = prev;
+ while (prev && offset >= extent_map_end(prev_entry)) {
+ prev = rb_next(prev);
+ prev_entry = rb_entry(prev, struct extent_map, rb_node);
}
+ *prev_ret = prev;
+ prev = orig_prev;
- if (next_ret) {
+ prev_entry = rb_entry(prev, struct extent_map, rb_node);
+ while (prev && offset < prev_entry->start) {
+ prev = rb_prev(prev);
prev_entry = rb_entry(prev, struct extent_map, rb_node);
- while (prev && offset < prev_entry->start) {
- prev = rb_prev(prev);
- prev_entry = rb_entry(prev, struct extent_map, rb_node);
- }
- *next_ret = prev;
}
+ *next_ret = prev;
+
return NULL;
}
--
2.35.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 10/13] btrfs: remove unnecessary NULL pointer checks when searching extent maps
2022-09-19 14:06 ` [PATCH 10/13] btrfs: remove unnecessary NULL pointer checks when searching extent maps fdmanana
@ 2022-09-22 16:04 ` David Sterba
0 siblings, 0 replies; 20+ messages in thread
From: David Sterba @ 2022-09-22 16:04 UTC (permalink / raw)
To: fdmanana; +Cc: linux-btrfs
On Mon, Sep 19, 2022 at 03:06:37PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> The previous and next pointer arguments passed to __tree_search() are
> never NULL as the only caller of this function, __lookup_extent_mapping(),
> always passes the address of two on stack pointers. So remove the NULL
> checks.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
> fs/btrfs/extent_map.c | 25 +++++++++++--------------
> 1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
> index f1616aa8d0f5..12538ff04525 100644
> --- a/fs/btrfs/extent_map.c
> +++ b/fs/btrfs/extent_map.c
> @@ -163,24 +163,21 @@ static struct rb_node *__tree_search(struct rb_root *root, u64 offset,
> return n;
> }
I've added asserts to verify the pointers are not accidentaly NULL.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 11/13] btrfs: remove unnecessary next extent map search
2022-09-19 14:06 [PATCH 00/13] btrfs: fixes and cleanups around extent maps fdmanana
` (9 preceding siblings ...)
2022-09-19 14:06 ` [PATCH 10/13] btrfs: remove unnecessary NULL pointer checks when searching extent maps fdmanana
@ 2022-09-19 14:06 ` fdmanana
2022-09-19 14:06 ` [PATCH 12/13] btrfs: avoid pointless extent map tree search when flushing delalloc fdmanana
` (2 subsequent siblings)
13 siblings, 0 replies; 20+ messages in thread
From: fdmanana @ 2022-09-19 14:06 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
At __tree_search(), and its single caller __lookup_extent_mapping(), there
is no point in finding the next extent map that starts after the search
offset if we were able to find the previous extent map that ends before
our search offset, because __lookup_extent_mapping() ignores the next
acceptable extent map if we were able to find the previous one.
So just return immediately if we were able to find the previous extent
map, therefore avoiding wasting time iterating the tree looking for the
next extent map which will not be used by __lookup_extent_mapping().
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/extent_map.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index 12538ff04525..a5b031524c32 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -141,8 +141,7 @@ static int tree_insert(struct rb_root_cached *root, struct extent_map *em)
* it can't be found, try to find some neighboring extents
*/
static struct rb_node *__tree_search(struct rb_root *root, u64 offset,
- struct rb_node **prev_ret,
- struct rb_node **next_ret)
+ struct rb_node **prev_or_next_ret)
{
struct rb_node *n = root->rb_node;
struct rb_node *prev = NULL;
@@ -168,15 +167,23 @@ static struct rb_node *__tree_search(struct rb_root *root, u64 offset,
prev = rb_next(prev);
prev_entry = rb_entry(prev, struct extent_map, rb_node);
}
- *prev_ret = prev;
- prev = orig_prev;
+ /*
+ * Previous extent map found, return as in this case the caller does not
+ * care about the next one.
+ */
+ if (prev) {
+ *prev_or_next_ret = prev;
+ return NULL;
+ }
+
+ prev = orig_prev;
prev_entry = rb_entry(prev, struct extent_map, rb_node);
while (prev && offset < prev_entry->start) {
prev = rb_prev(prev);
prev_entry = rb_entry(prev, struct extent_map, rb_node);
}
- *next_ret = prev;
+ *prev_or_next_ret = prev;
return NULL;
}
@@ -422,16 +429,13 @@ __lookup_extent_mapping(struct extent_map_tree *tree,
{
struct extent_map *em;
struct rb_node *rb_node;
- struct rb_node *prev = NULL;
- struct rb_node *next = NULL;
+ struct rb_node *prev_or_next = NULL;
u64 end = range_end(start, len);
- rb_node = __tree_search(&tree->map.rb_root, start, &prev, &next);
+ rb_node = __tree_search(&tree->map.rb_root, start, &prev_or_next);
if (!rb_node) {
- if (prev)
- rb_node = prev;
- else if (next)
- rb_node = next;
+ if (prev_or_next)
+ rb_node = prev_or_next;
else
return NULL;
}
--
2.35.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 12/13] btrfs: avoid pointless extent map tree search when flushing delalloc
2022-09-19 14:06 [PATCH 00/13] btrfs: fixes and cleanups around extent maps fdmanana
` (10 preceding siblings ...)
2022-09-19 14:06 ` [PATCH 11/13] btrfs: remove unnecessary next extent map search fdmanana
@ 2022-09-19 14:06 ` fdmanana
2022-09-19 14:06 ` [PATCH 13/13] btrfs: drop extent map range more efficiently fdmanana
2022-09-22 16:25 ` [PATCH 00/13] btrfs: fixes and cleanups around extent maps David Sterba
13 siblings, 0 replies; 20+ messages in thread
From: fdmanana @ 2022-09-19 14:06 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
When flushing delalloc, in COW mode at cow_file_range(), before entering
the loop that allocates extents and creates ordered extents, we do a call
to btrfs_drop_extent_map_range() for the whole range. This is pointless
because in the loop we call create_io_em(), which will also call
btrfs_drop_extent_map_range() before inserting the new extent map.
So remove that call at cow_file_range() not only because it is not needed,
but also because it will make the btrfs_drop_extent_map_range() calls made
from create_io_em() waste time searching the extent map tree, and that
tree can be large for files with many extents. It also makes us waste time
at btrfs_drop_extent_map_range() allocating and freeing the split extent
maps for nothing.
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/inode.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 024183c7480f..32fe3712ea9d 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1254,7 +1254,6 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
}
alloc_hint = get_extent_allocation_hint(inode, start, num_bytes);
- btrfs_drop_extent_map_range(inode, start, start + num_bytes - 1, false);
/*
* Relocation relies on the relocated extents to have exactly the same
--
2.35.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 13/13] btrfs: drop extent map range more efficiently
2022-09-19 14:06 [PATCH 00/13] btrfs: fixes and cleanups around extent maps fdmanana
` (11 preceding siblings ...)
2022-09-19 14:06 ` [PATCH 12/13] btrfs: avoid pointless extent map tree search when flushing delalloc fdmanana
@ 2022-09-19 14:06 ` fdmanana
2022-09-22 16:25 ` [PATCH 00/13] btrfs: fixes and cleanups around extent maps David Sterba
13 siblings, 0 replies; 20+ messages in thread
From: fdmanana @ 2022-09-19 14:06 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
Currently when dropping extent maps for a file range, through
btrfs_drop_extent_map_range(), we do the following non-optimal things:
1) We lookup for extent maps one by one, always starting the search from
the root of the extent map tree. This is not efficient if we have
multiple extent maps in the range;
2) We check on every iteration if we have the 'split' and 'split2' spare
extent maps in case we need to split an extent map that intersects our
range but also crosses its boundaries (to the left, to the right or
both cases). If our target range is for example:
[2M, 8M)
And we have 3 extents maps in the range:
[1M, 3M) [3M, 6M) [6M, 10M[
The on the first iteration we allocate two extent maps for 'split' and
'split2', and use the 'split' to split the first extent map, so after
the split we set 'split' to 'split2' and then set 'split2' to NULL.
On the second iteration, we don't need to split the second extent map,
but because 'split2' is now NULL, we allocate a new extent map for
'split2'.
On the third iteration we need to split the third extent map, so we
use the extent map pointed by 'split'.
So we ended up allocating 3 extent maps for splitting, but all we
needed was 2 extent maps. We never need to allocate more than 2,
because extent maps that need to be split are always the first one
and the last one in the target range.
Improve on this by:
1) Using rb_next() to move on to the next extent map. This results in
iterating over less nodes of the tree and it does not require comparing
the ranges of nodes to our start/end offset;
2) Allocate the 2 extent maps for splitting before entering the loop and
never allocate more than 2. In practice it's very rare to have the
combination of both extent map allocations fail, since we have a
dedicated slab for extent maps, and also have the need to split two
extent maps.
This patch is part of a patchset comprised of the following patches:
btrfs: fix missed extent on fsync after dropping extent maps
btrfs: move btrfs_drop_extent_cache() to extent_map.c
btrfs: use extent_map_end() at btrfs_drop_extent_map_range()
btrfs: use cond_resched_rwlock_write() during inode eviction
btrfs: move open coded extent map tree deletion out of inode eviction
btrfs: add helper to replace extent map range with a new extent map
btrfs: remove the refcount warning/check at free_extent_map()
btrfs: remove unnecessary extent map initializations
btrfs: assert tree is locked when clearing extent map from logging
btrfs: remove unnecessary NULL pointer checks when searching extent maps
btrfs: remove unnecessary next extent map search
btrfs: avoid pointless extent map tree search when flushing delalloc
btrfs: drop extent map range more efficiently
And the following fio test was done before and after applying the whole
patchset, on a non-debug kernel (Debian's default kernel config) on a 12
cores Intel box with 64G of ram:
$ cat test.sh
#!/bin/bash
DEV=/dev/nvme0n1
MNT=/mnt/nvme0n1
MOUNT_OPTIONS="-o ssd"
MKFS_OPTIONS="-R free-space-tree -O no-holes"
cat <<EOF > /tmp/fio-job.ini
[writers]
rw=randwrite
fsync=8
fallocate=none
group_reporting=1
direct=0
bssplit=4k/20:8k/20:16k/20:32k/10:64k/10:128k/5:256k/5:512k/5:1m/5
ioengine=psync
filesize=2G
runtime=300
time_based
directory=$MNT
numjobs=8
thread
EOF
echo performance | \
tee /sys/devices/system/cpu/cpu*/cpufreq/scaling_governor
echo
echo "Using config:"
echo
cat /tmp/fio-job.ini
echo
umount $MNT &> /dev/null
mkfs.btrfs -f $MKFS_OPTIONS $DEV
mount $MOUNT_OPTIONS $DEV $MNT
fio /tmp/fio-job.ini
umount $MNT
Result before applying the patchset:
WRITE: bw=197MiB/s (206MB/s), 197MiB/s-197MiB/s (206MB/s-206MB/s), io=57.7GiB (61.9GB), run=300188-300188msec
Result after applying the patchset:
WRITE: bw=203MiB/s (213MB/s), 203MiB/s-203MiB/s (213MB/s-213MB/s), io=59.5GiB (63.9GB), run=300019-300019msec
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/extent_map.c | 119 ++++++++++++++++++++++++++----------------
1 file changed, 74 insertions(+), 45 deletions(-)
diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index a5b031524c32..a3ec464b22a4 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -700,11 +700,11 @@ static void drop_all_extent_maps_fast(struct extent_map_tree *tree)
void btrfs_drop_extent_map_range(struct btrfs_inode *inode, u64 start, u64 end,
bool skip_pinned)
{
- struct extent_map *split = NULL;
- struct extent_map *split2 = NULL;
+ struct extent_map *split;
+ struct extent_map *split2;
+ struct extent_map *em;
struct extent_map_tree *em_tree = &inode->extent_tree;
u64 len = end - start + 1;
- bool testend = true;
WARN_ON(end < start);
if (end == (u64)-1) {
@@ -713,57 +713,73 @@ void btrfs_drop_extent_map_range(struct btrfs_inode *inode, u64 start, u64 end,
return;
}
len = (u64)-1;
- testend = false;
+ } else {
+ /* Make end offset exclusive for use in the loop below. */
+ end++;
}
- while (1) {
- struct extent_map *em;
- u64 em_end;
+
+ /*
+ * It's ok if we fail to allocate the extent maps, see the comment near
+ * the bottom of the loop below. We only need two spare extent maps in
+ * the worst case, where the first extent map that intersects our range
+ * starts before the range and the last extent map that intersects our
+ * range ends after our range (and they might be the same extent map),
+ * because we need to split those two extent maps at the boundaries.
+ */
+ split = alloc_extent_map();
+ split2 = alloc_extent_map();
+
+ write_lock(&em_tree->lock);
+ em = lookup_extent_mapping(em_tree, start, len);
+
+ while (em) {
+ /* extent_map_end() returns exclusive value (last byte + 1). */
+ const u64 em_end = extent_map_end(em);
+ struct extent_map *next_em = NULL;
u64 gen;
unsigned long flags;
- bool ends_after_range = false;
- bool no_splits = false;
bool modified;
bool compressed;
- if (!split)
- split = alloc_extent_map();
- if (!split2)
- split2 = alloc_extent_map();
- if (!split || !split2)
- no_splits = true;
-
- write_lock(&em_tree->lock);
- em = lookup_extent_mapping(em_tree, start, len);
- if (!em) {
- write_unlock(&em_tree->lock);
- break;
+ if (em_end < end) {
+ next_em = next_extent_map(em);
+ if (next_em) {
+ if (next_em->start < end)
+ refcount_inc(&next_em->refs);
+ else
+ next_em = NULL;
+ }
}
- em_end = extent_map_end(em);
- if (testend && em_end > start + len)
- ends_after_range = true;
+
if (skip_pinned && test_bit(EXTENT_FLAG_PINNED, &em->flags)) {
- if (ends_after_range) {
- free_extent_map(em);
- write_unlock(&em_tree->lock);
- break;
- }
start = em_end;
- if (testend)
+ if (end != (u64)-1)
len = start + len - em_end;
- free_extent_map(em);
- write_unlock(&em_tree->lock);
- continue;
+ goto next;
}
- flags = em->flags;
- gen = em->generation;
- compressed = test_bit(EXTENT_FLAG_COMPRESSED, &em->flags);
+
clear_bit(EXTENT_FLAG_PINNED, &em->flags);
clear_bit(EXTENT_FLAG_LOGGING, &flags);
modified = !list_empty(&em->list);
- if (no_splits)
- goto next;
+
+ /*
+ * The extent map does not cross our target range, so no need to
+ * split it, we can remove it directly.
+ */
+ if (em->start >= start && em_end <= end)
+ goto remove_em;
+
+ flags = em->flags;
+ gen = em->generation;
+ compressed = test_bit(EXTENT_FLAG_COMPRESSED, &em->flags);
if (em->start < start) {
+ if (!split) {
+ split = split2;
+ split2 = NULL;
+ if (!split)
+ goto remove_em;
+ }
split->start = em->start;
split->len = start - em->start;
@@ -794,7 +810,13 @@ void btrfs_drop_extent_map_range(struct btrfs_inode *inode, u64 start, u64 end,
split = split2;
split2 = NULL;
}
- if (ends_after_range) {
+ if (em_end > end) {
+ if (!split) {
+ split = split2;
+ split2 = NULL;
+ if (!split)
+ goto remove_em;
+ }
split->start = start + len;
split->len = em_end - (start + len);
split->block_start = em->block_start;
@@ -840,7 +862,7 @@ void btrfs_drop_extent_map_range(struct btrfs_inode *inode, u64 start, u64 end,
free_extent_map(split);
split = NULL;
}
-next:
+remove_em:
if (extent_map_in_tree(em)) {
/*
* If the extent map is still in the tree it means that
@@ -862,20 +884,27 @@ void btrfs_drop_extent_map_range(struct btrfs_inode *inode, u64 start, u64 end,
* full fsync, otherwise a fast fsync will miss this
* extent if it's new and needs to be logged.
*/
- if ((em->start < start || ends_after_range) && modified) {
- ASSERT(no_splits);
+ if ((em->start < start || em_end > end) && modified) {
+ ASSERT(!split);
btrfs_set_inode_full_sync(inode);
}
remove_extent_mapping(em_tree, em);
}
- write_unlock(&em_tree->lock);
- /* Once for us. */
+ /*
+ * Once for the tree reference (we replaced or removed the
+ * extent map from the tree).
+ */
free_extent_map(em);
- /* And once for the tree. */
+next:
+ /* Once for us (for our lookup reference). */
free_extent_map(em);
+
+ em = next_em;
}
+ write_unlock(&em_tree->lock);
+
free_extent_map(split);
free_extent_map(split2);
}
--
2.35.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 00/13] btrfs: fixes and cleanups around extent maps
2022-09-19 14:06 [PATCH 00/13] btrfs: fixes and cleanups around extent maps fdmanana
` (12 preceding siblings ...)
2022-09-19 14:06 ` [PATCH 13/13] btrfs: drop extent map range more efficiently fdmanana
@ 2022-09-22 16:25 ` David Sterba
13 siblings, 0 replies; 20+ messages in thread
From: David Sterba @ 2022-09-22 16:25 UTC (permalink / raw)
To: fdmanana; +Cc: linux-btrfs
On Mon, Sep 19, 2022 at 03:06:27PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> The following patchset fixes a bug related to dropping extent maps that
> can make an fsync miss a new extent, does several cleanups and some
> small performance improvements when dropping and searching for extent
> maps as well as when flushing delalloc in COW mode. These came out while
> working on some upcoming changes for fiemap, but since they are really
> independent, I'm sending them as a separate patchset.
> The last patch in the series has a test and results in its changelog.
>
> Filipe Manana (13):
> btrfs: fix missed extent on fsync after dropping extent maps
> btrfs: move btrfs_drop_extent_cache() to extent_map.c
> btrfs: use extent_map_end() at btrfs_drop_extent_map_range()
> btrfs: use cond_resched_rwlock_write() during inode eviction
> btrfs: move open coded extent map tree deletion out of inode eviction
> btrfs: add helper to replace extent map range with a new extent map
> btrfs: remove the refcount warning/check at free_extent_map()
> btrfs: remove unnecessary extent map initializations
> btrfs: assert tree is locked when clearing extent map from logging
> btrfs: remove unnecessary NULL pointer checks when searching extent maps
> btrfs: remove unnecessary next extent map search
> btrfs: avoid pointless extent map tree search when flushing delalloc
> btrfs: drop extent map range more efficiently
Added to misc-next, namely for the first fix but the cleanups seem safe
and the last patch with the performance improvement will be another one
for the 6.1. Thanks.
^ permalink raw reply [flat|nested] 20+ messages in thread