Re: [PATCH v2 1/4] mm: migrate: Move the page count validation to the proper place

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 2021/8/15 18:34, Matthew Wilcox wrote:
> On Sun, Aug 15, 2021 at 02:23:03PM +0800, Baolin Wang wrote:
>> We've got the expected count for anonymous page or file page by
>> expected_page_refs() at the beginning of migrate_page_move_mapping(),
>> thus we should move the page count validation a little forward to
>> reduce duplicated code.
>>
>> Moreover the i_pages lock is not used to guarantee the page refcount
>> safety in migrate_page_move_mapping(), so we can move the getting page
>> count out of the i_pages lock. Since now the migration page has
>> established a migration pte under the page lock now, with the page
>> refcount freezing validation, to ensure that the page references
>> meet the migration requirement.
> 
> I remain unconvinced by this.
> 
> Looking at folio_migrate_mapping() a little more deeply, I don't
> understand why we first check folio_ref_count() and then attempt
> to free the refcount.  Why not just try to freeze it directly?
> 
> ie instead of your patch, this:
> 
> +++ b/mm/migrate.c
> @@ -403,13 +403,8 @@ int folio_migrate_mapping(struct address_space *mapping,
>          newzone = folio_zone(newfolio);
> 
>          xas_lock_irq(&xas);
> -       if (folio_ref_count(folio) != expected_count ||
> -           xas_load(&xas) != folio) {
> -               xas_unlock_irq(&xas);
> -               return -EAGAIN;
> -       }
> -
> -       if (!folio_ref_freeze(folio, expected_count)) {
> +       if (xas_load(&xas) != folio ||
> +           !folio_ref_freeze(folio, expected_count)) {
>                  xas_unlock_irq(&xas);
>                  return -EAGAIN;
>          }

I think this is reasonable, like what we've done in __remove_mapping().

> And since we've got the lock on the page, how can somebody else be
> removing it from the page cache?  I think that xas_load() can be
> removed too.  So even more simply,

Good point, this is more simply.

> 
> +++ b/mm/migrate.c
> @@ -403,12 +403,6 @@ int folio_migrate_mapping(struct address_space *mapping,
>          newzone = folio_zone(newfolio);
> 
>          xas_lock_irq(&xas);
> -       if (folio_ref_count(folio) != expected_count ||
> -           xas_load(&xas) != folio) {
> -               xas_unlock_irq(&xas);
> -               return -EAGAIN;
> -       }
> -
>          if (!folio_ref_freeze(folio, expected_count)) {
>                  xas_unlock_irq(&xas);
>                  return -EAGAIN;
> 
> but I'm not really set up to test page migration.  Does your test suite
> test migrating file-backed pages?

Yes, I've tested above changes, and the mapped file pages migration 
works well. So can I resend this patch set with your Suggested-by tag? 
Thanks.