| Message ID | d20226362b9b193d85f63e81ee128ef3062e2203.1690171333.git.naohiro.aota@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: zoned: write-time activation of metadata block group | expand |
On Mon, Jul 24, 2023 at 01:18:30PM +0900, Naohiro Aota wrote: > For metadata write out on the zoned mode, we call > btrfs_check_meta_write_pointer() to check if an extent buffer to be written > is aligned to the write pointer. > > We lookup for a block group containing the extent buffer for every extent > buffer, which take unnecessary effort as the writing extent buffers are > mostly contiguous. > > Introduce "bg_context" to cache the block group working on. As someone who already found the eb_context naming and handling in the existing code highly confusing, I wonder if we should first add a new eb_write_context structure, which initially contains the wbc and eb_context pointers, and which also would grow the bg. This should make argument passing a little simpler, but most importantly removes all the dealing with the double pointers that need a lot of checking and clearing. > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c > index 5e4285ae112c..58bd2de4026d 100644 > --- a/fs/btrfs/zoned.c > +++ b/fs/btrfs/zoned.c > @@ -1751,27 +1751,33 @@ bool btrfs_check_meta_write_pointer(struct btrfs_fs_info *fs_info, > struct extent_buffer *eb, > struct btrfs_block_group **cache_ret) > { > > > - struct btrfs_block_group *cache; > - bool ret = true; > + struct btrfs_block_group *cache = NULL; .. and independent of the above comment, I also found the "cache" and "cache_ret" namings here very highly confusing. What speaks again using a bg-based naming that makes it clear what we're dealing with?
On Mon, Jul 24, 2023 at 08:00:04AM -0700, Christoph Hellwig wrote: > On Mon, Jul 24, 2023 at 01:18:30PM +0900, Naohiro Aota wrote: > > For metadata write out on the zoned mode, we call > > btrfs_check_meta_write_pointer() to check if an extent buffer to be written > > is aligned to the write pointer. > > > > We lookup for a block group containing the extent buffer for every extent > > buffer, which take unnecessary effort as the writing extent buffers are > > mostly contiguous. > > > > Introduce "bg_context" to cache the block group working on. > > As someone who already found the eb_context naming and handling in the > existing code highly confusing, I wonder if we should first add a new > eb_write_context structure, which initially contains the wbc > and eb_context pointers, and which also would grow the bg. This > should make argument passing a little simpler, but most importantly > removes all the dealing with the double pointers that need a lot > of checking and clearing. Ah, that works. Actually, I previously used the bio_ctrl for that purpose, but it was removed from the metadata context. > > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c > > index 5e4285ae112c..58bd2de4026d 100644 > > --- a/fs/btrfs/zoned.c > > +++ b/fs/btrfs/zoned.c > > @@ -1751,27 +1751,33 @@ bool btrfs_check_meta_write_pointer(struct btrfs_fs_info *fs_info, > > struct extent_buffer *eb, > > struct btrfs_block_group **cache_ret) > > { > > > > > > - struct btrfs_block_group *cache; > > - bool ret = true; > > + struct btrfs_block_group *cache = NULL; > > .. and independent of the above comment, I also found the "cache" and > "cache_ret" namings here very highly confusing. What speaks again > using a bg-based naming that makes it clear what we're dealing with? Yes. That is because the original code was written before than "btrfs_block_group_cache" is renamed to "btrfs_block_group". This is good opportunity to rename it as well.
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 91282aefcb77..c7a88d2b5555 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -1855,10 +1855,10 @@ static int submit_eb_subpage(struct page *page, struct writeback_control *wbc) * Return <0 for fatal error. */ static int submit_eb_page(struct page *page, struct writeback_control *wbc, - struct extent_buffer **eb_context) + struct extent_buffer **eb_context, + struct btrfs_block_group **bg_context) { struct address_space *mapping = page->mapping; - struct btrfs_block_group *cache = NULL; struct extent_buffer *eb; int ret; @@ -1894,7 +1894,7 @@ static int submit_eb_page(struct page *page, struct writeback_control *wbc, if (!ret) return 0; - if (!btrfs_check_meta_write_pointer(eb->fs_info, eb, &cache)) { + if (!btrfs_check_meta_write_pointer(eb->fs_info, eb, bg_context)) { /* * If for_sync, this hole will be filled with * trasnsaction commit. @@ -1910,18 +1910,15 @@ static int submit_eb_page(struct page *page, struct writeback_control *wbc, *eb_context = eb; if (!lock_extent_buffer_for_io(eb, wbc)) { - btrfs_revert_meta_write_pointer(cache, eb); - if (cache) - btrfs_put_block_group(cache); + btrfs_revert_meta_write_pointer(*bg_context, eb); free_extent_buffer(eb); return 0; } - if (cache) { + if (*bg_context) { /* * Implies write in zoned mode. Mark the last eb in a block group. */ - btrfs_schedule_zone_finish_bg(cache, eb); - btrfs_put_block_group(cache); + btrfs_schedule_zone_finish_bg(*bg_context, eb); } write_one_eb(eb, wbc); free_extent_buffer(eb); @@ -1932,6 +1929,7 @@ int btree_write_cache_pages(struct address_space *mapping, struct writeback_control *wbc) { struct extent_buffer *eb_context = NULL; + struct btrfs_block_group *bg_context = NULL; struct btrfs_fs_info *fs_info = BTRFS_I(mapping->host)->root->fs_info; int ret = 0; int done = 0; @@ -1973,7 +1971,7 @@ int btree_write_cache_pages(struct address_space *mapping, for (i = 0; i < nr_folios; i++) { struct folio *folio = fbatch.folios[i]; - ret = submit_eb_page(&folio->page, wbc, &eb_context); + ret = submit_eb_page(&folio->page, wbc, &eb_context, &bg_context); if (ret == 0) continue; if (ret < 0) { @@ -2034,6 +2032,9 @@ int btree_write_cache_pages(struct address_space *mapping, ret = 0; if (!ret && BTRFS_FS_ERROR(fs_info)) ret = -EROFS; + + if (bg_context) + btrfs_put_block_group(bg_context); btrfs_zoned_meta_io_unlock(fs_info); return ret; } diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c index 5e4285ae112c..58bd2de4026d 100644 --- a/fs/btrfs/zoned.c +++ b/fs/btrfs/zoned.c @@ -1751,27 +1751,33 @@ bool btrfs_check_meta_write_pointer(struct btrfs_fs_info *fs_info, struct extent_buffer *eb, struct btrfs_block_group **cache_ret) { - struct btrfs_block_group *cache; - bool ret = true; + struct btrfs_block_group *cache = NULL; if (!btrfs_is_zoned(fs_info)) return true; - cache = btrfs_lookup_block_group(fs_info, eb->start); - if (!cache) - return true; + if (*cache_ret) { + cache = *cache_ret; + if (cache->start > eb->start || + cache->start + cache->length <= eb->start) { + btrfs_put_block_group(cache); + cache = NULL; + *cache_ret = NULL; + } + } - 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; + if (!cache) { + cache = btrfs_lookup_block_group(fs_info, eb->start); + if (!cache) + return true; + *cache_ret = cache; } - *cache_ret = cache; + if (cache->meta_write_pointer != eb->start) + return false; + cache->meta_write_pointer = eb->start + eb->len; - return ret; + return true; } void btrfs_revert_meta_write_pointer(struct btrfs_block_group *cache,
For metadata write out on the zoned mode, we call btrfs_check_meta_write_pointer() to check if an extent buffer to be written is aligned to the write pointer. We lookup for a block group containing the extent buffer for every extent buffer, which take unnecessary effort as the writing extent buffers are mostly contiguous. Introduce "bg_context" to cache the block group working on. Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> --- fs/btrfs/extent_io.c | 21 +++++++++++---------- fs/btrfs/zoned.c | 32 +++++++++++++++++++------------- 2 files changed, 30 insertions(+), 23 deletions(-)