Re: [PATCH 13/21] btrfs: don't use btrfs_bio_ctrl for extent buffer writing

[David Sterba <dsterba@suse.cz>]

On Wed, May 24, 2023 at 07:50:03AM +0200, Christoph Hellwig wrote:
> On Tue, May 23, 2023 at 08:45:41PM +0200, David Sterba wrote:
> > On Wed, May 03, 2023 at 05:24:33PM +0200, Christoph Hellwig wrote:
> > > The btrfs_bio_ctrl machinery is overkill for writing extent_buffers
> > > as we always operate on PAGE SIZE chunks (or one smaller one for the
> > > subpage case) that are contigous and are guaranteed to fit into a
> > > single bio.  Replace it with open coded btrfs_bio_alloc, __bio_add_page
> > > and btrfs_submit_bio calls.
> > 
> > submit_extent_page hasn't been open coded completely, there's still call
> > to wbc_account_cgroup_owner() but it's probably skipped for other
> > reasons as this is metadata inode.
> 
> Hmm, I was under the impression that we never did cgroup accounting for
> metadata.  While that is true for the rest of the kernel, I fear I might
> have changed semantics for btrfs here (for better or worse).  Let me
> dig into the code again, but I suspect I'll need to add
> wbc_account_cgroup_owner and wbc_init_bio back to not change semantics
> vs the old code.

Until we know for sure I'd rather keep the call, so something like that
(untested):

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index fcd0563f7a15..a6480fa0366c 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1833,6 +1833,8 @@ static noinline_for_stack void write_one_eb(struct extent_buffer *eb,
                        wbc->nr_to_write--;
                }
                __bio_add_page(&bbio->bio, p, eb->len, eb->start - page_offset(p));
+               if (bio_ctrl->wbc)
+                       wbc_account_cgroup_owner(bio_ctrl->wbc, p, len);
                unlock_page(p);
        } else {
                for (int i = 0; i < num_extent_pages(eb); i++) {
@@ -1842,6 +1844,8 @@ static noinline_for_stack void write_one_eb(struct extent_buffer *eb,
                        clear_page_dirty_for_io(p);
                        set_page_writeback(p);
                        __bio_add_page(&bbio->bio, p, PAGE_SIZE, 0);
+                       if (bio_ctrl->wbc)
+                               wbc_account_cgroup_owner(bio_ctrl->wbc, p, len);
                        wbc->nr_to_write--;
                        unlock_page(p);
                }
@@ -4090,9 +4094,15 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num,
        if (eb->fs_info->nodesize < PAGE_SIZE) {
                __bio_add_page(&bbio->bio, eb->pages[0], eb->len,
                               eb->start - page_offset(eb->pages[0]));
+               if (bio_ctrl->wbc)
+                       wbc_account_cgroup_owner(bio_ctrl->wbc, eb->pages[0], len);
        } else {
-               for (i = 0; i < num_pages; i++)
+               for (i = 0; i < num_pages; i++) {
                        __bio_add_page(&bbio->bio, eb->pages[i], PAGE_SIZE, 0);
+                       if (bio_ctrl->wbc)
+                               wbc_account_cgroup_owner(bio_ctrl->wbc,
+                                                        eb->pages[i], len);
+               }
        }
        btrfs_submit_bio(bbio, mirror_num);
 
---

I can fold it to the patches.