Skip to contentSkip to conversation

btrfs: preparation patches for the incoming metadata folio conversion

PATCH Superseded Qu Wenruo 3 participants 9 messages Jul 11, 2023 Btrfs file system

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Qu Wenruo

author
#0 lore raw

cover btrfs: preparation patches for the incoming metadata folio conversion

[BACKGROUND]

Recently I'm checking on the feasibility on converting metadata handling to go a folio based solution.

The best part of using a single folio for metadata is, we can get rid of the complexity of cross-page handling, everything would be just a single memory operation on a continuous memory range.

[PITFALLS]

One of the biggest problem for metadata folio conversion is, we still need the current page based solution (or folios with order 0) as a fallback solution when we can not get a high order folio.

In that case, there would be a hell to handle the four different combinations (folio/folio, folio/page, page/folio, page/page) for extent buffer helpers involving two extent buffers.

[OBJECTIVE]

So this patchset is the preparation to reduce direct page operations for metadata.

The patchset would do this mostly be concentrate the operations to use the common helper, write_extent_buffer() and read_extent_buffer().

For bitmap operations it's much complex, thus this patchset refactor it completely to go a 3 part solution:

- Handle the first byte
- Handle the byte aligned ranges
- Handle the last byte

This needs more complex testing (which I failed several times during development) to prevent regression.

Finally there is only one function which can not be properly migrated, memmove_extent_buffer(), which has to use memmove() calls, thus must go per-page mapping handling.

Thankfully if we go folio in the end, the folio based handling would just be a single memmove(), thus it won't be too much burden.

Qu Wenruo (6):
btrfs: tests: enhance extent buffer bitmap tests
btrfs: refactor extent buffer bitmaps operations
btrfs: use write_extent_buffer() to implement
write_extent_buffer_*id()
btrfs: refactor memcpy_extent_buffer()
btrfs: refactor copy_extent_buffer_full()
btrfs: call copy_extent_buffer_full() inside
btrfs_clone_extent_buffer()

 fs/btrfs/extent_io.c             | 219 ++++++++++++++-----------------
 fs/btrfs/tests/extent-io-tests.c | 161 +++++++++++++++--------
 2 files changed, 204 insertions(+), 176 deletions(-)
Signature · 1 line
2.41.0
author
#1 lore raw

PATCH 1/6 btrfs: tests: enhance extent buffer bitmap tests

Enhance extent bitmap tests for the following aspects:

- Remove unnecessary @len from __test_eb_bitmaps() We can fetch the length from extent buffer

- Explicitly distinguish bit and byte length
Now every start/len inside bitmap tests would have either "byte_" or "bit_" prefix to make it more explicit.

- Better error reporting

If we have mismatch bits, the error report would dump the following contents:

* start bytenr
* bit number
* the full byte from bitmap
* the full byte from the extent

This is to save developers time so obvious problem can be found immediately

- Extract bitmap set/clear and check operation into two helpers This is to save some code lines, as we will have more tests to do.

- Add new tests

The following tests are added, mostly for the incoming extent bitmap accessor refactor:

* Set bits inside the same byte
* Clear bits inside the same byte
* Cross byte boundary set
* Cross byte boundary clear
* Cross multi-byte boundary set
* Cross multi-byte boundary clear

Those new tests have already saved my backend for the incoming extent buffer bitmap refactor.

---
 fs/btrfs/tests/extent-io-tests.c | 161 +++++++++++++++++++++----------
 1 file changed, 108 insertions(+), 53 deletions(-)
fs/btrfs/tests/extent-io-tests.c +108 -53
index f6bc6d738555..f97f344e28ab 100644
@@ -319,86 +319,138 @@ static int test_find_delalloc(u32 sectorsize)
 	return ret;
 }
 
-static int check_eb_bitmap(unsigned long *bitmap, struct extent_buffer *eb,
-			   unsigned long len)
+static int check_eb_bitmap(unsigned long *bitmap, struct extent_buffer *eb)
 {
 	unsigned long i;
 
-	for (i = 0; i < len * BITS_PER_BYTE; i++) {
+	for (i = 0; i < eb->len * BITS_PER_BYTE; i++) {
 		int bit, bit1;
 
 		bit = !!test_bit(i, bitmap);
 		bit1 = !!extent_buffer_test_bit(eb, 0, i);
 		if (bit1 != bit) {
-			test_err("bits do not match");
+			u8 has;
+			u8 expect;
+
+			read_extent_buffer(eb, &has, i / BITS_PER_BYTE, 1);
+			expect = bitmap_get_value8(bitmap, ALIGN(i, BITS_PER_BYTE));
+
+			test_err("bits not match, start byte 0 bit %lu, byte %lu has 0x%02x expect 0x%02x",
+				 i, i / BITS_PER_BYTE, has, expect);
 			return -EINVAL;
 		}
 
 		bit1 = !!extent_buffer_test_bit(eb, i / BITS_PER_BYTE,
 						i % BITS_PER_BYTE);
 		if (bit1 != bit) {
-			test_err("offset bits do not match");
+			u8 has;
+			u8 expect;
+
+			read_extent_buffer(eb, &has, i / BITS_PER_BYTE, 1);
+			expect = bitmap_get_value8(bitmap, ALIGN(i, BITS_PER_BYTE));
+
+			test_err("bits not match, start byte %lu bit %lu, byte %lu has 0x%02x expect 0x%02x",
+				 i / BITS_PER_BYTE, i % BITS_PER_BYTE,
+				 i / BITS_PER_BYTE, has, expect);
 			return -EINVAL;
 		}
 	}
 	return 0;
 }
 
-static int __test_eb_bitmaps(unsigned long *bitmap, struct extent_buffer *eb,
-			     unsigned long len)
+static int test_bitmap_set(const char *name, unsigned long *bitmap,
+			   struct extent_buffer *eb,
+			   unsigned long byte_start, unsigned long bit_start,
+			   unsigned long bit_len)
+{
+	int ret;
+
+	bitmap_set(bitmap, byte_start * BITS_PER_BYTE + bit_start, bit_len);
+	extent_buffer_bitmap_set(eb, byte_start, bit_start, bit_len);
+	ret = check_eb_bitmap(bitmap, eb);
+	if (ret < 0)
+		test_err("%s test failed", name);
+	return ret;
+}
+
+static int test_bitmap_clear(const char *name, unsigned long *bitmap,
+			     struct extent_buffer *eb,
+			     unsigned long byte_start, unsigned long bit_start,
+			     unsigned long bit_len)
+{
+	int ret;
+
+	bitmap_clear(bitmap, byte_start * BITS_PER_BYTE + bit_start, bit_len);
+	extent_buffer_bitmap_clear(eb, byte_start, bit_start, bit_len);
+	ret = check_eb_bitmap(bitmap, eb);
+	if (ret < 0)
+		test_err("%s test failed", name);
+	return ret;
+}
+static int __test_eb_bitmaps(unsigned long *bitmap, struct extent_buffer *eb)
 {
 	unsigned long i, j;
+	unsigned long byte_len = eb->len;
 	u32 x;
 	int ret;
 
-	memset(bitmap, 0, len);
-	memzero_extent_buffer(eb, 0, len);
-	if (memcmp_extent_buffer(eb, bitmap, 0, len) != 0) {
-		test_err("bitmap was not zeroed");
-		return -EINVAL;
-	}
-
-	bitmap_set(bitmap, 0, len * BITS_PER_BYTE);
-	extent_buffer_bitmap_set(eb, 0, 0, len * BITS_PER_BYTE);
-	ret = check_eb_bitmap(bitmap, eb, len);
-	if (ret) {
-		test_err("setting all bits failed");
+	ret = test_bitmap_clear("clear all run 1", bitmap, eb, 0, 0,
+				byte_len * BITS_PER_BYTE);
+	if (ret < 0)
 		return ret;
-	}
 
-	bitmap_clear(bitmap, 0, len * BITS_PER_BYTE);
-	extent_buffer_bitmap_clear(eb, 0, 0, len * BITS_PER_BYTE);
-	ret = check_eb_bitmap(bitmap, eb, len);
-	if (ret) {
-		test_err("clearing all bits failed");
+	ret = test_bitmap_set("set all", bitmap, eb, 0, 0,
+			      byte_len * BITS_PER_BYTE);
+	if (ret < 0)
+		return ret;
+
+	ret = test_bitmap_clear("clear all run 2", bitmap, eb, 0, 0,
+				byte_len * BITS_PER_BYTE);
+	if (ret < 0)
+		return ret;
+
+	ret = test_bitmap_set("same byte set", bitmap, eb, 0, 2, 4);
+	if (ret < 0)
+		return ret;
+
+	ret = test_bitmap_clear("same byte partial clear", bitmap, eb, 0, 4, 1);
+	if (ret < 0)
+		return ret;
+
+	ret = test_bitmap_set("cross byte set", bitmap, eb, 2, 4, 8);
+	if (ret < 0)
+		return ret;
+
+	ret = test_bitmap_set("cross multi byte set", bitmap, eb, 4, 4, 24);
+	if (ret < 0)
+		return ret;
+
+	ret = test_bitmap_clear("cross byte clear", bitmap, eb, 2, 6, 4);
+	if (ret < 0)
+		return ret;
+
+	ret = test_bitmap_clear("cross multi byte clear", bitmap, eb, 4, 6, 20);
+	if (ret < 0)
 		return ret;
-	}
 
 	/* Straddling pages test */
-	if (len > PAGE_SIZE) {
-		bitmap_set(bitmap,
-			(PAGE_SIZE - sizeof(long) / 2) * BITS_PER_BYTE,
-			sizeof(long) * BITS_PER_BYTE);
-		extent_buffer_bitmap_set(eb, PAGE_SIZE - sizeof(long) / 2, 0,
-					sizeof(long) * BITS_PER_BYTE);
-		ret = check_eb_bitmap(bitmap, eb, len);
-		if (ret) {
-			test_err("setting straddling pages failed");
+	if (byte_len > PAGE_SIZE) {
+		ret = test_bitmap_set("cross page set", bitmap, eb,
+				      PAGE_SIZE - sizeof(long) / 2, 0,
+				      sizeof(long) * BITS_PER_BYTE);
+		if (ret < 0)
 			return ret;
-		}
 
-		bitmap_set(bitmap, 0, len * BITS_PER_BYTE);
-		bitmap_clear(bitmap,
-			(PAGE_SIZE - sizeof(long) / 2) * BITS_PER_BYTE,
-			sizeof(long) * BITS_PER_BYTE);
-		extent_buffer_bitmap_set(eb, 0, 0, len * BITS_PER_BYTE);
-		extent_buffer_bitmap_clear(eb, PAGE_SIZE - sizeof(long) / 2, 0,
-					sizeof(long) * BITS_PER_BYTE);
-		ret = check_eb_bitmap(bitmap, eb, len);
-		if (ret) {
-			test_err("clearing straddling pages failed");
+		ret = test_bitmap_set("cross page set all", bitmap, eb, 0, 0,
+				      byte_len * BITS_PER_BYTE);
+		if (ret < 0)
+			return ret;
+
+		ret = test_bitmap_clear("cross page clear", bitmap, eb,
+				PAGE_SIZE - sizeof(long) / 2, 0,
+				sizeof(long) * BITS_PER_BYTE);
+		if (ret < 0)
 			return ret;
-		}
 	}
 
 	/*
@@ -406,9 +458,12 @@ static int __test_eb_bitmaps(unsigned long *bitmap, struct extent_buffer *eb,
 	 * something repetitive that could miss some hypothetical off-by-n bug.
 	 */
 	x = 0;
-	bitmap_clear(bitmap, 0, len * BITS_PER_BYTE);
-	extent_buffer_bitmap_clear(eb, 0, 0, len * BITS_PER_BYTE);
-	for (i = 0; i < len * BITS_PER_BYTE / 32; i++) {
+	ret = test_bitmap_clear("clear all run 3", bitmap, eb, 0, 0,
+				byte_len * BITS_PER_BYTE);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < byte_len * BITS_PER_BYTE / 32; i++) {
 		x = (0x19660dULL * (u64)x + 0x3c6ef35fULL) & 0xffffffffU;
 		for (j = 0; j < 32; j++) {
 			if (x & (1U << j)) {
@@ -418,7 +473,7 @@ static int __test_eb_bitmaps(unsigned long *bitmap, struct extent_buffer *eb,
 		}
 	}
 
-	ret = check_eb_bitmap(bitmap, eb, len);
+	ret = check_eb_bitmap(bitmap, eb);
 	if (ret) {
 		test_err("random bit pattern failed");
 		return ret;
@@ -456,7 +511,7 @@ static int test_eb_bitmaps(u32 sectorsize, u32 nodesize)
 		goto out;
 	}
 
-	ret = __test_eb_bitmaps(bitmap, eb, nodesize);
+	ret = __test_eb_bitmaps(bitmap, eb);
 	if (ret)
 		goto out;
 
@@ -473,7 +528,7 @@ static int test_eb_bitmaps(u32 sectorsize, u32 nodesize)
 		goto out;
 	}
 
-	ret = __test_eb_bitmaps(bitmap, eb, nodesize);
+	ret = __test_eb_bitmaps(bitmap, eb);
 out:
 	free_extent_buffer(eb);
 	kfree(bitmap);
Signature · 1 line
2.41.0
author
#2 lore raw

PATCH 2/6 btrfs: refactor extent buffer bitmaps operations

In reply to Qu Wenruo

[BACKGROUND]
Currently we handle extent bitmaps manually in extent_buffer_bitmap_set() and extent_buffer_bitmap_clear().

Although with various helper like eb_bitmap_offset() it's still a little messy to read.
The code seems to be a copy of bitmap_set(), but with all the cross-page handling embedded into the code.

[ENHANCEMENT]
This patch would enhance the readability by introducing two helpers:

- memset_extent_buffer()
To handle the byte aligned range, thus all the cross-page handling is done there.

- extent_buffer_get_byte()
This for the first and the last byte operation, which only needs to grab one byte, thus no need for any cross-page handling.

So we can split both extent_buffer_bitmap_set() and extent_buffer_bitmap_clear() into 3 parts:

- Handle the first byte
If the range fits inside the first byte, we can exit early.

- Handle the byte aligned part
This is the part which can have cross-page operations, and it would be handled by memset_extent_buffer().

- Handle the last byte

This refactor does not only make the code a little easier to read, but also make later folio/page switch much easier, as the switch only needs to be done inside memset_extent_buffer() and extent_buffer_get_byte().

---
 fs/btrfs/extent_io.c | 141 +++++++++++++++++++++----------------------
 1 file changed, 68 insertions(+), 73 deletions(-)
fs/btrfs/extent_io.c +68 -73
index a845a90d46f7..6a7abcbe6bec 100644
@@ -4229,32 +4229,30 @@ void write_extent_buffer(const struct extent_buffer *eb, const void *srcv,
 	}
 }
 
+static void memset_extent_buffer(const struct extent_buffer *eb, int c,
+				 unsigned long start, unsigned long len)
+{
+	unsigned long cur = start;
+
+	while (cur < start + len) {
+		int index = get_eb_page_index(cur);
+		int offset = get_eb_offset_in_page(eb, cur);
+		int cur_len = min(start + len - cur, PAGE_SIZE - offset);
+		struct page *page = eb->pages[index];
+
+		assert_eb_page_uptodate(eb, page);
+		memset(page_address(page) + offset, c, cur_len);
+
+		cur += cur_len;
+	}
+}
+
 void memzero_extent_buffer(const struct extent_buffer *eb, unsigned long start,
 		unsigned long len)
 {
-	size_t cur;
-	size_t offset;
-	struct page *page;
-	char *kaddr;
-	unsigned long i = get_eb_page_index(start);
-
 	if (check_eb_range(eb, start, len))
 		return;
-
-	offset = get_eb_offset_in_page(eb, start);
-
-	while (len > 0) {
-		page = eb->pages[i];
-		assert_eb_page_uptodate(eb, page);
-
-		cur = min(len, PAGE_SIZE - offset);
-		kaddr = page_address(page);
-		memset(kaddr + offset, 0, cur);
-
-		len -= cur;
-		offset = 0;
-		i++;
-	}
+	return memset_extent_buffer(eb, 0, start, len);
 }
 
 void copy_extent_buffer_full(const struct extent_buffer *dst,
@@ -4371,6 +4369,15 @@ int extent_buffer_test_bit(const struct extent_buffer *eb, unsigned long start,
 	return 1U & (kaddr[offset] >> (nr & (BITS_PER_BYTE - 1)));
 }
 
+static u8 *extent_buffer_get_byte(const struct extent_buffer *eb, unsigned long bytenr)
+{
+	unsigned long index = get_eb_page_index(bytenr);
+
+	if (check_eb_range(eb, bytenr, 1))
+		return NULL;
+	return page_address(eb->pages[index]) + get_eb_offset_in_page(eb, bytenr);
+}
+
 /*
  * Set an area of a bitmap to 1.
  *
@@ -4382,35 +4389,29 @@ int extent_buffer_test_bit(const struct extent_buffer *eb, unsigned long start,
 void extent_buffer_bitmap_set(const struct extent_buffer *eb, unsigned long start,
 			      unsigned long pos, unsigned long len)
 {
+	unsigned int first_byte = start + BIT_BYTE(pos);
+	unsigned int last_byte = start + BIT_BYTE(pos + len - 1);
+	bool same_byte = (first_byte == last_byte);
+	u8 mask = BITMAP_FIRST_BYTE_MASK(pos);
 	u8 *kaddr;
-	struct page *page;
-	unsigned long i;
-	size_t offset;
-	const unsigned int size = pos + len;
-	int bits_to_set = BITS_PER_BYTE - (pos % BITS_PER_BYTE);
-	u8 mask_to_set = BITMAP_FIRST_BYTE_MASK(pos);
 
-	eb_bitmap_offset(eb, start, pos, &i, &offset);
-	page = eb->pages[i];
-	assert_eb_page_uptodate(eb, page);
-	kaddr = page_address(page);
+	if (same_byte)
+		mask &= BITMAP_LAST_BYTE_MASK(pos + len);
 
-	while (len >= bits_to_set) {
-		kaddr[offset] |= mask_to_set;
-		len -= bits_to_set;
-		bits_to_set = BITS_PER_BYTE;
-		mask_to_set = ~0;
-		if (++offset >= PAGE_SIZE && len > 0) {
-			offset = 0;
-			page = eb->pages[++i];
-			assert_eb_page_uptodate(eb, page);
-			kaddr = page_address(page);
-		}
-	}
-	if (len) {
-		mask_to_set &= BITMAP_LAST_BYTE_MASK(size);
-		kaddr[offset] |= mask_to_set;
-	}
+	/* Handle the first byte. */
+	kaddr = extent_buffer_get_byte(eb, first_byte);
+	*kaddr |= mask;
+	if (same_byte)
+		return;
+
+	/* Handle the byte aligned part. */
+	ASSERT(first_byte + 1 <= last_byte);
+	memset_extent_buffer(eb, 0xff, first_byte + 1,
+			     last_byte - first_byte - 1);
+
+	/* Handle the last byte. */
+	kaddr = extent_buffer_get_byte(eb, last_byte);
+	*kaddr |= BITMAP_LAST_BYTE_MASK(pos + len);
 }
 
 
@@ -4426,35 +4427,29 @@ void extent_buffer_bitmap_clear(const struct extent_buffer *eb,
 				unsigned long start, unsigned long pos,
 				unsigned long len)
 {
+	int first_byte = start + BIT_BYTE(pos);
+	int last_byte = start + BIT_BYTE(pos + len - 1);
+	bool same_byte = (first_byte == last_byte);
+	u8 mask = BITMAP_FIRST_BYTE_MASK(pos);
 	u8 *kaddr;
-	struct page *page;
-	unsigned long i;
-	size_t offset;
-	const unsigned int size = pos + len;
-	int bits_to_clear = BITS_PER_BYTE - (pos % BITS_PER_BYTE);
-	u8 mask_to_clear = BITMAP_FIRST_BYTE_MASK(pos);
 
-	eb_bitmap_offset(eb, start, pos, &i, &offset);
-	page = eb->pages[i];
-	assert_eb_page_uptodate(eb, page);
-	kaddr = page_address(page);
+	if (same_byte)
+		mask &= BITMAP_LAST_BYTE_MASK(pos + len);
 
-	while (len >= bits_to_clear) {
-		kaddr[offset] &= ~mask_to_clear;
-		len -= bits_to_clear;
-		bits_to_clear = BITS_PER_BYTE;
-		mask_to_clear = ~0;
-		if (++offset >= PAGE_SIZE && len > 0) {
-			offset = 0;
-			page = eb->pages[++i];
-			assert_eb_page_uptodate(eb, page);
-			kaddr = page_address(page);
-		}
-	}
-	if (len) {
-		mask_to_clear &= BITMAP_LAST_BYTE_MASK(size);
-		kaddr[offset] &= ~mask_to_clear;
-	}
+	/* Handle the first byte. */
+	kaddr = extent_buffer_get_byte(eb, first_byte);
+	*kaddr &= ~mask;
+	if (same_byte)
+		return;
+
+	/* Handle the byte aligned part. */
+	ASSERT(first_byte + 1 <= last_byte);
+	memset_extent_buffer(eb, 0, first_byte + 1,
+			     last_byte - first_byte - 1);
+
+	/* Handle the last byte. */
+	kaddr = extent_buffer_get_byte(eb, last_byte);
+	*kaddr &= ~BITMAP_LAST_BYTE_MASK(pos + len);
 }
 
 static inline bool areas_overlap(unsigned long src, unsigned long dst, unsigned long len)
Signature · 1 line
2.41.0
author
#3 lore raw

PATCH 3/6 btrfs: use write_extent_buffer() to implement write_extent_buffer_*id()

In reply to Qu Wenruo

For helpers write_extent_buffer_chunk_tree_uuid() and write_extent_buffer_fsid(), they can be implemented by write_extent_buffer().

And those two helpers are not that hot, they only get called during initialization of a new tree block.

There is not much need for those slightly optimized versions.

This would make later page/folio switch much easier, as all change only need to happen in write_extent_buffer().

---
 fs/btrfs/extent_io.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)
fs/btrfs/extent_io.c +5 -12
index 6a7abcbe6bec..fef5a7b6c60a 100644
@@ -4178,23 +4178,16 @@ static void assert_eb_page_uptodate(const struct extent_buffer *eb,
 void write_extent_buffer_chunk_tree_uuid(const struct extent_buffer *eb,
 		const void *srcv)
 {
-	char *kaddr;
-
-	assert_eb_page_uptodate(eb, eb->pages[0]);
-	kaddr = page_address(eb->pages[0]) +
-		get_eb_offset_in_page(eb, offsetof(struct btrfs_header,
-						   chunk_tree_uuid));
-	memcpy(kaddr, srcv, BTRFS_FSID_SIZE);
+	write_extent_buffer(eb, srcv,
+			    offsetof(struct btrfs_header, chunk_tree_uuid),
+			    BTRFS_FSID_SIZE);
 }
 
 void write_extent_buffer_fsid(const struct extent_buffer *eb, const void *srcv)
 {
-	char *kaddr;
 
-	assert_eb_page_uptodate(eb, eb->pages[0]);
-	kaddr = page_address(eb->pages[0]) +
-		get_eb_offset_in_page(eb, offsetof(struct btrfs_header, fsid));
-	memcpy(kaddr, srcv, BTRFS_FSID_SIZE);
+	write_extent_buffer(eb, srcv, offsetof(struct btrfs_header, fsid),
+			    BTRFS_FSID_SIZE);
 }
 
 void write_extent_buffer(const struct extent_buffer *eb, const void *srcv,
Signature · 1 line
2.41.0
#4 lore raw

Re: [PATCH 3/6] btrfs: use write_extent_buffer() to implement write_extent_buffer_*id()

Quoted 24 lines
fs/btrfs/extent_io.c +3 -8
index 6a7abcbe6bec..fef5a7b6c60a 100644
@@ -4178,23 +4178,16 @@ static void assert_eb_page_uptodate(const struct extent_buffer *eb,
  void write_extent_buffer_chunk_tree_uuid(const struct extent_buffer *eb,
  		const void *srcv)
  {
-	char *kaddr;
-
-	assert_eb_page_uptodate(eb, eb->pages[0]);
-	kaddr = page_address(eb->pages[0]) +
-		get_eb_offset_in_page(eb, offsetof(struct btrfs_header,
-						   chunk_tree_uuid));
-	memcpy(kaddr, srcv, BTRFS_FSID_SIZE);
+	write_extent_buffer(eb, srcv,
+			    offsetof(struct btrfs_header, chunk_tree_uuid),
+			    BTRFS_FSID_SIZE);
  }
 
  void write_extent_buffer_fsid(const struct extent_buffer *eb, const void *srcv)
  {
-	char *kaddr;

fs/btrfs/extent_io.c static void assert_eb_page_uptodate(const struct extent_buffer *eb, · line 4193

Quoted 10 lines
fs/btrfs/extent_io.c +2 -4
 
-	assert_eb_page_uptodate(eb, eb->pages[0]);
-	kaddr = page_address(eb->pages[0]) +
-		get_eb_offset_in_page(eb, offsetof(struct btrfs_header, fsid));
-	memcpy(kaddr, srcv, BTRFS_FSID_SIZE);
+	write_extent_buffer(eb, srcv, offsetof(struct btrfs_header, fsid),
+			    BTRFS_FSID_SIZE);
  }
 
  void write_extent_buffer(const struct extent_buffer *eb, const void *srcv,

write_extent_buffer_chunk_tree_uuid() has only one caller in kernel now; perhaps inline the function into its only callsite? On the other hand, it has several more in -progs, so maybe the name is useful and it could be moved it into extent_io.h since it's such a thin wrapper around write_extent_buffer()?

write_extent_buffer_fsid() has three in kernel and five in -progs, maybe also into the .h?

#5 lore raw

Re: [PATCH 3/6] btrfs: use write_extent_buffer() to implement write_extent_buffer_*id()

Quoted 42 lines · Sweet Tea Dorminy
Quoted 27 lines
fs/btrfs/extent_io.c +0 -0
index 6a7abcbe6bec..fef5a7b6c60a 100644
@@ -4178,23 +4178,16 @@ static void assert_eb_page_uptodate(const
 struct extent_buffer *eb,

  void write_extent_buffer_chunk_tree_uuid(const struct extent_buffer *eb,
          const void *srcv)
  {
-    char *kaddr;
-
-    assert_eb_page_uptodate(eb, eb->pages[0]);
-    kaddr = page_address(eb->pages[0]) +
-        get_eb_offset_in_page(eb, offsetof(struct btrfs_header, -                           chunk_tree_uuid));
-    memcpy(kaddr, srcv, BTRFS_FSID_SIZE);
+    write_extent_buffer(eb, srcv,
+                offsetof(struct btrfs_header, chunk_tree_uuid), +                BTRFS_FSID_SIZE);
  }
  void write_extent_buffer_fsid(const struct extent_buffer *eb, const void *srcv)
  {
-    char *kaddr;
-    assert_eb_page_uptodate(eb, eb->pages[0]);

Quoted 8 lines

-    kaddr = page_address(eb->pages[0]) +
-        get_eb_offset_in_page(eb, offsetof(struct btrfs_header, fsid)); -    memcpy(kaddr, srcv, BTRFS_FSID_SIZE);
+    write_extent_buffer(eb, srcv, offsetof(struct btrfs_header, fsid), +                BTRFS_FSID_SIZE);
  }
  void write_extent_buffer(const struct extent_buffer *eb, const void *srcv,

write_extent_buffer_chunk_tree_uuid() has only one caller in kernel now; perhaps inline the function into its only callsite? On the other hand, it has several more in -progs, so maybe the name is useful and it could be moved it into extent_io.h since it's such a thin wrapper around write_extent_buffer()?

Quoted 2 lines

write_extent_buffer_fsid() has three in kernel and five in -progs, maybe also into the .h?

This sound very reasonable, would go this direction in the next update.

Thanks,
Qu

author
#6 lore raw

PATCH 4/6 btrfs: refactor memcpy_extent_buffer()

In reply to Qu Wenruo

[BACKGROUND]
Currently memcpy_extent_buffer() goes a loop where it would stop at any page boundary inside [dst_offset, dst_offset + len) or [src_offset, src_offset + len).

This is mostly allowing us to go copy_pages(), but if we're going folio we will need to handle multi-page (the old behavior) or single folio (the new optimization).

The current code would be a burden for future changes.

[ENHANCEMENT]
Instead of sticking with copy_pages(), here we utilize write_extent_buffer() to handle writing into the dst range.

Now we only need to handle the page boundaries inside the source range, making later switch to folio much easier.

---
 fs/btrfs/extent_io.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)
fs/btrfs/extent_io.c +13 -23
index fef5a7b6c60a..3125108c5339 100644
@@ -4198,6 +4198,8 @@ void write_extent_buffer(const struct extent_buffer *eb, const void *srcv,
 	struct page *page;
 	char *kaddr;
 	char *src = (char *)srcv;
+	/* For unmapped (dummy) ebs, no need to check their uptodate status. */
+	bool check_uptodate = !test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);
 	unsigned long i = get_eb_page_index(start);
 
 	WARN_ON(test_bit(EXTENT_BUFFER_NO_CHECK, &eb->bflags));
@@ -4209,7 +4211,8 @@ void write_extent_buffer(const struct extent_buffer *eb, const void *srcv,
 
 	while (len > 0) {
 		page = eb->pages[i];
-		assert_eb_page_uptodate(eb, page);
+		if (check_uptodate)
+			assert_eb_page_uptodate(eb, page);
 
 		cur = min(len, PAGE_SIZE - offset);
 		kaddr = page_address(page);
@@ -4477,34 +4480,21 @@ void memcpy_extent_buffer(const struct extent_buffer *dst,
 			  unsigned long dst_offset, unsigned long src_offset,
 			  unsigned long len)
 {
-	size_t cur;
-	size_t dst_off_in_page;
-	size_t src_off_in_page;
-	unsigned long dst_i;
-	unsigned long src_i;
+	unsigned long cur = src_offset;
 
 	if (check_eb_range(dst, dst_offset, len) ||
 	    check_eb_range(dst, src_offset, len))
 		return;
 
-	while (len > 0) {
-		dst_off_in_page = get_eb_offset_in_page(dst, dst_offset);
-		src_off_in_page = get_eb_offset_in_page(dst, src_offset);
+	while (cur < src_offset + len) {
+		int index = get_eb_page_index(cur);
+		unsigned long offset = get_eb_offset_in_page(dst, cur);
+		unsigned long cur_len = min(src_offset + len - cur, PAGE_SIZE - offset);
+		unsigned long offset_to_start = cur - src_offset;
+		void *src_addr = page_address(dst->pages[index]) + offset;
 
-		dst_i = get_eb_page_index(dst_offset);
-		src_i = get_eb_page_index(src_offset);
-
-		cur = min(len, (unsigned long)(PAGE_SIZE -
-					       src_off_in_page));
-		cur = min_t(unsigned long, cur,
-			(unsigned long)(PAGE_SIZE - dst_off_in_page));
-
-		copy_pages(dst->pages[dst_i], dst->pages[src_i],
-			   dst_off_in_page, src_off_in_page, cur);
-
-		src_offset += cur;
-		dst_offset += cur;
-		len -= cur;
+		write_extent_buffer(dst, src_addr, dst_offset + offset_to_start, cur_len);
+		cur += cur_len;
 	}
 }
 
Signature · 1 line
2.41.0
author
#7 lore raw

PATCH 5/6 btrfs: refactor copy_extent_buffer_full()

In reply to Qu Wenruo

[BACKGROUND]
copy_extent_buffer_full() currently does different handling for regular and subpage cases, for regular cases it does a page by page copying. For subpage cases, it just copy the content.

This is fine for the page based extent buffer code, but for the incoming folio conversion, it can be a burden to add a new branch just to handle all the different combinations (subpage vs regular, one single folio vs multi pages).

[ENHANCE]
Instead of handling the different combinations, just go one single handling for all cases, utilizing write_extent_buffer() to do the copying.

---
 fs/btrfs/extent_io.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)
fs/btrfs/extent_io.c +9 -14
index 3125108c5339..9425380c110e 100644
@@ -4254,24 +4254,19 @@ void memzero_extent_buffer(const struct extent_buffer *eb, unsigned long start,
 void copy_extent_buffer_full(const struct extent_buffer *dst,
 			     const struct extent_buffer *src)
 {
-	int i;
-	int num_pages;
+	unsigned long cur = 0;
 
 	ASSERT(dst->len == src->len);
 
-	if (dst->fs_info->nodesize >= PAGE_SIZE) {
-		num_pages = num_extent_pages(dst);
-		for (i = 0; i < num_pages; i++)
-			copy_page(page_address(dst->pages[i]),
-				  page_address(src->pages[i]));
-	} else {
-		size_t src_offset = get_eb_offset_in_page(src, 0);
-		size_t dst_offset = get_eb_offset_in_page(dst, 0);
+	while (cur < src->len) {
+		int index = get_eb_page_index(cur);
+		unsigned long offset = get_eb_offset_in_page(src, cur);
+		unsigned long cur_len = min(src->len, PAGE_SIZE - offset);
+		void *addr = page_address(src->pages[index]) + offset;
 
-		ASSERT(src->fs_info->nodesize < PAGE_SIZE);
-		memcpy(page_address(dst->pages[0]) + dst_offset,
-		       page_address(src->pages[0]) + src_offset,
-		       src->len);
+		write_extent_buffer(dst, addr, cur, cur_len);
+
+		cur += cur_len;
 	}
 }
 
Signature · 1 line
2.41.0
author
#8 lore raw

PATCH 6/6 btrfs: call copy_extent_buffer_full() inside btrfs_clone_extent_buffer()

In reply to Qu Wenruo

Function btrfs_clone_extent_buffer() is calling of copy_page() directly.

To make later migration for folio easier, just call copy_extent_buffer_full() instead.

---
 fs/btrfs/extent_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/btrfs/extent_io.c +1 -1
index 9425380c110e..555cf17c13f9 100644
@@ -3285,8 +3285,8 @@ struct extent_buffer *btrfs_clone_extent_buffer(const struct extent_buffer *src)
 			return NULL;
 		}
 		WARN_ON(PageDirty(p));
-		copy_page(page_address(p), page_address(src->pages[i]));
 	}
+	copy_extent_buffer_full(new, src);
 	set_extent_buffer_uptodate(new);
 
 	return new;
Signature · 1 line
2.41.0