| Message ID | 2c53f7555d45d6697e836fa2bb7dce137ab04c99.1728175215.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: remove btrfs_set_range_writeback() | expand |
I have more comments on the grammar than on the code, sorry. On Sun, Oct 06, 2024 at 11:10:22AM +1030, Qu Wenruo wrote: > The function btrfs_set_range_writeback() is originally a callback for was > metadata and data, to mark a range with writeback flag. > > Then it was converted into a common function call for both metadata and > data. > > >From the very beginning, the function is only called on a full page, 'had been' because of the other past tense, idk > later converted to handle range inside a page. > > But it never needs to handle multiple pages, and since commit needed > 8189197425e7 ("btrfs: refactor __extent_writepage_io() to do > sector-by-sector submission") the function is only called on a has been > sector-by-sector basis. > > This makes the function unnecessary, and can be converted to a simple > btrfs_folio_set_writeback() call instead. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/btrfs_inode.h | 1 - > fs/btrfs/extent_io.c | 2 +- > fs/btrfs/inode.c | 22 ---------------------- > 3 files changed, 1 insertion(+), 24 deletions(-) > > diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h > index e152fde888fc..c514bab532fa 100644 > --- a/fs/btrfs/btrfs_inode.h > +++ b/fs/btrfs/btrfs_inode.h > @@ -577,7 +577,6 @@ void btrfs_merge_delalloc_extent(struct btrfs_inode *inode, struct extent_state > struct extent_state *other); > void btrfs_split_delalloc_extent(struct btrfs_inode *inode, > struct extent_state *orig, u64 split); > -void btrfs_set_range_writeback(struct btrfs_inode *inode, u64 start, u64 end); > void btrfs_evict_inode(struct inode *inode); > struct inode *btrfs_alloc_inode(struct super_block *sb); > void btrfs_destroy_inode(struct inode *inode); > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 9fbc83c76b94..d87dcafab537 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -1359,7 +1359,7 @@ static int submit_one_sector(struct btrfs_inode *inode, > * a folio for a range already written to disk. > */ > btrfs_folio_clear_dirty(fs_info, folio, filepos, sectorsize); > - btrfs_set_range_writeback(inode, filepos, filepos + sectorsize - 1); > + btrfs_folio_set_writeback(fs_info, folio, filepos, sectorsize); > /* > * Above call should set the whole folio with writeback flag, even > * just for a single subpage sector. > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 103ec917ca9d..21e51924742a 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -8939,28 +8939,6 @@ static int btrfs_tmpfile(struct mnt_idmap *idmap, struct inode *dir, > return finish_open_simple(file, ret); > } > > -void btrfs_set_range_writeback(struct btrfs_inode *inode, u64 start, u64 end) > -{ > - struct btrfs_fs_info *fs_info = inode->root->fs_info; > - unsigned long index = start >> PAGE_SHIFT; > - unsigned long end_index = end >> PAGE_SHIFT; > - struct folio *folio; > - u32 len; > - > - ASSERT(end + 1 - start <= U32_MAX); > - len = end + 1 - start; > - while (index <= end_index) { > - folio = __filemap_get_folio(inode->vfs_inode.i_mapping, index, 0, 0); > - ASSERT(!IS_ERR(folio)); /* folios should be in the extent_io_tree */ > - > - /* This is for data, which doesn't yet support larger folio. */ > - ASSERT(folio_order(folio) == 0); > - btrfs_folio_set_writeback(fs_info, folio, start, len); So the new code is just btrfs_folio_set_writeback(), with the removed comment and assertion, what's the status regarding large folios? I assume that this now implicitly supports them, we don't necessarily need the assertion as I think we have more places where this would be detected.
在 2024/10/9 03:13, David Sterba 写道: > I have more comments on the grammar than on the code, sorry. > > On Sun, Oct 06, 2024 at 11:10:22AM +1030, Qu Wenruo wrote: >> The function btrfs_set_range_writeback() is originally a callback for > was > >> metadata and data, to mark a range with writeback flag. >> >> Then it was converted into a common function call for both metadata and >> data. >> >> >From the very beginning, the function is only called on a full page, > 'had been' because of the other past tense, idk > >> later converted to handle range inside a page. >> >> But it never needs to handle multiple pages, and since commit > needed > >> 8189197425e7 ("btrfs: refactor __extent_writepage_io() to do >> sector-by-sector submission") the function is only called on a > has been > >> sector-by-sector basis. >> >> This makes the function unnecessary, and can be converted to a simple >> btrfs_folio_set_writeback() call instead. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/btrfs_inode.h | 1 - >> fs/btrfs/extent_io.c | 2 +- >> fs/btrfs/inode.c | 22 ---------------------- >> 3 files changed, 1 insertion(+), 24 deletions(-) >> >> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h >> index e152fde888fc..c514bab532fa 100644 >> --- a/fs/btrfs/btrfs_inode.h >> +++ b/fs/btrfs/btrfs_inode.h >> @@ -577,7 +577,6 @@ void btrfs_merge_delalloc_extent(struct btrfs_inode *inode, struct extent_state >> struct extent_state *other); >> void btrfs_split_delalloc_extent(struct btrfs_inode *inode, >> struct extent_state *orig, u64 split); >> -void btrfs_set_range_writeback(struct btrfs_inode *inode, u64 start, u64 end); >> void btrfs_evict_inode(struct inode *inode); >> struct inode *btrfs_alloc_inode(struct super_block *sb); >> void btrfs_destroy_inode(struct inode *inode); >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >> index 9fbc83c76b94..d87dcafab537 100644 >> --- a/fs/btrfs/extent_io.c >> +++ b/fs/btrfs/extent_io.c >> @@ -1359,7 +1359,7 @@ static int submit_one_sector(struct btrfs_inode *inode, >> * a folio for a range already written to disk. >> */ >> btrfs_folio_clear_dirty(fs_info, folio, filepos, sectorsize); >> - btrfs_set_range_writeback(inode, filepos, filepos + sectorsize - 1); >> + btrfs_folio_set_writeback(fs_info, folio, filepos, sectorsize); >> /* >> * Above call should set the whole folio with writeback flag, even >> * just for a single subpage sector. >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c >> index 103ec917ca9d..21e51924742a 100644 >> --- a/fs/btrfs/inode.c >> +++ b/fs/btrfs/inode.c >> @@ -8939,28 +8939,6 @@ static int btrfs_tmpfile(struct mnt_idmap *idmap, struct inode *dir, >> return finish_open_simple(file, ret); >> } >> >> -void btrfs_set_range_writeback(struct btrfs_inode *inode, u64 start, u64 end) >> -{ >> - struct btrfs_fs_info *fs_info = inode->root->fs_info; >> - unsigned long index = start >> PAGE_SHIFT; >> - unsigned long end_index = end >> PAGE_SHIFT; >> - struct folio *folio; >> - u32 len; >> - >> - ASSERT(end + 1 - start <= U32_MAX); >> - len = end + 1 - start; >> - while (index <= end_index) { >> - folio = __filemap_get_folio(inode->vfs_inode.i_mapping, index, 0, 0); >> - ASSERT(!IS_ERR(folio)); /* folios should be in the extent_io_tree */ >> - >> - /* This is for data, which doesn't yet support larger folio. */ >> - ASSERT(folio_order(folio) == 0); >> - btrfs_folio_set_writeback(fs_info, folio, start, len); > > So the new code is just btrfs_folio_set_writeback(), with the removed > comment and assertion, Firstly, the length check is already inside btrfs_folio_set_writeback() for the subpage cases. If it's not subpage, we do not even need to check the range (it's always page aligned). Secondly for the folio, we do not need the ASSERT(), because this time we have the folio pointer already. So for the assert part, there is no change. > what's the status regarding large folios? That stays the same, no larger folio support. The larger folio support requires us to get rid of the per-fs sectors_per_page check, but using folio_size() to do the calculation. That will still be a lot of work to do before we can support larger folios for data. Thanks, Qu > > I assume that this now implicitly supports them, we don't necessarily > need the assertion as I think we have more places where this would be > detected. >
On Wed, Oct 09, 2024 at 07:26:36AM +1030, Qu Wenruo wrote: > >> --- a/fs/btrfs/inode.c > >> +++ b/fs/btrfs/inode.c > >> @@ -8939,28 +8939,6 @@ static int btrfs_tmpfile(struct mnt_idmap *idmap, struct inode *dir, > >> return finish_open_simple(file, ret); > >> } > >> > >> -void btrfs_set_range_writeback(struct btrfs_inode *inode, u64 start, u64 end) > >> -{ > >> - struct btrfs_fs_info *fs_info = inode->root->fs_info; > >> - unsigned long index = start >> PAGE_SHIFT; > >> - unsigned long end_index = end >> PAGE_SHIFT; > >> - struct folio *folio; > >> - u32 len; > >> - > >> - ASSERT(end + 1 - start <= U32_MAX); > >> - len = end + 1 - start; > >> - while (index <= end_index) { > >> - folio = __filemap_get_folio(inode->vfs_inode.i_mapping, index, 0, 0); > >> - ASSERT(!IS_ERR(folio)); /* folios should be in the extent_io_tree */ > >> - > >> - /* This is for data, which doesn't yet support larger folio. */ > >> - ASSERT(folio_order(folio) == 0); > >> - btrfs_folio_set_writeback(fs_info, folio, start, len); > > > > So the new code is just btrfs_folio_set_writeback(), with the removed > > comment and assertion, > > Firstly, the length check is already inside btrfs_folio_set_writeback() > for the subpage cases. > If it's not subpage, we do not even need to check the range (it's always > page aligned). > > Secondly for the folio, we do not need the ASSERT(), because this time > we have the folio pointer already. > > So for the assert part, there is no change. Ok. > > what's the status regarding large folios? > > That stays the same, no larger folio support. > > The larger folio support requires us to get rid of the per-fs > sectors_per_page check, but using folio_size() to do the calculation. > > That will still be a lot of work to do before we can support larger > folios for data. My question was about this specific place in the code, if we e.g. remove various assertions making sure we don't accidentally get there with large folios after they get enabled in the future. It's ok when other code makes different checks that would prevent it, but that's what I did not immediately see. Thanks.
在 2024/10/9 08:09, David Sterba 写道: > On Wed, Oct 09, 2024 at 07:26:36AM +1030, Qu Wenruo wrote: >>>> --- a/fs/btrfs/inode.c >>>> +++ b/fs/btrfs/inode.c >>>> @@ -8939,28 +8939,6 @@ static int btrfs_tmpfile(struct mnt_idmap *idmap, struct inode *dir, >>>> return finish_open_simple(file, ret); >>>> } >>>> >>>> -void btrfs_set_range_writeback(struct btrfs_inode *inode, u64 start, u64 end) >>>> -{ >>>> - struct btrfs_fs_info *fs_info = inode->root->fs_info; >>>> - unsigned long index = start >> PAGE_SHIFT; >>>> - unsigned long end_index = end >> PAGE_SHIFT; >>>> - struct folio *folio; >>>> - u32 len; >>>> - >>>> - ASSERT(end + 1 - start <= U32_MAX); >>>> - len = end + 1 - start; >>>> - while (index <= end_index) { >>>> - folio = __filemap_get_folio(inode->vfs_inode.i_mapping, index, 0, 0); >>>> - ASSERT(!IS_ERR(folio)); /* folios should be in the extent_io_tree */ >>>> - >>>> - /* This is for data, which doesn't yet support larger folio. */ >>>> - ASSERT(folio_order(folio) == 0); >>>> - btrfs_folio_set_writeback(fs_info, folio, start, len); >>> >>> So the new code is just btrfs_folio_set_writeback(), with the removed >>> comment and assertion, >> >> Firstly, the length check is already inside btrfs_folio_set_writeback() >> for the subpage cases. >> If it's not subpage, we do not even need to check the range (it's always >> page aligned). >> >> Secondly for the folio, we do not need the ASSERT(), because this time >> we have the folio pointer already. >> >> So for the assert part, there is no change. > > Ok. > >>> what's the status regarding large folios? >> >> That stays the same, no larger folio support. >> >> The larger folio support requires us to get rid of the per-fs >> sectors_per_page check, but using folio_size() to do the calculation. >> >> That will still be a lot of work to do before we can support larger >> folios for data. > > My question was about this specific place in the code, if we e.g. remove > various assertions making sure we don't accidentally get there with > large folios after they get enabled in the future. It's ok when other > code makes different checks that would prevent it, but that's what I did > not immediately see. Thanks. Oh, in that case we have quite some such checks already in place. For this particular case (buffered write path), we have ASSERT() checking for the folio order at end_bbio_data_read(), the same applies to the buffered read path too: ASSERT(folio_order(folio) == 0); So it's still fine, although a little late (at endio time vs at submission time). Furthermore if we're going subpage, btrfs_subpage_assert() also does the folio order check. So we should get the ASSERT()s triggered pretty easily if there is a larger folio passed in. Thanks, Qu
On Sun, Oct 06, 2024 at 11:10:22AM +1030, Qu Wenruo wrote: > The function btrfs_set_range_writeback() is originally a callback for > metadata and data, to mark a range with writeback flag. > > Then it was converted into a common function call for both metadata and > data. > > >From the very beginning, the function is only called on a full page, > later converted to handle range inside a page. > > But it never needs to handle multiple pages, and since commit > 8189197425e7 ("btrfs: refactor __extent_writepage_io() to do > sector-by-sector submission") the function is only called on a > sector-by-sector basis. > > This makes the function unnecessary, and can be converted to a simple > btrfs_folio_set_writeback() call instead. > > Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: David Sterba <dsterba@suse.com>
diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h index e152fde888fc..c514bab532fa 100644 --- a/fs/btrfs/btrfs_inode.h +++ b/fs/btrfs/btrfs_inode.h @@ -577,7 +577,6 @@ void btrfs_merge_delalloc_extent(struct btrfs_inode *inode, struct extent_state struct extent_state *other); void btrfs_split_delalloc_extent(struct btrfs_inode *inode, struct extent_state *orig, u64 split); -void btrfs_set_range_writeback(struct btrfs_inode *inode, u64 start, u64 end); void btrfs_evict_inode(struct inode *inode); struct inode *btrfs_alloc_inode(struct super_block *sb); void btrfs_destroy_inode(struct inode *inode); diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 9fbc83c76b94..d87dcafab537 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -1359,7 +1359,7 @@ static int submit_one_sector(struct btrfs_inode *inode, * a folio for a range already written to disk. */ btrfs_folio_clear_dirty(fs_info, folio, filepos, sectorsize); - btrfs_set_range_writeback(inode, filepos, filepos + sectorsize - 1); + btrfs_folio_set_writeback(fs_info, folio, filepos, sectorsize); /* * Above call should set the whole folio with writeback flag, even * just for a single subpage sector. diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 103ec917ca9d..21e51924742a 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -8939,28 +8939,6 @@ static int btrfs_tmpfile(struct mnt_idmap *idmap, struct inode *dir, return finish_open_simple(file, ret); } -void btrfs_set_range_writeback(struct btrfs_inode *inode, u64 start, u64 end) -{ - struct btrfs_fs_info *fs_info = inode->root->fs_info; - unsigned long index = start >> PAGE_SHIFT; - unsigned long end_index = end >> PAGE_SHIFT; - struct folio *folio; - u32 len; - - ASSERT(end + 1 - start <= U32_MAX); - len = end + 1 - start; - while (index <= end_index) { - folio = __filemap_get_folio(inode->vfs_inode.i_mapping, index, 0, 0); - ASSERT(!IS_ERR(folio)); /* folios should be in the extent_io_tree */ - - /* This is for data, which doesn't yet support larger folio. */ - ASSERT(folio_order(folio) == 0); - btrfs_folio_set_writeback(fs_info, folio, start, len); - folio_put(folio); - index++; - } -} - int btrfs_encoded_io_compression_from_extent(struct btrfs_fs_info *fs_info, int compress_type) {
The function btrfs_set_range_writeback() is originally a callback for metadata and data, to mark a range with writeback flag. Then it was converted into a common function call for both metadata and data. From the very beginning, the function is only called on a full page, later converted to handle range inside a page. But it never needs to handle multiple pages, and since commit 8189197425e7 ("btrfs: refactor __extent_writepage_io() to do sector-by-sector submission") the function is only called on a sector-by-sector basis. This makes the function unnecessary, and can be converted to a simple btrfs_folio_set_writeback() call instead. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/btrfs_inode.h | 1 - fs/btrfs/extent_io.c | 2 +- fs/btrfs/inode.c | 22 ---------------------- 3 files changed, 1 insertion(+), 24 deletions(-)