On 2023/4/18 09:17, Dominique Martinet wrote:
From: Dominique Martinet <dominique.marti...@atmark-techno.com>
The subject can be changed to "btrfs: fix offset when reading compressed
extents".
The original one is a little too generic.
btrfs_read_extent_reg correctly computed the extent offset in the
BTRFS_COMPRESS_NONE case, but did not account for the 'offset - key.offset'
part correctly in the compressed case, making the function read
incorrect data.
In the case I examined, the last 4k of a file was corrupted and
contained data from a few blocks prior:
btrfs_file_read()
-> btrfs_read_extent_reg
(aligned part loop from 0x40480000 to 0x40ba0000, 128KB at a time)
last read had 0x4000 bytes in extent, but aligned_end - cur limited
the read to 0x3000 (offset 0x720000)
-> read_and_truncate_page
-> btrfs_read_extent_reg
reading the last 0x1000 bytes from offset 0x73000 (end of the
previous 0x4000) into 0x40ba3000
here key.offset = 0x70000 so we need to use it to recover the
0x3000 offset.
You can use "btrfs ins dump-tree" to provide a much easier to read
on-disk data layout.
And you can also add a smaller reproducer.
Confirmed by checking data, before patch:
u-boot=> mmc load 1:1 $loadaddr boot/uImage
u-boot=> md 0x40ba0000
40ba0000: c0232ff8 c0c722cb 030e94be bf10000c ./#.."..........
u-boot=> md 0x40ba3000
40ba3000: c0232ff8 c0c722cb 030e94be bf10000c ./#.."..........
After patch (also confirmed with data read from linux):
u-boot=> md 0x40ba3000
40ba3000: 64cc9f03 81142256 6910000c 0c03483c ...dV".....i<H..
Note that the code previously (before commit e3427184f38a ("fs: btrfs:
Implement btrfs_file_read()")) did not split that read in two, so
this is a regression.
Fixes: a26a6bedafcf ("fs: btrfs: Introduce btrfs_read_extent_inline() and
btrfs_read_extent_reg()")
Signed-off-by: Dominique Martinet <dominique.marti...@atmark-techno.com>
---
fs/btrfs/inode.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 40025662f250..3d6e39e6544d 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -443,6 +443,8 @@ int btrfs_read_extent_reg(struct btrfs_path *path,
IS_ALIGNED(len, fs_info->sectorsize));
ASSERT(offset >= key.offset &&
offset + len <= key.offset + extent_num_bytes);
+ /* offset is now offset within extent */
+ offset = btrfs_file_extent_offset(leaf, fi) + offset - key.offset;
I prefer not to use the @offset, explained later.
/* Preallocated or hole , fill @dest with zero */
if (btrfs_file_extent_type(leaf, fi) == BTRFS_FILE_EXTENT_PREALLOC ||
@@ -454,9 +456,7 @@ int btrfs_read_extent_reg(struct btrfs_path *path,
if (btrfs_file_extent_compression(leaf, fi) == BTRFS_COMPRESS_NONE) {
u64 logical;
- logical = btrfs_file_extent_disk_bytenr(leaf, fi) +
- btrfs_file_extent_offset(leaf, fi) +
- offset - key.offset;
+ logical = btrfs_file_extent_disk_bytenr(leaf, fi) + offset;
This is touching non-compressed path, which is very weird as your commit
message said this part is correct.
I prefer the original one to show everything we need to take into
consideration.
Otherwise looks good to me.
Thanks,
Qu
read = len;
num_copies = btrfs_num_copies(fs_info, logical, len);
@@ -511,7 +511,7 @@ int btrfs_read_extent_reg(struct btrfs_path *path,
if (ret < dsize)
memset(dbuf + ret, 0, dsize - ret);
/* Then copy the needed part */
- memcpy(dest, dbuf + btrfs_file_extent_offset(leaf, fi), len);
+ memcpy(dest, dbuf + offset, len);
ret = len;
out:
free(cbuf);
