diff mbox series

[v2,04/10] btrfs: zoned: defer advancing meta_write_pointer

Message ID b33f5b9b41cf80665f8df12c7094e260a38938bb.1690823282.git.naohiro.aota@wdc.com (mailing list archive)
State New, archived
Headers show
Series btrfs: zoned: write-time activation of metadata block group | expand

Commit Message

Naohiro Aota July 31, 2023, 5:17 p.m. UTC 
We currently advance the meta_write_pointer in
btrfs_check_meta_write_pointer(). That make it necessary to revert to it
when locking the buffer failed. Instead, we can advance it just before
sending the buffer.

Also, this is necessary for the following commit. In the commit, it needs
to release the zoned_meta_io_lock to allow IOs to come in and wait for them
to fill the currently active block group. If we advance the
meta_write_pointer before locking the extent buffer, the following extent
buffer can pass the meta_write_pointer check, resuting in an unaligned
write failure.

Advancing the pointer is still thread-safe as the extent buffer is locked.

Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 fs/btrfs/extent_io.c |  8 ++++----
 fs/btrfs/zoned.c     | 15 +--------------
 fs/btrfs/zoned.h     |  8 --------
 3 files changed, 5 insertions(+), 26 deletions(-)

Comments

Christoph Hellwig Aug. 1, 2023, 7:58 a.m. UTC | #1 
On Tue, Aug 01, 2023 at 02:17:13AM +0900, Naohiro Aota wrote:
>  	if (!lock_extent_buffer_for_io(eb, wbc)) {
> -		btrfs_revert_meta_write_pointer(ctx->block_group, eb);
>  		free_extent_buffer(eb);
>  		return 0;
>  	}
>  	if (ctx->block_group) {
> -		/*
> -		 * Implies write in zoned mode. Mark the last eb in a block group.
> -		 */
> +		/* Implies write in zoned mode. */

.. maybe ->block_group should be named ->zoned_bg to make this
implication very clear to everyone touching the code?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Johannes Thumshirn Aug. 1, 2023, 12:13 p.m. UTC | #2 
Looks good,

Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Naohiro Aota Aug. 2, 2023, 1:35 a.m. UTC | #3 
On Tue, Aug 01, 2023 at 12:58:04AM -0700, Christoph Hellwig wrote:
> On Tue, Aug 01, 2023 at 02:17:13AM +0900, Naohiro Aota wrote:
> >  	if (!lock_extent_buffer_for_io(eb, wbc)) {
> > -		btrfs_revert_meta_write_pointer(ctx->block_group, eb);
> >  		free_extent_buffer(eb);
> >  		return 0;
> >  	}
> >  	if (ctx->block_group) {
> > -		/*
> > -		 * Implies write in zoned mode. Mark the last eb in a block group.
> > -		 */
> > +		/* Implies write in zoned mode. */
> 
> .. maybe ->block_group should be named ->zoned_bg to make this
> implication very clear to everyone touching the code?

Indeed. I'll modify the patch 2 and add a comment as well.

> 
> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 012f2853b835..5388c2c3c6f4 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1834,15 +1834,15 @@  static int submit_eb_page(struct page *page, struct btrfs_eb_write_context *ctx)
 	}
 
 	if (!lock_extent_buffer_for_io(eb, wbc)) {
-		btrfs_revert_meta_write_pointer(ctx->block_group, eb);
 		free_extent_buffer(eb);
 		return 0;
 	}
 	if (ctx->block_group) {
-		/*
-		 * Implies write in zoned mode. Mark the last eb in a block group.
-		 */
+		/* Implies write in zoned mode. */
+
+		/* Mark the last eb in the block group. */
 		btrfs_schedule_zone_finish_bg(ctx->block_group, eb);
+		ctx->block_group->meta_write_pointer += eb->len;
 	}
 	write_one_eb(eb, wbc);
 	free_extent_buffer(eb);
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 0aa32b19adb5..fa595eca39ca 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1781,11 +1781,8 @@  int btrfs_check_meta_write_pointer(struct btrfs_fs_info *fs_info,
 		ctx->block_group = block_group;
 	}
 
-	if (block_group->meta_write_pointer == eb->start) {
-		block_group->meta_write_pointer = eb->start + eb->len;
-
+	if (block_group->meta_write_pointer == eb->start)
 		return 0;
-	}
 
 	/* If for_sync, this hole will be filled with trasnsaction commit. */
 	if (wbc->sync_mode == WB_SYNC_ALL && !wbc->for_sync)
@@ -1793,16 +1790,6 @@  int btrfs_check_meta_write_pointer(struct btrfs_fs_info *fs_info,
 	return -EBUSY;
 }
 
-void btrfs_revert_meta_write_pointer(struct btrfs_block_group *cache,
-				     struct extent_buffer *eb)
-{
-	if (!btrfs_is_zoned(eb->fs_info) || !cache)
-		return;
-
-	ASSERT(cache->meta_write_pointer == eb->start + eb->len);
-	cache->meta_write_pointer = eb->start;
-}
-
 int btrfs_zoned_issue_zeroout(struct btrfs_device *device, u64 physical, u64 length)
 {
 	if (!btrfs_dev_is_sequential(device, physical))
diff --git a/fs/btrfs/zoned.h b/fs/btrfs/zoned.h
index c0859d8be152..74ec37a25808 100644
--- a/fs/btrfs/zoned.h
+++ b/fs/btrfs/zoned.h
@@ -60,8 +60,6 @@  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,
 				   struct btrfs_eb_write_context *ctx);
-void btrfs_revert_meta_write_pointer(struct btrfs_block_group *cache,
-				     struct extent_buffer *eb);
 int btrfs_zoned_issue_zeroout(struct btrfs_device *device, u64 physical, u64 length);
 int btrfs_sync_zone_write_pointer(struct btrfs_device *tgt_dev, u64 logical,
 				  u64 physical_start, u64 physical_pos);
@@ -194,12 +192,6 @@  static inline int btrfs_check_meta_write_pointer(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
-static inline void btrfs_revert_meta_write_pointer(
-						struct btrfs_block_group *cache,
-						struct extent_buffer *eb)
-{
-}
-
 static inline int btrfs_zoned_issue_zeroout(struct btrfs_device *device,
 					    u64 physical, u64 length)
 {