| 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 >
[CHANGELOG] v2: - Split the subpage.[ch] modification into 3 patches - 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(-)