| Message ID | cover.1704704328.git.wqu@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs: fix and simplify the inline extent decompression path for subpage | expand |
On Mon, Jan 08, 2024 at 07:38:43PM +1030, Qu Wenruo wrote: > There is a long existing bug in subpage inline extent reflinking to > another location. > > The bug is caused by an existing bad code, which is from the beginning > of btrfs. > The bad code is never properly explained and got further copied into new > compression code. I think there was an idea to let an inline compressed extent to span more than one page, but it never materialized. There could be code handling it but due to the invariants (like that inline extent is always smaller than a sector) it's never executed. > The bad condition never got properly triggered by different reasons for > different platforms: > > - On 4K page sized system, the @start_byte is always 0 > Thus the existing checks are all dead code, thus never triggered. > > - For subpage (4K sectorsize 64K page size) cases, inline extent > creation is disable for a different reason > Since no inline extent can be created, there is no way to reflink > any inlined extent thus no way to trigger it. > > The fixes are mostly going to rework the decompression loop, making sure > the input and output buffer are always large enough for inline extent. > Thus no need for any loop, but a single decompression call. Yeah, as long as we have the page == sectorsize it's not necessary. > But the difficulty lies in how to properly test the bug. > For now I'm only doing cross-platform tests, using image created on > x86_64, and do the reflink on aarch64. > Not sure if it's possible to upload a binary image for fstests, or I > don't have any good way to test the bug. We had this discussion year ago (no crafted images) but the maintainer of fstests changed meanwhile so you can try again. You can always add that to progs but it won't get the same level of testing due to its primary purpose to test the userspace code. > Qu Wenruo (3): > btrfs: zlib: fix and simplify the inline extent decompression > btrfs: lzo: fix and simplify the inline extent decompression > btrfs: zstd: fix and simplify the inline extent decompression Added to misc-next, thanks. There are new error messages when the decompressed length is not expected, this could be still improved (and made more consistent). The name of the file or inode number and filesystem identification are missing, but that's how the current code works.
On 2024/1/10 13:59, David Sterba wrote: > On Mon, Jan 08, 2024 at 07:38:43PM +1030, Qu Wenruo wrote: >> There is a long existing bug in subpage inline extent reflinking to >> another location. >> >> The bug is caused by an existing bad code, which is from the beginning >> of btrfs. >> The bad code is never properly explained and got further copied into new >> compression code. > > I think there was an idea to let an inline compressed extent to span > more than one page, but it never materialized. There could be code > handling it but due to the invariants (like that inline extent is always > smaller than a sector) it's never executed. Unfortunately it doesn't look like this. For multiple page cases, we should not call `offset_in_page()` for that @start_byte, and even in that case, we still don't need that @start_byte parameter. To me, that parameter is just for subpage, but not properly documented and since it's mostly dead code, we copy them around without properly knowing the reason. > >> The bad condition never got properly triggered by different reasons for >> different platforms: >> >> - On 4K page sized system, the @start_byte is always 0 >> Thus the existing checks are all dead code, thus never triggered. >> >> - For subpage (4K sectorsize 64K page size) cases, inline extent >> creation is disable for a different reason >> Since no inline extent can be created, there is no way to reflink >> any inlined extent thus no way to trigger it. >> >> The fixes are mostly going to rework the decompression loop, making sure >> the input and output buffer are always large enough for inline extent. >> Thus no need for any loop, but a single decompression call. > > Yeah, as long as we have the page == sectorsize it's not necessary. > >> But the difficulty lies in how to properly test the bug. >> For now I'm only doing cross-platform tests, using image created on >> x86_64, and do the reflink on aarch64. >> Not sure if it's possible to upload a binary image for fstests, or I >> don't have any good way to test the bug. > > We had this discussion year ago (no crafted images) but the maintainer > of fstests changed meanwhile so you can try again. It looks like that's the best way to go for now. Thanks, Qu > You can always add > that to progs but it won't get the same level of testing due to its > primary purpose to test the userspace code. > >> Qu Wenruo (3): >> btrfs: zlib: fix and simplify the inline extent decompression >> btrfs: lzo: fix and simplify the inline extent decompression >> btrfs: zstd: fix and simplify the inline extent decompression > > Added to misc-next, thanks. > > There are new error messages when the decompressed length is not > expected, this could be still improved (and made more consistent). The > name of the file or inode number and filesystem identification are > missing, but that's how the current code works. >