Re: [PATCH v2] mm: release private data before split THP

[Yang Shi <shy828301@gmail.com>]

On Thu, Aug 4, 2022 at 11:29 PM Yin Fengwei <fengwei.yin@intel.com> wrote:
>
> If there is private data attached to THP, the refcount of
> THP will be increased and block the THP split. Release
> private data attached to THP before split it to increase
> the chance of splitting THP successfully.
>
> There was a memory failure issue hit during HW error
> injection testing with 5.18 kernel + xfs as rootfs. Test
> got killed and system reboot was required to re-run the
> test.
>
> The issue was tracked down to THP split failure caused the
> memory failure not being handled. The page dump showed:
>
> [ 1785.433075] page:0000000025f9530b refcount:18 mapcount:0 mapping:000000008162eea7 index:0xa10 pfn:0x2f0200
> [ 1785.443954] head:0000000025f9530b order:4 compound_mapcount:0 compound_pincount:0
> [ 1785.452408] memcg:ff4247f2d28e9000
> [ 1785.456304] aops:xfs_address_space_operations ino:8555182 dentry name:"baseos-filenames.solvx"
> [ 1785.466612] flags: 0x1000000000012036(referenced|uptodate|lru|active|private|head|node=0|zone=2)
> [ 1785.476514] raw: 1000000000012036 ffb9460f8bc07c08 ffb9460f8bc08408 ff4247f22e6299f8
> [ 1785.485268] raw: 0000000000000a10 ff4247f194ade900 00000012ffffffff ff4247f2d28e9000
>
> It was like the error was injected to a large folio for xfs
> with private data attached.
>
> With private data released before split THP, the test case
> could be run successfully many times without reboot system.
>
> Co-developed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Reviewed-by: Aaron Lu <aaron.lu@intel.com>
> ---
> Changelog from v1:
>  - Move private release to split_huge_page_to_list
>    to cover wider path per Yang's comment
>  - Update to commit message
>
> Changelog from RFC:
>  - Use new folio API per Mathhew Wilcox's suggestion
>  - Add one line comment before re-get folio of page per
>    Miaohe's comment
>  - Remove RFC tag
>  - Add Co-developed-by of Qiuxu who did a lot of debugging
>    work to locate where the real issue is
>
>  mm/huge_memory.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 15965084816d..edcbc6c2bb3f 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2590,6 +2590,12 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>                         goto out;
>                 }
>
> +               if (folio_test_private(folio) &&
> +                               !filemap_release_folio(folio, GFP_KERNEL)) {

The GFP_KERNEL is fine for most THP split callsites except for the
memory reclaim path since it might not allow certain flags to avoid
recursion, for example, nested reclaim, issue I/O, etc. The most
filesystems clear __GFP_FS. However it should not be a real life
problem now since AFAIK just xfs supports large folios for now and xfs
uses iomap release_folio() method which actually ignores gfp flags.

So it sounds safer to follow the gfp convention used by
xas_split_alloc() in the below. The best way is to pass in the gfp
flag from the reclaimer IMO, but it seems overkilling at the moment.

> +                       ret = -EBUSY;
> +                       goto out;
> +               }
> +
>                 xas_split_alloc(&xas, head, compound_order(head),
>                                 mapping_gfp_mask(mapping) & GFP_RECLAIM_MASK);
>                 if (xas_error(&xas)) {
>
> base-commit: 31be1d0fbd950395701d9fd47d8fb1f99c996f61
> --
> 2.25.1
>