Since the beginning of zoned mode, I've promised Josef to get rid of the
extent_buffer redirtying, but never actually got around to doing so.
Then 2 weeks ago our CI has hit an ASSERT() in this area and I started to look
into it again. After some discussion with Christoph we came to the conclusion
to finally take the time and get rid of the extent_buffer redirtying once and
for all.
Patch one renames EXTENT_BUFFER_NO_CHECK into EXTENT_BUFFER_CANCELLED, because
this fits the new model somewhat better.
Number two sets the cancel bit instead of clearing the dirty bit from a zoned
extent_buffer.
Number three removes the last remaining bits of btrfs_redirty_list_add().
The last two patches in this series are just trivial cleanups I came across
while looking at the code.
---
Johannes Thumshirn (5):
btrfs: rename EXTENT_BUFFER_NO_CHECK to EXTENT_BUFFER_CANCELLED
btrfs: zoned: don't clear dirty flag of extent buffer
btrfs: remove now unneeded btrfs_redirty_list_add
btrfs: use memset_page instead of opencoding it
btrfs: reflow btrfs_free_tree_block
fs/btrfs/disk-io.c | 4 +-
fs/btrfs/extent-tree.c | 102 ++++++++++++++++++++++++-------------------------
fs/btrfs/extent_io.c | 9 +++--
fs/btrfs/extent_io.h | 3 +-
fs/btrfs/tree-log.c | 1 -
fs/btrfs/zoned.c | 16 --------
fs/btrfs/zoned.h | 5 ---
7 files changed, 60 insertions(+), 80 deletions(-)
---
base-commit: 592afe8e8b7ceee58107757fd29ff3290e6539e3
change-id: 20231120-josef-generic-163-f4df4eab2c98
Best regards,
--
Johannes Thumshirn <[email protected]>
EXTENT_BUFFER_CANCELLED better describes the state of the extent buffer,
namely its writeout has been cancelled.
Signed-off-by: Johannes Thumshirn <[email protected]>
---
fs/btrfs/disk-io.c | 2 +-
fs/btrfs/extent-tree.c | 2 +-
fs/btrfs/extent_io.c | 2 +-
fs/btrfs/extent_io.h | 3 ++-
fs/btrfs/zoned.c | 2 +-
5 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 5ac6789ca55f..ff6140e7eef7 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -254,7 +254,7 @@ blk_status_t btree_csum_one_bio(struct btrfs_bio *bbio)
if (WARN_ON_ONCE(bbio->bio.bi_iter.bi_size != eb->len))
return BLK_STS_IOERR;
- if (test_bit(EXTENT_BUFFER_NO_CHECK, &eb->bflags)) {
+ if (test_bit(EXTENT_BUFFER_CANCELLED, &eb->bflags)) {
WARN_ON_ONCE(found_start != 0);
return BLK_STS_OK;
}
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 0455935ff558..f6cbbec539fa 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5041,7 +5041,7 @@ btrfs_init_new_buffer(struct btrfs_trans_handle *trans, struct btrfs_root *root,
__btrfs_tree_lock(buf, nest);
btrfs_clear_buffer_dirty(trans, buf);
clear_bit(EXTENT_BUFFER_STALE, &buf->bflags);
- clear_bit(EXTENT_BUFFER_NO_CHECK, &buf->bflags);
+ clear_bit(EXTENT_BUFFER_CANCELLED, &buf->bflags);
set_extent_buffer_uptodate(buf);
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 03cef28d9e37..74f984885719 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4139,7 +4139,7 @@ static void __write_extent_buffer(const struct extent_buffer *eb,
/* For unmapped (dummy) ebs, no need to check their uptodate status. */
const bool check_uptodate = !test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);
- WARN_ON(test_bit(EXTENT_BUFFER_NO_CHECK, &eb->bflags));
+ WARN_ON(test_bit(EXTENT_BUFFER_CANCELLED, &eb->bflags));
if (check_eb_range(eb, start, len))
return;
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 2171057a4477..d5c9079dc578 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -28,7 +28,8 @@ enum {
EXTENT_BUFFER_IN_TREE,
/* write IO error */
EXTENT_BUFFER_WRITE_ERR,
- EXTENT_BUFFER_NO_CHECK,
+ /* Indicate the extent buffer write out is cancelled (for zoned) */
+ EXTENT_BUFFER_CANCELLED,
/* Indicate that extent buffer pages a being read */
EXTENT_BUFFER_READING,
};
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 188378ca19c7..89cd1664efe1 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1725,7 +1725,7 @@ void btrfs_redirty_list_add(struct btrfs_transaction *trans,
ASSERT(!test_bit(EXTENT_BUFFER_DIRTY, &eb->bflags));
memzero_extent_buffer(eb, 0, eb->len);
- set_bit(EXTENT_BUFFER_NO_CHECK, &eb->bflags);
+ set_bit(EXTENT_BUFFER_CANCELLED, &eb->bflags);
set_extent_buffer_dirty(eb);
set_extent_bit(&trans->dirty_pages, eb->start, eb->start + eb->len - 1,
EXTENT_DIRTY, NULL);
--
2.41.0
Now that we're not clearing the dirty flag off of extent_buffers in zoned mode,
all that is left of btrfs_redirty_list_add() is a memzero() and some
ASSERT()ions.
As we're also memzero()ing the buffer on write-out btrfs_redirty_list_add()
has become obsolete and can be removed.
Signed-off-by: Johannes Thumshirn <[email protected]>
---
fs/btrfs/extent-tree.c | 5 +----
fs/btrfs/tree-log.c | 1 -
fs/btrfs/zoned.c | 17 -----------------
fs/btrfs/zoned.h | 5 -----
4 files changed, 1 insertion(+), 27 deletions(-)
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index f6cbbec539fa..6beff11e692e 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3445,10 +3445,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
if (root_id != BTRFS_TREE_LOG_OBJECTID) {
ret = check_ref_cleanup(trans, buf->start);
- if (!ret) {
- btrfs_redirty_list_add(trans->transaction, buf);
+ if (!ret)
goto out;
- }
}
cache = btrfs_lookup_block_group(fs_info, buf->start);
@@ -3479,7 +3477,6 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
must_pin = true;
if (must_pin || btrfs_is_zoned(fs_info)) {
- btrfs_redirty_list_add(trans->transaction, buf);
pin_down_extent(trans, cache, buf->start, buf->len, 1);
btrfs_put_block_group(cache);
goto out;
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 7d6729d9fd2f..bee065851185 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -2575,7 +2575,6 @@ static int clean_log_buffer(struct btrfs_trans_handle *trans,
ret = btrfs_pin_reserved_extent(trans, eb);
if (ret)
return ret;
- btrfs_redirty_list_add(trans->transaction, eb);
} else {
unaccount_log_buffer(eb->fs_info, eb->start);
}
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 117e041bdc7a..931ccc839152 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1715,23 +1715,6 @@ void btrfs_calc_zone_unusable(struct btrfs_block_group *cache)
cache->zone_unusable = unusable;
}
-void btrfs_redirty_list_add(struct btrfs_transaction *trans,
- struct extent_buffer *eb)
-{
- if (!btrfs_is_zoned(eb->fs_info) ||
- btrfs_header_flag(eb, BTRFS_HEADER_FLAG_WRITTEN))
- return;
-
- ASSERT(test_bit(EXTENT_BUFFER_DIRTY, &eb->bflags));
- ASSERT(test_bit(EXTENT_BUFFER_CANCELLED, &eb->bflags));
-
- memzero_extent_buffer(eb, 0, eb->len);
- set_bit(EXTENT_BUFFER_CANCELLED, &eb->bflags);
- set_extent_buffer_dirty(eb);
- set_extent_bit(&trans->dirty_pages, eb->start, eb->start + eb->len - 1,
- EXTENT_DIRTY, NULL);
-}
-
bool btrfs_use_zone_append(struct btrfs_bio *bbio)
{
u64 start = (bbio->bio.bi_iter.bi_sector << SECTOR_SHIFT);
diff --git a/fs/btrfs/zoned.h b/fs/btrfs/zoned.h
index b9cec523b778..7bfe1d677310 100644
--- a/fs/btrfs/zoned.h
+++ b/fs/btrfs/zoned.h
@@ -59,8 +59,6 @@ int btrfs_reset_device_zone(struct btrfs_device *device, u64 physical,
int btrfs_ensure_empty_zones(struct btrfs_device *device, u64 start, u64 size);
int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new);
void btrfs_calc_zone_unusable(struct btrfs_block_group *cache);
-void btrfs_redirty_list_add(struct btrfs_transaction *trans,
- struct extent_buffer *eb);
bool btrfs_use_zone_append(struct btrfs_bio *bbio);
void btrfs_record_physical_zoned(struct btrfs_bio *bbio);
int btrfs_check_meta_write_pointer(struct btrfs_fs_info *fs_info,
@@ -180,9 +178,6 @@ static inline int btrfs_load_block_group_zone_info(
static inline void btrfs_calc_zone_unusable(struct btrfs_block_group *cache) { }
-static inline void btrfs_redirty_list_add(struct btrfs_transaction *trans,
- struct extent_buffer *eb) { }
-
static inline bool btrfs_use_zone_append(struct btrfs_bio *bbio)
{
return false;
--
2.41.0
One a zoned filesystem, never clear the dirty flag of an extent buffer,
but instead mark it as cancelled.
On writeout, when encountering cancelled extent_buffers, zero them out.
Signed-off-by: Johannes Thumshirn <[email protected]>
---
fs/btrfs/disk-io.c | 2 +-
fs/btrfs/extent_io.c | 7 +++++--
fs/btrfs/zoned.c | 3 ++-
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index ff6140e7eef7..f259bae1c3ee 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -255,7 +255,7 @@ blk_status_t btree_csum_one_bio(struct btrfs_bio *bbio)
return BLK_STS_IOERR;
if (test_bit(EXTENT_BUFFER_CANCELLED, &eb->bflags)) {
- WARN_ON_ONCE(found_start != 0);
+ memzero_extent_buffer(eb, 0, eb->len);
return BLK_STS_OK;
}
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 74f984885719..8bc5025ce278 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3748,6 +3748,11 @@ void btrfs_clear_buffer_dirty(struct btrfs_trans_handle *trans,
if (trans && btrfs_header_generation(eb) != trans->transid)
return;
+ if (btrfs_is_zoned(fs_info)) {
+ set_bit(EXTENT_BUFFER_CANCELLED, &eb->bflags);
+ return;
+ }
+
if (!test_and_clear_bit(EXTENT_BUFFER_DIRTY, &eb->bflags))
return;
@@ -4139,8 +4144,6 @@ static void __write_extent_buffer(const struct extent_buffer *eb,
/* For unmapped (dummy) ebs, no need to check their uptodate status. */
const bool check_uptodate = !test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);
- WARN_ON(test_bit(EXTENT_BUFFER_CANCELLED, &eb->bflags));
-
if (check_eb_range(eb, start, len))
return;
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 89cd1664efe1..117e041bdc7a 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1722,7 +1722,8 @@ void btrfs_redirty_list_add(struct btrfs_transaction *trans,
btrfs_header_flag(eb, BTRFS_HEADER_FLAG_WRITTEN))
return;
- ASSERT(!test_bit(EXTENT_BUFFER_DIRTY, &eb->bflags));
+ ASSERT(test_bit(EXTENT_BUFFER_DIRTY, &eb->bflags));
+ ASSERT(test_bit(EXTENT_BUFFER_CANCELLED, &eb->bflags));
memzero_extent_buffer(eb, 0, eb->len);
set_bit(EXTENT_BUFFER_CANCELLED, &eb->bflags);
--
2.41.0
Use memset_page() in memset_extent_buffer() instead of opencoding it.
This does not not change any functionality.
Signed-off-by: Johannes Thumshirn <[email protected]>
---
fs/btrfs/extent_io.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 8bc5025ce278..b8a73ebe3485 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4186,7 +4186,7 @@ static void memset_extent_buffer(const struct extent_buffer *eb, int c,
struct page *page = eb->pages[index];
assert_eb_page_uptodate(eb, page);
- memset(page_address(page) + offset, c, cur_len);
+ memset_page(page, offset, c, cur_len);
cur += cur_len;
}
--
2.41.0
Reflow btrfs_free_tree_block() so that there is one level of indentation
needed.
This patch has no functional changes.
Signed-off-by: Johannes Thumshirn <[email protected]>
---
fs/btrfs/extent-tree.c | 97 +++++++++++++++++++++++++-------------------------
1 file changed, 49 insertions(+), 48 deletions(-)
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 6beff11e692e..06818fe90fff 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3426,6 +3426,7 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
{
struct btrfs_fs_info *fs_info = trans->fs_info;
struct btrfs_ref generic_ref = { 0 };
+ struct btrfs_block_group *cache;
int ret;
btrfs_init_generic_ref(&generic_ref, BTRFS_DROP_DELAYED_REF,
@@ -3439,64 +3440,64 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
BUG_ON(ret); /* -ENOMEM */
}
- if (last_ref && btrfs_header_generation(buf) == trans->transid) {
- struct btrfs_block_group *cache;
- bool must_pin = false;
-
- if (root_id != BTRFS_TREE_LOG_OBJECTID) {
- ret = check_ref_cleanup(trans, buf->start);
- if (!ret)
- goto out;
- }
+ if (!last_ref)
+ return;
- cache = btrfs_lookup_block_group(fs_info, buf->start);
+ if (btrfs_header_generation(buf) != trans->transid)
+ goto out;
- if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
- pin_down_extent(trans, cache, buf->start, buf->len, 1);
- btrfs_put_block_group(cache);
+ if (root_id != BTRFS_TREE_LOG_OBJECTID) {
+ ret = check_ref_cleanup(trans, buf->start);
+ if (!ret)
goto out;
- }
+ }
- /*
- * If there are tree mod log users we may have recorded mod log
- * operations for this node. If we re-allocate this node we
- * could replay operations on this node that happened when it
- * existed in a completely different root. For example if it
- * was part of root A, then was reallocated to root B, and we
- * are doing a btrfs_old_search_slot(root b), we could replay
- * operations that happened when the block was part of root A,
- * giving us an inconsistent view of the btree.
- *
- * We are safe from races here because at this point no other
- * node or root points to this extent buffer, so if after this
- * check a new tree mod log user joins we will not have an
- * existing log of operations on this node that we have to
- * contend with.
- */
- if (test_bit(BTRFS_FS_TREE_MOD_LOG_USERS, &fs_info->flags))
- must_pin = true;
+ cache = btrfs_lookup_block_group(fs_info, buf->start);
- if (must_pin || btrfs_is_zoned(fs_info)) {
- pin_down_extent(trans, cache, buf->start, buf->len, 1);
- btrfs_put_block_group(cache);
- goto out;
- }
+ if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
+ pin_down_extent(trans, cache, buf->start, buf->len, 1);
+ btrfs_put_block_group(cache);
+ goto out;
+ }
- WARN_ON(test_bit(EXTENT_BUFFER_DIRTY, &buf->bflags));
+ /*
+ * If there are tree mod log users we may have recorded mod log
+ * operations for this node. If we re-allocate this node we
+ * could replay operations on this node that happened when it
+ * existed in a completely different root. For example if it
+ * was part of root A, then was reallocated to root B, and we
+ * are doing a btrfs_old_search_slot(root b), we could replay
+ * operations that happened when the block was part of root A,
+ * giving us an inconsistent view of the btree.
+ *
+ * We are safe from races here because at this point no other
+ * node or root points to this extent buffer, so if after this
+ * check a new tree mod log user joins we will not have an
+ * existing log of operations on this node that we have to
+ * contend with.
+ */
- btrfs_add_free_space(cache, buf->start, buf->len);
- btrfs_free_reserved_bytes(cache, buf->len, 0);
+ if (test_bit(BTRFS_FS_TREE_MOD_LOG_USERS, &fs_info->flags)
+ || btrfs_is_zoned(fs_info)) {
+ pin_down_extent(trans, cache, buf->start, buf->len, 1);
btrfs_put_block_group(cache);
- trace_btrfs_reserved_extent_free(fs_info, buf->start, buf->len);
+ goto out;
}
+
+ WARN_ON(test_bit(EXTENT_BUFFER_DIRTY, &buf->bflags));
+
+ btrfs_add_free_space(cache, buf->start, buf->len);
+ btrfs_free_reserved_bytes(cache, buf->len, 0);
+ btrfs_put_block_group(cache);
+ trace_btrfs_reserved_extent_free(fs_info, buf->start, buf->len);
+
out:
- if (last_ref) {
- /*
- * Deleting the buffer, clear the corrupt flag since it doesn't
- * matter anymore.
- */
- clear_bit(EXTENT_BUFFER_CORRUPT, &buf->bflags);
- }
+
+ /*
+ * Deleting the buffer, clear the corrupt flag since it doesn't
+ * matter anymore.
+ */
+ clear_bit(EXTENT_BUFFER_CORRUPT, &buf->bflags);
}
/* Can return -ENOMEM */
--
2.41.0
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
> if (test_bit(EXTENT_BUFFER_CANCELLED, &eb->bflags)) {
> - WARN_ON_ONCE(found_start != 0);
> + memzero_extent_buffer(eb, 0, eb->len);
> return BLK_STS_OK;
> + if (btrfs_is_zoned(fs_info)) {
> + set_bit(EXTENT_BUFFER_CANCELLED, &eb->bflags);
> + return;
> + }
Maybe these two places would benefit from comments on why the buffer
is just marked as cancelled and zeroed out (that is to keep the
write order because block numbers are already assigned)?
Otherwise this looks great:
Reviewed-by: Christoph Hellwig <[email protected]>
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
On Tue, Nov 21, 2023 at 08:32:29AM -0800, Johannes Thumshirn wrote:
> Since the beginning of zoned mode, I've promised Josef to get rid of the
> extent_buffer redirtying, but never actually got around to doing so.
>
> Then 2 weeks ago our CI has hit an ASSERT() in this area and I started to look
> into it again. After some discussion with Christoph we came to the conclusion
> to finally take the time and get rid of the extent_buffer redirtying once and
> for all.
>
> Patch one renames EXTENT_BUFFER_NO_CHECK into EXTENT_BUFFER_CANCELLED, because
> this fits the new model somewhat better.
>
> Number two sets the cancel bit instead of clearing the dirty bit from a zoned
> extent_buffer.
>
> Number three removes the last remaining bits of btrfs_redirty_list_add().
>
> The last two patches in this series are just trivial cleanups I came across
> while looking at the code.
>
Reviewed-by: Josef Bacik <[email protected]>
Thanks,
Josef
On Tue, Nov 21, 2023 at 08:32:30AM -0800, Johannes Thumshirn wrote:
> EXTENT_BUFFER_CANCELLED better describes the state of the extent buffer,
> namely its writeout has been cancelled.
I've read the patches a few times and still can't see how the meaning of
'cancelled' fits. It's about cancelling write out yes, but I don't see
anywhere explained why and why the eb is zeroed. This could be put next
to the enum definition or to function that does the main part of the
logic. You can also rename it to CANCELLED_WRITEOUT or use _ZONED_ in
the name so it's clear that it has a special purpose etc, but as it is
now I think it should be improved.
On 22.11.23 14:27, David Sterba wrote:
> On Tue, Nov 21, 2023 at 08:32:30AM -0800, Johannes Thumshirn wrote:
>> EXTENT_BUFFER_CANCELLED better describes the state of the extent buffer,
>> namely its writeout has been cancelled.
>
> I've read the patches a few times and still can't see how the meaning of
> 'cancelled' fits. It's about cancelling write out yes, but I don't see
> anywhere explained why and why the eb is zeroed. This could be put next
> to the enum definition or to function that does the main part of the
> logic. You can also rename it to CANCELLED_WRITEOUT or use _ZONED_ in
> the name so it's clear that it has a special purpose etc, but as it is
> now I think it should be improved.
>
How about EXTENT_BUFFER_ZONED_ZEROOUT? It a) indicates the buffer is
zeroed out and b) only for zoned mode.