btrfs: used compressed_bio structure for read and write
Subsystems (2)
v2 cover btrfs: used compressed_bio structure for read and write
[CHANGELOG]
v2:
- Fix an error in error path of of compress_file_range()
If btrfs_compress_bio() returned an error, we should reset @cb to NULL before doing error handling, or cleanup_compressed_bio() can be called on an error pointer.
- Fix several bugs in zstd_compress_bio() which causes compression failure
There are several different bugs in the mostly copy-n-pasted code:
* Uncommon @start and @len usage
The old code allows @start and @len to be modified, which is against the more common practice nowadays.
* Fix a incorrect input buffer check
Which cause us to incorrectly end the compression early and fallback to uncompressed write.
I was never a huge fan of the current btrfs_compress_folios() interface:
- Complex and duplicated parameter list
* A folio array to hold all folios
Which means extra error handling.
* A @nr_folios pointer
That pointer is both input and output, representing the number of max folios, but also the number of compressed folios.
The number of input folios is not really necessary, it's always no larger than DIV_ROUND_UP(len, PAGE_SIZE) in the first place.
* A @total_in pointer
Again an pointer as both input and output, representing the filemap range length, and how many bytes are compressed in this run.
However if we failed to compress the full range, all supported algorithms will return an error, thus fallback to uncompressed path.
Thus there is no need to use it as an output pointer.
* A @total_compressed point
Again an pointer as both input and output, representing the max number of compressed size, and the final compressed size.
However we do not need it as an input at all, we always error out if the compressed size is larger than the original size.
- Extra error cleanup handling
We need to cleanup the compressed_folios[] array during error handling.
Replace the old btrfs_compress_folios() interface with btrfs_compress_bio(), which has the following benefits:
- Simplified parameter list
* inode
* start
* len
* compress_type
* compress_level
* write_flags
No parameter is sharing input and output members, and all are very straightforward (except the last write_flags, which is just an extra bio flag).
- Directly return a compressed_bio structure
With minor modifications, that pointer can be passed to btrfs_submit_bio().
The caller still needs to do proper round up and fill the proper disk_bytenr/num_bytes before submission.
And for error handling, simply call cleanup_compressed_bio() then everything is cleaned up properly (at least I hope so).
- No more extra folios array passing and error handling
Qu Wenruo (9):
btrfs: introduce lzo_compress_bio() helper
btrfs: introduce zstd_compress_bio() helper
btrfs: introduce zlib_compress_bio() helper
btrfs: introduce btrfs_compress_bio() helper
btrfs: switch to btrfs_compress_bio() interface for compressed writes
btrfs: remove the old btrfs_compress_folios() infrastructures
btrfs: get rid of compressed_folios[] usage for compressed read
btrfs: get rid of compressed_folios[] usage for encoded writes
btrfs: get rid of compressed_bio::compressed_folios[]
fs/btrfs/compression.c | 205 +++++++++++++++++-------------------- fs/btrfs/compression.h | 40 ++++---- fs/btrfs/inode.c | 219 ++++++++++++++++++++-------------------- fs/btrfs/lzo.c | 223 +++++++++++++++++++++++++++-------------- fs/btrfs/zlib.c | 64 ++++++------ fs/btrfs/zstd.c | 98 +++++++++--------- 6 files changed, 450 insertions(+), 399 deletions(-)
Signature · 1 line
2.52.0
PATCH v2 1/9 btrfs: introduce lzo_compress_bio() helper
The new helper has the following enhancements against the existing lzo_compress_folios()
- Much smaller parameter list
No more shared IN/OUT members, no need to pre-allocate a compressed_folios[] array.
Just a workspace list header and a compressed_bio pointer.
Everything else can be fetched from that @cb pointer.
- Read-to-be-submitted compressed bio
Although the caller still needs to do some common works like rounding up and zeroing the tailing part of the last fs block.
Some workloads are specific to lzo that is not needed with other multi-run compression interfaces:
- Need to write a LZO header or segment header
Use the new write_and_queue_folio() helper to do the bio_add_folio() call and folio switching.
- Need to update the LZO header after compression is done
Use bio_first_folio_all() to grab the first folio and update the header.
- Extra corner case of error handling
This can happen when we have queued part of a folio and hit an error. In that case those folios will be released by the bio. Thus we can only release the folio that has no queued part.
--- fs/btrfs/compression.h | 1 + fs/btrfs/lzo.c | 261 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 262 insertions(+)
fs/btrfs/compression.h +1 -0
@@ -161,6 +161,7 @@ struct list_head *zlib_get_workspace(struct btrfs_fs_info *fs_info, unsigned intint lzo_compress_folios (struct list_head *ws,struct btrfs_inode *inode,u64 start,struct folio **folios,unsigned long *out_folios,unsigned long *total_in,unsigned long *total_out); +int lzo_compress_bio (struct list_head *ws,struct compressed_bio *cb);int lzo_decompress_bio (struct list_head *ws,struct compressed_bio *cb); int lzo_decompress(struct list_head *ws, const u8 *data_in, struct folio *dest_folio, unsigned long dest_pgoff, size_t srclen,
fs/btrfs/lzo.c +261 -0
273 lines not on this page · raw message
Signature · 1 line
2.52.0
PATCH v2 2/9 btrfs: introduce zstd_compress_bio() helper
In reply to Qu WenruoThe new helper has the following enhancements against the existing zstd_compress_folios()
- Much smaller parameter list
No more shared IN/OUT members, no need to pre-allocate a compressed_folios[] array.
Just a workspace and compressed_bio pointer, everything we need can be extracted from that @cb pointer.
- Ready-to-be-submitted compressed bio
Although the caller still needs to do some common works like rounding up and zeroing the tailing part of the last fs block.
Overall the workflow is the same as zstd_compress_folios(), but with some minor changes:
- @start/@len is now constant
For the current input file offset, use @start + @tot_in instead.
The original change of @start and @len makes it pretty hard to know what value we're really comparing to.
- No more @cur_len
It's only utilized when switching input buffer.
Directly use btrfs_calc_input_length() instead.
--- fs/btrfs/compression.h | 1 + fs/btrfs/zstd.c | 185 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 186 insertions(+)
fs/btrfs/compression.h +1 -0
@@ -172,6 +172,7 @@ void lzo_free_workspace(struct list_head *ws);int zstd_compress_folios (struct list_head *ws,struct btrfs_inode *inode,u64 start,struct folio **folios,unsigned long *out_folios,unsigned long *total_in,unsigned long *total_out); +int zstd_compress_bio (struct list_head *ws,struct compressed_bio *cb);int zstd_decompress_bio (struct list_head *ws,struct compressed_bio *cb); int zstd_decompress(struct list_head *ws, const u8 *data_in, struct folio *dest_folio, unsigned long dest_pgoff, size_t srclen,
fs/btrfs/zstd.c +185 -0
191 lines not on this page · raw message
Signature · 1 line
2.52.0
PATCH v2 3/9 btrfs: introduce zlib_compress_bio() helper
In reply to Qu WenruoThe new helper has the following enhancements against the existing zlib_compress_folios()
- Much smaller parameter list
No more shared IN/OUT members, no need to pre-allocate a compressed_folios[] array.
Just a workspace and compressed_bio pointer, everything we need can be extracted from that @cb pointer.
- Ready-to-be-submitted compressed bio
Although the caller still needs to do some common works like rounding up and zeroing the tailing part of the last fs block.
--- fs/btrfs/compression.h | 1 + fs/btrfs/zlib.c | 193 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 194 insertions(+)
fs/btrfs/compression.h +1 -0
@@ -150,6 +150,7 @@ int btrfs_compress_filemap_get_folio(struct address_space *mapping, u64 start,int zlib_compress_folios (struct list_head *ws,struct btrfs_inode *inode,u64 start,struct folio **folios,unsigned long *out_folios,unsigned long *total_in,unsigned long *total_out); +int zlib_compress_bio (struct list_head *ws,struct compressed_bio *cb);int zlib_decompress_bio (struct list_head *ws,struct compressed_bio *cb); int zlib_decompress(struct list_head *ws, const u8 *data_in, struct folio *dest_folio, unsigned long dest_pgoff, size_t srclen,
fs/btrfs/zlib.c +193 -0
199 lines not on this page · raw message
Signature · 1 line
2.52.0
PATCH v2 4/9 btrfs: introduce btrfs_compress_bio() helper
In reply to Qu WenruoThe helper will allocate a new compressed_bio, do the compression, and return it to the caller.
This greatly simplifies the compression path, as we no longer need to allocate a folio array thus no extra error path, furthermore the compressed bio structure can be utilized for submission with very minor modifications (like rounding up the bi_size and populate the bi_sector).
--- fs/btrfs/compression.c | 63 ++++++++++++++++++++++++++++++++++++++++++ fs/btrfs/compression.h | 13 +++++++++ 2 files changed, 76 insertions(+)
fs/btrfs/compression.c +63 -0
@@ -1064,6 +1064,69 @@ int btrfs_compress_folios(unsigned int type, int level, struct btrfs_inode *inodreturn ret; } +/* +* Given an address space and start and length, compress the page cache +* contents into @cb. +* +* @type_level is encoded algorithm and level, where level 0 means whatever +* default the algorithm chooses and is opaque here; +* - compression algo are 0-3 +* - the level are bits 4-7 +* +* @cb->bbio.bio.bi_iter.bi_size will indicate the compressed data size. +* The bi_size may not be sectorsize aligned, thus the caller still need +* to do the round up before submission. +*/ +struct compressed_bio *btrfs_compress_bio (struct btrfs_inode *inode, +u64 start,u32 len,unsigned int type, +int level,blk_opf_t write_flags) +{ +struct btrfs_fs_info *fs_info = inode->root->fs_info; +struct list_head *workspace; +struct compressed_bio *cb; +int ret; + + cb =alloc_compressed_bio (inode, start,REQ_OP_WRITE | write_flags, + end_bbio_compressed_write); + cb->start = start; + cb->len = len; + cb->writeback = true; + cb->compress_type = type; + + level =btrfs_compress_set_level (type, level); + workspace =get_workspace (fs_info, type, level); +switch (type) { +case BTRFS_COMPRESS_ZLIB : + ret =zlib_compress_bio (workspace, cb); +break ; +case BTRFS_COMPRESS_LZO : + ret =lzo_compress_bio (workspace, cb); +break ; +case BTRFS_COMPRESS_ZSTD : + ret =zstd_compress_bio (workspace, cb); +break ; +case BTRFS_COMPRESS_NONE : +default : +/* +* This can happen when compression races with remount setting +* it to 'no compress', while caller doesn't call +* inode_need_compress() to check if we really need to +* compress. +* +* Not a big deal, just need to inform caller that we +* haven't allocated any pages yet. +*/ + ret = -E2BIG ; + } + +put_workspace (fs_info, type, workspace); +if (ret <0 ) { +cleanup_compressed_bio (cb); +return ERR_PTR (ret); + } +return cb; +} +static int btrfs_decompress_bio (struct compressed_bio *cb) {struct btrfs_fs_info *fs_info =cb_to_fs_info (cb);
fs/btrfs/compression.h +13 -0
@@ -146,6 +146,19 @@ int btrfs_compress_heuristic(struct btrfs_inode *inode, u64 start, u64 end);int btrfs_compress_filemap_get_folio (struct address_space *mapping,u64 start,struct folio **in_folio_ret); +struct compressed_bio *btrfs_compress_bio (struct btrfs_inode *inode, +u64 start,u32 len,unsigned int type, +int level,blk_opf_t write_flags); + +static inline void cleanup_compressed_bio (struct compressed_bio *cb) +{ +struct bio *bio = &cb->bbio.bio; +struct folio_iter fi; + +bio_for_each_folio_all (fi, bio) +folio_put (fi.folio); +bio_put (bio); +} int zlib_compress_folios(struct list_head *ws, struct btrfs_inode *inode, u64 start, struct folio **folios, unsigned long *out_folios,
Signature · 1 line
2.52.0
Re: [PATCH 4/9] btrfs: introduce btrfs_compress_bio() helper
Quoted 25 lines · Qu Wenruo
The helper will allocate a new
compressed_bio, do the compression, and return it to the caller.This greatly simplifies the compression path, as we no longer need to allocate a folio array thus no extra error path, furthermore the compressed bio structure can be utilized for submission with very minor modifications (like rounding up the
bi_sizeand populate thebi_sector).Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/compression.c | 63 ++++++++++++++++++++++++++++++++++++++++++ fs/btrfs/compression.h | 13 +++++++++ 2 files changed, 76 insertions(+)fs/btrfs/compression.c +3
-0@@ -1064,6 +1064,69 @@ int btrfs_compress_folios(unsigned int type, int level, struct btrfs_inode *inodreturn ret; } +/* +* Given an address space and start and length, compress the page cache +* contents into @cb.
fs/btrfs/compression.c int btrfs_compress_folios(unsigned int type, int level, struct btrfs_inode *inod · line 1079
Quoted 10 lines
fs/btrfs/compression.c +10
-0+* +* @type_level is encoded algorithm and level, where level 0 means whatever +* default the algorithm chooses and is opaque here; +* - compression algo are 0-3 +* - the level are bits 4-7 +* +* @cb->bbio.bio.bi_iter.bi_size will indicate the compressed data size. +* The bi_size may not be sectorsize aligned, thus the caller still need +* to do the round up before submission. +*/
I think it would be helpful to extend this documentation to include teh fact that it allocates the compressed folios as a side effect and that the caller is responsible for freeing them with the bio folio loop.
Quoted 60 lines
fs/btrfs/compression.c +50
-0+struct compressed_bio *btrfs_compress_bio (struct btrfs_inode *inode, +u64 start,u32 len,unsigned int type, +int level,blk_opf_t write_flags) +{ +struct btrfs_fs_info *fs_info = inode->root->fs_info; +struct list_head *workspace; +struct compressed_bio *cb; +int ret; + + cb =alloc_compressed_bio (inode, start,REQ_OP_WRITE | write_flags, + end_bbio_compressed_write); + cb->start = start; + cb->len = len; + cb->writeback = true; + cb->compress_type = type; + + level =btrfs_compress_set_level (type, level); + workspace =get_workspace (fs_info, type, level); +switch (type) { +case BTRFS_COMPRESS_ZLIB : + ret =zlib_compress_bio (workspace, cb); +break ; +case BTRFS_COMPRESS_LZO : + ret =lzo_compress_bio (workspace, cb); +break ; +case BTRFS_COMPRESS_ZSTD : + ret =zstd_compress_bio (workspace, cb); +break ; +case BTRFS_COMPRESS_NONE : +default : +/* +* This can happen when compression races with remount setting +* it to 'no compress', while caller doesn't call +* inode_need_compress() to check if we really need to +* compress. +* +* Not a big deal, just need to inform caller that we +* haven't allocated any pages yet. +*/ + ret = -E2BIG ; + } + +put_workspace (fs_info, type, workspace); +if (ret <0 ) { +cleanup_compressed_bio (cb); +return ERR_PTR (ret); + } +return cb; +} +static int btrfs_decompress_bio (struct compressed_bio *cb) {struct btrfs_fs_info *fs_info =cb_to_fs_info (cb);fs/btrfs/compression.h +0
-0@@ -146,6 +146,19 @@ int btrfs_compress_heuristic(struct btrfs_inode *inode, u64 start, u64 end); int btrfs_compress_filemap_get_folio(struct address_space *mapping, u64 start,
fs/btrfs/compression.h int btrfs_compress_heuristic(struct btrfs_inode *inode, u64 start, u64 end); · line 157
Quoted 10 lines
fs/btrfs/compression.h +9
-0struct folio **in_folio_ret); +struct compressed_bio *btrfs_compress_bio (struct btrfs_inode *inode, +u64 start,u32 len,unsigned int type, +int level,blk_opf_t write_flags); + +static inline void cleanup_compressed_bio (struct compressed_bio *cb) +{ +struct bio *bio = &cb->bbio.bio; +struct folio_iter fi; +
Isn't it possible to have an error half way through preparing the compressed folios and to have a number of folios on the bio which were allocated with btrfs_alloc_compr_folio so should be freed with btrfs_free_compr_folio to keep the compressed pool management accurate? Otherwise, I think we end up leaking the pool and not getting to use it ever again?
Or is the caller responsible for that even if we return an error? If so, why do we want to do an unconditional folio_put() here? Is there another get that I am missing?
Quoted 10 lines
fs/btrfs/compression.h +4
-0+bio_for_each_folio_all (fi, bio) +folio_put (fi.folio); +bio_put (bio); +} int zlib_compress_folios(struct list_head *ws, struct btrfs_inode *inode, u64 start, struct folio **folios, unsigned long *out_folios,Signature · 1 line
2.52.0
Re: [PATCH 4/9] btrfs: introduce btrfs_compress_bio() helper
Quoted 1 line · Boris Burkov
On Mon, Jan 26, 2026 at 01:54:11PM +1030, Qu Wenruo wrote:
[...]
Quoted 14 lines
Quoted 5 lines
+/*
+ * Given an address space and start and length, compress the page cache
+ * contents into @cb.
+ *
+ * @type_level is encoded algorithm and level, where level 0 means whateverQuoted 8 lines
+ * default the algorithm chooses and is opaque here;
+ * - compression algo are 0-3
+ * - the level are bits 4-7
+ *
+ * @cb->bbio.bio.bi_iter.bi_size will indicate the compressed data size.
+ * Thebi_sizemay not be sectorsize aligned, thus the caller still need
+ * to do the round up before submission.
+ */
Quoted 3 lines
I think it would be helpful to extend this documentation to include teh fact that it allocates the compressed folios as a side effect and that the caller is responsible for freeing them with the bio folio loop.
I don't think it's a side effect, it's the expected behavior. The same as the old btrfs_compress_folios().
And I agree with you that we should mention that folios are allocated during the function, and the allocation is through btrfs_alloc_compr_folio(), thus all those folios should also be freed by btrfs_free_compr_folio(), which is normally done by cleanup_compressed_bio().
[...]
Quoted 7 lines
Quoted 7 lines
fs/btrfs/compression.h +7
-0+struct compressed_bio *btrfs_compress_bio (struct btrfs_inode *inode, +u64 start,u32 len,unsigned int type, +int level,blk_opf_t write_flags); + +static inline void cleanup_compressed_bio (struct compressed_bio *cb) +{ +struct bio *bio = &cb->bbio.bio;
Quoted 8 lines
fs/btrfs/compression.h int btrfs_compress_heuristic(struct btrfs_inode *inode, u64 start, u64 end); · line 157
Quoted 2 lines
fs/btrfs/compression.h +2
-0+struct folio_iter fi; +Isn't it possible to have an error half way through preparing the compressed folios and to have a number of folios on the bio which were allocated with
btrfs_alloc_compr_folioso should be freed withbtrfs_free_compr_folioto keep the compressed pool management accurate?
My bad, I missed this location, thanks a lot for point this out.
Will get it fix in the next up.
Thanks,
Qu
Quoted 18 lines
Otherwise, I think we end up leaking the pool and not getting to use it ever again?
Or is the caller responsible for that even if we return an error? If so, why do we want to do an unconditional
folio_put()here? Is there another get that I am missing?Quoted 10 lines
fs/btrfs/compression.h +4
-0+bio_for_each_folio_all (fi, bio) +folio_put (fi.folio); +bio_put (bio); +} int zlib_compress_folios(struct list_head *ws, struct btrfs_inode *inode, u64 start, struct folio **folios, unsigned long *out_folios,Signature · 1 line
2.52.0
PATCH v2 5/9 btrfs: switch to btrfs_compress_bio() interface for compressed writes
In reply to Qu WenruoThis switch has the following benefits:
- A single structure to handle all compression
No more extra members like compressed_folios[] nor compress_type, all those members.
This means the structure of async_extent is much smaller.
- Simpler error handling
A single cleanup_compressed_bio() will handle everything, no extra compressed_folios[] array to bother.
Some special notes:
- Compressed folios releasing
Now we go bio_for_each_folio_all() loop to release the folios of the bio. This will work for both the old compressed_folios[] array and the new pure bio method.
For old compressed_folios[], all folios of that array is queued into the bio, thus releasing the folios from the bio is the same as releasing each folio of that array. We jut need to be sure no double releasing from the array and bio.
For the new pure bio method, that array is NULL, just usual folio releasing of the bio.
- Rounding up the bio to block size
We can not simply increase bi_size, as that will not increase the length of the last bvec.
Thus we have to properly add the last part into the bio. This will be done by the helper, round_up_last_block().
--- fs/btrfs/compression.c | 17 ++--- fs/btrfs/inode.c | 161 +++++++++++++++++++---------------------- 2 files changed, 84 insertions(+), 94 deletions(-)
fs/btrfs/compression.c +8 -9
@@ -155,13 +155,6 @@ static int compression_decompress(int type, struct list_head *ws, } } -static void btrfs_free_compressed_folios (struct compressed_bio *cb) -{ -for (unsigned int i =0 ; i < cb->nr_folios; i++) -btrfs_free_compr_folio (cb->compressed_folios[i]); -kfree (cb->compressed_folios); -} -static int btrfs_decompress_bio (struct compressed_bio *cb);/*
@@ -270,12 +263,15 @@ static void end_bbio_compressed_read(struct btrfs_bio *bbio) {struct compressed_bio *cb =to_compressed_bio (bbio);blk_status_t status = bbio->bio.bi_status; +struct folio_iter fi;if (!status) status =errno_to_blk_status (btrfs_decompress_bio (cb)); -btrfs_free_compressed_folios (cb);btrfs_bio_end_io (cb->orig_bbio, status); +bio_for_each_folio_all (fi, &bbio->bio) +btrfs_free_compr_folio (fi.folio); +kfree (cb->compressed_folios);bio_put (&bbio->bio); }
@@ -326,6 +322,7 @@ static noinline void end_compressed_writeback(const struct compressed_bio *cb)static void end_bbio_compressed_write (struct btrfs_bio *bbio) {struct compressed_bio *cb =to_compressed_bio (bbio); +struct folio_iter fi;btrfs_finish_ordered_extent (cb->bbio.ordered,NULL , cb->start, cb->len, cb->bbio.bio.bi_status ==BLK_STS_OK );
@@ -333,7 +330,9 @@ static void end_bbio_compressed_write(struct btrfs_bio *bbio)if (cb->writeback)end_compressed_writeback (cb);/* Note, our inode could be gone now. */ -btrfs_free_compressed_folios (cb); +bio_for_each_folio_all (fi, &bbio->bio) +btrfs_free_compr_folio (fi.folio); +kfree (cb->compressed_folios);bio_put (&cb->bbio.bio); }
fs/btrfs/inode.c +76 -85
317 lines not on this page · raw message
Signature · 1 line
2.52.0
PATCH v2 6/9 btrfs: remove the old btrfs_compress_folios() infrastructures
In reply to Qu WenruoSince it's replaced by btrfs_compress_bio(), remove all involved functions.
--- fs/btrfs/compression.c | 70 --------------- fs/btrfs/compression.h | 12 --- fs/btrfs/inode.c | 2 +- fs/btrfs/lzo.c | 188 ---------------------------------------- fs/btrfs/zlib.c | 189 ----------------------------------------- fs/btrfs/zstd.c | 189 ----------------------------------------- 6 files changed, 1 insertion(+), 649 deletions(-)
fs/btrfs/compression.c +0 -70
@@ -86,37 +86,6 @@ bool btrfs_compress_is_valid_type(const char *str, size_t len)return false; } -static int compression_compress_pages (int type,struct list_head *ws, -struct btrfs_inode *inode,u64 start, -struct folio **folios,unsigned long *out_folios, -unsigned long *total_in,unsigned long *total_out) -{ -switch (type) { -case BTRFS_COMPRESS_ZLIB : -return zlib_compress_folios (ws, inode, start, folios, - out_folios, total_in, total_out); -case BTRFS_COMPRESS_LZO : -return lzo_compress_folios (ws, inode, start, folios, - out_folios, total_in, total_out); -case BTRFS_COMPRESS_ZSTD : -return zstd_compress_folios (ws, inode, start, folios, - out_folios, total_in, total_out); -case BTRFS_COMPRESS_NONE : -default : -/* -* This can happen when compression races with remount setting -* it to 'no compress', while caller doesn't call -* inode_need_compress() to check if we really need to -* compress. -* -* Not a big deal, just need to inform caller that we -* haven't allocated any pages yet. -*/ - *out_folios =0 ; -return -E2BIG ; - } -} -static int compression_decompress_bio (struct list_head *ws,struct compressed_bio *cb) {
@@ -1024,45 +993,6 @@ int btrfs_compress_filemap_get_folio(struct address_space *mapping, u64 start,return 0 ; } -/* -* Given an address space and start and length, compress the bytes into @pages -* that are allocated on demand. -* -* @type_level is encoded algorithm and level, where level 0 means whatever -* default the algorithm chooses and is opaque here; -* - compression algo are 0-3 -* - the level are bits 4-7 -* -* @out_folios is an in/out parameter, holds maximum number of folios to allocate -* and returns number of actually allocated folios -* -* @total_in is used to return the number of bytes actually read. It -* may be smaller than the input length if we had to exit early because we -* ran out of room in the folios array or because we cross the -* max_out threshold. -* -* @total_out is an in/out parameter, must be set to the input length and will -* be also used to return the total number of compressed bytes -*/ -int btrfs_compress_folios (unsigned int type,int level,struct btrfs_inode *inode, -u64 start,struct folio **folios,unsigned long *out_folios, -unsigned long *total_in,unsigned long *total_out) -{ -struct btrfs_fs_info *fs_info = inode->root->fs_info; -const unsigned long orig_len = *total_out; -struct list_head *workspace; -int ret; - - level =btrfs_compress_set_level (type, level); - workspace =get_workspace (fs_info, type, level); - ret =compression_compress_pages (type, workspace, inode, start, folios, - out_folios, total_in, total_out); -/* The total read-in bytes should be no larger than the input. */ -ASSERT (*total_in <= orig_len); -put_workspace (fs_info, type, workspace); -return ret; -} -/* * Given an address space and start and length, compress the page cache * contents into @cb.
fs/btrfs/compression.h +0 -12
@@ -91,9 +91,6 @@ int __init btrfs_init_compress(void);void __cold btrfs_exit_compress (void );bool btrfs_compress_level_valid (unsigned int type,int level); -int btrfs_compress_folios (unsigned int type,int level,struct btrfs_inode *inode, -u64 start,struct folio **folios,unsigned long *out_folios, -unsigned long *total_in,unsigned long *total_out);int btrfs_decompress (int type,const u8 *data_in,struct folio *dest_folio,unsigned long dest_pgoff,size_t srclen,size_t destlen); int btrfs_decompress_buf2page(const char *buf, u32 buf_len,
@@ -160,9 +157,6 @@ static inline void cleanup_compressed_bio(struct compressed_bio *cb)bio_put (bio); } -int zlib_compress_folios (struct list_head *ws,struct btrfs_inode *inode, -u64 start,struct folio **folios,unsigned long *out_folios, -unsigned long *total_in,unsigned long *total_out);int zlib_compress_bio (struct list_head *ws,struct compressed_bio *cb);int zlib_decompress_bio (struct list_head *ws,struct compressed_bio *cb); int zlib_decompress(struct list_head *ws, const u8 *data_in,
@@ -172,9 +166,6 @@ struct list_head *zlib_alloc_workspace(struct btrfs_fs_info *fs_info, unsigned ivoid zlib_free_workspace (struct list_head *ws);struct list_head *zlib_get_workspace (struct btrfs_fs_info *fs_info,unsigned int level); -int lzo_compress_folios (struct list_head *ws,struct btrfs_inode *inode, -u64 start,struct folio **folios,unsigned long *out_folios, -unsigned long *total_in,unsigned long *total_out);int lzo_compress_bio (struct list_head *ws,struct compressed_bio *cb);int lzo_decompress_bio (struct list_head *ws,struct compressed_bio *cb); int lzo_decompress(struct list_head *ws, const u8 *data_in,
@@ -183,9 +174,6 @@ int lzo_decompress(struct list_head *ws, const u8 *data_in,struct list_head *lzo_alloc_workspace (struct btrfs_fs_info *fs_info);void lzo_free_workspace (struct list_head *ws); -int zstd_compress_folios (struct list_head *ws,struct btrfs_inode *inode, -u64 start,struct folio **folios,unsigned long *out_folios, -unsigned long *total_in,unsigned long *total_out);int zstd_compress_bio (struct list_head *ws,struct compressed_bio *cb);int zstd_decompress_bio (struct list_head *ws,struct compressed_bio *cb); int zstd_decompress(struct list_head *ws, const u8 *data_in,
fs/btrfs/inode.c +1 -1
@@ -943,7 +943,7 @@ static void compress_file_range(struct btrfs_work *work)/* * All the folios should have been locked thus no failure. * -* And even if some folios are missing, btrfs_compress_folios() +* And even if some folios are missing, btrfs_compress_bio() * would handle them correctly, so here just do an ASSERT() check for * early logic errors. */
fs/btrfs/lzo.c +0 -188
200 lines not on this page · raw message
fs/btrfs/zlib.c +0 -189
195 lines not on this page · raw message
fs/btrfs/zstd.c +0 -189
195 lines not on this page · raw message
Signature · 1 line
2.52.0
PATCH v2 7/9 btrfs: get rid of compressed_folios[] usage for compressed read
In reply to Qu WenruoCurrently btrfs_submit_compressed_read() still uses compressed_bio::compressed_folios[] array.
Change it to allocate each folio and queue them into the compressed bio so that we do not need to allocate that array.
This removes one error path, and paves the way to completely remove compressed_folios[] array.
--- fs/btrfs/compression.c | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-)
fs/btrfs/compression.c +21 -19
@@ -538,13 +538,13 @@ void btrfs_submit_compressed_read(struct btrfs_bio *bbio)struct extent_map_tree *em_tree = &inode->extent_tree;struct compressed_bio *cb;unsigned int compressed_len; +const u32 min_folio_size =btrfs_min_folio_size (fs_info);u64 file_offset = bbio->file_offset;u64 em_len;u64 em_start;struct extent_map *em;unsigned long pflags;int memstall =0 ; -blk_status_t status;int ret;/* we need the actual starting offset of this extent in the file */
@@ -552,7 +552,7 @@ void btrfs_submit_compressed_read(struct btrfs_bio *bbio) em =btrfs_lookup_extent_mapping (em_tree, file_offset, fs_info->sectorsize);read_unlock (&em_tree->lock);if (!em) { - status =BLK_STS_IOERR ; + ret = -EIO ; goto out; }
@@ -574,27 +574,31 @@ void btrfs_submit_compressed_read(struct btrfs_bio *bbio)btrfs_free_extent_map (em); - cb->nr_folios =DIV_ROUND_UP (compressed_len,btrfs_min_folio_size (fs_info)); - cb->compressed_folios =kcalloc (cb->nr_folios,sizeof (struct folio *),GFP_NOFS ); -if (!cb->compressed_folios) { - status =BLK_STS_RESOURCE ; - goto out_free_bio; - } +for (int i =0 ; i * min_folio_size < compressed_len; i++) { +struct folio *folio; +u32 cur_len =min (compressed_len - i * min_folio_size, + min_folio_size); - ret =btrfs_alloc_folio_array (cb->nr_folios, fs_info->block_min_order, - cb->compressed_folios); -if (ret) { - status =BLK_STS_RESOURCE ; - goto out_free_compressed_pages; + folio =btrfs_alloc_compr_folio (fs_info); +if (!folio) { + ret = -ENOMEM ; + goto out_free_bio; + } + + ret =bio_add_folio (&cb->bbio.bio, folio, cur_len,0 ); +if (unlikely (!ret)) { +folio_put (folio); + ret = -EINVAL ; + goto out_free_bio; + } } +ASSERT (cb->bbio.bio.bi_iter.bi_size == compressed_len);add_ra_bio_pages (&inode->vfs_inode, em_start + em_len, cb, &memstall, &pflags); -/* include any pages we added in add_ra-bio_pages */ cb->len = bbio->bio.bi_iter.bi_size; cb->bbio.bio.bi_iter.bi_sector = bbio->bio.bi_iter.bi_sector; -btrfs_add_compressed_bio_folios (cb);if (memstall)psi_memstall_leave (&pflags);
@@ -602,12 +606,10 @@ void btrfs_submit_compressed_read(struct btrfs_bio *bbio)btrfs_submit_bbio (&cb->bbio,0 );return ; -out_free_compressed_pages: -kfree (cb->compressed_folios); out_free_bio: -bio_put (&cb->bbio.bio); +cleanup_compressed_bio (cb); out: -btrfs_bio_end_io (bbio, status); +btrfs_bio_end_io (bbio,errno_to_blk_status (ret)); }/*
Signature · 1 line
2.52.0
PATCH v2 8/9 btrfs: get rid of compressed_folios[] usage for encoded writes
In reply to Qu WenruoCurrently only encoded writes utilized btrfs_submit_compressed_write(), which utilized compressed_bio::compressed_folios[] array.
Change the only call site to call the new helper, btrfs_alloc_compressed_write(), to allocate a compressed bio, then queue needed folios into that bio, and finally call btrfs_submit_compressed_write() to submit the compreseed bio.
This change has one hidden benefit, previously we use btrfs_alloc_folio_array() for the folios of btrfs_submit_compressed_read(), which doesn't utilize the compression page pool for bs == ps cases.
Now we call btrfs_alloc_compr_folio() which will benefit from page pool.
The other obvious benefit is that we no longer need to allocate an array to hold all those folios, thus one less error path.
--- fs/btrfs/compression.c | 53 +++++++++++++++++---------------------- fs/btrfs/compression.h | 6 ++--- fs/btrfs/inode.c | 56 +++++++++++++++++++++++------------------- 3 files changed, 56 insertions(+), 59 deletions(-)
fs/btrfs/compression.c +22 -31
@@ -305,25 +305,6 @@ static void end_bbio_compressed_write(struct btrfs_bio *bbio)bio_put (&cb->bbio.bio); } -static void btrfs_add_compressed_bio_folios (struct compressed_bio *cb) -{ -struct bio *bio = &cb->bbio.bio; -u32 offset =0 ; -unsigned int findex =0 ; - -while (offset < cb->compressed_len) { -struct folio *folio = cb->compressed_folios[findex]; -u32 len =min_t (u32, cb->compressed_len - offset,folio_size (folio)); -int ret; - -/* Maximum compressed extent is smaller than bio size limit. */ - ret =bio_add_folio (bio, folio, len,0 ); -ASSERT (ret); - offset += len; - findex++; - } -} -/* * worker function to build and submit bios for previously compressed pages. * The corresponding pages in the inode should be marked for writeback
@@ -334,34 +315,44 @@ static void btrfs_add_compressed_bio_folios(struct compressed_bio *cb)* the end io hooks. */ void btrfs_submit_compressed_write (struct btrfs_ordered_extent *ordered, -struct folio **compressed_folios, -unsigned int nr_folios, -blk_opf_t write_flags, -bool writeback) +struct compressed_bio *cb) {struct btrfs_inode *inode = ordered->inode;struct btrfs_fs_info *fs_info = inode->root->fs_info; -struct compressed_bio *cb;ASSERT (IS_ALIGNED (ordered->file_offset, fs_info->sectorsize));ASSERT (IS_ALIGNED (ordered->num_bytes, fs_info->sectorsize)); +ASSERT (cb->writeback); - cb =alloc_compressed_bio (inode, ordered->file_offset, -REQ_OP_WRITE | write_flags, - end_bbio_compressed_write); cb->start = ordered->file_offset; cb->len = ordered->num_bytes; - cb->compressed_folios = compressed_folios; cb->compressed_len = ordered->disk_num_bytes; - cb->writeback = writeback; - cb->nr_folios = nr_folios; cb->bbio.bio.bi_iter.bi_sector = ordered->disk_bytenr >>SECTOR_SHIFT ; cb->bbio.ordered = ordered; -btrfs_add_compressed_bio_folios (cb);btrfs_submit_bbio (&cb->bbio,0 ); } +/* +* Allocate a compressed write bio for @inode file offset @start length @len. +* +* The caller still needs to properly queue all folios and populate involved +* members. +*/ +struct compressed_bio *btrfs_alloc_compressed_write (struct btrfs_inode *inode, +u64 start,u64 len) +{ +struct compressed_bio *cb; + + cb =alloc_compressed_bio (inode, start,REQ_OP_WRITE , + end_bbio_compressed_write); + cb->start = start; + cb->len = len; + cb->writeback = true; + +return cb; +} +/* * Add extra pages in the same compressed file extent so that we don't need to * re-read the same extent again and again.
fs/btrfs/compression.h +3 -3
@@ -96,10 +96,10 @@ int btrfs_decompress(int type, const u8 *data_in, struct folio *dest_folio,int btrfs_decompress_buf2page (const char *buf,u32 buf_len,struct compressed_bio *cb,u32 decompressed); +struct compressed_bio *btrfs_alloc_compressed_write (struct btrfs_inode *inode, +u64 start,u64 len);void btrfs_submit_compressed_write (struct btrfs_ordered_extent *ordered, -struct folio **compressed_folios, -unsigned int nr_folios,blk_opf_t write_flags, -bool writeback); +struct compressed_bio *cb);void btrfs_submit_compressed_read (struct btrfs_bio *bbio);int btrfs_compress_str2level (unsigned int type,const char *str,int *level_ret);
fs/btrfs/inode.c +31 -25
107 lines not on this page · raw message
Signature · 1 line
2.52.0
PATCH v2 9/9 btrfs: get rid of compressed_bio::compressed_folios[]
In reply to Qu WenruoNow there is no one utilizing that member, we can safely remove it along with compressed_bio::nr_folios member.
--- fs/btrfs/compression.c | 2 -- fs/btrfs/compression.h | 6 ------ 2 files changed, 8 deletions(-)
fs/btrfs/compression.c +0 -2
@@ -240,7 +240,6 @@ static void end_bbio_compressed_read(struct btrfs_bio *bbio)btrfs_bio_end_io (cb->orig_bbio, status);bio_for_each_folio_all (fi, &bbio->bio)btrfs_free_compr_folio (fi.folio); -kfree (cb->compressed_folios);bio_put (&bbio->bio); }
@@ -301,7 +300,6 @@ static void end_bbio_compressed_write(struct btrfs_bio *bbio)/* Note, our inode could be gone now. */ bio_for_each_folio_all (fi, &bbio->bio)btrfs_free_compr_folio (fi.folio); -kfree (cb->compressed_folios);bio_put (&cb->bbio.bio); }
fs/btrfs/compression.h +0 -6
@@ -42,12 +42,6 @@ static_assert((BTRFS_MAX_COMPRESSED % PAGE_SIZE) == 0);#define BTRFS_ZLIB_DEFAULT_LEVEL 3struct compressed_bio { -/* Number of compressed folios in the array. */ -unsigned int nr_folios; - -/* The folios with the compressed data on them. */ -struct folio **compressed_folios; -/* starting offset in the inode for our pages */ u64 start;
Signature · 1 line
2.52.0
Quoted 75 lines · Qu Wenruo
[CHANGELOG]
v2:
- Fix an error in error path of ofcompress_file_range()
Ifbtrfs_compress_bio()returned an error, we should reset @cb to NULL before doing error handling, orcleanup_compressed_bio()can be called on an error pointer.- Fix several bugs in
zstd_compress_bio()which causes compression failure
There are several different bugs in the mostly copy-n-pasted code:* Uncommon @start and @len usage
The old code allows @start and @len to be modified, which is against the more common practice nowadays.* Fix a incorrect input buffer check
Which cause us to incorrectly end the compression early and fallback to uncompressed write.I was never a huge fan of the current
btrfs_compress_folios()interface:- Complex and duplicated parameter list
* A folio array to hold all folios
Which means extra error handling.* A @nr_folios pointer
That pointer is both input and output, representing the number of max folios, but also the number of compressed folios.The number of input folios is not really necessary, it's always no larger than
DIV_ROUND_UP(len, PAGE_SIZE)in the first place.* A @total_in pointer
Again an pointer as both input and output, representing the filemap range length, and how many bytes are compressed in this run.However if we failed to compress the full range, all supported algorithms will return an error, thus fallback to uncompressed path.
Thus there is no need to use it as an output pointer.
* A @total_compressed point
Again an pointer as both input and output, representing the max number of compressed size, and the final compressed size.However we do not need it as an input at all, we always error out if the compressed size is larger than the original size.
- Extra error cleanup handling
We need to cleanup the compressed_folios[] array during error handling.
Replace the old
btrfs_compress_folios()interface withbtrfs_compress_bio(), which has the following benefits:- Simplified parameter list
* inode
* start
* len
*compress_type
*compress_level
*write_flagsNo parameter is sharing input and output members, and all are very straightforward (except the last
write_flags, which is just an extra bio flag).- Directly return a
compressed_biostructureWith minor modifications, that pointer can be passed to
btrfs_submit_bio().
Quoted 7 lines
The caller still needs to do proper round up and fill the proper disk_bytenr/num_bytes before submission.
And for error handling, simply call
cleanup_compressed_bio()then everything is cleaned up properly (at least I hope so).- No more extra folios array passing and error handling
I really like your new design! I am a little worried about making sure all the cleanup happens accurately across the change but I think this will be really nice. Storing the folios directly on the bio and using that to track them is indeed much cleaner, in my opinion.
I previously tried to send a patch to track compr_folio leaks, so maybe resurrecting that could be helpful (in CONFIG_BTRFS_DEBUG?) to make sure we do this safely?
Please feel free to lift/borrow/steal/be-inspired-by/whatever:
https://lore.kernel.org/linux-btrfs/95f5c89f52556f69decc7f18a6fd1f2c09d711c9.1747095560.git.boris@bur.io/
Also, the patch series did not successfully apply for me on btrfs/for-next and since it is a pretty large change that made it harder to do a really thorough review.
Thanks,
Boris
Quoted 23 lines
Qu Wenruo (9):
btrfs: introducelzo_compress_bio()helper
btrfs: introducezstd_compress_bio()helper
btrfs: introducezlib_compress_bio()helper
btrfs: introducebtrfs_compress_bio()helper
btrfs: switch tobtrfs_compress_bio()interface for compressed writes
btrfs: remove the oldbtrfs_compress_folios()infrastructures
btrfs: get rid of compressed_folios[] usage for compressed read
btrfs: get rid of compressed_folios[] usage for encoded writes
btrfs: get rid of compressed_bio::compressed_folios[]fs/btrfs/compression.c | 205 +++++++++++++++++-------------------- fs/btrfs/compression.h | 40 ++++---- fs/btrfs/inode.c | 219 ++++++++++++++++++++-------------------- fs/btrfs/lzo.c | 223 +++++++++++++++++++++++++++-------------- fs/btrfs/zlib.c | 64 ++++++------ fs/btrfs/zstd.c | 98 +++++++++--------- 6 files changed, 450 insertions(+), 399 deletions(-)Signature · 1 line
2.52.0