Re: [Devel] [PATCH hci-8.1 1/2] mm/memcontrol: allocate numa_migrate targets of the source folio order

[Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>]

> memcg_numa_migrate_new_folio() treats every large folio as a PMD-sized
> THP and allocates an HPAGE_PMD_ORDER destination for it. The branch is
> the 4.18 pattern of new_page() in mm/mempolicy.c: a compound page on
> the LRU could only be a THP then, so PageTransHuge() implied
> HPAGE_PMD_ORDER.
> 
> Today the page cache holds large folios of any order (NFS sizes a folio
> to the write, XFS and shmem have them too) and anonymous memory has
> mTHP, while folio_test_large() is still just folio_test_head() and is
> true for any order from 1 up. migrate_pages() never checks that the
> destination order matches the source.
> 
> For an order-1 or order-4 file folio the destination is an order-9
> folio that inherits the source index and page cache reference count,
> so every later page cache operation that sizes the folio by its order
> goes wrong: nrpages underflows, neighbouring xarray slots get wiped,
> the folio can never be freed.
> 
> On a two-node host a VM migration to another NUMA node along with a
> backup to NFS ends up with a negative nrpages, and closing the backup
> file dies in nfs4_evict_inode():
> 
>   kernel BUG at fs/inode.c:737!
>   RIP: 0010:clear_inode+0x75/0x80
>   Call Trace:
>    nfs4_evict_inode+0x21/0x80 [nfsv4]
>    evict+0x115/0x2a0
>    nfs4_delegreturn_release+0x55/0xa0 [nfsv4]
>    rpc_free_task+0x39/0x60 [sunrpc]
>    __rpc_execute+0x3d1/0x3e0 [sunrpc]
> 
> A debug build dies earlier, on the first migration of a large folio of
> any file system, in mem_cgroup_migrate():
> 
>   VM_BUG_ON_FOLIO(folio_nr_pages(old) != folio_nr_pages(new))
> 
> Make memcg_numa_migrate_new_folio() handle large folios properly.
> 
> Fixes: 9f8a754ad6c2 ("mm: memcontrol: add memory.numa_migrate file")
> https://virtuozzo.atlassian.net/browse/VSTOR-151607
> 
> Feature: mm: interface to migrate memory between NUMA nodes upon userspace request
> Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 0734c0fa21d0..d8a87913ace3 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3480,6 +3480,7 @@ static struct folio *memcg_numa_migrate_new_folio(struct folio *old_folio,
>  {
>  	struct memcg_numa_migrate_struct *ms = (void *)private;
>  	gfp_t gfp_mask = GFP_HIGHUSER_MOVABLE | __GFP_NORETRY | __GFP_NOWARN;
> +	unsigned int order = 0;
>  
>  	ms->current_node = next_node(ms->current_node, *ms->target_nodes);
>  	if (ms->current_node >= MAX_NUMNODES) {
> @@ -3487,13 +3488,13 @@ static struct folio *memcg_numa_migrate_new_folio(struct folio *old_folio,
>  		VM_BUG_ON(ms->current_node >= MAX_NUMNODES);
>  	}
>  
> -	if (thp_migration_supported() && folio_test_large(old_folio)) {
> -		return __folio_alloc(GFP_TRANSHUGE_LIGHT | __GFP_THISNODE,
> -				HPAGE_PMD_ORDER, ms->current_node,
> -				ms->target_nodes);
> +	if (folio_test_large(old_folio)) {
> +		gfp_mask = GFP_TRANSHUGE_LIGHT | __GFP_THISNODE;
> +		order = folio_order(old_folio);
>  	}
>  
> -	return __folio_alloc(gfp_mask, 0, ms->current_node, ms->target_nodes);
> +	return __folio_alloc(gfp_mask, order, ms->current_node,
> +			     ms->target_nodes);

The patch looks good to me. There is another issue that is about parsing and
has nothing to do with this patch but I though of bringing it up. If we
write an empty nodelist to memory.numa_migrate it will get past
nodes_subset. memcg_numa_migrate_new_folio will pass MAX_NUMNODES to
__folio_alloc and will cause OOB. We need another patch that adds
nodes_empty next to nodes_subset.

-- 
Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel