Re: [PATCH v10 3/3] mm, pmem, xfs: Introduce MF_MEM_REMOVE for unbind

[Shiyang Ruan <ruansy.fnst@fujitsu.com>]

在 2023/2/27 8:07, Dave Chinner 写道:
> On Fri, Feb 17, 2023 at 02:48:32PM +0000, Shiyang Ruan wrote:
>> This patch is inspired by Dan's "mm, dax, pmem: Introduce
>> dev_pagemap_failure()"[1].  With the help of dax_holder and
>> ->notify_failure() mechanism, the pmem driver is able to ask filesystem
>> (or mapped device) on it to unmap all files in use and notify processes
>> who are using those files.
>>
>> Call trace:
>> trigger unbind
>>   -> unbind_store()
>>    -> ... (skip)
>>     -> devres_release_all()   # was pmem driver ->remove() in v1
>>      -> kill_dax()
>>       -> dax_holder_notify_failure(dax_dev, 0, U64_MAX, MF_MEM_PRE_REMOVE)
>>        -> xfs_dax_notify_failure()
>>
>> Introduce MF_MEM_PRE_REMOVE to let filesystem know this is a remove
>> event.  So do not shutdown filesystem directly if something not
>> supported, or if failure range includes metadata area.  Make sure all
>> files and processes are handled correctly.
>>
>> [1]: https://lore.kernel.org/linux-mm/161604050314.1463742.14151665140035795571.stgit@dwillia2-desk3.amr.corp.intel.com/
>>
>> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
> 
> .....
> 
>> ---
>> @@ -225,6 +242,15 @@ xfs_dax_notify_failure(
>>   	if (offset + len - 1 > ddev_end)
>>   		len = ddev_end - offset + 1;
>>   
>> +	if (mf_flags & MF_MEM_PRE_REMOVE) {
>> +		xfs_info(mp, "device is about to be removed!");
>> +		error = freeze_super(mp->m_super);
>> +		if (error)
>> +			return error;
>> +		/* invalidate_inode_pages2() invalidates dax mapping */
>> +		super_drop_pagecache(mp->m_super, invalidate_inode_pages2);
>> +	}
> 
> Why do you still need to drop the pagecache here? My suggestion was
> to replace it with freezing the filesystem at this point is to stop
> it being dirtied further before the device remove actually occurs.
> The userspace processes will be killed, their DAX mappings reclaimed
> and the filesystem shut down before device removal occurs, so
> super_drop_pagecache() is largely superfluous as it doesn't actually
> provide any protection against racing with new mappings or dirtying
> of existing/newly created mappings.
> 
> Freezing doesn't stop the creation of new mappings, either, it just
> cleans all the dirty mappings and halts anything that is trying to

This is the point I wasn't aware of.

> dirty existing clean mappings. It's not until we kill the userspace
> processes that new mappings will be stopped, and it's not until we
> shut the filesystem down that the filesystem itself will stop
> accessing the storage.
> 
> Hence I don't see why you retained super_drop_pagecache() here at
> all. Can you explain why it is still needed?


So I was just afraid that it's not enough for rmap & processes killer to 
invalidate the dax mappings.  If something error happened during the 
rmap walker, the fs will shutdown and there is no chance to invalidate 
the rest mappings whose user didn't be killed yet.

Now that freezing the fs is enough, I will remove the drop cache code.


--
Thanks,
Ruan.

> 
> -Dave.