| Message ID | 20220714103421.1988696-1-ruansy.fnst@fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,v6] mm, pmem, xfs: Introduce MF_MEM_REMOVE for unbind | expand |
On Thu, Jul 14, 2022 at 10:34:29AM +0000, ruansy.fnst@fujitsu.com 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@dwillia2-desk3.amr.corp.intel.com/ > > Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> Looks reasonable to me now, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > 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); > + if (error) > + return error; > + } > + > if (mp->m_rtdev_targp && mp->m_rtdev_targp->bt_daxdev == dax_dev) { > xfs_warn(mp, > "notify_failure() not supported on realtime device!"); > @@ -188,6 +201,8 @@ xfs_dax_notify_failure( > > if (mp->m_logdev_targp && mp->m_logdev_targp->bt_daxdev == dax_dev && > mp->m_logdev_targp != mp->m_ddev_targp) { > + if (mf_flags & MF_MEM_PRE_REMOVE) > + return 0; > xfs_err(mp, "ondisk log corrupt, shutting down fs!"); > xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_ONDISK); > return -EFSCORRUPTED; > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 4287bec50c28..2ddfb76c8a83 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -3188,6 +3188,7 @@ enum mf_flags { > MF_SOFT_OFFLINE = 1 << 3, > MF_UNPOISON = 1 << 4, > MF_SW_SIMULATED = 1 << 5, > + MF_MEM_PRE_REMOVE = 1 << 6, > }; > int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index, > unsigned long count, int mf_flags); > -- > 2.37.0
ruansy.fnst@fujitsu.com 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@dwillia2-desk3.amr.corp.intel.com/ > > Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> > --- > 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? 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? 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?
On Thu, Jul 14, 2022 at 11:21:44AM -0700, Dan Williams wrote: > ruansy.fnst@fujitsu.com 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@dwillia2-desk3.amr.corp.intel.com/ > > > > Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> > > --- > > 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. > 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 --D
Darrick J. Wong wrote: > On Thu, Jul 14, 2022 at 11:21:44AM -0700, Dan Williams wrote: > > ruansy.fnst@fujitsu.com 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@dwillia2-desk3.amr.corp.intel.com/ > > > > > > Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> > > > --- > > > 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. > > > 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.
在 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@fujitsu.com 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@dwillia2-desk3.amr.corp.intel.com/ >>>> >>>> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> >>>> --- >>>> 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... -- 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.
On Wed, Aug 03, 2022 at 02:43:20AM +0000, ruansy.fnst@fujitsu.com 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@fujitsu.com 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@dwillia2-desk3.amr.corp.intel.com/ > >>>> > >>>> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> > >>>> --- > >>>> 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.
在 2022/8/3 12:33, Darrick J. Wong 写道: > On Wed, Aug 03, 2022 at 02:43:20AM +0000, ruansy.fnst@fujitsu.com 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@fujitsu.com 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@dwillia2-desk3.amr.corp.intel.com/ >>>>>> >>>>>> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> >>>>>> --- >>>>>> 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.
On Thu, Aug 18, 2022 at 07:19:28PM +0800, Shiyang Ruan wrote: > > > 在 2022/8/3 12:33, Darrick J. Wong 写道: > > On Wed, Aug 03, 2022 at 02:43:20AM +0000, ruansy.fnst@fujitsu.com 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@fujitsu.com 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@dwillia2-desk3.amr.corp.intel.com/ > > > > > > > > > > > > > > Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> > > > > > > > --- > > > > > > > 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. <nod> I just sent you a patch. Sorry, it took me a few days to unbusy enough to start testing 6.0-rc1. You're not annoying at all. :) --D > > -- > 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.
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); + if (error) + return error; + } + if (mp->m_rtdev_targp && mp->m_rtdev_targp->bt_daxdev == dax_dev) { xfs_warn(mp, "notify_failure() not supported on realtime device!"); @@ -188,6 +201,8 @@ xfs_dax_notify_failure( if (mp->m_logdev_targp && mp->m_logdev_targp->bt_daxdev == dax_dev && mp->m_logdev_targp != mp->m_ddev_targp) { + if (mf_flags & MF_MEM_PRE_REMOVE) + return 0; xfs_err(mp, "ondisk log corrupt, shutting down fs!"); xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_ONDISK); return -EFSCORRUPTED; diff --git a/include/linux/mm.h b/include/linux/mm.h index 4287bec50c28..2ddfb76c8a83 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -3188,6 +3188,7 @@ enum mf_flags { MF_SOFT_OFFLINE = 1 << 3, MF_UNPOISON = 1 << 4, MF_SW_SIMULATED = 1 << 5, + MF_MEM_PRE_REMOVE = 1 << 6, }; int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index, unsigned long count, int mf_flags);
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@dwillia2-desk3.amr.corp.intel.com/ Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> --- drivers/dax/super.c | 3 ++- fs/xfs/xfs_notify_failure.c | 15 +++++++++++++++ include/linux/mm.h | 1 + 3 files changed, 18 insertions(+), 1 deletion(-)