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

Shiyang Ruan <ruansy.fnst@xxxxxxxxxxx> · Thu, 18 Aug 2022 19:19:28 +0800

在 2022/8/3 12:33, 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. :)

Hi, Darrick

How is your patch going on?  Forgive me for being so annoying.  I'm 
afraid of missing your patch, so I'm asking for confirmation.


--
Thanks,
Ruan.


--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.






