[PATCH 0/2] btrfs: fix and simplify heuristic_collect_sample()

[PATCH 0/2] btrfs: fix and simplify heuristic_collect_sample()

[Qu Wenruo]

The first one fixes an off-by-one problem related to
inode_need_compress() and heuristic_collect_sample().
Thankfully that off-by-one is not causing any real problems.

The second one enhances heuristic_collect_sample() to handle large
folios in a more efficient way, inspired by the recent cleanup from Tal.

Qu Wenruo (2):
  btrfs: fix off-by-one end related to inode_need_compress()
  btrfs: simplify heuristic_collect_sample() to handle large folios
    better

 fs/btrfs/compression.c | 51 +++++++++++++++++-------------------------
 fs/btrfs/inode.c       |  5 ++++-
 2 files changed, 25 insertions(+), 31 deletions(-)

-- 
2.55.0

[PATCH 1/2] btrfs: fix off-by-one end related to inode_need_compress()

[Qu Wenruo]

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

[PATCH 2/2] btrfs: simplify heuristic_collect_sample() to handle large folios better

[Qu Wenruo]

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