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

[Boris Burkov <boris@bur.io>]

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.

> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/inode.c | 162 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 160 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index fbc0e5426d45..f47106c3f9ed 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7804,16 +7804,174 @@ static bool try_submit_compressed(struct btrfs_bio *parent)
>  	return false;
>  }
>  
> +static void end_bbio_delayed_uncompressed(struct btrfs_bio *bbio)
> +{
> +	struct delayed_bio_private *dbp = bbio->private;
> +	struct btrfs_bio *parent = dbp->delayed_bbio;
> +	struct folio_iter fi;
> +
> +	bio_for_each_folio_all(fi, &bbio->bio)
> +		folio_put(fi.folio);
> +	btrfs_bio_end_io(parent, bbio->bio.bi_status);
> +	bio_put(&bbio->bio);
> +}
> +
> +static struct btrfs_bio *child_bbio_from_page_cache(struct btrfs_bio *parent,
> +						    u64 fileoff, u32 len)
> +{
> +	struct btrfs_inode *inode = parent->inode;
> +	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> +	struct address_space *mapping = inode->vfs_inode.i_mapping;
> +	struct btrfs_bio *bbio;
> +	struct folio_iter fi;
> +	u64 cur = fileoff;
> +	int ret;
> +
> +	bbio = btrfs_bio_alloc(len >> fs_info->sectorsize_bits, REQ_OP_WRITE,
> +			       inode, fileoff, end_bbio_delayed_uncompressed,
> +			       parent->private);
> +
> +	while (cur < fileoff + len) {
> +		struct folio *folio;
> +		u32 cur_len;
> +		bool queued;
> +
> +		folio = filemap_get_folio(mapping, cur >> PAGE_SHIFT);
> +		if (IS_ERR(folio)) {
> +			ret = PTR_ERR(folio);
> +			goto error;
> +		}
> +		cur_len = min_t(u64, folio_next_pos(folio), fileoff + len) - cur;
> +		queued = bio_add_folio(&bbio->bio, folio, cur_len,
> +				    offset_in_folio(folio, cur));
> +		/* There should be enough slots for the bio. */
> +		if (WARN_ON(!queued)) {
> +			ret = -EIO;
> +			folio_put(folio);
> +			goto error;
> +		}
> +		cur += cur_len;
> +	}
> +
> +	return bbio;
> +error:
> +	bio_for_each_folio_all(fi, &bbio->bio)
> +		folio_put(fi.folio);
> +	bio_put(&bbio->bio);
> +	return ERR_PTR(ret);
> +}
> +
> +static int submit_one_uncompressed_range(struct btrfs_bio *parent, struct btrfs_key *ins,
> +					 struct extent_state **cached, u64 file_offset,
> +					 u32 num_bytes, u64 alloc_hint, u32 *ret_alloc_size)
> +{
> +	struct btrfs_inode *inode = parent->inode;
> +	struct btrfs_root *root = inode->root;
> +	struct btrfs_fs_info *fs_info = root->fs_info;
> +	struct btrfs_ordered_extent *ordered;
> +	struct btrfs_file_extent file_extent;
> +	struct btrfs_bio *child = NULL;
> +	struct extent_map *em;
> +	u64 cur_end;
> +	u32 cur_len = 0;
> +	int ret;
> +

Suppose the compressed try failed without reserving, like in the actual
compression checks, then this reservation is currently valid and
reasonable and not a double dip. (if compression succeeded and reserved
then failed, this is the problematic one)

> +	ret = btrfs_reserve_extent(root, num_bytes, num_bytes, fs_info->sectorsize,
> +				   0, alloc_hint, ins, true, true);
> +	if (ret < 0)
> +		return ret;
> +
> +	cur_len = ins->offset;
> +	cur_end = file_offset + cur_len - 1;
> +
> +	file_extent.disk_bytenr = ins->objectid;
> +	file_extent.disk_num_bytes = ins->offset;
> +	file_extent.num_bytes = ins->offset;
> +	file_extent.ram_bytes = ins->offset;
> +	file_extent.offset = 0;
> +	file_extent.compression = BTRFS_COMPRESS_NONE;
> +
> +	child = child_bbio_from_page_cache(parent, file_offset, cur_len);
> +	if (IS_ERR(child)) {
> +		ret = PTR_ERR(child);
> +		child = NULL;
> +		goto free_reserved;
> +	}
> +
> +	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)

> +	}
> +	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
>