Re: [PATCH v6 4/5] mm/migrate: skip migrating folios under writeback with AS_WRITEBACK_INDETERMINATE mappings

[David Hildenbrand <david@redhat.com>]

On 19.12.24 17:22, Shakeel Butt wrote:
> On Thu, Dec 19, 2024 at 10:55:10AM -0500, Zi Yan wrote:
>> On 19 Dec 2024, at 10:53, Shakeel Butt wrote:
>>
>>> On Thu, Dec 19, 2024 at 04:47:18PM +0100, David Hildenbrand wrote:
>>>> On 19.12.24 16:43, Shakeel Butt wrote:
>>>>> On Thu, Dec 19, 2024 at 02:05:04PM +0100, David Hildenbrand wrote:
>>>>>> On 23.11.24 00:23, Joanne Koong wrote:
>>>>>>> For migrations called in MIGRATE_SYNC mode, skip migrating the folio if
>>>>>>> it is under writeback and has the AS_WRITEBACK_INDETERMINATE flag set on its
>>>>>>> mapping. If the AS_WRITEBACK_INDETERMINATE flag is set on the mapping, the
>>>>>>> writeback may take an indeterminate amount of time to complete, and
>>>>>>> waits may get stuck.
>>>>>>>
>>>>>>> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
>>>>>>> Reviewed-by: Shakeel Butt <shakeel.butt@linux.dev>
>>>>>>> ---
>>>>>>>     mm/migrate.c | 5 ++++-
>>>>>>>     1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>>>>> index df91248755e4..fe73284e5246 100644
>>>>>>> --- a/mm/migrate.c
>>>>>>> +++ b/mm/migrate.c
>>>>>>> @@ -1260,7 +1260,10 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>>>>>>>     		 */
>>>>>>>     		switch (mode) {
>>>>>>>     		case MIGRATE_SYNC:
>>>>>>> -			break;
>>>>>>> +			if (!src->mapping ||
>>>>>>> +			    !mapping_writeback_indeterminate(src->mapping))
>>>>>>> +				break;
>>>>>>> +			fallthrough;
>>>>>>>     		default:
>>>>>>>     			rc = -EBUSY;
>>>>>>>     			goto out;
>>>>>>
>>>>>> Ehm, doesn't this mean that any fuse user can essentially completely block
>>>>>> CMA allocations, memory compaction, memory hotunplug, memory poisoning... ?!
>>>>>>
>>>>>> That sounds very bad.
>>>>>
>>>>> The page under writeback are already unmovable while they are under
>>>>> writeback. This patch is only making potentially unrelated tasks to
>>>>> synchronously wait on writeback completion for such pages which in worst
>>>>> case can be indefinite. This actually is solving an isolation issue on a
>>>>> multi-tenant machine.
>>>>>
>>>> Are you sure, because I read in the cover letter:
>>>>
>>>> "In the current FUSE writeback design (see commit 3be5a52b30aa ("fuse:
>>>> support writable mmap"))), a temp page is allocated for every dirty
>>>> page to be written back, the contents of the dirty page are copied over to
>>>> the temp page, and the temp page gets handed to the server to write back.
>>>> This is done so that writeback may be immediately cleared on the dirty
>>>> page,"
>>>>
>>>> Which to me means that they are immediately movable again?
>>>
>>> Oh sorry, my mistake, yes this will become an isolation issue with the
>>> removal of the temp page in-between which this series is doing. I think
>>> the tradeoff is between extra memory plus slow write performance versus
>>> temporary unmovable memory.
>>
>> No, the tradeoff is slow FUSE performance vs whole system slowdown due to
>> memory fragmentation. AS_WRITEBACK_INDETERMINATE indicates it is not
>> temporary.
> 
> If you check the code just above this patch, this
> mapping_writeback_indeterminate() check only happen for pages under
> writeback which is a temp state. Anyways, fuse folios should not be
> unmovable for their lifetime but only while under writeback which is
> same for all fs.

But there, writeback is expected to be a temporary thing, not possibly: 
"AS_WRITEBACK_INDETERMINATE", that is a BIG difference.

I'll have to NACK anything that violates ZONE_MOVABLE / ALLOC_CMA 
guarantees, and unfortunately, it sounds like this is the case here, 
unless I am missing something important.

-- 
Cheers,

David / dhildenb