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.
