| Message ID | cover.1742443383.git.wqu@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs: refactor btrfs_buffered_write() for the incoming large data folios | expand |
On Thu, Mar 20, 2025 at 04:04:07PM +1030, Qu Wenruo wrote: > The function btrfs_buffered_write() is implementing all the complex > heavy-lifting work inside a huge while () loop, which makes later large > data folios work much harder. > > The first patch is a patch that already submitted to the mailing list > recently, but all later reworks depends on that patch, thus it is > included in the series. > > The core of the whole series is to introduce a helper function, > copy_one_range() to do the buffer copy into one single folio. > > Patch 2 is a preparation that moves the error cleanup into the main loop, > so we do not have dedicated out-of-loop cleanup. > > Patch 3 is another preparation that extract the space reservation code > into a helper, make the final refactor patch a little more smaller. > > And patch 4 is the main dish, with all the refactoring happening inside > it. > > Qu Wenruo (4): > btrfs: remove force_page_uptodate variable from btrfs_buffered_write() > btrfs: cleanup the reserved space inside the loop of > btrfs_buffered_write() > btrfs: extract the space reservation code from btrfs_buffered_write() > btrfs: extract the main loop of btrfs_buffered_write() into a helper I'm looking at the committed patches in for-next and there are still too many whitespace and formatting issues, atop those pointed out in the mail discussion. It's probably because the code moved and inherited the formatting but this is one of the oportunities to fix it in the final version. I fixed what I saw, but plase try to reformat the code according to the best pratices. No big deal if something slips, I'd rather you focus on the code than on formattig but in this patchset it looked like a systematic error. In case of factoring out code and moving it around I suggest to do it in two steps, first move the code, make sure it's correct etc, commit, and then open the changed code in editor in diff mode. If you're using fugitive.vim the command ":Gdiff HEAD^" clearly shows the changed code and doing the styling and formatting pass is quite easy.
在 2025/3/28 03:17, David Sterba 写道: > On Thu, Mar 20, 2025 at 04:04:07PM +1030, Qu Wenruo wrote: >> The function btrfs_buffered_write() is implementing all the complex >> heavy-lifting work inside a huge while () loop, which makes later large >> data folios work much harder. >> >> The first patch is a patch that already submitted to the mailing list >> recently, but all later reworks depends on that patch, thus it is >> included in the series. >> >> The core of the whole series is to introduce a helper function, >> copy_one_range() to do the buffer copy into one single folio. >> >> Patch 2 is a preparation that moves the error cleanup into the main loop, >> so we do not have dedicated out-of-loop cleanup. >> >> Patch 3 is another preparation that extract the space reservation code >> into a helper, make the final refactor patch a little more smaller. >> >> And patch 4 is the main dish, with all the refactoring happening inside >> it. >> >> Qu Wenruo (4): >> btrfs: remove force_page_uptodate variable from btrfs_buffered_write() >> btrfs: cleanup the reserved space inside the loop of >> btrfs_buffered_write() >> btrfs: extract the space reservation code from btrfs_buffered_write() >> btrfs: extract the main loop of btrfs_buffered_write() into a helper > > I'm looking at the committed patches in for-next and there are still too > many whitespace and formatting issues, atop those pointed out in the > mail discussion. It's probably because the code moved and inherited the > formatting but this is one of the oportunities to fix it in the final > version. > > I fixed what I saw, but plase try to reformat the code according to the > best pratices. No big deal if something slips, I'd rather you focus on > the code than on formattig but in this patchset it looked like a > systematic error. > > In case of factoring out code and moving it around I suggest to do it in > two steps, first move the code, make sure it's correct etc, commit, and > then open the changed code in editor in diff mode. If you're using > fugitive.vim the command ":Gdiff HEAD^" clearly shows the changed code > and doing the styling and formatting pass is quite easy. > This is a little weird, IIRC the workflow hooks should detect those whitespace errors at commit time, e.g: $ git commit -a --amend ERROR:TRAILING_WHITESPACE: trailing whitespace #9: FILE: fs/btrfs/file.c:2207: +^I$ total: 1 errors, 0 warnings, 7 lines checked Checkpatch found errors, would you like to fix them up? (Yn) But it was never triggered at any of the code move. I know I missed a lot of style changes when moving the code, but I didn't expect any whitespace errors. Mind to provide some examples where the git hooks didn't catch them? Thanks, Qu
On Fri, Mar 28, 2025 at 07:17:39AM +1030, Qu Wenruo wrote: > 在 2025/3/28 03:17, David Sterba 写道: > > On Thu, Mar 20, 2025 at 04:04:07PM +1030, Qu Wenruo wrote: > >> The function btrfs_buffered_write() is implementing all the complex > >> heavy-lifting work inside a huge while () loop, which makes later large > >> data folios work much harder. > >> > >> The first patch is a patch that already submitted to the mailing list > >> recently, but all later reworks depends on that patch, thus it is > >> included in the series. > >> > >> The core of the whole series is to introduce a helper function, > >> copy_one_range() to do the buffer copy into one single folio. > >> > >> Patch 2 is a preparation that moves the error cleanup into the main loop, > >> so we do not have dedicated out-of-loop cleanup. > >> > >> Patch 3 is another preparation that extract the space reservation code > >> into a helper, make the final refactor patch a little more smaller. > >> > >> And patch 4 is the main dish, with all the refactoring happening inside > >> it. > >> > >> Qu Wenruo (4): > >> btrfs: remove force_page_uptodate variable from btrfs_buffered_write() > >> btrfs: cleanup the reserved space inside the loop of > >> btrfs_buffered_write() > >> btrfs: extract the space reservation code from btrfs_buffered_write() > >> btrfs: extract the main loop of btrfs_buffered_write() into a helper > > > > I'm looking at the committed patches in for-next and there are still too > > many whitespace and formatting issues, atop those pointed out in the > > mail discussion. It's probably because the code moved and inherited the > > formatting but this is one of the oportunities to fix it in the final > > version. > > > > I fixed what I saw, but plase try to reformat the code according to the > > best pratices. No big deal if something slips, I'd rather you focus on > > the code than on formattig but in this patchset it looked like a > > systematic error. > > > > In case of factoring out code and moving it around I suggest to do it in > > two steps, first move the code, make sure it's correct etc, commit, and > > then open the changed code in editor in diff mode. If you're using > > fugitive.vim the command ":Gdiff HEAD^" clearly shows the changed code > > and doing the styling and formatting pass is quite easy. > > > This is a little weird, IIRC the workflow hooks should detect those > whitespace errors at commit time, e.g: > > $ git commit -a --amend > ERROR:TRAILING_WHITESPACE: trailing whitespace > #9: FILE: fs/btrfs/file.c:2207: > +^I$ > > total: 1 errors, 0 warnings, 7 lines checked > Checkpatch found errors, would you like to fix them up? (Yn) > > But it was never triggered at any of the code move. > > I know I missed a lot of style changes when moving the code, but I > didn't expect any whitespace errors. > > Mind to provide some examples where the git hooks didn't catch them? I don't use the git hooks for checks so I'll copy it from the patches: https://lore.kernel.org/linux-btrfs/b0bd320dba85d72a34a4f7e5ba6b6c42caedbe41.1742443383.git.wqu@suse.com/ @@ -1074,6 +1074,27 @@ int btrfs_write_check(struct kiocb *iocb, size_t count) return 0; } +static void release_space(struct btrfs_inode *inode, + struct extent_changeset *data_reserved, + u64 start, u64 len, + bool only_release_metadata) +{ + const struct btrfs_fs_info *fs_info = inode->root->fs_info; + + if (!len) + return; + + if (only_release_metadata) { + btrfs_check_nocow_unlock(inode); + btrfs_delalloc_release_metadata(inode, len, true); + } else { + btrfs_delalloc_release_space(inode, + data_reserved, + round_down(start, fs_info->sectorsize), + len, true); + } +} --- The parameters of btrfs_delalloc_release_space(), matching its origin: - if (release_bytes) { - if (only_release_metadata) { - btrfs_check_nocow_unlock(BTRFS_I(inode)); - btrfs_delalloc_release_metadata(BTRFS_I(inode), - release_bytes, true); - } else { - btrfs_delalloc_release_space(BTRFS_I(inode), - data_reserved, - round_down(pos, fs_info->sectorsize), - release_bytes, true); - } - } --- Maybe the checks ignore possible whitespace adjustments in moved code, e.g. comparing the - and + change literally and not taking the new location into account. Basically the same pattern in https://lore.kernel.org/linux-btrfs/522bfb923444f08b2c68c51a05cb5ca8b3ac7a77.1742443383.git.wqu@suse.com/ code moved to reserve_space() from btrfs_buffered_write(), parameters passed to btrfs_free_reserved_data_space(). I remember fixing more code like this, but hopefully the two examples would be sufficient to test the hook checks.
在 2025/3/29 05:42, David Sterba 写道: > On Fri, Mar 28, 2025 at 07:17:39AM +1030, Qu Wenruo wrote: >> 在 2025/3/28 03:17, David Sterba 写道: >>> On Thu, Mar 20, 2025 at 04:04:07PM +1030, Qu Wenruo wrote: >>>> The function btrfs_buffered_write() is implementing all the complex >>>> heavy-lifting work inside a huge while () loop, which makes later large >>>> data folios work much harder. >>>> >>>> The first patch is a patch that already submitted to the mailing list >>>> recently, but all later reworks depends on that patch, thus it is >>>> included in the series. >>>> >>>> The core of the whole series is to introduce a helper function, >>>> copy_one_range() to do the buffer copy into one single folio. >>>> >>>> Patch 2 is a preparation that moves the error cleanup into the main loop, >>>> so we do not have dedicated out-of-loop cleanup. >>>> >>>> Patch 3 is another preparation that extract the space reservation code >>>> into a helper, make the final refactor patch a little more smaller. >>>> >>>> And patch 4 is the main dish, with all the refactoring happening inside >>>> it. >>>> >>>> Qu Wenruo (4): >>>> btrfs: remove force_page_uptodate variable from btrfs_buffered_write() >>>> btrfs: cleanup the reserved space inside the loop of >>>> btrfs_buffered_write() >>>> btrfs: extract the space reservation code from btrfs_buffered_write() >>>> btrfs: extract the main loop of btrfs_buffered_write() into a helper >>> >>> I'm looking at the committed patches in for-next and there are still too >>> many whitespace and formatting issues, atop those pointed out in the >>> mail discussion. It's probably because the code moved and inherited the >>> formatting but this is one of the oportunities to fix it in the final >>> version. >>> >>> I fixed what I saw, but plase try to reformat the code according to the >>> best pratices. No big deal if something slips, I'd rather you focus on >>> the code than on formattig but in this patchset it looked like a >>> systematic error. >>> >>> In case of factoring out code and moving it around I suggest to do it in >>> two steps, first move the code, make sure it's correct etc, commit, and >>> then open the changed code in editor in diff mode. If you're using >>> fugitive.vim the command ":Gdiff HEAD^" clearly shows the changed code >>> and doing the styling and formatting pass is quite easy. >>> >> This is a little weird, IIRC the workflow hooks should detect those >> whitespace errors at commit time, e.g: >> >> $ git commit -a --amend >> ERROR:TRAILING_WHITESPACE: trailing whitespace >> #9: FILE: fs/btrfs/file.c:2207: >> +^I$ >> >> total: 1 errors, 0 warnings, 7 lines checked >> Checkpatch found errors, would you like to fix them up? (Yn) >> >> But it was never triggered at any of the code move. >> >> I know I missed a lot of style changes when moving the code, but I >> didn't expect any whitespace errors. >> >> Mind to provide some examples where the git hooks didn't catch them? > > I don't use the git hooks for checks so I'll copy it from the patches: > > https://lore.kernel.org/linux-btrfs/b0bd320dba85d72a34a4f7e5ba6b6c42caedbe41.1742443383.git.wqu@suse.com/ > > @@ -1074,6 +1074,27 @@ int btrfs_write_check(struct kiocb *iocb, size_t count) > return 0; > } > > +static void release_space(struct btrfs_inode *inode, > + struct extent_changeset *data_reserved, > + u64 start, u64 len, > + bool only_release_metadata) > +{ > + const struct btrfs_fs_info *fs_info = inode->root->fs_info; > + > + if (!len) > + return; > + > + if (only_release_metadata) { > + btrfs_check_nocow_unlock(inode); > + btrfs_delalloc_release_metadata(inode, len, true); > + } else { > + btrfs_delalloc_release_space(inode, > + data_reserved, > + round_down(start, fs_info->sectorsize), > + len, true); > + } > +} > --- > > The parameters of btrfs_delalloc_release_space(), matching its origin: > > - if (release_bytes) { > - if (only_release_metadata) { > - btrfs_check_nocow_unlock(BTRFS_I(inode)); > - btrfs_delalloc_release_metadata(BTRFS_I(inode), > - release_bytes, true); > - } else { > - btrfs_delalloc_release_space(BTRFS_I(inode), > - data_reserved, > - round_down(pos, fs_info->sectorsize), > - release_bytes, true); > - } > - } > --- > > Maybe the checks ignore possible whitespace adjustments in moved code, > e.g. comparing the - and + change literally and not taking the new > location into account. Sorry, I just compared the patch and the commit inside for-next branch, but I didn't see any white-space related changes. Only the fs_info grabbing is moved into the else branch inside release_space(), and moving the indent of parameter to align with the '(' for btrfs_delalloc_release_space(). If by white-space, you mean strict indent for all parameters, that's not (and I guess it will never) be implemented by check-patch.pl. > > Basically the same pattern in https://lore.kernel.org/linux-btrfs/522bfb923444f08b2c68c51a05cb5ca8b3ac7a77.1742443383.git.wqu@suse.com/ The same indent pattern you modified. Personally speaking, I do not think the indent alignment to '(' should be mandatory. The indent different is small, won't change the reader's ability to know it's a parameter, and there are both types of indent usages across the filesystems. To be honest, aligning to '(' will only make future function rename harder, thus I prefer fixed tab indent other than strong alignment. But if you insist, I can follow it in the future, but since it's not included in checkpatch.pl, there will be missing ones here and there. Although I really prefer to not have such a mandatory alignment requirement, to reduce the burden on you and all other developers. Thanks, Qu > > code moved to reserve_space() from btrfs_buffered_write(), parameters passed to > btrfs_free_reserved_data_space(). > > I remember fixing more code like this, but hopefully the two examples would be > sufficient to test the hook checks.
The function btrfs_buffered_write() is implementing all the complex heavy-lifting work inside a huge while () loop, which makes later large data folios work much harder. The first patch is a patch that already submitted to the mailing list recently, but all later reworks depends on that patch, thus it is included in the series. The core of the whole series is to introduce a helper function, copy_one_range() to do the buffer copy into one single folio. Patch 2 is a preparation that moves the error cleanup into the main loop, so we do not have dedicated out-of-loop cleanup. Patch 3 is another preparation that extract the space reservation code into a helper, make the final refactor patch a little more smaller. And patch 4 is the main dish, with all the refactoring happening inside it. Qu Wenruo (4): btrfs: remove force_page_uptodate variable from btrfs_buffered_write() btrfs: cleanup the reserved space inside the loop of btrfs_buffered_write() btrfs: extract the space reservation code from btrfs_buffered_write() btrfs: extract the main loop of btrfs_buffered_write() into a helper fs/btrfs/file.c | 434 ++++++++++++++++++++++++++---------------------- 1 file changed, 231 insertions(+), 203 deletions(-)