| Message ID | 20220606204050.2625949-6-willy@infradead.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Convert aops->migratepage to aops->migrate_folio | expand |
On Mon, Jun 06, 2022 at 09:40:35PM +0100, Matthew Wilcox (Oracle) wrote: > Now that both callers have a folio, convert this function to > take a folio & rename it. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > mm/migrate.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index 77b8c662c9ca..e0a593e5b5f9 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -337,13 +337,18 @@ void pmd_migration_entry_wait(struct mm_struct *mm, pmd_t *pmd) > } > #endif > > -static int expected_page_refs(struct address_space *mapping, struct page *page) > +static int folio_expected_refs(struct address_space *mapping, > + struct folio *folio) > { > - int expected_count = 1; > + int refs = 1; > + if (!mapping) > + return refs; > > - if (mapping) > - expected_count += compound_nr(page) + page_has_private(page); > - return expected_count; > + refs += folio_nr_pages(folio); > + if (folio_get_private(folio)) > + refs++; Why not folio_has_private() (as seems to be used for later page_has_private() conversions) here? > + > + return refs;; Nit: extra ; Brian > } > > /* > @@ -360,7 +365,7 @@ int folio_migrate_mapping(struct address_space *mapping, > XA_STATE(xas, &mapping->i_pages, folio_index(folio)); > struct zone *oldzone, *newzone; > int dirty; > - int expected_count = expected_page_refs(mapping, &folio->page) + extra_count; > + int expected_count = folio_expected_refs(mapping, folio) + extra_count; > long nr = folio_nr_pages(folio); > > if (!mapping) { > @@ -670,7 +675,7 @@ static int __buffer_migrate_folio(struct address_space *mapping, > return migrate_page(mapping, &dst->page, &src->page, mode); > > /* Check whether page does not have extra refs before we do more work */ > - expected_count = expected_page_refs(mapping, &src->page); > + expected_count = folio_expected_refs(mapping, src); > if (folio_ref_count(src) != expected_count) > return -EAGAIN; > > -- > 2.35.1 > >
On Tue, Jun 07, 2022 at 09:41:57AM -0400, Brian Foster wrote: > On Mon, Jun 06, 2022 at 09:40:35PM +0100, Matthew Wilcox (Oracle) wrote: > > -static int expected_page_refs(struct address_space *mapping, struct page *page) > > +static int folio_expected_refs(struct address_space *mapping, > > + struct folio *folio) > > { > > - int expected_count = 1; > > + int refs = 1; > > + if (!mapping) > > + return refs; > > > > - if (mapping) > > - expected_count += compound_nr(page) + page_has_private(page); > > - return expected_count; > > + refs += folio_nr_pages(folio); > > + if (folio_get_private(folio)) > > + refs++; > > Why not folio_has_private() (as seems to be used for later > page_has_private() conversions) here? We have a horrid confusion that I'm trying to clean up stealthily without anyone noticing. I would have gotten away with it too if it weren't for you pesky kids. #define PAGE_FLAGS_PRIVATE \ (1UL << PG_private | 1UL << PG_private_2) static inline int page_has_private(struct page *page) { return !!(page->flags & PAGE_FLAGS_PRIVATE); } So what this function is saying is that there is one extra refcount expected on the struct page if PG_private _or_ PG_private_2 is set. How are filesystems expected to manage their page's refcount with this rule? Increment the refcount when setting PG_private unless PG_private_2 is already set? Decrement the refcount when clearing PG_private_2 unless PG_private is set? This is garbage. IMO, PG_private_2 should have no bearing on the page's refcount. Only btrfs and the netfs's use private_2 and neither of them do anything to the refcount when setting/clearing it. So that's what I'm implementing here. > > + > > + return refs;; > > Nit: extra ; Oh, that's where it went ;-) I had a compile error due to a missing semicolon at some point, and thought it was just a typo ...
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/mm/migrate.c b/mm/migrate.c index 77b8c662c9ca..e0a593e5b5f9 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -337,13 +337,18 @@ void pmd_migration_entry_wait(struct mm_struct *mm, pmd_t *pmd) } #endif -static int expected_page_refs(struct address_space *mapping, struct page *page) +static int folio_expected_refs(struct address_space *mapping, + struct folio *folio) { - int expected_count = 1; + int refs = 1; + if (!mapping) + return refs; - if (mapping) - expected_count += compound_nr(page) + page_has_private(page); - return expected_count; + refs += folio_nr_pages(folio); + if (folio_get_private(folio)) + refs++; + + return refs;; } /* @@ -360,7 +365,7 @@ int folio_migrate_mapping(struct address_space *mapping, XA_STATE(xas, &mapping->i_pages, folio_index(folio)); struct zone *oldzone, *newzone; int dirty; - int expected_count = expected_page_refs(mapping, &folio->page) + extra_count; + int expected_count = folio_expected_refs(mapping, folio) + extra_count; long nr = folio_nr_pages(folio); if (!mapping) { @@ -670,7 +675,7 @@ static int __buffer_migrate_folio(struct address_space *mapping, return migrate_page(mapping, &dst->page, &src->page, mode); /* Check whether page does not have extra refs before we do more work */ - expected_count = expected_page_refs(mapping, &src->page); + expected_count = folio_expected_refs(mapping, src); if (folio_ref_count(src) != expected_count) return -EAGAIN;
Now that both callers have a folio, convert this function to take a folio & rename it. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- mm/migrate.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)