* [PATCH 01/12] btrfs: don't set force_bio_submit in read_extent_buffer_subpage
2023-02-27 15:16 cleanup submit_extent_page and friends v2 Christoph Hellwig
@ 2023-02-27 15:16 ` Christoph Hellwig
2023-02-27 16:05 ` Johannes Thumshirn
2023-02-27 15:16 ` [PATCH 02/12] btrfs: remove the force_bio_submit to submit_extent_page Christoph Hellwig
` (11 subsequent siblings)
12 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2023-02-27 15:16 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba; +Cc: linux-btrfs
When read_extent_buffer_subpage calls submit_extent_page, it does
so on a freshly initialized btrfs_bio_ctrl structure that can't have
a valid bio to submit. Clear the force_bio_submit parameter to false
as there is nothing to submit.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/btrfs/extent_io.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 1034e9c86e9bd4..90842f5a978931 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4448,7 +4448,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,
eb->start, page, eb->len,
- eb->start - page_offset(page), 0, true);
+ eb->start - page_offset(page), 0, false);
if (ret) {
/*
* In the endio function, if we hit something wrong we will
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 02/12] btrfs: remove the force_bio_submit to submit_extent_page
2023-02-27 15:16 cleanup submit_extent_page and friends v2 Christoph Hellwig
2023-02-27 15:16 ` [PATCH 01/12] btrfs: don't set force_bio_submit in read_extent_buffer_subpage Christoph Hellwig
@ 2023-02-27 15:16 ` Christoph Hellwig
2023-02-27 15:16 ` [PATCH 03/12] btrfs: store the bio opf in struct btrfs_bio_ctrl Christoph Hellwig
` (10 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2023-02-27 15:16 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba; +Cc: linux-btrfs, Johannes Thumshirn
If force_bio_submit, submit_extent_page simply calls submit_one_bio as
the first thing. This can just be moved to the only caller that sets
force_bio_submit to true.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
fs/btrfs/extent_io.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 90842f5a978931..908a67cf5fb310 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1027,8 +1027,7 @@ 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,
- enum btrfs_compression_type compress_type,
- bool force_bio_submit)
+ enum btrfs_compression_type compress_type)
{
struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
unsigned int cur = pg_offset;
@@ -1040,9 +1039,6 @@ static int submit_extent_page(blk_opf_t opf,
ASSERT(bio_ctrl->end_io_func);
- if (force_bio_submit)
- submit_one_bio(bio_ctrl);
-
while (cur < pg_offset + size) {
u32 offset = cur - pg_offset;
int added;
@@ -1331,10 +1327,11 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
continue;
}
+ if (force_bio_submit)
+ submit_one_bio(bio_ctrl);
ret = submit_extent_page(REQ_OP_READ | read_flags, NULL,
bio_ctrl, disk_bytenr, page, iosize,
- pg_offset, this_bio_flag,
- force_bio_submit);
+ pg_offset, this_bio_flag);
if (ret) {
/*
* We have to unlock the remaining range, or the page
@@ -1645,8 +1642,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
ret = submit_extent_page(op | write_flags, wbc,
bio_ctrl, disk_bytenr,
page, iosize,
- cur - page_offset(page),
- 0, false);
+ cur - page_offset(page), 0);
if (ret) {
has_error = true;
if (!saved_ret)
@@ -2139,7 +2135,7 @@ static int write_one_subpage_eb(struct extent_buffer *eb,
ret = submit_extent_page(REQ_OP_WRITE | write_flags, wbc,
bio_ctrl, eb->start, page, eb->len,
- eb->start - page_offset(page), 0, false);
+ eb->start - page_offset(page), 0);
if (ret) {
btrfs_subpage_clear_writeback(fs_info, page, eb->start, eb->len);
set_btree_ioerr(page, eb);
@@ -2180,7 +2176,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,
bio_ctrl, disk_bytenr, p,
- PAGE_SIZE, 0, 0, false);
+ PAGE_SIZE, 0, 0);
if (ret) {
set_btree_ioerr(p, eb);
if (PageWriteback(p))
@@ -4448,7 +4444,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,
eb->start, page, eb->len,
- eb->start - page_offset(page), 0, false);
+ eb->start - page_offset(page), 0);
if (ret) {
/*
* In the endio function, if we hit something wrong we will
@@ -4558,7 +4554,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, 0, false);
+ PAGE_SIZE, 0, 0);
if (err) {
/*
* We failed to submit the bio so it's the
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 03/12] btrfs: store the bio opf in struct btrfs_bio_ctrl
2023-02-27 15:16 cleanup submit_extent_page and friends v2 Christoph Hellwig
2023-02-27 15:16 ` [PATCH 01/12] btrfs: don't set force_bio_submit in read_extent_buffer_subpage Christoph Hellwig
2023-02-27 15:16 ` [PATCH 02/12] btrfs: remove the force_bio_submit to submit_extent_page Christoph Hellwig
@ 2023-02-27 15:16 ` Christoph Hellwig
2023-02-27 15:16 ` [PATCH 04/12] btrfs: remove the sync_io flag " Christoph Hellwig
` (9 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2023-02-27 15:16 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba
Cc: linux-btrfs, Johannes Thumshirn, Qu Wenruo
The bio op and flags never change over the life time of a bio_ctrl,
so move it in there instead of passing it down the deep callchain
all the way down to alloc_new_bio.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
---
fs/btrfs/extent_io.c | 65 ++++++++++++++++++++------------------------
1 file changed, 29 insertions(+), 36 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 908a67cf5fb310..2fb23e939b9975 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -101,6 +101,7 @@ struct btrfs_bio_ctrl {
int mirror_num;
enum btrfs_compression_type compress_type;
u32 len_to_oe_boundary;
+ blk_opf_t opf;
btrfs_bio_end_io_t end_io_func;
/*
@@ -973,15 +974,15 @@ static void calc_bio_boundaries(struct btrfs_bio_ctrl *bio_ctrl,
static void alloc_new_bio(struct btrfs_inode *inode,
struct btrfs_bio_ctrl *bio_ctrl,
- struct writeback_control *wbc, blk_opf_t opf,
+ struct writeback_control *wbc,
u64 disk_bytenr, u32 offset, u64 file_offset,
enum btrfs_compression_type compress_type)
{
struct btrfs_fs_info *fs_info = inode->root->fs_info;
struct bio *bio;
- bio = btrfs_bio_alloc(BIO_MAX_VECS, opf, inode, bio_ctrl->end_io_func,
- NULL);
+ bio = btrfs_bio_alloc(BIO_MAX_VECS, bio_ctrl->opf, inode,
+ 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.
@@ -1008,7 +1009,6 @@ static void alloc_new_bio(struct btrfs_inode *inode,
}
/*
- * @opf: bio REQ_OP_* and REQ_* flags as one value
* @wbc: optional writeback control for io accounting
* @disk_bytenr: logical bytenr where the write will be
* @page: page to add to the bio
@@ -1022,8 +1022,7 @@ static void alloc_new_bio(struct btrfs_inode *inode,
* 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,
+static int submit_extent_page(struct writeback_control *wbc,
struct btrfs_bio_ctrl *bio_ctrl,
u64 disk_bytenr, struct page *page,
size_t size, unsigned long pg_offset,
@@ -1045,7 +1044,7 @@ static int submit_extent_page(blk_opf_t opf,
/* Allocate new bio if needed */
if (!bio_ctrl->bio) {
- alloc_new_bio(inode, bio_ctrl, wbc, opf, disk_bytenr,
+ alloc_new_bio(inode, bio_ctrl, wbc, disk_bytenr,
offset, page_offset(page) + cur,
compress_type);
}
@@ -1189,8 +1188,7 @@ __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
* return 0 on success, otherwise return error
*/
static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
- struct btrfs_bio_ctrl *bio_ctrl,
- blk_opf_t read_flags, u64 *prev_em_start)
+ struct btrfs_bio_ctrl *bio_ctrl, u64 *prev_em_start)
{
struct inode *inode = page->mapping->host;
struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
@@ -1329,8 +1327,7 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
if (force_bio_submit)
submit_one_bio(bio_ctrl);
- ret = submit_extent_page(REQ_OP_READ | read_flags, NULL,
- bio_ctrl, disk_bytenr, page, iosize,
+ ret = submit_extent_page(NULL, bio_ctrl, disk_bytenr, page, iosize,
pg_offset, this_bio_flag);
if (ret) {
/*
@@ -1354,12 +1351,12 @@ int btrfs_read_folio(struct file *file, struct folio *folio)
struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
u64 start = page_offset(page);
u64 end = start + PAGE_SIZE - 1;
- struct btrfs_bio_ctrl bio_ctrl = { 0 };
+ struct btrfs_bio_ctrl bio_ctrl = { .opf = REQ_OP_READ };
int ret;
btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
- ret = btrfs_do_readpage(page, NULL, &bio_ctrl, 0, NULL);
+ ret = btrfs_do_readpage(page, NULL, &bio_ctrl, NULL);
/*
* If btrfs_do_readpage() failed we will want to submit the assembled
* bio to do the cleanup.
@@ -1381,7 +1378,7 @@ static inline void contiguous_readpages(struct page *pages[], int nr_pages,
for (index = 0; index < nr_pages; index++) {
btrfs_do_readpage(pages[index], em_cached, bio_ctrl,
- REQ_RAHEAD, prev_em_start);
+ prev_em_start);
put_page(pages[index]);
}
}
@@ -1531,8 +1528,6 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
int saved_ret = 0;
int ret = 0;
int nr = 0;
- enum req_op op = REQ_OP_WRITE;
- const blk_opf_t write_flags = wbc_to_write_flags(wbc);
bool has_error = false;
bool compressed;
@@ -1639,10 +1634,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,
- bio_ctrl, disk_bytenr,
- page, iosize,
- cur - page_offset(page), 0);
+ ret = submit_extent_page(wbc, bio_ctrl, disk_bytenr, page,
+ iosize, cur - page_offset(page), 0);
if (ret) {
has_error = true;
if (!saved_ret)
@@ -2115,7 +2108,6 @@ static int write_one_subpage_eb(struct extent_buffer *eb,
{
struct btrfs_fs_info *fs_info = eb->fs_info;
struct page *page = eb->pages[0];
- blk_opf_t write_flags = wbc_to_write_flags(wbc);
bool no_dirty_ebs = false;
int ret;
@@ -2133,8 +2125,7 @@ static int write_one_subpage_eb(struct extent_buffer *eb,
bio_ctrl->end_io_func = end_bio_subpage_eb_writepage;
- ret = submit_extent_page(REQ_OP_WRITE | write_flags, wbc,
- bio_ctrl, eb->start, page, eb->len,
+ ret = submit_extent_page(wbc, bio_ctrl, eb->start, page, eb->len,
eb->start - page_offset(page), 0);
if (ret) {
btrfs_subpage_clear_writeback(fs_info, page, eb->start, eb->len);
@@ -2161,7 +2152,6 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
{
u64 disk_bytenr = eb->start;
int i, num_pages;
- blk_opf_t write_flags = wbc_to_write_flags(wbc);
int ret = 0;
prepare_eb_write(eb);
@@ -2174,8 +2164,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,
- bio_ctrl, disk_bytenr, p,
+ ret = submit_extent_page(wbc, bio_ctrl, disk_bytenr, p,
PAGE_SIZE, 0, 0);
if (ret) {
set_btree_ioerr(p, eb);
@@ -2397,6 +2386,7 @@ int btree_write_cache_pages(struct address_space *mapping,
{
struct extent_buffer *eb_context = NULL;
struct btrfs_bio_ctrl bio_ctrl = {
+ .opf = REQ_OP_WRITE | wbc_to_write_flags(wbc),
.extent_locked = 0,
.sync_io = (wbc->sync_mode == WB_SYNC_ALL),
};
@@ -2683,10 +2673,6 @@ int extent_write_locked_range(struct inode *inode, u64 start, u64 end)
u64 cur = start;
unsigned long nr_pages;
const u32 sectorsize = btrfs_sb(inode->i_sb)->sectorsize;
- struct btrfs_bio_ctrl bio_ctrl = {
- .extent_locked = 1,
- .sync_io = 1,
- };
struct writeback_control wbc_writepages = {
.sync_mode = WB_SYNC_ALL,
.range_start = start,
@@ -2695,6 +2681,11 @@ int extent_write_locked_range(struct inode *inode, u64 start, u64 end)
.punt_to_cgroup = 1,
.no_cgroup_owner = 1,
};
+ struct btrfs_bio_ctrl bio_ctrl = {
+ .opf = REQ_OP_WRITE | wbc_to_write_flags(&wbc_writepages),
+ .extent_locked = 1,
+ .sync_io = 1,
+ };
ASSERT(IS_ALIGNED(start, sectorsize) && IS_ALIGNED(end + 1, sectorsize));
nr_pages = (round_up(end, PAGE_SIZE) - round_down(start, PAGE_SIZE)) >>
@@ -2738,6 +2729,7 @@ int extent_writepages(struct address_space *mapping,
struct inode *inode = mapping->host;
int ret = 0;
struct btrfs_bio_ctrl bio_ctrl = {
+ .opf = REQ_OP_WRITE | wbc_to_write_flags(wbc),
.extent_locked = 0,
.sync_io = (wbc->sync_mode == WB_SYNC_ALL),
};
@@ -2755,7 +2747,7 @@ int extent_writepages(struct address_space *mapping,
void extent_readahead(struct readahead_control *rac)
{
- struct btrfs_bio_ctrl bio_ctrl = { 0 };
+ struct btrfs_bio_ctrl bio_ctrl = { .opf = REQ_OP_READ | REQ_RAHEAD };
struct page *pagepool[16];
struct extent_map *em_cached = NULL;
u64 prev_em_start = (u64)-1;
@@ -4402,6 +4394,7 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
struct page *page = eb->pages[0];
struct extent_state *cached_state = NULL;
struct btrfs_bio_ctrl bio_ctrl = {
+ .opf = REQ_OP_READ,
.mirror_num = mirror_num,
.parent_check = check,
};
@@ -4442,8 +4435,7 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
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,
+ ret = submit_extent_page(NULL, &bio_ctrl, eb->start, page, eb->len,
eb->start - page_offset(page), 0);
if (ret) {
/*
@@ -4478,6 +4470,7 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num,
int num_pages;
unsigned long num_reads = 0;
struct btrfs_bio_ctrl bio_ctrl = {
+ .opf = REQ_OP_READ,
.mirror_num = mirror_num,
.parent_check = check,
};
@@ -4552,9 +4545,9 @@ 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, 0);
+ err = submit_extent_page(NULL, &bio_ctrl,
+ page_offset(page), page,
+ PAGE_SIZE, 0, 0);
if (err) {
/*
* We failed to submit the bio so it's the
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 04/12] btrfs: remove the sync_io flag in struct btrfs_bio_ctrl
2023-02-27 15:16 cleanup submit_extent_page and friends v2 Christoph Hellwig
` (2 preceding siblings ...)
2023-02-27 15:16 ` [PATCH 03/12] btrfs: store the bio opf in struct btrfs_bio_ctrl Christoph Hellwig
@ 2023-02-27 15:16 ` Christoph Hellwig
2023-02-27 15:16 ` [PATCH 05/12] btrfs: add a wbc pointer to " Christoph Hellwig
` (8 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2023-02-27 15:16 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba; +Cc: linux-btrfs, Johannes Thumshirn
The sync_io flag is equivalent to wbc->sync_mode == WB_SYNC_ALL, so
just check for that and remove the separate flag.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
fs/btrfs/extent_io.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 2fb23e939b9975..432fd231935acc 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -118,9 +118,6 @@ struct btrfs_bio_ctrl {
* does the unlocking.
*/
bool extent_locked;
-
- /* Tell the submit_bio code to use REQ_SYNC */
- bool sync_io;
};
static void submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
@@ -1802,6 +1799,7 @@ static void end_extent_buffer_writeback(struct extent_buffer *eb)
* Return <0 if something went wrong, no page is locked.
*/
static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb,
+ struct writeback_control *wbc,
struct btrfs_bio_ctrl *bio_ctrl)
{
struct btrfs_fs_info *fs_info = eb->fs_info;
@@ -1817,7 +1815,7 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
if (test_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags)) {
btrfs_tree_unlock(eb);
- if (!bio_ctrl->sync_io)
+ if (wbc->sync_mode != WB_SYNC_ALL)
return 0;
if (!flush) {
submit_write_bio(bio_ctrl, 0);
@@ -2260,7 +2258,7 @@ static int submit_eb_subpage(struct page *page,
if (!eb)
continue;
- ret = lock_extent_buffer_for_io(eb, bio_ctrl);
+ ret = lock_extent_buffer_for_io(eb, wbc, bio_ctrl);
if (ret == 0) {
free_extent_buffer(eb);
continue;
@@ -2359,7 +2357,7 @@ static int submit_eb_page(struct page *page, struct writeback_control *wbc,
*eb_context = eb;
- ret = lock_extent_buffer_for_io(eb, bio_ctrl);
+ ret = lock_extent_buffer_for_io(eb, wbc, bio_ctrl);
if (ret <= 0) {
btrfs_revert_meta_write_pointer(cache, eb);
if (cache)
@@ -2388,7 +2386,6 @@ int btree_write_cache_pages(struct address_space *mapping,
struct btrfs_bio_ctrl bio_ctrl = {
.opf = REQ_OP_WRITE | wbc_to_write_flags(wbc),
.extent_locked = 0,
- .sync_io = (wbc->sync_mode == WB_SYNC_ALL),
};
struct btrfs_fs_info *fs_info = BTRFS_I(mapping->host)->root->fs_info;
int ret = 0;
@@ -2684,7 +2681,6 @@ int extent_write_locked_range(struct inode *inode, u64 start, u64 end)
struct btrfs_bio_ctrl bio_ctrl = {
.opf = REQ_OP_WRITE | wbc_to_write_flags(&wbc_writepages),
.extent_locked = 1,
- .sync_io = 1,
};
ASSERT(IS_ALIGNED(start, sectorsize) && IS_ALIGNED(end + 1, sectorsize));
@@ -2731,7 +2727,6 @@ int extent_writepages(struct address_space *mapping,
struct btrfs_bio_ctrl bio_ctrl = {
.opf = REQ_OP_WRITE | wbc_to_write_flags(wbc),
.extent_locked = 0,
- .sync_io = (wbc->sync_mode == WB_SYNC_ALL),
};
/*
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 05/12] btrfs: add a wbc pointer to struct btrfs_bio_ctrl
2023-02-27 15:16 cleanup submit_extent_page and friends v2 Christoph Hellwig
` (3 preceding siblings ...)
2023-02-27 15:16 ` [PATCH 04/12] btrfs: remove the sync_io flag " Christoph Hellwig
@ 2023-02-27 15:16 ` Christoph Hellwig
2023-02-27 15:16 ` [PATCH 06/12] btrfs: move the compress_type check out of btrfs_bio_add_page Christoph Hellwig
` (7 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2023-02-27 15:16 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba; +Cc: linux-btrfs, Johannes Thumshirn
Instead of passing down the wbc pointer the deep callchain, just
add it to the btrfs_bio_ctrl structure.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
fs/btrfs/extent_io.c | 81 ++++++++++++++++++++++----------------------
1 file changed, 40 insertions(+), 41 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 432fd231935acc..1de56abd7308c7 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -103,6 +103,7 @@ struct btrfs_bio_ctrl {
u32 len_to_oe_boundary;
blk_opf_t opf;
btrfs_bio_end_io_t end_io_func;
+ struct writeback_control *wbc;
/*
* This is for metadata read, to provide the extra needed verification
@@ -971,7 +972,6 @@ static void calc_bio_boundaries(struct btrfs_bio_ctrl *bio_ctrl,
static void alloc_new_bio(struct btrfs_inode *inode,
struct btrfs_bio_ctrl *bio_ctrl,
- struct writeback_control *wbc,
u64 disk_bytenr, u32 offset, u64 file_offset,
enum btrfs_compression_type compress_type)
{
@@ -993,7 +993,7 @@ static void alloc_new_bio(struct btrfs_inode *inode,
bio_ctrl->compress_type = compress_type;
calc_bio_boundaries(bio_ctrl, inode, file_offset);
- if (wbc) {
+ if (bio_ctrl->wbc) {
/*
* Pick the last added device to support cgroup writeback. For
* multi-device file systems this means blk-cgroup policies have
@@ -1001,12 +1001,11 @@ static void alloc_new_bio(struct btrfs_inode *inode,
* This is a bit odd but has been like that for a long time.
*/
bio_set_dev(bio, fs_info->fs_devices->latest_dev->bdev);
- wbc_init_bio(wbc, bio);
+ wbc_init_bio(bio_ctrl->wbc, bio);
}
}
/*
- * @wbc: optional writeback control for io accounting
* @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
@@ -1019,8 +1018,7 @@ static void alloc_new_bio(struct btrfs_inode *inode,
* The mirror number for this IO should already be initizlied in
* @bio_ctrl->mirror_num.
*/
-static int submit_extent_page(struct writeback_control *wbc,
- struct btrfs_bio_ctrl *bio_ctrl,
+static int submit_extent_page(struct btrfs_bio_ctrl *bio_ctrl,
u64 disk_bytenr, struct page *page,
size_t size, unsigned long pg_offset,
enum btrfs_compression_type compress_type)
@@ -1041,7 +1039,7 @@ static int submit_extent_page(struct writeback_control *wbc,
/* Allocate new bio if needed */
if (!bio_ctrl->bio) {
- alloc_new_bio(inode, bio_ctrl, wbc, disk_bytenr,
+ alloc_new_bio(inode, bio_ctrl, disk_bytenr,
offset, page_offset(page) + cur,
compress_type);
}
@@ -1063,8 +1061,8 @@ static int submit_extent_page(struct writeback_control *wbc,
ASSERT(added == 0 || added == size - offset);
/* At least we added some page, update the account */
- if (wbc && added)
- wbc_account_cgroup_owner(wbc, page, added);
+ if (bio_ctrl->wbc && added)
+ wbc_account_cgroup_owner(bio_ctrl->wbc, page, added);
/* We have reached boundary, submit right now */
if (added < size - offset) {
@@ -1324,7 +1322,7 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
if (force_bio_submit)
submit_one_bio(bio_ctrl);
- ret = submit_extent_page(NULL, bio_ctrl, disk_bytenr, page, iosize,
+ ret = submit_extent_page(bio_ctrl, disk_bytenr, page, iosize,
pg_offset, this_bio_flag);
if (ret) {
/*
@@ -1511,7 +1509,6 @@ static void find_next_dirty_byte(struct btrfs_fs_info *fs_info,
*/
static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
struct page *page,
- struct writeback_control *wbc,
struct btrfs_bio_ctrl *bio_ctrl,
loff_t i_size,
int *nr_ret)
@@ -1531,7 +1528,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
ret = btrfs_writepage_cow_fixup(page);
if (ret) {
/* Fixup worker will requeue */
- redirty_page_for_writepage(wbc, page);
+ redirty_page_for_writepage(bio_ctrl->wbc, page);
unlock_page(page);
return 1;
}
@@ -1540,7 +1537,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
* we don't want to touch the inode after unlocking the page,
* so we update the mapping writeback index now
*/
- wbc->nr_to_write--;
+ bio_ctrl->wbc->nr_to_write--;
bio_ctrl->end_io_func = end_bio_extent_writepage;
while (cur <= end) {
@@ -1631,7 +1628,7 @@ 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(wbc, bio_ctrl, disk_bytenr, page,
+ ret = submit_extent_page(bio_ctrl, disk_bytenr, page,
iosize, cur - page_offset(page), 0);
if (ret) {
has_error = true;
@@ -1668,7 +1665,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
* Return 0 if everything goes well.
* Return <0 for error.
*/
-static int __extent_writepage(struct page *page, struct writeback_control *wbc,
+static int __extent_writepage(struct page *page,
struct btrfs_bio_ctrl *bio_ctrl)
{
struct folio *folio = page_folio(page);
@@ -1682,7 +1679,7 @@ static int __extent_writepage(struct page *page, struct writeback_control *wbc,
loff_t i_size = i_size_read(inode);
unsigned long end_index = i_size >> PAGE_SHIFT;
- trace___extent_writepage(page, inode, wbc);
+ trace___extent_writepage(page, inode, bio_ctrl->wbc);
WARN_ON(!PageLocked(page));
@@ -1707,14 +1704,14 @@ static int __extent_writepage(struct page *page, struct writeback_control *wbc,
}
if (!bio_ctrl->extent_locked) {
- ret = writepage_delalloc(BTRFS_I(inode), page, wbc);
+ ret = writepage_delalloc(BTRFS_I(inode), page, bio_ctrl->wbc);
if (ret == 1)
return 0;
if (ret)
goto done;
}
- ret = __extent_writepage_io(BTRFS_I(inode), page, wbc, bio_ctrl, i_size,
+ ret = __extent_writepage_io(BTRFS_I(inode), page, bio_ctrl, i_size,
&nr);
if (ret == 1)
return 0;
@@ -1759,6 +1756,8 @@ static int __extent_writepage(struct page *page, struct writeback_control *wbc,
if (PageError(page))
end_extent_writepage(page, ret, page_start, page_end);
if (bio_ctrl->extent_locked) {
+ struct writeback_control *wbc = bio_ctrl->wbc;
+
/*
* If bio_ctrl->extent_locked, it's from extent_write_locked_range(),
* the page can either be locked by lock_page() or
@@ -1799,7 +1798,6 @@ static void end_extent_buffer_writeback(struct extent_buffer *eb)
* Return <0 if something went wrong, no page is locked.
*/
static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb,
- struct writeback_control *wbc,
struct btrfs_bio_ctrl *bio_ctrl)
{
struct btrfs_fs_info *fs_info = eb->fs_info;
@@ -1815,7 +1813,7 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
if (test_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags)) {
btrfs_tree_unlock(eb);
- if (wbc->sync_mode != WB_SYNC_ALL)
+ if (bio_ctrl->wbc->sync_mode != WB_SYNC_ALL)
return 0;
if (!flush) {
submit_write_bio(bio_ctrl, 0);
@@ -2101,7 +2099,6 @@ static void prepare_eb_write(struct extent_buffer *eb)
* Page locking is only utilized at minimum to keep the VMM code happy.
*/
static int write_one_subpage_eb(struct extent_buffer *eb,
- struct writeback_control *wbc,
struct btrfs_bio_ctrl *bio_ctrl)
{
struct btrfs_fs_info *fs_info = eb->fs_info;
@@ -2123,7 +2120,7 @@ static int write_one_subpage_eb(struct extent_buffer *eb,
bio_ctrl->end_io_func = end_bio_subpage_eb_writepage;
- ret = submit_extent_page(wbc, bio_ctrl, eb->start, page, eb->len,
+ ret = submit_extent_page(bio_ctrl, eb->start, page, eb->len,
eb->start - page_offset(page), 0);
if (ret) {
btrfs_subpage_clear_writeback(fs_info, page, eb->start, eb->len);
@@ -2140,12 +2137,11 @@ static int write_one_subpage_eb(struct extent_buffer *eb,
* dirty anymore, we have submitted a page. Update nr_written in wbc.
*/
if (no_dirty_ebs)
- wbc->nr_to_write--;
+ bio_ctrl->wbc->nr_to_write--;
return ret;
}
static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
- struct writeback_control *wbc,
struct btrfs_bio_ctrl *bio_ctrl)
{
u64 disk_bytenr = eb->start;
@@ -2162,7 +2158,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(wbc, bio_ctrl, disk_bytenr, p,
+ ret = submit_extent_page(bio_ctrl, disk_bytenr, p,
PAGE_SIZE, 0, 0);
if (ret) {
set_btree_ioerr(p, eb);
@@ -2174,7 +2170,7 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
break;
}
disk_bytenr += PAGE_SIZE;
- wbc->nr_to_write--;
+ bio_ctrl->wbc->nr_to_write--;
unlock_page(p);
}
@@ -2204,7 +2200,6 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
* Return <0 for fatal error.
*/
static int submit_eb_subpage(struct page *page,
- struct writeback_control *wbc,
struct btrfs_bio_ctrl *bio_ctrl)
{
struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
@@ -2258,7 +2253,7 @@ static int submit_eb_subpage(struct page *page,
if (!eb)
continue;
- ret = lock_extent_buffer_for_io(eb, wbc, bio_ctrl);
+ ret = lock_extent_buffer_for_io(eb, bio_ctrl);
if (ret == 0) {
free_extent_buffer(eb);
continue;
@@ -2267,7 +2262,7 @@ static int submit_eb_subpage(struct page *page,
free_extent_buffer(eb);
goto cleanup;
}
- ret = write_one_subpage_eb(eb, wbc, bio_ctrl);
+ ret = write_one_subpage_eb(eb, bio_ctrl);
free_extent_buffer(eb);
if (ret < 0)
goto cleanup;
@@ -2301,7 +2296,7 @@ static int submit_eb_subpage(struct page *page,
* previous call.
* Return <0 for fatal error.
*/
-static int submit_eb_page(struct page *page, struct writeback_control *wbc,
+static int submit_eb_page(struct page *page,
struct btrfs_bio_ctrl *bio_ctrl,
struct extent_buffer **eb_context)
{
@@ -2314,7 +2309,7 @@ static int submit_eb_page(struct page *page, struct writeback_control *wbc,
return 0;
if (btrfs_sb(page->mapping->host->i_sb)->nodesize < PAGE_SIZE)
- return submit_eb_subpage(page, wbc, bio_ctrl);
+ return submit_eb_subpage(page, bio_ctrl);
spin_lock(&mapping->private_lock);
if (!PagePrivate(page)) {
@@ -2347,7 +2342,8 @@ static int submit_eb_page(struct page *page, struct writeback_control *wbc,
* If for_sync, this hole will be filled with
* trasnsaction commit.
*/
- if (wbc->sync_mode == WB_SYNC_ALL && !wbc->for_sync)
+ if (bio_ctrl->wbc->sync_mode == WB_SYNC_ALL &&
+ !bio_ctrl->wbc->for_sync)
ret = -EAGAIN;
else
ret = 0;
@@ -2357,7 +2353,7 @@ static int submit_eb_page(struct page *page, struct writeback_control *wbc,
*eb_context = eb;
- ret = lock_extent_buffer_for_io(eb, wbc, bio_ctrl);
+ ret = lock_extent_buffer_for_io(eb, bio_ctrl);
if (ret <= 0) {
btrfs_revert_meta_write_pointer(cache, eb);
if (cache)
@@ -2372,7 +2368,7 @@ static int submit_eb_page(struct page *page, struct writeback_control *wbc,
btrfs_schedule_zone_finish_bg(cache, eb);
btrfs_put_block_group(cache);
}
- ret = write_one_eb(eb, wbc, bio_ctrl);
+ ret = write_one_eb(eb, bio_ctrl);
free_extent_buffer(eb);
if (ret < 0)
return ret;
@@ -2384,6 +2380,7 @@ int btree_write_cache_pages(struct address_space *mapping,
{
struct extent_buffer *eb_context = NULL;
struct btrfs_bio_ctrl bio_ctrl = {
+ .wbc = wbc,
.opf = REQ_OP_WRITE | wbc_to_write_flags(wbc),
.extent_locked = 0,
};
@@ -2428,7 +2425,7 @@ int btree_write_cache_pages(struct address_space *mapping,
for (i = 0; i < nr_pages; i++) {
struct page *page = pvec.pages[i];
- ret = submit_eb_page(page, wbc, &bio_ctrl, &eb_context);
+ ret = submit_eb_page(page, &bio_ctrl, &eb_context);
if (ret == 0)
continue;
if (ret < 0) {
@@ -2511,9 +2508,9 @@ int btree_write_cache_pages(struct address_space *mapping,
* existing IO to complete.
*/
static int extent_write_cache_pages(struct address_space *mapping,
- struct writeback_control *wbc,
struct btrfs_bio_ctrl *bio_ctrl)
{
+ struct writeback_control *wbc = bio_ctrl->wbc;
struct inode *inode = mapping->host;
int ret = 0;
int done = 0;
@@ -2614,7 +2611,7 @@ static int extent_write_cache_pages(struct address_space *mapping,
continue;
}
- ret = __extent_writepage(page, wbc, bio_ctrl);
+ ret = __extent_writepage(page, bio_ctrl);
if (ret < 0) {
done = 1;
break;
@@ -2679,6 +2676,7 @@ int extent_write_locked_range(struct inode *inode, u64 start, u64 end)
.no_cgroup_owner = 1,
};
struct btrfs_bio_ctrl bio_ctrl = {
+ .wbc = &wbc_writepages,
.opf = REQ_OP_WRITE | wbc_to_write_flags(&wbc_writepages),
.extent_locked = 1,
};
@@ -2701,7 +2699,7 @@ int extent_write_locked_range(struct inode *inode, u64 start, u64 end)
ASSERT(PageLocked(page));
ASSERT(PageDirty(page));
clear_page_dirty_for_io(page);
- ret = __extent_writepage(page, &wbc_writepages, &bio_ctrl);
+ ret = __extent_writepage(page, &bio_ctrl);
ASSERT(ret <= 0);
if (ret < 0) {
found_error = true;
@@ -2725,6 +2723,7 @@ int extent_writepages(struct address_space *mapping,
struct inode *inode = mapping->host;
int ret = 0;
struct btrfs_bio_ctrl bio_ctrl = {
+ .wbc = wbc,
.opf = REQ_OP_WRITE | wbc_to_write_flags(wbc),
.extent_locked = 0,
};
@@ -2734,7 +2733,7 @@ int extent_writepages(struct address_space *mapping,
* protect the write pointer updates.
*/
btrfs_zoned_data_reloc_lock(BTRFS_I(inode));
- ret = extent_write_cache_pages(mapping, wbc, &bio_ctrl);
+ ret = extent_write_cache_pages(mapping, &bio_ctrl);
submit_write_bio(&bio_ctrl, ret);
btrfs_zoned_data_reloc_unlock(BTRFS_I(inode));
return ret;
@@ -4430,7 +4429,7 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
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(NULL, &bio_ctrl, eb->start, page, eb->len,
+ ret = submit_extent_page(&bio_ctrl, eb->start, page, eb->len,
eb->start - page_offset(page), 0);
if (ret) {
/*
@@ -4540,7 +4539,7 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num,
}
ClearPageError(page);
- err = submit_extent_page(NULL, &bio_ctrl,
+ err = submit_extent_page(&bio_ctrl,
page_offset(page), page,
PAGE_SIZE, 0, 0);
if (err) {
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 06/12] btrfs: move the compress_type check out of btrfs_bio_add_page
2023-02-27 15:16 cleanup submit_extent_page and friends v2 Christoph Hellwig
` (4 preceding siblings ...)
2023-02-27 15:16 ` [PATCH 05/12] btrfs: add a wbc pointer to " Christoph Hellwig
@ 2023-02-27 15:16 ` Christoph Hellwig
2023-02-27 16:20 ` Johannes Thumshirn
2023-02-27 15:16 ` [PATCH 07/12] btrfs: rename the this_bio_flag variable in btrfs_do_readpage Christoph Hellwig
` (6 subsequent siblings)
12 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2023-02-27 15:16 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba; +Cc: linux-btrfs
The compress_type can only change on a per-extent basis. So instead of
checking it for every page in btrfs_bio_add_page, do the check once in
btrfs_do_readpage, which is the only caller of btrfs_bio_add_page and
submit_extent_page that deals with compressed extents.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/btrfs/extent_io.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 1de56abd7308c7..eac31f212069a0 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -875,7 +875,6 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array)
* a contiguous page to the previous one
* @size: portion of page that we want to write
* @pg_offset: starting offset in the page
- * @compress_type: compression type of the current bio to see if we can merge them
*
* Attempt to add a page to bio considering stripe alignment etc.
*
@@ -886,8 +885,7 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array)
static int btrfs_bio_add_page(struct btrfs_bio_ctrl *bio_ctrl,
struct page *page,
u64 disk_bytenr, unsigned int size,
- unsigned int pg_offset,
- enum btrfs_compression_type compress_type)
+ unsigned int pg_offset)
{
struct bio *bio = bio_ctrl->bio;
u32 bio_size = bio->bi_iter.bi_size;
@@ -898,9 +896,6 @@ static int btrfs_bio_add_page(struct btrfs_bio_ctrl *bio_ctrl,
ASSERT(bio);
/* The limit should be calculated when bio_ctrl->bio is allocated */
ASSERT(bio_ctrl->len_to_oe_boundary);
- if (bio_ctrl->compress_type != compress_type)
- return 0;
-
if (bio->bi_iter.bi_size == 0) {
/* We can always add a page into an empty bio. */
@@ -1049,12 +1044,11 @@ static int submit_extent_page(struct btrfs_bio_ctrl *bio_ctrl,
*/
if (compress_type != BTRFS_COMPRESS_NONE)
added = btrfs_bio_add_page(bio_ctrl, page, disk_bytenr,
- size - offset, pg_offset + offset,
- compress_type);
+ size - offset, pg_offset + offset);
else
added = btrfs_bio_add_page(bio_ctrl, page,
disk_bytenr + offset, size - offset,
- pg_offset + offset, compress_type);
+ pg_offset + offset);
/* Metadata page range should never be split */
if (!is_data_inode(&inode->vfs_inode))
@@ -1320,6 +1314,9 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
continue;
}
+ if (bio_ctrl->compress_type != this_bio_flag)
+ submit_one_bio(bio_ctrl);
+
if (force_bio_submit)
submit_one_bio(bio_ctrl);
ret = submit_extent_page(bio_ctrl, disk_bytenr, page, iosize,
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 06/12] btrfs: move the compress_type check out of btrfs_bio_add_page
2023-02-27 15:16 ` [PATCH 06/12] btrfs: move the compress_type check out of btrfs_bio_add_page Christoph Hellwig
@ 2023-02-27 16:20 ` Johannes Thumshirn
2023-02-28 2:58 ` Christoph Hellwig
0 siblings, 1 reply; 21+ messages in thread
From: Johannes Thumshirn @ 2023-02-27 16:20 UTC (permalink / raw)
To: Christoph Hellwig, Chris Mason, Josef Bacik, David Sterba
Cc: linux-btrfs@vger.kernel.org
On 27.02.23 16:17, Christoph Hellwig wrote:
> + if (bio_ctrl->compress_type != this_bio_flag)
> + submit_one_bio(bio_ctrl);
> +
> if (force_bio_submit)
> submit_one_bio(bio_ctrl);
Sorry for not having noticed this earlier. But this looks odd TBH.
if (bio_ctrl->compress_type != this_bio_flag ||
force_bio_submit)
submit_one_bio(bio_ctrl);
looks better IMHO. Or as it changes a bit in 8/12 maybe:
if (bio_ctrl->compress_type != this_bio_flag)
force_bio_submit = true;
if (force_bio_submit)
submit_one_bio(bio_ctrl);
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 06/12] btrfs: move the compress_type check out of btrfs_bio_add_page
2023-02-27 16:20 ` Johannes Thumshirn
@ 2023-02-28 2:58 ` Christoph Hellwig
2023-02-28 9:02 ` Johannes Thumshirn
0 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2023-02-28 2:58 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: Christoph Hellwig, Chris Mason, Josef Bacik, David Sterba,
linux-btrfs@vger.kernel.org
On Mon, Feb 27, 2023 at 04:20:54PM +0000, Johannes Thumshirn wrote:
> On 27.02.23 16:17, Christoph Hellwig wrote:
> > + if (bio_ctrl->compress_type != this_bio_flag)
> > + submit_one_bio(bio_ctrl);
> > +
> > if (force_bio_submit)
> > submit_one_bio(bio_ctrl);
>
>
> Sorry for not having noticed this earlier. But this looks odd TBH.
>
> if (bio_ctrl->compress_type != this_bio_flag ||
> force_bio_submit)
> submit_one_bio(bio_ctrl);
It does. But with patch 8 it would stop working, as we must
uptdate the compress_type field after submitting the bio for that case.
Been there, done that and it broke :)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 06/12] btrfs: move the compress_type check out of btrfs_bio_add_page
2023-02-28 2:58 ` Christoph Hellwig
@ 2023-02-28 9:02 ` Johannes Thumshirn
2023-02-28 13:13 ` Christoph Hellwig
0 siblings, 1 reply; 21+ messages in thread
From: Johannes Thumshirn @ 2023-02-28 9:02 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Chris Mason, Josef Bacik, David Sterba,
linux-btrfs@vger.kernel.org
On 28.02.23 03:58, Christoph Hellwig wrote:
> On Mon, Feb 27, 2023 at 04:20:54PM +0000, Johannes Thumshirn wrote:
>> On 27.02.23 16:17, Christoph Hellwig wrote:
>>> + if (bio_ctrl->compress_type != this_bio_flag)
>>> + submit_one_bio(bio_ctrl);
>>> +
>>> if (force_bio_submit)
>>> submit_one_bio(bio_ctrl);
>>
>>
>> Sorry for not having noticed this earlier. But this looks odd TBH.
>>
>> if (bio_ctrl->compress_type != this_bio_flag ||
>> force_bio_submit)
>> submit_one_bio(bio_ctrl);
>
> It does. But with patch 8 it would stop working, as we must
> uptdate the compress_type field after submitting the bio for that case.
>
> Been there, done that and it broke :)
>
Sh*t, then at least let's document that with a comment. Otherwise someone
will see the code and try to clean it up and possibly break it.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 07/12] btrfs: rename the this_bio_flag variable in btrfs_do_readpage
2023-02-27 15:16 cleanup submit_extent_page and friends v2 Christoph Hellwig
` (5 preceding siblings ...)
2023-02-27 15:16 ` [PATCH 06/12] btrfs: move the compress_type check out of btrfs_bio_add_page Christoph Hellwig
@ 2023-02-27 15:16 ` Christoph Hellwig
2023-02-27 15:17 ` [PATCH 08/12] btrfs: remove the compress_type argument to submit_extent_page Christoph Hellwig
` (5 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2023-02-27 15:16 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba
Cc: linux-btrfs, Johannes Thumshirn, Qu Wenruo
Rename this_bio_flag to compress_type to match the surrounding code
and better document the intent. Also use the proper enum type instead
of unsigned long.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
---
fs/btrfs/extent_io.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index eac31f212069a0..b54e1911c476f8 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1213,7 +1213,7 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
bio_ctrl->end_io_func = end_bio_extent_readpage;
begin_page_read(fs_info, page);
while (cur <= end) {
- unsigned long this_bio_flag = 0;
+ enum btrfs_compression_type compress_type = BTRFS_COMPRESS_NONE;
bool force_bio_submit = false;
u64 disk_bytenr;
@@ -1238,11 +1238,11 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
BUG_ON(end < cur);
if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags))
- this_bio_flag = em->compress_type;
+ compress_type = em->compress_type;
iosize = min(extent_map_end(em) - cur, end - cur + 1);
iosize = ALIGN(iosize, blocksize);
- if (this_bio_flag != BTRFS_COMPRESS_NONE)
+ if (compress_type != BTRFS_COMPRESS_NONE)
disk_bytenr = em->block_start;
else
disk_bytenr = em->block_start + extent_offset;
@@ -1314,13 +1314,13 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
continue;
}
- if (bio_ctrl->compress_type != this_bio_flag)
+ if (bio_ctrl->compress_type != compress_type)
submit_one_bio(bio_ctrl);
if (force_bio_submit)
submit_one_bio(bio_ctrl);
ret = submit_extent_page(bio_ctrl, disk_bytenr, page, iosize,
- pg_offset, this_bio_flag);
+ pg_offset, compress_type);
if (ret) {
/*
* We have to unlock the remaining range, or the page
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 08/12] btrfs: remove the compress_type argument to submit_extent_page
2023-02-27 15:16 cleanup submit_extent_page and friends v2 Christoph Hellwig
` (6 preceding siblings ...)
2023-02-27 15:16 ` [PATCH 07/12] btrfs: rename the this_bio_flag variable in btrfs_do_readpage Christoph Hellwig
@ 2023-02-27 15:17 ` Christoph Hellwig
2023-02-27 15:17 ` [PATCH 09/12] btrfs: remove the submit_extent_page return value Christoph Hellwig
` (4 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2023-02-27 15:17 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba; +Cc: linux-btrfs, Johannes Thumshirn
Update the compress_type in the btrfs_bio_ctrl after forcing out the
previous bio in btrfs_do_readpage, so that alloc_new_bio can just use
the compress_type member in struct btrfs_bio_ctrl instead of passing the
same information redudantly as a function argument.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
fs/btrfs/extent_io.c | 31 ++++++++++++++-----------------
1 file changed, 14 insertions(+), 17 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index b54e1911c476f8..5ab4d401505891 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -967,8 +967,7 @@ static void calc_bio_boundaries(struct btrfs_bio_ctrl *bio_ctrl,
static void alloc_new_bio(struct btrfs_inode *inode,
struct btrfs_bio_ctrl *bio_ctrl,
- u64 disk_bytenr, u32 offset, u64 file_offset,
- enum btrfs_compression_type compress_type)
+ u64 disk_bytenr, u32 offset, u64 file_offset)
{
struct btrfs_fs_info *fs_info = inode->root->fs_info;
struct bio *bio;
@@ -979,13 +978,12 @@ static void alloc_new_bio(struct btrfs_inode *inode,
* For compressed page range, its disk_bytenr is always @disk_bytenr
* passed in, no matter if we have added any range into previous bio.
*/
- if (compress_type != BTRFS_COMPRESS_NONE)
+ if (bio_ctrl->compress_type != BTRFS_COMPRESS_NONE)
bio->bi_iter.bi_sector = disk_bytenr >> SECTOR_SHIFT;
else
bio->bi_iter.bi_sector = (disk_bytenr + offset) >> SECTOR_SHIFT;
btrfs_bio(bio)->file_offset = file_offset;
bio_ctrl->bio = bio;
- bio_ctrl->compress_type = compress_type;
calc_bio_boundaries(bio_ctrl, inode, file_offset);
if (bio_ctrl->wbc) {
@@ -1006,7 +1004,6 @@ static void 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
- * @compress_type: compress type for current bio
*
* The will either add the page into the existing @bio_ctrl->bio, or allocate a
* new one in @bio_ctrl->bio.
@@ -1015,8 +1012,7 @@ static void alloc_new_bio(struct btrfs_inode *inode,
*/
static int submit_extent_page(struct btrfs_bio_ctrl *bio_ctrl,
u64 disk_bytenr, struct page *page,
- size_t size, unsigned long pg_offset,
- enum btrfs_compression_type compress_type)
+ size_t size, unsigned long pg_offset)
{
struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
unsigned int cur = pg_offset;
@@ -1035,14 +1031,13 @@ static int submit_extent_page(struct btrfs_bio_ctrl *bio_ctrl,
/* Allocate new bio if needed */
if (!bio_ctrl->bio) {
alloc_new_bio(inode, bio_ctrl, disk_bytenr,
- offset, page_offset(page) + cur,
- compress_type);
+ offset, page_offset(page) + cur);
}
/*
* We must go through btrfs_bio_add_page() to ensure each
* page range won't cross various boundaries.
*/
- if (compress_type != BTRFS_COMPRESS_NONE)
+ if (bio_ctrl->compress_type != BTRFS_COMPRESS_NONE)
added = btrfs_bio_add_page(bio_ctrl, page, disk_bytenr,
size - offset, pg_offset + offset);
else
@@ -1314,13 +1309,15 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
continue;
}
- if (bio_ctrl->compress_type != compress_type)
+ if (bio_ctrl->compress_type != compress_type) {
submit_one_bio(bio_ctrl);
+ bio_ctrl->compress_type = compress_type;
+ }
if (force_bio_submit)
submit_one_bio(bio_ctrl);
ret = submit_extent_page(bio_ctrl, disk_bytenr, page, iosize,
- pg_offset, compress_type);
+ pg_offset);
if (ret) {
/*
* We have to unlock the remaining range, or the page
@@ -1626,7 +1623,7 @@ 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(bio_ctrl, disk_bytenr, page,
- iosize, cur - page_offset(page), 0);
+ iosize, cur - page_offset(page));
if (ret) {
has_error = true;
if (!saved_ret)
@@ -2118,7 +2115,7 @@ static int write_one_subpage_eb(struct extent_buffer *eb,
bio_ctrl->end_io_func = end_bio_subpage_eb_writepage;
ret = submit_extent_page(bio_ctrl, eb->start, page, eb->len,
- eb->start - page_offset(page), 0);
+ eb->start - page_offset(page));
if (ret) {
btrfs_subpage_clear_writeback(fs_info, page, eb->start, eb->len);
set_btree_ioerr(page, eb);
@@ -2156,7 +2153,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(bio_ctrl, disk_bytenr, p,
- PAGE_SIZE, 0, 0);
+ PAGE_SIZE, 0);
if (ret) {
set_btree_ioerr(p, eb);
if (PageWriteback(p))
@@ -4427,7 +4424,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(&bio_ctrl, eb->start, page, eb->len,
- eb->start - page_offset(page), 0);
+ eb->start - page_offset(page));
if (ret) {
/*
* In the endio function, if we hit something wrong we will
@@ -4538,7 +4535,7 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num,
ClearPageError(page);
err = submit_extent_page(&bio_ctrl,
page_offset(page), page,
- PAGE_SIZE, 0, 0);
+ PAGE_SIZE, 0);
if (err) {
/*
* We failed to submit the bio so it's the
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 09/12] btrfs: remove the submit_extent_page return value
2023-02-27 15:16 cleanup submit_extent_page and friends v2 Christoph Hellwig
` (7 preceding siblings ...)
2023-02-27 15:17 ` [PATCH 08/12] btrfs: remove the compress_type argument to submit_extent_page Christoph Hellwig
@ 2023-02-27 15:17 ` Christoph Hellwig
2023-02-27 15:17 ` [PATCH 10/12] btrfs: simplify the error handling in __extent_writepage_io Christoph Hellwig
` (3 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2023-02-27 15:17 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba; +Cc: linux-btrfs, Johannes Thumshirn
submit_extent_page always returns 0 since commit d5e4377d5051 ("btrfs:
split zone append bios in btrfs_submit_bio"). Change it to a void return
type and remove all the unreachable error handling code in the callers.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
fs/btrfs/extent_io.c | 156 ++++++++++---------------------------------
1 file changed, 35 insertions(+), 121 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 5ab4d401505891..b4d986bef2631f 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1010,9 +1010,9 @@ static void alloc_new_bio(struct btrfs_inode *inode,
* The mirror number for this IO should already be initizlied in
* @bio_ctrl->mirror_num.
*/
-static int submit_extent_page(struct btrfs_bio_ctrl *bio_ctrl,
- u64 disk_bytenr, struct page *page,
- size_t size, unsigned long pg_offset)
+static void submit_extent_page(struct btrfs_bio_ctrl *bio_ctrl,
+ u64 disk_bytenr, struct page *page,
+ size_t size, unsigned long pg_offset)
{
struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
unsigned int cur = pg_offset;
@@ -1061,7 +1061,6 @@ static int submit_extent_page(struct btrfs_bio_ctrl *bio_ctrl,
}
cur += added;
}
- return 0;
}
static int attach_extent_buffer_page(struct extent_buffer *eb,
@@ -1194,7 +1193,7 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
unlock_extent(tree, start, end, NULL);
btrfs_page_set_error(fs_info, page, start, PAGE_SIZE);
unlock_page(page);
- goto out;
+ return ret;
}
if (page->index == last_byte >> PAGE_SHIFT) {
@@ -1225,8 +1224,7 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
if (IS_ERR(em)) {
unlock_extent(tree, cur, end, NULL);
end_page_read(page, false, cur, end + 1 - cur);
- ret = PTR_ERR(em);
- break;
+ return PTR_ERR(em);
}
extent_offset = cur - em->start;
BUG_ON(extent_map_end(em) <= cur);
@@ -1316,22 +1314,13 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
if (force_bio_submit)
submit_one_bio(bio_ctrl);
- ret = submit_extent_page(bio_ctrl, disk_bytenr, page, iosize,
- pg_offset);
- if (ret) {
- /*
- * We have to unlock the remaining range, or the page
- * will never be unlocked.
- */
- unlock_extent(tree, cur, end, NULL);
- end_page_read(page, false, cur, end + 1 - cur);
- goto out;
- }
+ submit_extent_page(bio_ctrl, disk_bytenr, page, iosize,
+ pg_offset);
cur = cur + iosize;
pg_offset += iosize;
}
-out:
- return ret;
+
+ return 0;
}
int btrfs_read_folio(struct file *file, struct folio *folio)
@@ -1622,19 +1611,9 @@ 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(bio_ctrl, disk_bytenr, page,
- iosize, cur - page_offset(page));
- if (ret) {
- has_error = true;
- if (!saved_ret)
- saved_ret = ret;
-
- btrfs_page_set_error(fs_info, page, cur, iosize);
- if (PageWriteback(page))
- btrfs_page_clear_writeback(fs_info, page, cur,
- iosize);
- }
-
+ submit_extent_page(bio_ctrl, disk_bytenr, page, iosize,
+ cur - page_offset(page));
+ ret = 0;
cur += iosize;
nr++;
}
@@ -2092,13 +2071,12 @@ static void prepare_eb_write(struct extent_buffer *eb)
* Unlike the work in write_one_eb(), we rely completely on extent locking.
* Page locking is only utilized at minimum to keep the VMM code happy.
*/
-static int write_one_subpage_eb(struct extent_buffer *eb,
- struct btrfs_bio_ctrl *bio_ctrl)
+static void write_one_subpage_eb(struct extent_buffer *eb,
+ struct btrfs_bio_ctrl *bio_ctrl)
{
struct btrfs_fs_info *fs_info = eb->fs_info;
struct page *page = eb->pages[0];
bool no_dirty_ebs = false;
- int ret;
prepare_eb_write(eb);
@@ -2114,17 +2092,8 @@ static int write_one_subpage_eb(struct extent_buffer *eb,
bio_ctrl->end_io_func = end_bio_subpage_eb_writepage;
- ret = submit_extent_page(bio_ctrl, eb->start, page, eb->len,
- eb->start - page_offset(page));
- if (ret) {
- btrfs_subpage_clear_writeback(fs_info, page, eb->start, eb->len);
- set_btree_ioerr(page, eb);
- unlock_page(page);
-
- if (atomic_dec_and_test(&eb->io_pages))
- end_extent_buffer_writeback(eb);
- return -EIO;
- }
+ submit_extent_page(bio_ctrl, eb->start, page, eb->len,
+ eb->start - page_offset(page));
unlock_page(page);
/*
* Submission finished without problem, if no range of the page is
@@ -2132,15 +2101,13 @@ static int write_one_subpage_eb(struct extent_buffer *eb,
*/
if (no_dirty_ebs)
bio_ctrl->wbc->nr_to_write--;
- return ret;
}
-static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
+static noinline_for_stack void write_one_eb(struct extent_buffer *eb,
struct btrfs_bio_ctrl *bio_ctrl)
{
u64 disk_bytenr = eb->start;
int i, num_pages;
- int ret = 0;
prepare_eb_write(eb);
@@ -2152,31 +2119,11 @@ 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(bio_ctrl, disk_bytenr, p,
- PAGE_SIZE, 0);
- if (ret) {
- set_btree_ioerr(p, eb);
- if (PageWriteback(p))
- end_page_writeback(p);
- if (atomic_sub_and_test(num_pages - i, &eb->io_pages))
- end_extent_buffer_writeback(eb);
- ret = -EIO;
- break;
- }
+ submit_extent_page(bio_ctrl, disk_bytenr, p, PAGE_SIZE, 0);
disk_bytenr += PAGE_SIZE;
bio_ctrl->wbc->nr_to_write--;
unlock_page(p);
}
-
- if (unlikely(ret)) {
- for (; i < num_pages; i++) {
- struct page *p = eb->pages[i];
- clear_page_dirty_for_io(p);
- unlock_page(p);
- }
- }
-
- return ret;
}
/*
@@ -2256,10 +2203,8 @@ static int submit_eb_subpage(struct page *page,
free_extent_buffer(eb);
goto cleanup;
}
- ret = write_one_subpage_eb(eb, bio_ctrl);
+ write_one_subpage_eb(eb, bio_ctrl);
free_extent_buffer(eb);
- if (ret < 0)
- goto cleanup;
submitted++;
}
return submitted;
@@ -2362,10 +2307,8 @@ static int submit_eb_page(struct page *page,
btrfs_schedule_zone_finish_bg(cache, eb);
btrfs_put_block_group(cache);
}
- ret = write_one_eb(eb, bio_ctrl);
+ write_one_eb(eb, bio_ctrl);
free_extent_buffer(eb);
- if (ret < 0)
- return ret;
return 1;
}
@@ -4386,7 +4329,7 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
.mirror_num = mirror_num,
.parent_check = check,
};
- int ret = 0;
+ int ret;
ASSERT(!test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags));
ASSERT(PagePrivate(page));
@@ -4404,14 +4347,13 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
return ret;
}
- ret = 0;
if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags) ||
PageUptodate(page) ||
btrfs_subpage_test_uptodate(fs_info, page, eb->start, eb->len)) {
set_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags);
unlock_extent(io_tree, eb->start, eb->start + eb->len - 1,
&cached_state);
- return ret;
+ return 0;
}
clear_bit(EXTENT_BUFFER_READ_ERR, &eb->bflags);
@@ -4423,27 +4365,19 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
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(&bio_ctrl, eb->start, page, eb->len,
- eb->start - page_offset(page));
- if (ret) {
- /*
- * In the endio function, if we hit something wrong we will
- * increase the io_pages, so here we need to decrease it for
- * error path.
- */
- atomic_dec(&eb->io_pages);
- }
+ submit_extent_page(&bio_ctrl, eb->start, page, eb->len,
+ eb->start - page_offset(page));
submit_one_bio(&bio_ctrl);
- if (ret || wait != WAIT_COMPLETE) {
+ if (wait != WAIT_COMPLETE) {
free_extent_state(cached_state);
- return ret;
+ return 0;
}
wait_extent_bit(io_tree, eb->start, eb->start + eb->len - 1,
EXTENT_LOCKED, &cached_state);
if (!test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags))
- ret = -EIO;
- return ret;
+ return -EIO;
+ return 0;
}
int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num,
@@ -4451,8 +4385,6 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num,
{
int i;
struct page *page;
- int err;
- int ret = 0;
int locked_pages = 0;
int all_uptodate = 1;
int num_pages;
@@ -4526,27 +4458,9 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num,
page = eb->pages[i];
if (!PageUptodate(page)) {
- if (ret) {
- atomic_dec(&eb->io_pages);
- unlock_page(page);
- continue;
- }
-
ClearPageError(page);
- err = submit_extent_page(&bio_ctrl,
- page_offset(page), page,
- PAGE_SIZE, 0);
- if (err) {
- /*
- * We failed to submit the bio so it's the
- * caller's responsibility to perform cleanup
- * i.e unlock page/set error bit.
- */
- ret = err;
- SetPageError(page);
- unlock_page(page);
- atomic_dec(&eb->io_pages);
- }
+ submit_extent_page(&bio_ctrl, page_offset(page), page,
+ PAGE_SIZE, 0);
} else {
unlock_page(page);
}
@@ -4554,17 +4468,17 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num,
submit_one_bio(&bio_ctrl);
- if (ret || wait != WAIT_COMPLETE)
- return ret;
+ if (wait != WAIT_COMPLETE)
+ return 0;
for (i = 0; i < num_pages; i++) {
page = eb->pages[i];
wait_on_page_locked(page);
if (!PageUptodate(page))
- ret = -EIO;
+ return -EIO;
}
- return ret;
+ return 0;
unlock_exit:
while (locked_pages > 0) {
@@ -4572,7 +4486,7 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num,
page = eb->pages[locked_pages];
unlock_page(page);
}
- return ret;
+ return 0;
}
static bool report_eb_range(const struct extent_buffer *eb, unsigned long start,
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 10/12] btrfs: simplify the error handling in __extent_writepage_io
2023-02-27 15:16 cleanup submit_extent_page and friends v2 Christoph Hellwig
` (8 preceding siblings ...)
2023-02-27 15:17 ` [PATCH 09/12] btrfs: remove the submit_extent_page return value Christoph Hellwig
@ 2023-02-27 15:17 ` Christoph Hellwig
2023-02-27 15:17 ` [PATCH 11/12] btrfs: check for contiguity in submit_extent_page Christoph Hellwig
` (2 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2023-02-27 15:17 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba; +Cc: linux-btrfs, Johannes Thumshirn
Remove the has_error and saved_ret variables, and just jump to
a goto label for error handling from the only place returning
an error from the main loop.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
fs/btrfs/extent_io.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index b4d986bef2631f..d318b7f1b92cf3 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1502,10 +1502,8 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
u64 extent_offset;
u64 block_start;
struct extent_map *em;
- int saved_ret = 0;
int ret = 0;
int nr = 0;
- bool has_error = false;
bool compressed;
ret = btrfs_writepage_cow_fixup(page);
@@ -1556,10 +1554,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
if (IS_ERR(em)) {
btrfs_page_set_error(fs_info, page, cur, end - cur + 1);
ret = PTR_ERR_OR_ZERO(em);
- has_error = true;
- if (!saved_ret)
- saved_ret = ret;
- break;
+ goto out_error;
}
extent_offset = cur - em->start;
@@ -1613,18 +1608,19 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
submit_extent_page(bio_ctrl, disk_bytenr, page, iosize,
cur - page_offset(page));
- ret = 0;
cur += iosize;
nr++;
}
+
+ btrfs_page_assert_not_dirty(fs_info, page);
+ *nr_ret = nr;
+ return 0;
+
+out_error:
/*
* If we finish without problem, we should not only clear page dirty,
* but also empty subpage dirty bits
*/
- if (!has_error)
- btrfs_page_assert_not_dirty(fs_info, page);
- else
- ret = saved_ret;
*nr_ret = nr;
return ret;
}
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 11/12] btrfs: check for contiguity in submit_extent_page
2023-02-27 15:16 cleanup submit_extent_page and friends v2 Christoph Hellwig
` (9 preceding siblings ...)
2023-02-27 15:17 ` [PATCH 10/12] btrfs: simplify the error handling in __extent_writepage_io Christoph Hellwig
@ 2023-02-27 15:17 ` Christoph Hellwig
2023-02-27 15:17 ` [PATCH 12/12] btrfs: simplify submit_extent_page Christoph Hellwig
2023-02-27 21:21 ` cleanup submit_extent_page and friends v2 David Sterba
12 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2023-02-27 15:17 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba; +Cc: linux-btrfs, Johannes Thumshirn
Different loop iterations in btrfs_bio_add_page not only have
the same contiguity parameters, but also any non-initial operation
operates on a fresh bio anyway.
Factor out the contiguity check into a new btrfs_bio_is_contig and
only call it once in submit_extent_page before descending into the
bio_add_page loop.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
fs/btrfs/extent_io.c | 68 +++++++++++++++++++++++---------------------
1 file changed, 35 insertions(+), 33 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index d318b7f1b92cf3..8e6f0f07597e45 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -866,6 +866,37 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array)
return 0;
}
+static bool btrfs_bio_is_contig(struct btrfs_bio_ctrl *bio_ctrl,
+ struct page *page, u64 disk_bytenr,
+ unsigned int pg_offset)
+{
+ struct bio *bio = bio_ctrl->bio;
+ struct bio_vec *bvec = bio_last_bvec_all(bio);
+ const sector_t sector = disk_bytenr >> SECTOR_SHIFT;
+
+ if (bio_ctrl->compress_type != BTRFS_COMPRESS_NONE) {
+ /*
+ * For compression, all IO should have its logical bytenr
+ * set to the starting bytenr of the compressed extent.
+ */
+ return bio->bi_iter.bi_sector == sector;
+ }
+
+ /*
+ * The contig check requires the following conditions to be met:
+ * 1) The pages are belonging to the same inode
+ * This is implied by the call chain.
+ *
+ * 2) The range has adjacent logical bytenr
+ *
+ * 3) The range has adjacent file offset
+ * This is required for the usage of btrfs_bio->file_offset.
+ */
+ return bio_end_sector(bio) == sector &&
+ page_offset(bvec->bv_page) + bvec->bv_offset + bvec->bv_len ==
+ page_offset(page) + pg_offset;
+}
+
/*
* Attempt to add a page to bio.
*
@@ -890,44 +921,11 @@ static int btrfs_bio_add_page(struct btrfs_bio_ctrl *bio_ctrl,
struct bio *bio = bio_ctrl->bio;
u32 bio_size = bio->bi_iter.bi_size;
u32 real_size;
- const sector_t sector = disk_bytenr >> SECTOR_SHIFT;
- bool contig = false;
ASSERT(bio);
/* The limit should be calculated when bio_ctrl->bio is allocated */
ASSERT(bio_ctrl->len_to_oe_boundary);
- if (bio->bi_iter.bi_size == 0) {
- /* We can always add a page into an empty bio. */
- contig = true;
- } else if (bio_ctrl->compress_type == BTRFS_COMPRESS_NONE) {
- struct bio_vec *bvec = bio_last_bvec_all(bio);
-
- /*
- * The contig check requires the following conditions to be met:
- * 1) The pages are belonging to the same inode
- * This is implied by the call chain.
- *
- * 2) The range has adjacent logical bytenr
- *
- * 3) The range has adjacent file offset
- * This is required for the usage of btrfs_bio->file_offset.
- */
- if (bio_end_sector(bio) == sector &&
- page_offset(bvec->bv_page) + bvec->bv_offset +
- bvec->bv_len == page_offset(page) + pg_offset)
- contig = true;
- } else {
- /*
- * For compression, all IO should have its logical bytenr
- * set to the starting bytenr of the compressed extent.
- */
- contig = bio->bi_iter.bi_sector == sector;
- }
-
- if (!contig)
- return 0;
-
real_size = min(bio_ctrl->len_to_oe_boundary - bio_size, size);
/*
@@ -1024,6 +1022,10 @@ static void submit_extent_page(struct btrfs_bio_ctrl *bio_ctrl,
ASSERT(bio_ctrl->end_io_func);
+ if (bio_ctrl->bio &&
+ !btrfs_bio_is_contig(bio_ctrl, page, disk_bytenr, pg_offset))
+ submit_one_bio(bio_ctrl);
+
while (cur < pg_offset + size) {
u32 offset = cur - pg_offset;
int added;
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 12/12] btrfs: simplify submit_extent_page
2023-02-27 15:16 cleanup submit_extent_page and friends v2 Christoph Hellwig
` (10 preceding siblings ...)
2023-02-27 15:17 ` [PATCH 11/12] btrfs: check for contiguity in submit_extent_page Christoph Hellwig
@ 2023-02-27 15:17 ` Christoph Hellwig
2023-02-27 21:21 ` cleanup submit_extent_page and friends v2 David Sterba
12 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2023-02-27 15:17 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba; +Cc: linux-btrfs, Johannes Thumshirn
bio_add_page always adds either the entire range passed to it or nothing.
Based on that btrfs_bio_add_page can only return a length smaller than
the passed in one when hitting the ordered extent limit, which can only
happen for writes. Given that compressed writes never even use this code
path, this means that all the special cases for compressed extent offset
handling are dead code.
Reflow submit_extent_page to take advantage of this by inlining
btrfs_bio_add_page and handling the ordered extent limit by decremeting
it for each added range and thus significantly simplifying the loop.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
fs/btrfs/extent_io.c | 116 +++++++++++--------------------------------
1 file changed, 30 insertions(+), 86 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 8e6f0f07597e45..3ae42c70a82abc 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -897,47 +897,6 @@ static bool btrfs_bio_is_contig(struct btrfs_bio_ctrl *bio_ctrl,
page_offset(page) + pg_offset;
}
-/*
- * Attempt to add a page to bio.
- *
- * @bio_ctrl: record both the bio, and its bio_flags
- * @page: page to add to the bio
- * @disk_bytenr: offset of the new bio or to check whether we are adding
- * a contiguous page to the previous one
- * @size: portion of page that we want to write
- * @pg_offset: starting offset in the page
- *
- * Attempt to add a page to bio considering stripe alignment etc.
- *
- * Return >= 0 for the number of bytes added to the bio.
- * Can return 0 if the current bio is already at stripe/zone boundary.
- * Return <0 for error.
- */
-static int btrfs_bio_add_page(struct btrfs_bio_ctrl *bio_ctrl,
- struct page *page,
- u64 disk_bytenr, unsigned int size,
- unsigned int pg_offset)
-{
- struct bio *bio = bio_ctrl->bio;
- u32 bio_size = bio->bi_iter.bi_size;
- u32 real_size;
-
- ASSERT(bio);
- /* The limit should be calculated when bio_ctrl->bio is allocated */
- ASSERT(bio_ctrl->len_to_oe_boundary);
-
- real_size = min(bio_ctrl->len_to_oe_boundary - bio_size, size);
-
- /*
- * If real_size is 0, never call bio_add_*_page(), as even size is 0,
- * bio will still execute its endio function on the page!
- */
- if (real_size == 0)
- return 0;
-
- return bio_add_page(bio, page, real_size, pg_offset);
-}
-
static void calc_bio_boundaries(struct btrfs_bio_ctrl *bio_ctrl,
struct btrfs_inode *inode, u64 file_offset)
{
@@ -965,21 +924,14 @@ static void calc_bio_boundaries(struct btrfs_bio_ctrl *bio_ctrl,
static void alloc_new_bio(struct btrfs_inode *inode,
struct btrfs_bio_ctrl *bio_ctrl,
- u64 disk_bytenr, u32 offset, u64 file_offset)
+ u64 disk_bytenr, u64 file_offset)
{
struct btrfs_fs_info *fs_info = inode->root->fs_info;
struct bio *bio;
bio = btrfs_bio_alloc(BIO_MAX_VECS, bio_ctrl->opf, inode,
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.
- */
- if (bio_ctrl->compress_type != BTRFS_COMPRESS_NONE)
- bio->bi_iter.bi_sector = disk_bytenr >> SECTOR_SHIFT;
- else
- bio->bi_iter.bi_sector = (disk_bytenr + offset) >> SECTOR_SHIFT;
+ bio->bi_iter.bi_sector = disk_bytenr >> SECTOR_SHIFT;
btrfs_bio(bio)->file_offset = file_offset;
bio_ctrl->bio = bio;
calc_bio_boundaries(bio_ctrl, inode, file_offset);
@@ -1013,56 +965,48 @@ static void submit_extent_page(struct btrfs_bio_ctrl *bio_ctrl,
size_t size, unsigned long pg_offset)
{
struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
- unsigned int cur = pg_offset;
-
- ASSERT(bio_ctrl);
-
- ASSERT(pg_offset < PAGE_SIZE && size <= PAGE_SIZE &&
- pg_offset + size <= PAGE_SIZE);
+ ASSERT(pg_offset + size <= PAGE_SIZE);
ASSERT(bio_ctrl->end_io_func);
if (bio_ctrl->bio &&
!btrfs_bio_is_contig(bio_ctrl, page, disk_bytenr, pg_offset))
submit_one_bio(bio_ctrl);
- while (cur < pg_offset + size) {
- u32 offset = cur - pg_offset;
- int added;
+ do {
+ u32 len = size;
/* Allocate new bio if needed */
if (!bio_ctrl->bio) {
alloc_new_bio(inode, bio_ctrl, disk_bytenr,
- offset, page_offset(page) + cur);
+ page_offset(page) + pg_offset);
}
- /*
- * We must go through btrfs_bio_add_page() to ensure each
- * page range won't cross various boundaries.
- */
- if (bio_ctrl->compress_type != BTRFS_COMPRESS_NONE)
- added = btrfs_bio_add_page(bio_ctrl, page, disk_bytenr,
- size - offset, pg_offset + offset);
- else
- added = btrfs_bio_add_page(bio_ctrl, page,
- disk_bytenr + offset, size - offset,
- pg_offset + offset);
-
- /* Metadata page range should never be split */
- if (!is_data_inode(&inode->vfs_inode))
- ASSERT(added == 0 || added == size - offset);
-
- /* At least we added some page, update the account */
- if (bio_ctrl->wbc && added)
- wbc_account_cgroup_owner(bio_ctrl->wbc, page, added);
-
- /* We have reached boundary, submit right now */
- if (added < size - offset) {
- /* The bio should contain some page(s) */
- ASSERT(bio_ctrl->bio->bi_iter.bi_size);
+
+ /* Cap to the current ordered extent boundary if there is one */
+ if (len > bio_ctrl->len_to_oe_boundary) {
+ ASSERT(bio_ctrl->compress_type == BTRFS_COMPRESS_NONE);
+ ASSERT(is_data_inode(&inode->vfs_inode));
+ len = bio_ctrl->len_to_oe_boundary;
+ }
+
+ if (bio_add_page(bio_ctrl->bio, page, len, pg_offset) != len) {
+ /* bio full: move on to a new one */
submit_one_bio(bio_ctrl);
+ continue;
}
- cur += added;
- }
+
+ if (bio_ctrl->wbc)
+ wbc_account_cgroup_owner(bio_ctrl->wbc, page, len);
+
+ size -= len;
+ pg_offset += len;
+ disk_bytenr += len;
+ bio_ctrl->len_to_oe_boundary -= len;
+
+ /* Ordered extent boundary: move on to a new bio */
+ if (bio_ctrl->len_to_oe_boundary == 0)
+ submit_one_bio(bio_ctrl);
+ } while (size);
}
static int attach_extent_buffer_page(struct extent_buffer *eb,
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: cleanup submit_extent_page and friends v2
2023-02-27 15:16 cleanup submit_extent_page and friends v2 Christoph Hellwig
` (11 preceding siblings ...)
2023-02-27 15:17 ` [PATCH 12/12] btrfs: simplify submit_extent_page Christoph Hellwig
@ 2023-02-27 21:21 ` David Sterba
12 siblings, 0 replies; 21+ messages in thread
From: David Sterba @ 2023-02-27 21:21 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Chris Mason, Josef Bacik, David Sterba, linux-btrfs
On Mon, Feb 27, 2023 at 08:16:52AM -0700, Christoph Hellwig wrote:
> Hi all,
>
> this series cleans up submit_extent_page and it's callers and callees.
>
> Changes since v1:
>
> - remove a trailing whitespace (tab) added in an earlier patch that
> gets removed until the end
> - make a commit message more detailed
> - minor tweak to a comment
> - reorder the first two patches
The submit_extent_page certainly needed a cleanup, the patch granularity
is good. Added to misc-next, thanks.
^ permalink raw reply [flat|nested] 21+ messages in thread