Now that both callers have a folio, convert this function to
take a folio & rename it.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
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++;
+
+ 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;
--
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 ...
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) <[email protected]>
> ---
> 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
>
>
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>