Structure and callback cleanups
David Sterba
maintainercover Structure and callback cleanups
A handful of removed structure members that are not used, async_submit_bio is down by 16 bytes and async_cow by 8.
Some of the extent_io_ops callbacks are unnecessarily called indirectly. The rest of extent_io_ops is going to be transformed in following series.
David Sterba (7):
btrfs: remove unused member async_submit_bio::fs_info
btrfs: remove unused member async_submit_bio::bio_flags
btrfs: remove redundant member async_cow::root
btrfs: unify end_io callbacks of async_submit_bio
btrfs: drop extent_io_ops::tree_fs_info callback
btrfs: drop extent_io_ops::merge_bio_hook callback
btrfs: drop extent_io_ops::set_range_writeback callback
fs/btrfs/compression.c | 10 +++------- fs/btrfs/ctree.h | 2 +- fs/btrfs/disk-io.c | 42 +++--------------------------------------- fs/btrfs/disk-io.h | 5 +++-- fs/btrfs/extent_io.c | 28 +++++++--------------------- fs/btrfs/extent_io.h | 8 -------- fs/btrfs/inode.c | 32 ++++++++------------------------ 7 files changed, 25 insertions(+), 102 deletions(-)
Signature · 1 line
2.18.0
David Sterba
maintainerPATCH 1/7 btrfs: remove unused member async_submit_bio::fs_info
Introduced by c6100a4b4e3d1 ("Btrfs: replace tree->mapping with tree->private_data") to be used in run_one_async_done where it got unused after 736cd52e0c720103 ("Btrfs: remove nr_async_submits and async_submit_draining").
Signed-off-by David Sterba <[email protected]>
--- fs/btrfs/disk-io.c | 2 -- 1 file changed, 2 deletions(-)
fs/btrfs/disk-io.c +0 -2
@@ -106,7 +106,6 @@ void __cold btrfs_end_io_wq_exit(void)*/ struct async_submit_bio {void *private_data; -struct btrfs_fs_info *fs_info;struct bio *bio;extent_submit_bio_start_t *submit_bio_start;extent_submit_bio_done_t *submit_bio_done;
@@ -801,7 +800,6 @@ blk_status_t btrfs_wq_submit_bio(struct btrfs_fs_info *fs_info, struct bio *bio,return BLK_STS_RESOURCE ; async->private_data = private_data; - async->fs_info = fs_info; async->bio = bio; async->mirror_num = mirror_num; async->submit_bio_start = submit_bio_start;
Signature · 1 line
2.18.0
David Sterba
maintainerPATCH 2/7 btrfs: remove unused member async_submit_bio::bio_flags
In reply to David SterbaAfter splitting the start and end hooks in a758781d4b76c3 ("btrfs:
separate types for submit_bio_start and submit_bio_done"), some of the function arguments were dropped but not removed from the structure.
Signed-off-by David Sterba <[email protected]>
--- fs/btrfs/disk-io.c | 2 -- 1 file changed, 2 deletions(-)
fs/btrfs/disk-io.c +0 -2
@@ -110,7 +110,6 @@ struct async_submit_bio {extent_submit_bio_start_t *submit_bio_start;extent_submit_bio_done_t *submit_bio_done;int mirror_num; -unsigned long bio_flags;/* * bio_offset is optional, can be used if the pages in the bio * can't tell us where in the file the bio should go
@@ -808,7 +807,6 @@ blk_status_t btrfs_wq_submit_bio(struct btrfs_fs_info *fs_info, struct bio *bio,btrfs_init_work (&async->work, btrfs_worker_helper, run_one_async_start, run_one_async_done, run_one_async_free); - async->bio_flags = bio_flags; async->bio_offset = bio_offset; async->status =0 ;
Signature · 1 line
2.18.0
David Sterba
maintainerPATCH 3/7 btrfs: remove redundant member async_cow::root
In reply to David SterbaThe root is only used to get fs_info out of it, but the same can be retrieved from the inode that's in async_cow.
Signed-off-by David Sterba <[email protected]>
--- fs/btrfs/inode.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
fs/btrfs/inode.c +1 -6
@@ -358,7 +358,6 @@ struct async_extent {struct async_cow {struct inode *inode; -struct btrfs_root *root;struct page *locked_page;u64 start;u64 end;
@@ -1144,13 +1143,11 @@ static noinline void async_cow_submit(struct btrfs_work *work) {struct btrfs_fs_info *fs_info;struct async_cow *async_cow; -struct btrfs_root *root;unsigned long nr_pages; async_cow = container_of(work, struct async_cow, work); - root = async_cow->root; - fs_info = root->fs_info; + fs_info =BTRFS_I (async_cow->inode)->root->fs_info; nr_pages = (async_cow->end - async_cow->start +PAGE_SIZE ) >>PAGE_SHIFT ;
@@ -1179,7 +1176,6 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page, {struct btrfs_fs_info *fs_info =btrfs_sb (inode->i_sb);struct async_cow *async_cow; -struct btrfs_root *root =BTRFS_I (inode)->root;unsigned long nr_pages;u64 cur_end;
@@ -1189,7 +1185,6 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page, async_cow =kmalloc (sizeof (*async_cow),GFP_NOFS );BUG_ON (!async_cow);/* -ENOMEM */ async_cow->inode =igrab (inode); - async_cow->root = root; async_cow->locked_page = locked_page; async_cow->start = start; async_cow->write_flags = write_flags;
Signature · 1 line
2.18.0
Re: [PATCH 3/7] btrfs: remove redundant member async_cow::root
Quoted 22 lines · David Sterba
The root is only used to get
fs_infoout of it, but the same can be retrieved from the inode that's inasync_cow.Signed-off-by David Sterba <[email protected]>--- fs/btrfs/inode.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)fs/btrfs/inode.c +0
-1@@ -358,7 +358,6 @@ struct async_extent {struct async_cow {struct inode *inode; -struct btrfs_root *root;struct page *locked_page;u64 start;u64 end;@@ -1144,13 +1143,11 @@ static noinline void async_cow_submit(struct btrfs_work *work) {
fs/btrfs/inode.c static noinline void async_cow_submit(struct btrfs_work *work) · line 1150
Quoted 10 lines
fs/btrfs/inode.c +1
-3struct btrfs_fs_info *fs_info;struct async_cow *async_cow; -struct btrfs_root *root;unsigned long nr_pages; async_cow = container_of(work, struct async_cow, work); - root = async_cow->root; - fs_info = root->fs_info; + fs_info =BTRFS_I (async_cow->inode)->root->fs_info;
This crashes if the inode is NULL, which can happen in async_cow_start if compress_file_range returns 0 in num_added. The inode is moved to delayed iputs and async_cow->inode zeroed.
Patch removed for now. We can reach the inode via the async_cow::locked_page, but it's 5 pointers to chase and I want to have a second thought on that if the 8 bytes in the structure is not cheaper.
fs/btrfs/inode.c +3 -1
@@ -1143,11 +1143,13 @@ static noinline void async_cow_submit(struct btrfs_work *work) {struct btrfs_fs_info *fs_info;struct async_cow *async_cow; +struct inode *inode;unsigned long nr_pages; async_cow = container_of(work, struct async_cow, work); - fs_info =BTRFS_I (async_cow->inode)->root->fs_info; + inode = async_cow->locked_page->mapping->host; + fs_info =BTRFS_I (inode)->root->fs_info; nr_pages = (async_cow->end - async_cow->start +PAGE_SIZE ) >>PAGE_SHIFT ;
---
David Sterba
maintainerPATCH 4/7 btrfs: unify end_io callbacks of async_submit_bio
In reply to David SterbaThe end_io callbacks passed to btrfs_wq_submit_bio (btrfs_submit_bio_done and btree_submit_bio_done) are effectively the same code, there's no point to do the indirection. Export btrfs_submit_bio_done and call it directly.
Signed-off-by David Sterba <[email protected]>
--- fs/btrfs/disk-io.c | 28 +++------------------------- fs/btrfs/disk-io.h | 5 +++-- fs/btrfs/extent_io.h | 3 --- fs/btrfs/inode.c | 8 +++----- 4 files changed, 9 insertions(+), 35 deletions(-)
fs/btrfs/disk-io.c +3 -25
@@ -108,7 +108,6 @@ struct async_submit_bio {void *private_data;struct bio *bio;extent_submit_bio_start_t *submit_bio_start; -extent_submit_bio_done_t *submit_bio_done;int mirror_num;/* * bio_offset is optional, can be used if the pages in the bio
@@ -775,7 +774,7 @@ static void run_one_async_done(struct btrfs_work *work)return ; } - async->submit_bio_done (async->private_data, async->bio, async->mirror_num); +btrfs_submit_bio_done (async->private_data, async->bio, async->mirror_num); }static void run_one_async_free (struct btrfs_work *work)
@@ -789,8 +788,7 @@ static void run_one_async_free(struct btrfs_work *work)blk_status_t btrfs_wq_submit_bio (struct btrfs_fs_info *fs_info,struct bio *bio,int mirror_num,unsigned long bio_flags,u64 bio_offset,void *private_data, -extent_submit_bio_start_t *submit_bio_start, -extent_submit_bio_done_t *submit_bio_done) +extent_submit_bio_start_t *submit_bio_start) {struct async_submit_bio *async;
@@ -802,7 +800,6 @@ blk_status_t btrfs_wq_submit_bio(struct btrfs_fs_info *fs_info, struct bio *bio, async->bio = bio; async->mirror_num = mirror_num; async->submit_bio_start = submit_bio_start; - async->submit_bio_done = submit_bio_done;btrfs_init_work (&async->work, btrfs_worker_helper, run_one_async_start, run_one_async_done, run_one_async_free);
@@ -845,24 +842,6 @@ static blk_status_t btree_submit_bio_start(void *private_data, struct bio *bio,return btree_csum_one_bio (bio); } -static blk_status_t btree_submit_bio_done (void *private_data,struct bio *bio, -int mirror_num) -{ -struct inode *inode = private_data; -blk_status_t ret; - -/* -* when we're called for a write, we're already in the async -* submission context. Just jump into btrfs_map_bio -*/ - ret =btrfs_map_bio (btrfs_sb (inode->i_sb), bio, mirror_num,1 ); -if (ret) { - bio->bi_status = ret; -bio_endio (bio); - } -return ret; -} -static int check_async_write (struct btrfs_inode *bi) {if (atomic_read (&bi->sync_writers))
@@ -905,8 +884,7 @@ static blk_status_t btree_submit_bio_hook(void *private_data, struct bio *bio,*/ ret =btrfs_wq_submit_bio (fs_info, bio, mirror_num,0 , bio_offset, private_data, - btree_submit_bio_start, - btree_submit_bio_done); + btree_submit_bio_start); }if (ret)
fs/btrfs/disk-io.h +3 -2
@@ -120,8 +120,9 @@ blk_status_t btrfs_bio_wq_end_io(struct btrfs_fs_info *info, struct bio *bio,blk_status_t btrfs_wq_submit_bio (struct btrfs_fs_info *fs_info,struct bio *bio,int mirror_num,unsigned long bio_flags,u64 bio_offset,void *private_data, -extent_submit_bio_start_t *submit_bio_start, -extent_submit_bio_done_t *submit_bio_done); +extent_submit_bio_start_t *submit_bio_start); +blk_status_t btrfs_submit_bio_done (void *private_data,struct bio *bio, +int mirror_num);int btrfs_write_tree_block (struct extent_buffer *buf);void btrfs_wait_tree_block_writeback (struct extent_buffer *buf); int btrfs_init_log_root_tree(struct btrfs_trans_handle *trans,
fs/btrfs/extent_io.h +0 -3
@@ -92,9 +92,6 @@ typedef blk_status_t (extent_submit_bio_hook_t)(void *private_data, struct bio *typedef blk_status_t (extent_submit_bio_start_t )(void *private_data,struct bio *bio,u64 bio_offset); -typedef blk_status_t (extent_submit_bio_done_t )(void *private_data, -struct bio *bio,int mirror_num); -struct extent_io_ops {/* * The following callbacks must be allways defined, the function
fs/btrfs/inode.c +3 -5
@@ -1952,7 +1952,7 @@ static blk_status_t btrfs_submit_bio_start(void *private_data, struct bio *bio,* At IO completion time the cums attached on the ordered extent record * are inserted into the btree */ -static blk_status_t btrfs_submit_bio_done (void *private_data,struct bio *bio, +blk_status_t btrfs_submit_bio_done (void *private_data,struct bio *bio,int mirror_num) {struct inode *inode = private_data;
@@ -2025,8 +2025,7 @@ static blk_status_t btrfs_submit_bio_hook(void *private_data, struct bio *bio,/* we're doing a write, do the async checksumming */ ret =btrfs_wq_submit_bio (fs_info, bio, mirror_num, bio_flags, bio_offset, inode, - btrfs_submit_bio_start, - btrfs_submit_bio_done); + btrfs_submit_bio_start); goto out; }else if (!skip_sum) { ret =btrfs_csum_one_bio (inode, bio,0 ,0 );
@@ -8290,8 +8289,7 @@ static inline blk_status_t btrfs_submit_dio_bio(struct bio *bio,if (write && async_submit) { ret =btrfs_wq_submit_bio (fs_info, bio,0 ,0 , file_offset, inode, - btrfs_submit_bio_start_direct_io, - btrfs_submit_bio_done); + btrfs_submit_bio_start_direct_io); goto err; }else if (write) {/*
Signature · 1 line
2.18.0
David Sterba
maintainerPATCH 5/7 btrfs: drop extent_io_ops::tree_fs_info callback
In reply to David SterbaAll implementations of the callback are trivial and do the same and there's only user. Merge everything together.
Signed-off-by David Sterba <[email protected]>
--- fs/btrfs/disk-io.c | 7 ------- fs/btrfs/extent_io.c | 14 ++++---------- fs/btrfs/extent_io.h | 1 - fs/btrfs/inode.c | 7 ------- 4 files changed, 4 insertions(+), 25 deletions(-)
fs/btrfs/disk-io.c +0 -7
@@ -4523,12 +4523,6 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)return 0 ; } -static struct btrfs_fs_info *btree_fs_info (void *private_data) -{ -struct inode *inode = private_data; -return btrfs_sb (inode->i_sb); -} -static const struct extent_io_ops btree_extent_io_ops = {/* mandatory callbacks */ .submit_bio_hook = btree_submit_bio_hook,
@@ -4537,7 +4531,6 @@ static const struct extent_io_ops btree_extent_io_ops = { .merge_bio_hook = btrfs_merge_bio_hook, .readpage_io_failed_hook = btree_io_failed_hook, .set_range_writeback = btrfs_set_range_writeback, - .tree_fs_info = btree_fs_info,/* optional callbacks */ };
fs/btrfs/extent_io.c +4 -10
@@ -140,14 +140,6 @@ static int add_extent_changeset(struct extent_state *state, unsigned bits,static void flush_write_bio (struct extent_page_data *epd); -static inline struct btrfs_fs_info * -tree_fs_info (struct extent_io_tree *tree) -{ -if (tree->ops) -return tree->ops->tree_fs_info (tree->private_data); -return NULL ; -} -int __init extent_io_init (void ) { extent_state_cache = kmem_cache_create("btrfs_extent_state",
@@ -564,8 +556,10 @@ alloc_extent_state_atomic(struct extent_state *prealloc)static void extent_io_tree_panic (struct extent_io_tree *tree,int err) { -btrfs_panic (tree_fs_info (tree), err, -"Locking error: Extent tree was modified by another thread while locked." ); +struct inode *inode = tree->private_data; + +btrfs_panic (btrfs_sb (inode->i_sb), err, +"locking error: extent tree was modified by another thread while locked" ); }/*
fs/btrfs/extent_io.h +0 -1
@@ -105,7 +105,6 @@ struct extent_io_ops { size_t size, struct bio *bio, unsigned long bio_flags);int (*readpage_io_failed_hook)(struct page *page,int failed_mirror); -struct btrfs_fs_info *(*tree_fs_info)(void *private_data);void (*set_range_writeback)(void *private_data,u64 start,u64 end);/*
fs/btrfs/inode.c +0 -7
@@ -10468,12 +10468,6 @@ static int btrfs_readpage_io_failed_hook(struct page *page, int failed_mirror)return -EAGAIN ; } -static struct btrfs_fs_info *iotree_fs_info (void *private_data) -{ -struct inode *inode = private_data; -return btrfs_sb (inode->i_sb); -} -static void btrfs_check_extent_io_range (void *private_data,const char *caller,u64 start,u64 end) {
@@ -10548,7 +10542,6 @@ static const struct extent_io_ops btrfs_extent_io_ops = { .readpage_end_io_hook = btrfs_readpage_end_io_hook, .merge_bio_hook = btrfs_merge_bio_hook, .readpage_io_failed_hook = btrfs_readpage_io_failed_hook, - .tree_fs_info = iotree_fs_info, .set_range_writeback = btrfs_set_range_writeback,/* optional callbacks */
Signature · 1 line
2.18.0
David Sterba
maintainerPATCH 6/7 btrfs: drop extent_io_ops::merge_bio_hook callback
In reply to David SterbaThe data and metadata callback implementation both use the same function. We can remove the call indirection completely.
Signed-off-by David Sterba <[email protected]>
--- fs/btrfs/compression.c | 10 +++------- fs/btrfs/disk-io.c | 2 -- fs/btrfs/extent_io.c | 4 ++-- fs/btrfs/extent_io.h | 3 --- fs/btrfs/inode.c | 5 ++--- 5 files changed, 7 insertions(+), 17 deletions(-)
fs/btrfs/compression.c +3 -7
@@ -299,7 +299,6 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,struct bio *bio =NULL ;struct compressed_bio *cb;unsigned long bytes_left; -struct extent_io_tree *io_tree = &BTRFS_I (inode)->io_tree;int pg_index =0 ;struct page *page;u64 first_byte = disk_start;
@@ -338,9 +337,7 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start, page = compressed_pages[pg_index]; page->mapping = inode->i_mapping;if (bio->bi_iter.bi_size) - submit = io_tree->ops->merge_bio_hook (page,0 , -PAGE_SIZE , - bio,0 ); + submit =btrfs_merge_bio_hook (page,0 ,PAGE_SIZE , bio,0 ); page->mapping =NULL ; if (submit || bio_add_page(bio, page, PAGE_SIZE, 0) <
@@ -622,9 +619,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio, page->index = em_start >>PAGE_SHIFT ;if (comp_bio->bi_iter.bi_size) - submit = tree->ops->merge_bio_hook (page,0 , -PAGE_SIZE , - comp_bio,0 ); + submit =btrfs_merge_bio_hook (page,0 ,PAGE_SIZE , + comp_bio,0 ); page->mapping =NULL ; if (submit || bio_add_page(comp_bio, page, PAGE_SIZE, 0) <
fs/btrfs/disk-io.c +0 -2
@@ -4527,8 +4527,6 @@ static const struct extent_io_ops btree_extent_io_ops = {/* mandatory callbacks */ .submit_bio_hook = btree_submit_bio_hook, .readpage_end_io_hook = btree_readpage_end_io_hook, -/* note we're sharing with inode.c for the merge bio hook */ - .merge_bio_hook = btrfs_merge_bio_hook, .readpage_io_failed_hook = btree_io_failed_hook, .set_range_writeback = btrfs_set_range_writeback,
fs/btrfs/extent_io.c +2 -2
@@ -2784,8 +2784,8 @@ static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree,else contig =bio_end_sector (bio) == sector; -if (tree->ops && tree->ops->merge_bio_hook (page, offset, - page_size, bio, bio_flags)) +if (tree->ops &&btrfs_merge_bio_hook (page, offset, page_size, + bio, bio_flags)) can_merge = false; if (prev_bio_flags != bio_flags || !contig || !can_merge ||
fs/btrfs/extent_io.h +0 -3
@@ -101,9 +101,6 @@ struct extent_io_ops {int (*readpage_end_io_hook)(struct btrfs_io_bio *io_bio,u64 phy_offset,struct page *page,u64 start,u64 end,int mirror); -int (*merge_bio_hook)(struct page *page,unsigned long offset, -size_t size,struct bio *bio, -unsigned long bio_flags);int (*readpage_io_failed_hook)(struct page *page,int failed_mirror);void (*set_range_writeback)(void *private_data,u64 start,u64 end);
fs/btrfs/inode.c +2 -3
@@ -1893,8 +1893,8 @@ static void btrfs_clear_bit_hook(void *private_data, }/* -* extent_io.c merge_bio_hook, this must check the chunk tree to make sure -* we don't create bios that span stripes or chunks +* Merge bio hook, this must check the chunk tree to make sure we don't create +* bios that span stripes or chunks * * return 1 if page cannot be merged to bio * return 0 if page can be merged to bio
@@ -10540,7 +10540,6 @@ static const struct extent_io_ops btrfs_extent_io_ops = {/* mandatory callbacks */ .submit_bio_hook = btrfs_submit_bio_hook, .readpage_end_io_hook = btrfs_readpage_end_io_hook, - .merge_bio_hook = btrfs_merge_bio_hook, .readpage_io_failed_hook = btrfs_readpage_io_failed_hook, .set_range_writeback = btrfs_set_range_writeback,
Signature · 1 line
2.18.0
Re: [PATCH 6/7] btrfs: drop extent_io_ops::merge_bio_hook callback
Quoted 65 lines · David Sterba
The data and metadata callback implementation both use the same function. We can remove the call indirection completely.
Signed-off-by David Sterba <[email protected]>--- fs/btrfs/compression.c | 10 +++------- fs/btrfs/disk-io.c | 2 -- fs/btrfs/extent_io.c | 4 ++-- fs/btrfs/extent_io.h | 3 --- fs/btrfs/inode.c | 5 ++--- 5 files changed, 7 insertions(+), 17 deletions(-)fs/btrfs/compression.c +3
-7@@ -299,7 +299,6 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,struct bio *bio =NULL ;struct compressed_bio *cb;unsigned long bytes_left; -struct extent_io_tree *io_tree = &BTRFS_I (inode)->io_tree;int pg_index =0 ;struct page *page;u64 first_byte = disk_start;@@ -338,9 +337,7 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start, page = compressed_pages[pg_index]; page->mapping = inode->i_mapping;if (bio->bi_iter.bi_size) - submit = io_tree->ops->merge_bio_hook (page,0 , -PAGE_SIZE , - bio,0 ); + submit =btrfs_merge_bio_hook (page,0 ,PAGE_SIZE , bio,0 ); page->mapping =NULL ; if (submit || bio_add_page(bio, page, PAGE_SIZE, 0) <@@ -622,9 +619,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio, page->index = em_start >>PAGE_SHIFT ;if (comp_bio->bi_iter.bi_size) - submit = tree->ops->merge_bio_hook (page,0 , -PAGE_SIZE , - comp_bio,0 ); + submit =btrfs_merge_bio_hook (page,0 ,PAGE_SIZE , + comp_bio,0 ); page->mapping =NULL ; if (submit || bio_add_page(comp_bio, page, PAGE_SIZE, 0) <fs/btrfs/disk-io.c +0
-2@@ -4527,8 +4527,6 @@ static const struct extent_io_ops btree_extent_io_ops = {/* mandatory callbacks */ .submit_bio_hook = btree_submit_bio_hook, .readpage_end_io_hook = btree_readpage_end_io_hook, -/* note we're sharing with inode.c for the merge bio hook */ - .merge_bio_hook = btrfs_merge_bio_hook, .readpage_io_failed_hook = btree_io_failed_hook, .set_range_writeback = btrfs_set_range_writeback,fs/btrfs/extent_io.c +0
-0
fs/btrfs/extent_io.c static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree, · line 2788
Quoted 7 lines
fs/btrfs/extent_io.c +2
-2@@ -2784,8 +2784,8 @@ static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree,else contig =bio_end_sector (bio) == sector; -if (tree->ops && tree->ops->merge_bio_hook (page, offset, - page_size, bio, bio_flags)) +if (tree->ops &&btrfs_merge_bio_hook (page, offset, page_size, + bio, bio_flags))
While this is correct I dislike having the if (tree->ops ) check. So under what conditions do we NOT have the tree->ops set? Looking at the code it seems we are setting it everytime we create an inode in btrfs_create. We also set it for symlinks (btrfs_symlink) and a tmp file (btrfs_tmpfile). Strangely enough in btrfs_read_locked_inode we only set it for regular files (S_IFREG).
submit_extent_page seems to be called for both metadata but as we no longer distinguish between eb merge_hook and data merge_hook perhaps the tree->ops could be killed as well ?
Quoted 41 lines
fs/btrfs/extent_io.c +0
-0can_merge = false; if (prev_bio_flags != bio_flags || !contig || !can_merge ||fs/btrfs/extent_io.h +0
-3@@ -101,9 +101,6 @@ struct extent_io_ops {int (*readpage_end_io_hook)(struct btrfs_io_bio *io_bio,u64 phy_offset,struct page *page,u64 start,u64 end,int mirror); -int (*merge_bio_hook)(struct page *page,unsigned long offset, -size_t size,struct bio *bio, -unsigned long bio_flags);int (*readpage_io_failed_hook)(struct page *page,int failed_mirror);void (*set_range_writeback)(void *private_data,u64 start,u64 end);fs/btrfs/inode.c +2
-3@@ -1893,8 +1893,8 @@ static void btrfs_clear_bit_hook(void *private_data, }/* -* extent_io.c merge_bio_hook, this must check the chunk tree to make sure -* we don't create bios that span stripes or chunks +* Merge bio hook, this must check the chunk tree to make sure we don't create +* bios that span stripes or chunks * * return 1 if page cannot be merged to bio * return 0 if page can be merged to bio @@ -10540,7 +10540,6 @@ static const struct extent_io_ops btrfs_extent_io_ops = {/* mandatory callbacks */ .submit_bio_hook = btrfs_submit_bio_hook, .readpage_end_io_hook = btrfs_readpage_end_io_hook, - .merge_bio_hook = btrfs_merge_bio_hook, .readpage_io_failed_hook = btrfs_readpage_io_failed_hook, .set_range_writeback = btrfs_set_range_writeback,
Re: [PATCH 6/7] btrfs: drop extent_io_ops::merge_bio_hook callback
Quoted 69 lines · Nikolay Borisov
Quoted 68 lines · David Sterba
The data and metadata callback implementation both use the same function. We can remove the call indirection completely.
Signed-off-by David Sterba <[email protected]>--- fs/btrfs/compression.c | 10 +++------- fs/btrfs/disk-io.c | 2 -- fs/btrfs/extent_io.c | 4 ++-- fs/btrfs/extent_io.h | 3 --- fs/btrfs/inode.c | 5 ++--- 5 files changed, 7 insertions(+), 17 deletions(-)fs/btrfs/compression.c +3
-7@@ -299,7 +299,6 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,struct bio *bio =NULL ;struct compressed_bio *cb;unsigned long bytes_left; -struct extent_io_tree *io_tree = &BTRFS_I (inode)->io_tree;int pg_index =0 ;struct page *page;u64 first_byte = disk_start;@@ -338,9 +337,7 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start, page = compressed_pages[pg_index]; page->mapping = inode->i_mapping;if (bio->bi_iter.bi_size) - submit = io_tree->ops->merge_bio_hook (page,0 , -PAGE_SIZE , - bio,0 ); + submit =btrfs_merge_bio_hook (page,0 ,PAGE_SIZE , bio,0 ); page->mapping =NULL ; if (submit || bio_add_page(bio, page, PAGE_SIZE, 0) <@@ -622,9 +619,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio, page->index = em_start >>PAGE_SHIFT ;if (comp_bio->bi_iter.bi_size) - submit = tree->ops->merge_bio_hook (page,0 , -PAGE_SIZE , - comp_bio,0 ); + submit =btrfs_merge_bio_hook (page,0 ,PAGE_SIZE , + comp_bio,0 ); page->mapping =NULL ; if (submit || bio_add_page(comp_bio, page, PAGE_SIZE, 0) <fs/btrfs/disk-io.c +0
-2@@ -4527,8 +4527,6 @@ static const struct extent_io_ops btree_extent_io_ops = {/* mandatory callbacks */ .submit_bio_hook = btree_submit_bio_hook, .readpage_end_io_hook = btree_readpage_end_io_hook, -/* note we're sharing with inode.c for the merge bio hook */ - .merge_bio_hook = btrfs_merge_bio_hook, .readpage_io_failed_hook = btree_io_failed_hook, .set_range_writeback = btrfs_set_range_writeback,fs/btrfs/extent_io.c +0
-0@@ -2784,8 +2784,8 @@ static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree,else contig =bio_end_sector (bio) == sector;
Quoted 11 lines
fs/btrfs/extent_io.c static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree, · line 2788
Quoted 4 lines
fs/btrfs/extent_io.c +2
-2-if (tree->ops && tree->ops->merge_bio_hook (page, offset, - page_size, bio, bio_flags)) +if (tree->ops &&btrfs_merge_bio_hook (page, offset, page_size, + bio, bio_flags))While this is correct I dislike having the if (
tree->ops) check. So under what conditions do we NOT have thetree->opsset? Looking at the code it seems we are setting it everytime we create an inode inbtrfs_create. We also set it for symlinks (btrfs_symlink) and a tmp file (btrfs_tmpfile). Strangely enough inbtrfs_read_locked_inodewe only set it for regular files (S_IFREG).
That's correct. I did not notice setting extent_io ops in the symlink and tmpfile functions and found only the weirdness in btrfs_read_locked_inode, but it looks like we can drop the tree->ops completely.
Quoted 3 lines
submit_extent_pageseems to be called for both metadata but as we no longer distinguish between ebmerge_hookand datamerge_hookperhaps thetree->opscould be killed as well ?
I have WIP patches that replace the callbacks that touch the callsites doing the tree->ops checks, so I'll keep it mind that there are more cleanups to be done.
David Sterba
maintainerPATCH 7/7 btrfs: drop extent_io_ops::set_range_writeback callback
In reply to David SterbaThe data and metadata callback implementation both use the same function. We can remove the call indirection and intermediate helper completely.
Signed-off-by David Sterba <[email protected]>
--- fs/btrfs/ctree.h | 2 +- fs/btrfs/disk-io.c | 1 - fs/btrfs/extent_io.c | 10 +--------- fs/btrfs/extent_io.h | 1 - fs/btrfs/inode.c | 5 ++--- 5 files changed, 4 insertions(+), 15 deletions(-)
fs/btrfs/ctree.h +1 -1
@@ -3172,7 +3172,7 @@ int btrfs_create_subvol_root(struct btrfs_trans_handle *trans,int btrfs_merge_bio_hook (struct page *page,unsigned long offset,size_t size,struct bio *bio,unsigned long bio_flags); -void btrfs_set_range_writeback (void *private_data,u64 start,u64 end); +void btrfs_set_range_writeback (struct extent_io_tree *tree,u64 start,u64 end);vm_fault_t btrfs_page_mkwrite (struct vm_fault *vmf);int btrfs_readpage (struct file *file,struct page *page);void btrfs_evict_inode (struct inode *inode);
fs/btrfs/disk-io.c +0 -1
@@ -4528,7 +4528,6 @@ static const struct extent_io_ops btree_extent_io_ops = { .submit_bio_hook = btree_submit_bio_hook, .readpage_end_io_hook = btree_readpage_end_io_hook, .readpage_io_failed_hook = btree_io_failed_hook, - .set_range_writeback = btrfs_set_range_writeback,/* optional callbacks */ };
fs/btrfs/extent_io.c +1 -9
@@ -1380,14 +1380,6 @@ void extent_range_redirty_for_io(struct inode *inode, u64 start, u64 end) } } -/* -* helper function to set both pages and extents in the tree writeback -*/ -static void set_range_writeback (struct extent_io_tree *tree,u64 start,u64 end) -{ - tree->ops->set_range_writeback (tree->private_data, start, end); -} -/* find the first state struct with 'bits' set after 'start', and * return it. tree->lock must be held. NULL will returned if * nothing was found after 'start'
@@ -3416,7 +3408,7 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,continue ; } -set_range_writeback (tree, cur, cur + iosize -1 ); +btrfs_set_range_writeback (tree, cur, cur + iosize -1 );if (!PageWriteback (page)) { btrfs_err(BTRFS_I(inode)->root->fs_info, "page %lu not writeback, cur %llu end %llu",
fs/btrfs/extent_io.h +0 -1
@@ -102,7 +102,6 @@ struct extent_io_ops { struct page *page, u64 start, u64 end, int mirror);int (*readpage_io_failed_hook)(struct page *page,int failed_mirror); -void (*set_range_writeback)(void *private_data,u64 start,u64 end);/* * Optional hooks, called if the pointer is not NULL
fs/btrfs/inode.c +2 -3
@@ -10482,9 +10482,9 @@ static void btrfs_check_extent_io_range(void *private_data, const char *caller, } } -void btrfs_set_range_writeback (void *private_data,u64 start,u64 end) +void btrfs_set_range_writeback (struct extent_io_tree *tree,u64 start,u64 end) { -struct inode *inode = private_data; +struct inode *inode = tree->private_data;unsigned long index = start >>PAGE_SHIFT ;unsigned long end_index = end >>PAGE_SHIFT ;struct page *page;
@@ -10541,7 +10541,6 @@ static const struct extent_io_ops btrfs_extent_io_ops = { .submit_bio_hook = btrfs_submit_bio_hook, .readpage_end_io_hook = btrfs_readpage_end_io_hook, .readpage_io_failed_hook = btrfs_readpage_io_failed_hook, - .set_range_writeback = btrfs_set_range_writeback,/* optional callbacks */ .fill_delalloc = run_delalloc_range,
Signature · 1 line
2.18.0
Quoted 16 lines · David Sterba
A handful of removed structure members that are not used,
async_submit_biois down by 16 bytes andasync_cowby 8.Some of the
extent_io_opscallbacks are unnecessarily called indirectly. The rest ofextent_io_opsis going to be transformed in following series.David Sterba (7):
btrfs: remove unused memberasync_submit_bio::fs_info
btrfs: remove unused memberasync_submit_bio::bio_flags
btrfs: remove redundant memberasync_cow::root
btrfs: unifyend_iocallbacks ofasync_submit_bio
btrfs: dropextent_io_ops::tree_fs_infocallback
btrfs: dropextent_io_ops::merge_bio_hookcallback
btrfs: dropextent_io_ops::set_range_writebackcallback
Quoted 9 lines
fs/btrfs/compression.c | 10 +++------- fs/btrfs/ctree.h | 2 +- fs/btrfs/disk-io.c | 42 +++--------------------------------------- fs/btrfs/disk-io.h | 5 +++-- fs/btrfs/extent_io.c | 28 +++++++--------------------- fs/btrfs/extent_io.h | 8 -------- fs/btrfs/inode.c | 32 ++++++++------------------------ 7 files changed, 25 insertions(+), 102 deletions(-)
For the whole series (apart from my comments on 6/7):
Reviewed-by Nikolay Borisov <[email protected]>
I'm guessing this will alleviate some of the performance hit stemming from spectre/meltdown mitigations.