* [PATCH 1/2] btrfs: fix off-by-one end related to inode_need_compress()
2026-09-10 2:05 [PATCH 0/2] btrfs: fix and simplify heuristic_collect_sample() Qu Wenruo
@ 2026-09-10 2:05 ` Qu Wenruo
2026-09-10 2:05 ` [PATCH 2/2] btrfs: simplify heuristic_collect_sample() to handle large folios better Qu Wenruo
1 sibling, 0 replies; 3+ messages in thread
From: Qu Wenruo @ 2026-09-10 2:05 UTC (permalink / raw)
To: linux-btrfs
In most cases btrfs uses @end as the inclusive end bytenr for a range,
and this applies to inode_need_compress().
However we have several sites not following the inclusive bytenr:
- run_delalloc_inline()
Which assigned @blocksize as @end for inode_need_compress()
This makes inode_need_compress() always skip the disk_i_size check.
- heuristic_collect_sample()
Which assigned "start + BTRFS_MAX_UNCOMPRESSED" to @end, which is
the exclusive bytenr.
Neither is really causing any real problem, as
heuristic_collect_sample() has proper checks to avoid reading anything
beyond @end, and the sampling read size is 16 bytes, so it has enough
headroom to handle that off-by-one problem.
But still I do not like anything out of the common scheme, so fix the
off-by-one @end for both call sites, and add extra ASSERT()s to catch
such unaligned parameters.
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
fs/btrfs/compression.c | 15 +++++++--------
fs/btrfs/inode.c | 5 ++++-
2 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 20169d028961..0134b9253702 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -1488,11 +1488,14 @@ static bool sample_repeated_patterns(struct heuristic_ws *ws)
static void heuristic_collect_sample(struct inode *inode, u64 start, u64 end,
struct heuristic_ws *ws)
{
+ const u32 blocksize = BTRFS_I(inode)->root->fs_info->sectorsize;
struct folio *folio;
pgoff_t index, index_end;
u32 i, curr_sample_pos;
u8 *in_data;
+ ASSERT(IS_ALIGNED(start, blocksize) && IS_ALIGNED(end + 1, blocksize));
+
/*
* Compression handles the input data by chunks of 128KiB
* (defined by BTRFS_MAX_UNCOMPRESSED)
@@ -1502,18 +1505,14 @@ static void heuristic_collect_sample(struct inode *inode, u64 start, u64 end,
* MAX_SAMPLE_SIZE - calculated under assumption that heuristic will
* process no more than BTRFS_MAX_UNCOMPRESSED at a time.
*/
- if (end - start > BTRFS_MAX_UNCOMPRESSED)
- end = start + BTRFS_MAX_UNCOMPRESSED;
+ if (end + 1 - start > BTRFS_MAX_UNCOMPRESSED)
+ end = start + BTRFS_MAX_UNCOMPRESSED - 1;
index = start >> PAGE_SHIFT;
index_end = end >> PAGE_SHIFT;
- /* Don't miss unaligned end */
- if (!PAGE_ALIGNED(end))
- index_end++;
-
curr_sample_pos = 0;
- while (index < index_end) {
+ while (index <= index_end) {
folio = filemap_get_folio(inode->i_mapping, index);
ASSERT(!IS_ERR(folio));
in_data = kmap_local_folio(folio,
@@ -1522,7 +1521,7 @@ static void heuristic_collect_sample(struct inode *inode, u64 start, u64 end,
i = start % PAGE_SIZE;
while (i < PAGE_SIZE - SAMPLING_READ_SIZE) {
/* Don't sample any garbage from the last page */
- if (start > end - SAMPLING_READ_SIZE)
+ if (start > end + 1 - SAMPLING_READ_SIZE)
break;
memcpy(&ws->sample[curr_sample_pos], &in_data[i],
SAMPLING_READ_SIZE);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index a85a7c561cf8..79f2181dc631 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -730,6 +730,9 @@ static inline int inode_need_compress(struct btrfs_inode *inode, u64 start,
u64 end, bool check_inline)
{
struct btrfs_fs_info *fs_info = inode->root->fs_info;
+ const u32 blocksize = fs_info->sectorsize;
+
+ ASSERT(IS_ALIGNED(start, blocksize) && IS_ALIGNED(end + 1, blocksize));
if (unlikely(!btrfs_inode_can_compress(inode))) {
DEBUG_WARN("BTRFS: unexpected compression for ino %llu", btrfs_ino(inode));
@@ -2331,7 +2334,7 @@ static int run_delalloc_inline(struct btrfs_inode *inode, struct folio *locked_f
btrfs_check_folio_write_protected(locked_folio);
if (btrfs_inode_can_compress(inode) &&
- inode_need_compress(inode, 0, blocksize, true)) {
+ inode_need_compress(inode, 0, blocksize - 1, true)) {
if (inode->defrag_compress > 0 &&
inode->defrag_compress < BTRFS_NR_COMPRESS_TYPES) {
compress_type = inode->defrag_compress;
--
2.55.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH 2/2] btrfs: simplify heuristic_collect_sample() to handle large folios better
2026-09-10 2:05 [PATCH 0/2] btrfs: fix and simplify heuristic_collect_sample() Qu Wenruo
2026-09-10 2:05 ` [PATCH 1/2] btrfs: fix off-by-one end related to inode_need_compress() Qu Wenruo
@ 2026-09-10 2:05 ` Qu Wenruo
1 sibling, 0 replies; 3+ messages in thread
From: Qu Wenruo @ 2026-09-10 2:05 UTC (permalink / raw)
To: linux-btrfs
Currently heuristic_collect_sample() is purely page size based, and it
has a lot of extra handling just inside the page.
However we already have large folio support, there is no need to look up
the same folio again and again.
Simplify the handling by:
- Use @cur as the iterator instead of page index
- Handle the sample copying on a per-folio basis
Although kmap_local_folio() requires an offset to handle HIGHMEM
page mapping, we have rejected large folios for HIGHMEM systems
completely.
So we can safely handle all sample copying inside the folio in one
go.
- Remove unnecessary unaligned range handling
All the range passed in should be block aligned, thus there is no need
to handle cases where sample crosses the block boundary.
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
fs/btrfs/compression.c | 40 ++++++++++++++++------------------------
1 file changed, 16 insertions(+), 24 deletions(-)
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 0134b9253702..ad90e14032db 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -1489,10 +1489,8 @@ static void heuristic_collect_sample(struct inode *inode, u64 start, u64 end,
struct heuristic_ws *ws)
{
const u32 blocksize = BTRFS_I(inode)->root->fs_info->sectorsize;
- struct folio *folio;
- pgoff_t index, index_end;
- u32 i, curr_sample_pos;
- u8 *in_data;
+ u64 cur = start;
+ u32 curr_sample_pos = 0;
ASSERT(IS_ALIGNED(start, blocksize) && IS_ALIGNED(end + 1, blocksize));
@@ -1508,33 +1506,27 @@ static void heuristic_collect_sample(struct inode *inode, u64 start, u64 end,
if (end + 1 - start > BTRFS_MAX_UNCOMPRESSED)
end = start + BTRFS_MAX_UNCOMPRESSED - 1;
- index = start >> PAGE_SHIFT;
- index_end = end >> PAGE_SHIFT;
+ while (cur < end) {
+ struct folio *folio;
+ void *in_data;
+ u64 next_pos;
- curr_sample_pos = 0;
- while (index <= index_end) {
- folio = filemap_get_folio(inode->i_mapping, index);
+ folio = filemap_get_folio(inode->i_mapping, cur >> PAGE_SHIFT);
+ /* All folios inside the range should exist and be locked. */
ASSERT(!IS_ERR(folio));
- in_data = kmap_local_folio(folio,
- offset_in_folio(folio, (u64)index << PAGE_SHIFT));
- /* Handle case where the start is not aligned to PAGE_SIZE */
- i = start % PAGE_SIZE;
- while (i < PAGE_SIZE - SAMPLING_READ_SIZE) {
- /* Don't sample any garbage from the last page */
- if (start > end + 1 - SAMPLING_READ_SIZE)
- break;
- memcpy(&ws->sample[curr_sample_pos], &in_data[i],
- SAMPLING_READ_SIZE);
- i += SAMPLING_INTERVAL;
- start += SAMPLING_INTERVAL;
+ next_pos = min_t(u64, end + 1, folio_next_pos(folio));
+ in_data = kmap_local_folio(folio, 0);
+
+ for (; cur < next_pos; cur += SAMPLING_INTERVAL) {
+ memcpy(&ws->sample[curr_sample_pos],
+ in_data + offset_in_folio(folio, cur),
+ SAMPLING_READ_SIZE);
curr_sample_pos += SAMPLING_READ_SIZE;
}
kunmap_local(in_data);
folio_put(folio);
-
- index++;
+ cur = next_pos;
}
-
ws->sample_size = curr_sample_pos;
}
--
2.55.0
^ permalink raw reply related [flat|nested] 3+ messages in thread