| Message ID | 20230523081322.331337-2-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/16] btrfs: fix range_end calculation in extent_write_locked_range | expand |
On Tue, May 23, 2023 at 10:13:07AM +0200, Christoph Hellwig wrote: > The range_end field in struct writeback_control is inclusive, just like > the end parameter passed to extent_write_locked_range. There should also be analysis what are the effects and how severe the bug is. The code is from 2008 so if it's really bad we would have seen something already.
On Mon, May 29, 2023 at 07:13:18PM +0200, David Sterba wrote: > On Tue, May 23, 2023 at 10:13:07AM +0200, Christoph Hellwig wrote: > > The range_end field in struct writeback_control is inclusive, just like > > the end parameter passed to extent_write_locked_range. > > There should also be analysis what are the effects and how severe the > bug is. The code is from 2008 so if it's really bad we would have seen > something already. Seems it's directly affecting subpage since e55a0de18572 ("btrfs: rework page locking in __extent_writepage()"), the range_end is passed there and adjusted by + 1, IOW expecting it to initially have the value of 'end'. For the normal case the wbc is passed down to extent_write_cache_pages and "end = wbc->range_end >> PAGE_SHIFT;" the final range [start, end) is one page larger, tagged for writeback and then processed in the loop. The actual writeback goes again to __extent_writepage writepage_delalloc __extent_writepage_io and the page gets written, so there's potentially some unneded work done, unless some other condition skips the io.
On Tue, May 30, 2023 at 03:13:04PM +0200, David Sterba wrote: > Seems it's directly affecting subpage since e55a0de18572 ("btrfs: rework > page locking in __extent_writepage()"), the range_end is passed there > and adjusted by + 1, IOW expecting it to initially have the value of > 'end'. This code is only used for compression, which isn't supported for zoned devices, and for compression, which unless I misremember the code only supported for multiples of the page size for the allocations.
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 5999ac3ee601db..c1b0ca94be34e1 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -2309,7 +2309,7 @@ int extent_write_locked_range(struct inode *inode, u64 start, u64 end) struct writeback_control wbc_writepages = { .sync_mode = WB_SYNC_ALL, .range_start = start, - .range_end = end + 1, + .range_end = end, .no_cgroup_owner = 1, }; struct btrfs_bio_ctrl bio_ctrl = {
The range_end field in struct writeback_control is inclusive, just like the end parameter passed to extent_write_locked_range. Fixes: 771ed689d2cd ("Btrfs: Optimize compressed writeback and reads") Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/btrfs/extent_io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)