| Message ID | 20231208192725.1523194-2-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: > Remove more hidden calls to compound_head() by using an array of folios > instead of pages. Also neaten the error path in defrag_one_range() by > adjusting the length of the array instead of checking for NULL. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Looks good to me. Reviewed-by: Qu Wenruo <wqu@suse.com> This time only some unrelated questions below. > --- > fs/btrfs/defrag.c | 44 +++++++++++++++++++++----------------------- > 1 file changed, 21 insertions(+), 23 deletions(-) > > diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c > index 17a13d3ed131..4b94362779fe 100644 > --- a/fs/btrfs/defrag.c > +++ b/fs/btrfs/defrag.c > @@ -861,7 +861,7 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em, > * NOTE: Caller should also wait for page writeback after the cluster is > * prepared, here we don't do writeback wait for each page. > */ > -static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t index) > +static struct folio *defrag_prepare_one_folio(struct btrfs_inode *inode, pgoff_t index) > { > struct address_space *mapping = inode->vfs_inode.i_mapping; > gfp_t mask = btrfs_alloc_write_mask(mapping); > @@ -875,7 +875,7 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i > folio = __filemap_get_folio(mapping, index, > FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask); > if (IS_ERR(folio)) > - return &folio->page; What's the proper way to grab the first page of a folio? For now, I'm going folio_page() and it's the same wrapper using folio->page, but I'm wondering would there be any recommendation for it. > + return folio; > > /* > * Since we can defragment files opened read-only, we can encounter > @@ -942,7 +942,7 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i > return ERR_PTR(-EIO); > } > } > - return &folio->page; > + return folio; > } > > struct defrag_target_range { > @@ -1163,7 +1163,7 @@ static_assert(PAGE_ALIGNED(CLUSTER_SIZE)); > */ > static int defrag_one_locked_target(struct btrfs_inode *inode, > struct defrag_target_range *target, > - struct page **pages, int nr_pages, > + struct folio **folios, int nr_pages, > struct extent_state **cached_state) > { > struct btrfs_fs_info *fs_info = inode->root->fs_info; > @@ -1172,7 +1172,7 @@ static int defrag_one_locked_target(struct btrfs_inode *inode, > const u64 len = target->len; > unsigned long last_index = (start + len - 1) >> PAGE_SHIFT; > unsigned long start_index = start >> PAGE_SHIFT; > - unsigned long first_index = page_index(pages[0]); > + unsigned long first_index = folios[0]->index; The same for index, there is a folio_index() wrapper. So should we go the wrapper or would the wrapper be gone in the future? > int ret = 0; > int i; > > @@ -1189,8 +1189,8 @@ static int defrag_one_locked_target(struct btrfs_inode *inode, > > /* Update the page status */ > for (i = start_index - first_index; i <= last_index - first_index; i++) { > - ClearPageChecked(pages[i]); > - btrfs_page_clamp_set_dirty(fs_info, pages[i], start, len); > + folio_clear_checked(folios[i]); > + btrfs_page_clamp_set_dirty(fs_info, &folios[i]->page, start, len); After my patch "2/3 btrfs: migrate subpage code to folio interfaces", btrfs_page_*() helpers accept folio parameter directly, thus this may lead to a conflicts. Thanks, Qu > } > btrfs_delalloc_release_extents(inode, len); > extent_changeset_free(data_reserved); > @@ -1206,7 +1206,7 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len, > struct defrag_target_range *entry; > struct defrag_target_range *tmp; > LIST_HEAD(target_list); > - struct page **pages; > + struct folio **folios; > const u32 sectorsize = inode->root->fs_info->sectorsize; > u64 last_index = (start + len - 1) >> PAGE_SHIFT; > u64 start_index = start >> PAGE_SHIFT; > @@ -1217,21 +1217,21 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len, > ASSERT(nr_pages <= CLUSTER_SIZE / PAGE_SIZE); > ASSERT(IS_ALIGNED(start, sectorsize) && IS_ALIGNED(len, sectorsize)); > > - pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS); > - if (!pages) > + folios = kcalloc(nr_pages, sizeof(struct folio *), GFP_NOFS); > + if (!folios) > return -ENOMEM; > > /* Prepare all pages */ > for (i = 0; i < nr_pages; i++) { > - pages[i] = defrag_prepare_one_page(inode, start_index + i); > - if (IS_ERR(pages[i])) { > - ret = PTR_ERR(pages[i]); > - pages[i] = NULL; > - goto free_pages; > + folios[i] = defrag_prepare_one_folio(inode, start_index + i); > + if (IS_ERR(folios[i])) { > + ret = PTR_ERR(folios[i]); > + nr_pages = i; > + goto free_folios; > } > } > for (i = 0; i < nr_pages; i++) > - wait_on_page_writeback(pages[i]); > + folio_wait_writeback(folios[i]); > > /* Lock the pages range */ > lock_extent(&inode->io_tree, start_index << PAGE_SHIFT, > @@ -1251,7 +1251,7 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len, > goto unlock_extent; > > list_for_each_entry(entry, &target_list, list) { > - ret = defrag_one_locked_target(inode, entry, pages, nr_pages, > + ret = defrag_one_locked_target(inode, entry, folios, nr_pages, > &cached_state); > if (ret < 0) > break; > @@ -1265,14 +1265,12 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len, > unlock_extent(&inode->io_tree, start_index << PAGE_SHIFT, > (last_index << PAGE_SHIFT) + PAGE_SIZE - 1, > &cached_state); > -free_pages: > +free_folios: > for (i = 0; i < nr_pages; i++) { > - if (pages[i]) { > - unlock_page(pages[i]); > - put_page(pages[i]); > - } > + folio_unlock(folios[i]); > + folio_put(folios[i]); > } > - kfree(pages); > + kfree(folios); > return ret; > } >
On Sat, Dec 09, 2023 at 07:44:34AM +1030, Qu Wenruo wrote: > > @@ -875,7 +875,7 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i > > folio = __filemap_get_folio(mapping, index, > > FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask); > > if (IS_ERR(folio)) > > - return &folio->page; > > What's the proper way to grab the first page of a folio? It depends why you're doing it. If you're just doing it temporarily to convert back from folio to page so you can make progress on the conversion, use &folio->page. That is a "bad code smell", but acceptable while you're doing the conversion. It's a good signal "there is more work here to be done". > For now, I'm going folio_page() and it's the same wrapper using folio->page, > but I'm wondering would there be any recommendation for it. folio_page() is good when you actually need a particular page. eg you're going to call kmap(). When we get to finally divorcing folio from page, folio_page() will still work, and &folio->page won't. > > @@ -1172,7 +1172,7 @@ static int defrag_one_locked_target(struct btrfs_inode *inode, > > const u64 len = target->len; > > unsigned long last_index = (start + len - 1) >> PAGE_SHIFT; > > unsigned long start_index = start >> PAGE_SHIFT; > > - unsigned long first_index = page_index(pages[0]); > > + unsigned long first_index = folios[0]->index; > > The same for index, there is a folio_index() wrapper. > > So should we go the wrapper or would the wrapper be gone in the future? folio_index() has a specific purpose (just like page_index() did, but it seems like a lot of filesystem people never read the kernel-doc on it). In short, filesystems never need to call folio_index(), nor page_index(). You can always just use folio->index unless you're in the direct-io path (and you shouldn't be looking at folio_index() then either!). > > @@ -1189,8 +1189,8 @@ static int defrag_one_locked_target(struct btrfs_inode *inode, > > /* Update the page status */ > > for (i = start_index - first_index; i <= last_index - first_index; i++) { > > - ClearPageChecked(pages[i]); > > - btrfs_page_clamp_set_dirty(fs_info, pages[i], start, len); > > + folio_clear_checked(folios[i]); > > + btrfs_page_clamp_set_dirty(fs_info, &folios[i]->page, start, len); > > After my patch "2/3 btrfs: migrate subpage code to folio interfaces", > btrfs_page_*() helpers accept folio parameter directly, thus this may lead > to a conflicts. I built against linux-next 20231207; I thought all your folio changes had landed there? If you want to adopt these patches and land them after yours, I am more than OK with that!
On 2023/12/9 07:54, Matthew Wilcox wrote: > On Sat, Dec 09, 2023 at 07:44:34AM +1030, Qu Wenruo wrote: >>> @@ -875,7 +875,7 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i >>> folio = __filemap_get_folio(mapping, index, >>> FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask); >>> if (IS_ERR(folio)) >>> - return &folio->page; >> >> What's the proper way to grab the first page of a folio? > > It depends why you're doing it. > > If you're just doing it temporarily to convert back from folio to page > so you can make progress on the conversion, use &folio->page. That > is a "bad code smell", but acceptable while you're doing the conversion. > It's a good signal "there is more work here to be done". > >> For now, I'm going folio_page() and it's the same wrapper using folio->page, >> but I'm wondering would there be any recommendation for it. > > folio_page() is good when you actually need a particular page. eg you're > going to call kmap(). When we get to finally divorcing folio from page, > folio_page() will still work, and &folio->page won't. > >>> @@ -1172,7 +1172,7 @@ static int defrag_one_locked_target(struct btrfs_inode *inode, >>> const u64 len = target->len; >>> unsigned long last_index = (start + len - 1) >> PAGE_SHIFT; >>> unsigned long start_index = start >> PAGE_SHIFT; >>> - unsigned long first_index = page_index(pages[0]); >>> + unsigned long first_index = folios[0]->index; >> >> The same for index, there is a folio_index() wrapper. >> >> So should we go the wrapper or would the wrapper be gone in the future? > > folio_index() has a specific purpose (just like page_index() did, but it > seems like a lot of filesystem people never read the kernel-doc on it). Indeed I should check the code, it says very clearly: * If you know * the page is definitely in the page cache, you can look at the folio's * index directly. > In short, filesystems never need to call folio_index(), nor page_index(). Yep, as when fs got the page they are mostly already mapped. > You can always just use folio->index unless you're in the direct-io path > (and you shouldn't be looking at folio_index() then either!). And DIO pages are never mapped thus never call that. > >>> @@ -1189,8 +1189,8 @@ static int defrag_one_locked_target(struct btrfs_inode *inode, >>> /* Update the page status */ >>> for (i = start_index - first_index; i <= last_index - first_index; i++) { >>> - ClearPageChecked(pages[i]); >>> - btrfs_page_clamp_set_dirty(fs_info, pages[i], start, len); >>> + folio_clear_checked(folios[i]); >>> + btrfs_page_clamp_set_dirty(fs_info, &folios[i]->page, start, len); >> >> After my patch "2/3 btrfs: migrate subpage code to folio interfaces", >> btrfs_page_*() helpers accept folio parameter directly, thus this may lead >> to a conflicts. > > I built against linux-next 20231207; I thought all your folio changes > had landed there? If you want to adopt these patches and land them > after yours, I am more than OK with that! No big deal, David and I can definitely handle the conflicts, no need to bother you. Thanks a lot for the lesson on the folio interface best practice! Qu >
diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c index 17a13d3ed131..4b94362779fe 100644 --- a/fs/btrfs/defrag.c +++ b/fs/btrfs/defrag.c @@ -861,7 +861,7 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em, * NOTE: Caller should also wait for page writeback after the cluster is * prepared, here we don't do writeback wait for each page. */ -static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t index) +static struct folio *defrag_prepare_one_folio(struct btrfs_inode *inode, pgoff_t index) { struct address_space *mapping = inode->vfs_inode.i_mapping; gfp_t mask = btrfs_alloc_write_mask(mapping); @@ -875,7 +875,7 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i folio = __filemap_get_folio(mapping, index, FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask); if (IS_ERR(folio)) - return &folio->page; + return folio; /* * Since we can defragment files opened read-only, we can encounter @@ -942,7 +942,7 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i return ERR_PTR(-EIO); } } - return &folio->page; + return folio; } struct defrag_target_range { @@ -1163,7 +1163,7 @@ static_assert(PAGE_ALIGNED(CLUSTER_SIZE)); */ static int defrag_one_locked_target(struct btrfs_inode *inode, struct defrag_target_range *target, - struct page **pages, int nr_pages, + struct folio **folios, int nr_pages, struct extent_state **cached_state) { struct btrfs_fs_info *fs_info = inode->root->fs_info; @@ -1172,7 +1172,7 @@ static int defrag_one_locked_target(struct btrfs_inode *inode, const u64 len = target->len; unsigned long last_index = (start + len - 1) >> PAGE_SHIFT; unsigned long start_index = start >> PAGE_SHIFT; - unsigned long first_index = page_index(pages[0]); + unsigned long first_index = folios[0]->index; int ret = 0; int i; @@ -1189,8 +1189,8 @@ static int defrag_one_locked_target(struct btrfs_inode *inode, /* Update the page status */ for (i = start_index - first_index; i <= last_index - first_index; i++) { - ClearPageChecked(pages[i]); - btrfs_page_clamp_set_dirty(fs_info, pages[i], start, len); + folio_clear_checked(folios[i]); + btrfs_page_clamp_set_dirty(fs_info, &folios[i]->page, start, len); } btrfs_delalloc_release_extents(inode, len); extent_changeset_free(data_reserved); @@ -1206,7 +1206,7 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len, struct defrag_target_range *entry; struct defrag_target_range *tmp; LIST_HEAD(target_list); - struct page **pages; + struct folio **folios; const u32 sectorsize = inode->root->fs_info->sectorsize; u64 last_index = (start + len - 1) >> PAGE_SHIFT; u64 start_index = start >> PAGE_SHIFT; @@ -1217,21 +1217,21 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len, ASSERT(nr_pages <= CLUSTER_SIZE / PAGE_SIZE); ASSERT(IS_ALIGNED(start, sectorsize) && IS_ALIGNED(len, sectorsize)); - pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS); - if (!pages) + folios = kcalloc(nr_pages, sizeof(struct folio *), GFP_NOFS); + if (!folios) return -ENOMEM; /* Prepare all pages */ for (i = 0; i < nr_pages; i++) { - pages[i] = defrag_prepare_one_page(inode, start_index + i); - if (IS_ERR(pages[i])) { - ret = PTR_ERR(pages[i]); - pages[i] = NULL; - goto free_pages; + folios[i] = defrag_prepare_one_folio(inode, start_index + i); + if (IS_ERR(folios[i])) { + ret = PTR_ERR(folios[i]); + nr_pages = i; + goto free_folios; } } for (i = 0; i < nr_pages; i++) - wait_on_page_writeback(pages[i]); + folio_wait_writeback(folios[i]); /* Lock the pages range */ lock_extent(&inode->io_tree, start_index << PAGE_SHIFT, @@ -1251,7 +1251,7 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len, goto unlock_extent; list_for_each_entry(entry, &target_list, list) { - ret = defrag_one_locked_target(inode, entry, pages, nr_pages, + ret = defrag_one_locked_target(inode, entry, folios, nr_pages, &cached_state); if (ret < 0) break; @@ -1265,14 +1265,12 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len, unlock_extent(&inode->io_tree, start_index << PAGE_SHIFT, (last_index << PAGE_SHIFT) + PAGE_SIZE - 1, &cached_state); -free_pages: +free_folios: for (i = 0; i < nr_pages; i++) { - if (pages[i]) { - unlock_page(pages[i]); - put_page(pages[i]); - } + folio_unlock(folios[i]); + folio_put(folios[i]); } - kfree(pages); + kfree(folios); return ret; }
Remove more hidden calls to compound_head() by using an array of folios instead of pages. Also neaten the error path in defrag_one_range() by adjusting the length of the array instead of checking for NULL. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- fs/btrfs/defrag.c | 44 +++++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 23 deletions(-)