Re: [PATCH v3 6/9] btrfs: introduce alloc_submit_compressed_bio() for compression

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2021/6/15 下午11:58, Johannes Thumshirn wrote:
> On 15/06/2021 14:18, Qu Wenruo wrote:
>> +static struct bio *alloc_compressed_bio(struct compressed_bio *cb, u64 disk_bytenr,
>> +					unsigned int opf, bio_end_io_t endio_func)
>> +{
>> +	struct bio *bio;
>> +
>> +	bio = btrfs_bio_alloc(disk_bytenr);
>> +	/* bioset allocation should not fail */
>> +	ASSERT(bio);
>
> Here you write that bio allocation shouldn't fail (because it's backed
> by a bioset/mempool and we're not calling from IRQ context).

But alloc_compressed_bio() has other error path, namingly
btrfs_get_chunk_map() and btrfs_get_io_geometry() can fail, thus caller
still need to check that.

Although thanks to your mention, I find that I should call bio_put() for
the above error cases before returning ERR_CAST().

Thanks,
Qu
>
> [...]
>
>> +	bio = alloc_compressed_bio(cb, first_byte, bio_op | write_flags,
>> +				   end_compressed_bio_write);
>> +	if (IS_ERR(bio)) {
>> +		kfree(cb);
>> +		return errno_to_blk_status(PTR_ERR(bio));
>>   	}
>
> Here you're checking for IS_ERR().
>
>> @@ -545,10 +569,14 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
>
> [...]
>
>> +			bio = alloc_compressed_bio(cb, first_byte,
>> +					bio_op | write_flags,
>> +					end_compressed_bio_write);
>> +			if (IS_ERR(bio)) {
>> +				ret = errno_to_blk_status(PTR_ERR(bio));
>> +				bio = NULL;
>> +				goto finish_cb;
>> +			}
>
> same
>
>> @@ -812,10 +840,13 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>
> [...]
>
>> +	comp_bio = alloc_compressed_bio(cb, cur_disk_byte, REQ_OP_READ,
>> +					end_compressed_bio_read);
>> +	if (IS_ERR(comp_bio)) {
>> +		ret = errno_to_blk_status(PTR_ERR(comp_bio));
>> +		comp_bio = NULL;
>> +		goto fail2;
>> +	}
>>
>
> same
>
> if btrfs_bio_alloc() would have failed we'd already crash on a nullptr
> dereference much earlier.
>