Received-SPF: pass (google.com: domain of linux-kernel+bounces-162442-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Zi Yan <ziy@nvidia.com>
To: Luis Chamberlain <mcgrof@kernel.org>
Cc: Vlastimil Babka <vbabka@suse.cz>, Sean Christopherson <seanjc@google.com>,
 Matthew Wilcox <willy@infradead.org>,
 "Pankaj Raghav (Samsung)" <kernel@pankajraghav.com>, djwong@kernel.org,
 brauner@kernel.org, david@fromorbit.com, chandan.babu@oracle.com,
 akpm@linux-foundation.org, linux-fsdevel@vger.kernel.org, hare@suse.de,
 linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-xfs@vger.kernel.org,
 gost.dev@samsung.com, p.raghav@samsung.com
Subject: Re: [PATCH v4 05/11] mm: do not split a folio if it has minimum folio
 order requirement
Date: Mon, 29 Apr 2024 10:29:29 -0400
Message-ID: <6799F341-9E37-4F3E-B0D0-B5B2138A5F5F@nvidia.com>
In-Reply-To: <Zi8aYA92pvjDY7d5@bombadil.infradead.org>
References: <20240425113746.335530-1-kernel@pankajraghav.com>
 <20240425113746.335530-6-kernel@pankajraghav.com>
 <Ziq4qAJ_p7P9Smpn@casper.infradead.org>
 <Zir5n6JNiX14VoPm@bombadil.infradead.org>
 <Ziw8w3P9vljrO9JV@bombadil.infradead.org>
 <Zi2e7ecKJK6p6ERu@bombadil.infradead.org>
 <Zi8aYA92pvjDY7d5@bombadil.infradead.org>
Content-Type: multipart/signed;
 boundary="=_MailMate_3CFDE44C-20AD-427E-9C6C-D6F7E093DEE1_=";
 micalg=pgp-sha512; protocol="application/pgp-signature"
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?DbPX2NcneEYnamAnGPrxjVkA+a0NpmScUbSPjzUnxasHdh2LJhDGfiv6QrhZ?=
 =?us-ascii?Q?aWC/FPBnpscQeujXY032m0cw/OYma7qFyiRIEi0giqEmyv0/UZOL5FVRGJN3?=
 =?us-ascii?Q?X/EWL3356j9CLN96MfHrccDaDaoowMcxLdWYV4FOVW5VjoScwh5TFoj12j8D?=
 =?us-ascii?Q?mJmN8B14KLea7xb40aYMMUblitebKLm5pmfUMgYA+5l9jQ20l+3D9s6lxatT?=
 =?us-ascii?Q?Gvuf0ovLIRLOJf3mCR37RBqB9PKKA59UzTSOW0Tku7tS8kBsO2LnMEkcPdzR?=
 =?us-ascii?Q?n2IGo/IZQUOnQCqluN/F+8s9xfcUmwX2VUa3LPOcecWNQj8l22h1uiLJwlET?=
 =?us-ascii?Q?OuA+8z9nEAe0eLLxP4qh2BCBSZIRJ/Oo4ndSgn9ngvG302GfwQPf3IxTFUu+?=
 =?us-ascii?Q?41iyEs5dN2GNxbblYUF16zl6E/0p+k/1N0YWYIjRVEC90W4Jg9xNxEw3kim/?=
 =?us-ascii?Q?nIVfQU7IHCUpdRiO9916nYkqvnTKHuvfI/h2nL+HfRI+Fdr+ydUgdPJeMARK?=
 =?us-ascii?Q?2dVaTDDIvQYVUwwgCl3HcCKDOFeD4PkM8aq1ElazElRqLpWp+MYV24t8oXBr?=
 =?us-ascii?Q?5ld09zM4yTT++1iKFTiGQJsqy+zb4CD8dTgBOl7O8k+dGB5P24weJD+/8e05?=
 =?us-ascii?Q?DHX/hV6+taMkFJAQLxnnPfELBeYJK9BT4WMiOvqiVAIkeuiQqgmTpQpfNFuY?=
 =?us-ascii?Q?xcerUyNTxJOoOUKDs2hDAgR4+bjZOTg52pxi3AmSWKLbJk5bypArfnGLDD1k?=
 =?us-ascii?Q?B+QGRtMHSzVhBvEU0YAOfFrXB51zlqoiICGLh3tMDxazVLoTcvUYsKwpT0rC?=
 =?us-ascii?Q?o7OsDlNZgCPDD8eXYY2BfGPaezOJ6YC/G2Qc8Ntc4X5KsHtwCI3JPIyNrxkP?=
 =?us-ascii?Q?o6BzXrvNK18ZIEPwwcqqCwUTTeoDhmZzrHZfkmWDuYDui4GjIgAPGNsnQfSW?=
 =?us-ascii?Q?nBNc/ztcFLKTBk8VYFvCjCYbDnkZicpMMQ44GEVdCkvTtWPMOBuwXfxliANx?=
 =?us-ascii?Q?aXYzvav+97HEw6XQWHisBOFvSKpFS03gcjkBVcTi1E4fDZmABDuZAXkHzl8T?=
 =?us-ascii?Q?dxh1jFMFburaOzdMEaAhtLq43mmiH7Fjv7jO0Wk54R66wPe72hpVSg9/emb7?=
 =?us-ascii?Q?b7UsW7HGQ4K7Os79RyMrt2kKCOyd1aHY1tnoMMPMmqCIW0jiFr9ZHqlho4tf?=
 =?us-ascii?Q?6gqP8W2zi1dE5BTyYs/Puwd30qveAgadbLD/V6e/LI1Y5U/d2bAyQsW5i1Pe?=
 =?us-ascii?Q?LDqLrkm0U6kIRUXlccqwVh5ziMWDFXDrf/YtfuSDeM5PeSo/CDcb+a8wTnK+?=
 =?us-ascii?Q?wUCwOkCpPhs2h/YWxfJ6HzpXajxGNIpHyZZPHuk7OEhBFftw8w+MErk3pUJJ?=
 =?us-ascii?Q?1Y+Pe26q6NeiJTapfE1exBl/sSiH28wxSMyYuFlbMpWZx+I4KQ13Q7GI54/f?=
 =?us-ascii?Q?cDjbpRcNdryTcOdwY6YxDw+HQNH9W5oh0ZZGXnyx+Fsy3vEvmE8YtEjifkkY?=
 =?us-ascii?Q?kcxu8Wsfbtsa3xv5jFHKqgrwwM4MetBE2olq3kVOTwx80jNhU53FB02F4kwl?=
 =?us-ascii?Q?wRU22ucg/UEgFUhEjnc=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ac40d570-8f53-4546-f2b9-08dc6858ca20
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB5744.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Apr 2024 14:29:33.8419
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /mwVVCfvMCzYUIIjl/cOQekcgPBEhlokb26zfg22Z5Sb0V2XhmTa9mxXdiTFDNu8
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR12MB7801

--=_MailMate_3CFDE44C-20AD-427E-9C6C-D6F7E093DEE1_=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

On 28 Apr 2024, at 23:56, Luis Chamberlain wrote:

> On Sat, Apr 27, 2024 at 05:57:17PM -0700, Luis Chamberlain wrote:
>> On Fri, Apr 26, 2024 at 04:46:11PM -0700, Luis Chamberlain wrote:
>>> On Thu, Apr 25, 2024 at 05:47:28PM -0700, Luis Chamberlain wrote:
>>>> On Thu, Apr 25, 2024 at 09:10:16PM +0100, Matthew Wilcox wrote:
>>>>> On Thu, Apr 25, 2024 at 01:37:40PM +0200, Pankaj Raghav (Samsung) w=
rote:
>>>>>> From: Pankaj Raghav <p.raghav@samsung.com>
>>>>>>
>>>>>> using that API for LBS is resulting in an NULL ptr dereference
>>>>>> error in the writeback path [1].
>>>>>>
>>>>>> [1] https://gist.github.com/mcgrof/d12f586ec6ebe32b2472b5d634c397d=
f
>>>>>
>>>>>  How would I go about reproducing this?
>>
>> Well so the below fixes this but I am not sure if this is correct.
>> folio_mark_dirty() at least says that a folio should not be truncated
>> while its running. I am not sure if we should try to split folios then=

>> even though we check for writeback once. truncate_inode_partial_folio(=
)
>> will folio_wait_writeback() but it will split_folio() before checking
>> for claiming to fail to truncate with folio_test_dirty(). But since th=
e
>> folio is locked its not clear why this should be possible.
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 83955362d41c..90195506211a 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -3058,7 +3058,7 @@ int split_huge_page_to_list_to_order(struct page=
 *page, struct list_head *list,
>>  	if (new_order >=3D folio_order(folio))
>>  		return -EINVAL;
>>
>> -	if (folio_test_writeback(folio))
>> +	if (folio_test_dirty(folio) || folio_test_writeback(folio))
>>  		return -EBUSY;
>>
>>  	if (!folio_test_anon(folio)) {
>
> I wondered what code path is causing this and triggering this null
> pointer, so I just sprinkled a check here:
>
> 	VM_BUG_ON_FOLIO(folio_test_dirty(folio), folio);
>
> The answer was:
>
> kcompactd() --> migrate_pages_batch()
>                   --> try_split_folio --> split_folio_to_list() -->
> 		       split_huge_page_to_list_to_order()
>

There are 3 try_split_folio() in migrate_pages_batch(). First one is to
split anonymous large folios that are on deferred split list, so not rela=
ted;
second one is to split THPs when thp migration is not supported, but
this is compaction, so not related; third one is to split large folios
when there is no same size free page in the system, and this should be
the one.

> Since it took running fstests generic/447 twice to reproduce the crash
> with a minorder and 16k block size, modified generic/447 as followed an=
d
> it helps to speed up the reproducer with just running the test once:
>
> diff --git a/tests/generic/447 b/tests/generic/447
> index 16b814ee7347..43050b58e8ba 100755
> --- a/tests/generic/447
> +++ b/tests/generic/447
> @@ -36,6 +39,15 @@ _scratch_mount >> "$seqres.full" 2>&1
>  testdir=3D"$SCRATCH_MNT/test-$seq"
>  mkdir "$testdir"
>
> +runfile=3D"$tmp.compaction"
> +touch $runfile
> +while [ -e $runfile ]; do
> +	echo 1 > /proc/sys/vm/compact_memory
> +	sleep 10
> +done &
> +compaction_pid=3D$!
> +
> +
>  # Setup for one million blocks, but we'll accept stress testing down t=
o
>  # 2^17 blocks... that should be plenty for anyone.
>  fnr=3D20
> @@ -69,6 +81,8 @@ _scratch_cycle_mount
>  echo "Delete file1"
>  rm -rf $testdir/file1
>
> +rm -f $runfile
> +wait > /dev/null 2>&1
>  # success, all done
>  status=3D0
>  exit
>
> And I verified that moving the check only to the migrate_pages_batch()
> path also fixes the crash:
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 73a052a382f1..83b528eb7100 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1484,7 +1484,12 @@ static inline int try_split_folio(struct folio *=
folio, struct list_head *split_f
>  	int rc;
>
>  	folio_lock(folio);
> +	if (folio_test_dirty(folio)) {
> +		rc =3D -EBUSY;
> +		goto out;
> +	}
>  	rc =3D split_folio_to_list(folio, split_folios);
> +out:
>  	folio_unlock(folio);
>  	if (!rc)
>  		list_move_tail(&folio->lru, split_folios);
>
> However I'd like compaction folks to review this. I see some indication=
s
> in the code that migration can race with truncation but we feel fine by=

> it by taking the folio lock. However here we have a case where we see
> the folio clearly locked and the folio is dirty. Other migraiton code
> seems to write back the code and can wait, here we just move on. Furthe=
r
> reading on commit 0003e2a414687 ("mm: Add AS_UNMOVABLE to mark mapping
> as completely unmovable") seems to hint that migration is safe if the
> mapping either does not exist or the mapping does exist but has
> mapping->a_ops->migrate_folio so I'd like further feedback on this.

During migration, all page table entries pointing to this dirty folio
are invalid, and accesses to this folio will cause page fault and
wait on the migration entry. I am not sure we need to skip dirty folios.

> Another thing which requires review is if we we split a folio but not
> down to order 0 but to the new min order, does the accounting on
> migrate_pages_batch() require changing?  And most puzzling, why do we

What accounting are you referring to? split code should take care of it.

> not see this with regular large folios, but we do see it with minorder =
?

I wonder if the split code handles folio->mapping->i_pages properly.
Does the i_pages store just folio pointers or also need all tail page
pointers? I am no expert in fs, thus need help.


--
Best Regards,
Yan, Zi

--=_MailMate_3CFDE44C-20AD-427E-9C6C-D6F7E093DEE1_=
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename=signature.asc
Content-Type: application/pgp-signature; name=signature.asc

-----BEGIN PGP SIGNATURE-----

iQJDBAEBCgAtFiEE6rR4j8RuQ2XmaZol4n+egRQHKFQFAmYvrskPHHppeUBudmlk
aWEuY29tAAoJEOJ/noEUByhUY/AQAJIqwPsNtL3RZCEp0irMLEgWWP6dCzsybB82
c1lJFSHDx8gWcTJ5mm3ZWywewh+KBwUXS//2524KCWk5VRr2IFl/7eydM91WAcbq
Zo6i17nh84KXEwzHFmYs80t6qYIxJufdo2HM0yk8U5RidHkKbyshHlPu/G3lEqD1
+WInHkspt4XfZK7q6ALoS8aZkpkYE2cH1Zjjx9tCy2kUdgY/Jj8iBvaGQXyqgBxt
0J75x1EVewP1dEKhKgPDJ8bA2eEcvWRm4PfS9E60SKX2F8b7lDHtxBufAzeqcjNh
DKnQ9rNGRfYZvMPlBrVKQmAD1awLW7BUF893XXgzh/qLkkXInBnpINzti8n92LCe
ISDEuiUUgSX2WEqqPdRkr2fUy8DcBnpJv3Eet3kEYhGw/16zSFZTtS5KBfrsaxMe
HClVLDrb0bZoSv63tkIn1eIt/f3XkE98/vEab09XHYX1qQGIihz/4xnZt/ccmy10
yYdicuWpDT7LOyPpll9RHxOz0WpMEAu6Sl0S1zStZtB7C7OjITEqsE40bzFtw1M0
5P8OtQCgSyf9g3z/zFo1A+bnYgVG9QtE1vKHJF8EKycX4WduU8ebVMy7AghTghyW
8rvN6Zc4kvTeaep7eTtxciNDXHOPJR85uF0FTlqZe9Nvsp9sYPT4n32Vf2z5cpwk
KgZh6ZCS
=VRI5
-----END PGP SIGNATURE-----

--=_MailMate_3CFDE44C-20AD-427E-9C6C-D6F7E093DEE1_=--