[PATCH v2 0/4] btrfs: first batch of zoned cleanups

[PATCH v2 0/4] btrfs: first batch of zoned cleanups

[Johannes Thumshirn]

Here's the first btach of cleanups for the zoned code. In contrast to the 1st
submission [1] it is a minimalized set of patches with no dependencies on
other subsystems in btrfs. All of these patches are independant and can be
applied on their own.

The remaining patches from v1 will be sent out separately, once properly
reworked.

[1] https://lore.kernel.org/linux-btrfs/cover.1637745470.git.johannes.thumshirn@wdc.com/

Johannes Thumshirn (4):
  btrfs: zoned: encapsulate inode locking for zoned relocation
  btrfs: zoned: simplify btrfs_check_meta_write_pointer
  btrfs: zoned: sink zone check into btrfs_repair_one_zone
  btrfs: zoned: it's pointless to check for REQ_OP_ZONE_APPEND and
    btrfs_is_zoned

 fs/btrfs/extent_io.c | 17 ++++++-----------
 fs/btrfs/scrub.c     |  4 ++--
 fs/btrfs/volumes.c   | 13 ++++++++-----
 fs/btrfs/volumes.h   |  2 +-
 fs/btrfs/zoned.c     | 26 ++++++++------------------
 fs/btrfs/zoned.h     | 17 +++++++++++++++++
 6 files changed, 42 insertions(+), 37 deletions(-)

-- 
2.31.1

[PATCH v2 1/4] btrfs: zoned: encapsulate inode locking for zoned relocation

[Johannes Thumshirn]

Encapsulate the inode lock needed for serializing the data relocation
writes on a zoned filesystem into a helper.

This streamlines the code reading flow and hides special casing for
zoned filesystems.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent_io.c |  8 ++------
 fs/btrfs/zoned.h     | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 1a67f4b3986b..cc27e6e6d6ce 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5184,8 +5184,6 @@ int extent_writepages(struct address_space *mapping,
 		      struct writeback_control *wbc)
 {
 	struct inode *inode = mapping->host;
-	const bool data_reloc = btrfs_is_data_reloc_root(BTRFS_I(inode)->root);
-	const bool zoned = btrfs_is_zoned(BTRFS_I(inode)->root->fs_info);
 	int ret = 0;
 	struct extent_page_data epd = {
 		.bio_ctrl = { 0 },
@@ -5197,11 +5195,9 @@ int extent_writepages(struct address_space *mapping,
 	 * Allow only a single thread to do the reloc work in zoned mode to
 	 * protect the write pointer updates.
 	 */
-	if (data_reloc && zoned)
-		btrfs_inode_lock(inode, 0);
+	btrfs_zoned_data_reloc_lock(inode);
 	ret = extent_write_cache_pages(mapping, wbc, &epd);
-	if (data_reloc && zoned)
-		btrfs_inode_unlock(inode, 0);
+	btrfs_zoned_data_reloc_unlock(inode);
 	ASSERT(ret <= 0);
 	if (ret < 0) {
 		end_write_bio(&epd, ret);
diff --git a/fs/btrfs/zoned.h b/fs/btrfs/zoned.h
index 4344f4818389..e3eaf03a3422 100644
--- a/fs/btrfs/zoned.h
+++ b/fs/btrfs/zoned.h
@@ -8,6 +8,7 @@
 #include "volumes.h"
 #include "disk-io.h"
 #include "block-group.h"
+#include "btrfs_inode.h"
 
 /*
  * Block groups with more than this value (percents) of unusable space will be
@@ -354,4 +355,20 @@ static inline void btrfs_clear_treelog_bg(struct btrfs_block_group *bg)
 	spin_unlock(&fs_info->treelog_bg_lock);
 }
 
+static inline void btrfs_zoned_data_reloc_lock(struct inode *inode)
+{
+	struct btrfs_root *root = BTRFS_I(inode)->root;
+
+	if (btrfs_is_data_reloc_root(root) && btrfs_is_zoned(root->fs_info))
+		btrfs_inode_lock(inode, 0);
+}
+
+static inline void btrfs_zoned_data_reloc_unlock(struct inode *inode)
+{
+	struct btrfs_root *root = BTRFS_I(inode)->root;
+
+	if (btrfs_is_data_reloc_root(root) && btrfs_is_zoned(root->fs_info))
+		btrfs_inode_unlock(inode, 0);
+}
+
 #endif
-- 
2.31.1

Re: [PATCH v2 1/4] btrfs: zoned: encapsulate inode locking for zoned relocation

[David Sterba]

On Tue, Dec 07, 2021 at 06:28:34AM -0800, Johannes Thumshirn wrote:
> Encapsulate the inode lock needed for serializing the data relocation
> writes on a zoned filesystem into a helper.
> 
> This streamlines the code reading flow and hides special casing for
> zoned filesystems.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/extent_io.c |  8 ++------
>  fs/btrfs/zoned.h     | 17 +++++++++++++++++
>  2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 1a67f4b3986b..cc27e6e6d6ce 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5184,8 +5184,6 @@ int extent_writepages(struct address_space *mapping,
>  		      struct writeback_control *wbc)
>  {
>  	struct inode *inode = mapping->host;
> -	const bool data_reloc = btrfs_is_data_reloc_root(BTRFS_I(inode)->root);
> -	const bool zoned = btrfs_is_zoned(BTRFS_I(inode)->root->fs_info);
>  	int ret = 0;
>  	struct extent_page_data epd = {
>  		.bio_ctrl = { 0 },
> @@ -5197,11 +5195,9 @@ int extent_writepages(struct address_space *mapping,
>  	 * Allow only a single thread to do the reloc work in zoned mode to
>  	 * protect the write pointer updates.
>  	 */
> -	if (data_reloc && zoned)
> -		btrfs_inode_lock(inode, 0);
> +	btrfs_zoned_data_reloc_lock(inode);
>  	ret = extent_write_cache_pages(mapping, wbc, &epd);
> -	if (data_reloc && zoned)
> -		btrfs_inode_unlock(inode, 0);
> +	btrfs_zoned_data_reloc_unlock(inode);
>  	ASSERT(ret <= 0);
>  	if (ret < 0) {
>  		end_write_bio(&epd, ret);
> diff --git a/fs/btrfs/zoned.h b/fs/btrfs/zoned.h
> index 4344f4818389..e3eaf03a3422 100644
> --- a/fs/btrfs/zoned.h
> +++ b/fs/btrfs/zoned.h
> @@ -8,6 +8,7 @@
>  #include "volumes.h"
>  #include "disk-io.h"
>  #include "block-group.h"
> +#include "btrfs_inode.h"
>  
>  /*
>   * Block groups with more than this value (percents) of unusable space will be
> @@ -354,4 +355,20 @@ static inline void btrfs_clear_treelog_bg(struct btrfs_block_group *bg)
>  	spin_unlock(&fs_info->treelog_bg_lock);
>  }
>  
> +static inline void btrfs_zoned_data_reloc_lock(struct inode *inode)

All internal API should use struct btrfs_inode, applied with the
following diff:

--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5195,9 +5195,9 @@ int extent_writepages(struct address_space *mapping,
         * Allow only a single thread to do the reloc work in zoned mode to
         * protect the write pointer updates.
         */
-       btrfs_zoned_data_reloc_lock(inode);
+       btrfs_zoned_data_reloc_lock(BTRFS_I(inode));
        ret = extent_write_cache_pages(mapping, wbc, &epd);
-       btrfs_zoned_data_reloc_unlock(inode);
+       btrfs_zoned_data_reloc_unlock(BTRFS_I(inode));
        ASSERT(ret <= 0);
        if (ret < 0) {
                end_write_bio(&epd, ret);
diff --git a/fs/btrfs/zoned.h b/fs/btrfs/zoned.h
index e3eaf03a3422..a7b4cd6dd9f4 100644
--- a/fs/btrfs/zoned.h
+++ b/fs/btrfs/zoned.h
@@ -355,20 +355,20 @@ static inline void btrfs_clear_treelog_bg(struct btrfs_block_group *bg)
        spin_unlock(&fs_info->treelog_bg_lock);
 }
 
-static inline void btrfs_zoned_data_reloc_lock(struct inode *inode)
+static inline void btrfs_zoned_data_reloc_lock(struct btrfs_inode *inode)
 {
-       struct btrfs_root *root = BTRFS_I(inode)->root;
+       struct btrfs_root *root = inode->root;
 
        if (btrfs_is_data_reloc_root(root) && btrfs_is_zoned(root->fs_info))
-               btrfs_inode_lock(inode, 0);
+               btrfs_inode_lock(&inode->vfs_inode, 0);
 }
 
-static inline void btrfs_zoned_data_reloc_unlock(struct inode *inode)
+static inline void btrfs_zoned_data_reloc_unlock(struct btrfs_inode *inode)
 {
-       struct btrfs_root *root = BTRFS_I(inode)->root;
+       struct btrfs_root *root = inode->root;
 
        if (btrfs_is_data_reloc_root(root) && btrfs_is_zoned(root->fs_info))
-               btrfs_inode_unlock(inode, 0);
+               btrfs_inode_unlock(&inode->vfs_inode, 0);
 }
 
 #endif

Re: [PATCH v2 1/4] btrfs: zoned: encapsulate inode locking for zoned relocation

[Johannes Thumshirn]

On 07/12/2021 20:08, David Sterba wrote:
> All internal API should use struct btrfs_inode, applied with the
> following diff:

Ah ok, good to know. I wanted  to have a bit less pointer casing 
with using 'struct inode'. Anyways I don't mind.

Thanks

Re: [PATCH v2 1/4] btrfs: zoned: encapsulate inode locking for zoned relocation

[David Sterba]

On Wed, Dec 08, 2021 at 09:06:14AM +0000, Johannes Thumshirn wrote:
> On 07/12/2021 20:08, David Sterba wrote:
> > All internal API should use struct btrfs_inode, applied with the
> > following diff:
> 
> Ah ok, good to know. I wanted  to have a bit less pointer casing 
> with using 'struct inode'. Anyways I don't mind.

Now it's a mix of both and needs more works to unify but at least new
code should be done the right way. The pointer chasing will go
eventually and I don't think it has a measurable impact.

[PATCH v2 2/4] btrfs: zoned: simplify btrfs_check_meta_write_pointer

[Johannes Thumshirn]

btrfs_check_meta_write_pointer() will always be called with a NULL
'cache_ret' argument.

As there's no need to check if we have a valid block_group passed in
remove these checks.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/zoned.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 9cdef5e8f6b7..39bce555d3c6 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1641,29 +1641,19 @@ bool btrfs_check_meta_write_pointer(struct btrfs_fs_info *fs_info,
 	if (!btrfs_is_zoned(fs_info))
 		return true;
 
-	cache = *cache_ret;
+	cache = btrfs_lookup_block_group(fs_info, eb->start);
+	if (!cache)
+		return true;
 
-	if (cache && (eb->start < cache->start ||
-		      cache->start + cache->length <= eb->start)) {
+	if (cache->meta_write_pointer != eb->start) {
 		btrfs_put_block_group(cache);
 		cache = NULL;
-		*cache_ret = NULL;
+		ret = false;
+	} else {
+		cache->meta_write_pointer = eb->start + eb->len;
 	}
 
-	if (!cache)
-		cache = btrfs_lookup_block_group(fs_info, eb->start);
-
-	if (cache) {
-		if (cache->meta_write_pointer != eb->start) {
-			btrfs_put_block_group(cache);
-			cache = NULL;
-			ret = false;
-		} else {
-			cache->meta_write_pointer = eb->start + eb->len;
-		}
-
-		*cache_ret = cache;
-	}
+	*cache_ret = cache;
 
 	return ret;
 }
-- 
2.31.1

[PATCH v2 3/4] btrfs: zoned: sink zone check into btrfs_repair_one_zone

[Johannes Thumshirn]

Sink zone check into btrfs_repair_one_zone() so we don't need to do it in
all callers.

Also as btrfs_repair_one_zone() doesn't return a sensible error, make it a
boolean function and return false in case it got called on a non-zoned
filesystem and true on a zoned filesystem.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent_io.c |  4 ++--
 fs/btrfs/scrub.c     |  4 ++--
 fs/btrfs/volumes.c   | 13 ++++++++-----
 fs/btrfs/volumes.h   |  2 +-
 4 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index cc27e6e6d6ce..5a7350145cc3 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2314,8 +2314,8 @@ static int repair_io_failure(struct btrfs_fs_info *fs_info, u64 ino, u64 start,
 	ASSERT(!(fs_info->sb->s_flags & SB_RDONLY));
 	BUG_ON(!mirror_num);
 
-	if (btrfs_is_zoned(fs_info))
-		return btrfs_repair_one_zone(fs_info, logical);
+	if (btrfs_repair_one_zone(fs_info, logical))
+		return 0;
 
 	bio = btrfs_bio_alloc(1);
 	bio->bi_iter.bi_size = 0;
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 15a123e67108..a4369ded86eb 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -852,8 +852,8 @@ static int scrub_handle_errored_block(struct scrub_block *sblock_to_check)
 	have_csum = sblock_to_check->pagev[0]->have_csum;
 	dev = sblock_to_check->pagev[0]->dev;
 
-	if (btrfs_is_zoned(fs_info) && !sctx->is_dev_replace)
-		return btrfs_repair_one_zone(fs_info, logical);
+	if (!sctx->is_dev_replace && btrfs_repair_one_zone(fs_info, logical))
+		return 0;
 
 	/*
 	 * We must use GFP_NOFS because the scrub task might be waiting for a
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index f38c230111be..a7071f34fe64 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -8339,23 +8339,26 @@ static int relocating_repair_kthread(void *data)
 	return ret;
 }
 
-int btrfs_repair_one_zone(struct btrfs_fs_info *fs_info, u64 logical)
+bool btrfs_repair_one_zone(struct btrfs_fs_info *fs_info, u64 logical)
 {
 	struct btrfs_block_group *cache;
 
+	if (!btrfs_is_zoned(fs_info))
+		return false;
+
 	/* Do not attempt to repair in degraded state */
 	if (btrfs_test_opt(fs_info, DEGRADED))
-		return 0;
+		return true;
 
 	cache = btrfs_lookup_block_group(fs_info, logical);
 	if (!cache)
-		return 0;
+		return true;
 
 	spin_lock(&cache->lock);
 	if (cache->relocating_repair) {
 		spin_unlock(&cache->lock);
 		btrfs_put_block_group(cache);
-		return 0;
+		return true;
 	}
 	cache->relocating_repair = 1;
 	spin_unlock(&cache->lock);
@@ -8363,5 +8366,5 @@ int btrfs_repair_one_zone(struct btrfs_fs_info *fs_info, u64 logical)
 	kthread_run(relocating_repair_kthread, cache,
 		    "btrfs-relocating-repair");
 
-	return 0;
+	return true;
 }
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 3b8130680749..9cf1d93a3d66 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -637,6 +637,6 @@ enum btrfs_raid_types __attribute_const__ btrfs_bg_flags_to_raid_index(u64 flags
 int btrfs_bg_type_to_factor(u64 flags);
 const char *btrfs_bg_type_to_raid_name(u64 flags);
 int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info);
-int btrfs_repair_one_zone(struct btrfs_fs_info *fs_info, u64 logical);
+bool btrfs_repair_one_zone(struct btrfs_fs_info *fs_info, u64 logical);
 
 #endif
-- 
2.31.1

[PATCH v2 4/4] btrfs: zoned: it's pointless to check for REQ_OP_ZONE_APPEND and btrfs_is_zoned

[Johannes Thumshirn]

REQ_OP_ZONE_APPEND can only work on zoned devices, so it is redundant to
check if the filesystem is zoned when REQ_OP_ZONE_APPEND is set as the
bio's bio_op.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent_io.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 5a7350145cc3..4f70d6d2ce70 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3283,8 +3283,7 @@ static int calc_bio_boundaries(struct btrfs_bio_ctrl *bio_ctrl,
 	else
 		bio_ctrl->len_to_stripe_boundary = (u32)geom.len;
 
-	if (!btrfs_is_zoned(fs_info) ||
-	    bio_op(bio_ctrl->bio) != REQ_OP_ZONE_APPEND) {
+	if (bio_op(bio_ctrl->bio) != REQ_OP_ZONE_APPEND) {
 		bio_ctrl->len_to_oe_boundary = U32_MAX;
 		return 0;
 	}
@@ -3339,7 +3338,7 @@ static int alloc_new_bio(struct btrfs_inode *inode,
 		bio_set_dev(bio, bdev);
 		wbc_init_bio(wbc, bio);
 	}
-	if (btrfs_is_zoned(fs_info) && bio_op(bio) == REQ_OP_ZONE_APPEND) {
+	if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
 		struct btrfs_device *device;
 
 		device = btrfs_zoned_get_device(fs_info, disk_bytenr,
-- 
2.31.1

Re: [PATCH v2 0/4] btrfs: first batch of zoned cleanups

[David Sterba]

On Tue, Dec 07, 2021 at 06:28:33AM -0800, Johannes Thumshirn wrote:
> Here's the first btach of cleanups for the zoned code. In contrast to the 1st
> submission [1] it is a minimalized set of patches with no dependencies on
> other subsystems in btrfs. All of these patches are independant and can be
> applied on their own.
> 
> The remaining patches from v1 will be sent out separately, once properly
> reworked.
> 
> [1] https://lore.kernel.org/linux-btrfs/cover.1637745470.git.johannes.thumshirn@wdc.com/
> 
> Johannes Thumshirn (4):
>   btrfs: zoned: encapsulate inode locking for zoned relocation
>   btrfs: zoned: simplify btrfs_check_meta_write_pointer
>   btrfs: zoned: sink zone check into btrfs_repair_one_zone
>   btrfs: zoned: it's pointless to check for REQ_OP_ZONE_APPEND and
>     btrfs_is_zoned

Added to misc-next, thanks.