| Message ID | 20231208192725.1523194-1-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] btrfs: Convert defrag_prepare_one_page() to use a folio | expand |
On 2023/12/9 05:57, Matthew Wilcox (Oracle) wrote: > Use a folio throughout defrag_prepare_one_page() to remove dozens of > hidden calls to compound_head(). There is no support here for large > folios; indeed, turn the existing check for PageCompound into a check > for large folios. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Looks good to me. Although some comments inlined below > --- > fs/btrfs/defrag.c | 53 ++++++++++++++++++++++++----------------------- > 1 file changed, 27 insertions(+), 26 deletions(-) > > diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c > index a9a068af8d6e..17a13d3ed131 100644 > --- a/fs/btrfs/defrag.c > +++ b/fs/btrfs/defrag.c > @@ -868,13 +868,14 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i > u64 page_start = (u64)index << PAGE_SHIFT; > u64 page_end = page_start + PAGE_SIZE - 1; > struct extent_state *cached_state = NULL; > - struct page *page; > + struct folio *folio; > int ret; > > again: > - page = find_or_create_page(mapping, index, mask); > - if (!page) > - return ERR_PTR(-ENOMEM); > + folio = __filemap_get_folio(mapping, index, > + FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask); When I was (and still am) a newbie to the folio interfaces, the "__" prefix is driving me away to use it. Mind to change it in the future? Like adding a new filemap_get_or_create_folio()? > + if (IS_ERR(folio)) > + return &folio->page; > > /* > * Since we can defragment files opened read-only, we can encounter > @@ -884,16 +885,16 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i > * executables that explicitly enable them, so this isn't very > * restrictive. > */ > - if (PageCompound(page)) { > - unlock_page(page); > - put_page(page); > + if (folio_test_large(folio)) { > + folio_unlock(folio); > + folio_put(folio); > return ERR_PTR(-ETXTBSY); > } > > - ret = set_page_extent_mapped(page); > + ret = set_page_extent_mapped(&folio->page); With my recent patches, set_page_extent_mapped() is already using folio interfaces, I guess it's time to finally change it to accept a folio. > if (ret < 0) { > - unlock_page(page); > - put_page(page); > + folio_unlock(folio); > + folio_put(folio); > return ERR_PTR(ret); > } > > @@ -908,17 +909,17 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i > if (!ordered) > break; > > - unlock_page(page); > + folio_unlock(folio); > btrfs_start_ordered_extent(ordered); > btrfs_put_ordered_extent(ordered); > - lock_page(page); > + folio_lock(folio); > /* > - * We unlocked the page above, so we need check if it was > + * We unlocked the folio above, so we need check if it was > * released or not. > */ > - if (page->mapping != mapping || !PagePrivate(page)) { > - unlock_page(page); > - put_page(page); > + if (folio->mapping != mapping || !folio->private) { This folio->private check is not the same as PagePrivate(page) IIRC. Isn't folio_test_private() more suitable here? Thanks, Qu > + folio_unlock(folio); > + folio_put(folio); > goto again; > } > } > @@ -927,21 +928,21 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i > * Now the page range has no ordered extent any more. Read the page to > * make it uptodate. > */ > - if (!PageUptodate(page)) { > - btrfs_read_folio(NULL, page_folio(page)); > - lock_page(page); > - if (page->mapping != mapping || !PagePrivate(page)) { > - unlock_page(page); > - put_page(page); > + if (!folio_test_uptodate(folio)) { > + btrfs_read_folio(NULL, folio); > + folio_lock(folio); > + if (folio->mapping != mapping || !folio->private) { > + folio_unlock(folio); > + folio_put(folio); > goto again; > } > - if (!PageUptodate(page)) { > - unlock_page(page); > - put_page(page); > + if (!folio_test_uptodate(folio)) { > + folio_unlock(folio); > + folio_put(folio); > return ERR_PTR(-EIO); > } > } > - return page; > + return &folio->page; > } > > struct defrag_target_range {
On Sat, Dec 09, 2023 at 07:26:45AM +1030, Qu Wenruo wrote: > > again: > > - page = find_or_create_page(mapping, index, mask); > > - if (!page) > > - return ERR_PTR(-ENOMEM); > > + folio = __filemap_get_folio(mapping, index, > > + FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask); > > When I was (and still am) a newbie to the folio interfaces, the "__" > prefix is driving me away to use it. > > Mind to change it in the future? Like adding a new > filemap_get_or_create_folio()? I'm always open to better naming ideas. We definitely have too many inline functions that call pagecache_get_page(): find_get_page() find_get_page_flags() find_lock_page() find_or_create_page() grab_cache_page_nowait() ... and I don't think anyone could tell you when to use which one without looking at the implementations or cribbing from another filesystem. So I've tried to keep it simple for the folio replacements and so far we only have: filemap_get_folio() filemap_lock_folio() filemap_grab_folio() and honestly, I don't love filemap_grab_folio() and I'm regretting its creation. What I do like is the creation of FGP_WRITEBEGIN, but that doesn't address your concern about the leading __. Would you be happier if we renamed __filemap_get_folio() to filemap_get_folio_flags()? This would all be much better if C allowed you to specify default arguments to functions :-P > > - ret = set_page_extent_mapped(page); > > + ret = set_page_extent_mapped(&folio->page); > > With my recent patches, set_page_extent_mapped() is already using folio > interfaces, I guess it's time to finally change it to accept a folio. I'll add this as a prep patch: diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index b5a2399ed420..99ae54ab004c 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -909,18 +909,22 @@ static int attach_extent_buffer_page(struct extent_buffer *eb, int set_page_extent_mapped(struct page *page) { - struct folio *folio = page_folio(page); + return set_folio_extent_mapped(page_folio(page)); +} + +int set_folio_extent_mapped(struct folio *folio) +{ struct btrfs_fs_info *fs_info; - ASSERT(page->mapping); + ASSERT(folio->mapping); if (folio_test_private(folio)) return 0; - fs_info = btrfs_sb(page->mapping->host->i_sb); + fs_info = btrfs_sb(folio->mapping->host->i_sb); - if (btrfs_is_subpage(fs_info, page)) - return btrfs_attach_subpage(fs_info, page, BTRFS_SUBPAGE_DATA); + if (btrfs_is_subpage(fs_info, &folio->page)) + return btrfs_attach_subpage(fs_info, &folio->page, BTRFS_SUBPAGE_DATA); folio_attach_private(folio, (void *)EXTENT_FOLIO_PRIVATE); return 0; diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h index c73d53c22ec5..b6b31fb41bdf 100644 --- a/fs/btrfs/extent_io.h +++ b/fs/btrfs/extent_io.h @@ -202,6 +202,7 @@ int btree_write_cache_pages(struct address_space *mapping, void extent_readahead(struct readahead_control *rac); int extent_fiemap(struct btrfs_inode *inode, struct fiemap_extent_info *fieinfo, u64 start, u64 len); +int set_folio_extent_mapped(struct folio *folio); int set_page_extent_mapped(struct page *page); void clear_page_extent_mapped(struct page *page); > > - if (page->mapping != mapping || !PagePrivate(page)) { > > - unlock_page(page); > > - put_page(page); > > + if (folio->mapping != mapping || !folio->private) { > > This folio->private check is not the same as PagePrivate(page) IIRC. > Isn't folio_test_private() more suitable here? One of the projects I'm pursuing on the side is getting rid of PG_private. There's really no need to burn a very precious page flag telling us whether the filesystem has something stored in folio->private when we could just look at folio->private instead. So yes, generates different code, but the two should be the same.
On 2023/12/9 07:46, Matthew Wilcox wrote: > On Sat, Dec 09, 2023 at 07:26:45AM +1030, Qu Wenruo wrote: >>> again: >>> - page = find_or_create_page(mapping, index, mask); >>> - if (!page) >>> - return ERR_PTR(-ENOMEM); >>> + folio = __filemap_get_folio(mapping, index, >>> + FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask); >> >> When I was (and still am) a newbie to the folio interfaces, the "__" >> prefix is driving me away to use it. >> >> Mind to change it in the future? Like adding a new >> filemap_get_or_create_folio()? > > I'm always open to better naming ideas. We definitely have too many > inline functions that call pagecache_get_page(): > > find_get_page() > find_get_page_flags() > find_lock_page() > find_or_create_page() > grab_cache_page_nowait() > > ... and I don't think anyone could tell you when to use which one > without looking at the implementations or cribbing from another > filesystem. > > So I've tried to keep it simple for the folio replacements and so > far we only have: > > filemap_get_folio() > filemap_lock_folio() > filemap_grab_folio() > > and honestly, I don't love filemap_grab_folio() and I'm regretting > its creation. In that case, I'd prefer just a single filemap_get_folio(), passing all the GFP and FGP flags just like __filemap_get_folio(), and get rid of any wrappers. In fact, I'd say the __filemap_get_folio() with all the FGP flags is easier to read to me at least. (And with direct FGP control for callers, we can make it easier to allocate higher order folios if the caller wants) > > What I do like is the creation of FGP_WRITEBEGIN, but that doesn't > address your concern about the leading __. Would you be happier if > we renamed __filemap_get_folio() to filemap_get_folio_flags()? Oh, with a _flags() suffix it would be also a good idea. > > This would all be much better if C allowed you to specify default > arguments to functions :-P > >>> - ret = set_page_extent_mapped(page); >>> + ret = set_page_extent_mapped(&folio->page); >> >> With my recent patches, set_page_extent_mapped() is already using folio >> interfaces, I guess it's time to finally change it to accept a folio. > > I'll add this as a prep patch: > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index b5a2399ed420..99ae54ab004c 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -909,18 +909,22 @@ static int attach_extent_buffer_page(struct extent_buffer *eb, > > int set_page_extent_mapped(struct page *page) > { > - struct folio *folio = page_folio(page); > + return set_folio_extent_mapped(page_folio(page)); > +} > + > +int set_folio_extent_mapped(struct folio *folio) > +{ > struct btrfs_fs_info *fs_info; > > - ASSERT(page->mapping); > + ASSERT(folio->mapping); > > if (folio_test_private(folio)) > return 0; > > - fs_info = btrfs_sb(page->mapping->host->i_sb); > + fs_info = btrfs_sb(folio->mapping->host->i_sb); > > - if (btrfs_is_subpage(fs_info, page)) > - return btrfs_attach_subpage(fs_info, page, BTRFS_SUBPAGE_DATA); > + if (btrfs_is_subpage(fs_info, &folio->page)) > + return btrfs_attach_subpage(fs_info, &folio->page, BTRFS_SUBPAGE_DATA); > > folio_attach_private(folio, (void *)EXTENT_FOLIO_PRIVATE); > return 0; > diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h > index c73d53c22ec5..b6b31fb41bdf 100644 > --- a/fs/btrfs/extent_io.h > +++ b/fs/btrfs/extent_io.h > @@ -202,6 +202,7 @@ int btree_write_cache_pages(struct address_space *mapping, > void extent_readahead(struct readahead_control *rac); > int extent_fiemap(struct btrfs_inode *inode, struct fiemap_extent_info *fieinfo, > u64 start, u64 len); > +int set_folio_extent_mapped(struct folio *folio); > int set_page_extent_mapped(struct page *page); > void clear_page_extent_mapped(struct page *page); > > >>> - if (page->mapping != mapping || !PagePrivate(page)) { >>> - unlock_page(page); >>> - put_page(page); >>> + if (folio->mapping != mapping || !folio->private) { >> >> This folio->private check is not the same as PagePrivate(page) IIRC. >> Isn't folio_test_private() more suitable here? > > One of the projects I'm pursuing on the side is getting rid of PG_private. > There's really no need to burn a very precious page flag telling us > whether the filesystem has something stored in folio->private when we > could just look at folio->private instead. > > So yes, generates different code, but the two should be the same. OK, got it. And hoping before we can get rid of PG_private, we can get rid of page usage inside btrfs first. Thanks, Qu
diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c index a9a068af8d6e..17a13d3ed131 100644 --- a/fs/btrfs/defrag.c +++ b/fs/btrfs/defrag.c @@ -868,13 +868,14 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i u64 page_start = (u64)index << PAGE_SHIFT; u64 page_end = page_start + PAGE_SIZE - 1; struct extent_state *cached_state = NULL; - struct page *page; + struct folio *folio; int ret; again: - page = find_or_create_page(mapping, index, mask); - if (!page) - return ERR_PTR(-ENOMEM); + folio = __filemap_get_folio(mapping, index, + FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask); + if (IS_ERR(folio)) + return &folio->page; /* * Since we can defragment files opened read-only, we can encounter @@ -884,16 +885,16 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i * executables that explicitly enable them, so this isn't very * restrictive. */ - if (PageCompound(page)) { - unlock_page(page); - put_page(page); + if (folio_test_large(folio)) { + folio_unlock(folio); + folio_put(folio); return ERR_PTR(-ETXTBSY); } - ret = set_page_extent_mapped(page); + ret = set_page_extent_mapped(&folio->page); if (ret < 0) { - unlock_page(page); - put_page(page); + folio_unlock(folio); + folio_put(folio); return ERR_PTR(ret); } @@ -908,17 +909,17 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i if (!ordered) break; - unlock_page(page); + folio_unlock(folio); btrfs_start_ordered_extent(ordered); btrfs_put_ordered_extent(ordered); - lock_page(page); + folio_lock(folio); /* - * We unlocked the page above, so we need check if it was + * We unlocked the folio above, so we need check if it was * released or not. */ - if (page->mapping != mapping || !PagePrivate(page)) { - unlock_page(page); - put_page(page); + if (folio->mapping != mapping || !folio->private) { + folio_unlock(folio); + folio_put(folio); goto again; } } @@ -927,21 +928,21 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i * Now the page range has no ordered extent any more. Read the page to * make it uptodate. */ - if (!PageUptodate(page)) { - btrfs_read_folio(NULL, page_folio(page)); - lock_page(page); - if (page->mapping != mapping || !PagePrivate(page)) { - unlock_page(page); - put_page(page); + if (!folio_test_uptodate(folio)) { + btrfs_read_folio(NULL, folio); + folio_lock(folio); + if (folio->mapping != mapping || !folio->private) { + folio_unlock(folio); + folio_put(folio); goto again; } - if (!PageUptodate(page)) { - unlock_page(page); - put_page(page); + if (!folio_test_uptodate(folio)) { + folio_unlock(folio); + folio_put(folio); return ERR_PTR(-EIO); } } - return page; + return &folio->page; } struct defrag_target_range {
Use a folio throughout defrag_prepare_one_page() to remove dozens of hidden calls to compound_head(). There is no support here for large folios; indeed, turn the existing check for PageCompound into a check for large folios. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- fs/btrfs/defrag.c | 53 ++++++++++++++++++++++++----------------------- 1 file changed, 27 insertions(+), 26 deletions(-)