| Message ID | 20181127185758.16220-1-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Refactor btrfs_merge_bio_hook | expand |
On Tue, Nov 27, 2018 at 08:57:58PM +0200, Nikolay Borisov wrote: > This function really checks whether adding more data to the bio will > straddle a stripe/chunk. So first let's give it a more appropraite > name - btrfs_bio_fits_in_stripe. Secondly, the offset parameter was > never used to just remove it. Thirdly, pages are submitted to either > btree or data inodes so it's guaranteed that tree->ops is set so replace > the check with an ASSERT. Finally, document the parameters of the > function. No functional changes. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> > - submit = btrfs_merge_bio_hook(page, 0, PAGE_SIZE, bio, 0); > + submit = btrfs_bio_fits_in_stripe(page, PAGE_SIZE, bio, > + 0); > - submit = btrfs_merge_bio_hook(page, 0, PAGE_SIZE, > - comp_bio, 0); > + submit = btrfs_bio_fits_in_stripe(page, PAGE_SIZE, > + comp_bio, 0); > - if (tree->ops && btrfs_merge_bio_hook(page, offset, page_size, > - bio, bio_flags)) > + ASSERT(tree->ops); > + if (btrfs_bio_fits_in_stripe(page, page_size, bio, bio_flags)) > can_merge = false; Got me curious if we could get rid of the size parameter, it's 2x PAGE_SIZE and could be something else in one case but it's not obvious if it really happens. Another thing I noticed is lack of proper error handling in all callers, as its' 0, 1, and negative errno. The error would be interpreted as true ie. add page to bio and continue.
On 28.11.18 г. 18:46 ч., David Sterba wrote: > On Tue, Nov 27, 2018 at 08:57:58PM +0200, Nikolay Borisov wrote: >> This function really checks whether adding more data to the bio will >> straddle a stripe/chunk. So first let's give it a more appropraite >> name - btrfs_bio_fits_in_stripe. Secondly, the offset parameter was >> never used to just remove it. Thirdly, pages are submitted to either >> btree or data inodes so it's guaranteed that tree->ops is set so replace >> the check with an ASSERT. Finally, document the parameters of the >> function. No functional changes. >> >> Signed-off-by: Nikolay Borisov <nborisov@suse.com> > > Reviewed-by: David Sterba <dsterba@suse.com> > >> - submit = btrfs_merge_bio_hook(page, 0, PAGE_SIZE, bio, 0); >> + submit = btrfs_bio_fits_in_stripe(page, PAGE_SIZE, bio, >> + 0); > >> - submit = btrfs_merge_bio_hook(page, 0, PAGE_SIZE, >> - comp_bio, 0); >> + submit = btrfs_bio_fits_in_stripe(page, PAGE_SIZE, >> + comp_bio, 0); > > >> - if (tree->ops && btrfs_merge_bio_hook(page, offset, page_size, >> - bio, bio_flags)) >> + ASSERT(tree->ops); >> + if (btrfs_bio_fits_in_stripe(page, page_size, bio, bio_flags)) >> can_merge = false; > > Got me curious if we could get rid of the size parameter, it's 2x > PAGE_SIZE and could be something else in one case but it's not obvious > if it really happens. > > Another thing I noticed is lack of proper error handling in all callers, > as its' 0, 1, and negative errno. The error would be interpreted as true > ie. add page to bio and continue. Actually anything other than 0 is returned then the current bio is actually submitted (I presume you refer to the code in compression.c). As a matter of fact I think btrfs_bio_fits_in_stripe could even be turned to return a bool value. THe only time this function could return an error is if the mapping logic goes haywire which could happen 'if (offset < stripe_offset) {' or we don't find a chunk for the given offset, which is unlikely. >
On Wed, Nov 28, 2018 at 06:51:59PM +0200, Nikolay Borisov wrote: > > Got me curious if we could get rid of the size parameter, it's 2x > > PAGE_SIZE and could be something else in one case but it's not obvious > > if it really happens. > > > > Another thing I noticed is lack of proper error handling in all callers, > > as its' 0, 1, and negative errno. The error would be interpreted as true > > ie. add page to bio and continue. > > Actually anything other than 0 is returned then the current bio is > actually submitted (I presume you refer to the code in compression.c). > As a matter of fact I think btrfs_bio_fits_in_stripe could even be > turned to return a bool value. > > THe only time this function could return an error is if the mapping > logic goes haywire which could happen 'if (offset < stripe_offset) {' or > we don't find a chunk for the given offset, which is unlikely. Unlikely yes, but if it's possible to trigger the mapping failure eg. by a crafted image, it should be handled. Besides the mapping errors, there are EIO or ENOMEM in __btrfs_map_block or its callees. I see all other callers of map block handle the errors.
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c index 34d50bc5c10d..dba59ae914b8 100644 --- a/fs/btrfs/compression.c +++ b/fs/btrfs/compression.c @@ -332,7 +332,8 @@ 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 = btrfs_merge_bio_hook(page, 0, PAGE_SIZE, bio, 0); + submit = btrfs_bio_fits_in_stripe(page, PAGE_SIZE, bio, + 0); page->mapping = NULL; if (submit || bio_add_page(bio, page, PAGE_SIZE, 0) < @@ -610,8 +611,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 = btrfs_merge_bio_hook(page, 0, PAGE_SIZE, - comp_bio, 0); + submit = btrfs_bio_fits_in_stripe(page, PAGE_SIZE, + comp_bio, 0); page->mapping = NULL; if (submit || bio_add_page(comp_bio, page, PAGE_SIZE, 0) < diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index a98507fa9192..791112a82775 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -3191,9 +3191,8 @@ void btrfs_merge_delalloc_extent(struct inode *inode, struct extent_state *new, struct extent_state *other); void btrfs_split_delalloc_extent(struct inode *inode, struct extent_state *orig, u64 split); -int btrfs_merge_bio_hook(struct page *page, unsigned long offset, - size_t size, struct bio *bio, - unsigned long bio_flags); +int btrfs_bio_fits_in_stripe(struct page *page, size_t size, struct bio *bio, + unsigned long bio_flags); 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); diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 582b4b1c41e0..19f4b8fd654f 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -2759,8 +2759,8 @@ static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree, else contig = bio_end_sector(bio) == sector; - if (tree->ops && btrfs_merge_bio_hook(page, offset, page_size, - bio, bio_flags)) + ASSERT(tree->ops); + if (btrfs_bio_fits_in_stripe(page, page_size, bio, bio_flags)) can_merge = false; if (prev_bio_flags != bio_flags || !contig || !can_merge || diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index be7d43c42779..11c533db2db7 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -1881,16 +1881,21 @@ void btrfs_clear_delalloc_extent(struct inode *vfs_inode, } /* - * Merge bio hook, this must check the chunk tree to make sure we don't create - * bios that span stripes or chunks + * btrfs_bio_fits_in_stripe - Checks whether the size of the given bio will fit + * in a chunk's stripe. This function ensures that bios do not span a + * stripe/chunk * - * return 1 if page cannot be merged to bio - * return 0 if page can be merged to bio + * @page - The page we are about to add to the bio + * @size - size we want to add to the bio + * @bio - bio we want to ensure is smaller than a stripe + * @bio_flags - flags of the bio + * + * return 1 if page cannot be added to the bio + * return 0 if page can be added to the bio * return error otherwise */ -int btrfs_merge_bio_hook(struct page *page, unsigned long offset, - size_t size, struct bio *bio, - unsigned long bio_flags) +int btrfs_bio_fits_in_stripe(struct page *page, size_t size, struct bio *bio, + unsigned long bio_flags) { struct inode *inode = page->mapping->host; struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
This function really checks whether adding more data to the bio will straddle a stripe/chunk. So first let's give it a more appropraite name - btrfs_bio_fits_in_stripe. Secondly, the offset parameter was never used to just remove it. Thirdly, pages are submitted to either btree or data inodes so it's guaranteed that tree->ops is set so replace the check with an ASSERT. Finally, document the parameters of the function. No functional changes. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/compression.c | 7 ++++--- fs/btrfs/ctree.h | 5 ++--- fs/btrfs/extent_io.c | 4 ++-- fs/btrfs/inode.c | 19 ++++++++++++------- 4 files changed, 20 insertions(+), 15 deletions(-)