Re: [PATCH] btrfs: Refactor btrfs_merge_bio_hook

[Nikolay Borisov <nborisov@suse.com>]

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.

>