Re: [PATCH v2 02/18] btrfs: extent_io: refactor __extent_writepage_io() to improve readability

[Josef Bacik <josef@toxicpanda.com>]

On 12/10/20 1:38 AM, Qu Wenruo wrote:
> The refactor involves the following modifications:
> - iosize alignment
>    In fact we don't really need to manually do alignment at all.
>    All extent maps should already be aligned, thus basic ASSERT() check
>    would be enough.
> 
> - redundant variables
>    We have extra variable like blocksize/pg_offset/end.
>    They are all unnecessary.
> 
>    @blocksize can be replaced by sectorsize size directly, and it's only
>    used to verify the em start/size is aligned.
> 
>    @pg_offset can be easily calculated using @cur and page_offset(page).
> 
>    @end is just assigned to @page_end and never modified, use @page_end
>    to replace it.
> 
> - remove some BUG_ON()s
>    The BUG_ON()s are for extent map, which we have tree-checker to check
>    on-disk extent data item and runtime check.
>    ASSERT() should be enough.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/extent_io.c | 37 +++++++++++++++++--------------------
>   1 file changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 2650e8720394..612fe60b367e 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3515,17 +3515,14 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
>   				 unsigned long nr_written,
>   				 int *nr_ret)
>   {
> +	struct btrfs_fs_info *fs_info = inode->root->fs_info;
>   	struct extent_io_tree *tree = &inode->io_tree;
>   	u64 start = page_offset(page);
>   	u64 page_end = start + PAGE_SIZE - 1;
> -	u64 end;
>   	u64 cur = start;
>   	u64 extent_offset;
>   	u64 block_start;
> -	u64 iosize;
>   	struct extent_map *em;
> -	size_t pg_offset = 0;
> -	size_t blocksize;
>   	int ret = 0;
>   	int nr = 0;
>   	const unsigned int write_flags = wbc_to_write_flags(wbc);
> @@ -3546,19 +3543,17 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
>   	 */
>   	update_nr_written(wbc, nr_written + 1);
>   
> -	end = page_end;
> -	blocksize = inode->vfs_inode.i_sb->s_blocksize;
> -
> -	while (cur <= end) {
> +	while (cur <= page_end) {
>   		u64 disk_bytenr;
>   		u64 em_end;
> +		u32 iosize;
>   
>   		if (cur >= i_size) {
>   			btrfs_writepage_endio_finish_ordered(page, cur,
>   							     page_end, 1);
>   			break;
>   		}
> -		em = btrfs_get_extent(inode, NULL, 0, cur, end - cur + 1);
> +		em = btrfs_get_extent(inode, NULL, 0, cur, page_end - cur + 1);
>   		if (IS_ERR_OR_NULL(em)) {
>   			SetPageError(page);
>   			ret = PTR_ERR_OR_ZERO(em);
> @@ -3567,16 +3562,20 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
>   
>   		extent_offset = cur - em->start;
>   		em_end = extent_map_end(em);
> -		BUG_ON(em_end <= cur);
> -		BUG_ON(end < cur);
> -		iosize = min(em_end - cur, end - cur + 1);
> -		iosize = ALIGN(iosize, blocksize);
> -		disk_bytenr = em->block_start + extent_offset;
> +		ASSERT(cur <= em_end);
> +		ASSERT(cur < page_end);
> +		ASSERT(IS_ALIGNED(em->start, fs_info->sectorsize));
> +		ASSERT(IS_ALIGNED(em->len, fs_info->sectorsize));
>   		block_start = em->block_start;
>   		compressed = test_bit(EXTENT_FLAG_COMPRESSED, &em->flags);
> +		disk_bytenr = em->block_start + extent_offset;
> +
> +		/* Note that em_end from extent_map_end() is exclusive */
> +		iosize = min(em_end, page_end + 1) - cur;
>   		free_extent_map(em);
>   		em = NULL;
>   
> +

Random extra whitespace.  Once you fix you can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef