Re: [RFC PATCH v6] mm, pmem, xfs: Introduce MF_MEM_REMOVE for unbind

[Darrick J. Wong]

On Wed, Aug 03, 2022 at 02:43:20AM +0000, ruansy.fnst@xxxxxxxxxxx wrote:
> 
> 在 2022/7/19 6:56, Dan Williams 写道:
> > Darrick J. Wong wrote:
> >> On Thu, Jul 14, 2022 at 11:21:44AM -0700, Dan Williams wrote:
> >>> ruansy.fnst@xxxxxxxxxxx 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.
> >>>>
> >>>> ==
> >>>> Changes since v5:
> >>>>    1. Renamed MF_MEM_REMOVE to MF_MEM_PRE_REMOVE
> >>>>    2. hold s_umount before sync_filesystem()
> >>>>    3. move sync_filesystem() after SB_BORN check
> >>>>    4. Rebased on next-20220714
> >>>>
> >>>> Changes since v4:
> >>>>    1. sync_filesystem() at the beginning when MF_MEM_REMOVE
> >>>>    2. Rebased on next-20220706
> >>>>
> >>>> [1]: https://lore.kernel.org/linux-mm/161604050314.1463742.14151665140035795571.stgit@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> >>>>
> >>>> Signed-off-by: Shiyang Ruan <ruansy.fnst@xxxxxxxxxxx>
> >>>> ---
> >>>>   drivers/dax/super.c         |  3 ++-
> >>>>   fs/xfs/xfs_notify_failure.c | 15 +++++++++++++++
> >>>>   include/linux/mm.h          |  1 +
> >>>>   3 files changed, 18 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> >>>> index 9b5e2a5eb0ae..cf9a64563fbe 100644
> >>>> --- a/drivers/dax/super.c
> >>>> +++ b/drivers/dax/super.c
> >>>> @@ -323,7 +323,8 @@ void kill_dax(struct dax_device *dax_dev)
> >>>>   		return;
> >>>>   
> >>>>   	if (dax_dev->holder_data != NULL)
> >>>> -		dax_holder_notify_failure(dax_dev, 0, U64_MAX, 0);
> >>>> +		dax_holder_notify_failure(dax_dev, 0, U64_MAX,
> >>>> +				MF_MEM_PRE_REMOVE);
> >>>>   
> >>>>   	clear_bit(DAXDEV_ALIVE, &dax_dev->flags);
> >>>>   	synchronize_srcu(&dax_srcu);
> >>>> diff --git a/fs/xfs/xfs_notify_failure.c b/fs/xfs/xfs_notify_failure.c
> >>>> index 69d9c83ea4b2..6da6747435eb 100644
> >>>> --- a/fs/xfs/xfs_notify_failure.c
> >>>> +++ b/fs/xfs/xfs_notify_failure.c
> >>>> @@ -76,6 +76,9 @@ xfs_dax_failure_fn(
> >>>>   
> >>>>   	if (XFS_RMAP_NON_INODE_OWNER(rec->rm_owner) ||
> >>>>   	    (rec->rm_flags & (XFS_RMAP_ATTR_FORK | XFS_RMAP_BMBT_BLOCK))) {
> >>>> +		/* Do not shutdown so early when device is to be removed */
> >>>> +		if (notify->mf_flags & MF_MEM_PRE_REMOVE)
> >>>> +			return 0;
> >>>>   		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_ONDISK);
> >>>>   		return -EFSCORRUPTED;
> >>>>   	}
> >>>> @@ -174,12 +177,22 @@ xfs_dax_notify_failure(
> >>>>   	struct xfs_mount	*mp = dax_holder(dax_dev);
> >>>>   	u64			ddev_start;
> >>>>   	u64			ddev_end;
> >>>> +	int			error;
> >>>>   
> >>>>   	if (!(mp->m_sb.sb_flags & SB_BORN)) {
> >>>>   		xfs_warn(mp, "filesystem is not ready for notify_failure()!");
> >>>>   		return -EIO;
> >>>>   	}
> >>>>   
> >>>> +	if (mf_flags & MF_MEM_PRE_REMOVE) {
> >>>> +		xfs_info(mp, "device is about to be removed!");
> >>>> +		down_write(&mp->m_super->s_umount);
> >>>> +		error = sync_filesystem(mp->m_super);
> >>>> +		up_write(&mp->m_super->s_umount);
> >>>
> >>> Are all mappings invalidated after this point?
> >>
> >> No; all this step does is pushes dirty filesystem [meta]data to pmem
> >> before we lose DAXDEV_ALIVE...
> >>
> >>> The goal of the removal notification is to invalidate all DAX mappings
> >>> that are no pointing to pfns that do not exist anymore, so just syncing
> >>> does not seem like enough, and the shutdown is skipped above. What am I
> >>> missing?
> >>
> >> ...however, the shutdown above only applies to filesystem metadata.  In
> >> effect, we avoid the fs shutdown in MF_MEM_PRE_REMOVE mode, which
> >> enables the mf_dax_kill_procs calls to proceed against mapped file data.
> >> I have a nagging suspicion that in non-PREREMOVE mode, we can end up
> >> shutting down the filesytem on an xattr block and the 'return
> >> -EFSCORRUPTED' actually prevents us from reaching all the remaining file
> >> data mappings.
> >>
> >> IOWs, I think that clause above really ought to have returned zero so
> >> that we keep the filesystem up while we're tearing down mappings, and
> >> only call xfs_force_shutdown() after we've had a chance to let
> >> xfs_dax_notify_ddev_failure() tear down all the mappings.
> >>
> >> I missed that subtlety in the initial ~30 rounds of review, but I figure
> >> at this point let's just land it in 5.20 and clean up that quirk for
> >> -rc1.
> > 
> > Sure, this is a good baseline to incrementally improve.
> 
> Hi Dan, Darrick
> 
> Do I need to fix somewhere on this patch?  I'm not sure if it is looked good...

Eh, wait for me to send the xfs pull request and then I'll clean things
up and send you a patch. :)

--D

> 
> --
> Thanks,
> Ruan.
> 
> > 
> >>
> >>> Notice that kill_dev_dax() does unmap_mapping_range() after invalidating
> >>> the dax device and that ensures that all existing mappings are gone and
> >>> cannot be re-established. As far as I can see a process with an existing
> >>> dax mapping will still be able to use it after this runs, no?
> >>
> >> I'm not sure where in akpm's tree I find kill_dev_dax()?  I'm cribbing
> >> off of:
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/tree/fs/xfs/xfs_notify_failure.c?h=mm-stable
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/tree/drivers/dax/bus.c?h=mm-stable#n381
> > 
> > Where the observation is that when device-dax is told that the device is
> > going away it invalidates all the active mappings to that single
> > character-device-inode. The hope being that in the fsdax case all the
> > dax-mapped filesystem inodes would experience the same irreversible
> > invalidation as the device is exiting.