Message ID | cover.1741591823.git.wqu@suse.com (mailing list archive) |
---|---|
Headers | show |
Series | btrfs: prepare for larger folios support | expand |
On Mon, Mar 10, 2025 at 06:05:56PM +1030, Qu Wenruo wrote: > [CHANGELOG] > v2: > - Split the subpage.[ch] modification into 3 patches I found the way you split it to be a little confusing, honestly. I would have preferred splitting it by operation (alloc_subpage, is_subpage, etc..), rather than a basically incorrect prep patch and then a sed patch on the subpage file. You already iterated on this, so I absolutely don't want to hold anything up on this observation, haha. If it's something others agree on, I wouldn't mind seeing it before the patches go in. I asked a question on patch 5 that I think is interesting, but please feel free to add: Reviewed-by: Boris Burkov <boris@bur.io> > - Rebased the latest for-next branch > Now all dependency are in for-next. > > This means: > > - Our subpage routine should check against the folio size other than > PAGE_SIZE > > - Make functions handling filemap folios to use folio_size() other than > PAGE_SIZE > > The most common paths are: > * Buffered reads/writes > * Uncompressed folio writeback > Already handled pretty well > > * Compressed read > * Compressed write > To take full advantage of larger folios, we should use folio_iter > other than bvec_iter. > This will be a dedicated patchset, and the existing bvec_iter can > still handle larger folios. > > Internal usages can still use page sized folios, or even pages, > including: > * Encoded reads/writes > * Compressed folios > * RAID56 internal pages > * Scrub internal pages > > This patchset will handle the above mentioned points by: > > - Prepare the subpage routine to handle larger folios > This will introduce a small overhead, as all checks are against folio > sizes, even on x86_64 we can no longer skip subpage completely. > > This is done in the first patch. > > - Convert straightforward PAGE_SIZE users to use folio_size() > This is done in the remaining patches. > > Currently this patchset is not a exhaustive conversion, I'm pretty sure > there are other complex situations which can cause problems. > Those problems can only be exposed and fixed after switching on the > experimental larger folios support later. > > Qu Wenruo (6): > btrfs: subpage: make btrfs_is_subpage() check against a folio > btrfs: add a @fsize parameter to btrfs_alloc_subpage() > btrfs: replace PAGE_SIZE with folio_size for subpage.[ch] > btrfs: prepare btrfs_launcher_folio() for larger folios support > btrfs: prepare extent_io.c for future larger folio support > btrfs: prepare btrfs_page_mkwrite() for larger folios > > fs/btrfs/extent_io.c | 49 ++++++++++++++++++++++++-------------------- > fs/btrfs/file.c | 19 +++++++++-------- > fs/btrfs/inode.c | 4 ++-- > fs/btrfs/subpage.c | 38 +++++++++++++++++----------------- > fs/btrfs/subpage.h | 16 +++++++-------- > 5 files changed, 66 insertions(+), 60 deletions(-) > > -- > 2.48.1 >
在 2025/3/11 10:02, Boris Burkov 写道: > On Mon, Mar 10, 2025 at 06:05:56PM +1030, Qu Wenruo wrote: >> [CHANGELOG] >> v2: >> - Split the subpage.[ch] modification into 3 patches > > I found the way you split it to be a little confusing, honestly. I would > have preferred splitting it by operation (alloc_subpage, is_subpage, > etc..), rather than a basically incorrect prep patch and then a sed patch > on the subpage file. That's why I initially merge the first 3 patches into one. If anyone else has some preference on the split, I'm pretty happy to follow. > > You already iterated on this, so I absolutely don't want to hold > anything up on this observation, haha. If it's something others agree > on, I wouldn't mind seeing it before the patches go in. > > I asked a question on patch 5 that I think is interesting, but please > feel free to add: > Reviewed-by: Boris Burkov <boris@bur.io> Thanks for the review, Qu > >> - Rebased the latest for-next branch >> Now all dependency are in for-next. >> >> This means: >> >> - Our subpage routine should check against the folio size other than >> PAGE_SIZE >> >> - Make functions handling filemap folios to use folio_size() other than >> PAGE_SIZE >> >> The most common paths are: >> * Buffered reads/writes >> * Uncompressed folio writeback >> Already handled pretty well >> >> * Compressed read >> * Compressed write >> To take full advantage of larger folios, we should use folio_iter >> other than bvec_iter. >> This will be a dedicated patchset, and the existing bvec_iter can >> still handle larger folios. >> >> Internal usages can still use page sized folios, or even pages, >> including: >> * Encoded reads/writes >> * Compressed folios >> * RAID56 internal pages >> * Scrub internal pages >> >> This patchset will handle the above mentioned points by: >> >> - Prepare the subpage routine to handle larger folios >> This will introduce a small overhead, as all checks are against folio >> sizes, even on x86_64 we can no longer skip subpage completely. >> >> This is done in the first patch. >> >> - Convert straightforward PAGE_SIZE users to use folio_size() >> This is done in the remaining patches. >> >> Currently this patchset is not a exhaustive conversion, I'm pretty sure >> there are other complex situations which can cause problems. >> Those problems can only be exposed and fixed after switching on the >> experimental larger folios support later. >> >> Qu Wenruo (6): >> btrfs: subpage: make btrfs_is_subpage() check against a folio >> btrfs: add a @fsize parameter to btrfs_alloc_subpage() >> btrfs: replace PAGE_SIZE with folio_size for subpage.[ch] >> btrfs: prepare btrfs_launcher_folio() for larger folios support >> btrfs: prepare extent_io.c for future larger folio support >> btrfs: prepare btrfs_page_mkwrite() for larger folios >> >> fs/btrfs/extent_io.c | 49 ++++++++++++++++++++++++-------------------- >> fs/btrfs/file.c | 19 +++++++++-------- >> fs/btrfs/inode.c | 4 ++-- >> fs/btrfs/subpage.c | 38 +++++++++++++++++----------------- >> fs/btrfs/subpage.h | 16 +++++++-------- >> 5 files changed, 66 insertions(+), 60 deletions(-) >> >> -- >> 2.48.1 >> >
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On Mon, Mar 10, 2025 at 06:05:56PM +1030, Qu Wenruo wrote: > [CHANGELOG] > v2: > - Split the subpage.[ch] modification into 3 patches > - Rebased the latest for-next branch > Now all dependency are in for-next. Please add the series to for-next, I haven't found anything that would need fixups or another resend so we cant get it to 6.15 queue. Thanks.
On Wed, Mar 12, 2025 at 02:44:55PM +0100, David Sterba wrote: > On Mon, Mar 10, 2025 at 06:05:56PM +1030, Qu Wenruo wrote: > > [CHANGELOG] > > v2: > > - Split the subpage.[ch] modification into 3 patches > > - Rebased the latest for-next branch > > Now all dependency are in for-next. > > Please add the series to for-next, I haven't found anything that would > need fixups or another resend so we cant get it to 6.15 queue. Thanks. This series is still broken for 32-bit targets as reported two weeks ago: https://lore.kernel.org/202502211908.aCcQQyEY-lkp@intel.com/ https://lore.kernel.org/20250225184136.GA1679809@ax162/ $ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- mrproper allmodconfig fs/btrfs/extent_io.o In file included from <command-line>: fs/btrfs/extent_io.c: In function 'extent_write_locked_range': include/linux/compiler_types.h:557:45: error: call to '__compiletime_assert_802' declared with attribute error: min(folio_pos(folio) + folio_size(folio) - 1, end) signedness error 557 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^ include/linux/compiler_types.h:538:25: note: in definition of macro '__compiletime_assert' 538 | prefix ## suffix(); \ | ^~~~~~ include/linux/compiler_types.h:557:9: note: in expansion of macro '_compiletime_assert' 557 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert' 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) | ^~~~~~~~~~~~~~~~~~ include/linux/minmax.h:93:9: note: in expansion of macro 'BUILD_BUG_ON_MSG' 93 | BUILD_BUG_ON_MSG(!__types_ok(ux, uy), \ | ^~~~~~~~~~~~~~~~ include/linux/minmax.h:98:9: note: in expansion of macro '__careful_cmp_once' 98 | __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_)) | ^~~~~~~~~~~~~~~~~~ include/linux/minmax.h:105:25: note: in expansion of macro '__careful_cmp' 105 | #define min(x, y) __careful_cmp(min, x, y) | ^~~~~~~~~~~~~ fs/btrfs/extent_io.c:2472:27: note: in expansion of macro 'min' 2472 | cur_end = min(folio_pos(folio) + folio_size(folio) - 1, end); | ^~~ Cheers, Nathan
在 2025/3/14 01:51, Nathan Chancellor 写道: > On Wed, Mar 12, 2025 at 02:44:55PM +0100, David Sterba wrote: >> On Mon, Mar 10, 2025 at 06:05:56PM +1030, Qu Wenruo wrote: >>> [CHANGELOG] >>> v2: >>> - Split the subpage.[ch] modification into 3 patches >>> - Rebased the latest for-next branch >>> Now all dependency are in for-next. >> >> Please add the series to for-next, I haven't found anything that would >> need fixups or another resend so we cant get it to 6.15 queue. Thanks. > > This series is still broken for 32-bit targets as reported two weeks ago: Now fixed in for-next branch. Thanks, Qu> > https://lore.kernel.org/202502211908.aCcQQyEY-lkp@intel.com/ > https://lore.kernel.org/20250225184136.GA1679809@ax162/ > > $ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- mrproper allmodconfig fs/btrfs/extent_io.o > In file included from <command-line>: > fs/btrfs/extent_io.c: In function 'extent_write_locked_range': > include/linux/compiler_types.h:557:45: error: call to '__compiletime_assert_802' declared with attribute error: min(folio_pos(folio) + folio_size(folio) - 1, end) signedness error > 557 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) > | ^ > include/linux/compiler_types.h:538:25: note: in definition of macro '__compiletime_assert' > 538 | prefix ## suffix(); \ > | ^~~~~~ > include/linux/compiler_types.h:557:9: note: in expansion of macro '_compiletime_assert' > 557 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) > | ^~~~~~~~~~~~~~~~~~~ > include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert' > 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) > | ^~~~~~~~~~~~~~~~~~ > include/linux/minmax.h:93:9: note: in expansion of macro 'BUILD_BUG_ON_MSG' > 93 | BUILD_BUG_ON_MSG(!__types_ok(ux, uy), \ > | ^~~~~~~~~~~~~~~~ > include/linux/minmax.h:98:9: note: in expansion of macro '__careful_cmp_once' > 98 | __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_)) > | ^~~~~~~~~~~~~~~~~~ > include/linux/minmax.h:105:25: note: in expansion of macro '__careful_cmp' > 105 | #define min(x, y) __careful_cmp(min, x, y) > | ^~~~~~~~~~~~~ > fs/btrfs/extent_io.c:2472:27: note: in expansion of macro 'min' > 2472 | cur_end = min(folio_pos(folio) + folio_size(folio) - 1, end); > | ^~~ > > Cheers, > Nathan >