* [PATCH v2 0/3] btrfs: optimize the argument list for submit_extent_page()
@ 2022-09-13 5:31 Qu Wenruo
2022-09-13 5:31 ` [PATCH v2 1/3] btrfs: update the comment " Qu Wenruo
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Qu Wenruo @ 2022-09-13 5:31 UTC (permalink / raw)
To: linux-btrfs
[Changelog]
v2:
- Add a patch to remove stale argument from the comments of
submit_extent_page()
- Update the comment of submit_extent_page() to reflect the new arugment
list
The argument list of submit_extent_page() is already a little long.
Although we have things like page, pg_len, pg_off which can not be saved
anyway, we can still improve the situation by:
- Update the stale comment of submit_extent_page()
Done by the first patch.
- Make sure @page, @pg_len, @pg_off are always batched together
Just like bio_add_page().
This is done by the second page, just switching the position between
@page and @disk_bytenr.
- Move @end_io_func arugment into btrfs_bio_ctrl structure.
Qu Wenruo (3):
btrfs: update the comment for submit_extent_page()
btrfs: switch the page and disk_bytenr argument position for
submit_extent_page()
btrfs: move end_io_func argument to btrfs_bio_ctrl structure
fs/btrfs/extent_io.c | 66 +++++++++++++++++++++++++-------------------
1 file changed, 37 insertions(+), 29 deletions(-)
--
2.37.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/3] btrfs: update the comment for submit_extent_page()
2022-09-13 5:31 [PATCH v2 0/3] btrfs: optimize the argument list for submit_extent_page() Qu Wenruo
@ 2022-09-13 5:31 ` Qu Wenruo
2022-09-13 6:01 ` Anand Jain
2022-09-13 5:31 ` [PATCH v2 2/3] btrfs: switch the page and disk_bytenr argument position " Qu Wenruo
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Qu Wenruo @ 2022-09-13 5:31 UTC (permalink / raw)
To: linux-btrfs
Since commit 390ed29b817e ("btrfs: refactor submit_extent_page() to make
bio and its flag tracing easier"), we are using bio_ctrl structure to
replace some of arguments of submit_extent_page().
But unfortunately that commit didn't update the comment for
submit_extent_page(), thus some arguments are stale like:
- bio_ret
- mirror_num
Those are all contained in bio_ctrl now.
- prev_bio_flags
We no longer use this flag to determine if we can merge bios.
So this patch will update the comment for submit_extent_page() to keep
it up-to-date.
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
fs/btrfs/extent_io.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index cea7d09c2dc1..a3e8232c25ed 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3347,11 +3347,13 @@ static int alloc_new_bio(struct btrfs_inode *inode,
* @size: portion of page that we want to write to
* @pg_offset: offset of the new bio or to check whether we are adding
* a contiguous page to the previous one
- * @bio_ret: must be valid pointer, newly allocated bio will be stored there
* @end_io_func: end_io callback for new bio
- * @mirror_num: desired mirror to read/write
- * @prev_bio_flags: flags of previous bio to see if we can merge the current one
* @compress_type: compress type for current bio
+ *
+ * The function will either add the page into the existing @bio_ctrl->bio,
+ * or allocate a new one in @bio_ctrl->bio.
+ * The mirror number for this IO should already be initizlied in
+ * @bio_ctrl->mirror_num.
*/
static int submit_extent_page(blk_opf_t opf,
struct writeback_control *wbc,
--
2.37.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/3] btrfs: switch the page and disk_bytenr argument position for submit_extent_page()
2022-09-13 5:31 [PATCH v2 0/3] btrfs: optimize the argument list for submit_extent_page() Qu Wenruo
2022-09-13 5:31 ` [PATCH v2 1/3] btrfs: update the comment " Qu Wenruo
@ 2022-09-13 5:31 ` Qu Wenruo
2022-09-13 5:31 ` [PATCH v2 3/3] btrfs: move end_io_func argument to btrfs_bio_ctrl structure Qu Wenruo
2022-09-21 9:18 ` [PATCH v2 0/3] btrfs: optimize the argument list for submit_extent_page() David Sterba
3 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2022-09-13 5:31 UTC (permalink / raw)
To: linux-btrfs; +Cc: Anand Jain
Normally we put (page, pg_len, pg_offset) arguments together, just like
what __bio_add_page() does.
But in submit_extent_page(), what we got is, (page, disk_bytenr, pg_len,
pg_offset), which sometimes can be confusing.
Change the order to (disk_bytenr, page, pg_len, pg_offset) to make it
to follow the common schema.
Reviewed-by: Anand Jain <anand.jain@oracle.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
fs/btrfs/extent_io.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index a3e8232c25ed..ddf1600ea32b 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3342,8 +3342,8 @@ static int alloc_new_bio(struct btrfs_inode *inode,
/*
* @opf: bio REQ_OP_* and REQ_* flags as one value
* @wbc: optional writeback control for io accounting
- * @page: page to add to the bio
* @disk_bytenr: logical bytenr where the write will be
+ * @page: page to add to the bio
* @size: portion of page that we want to write to
* @pg_offset: offset of the new bio or to check whether we are adding
* a contiguous page to the previous one
@@ -3358,7 +3358,7 @@ static int alloc_new_bio(struct btrfs_inode *inode,
static int submit_extent_page(blk_opf_t opf,
struct writeback_control *wbc,
struct btrfs_bio_ctrl *bio_ctrl,
- struct page *page, u64 disk_bytenr,
+ u64 disk_bytenr, struct page *page,
size_t size, unsigned long pg_offset,
btrfs_bio_end_io_t end_io_func,
enum btrfs_compression_type compress_type,
@@ -3676,7 +3676,7 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
}
ret = submit_extent_page(REQ_OP_READ | read_flags, NULL,
- bio_ctrl, page, disk_bytenr, iosize,
+ bio_ctrl, disk_bytenr, page, iosize,
pg_offset, end_bio_extent_readpage,
this_bio_flag, force_bio_submit);
if (ret) {
@@ -3990,8 +3990,8 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
btrfs_page_clear_dirty(fs_info, page, cur, iosize);
ret = submit_extent_page(op | write_flags, wbc,
- &epd->bio_ctrl, page,
- disk_bytenr, iosize,
+ &epd->bio_ctrl, disk_bytenr,
+ page, iosize,
cur - page_offset(page),
end_bio_extent_writepage,
0, false);
@@ -4487,7 +4487,7 @@ static int write_one_subpage_eb(struct extent_buffer *eb,
clear_page_dirty_for_io(page);
ret = submit_extent_page(REQ_OP_WRITE | write_flags, wbc,
- &epd->bio_ctrl, page, eb->start, eb->len,
+ &epd->bio_ctrl, eb->start, page, eb->len,
eb->start - page_offset(page),
end_bio_subpage_eb_writepage, 0, false);
if (ret) {
@@ -4527,7 +4527,7 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
clear_page_dirty_for_io(p);
set_page_writeback(p);
ret = submit_extent_page(REQ_OP_WRITE | write_flags, wbc,
- &epd->bio_ctrl, p, disk_bytenr,
+ &epd->bio_ctrl, disk_bytenr, p,
PAGE_SIZE, 0,
end_bio_extent_buffer_writepage,
0, false);
@@ -6784,7 +6784,7 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
btrfs_subpage_start_reader(fs_info, page, eb->start, eb->len);
ret = submit_extent_page(REQ_OP_READ, NULL, &bio_ctrl,
- page, eb->start, eb->len,
+ eb->start, page, eb->len,
eb->start - page_offset(page),
end_bio_extent_readpage, 0, true);
if (ret) {
@@ -6889,7 +6889,7 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
ClearPageError(page);
err = submit_extent_page(REQ_OP_READ, NULL,
- &bio_ctrl, page, page_offset(page),
+ &bio_ctrl, page_offset(page), page,
PAGE_SIZE, 0, end_bio_extent_readpage,
0, false);
if (err) {
--
2.37.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 3/3] btrfs: move end_io_func argument to btrfs_bio_ctrl structure
2022-09-13 5:31 [PATCH v2 0/3] btrfs: optimize the argument list for submit_extent_page() Qu Wenruo
2022-09-13 5:31 ` [PATCH v2 1/3] btrfs: update the comment " Qu Wenruo
2022-09-13 5:31 ` [PATCH v2 2/3] btrfs: switch the page and disk_bytenr argument position " Qu Wenruo
@ 2022-09-13 5:31 ` Qu Wenruo
2022-09-21 9:18 ` [PATCH v2 0/3] btrfs: optimize the argument list for submit_extent_page() David Sterba
3 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2022-09-13 5:31 UTC (permalink / raw)
To: linux-btrfs; +Cc: Anand Jain
For function submit_extent_page() and alloc_new_bio(), we have an
argument @end_io_func to indicate the end io function.
But that function never change inside any call site of them, thus no
need to pass the pointer around everywhere.
There is a better match for the lifespan of all the call sites, as we
have btrfs_bio_ctrl structure, thus we can put the endio function
pointer there, and grab the pointer every time we allocate a new bio.
Also add extra ASSERT()s to make sure every call site of
submit_extent_page() and alloc_new_bio() has properly set the pointer
inside btrfs_bio_ctrl.
This removes one argument from the already long argument list of
submit_extent_page().
Reviewed-by: Anand Jain <anand.jain@oracle.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
fs/btrfs/extent_io.c | 40 +++++++++++++++++++++++-----------------
1 file changed, 23 insertions(+), 17 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index ddf1600ea32b..2499a01f7638 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -147,6 +147,7 @@ struct btrfs_bio_ctrl {
enum btrfs_compression_type compress_type;
u32 len_to_stripe_boundary;
u32 len_to_oe_boundary;
+ btrfs_bio_end_io_t end_io_func;
};
struct extent_page_data {
@@ -3278,7 +3279,6 @@ static int alloc_new_bio(struct btrfs_inode *inode,
struct btrfs_bio_ctrl *bio_ctrl,
struct writeback_control *wbc,
blk_opf_t opf,
- btrfs_bio_end_io_t end_io_func,
u64 disk_bytenr, u32 offset, u64 file_offset,
enum btrfs_compression_type compress_type)
{
@@ -3286,7 +3286,9 @@ static int alloc_new_bio(struct btrfs_inode *inode,
struct bio *bio;
int ret;
- bio = btrfs_bio_alloc(BIO_MAX_VECS, opf, end_io_func, NULL);
+ ASSERT(bio_ctrl->end_io_func);
+
+ bio = btrfs_bio_alloc(BIO_MAX_VECS, opf, bio_ctrl->end_io_func, NULL);
/*
* For compressed page range, its disk_bytenr is always @disk_bytenr
* passed in, no matter if we have added any range into previous bio.
@@ -3347,7 +3349,6 @@ static int alloc_new_bio(struct btrfs_inode *inode,
* @size: portion of page that we want to write to
* @pg_offset: offset of the new bio or to check whether we are adding
* a contiguous page to the previous one
- * @end_io_func: end_io callback for new bio
* @compress_type: compress type for current bio
*
* The function will either add the page into the existing @bio_ctrl->bio,
@@ -3360,7 +3361,6 @@ static int submit_extent_page(blk_opf_t opf,
struct btrfs_bio_ctrl *bio_ctrl,
u64 disk_bytenr, struct page *page,
size_t size, unsigned long pg_offset,
- btrfs_bio_end_io_t end_io_func,
enum btrfs_compression_type compress_type,
bool force_bio_submit)
{
@@ -3372,6 +3372,9 @@ static int submit_extent_page(blk_opf_t opf,
ASSERT(pg_offset < PAGE_SIZE && size <= PAGE_SIZE &&
pg_offset + size <= PAGE_SIZE);
+
+ ASSERT(bio_ctrl->end_io_func);
+
if (force_bio_submit)
submit_one_bio(bio_ctrl);
@@ -3382,7 +3385,7 @@ static int submit_extent_page(blk_opf_t opf,
/* Allocate new bio if needed */
if (!bio_ctrl->bio) {
ret = alloc_new_bio(inode, bio_ctrl, wbc, opf,
- end_io_func, disk_bytenr, offset,
+ disk_bytenr, offset,
page_offset(page) + cur,
compress_type);
if (ret < 0)
@@ -3562,6 +3565,7 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
memzero_page(page, zero_offset, iosize);
}
}
+ bio_ctrl->end_io_func = end_bio_extent_readpage;
begin_page_read(fs_info, page);
while (cur <= end) {
unsigned long this_bio_flag = 0;
@@ -3677,8 +3681,8 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
ret = submit_extent_page(REQ_OP_READ | read_flags, NULL,
bio_ctrl, disk_bytenr, page, iosize,
- pg_offset, end_bio_extent_readpage,
- this_bio_flag, force_bio_submit);
+ pg_offset, this_bio_flag,
+ force_bio_submit);
if (ret) {
/*
* We have to unlock the remaining range, or the page
@@ -3897,6 +3901,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
*/
wbc->nr_to_write--;
+ epd->bio_ctrl.end_io_func = end_bio_extent_writepage;
while (cur <= end) {
u64 disk_bytenr;
u64 em_end;
@@ -3993,7 +3998,6 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
&epd->bio_ctrl, disk_bytenr,
page, iosize,
cur - page_offset(page),
- end_bio_extent_writepage,
0, false);
if (ret) {
has_error = true;
@@ -4486,10 +4490,11 @@ static int write_one_subpage_eb(struct extent_buffer *eb,
if (no_dirty_ebs)
clear_page_dirty_for_io(page);
+ epd->bio_ctrl.end_io_func = end_bio_subpage_eb_writepage;
+
ret = submit_extent_page(REQ_OP_WRITE | write_flags, wbc,
&epd->bio_ctrl, eb->start, page, eb->len,
- eb->start - page_offset(page),
- end_bio_subpage_eb_writepage, 0, false);
+ eb->start - page_offset(page), 0, false);
if (ret) {
btrfs_subpage_clear_writeback(fs_info, page, eb->start, eb->len);
set_btree_ioerr(page, eb);
@@ -4520,6 +4525,8 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
prepare_eb_write(eb);
+ epd->bio_ctrl.end_io_func = end_bio_extent_buffer_writepage;
+
num_pages = num_extent_pages(eb);
for (i = 0; i < num_pages; i++) {
struct page *p = eb->pages[i];
@@ -4528,9 +4535,7 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
set_page_writeback(p);
ret = submit_extent_page(REQ_OP_WRITE | write_flags, wbc,
&epd->bio_ctrl, disk_bytenr, p,
- PAGE_SIZE, 0,
- end_bio_extent_buffer_writepage,
- 0, false);
+ PAGE_SIZE, 0, 0, false);
if (ret) {
set_btree_ioerr(p, eb);
if (PageWriteback(p))
@@ -6780,13 +6785,14 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
eb->read_mirror = 0;
atomic_set(&eb->io_pages, 1);
check_buffer_tree_ref(eb);
+ bio_ctrl.end_io_func = end_bio_extent_readpage;
+
btrfs_subpage_clear_error(fs_info, page, eb->start, eb->len);
btrfs_subpage_start_reader(fs_info, page, eb->start, eb->len);
ret = submit_extent_page(REQ_OP_READ, NULL, &bio_ctrl,
eb->start, page, eb->len,
- eb->start - page_offset(page),
- end_bio_extent_readpage, 0, true);
+ eb->start - page_offset(page), 0, true);
if (ret) {
/*
* In the endio function, if we hit something wrong we will
@@ -6877,6 +6883,7 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
* set io_pages. See check_buffer_tree_ref for a more detailed comment.
*/
check_buffer_tree_ref(eb);
+ bio_ctrl.end_io_func = end_bio_extent_readpage;
for (i = 0; i < num_pages; i++) {
page = eb->pages[i];
@@ -6890,8 +6897,7 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
ClearPageError(page);
err = submit_extent_page(REQ_OP_READ, NULL,
&bio_ctrl, page_offset(page), page,
- PAGE_SIZE, 0, end_bio_extent_readpage,
- 0, false);
+ PAGE_SIZE, 0, 0, false);
if (err) {
/*
* We failed to submit the bio so it's the
--
2.37.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/3] btrfs: update the comment for submit_extent_page()
2022-09-13 5:31 ` [PATCH v2 1/3] btrfs: update the comment " Qu Wenruo
@ 2022-09-13 6:01 ` Anand Jain
0 siblings, 0 replies; 6+ messages in thread
From: Anand Jain @ 2022-09-13 6:01 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs
On 13/09/2022 13:31, Qu Wenruo wrote:
> Since commit 390ed29b817e ("btrfs: refactor submit_extent_page() to make
> bio and its flag tracing easier"), we are using bio_ctrl structure to
> replace some of arguments of submit_extent_page().
>
> But unfortunately that commit didn't update the comment for
> submit_extent_page(), thus some arguments are stale like:
>
> - bio_ret
> - mirror_num
> Those are all contained in bio_ctrl now.
>
> - prev_bio_flags
> We no longer use this flag to determine if we can merge bios.
>
> So this patch will update the comment for submit_extent_page() to keep
> it up-to-date.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
Reviewed-by: Anand Jain <anand.jain@oracle.com>
> ---
> fs/btrfs/extent_io.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index cea7d09c2dc1..a3e8232c25ed 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3347,11 +3347,13 @@ static int alloc_new_bio(struct btrfs_inode *inode,
> * @size: portion of page that we want to write to
> * @pg_offset: offset of the new bio or to check whether we are adding
> * a contiguous page to the previous one
> - * @bio_ret: must be valid pointer, newly allocated bio will be stored there
> * @end_io_func: end_io callback for new bio
> - * @mirror_num: desired mirror to read/write
> - * @prev_bio_flags: flags of previous bio to see if we can merge the current one
> * @compress_type: compress type for current bio
> + *
> + * The function will either add the page into the existing @bio_ctrl->bio,
> + * or allocate a new one in @bio_ctrl->bio.
> + * The mirror number for this IO should already be initizlied in
> + * @bio_ctrl->mirror_num.
> */
> static int submit_extent_page(blk_opf_t opf,
> struct writeback_control *wbc,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/3] btrfs: optimize the argument list for submit_extent_page()
2022-09-13 5:31 [PATCH v2 0/3] btrfs: optimize the argument list for submit_extent_page() Qu Wenruo
` (2 preceding siblings ...)
2022-09-13 5:31 ` [PATCH v2 3/3] btrfs: move end_io_func argument to btrfs_bio_ctrl structure Qu Wenruo
@ 2022-09-21 9:18 ` David Sterba
3 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2022-09-21 9:18 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
On Tue, Sep 13, 2022 at 01:31:11PM +0800, Qu Wenruo wrote:
> [Changelog]
> v2:
> - Add a patch to remove stale argument from the comments of
> submit_extent_page()
>
> - Update the comment of submit_extent_page() to reflect the new arugment
> list
>
> The argument list of submit_extent_page() is already a little long.
>
> Although we have things like page, pg_len, pg_off which can not be saved
> anyway, we can still improve the situation by:
>
> - Update the stale comment of submit_extent_page()
> Done by the first patch.
>
> - Make sure @page, @pg_len, @pg_off are always batched together
> Just like bio_add_page().
>
> This is done by the second page, just switching the position between
> @page and @disk_bytenr.
>
> - Move @end_io_func arugment into btrfs_bio_ctrl structure.
>
> Qu Wenruo (3):
> btrfs: update the comment for submit_extent_page()
> btrfs: switch the page and disk_bytenr argument position for
> submit_extent_page()
> btrfs: move end_io_func argument to btrfs_bio_ctrl structure
Added to misc-next, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-21 9:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-13 5:31 [PATCH v2 0/3] btrfs: optimize the argument list for submit_extent_page() Qu Wenruo
2022-09-13 5:31 ` [PATCH v2 1/3] btrfs: update the comment " Qu Wenruo
2022-09-13 6:01 ` Anand Jain
2022-09-13 5:31 ` [PATCH v2 2/3] btrfs: switch the page and disk_bytenr argument position " Qu Wenruo
2022-09-13 5:31 ` [PATCH v2 3/3] btrfs: move end_io_func argument to btrfs_bio_ctrl structure Qu Wenruo
2022-09-21 9:18 ` [PATCH v2 0/3] btrfs: optimize the argument list for submit_extent_page() David Sterba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).