Re: [PATCH v4 5/6] btrfs: implement uncompressed fallback for delayed bbio

[Qu Wenruo <wqu@suse.com>]

在 2026/9/25 03:06, Boris Burkov 写道:
> On Fri, Sep 18, 2026 at 09:00:30AM +0930, Qu Wenruo wrote:
>> When compression fails (either bad ratio, fragmented free space, or
>> writeback path chooses to submit the bio early), we have to fall back to
>> uncompressed writes.
>>
>> The uncompressed fallback is mostly the same as cow_file_range() but
>> with some changes:
>>
>> - Endio function is slightly different from the compressed path
>>    Only in the folio freeing handling.
>>
>> - Uncompressed fallback error handling
>>    Since at this stage, the folios already have WRITEBACK flag set, we do
>>    not need to do the usual page unlock/end writeback, but just free the
>>    reserved space and call it a day.
> 
> Continuing the reserved space issue from patch 4, here, both finishing
> that bug and showing a second very similar one.
> 
[...]
>> +	btrfs_lock_extent(&inode->io_tree, file_offset, cur_end, cached);
>> +	em = btrfs_create_io_em(inode, file_offset, &file_extent, BTRFS_ORDERED_REGULAR);
>> +	if (IS_ERR(em)) {
>> +		ret = PTR_ERR(em);
>> +		btrfs_unlock_extent(&inode->io_tree, file_offset, cur_end, cached);
>> +		goto free_reserved;
>> +	}
>> +	btrfs_free_extent_map(em);
>> +	ordered = btrfs_alloc_ordered_extent(inode, file_offset, &file_extent,
>> +					     1U << BTRFS_ORDERED_REGULAR);
>> +	if (IS_ERR(ordered)) {
>> +		btrfs_drop_extent_map_range(inode, file_offset, cur_end, false);
>> +		btrfs_unlock_extent(&inode->io_tree, file_offset, cur_end, cached);
>> +		ret = PTR_ERR(ordered);
>> +		goto free_reserved;
> 
> Now suppose this fails. We will have a missing OE range in the parent,
> so when we finish the parent, we will free its reservation (patch 1)

That's indeed another problem, and the layered OE is making the error 
handling much more complex.

I think for this double freeing between the child and parent cleanup, 
the child should clear the bitmap after doing the cleanup.

Thanks again for exposing this one,
Qu

> 
>> +	}
>> +	btrfs_dec_block_group_reservations(fs_info, ins->objectid);
>> +	btrfs_unlock_extent(&inode->io_tree, file_offset, cur_end, cached);
>> +
>> +	child->ordered = ordered;
>> +	child->private = parent->private;
>> +	child->end_io = end_bbio_delayed_uncompressed;
>> +	child->bio.bi_iter.bi_sector = ins->objectid >> SECTOR_SHIFT;
>> +	atomic_inc(&parent->pending_ios);
>> +	btrfs_submit_bbio(child, 0);
>> +	*ret_alloc_size = cur_len;
>> +	return 0;
>> +
>> +free_reserved:
>> +	if (child) {
>> +		struct folio_iter fi;
>> +
>> +		bio_for_each_folio_all(fi, &child->bio)
>> +			folio_put(fi.folio);
>> +		bio_put(&child->bio);
>> +	}
>> +	btrfs_qgroup_free_data(inode, NULL, file_offset, cur_len, NULL);
>> +	btrfs_dec_block_group_reservations(fs_info, ins->objectid);
> 
> But we have already freed it here, so it is a double free.
> 
>> +	btrfs_free_reserved_extent(fs_info, ins->objectid, ins->offset, true);
>> +	ASSERT(ret != -EAGAIN);
>> +	return ret;
>> +}
>> +
>>   static void run_delayed_bbio(struct work_struct *work)
>>   {
>>   	struct delayed_bio_private *dbp = container_of(work, struct delayed_bio_private, work);
>>   	struct btrfs_bio *parent = dbp->delayed_bbio;
>> +	struct btrfs_key ins;
>> +	struct extent_state *cached = NULL;
>> +	const u32 uncompressed_size = bio_get_size(&parent->bio);
>> +	const u64 start = parent->file_offset;
>> +	const u64 end = start + uncompressed_size - 1;
>> +	u64 cur = start;
>> +	u64 alloc_hint;
>> +	int ret = 0;
>>   
>>   	if (try_submit_compressed(parent))
>>   		goto finish;
>>   
>> -	/* Uncompressed fallback is not yet implemented. */
>> -	ASSERT(0);
>> +	alloc_hint = btrfs_get_extent_allocation_hint(parent->inode, start,
>> +						      uncompressed_size);
>> +	while (cur < end) {
>> +		u32 cur_len;
>> +
> 
> (re patch 4: this is the missing second reserving call)
> 
>> +		ret = submit_one_uncompressed_range(parent, &ins, &cached,
>> +						    cur, end + 1 - cur,
>> +						    alloc_hint, &cur_len);
>> +		if (ret < 0) {
>> +			cmpxchg(&parent->status, BLK_STS_OK, errno_to_blk_status(ret));
>> +			goto finish;
>> +		}
>> +		cur += cur_len;
>> +		alloc_hint = ins.objectid + ins.offset;
>> +	}
>>   
>>   finish:
>>   	/*
>> -- 
>> 2.55.0
>>