| Message ID | 1662114961-66-4-git-send-email-ruansy.fnst@fujitsu.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] xfs: fix the calculation of length and end | expand |
On Fri, Sep 02, 2022 at 10:36:01AM +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> > --- > drivers/dax/super.c | 3 ++- > fs/xfs/xfs_notify_failure.c | 23 +++++++++++++++++++++++ > include/linux/mm.h | 1 + > 3 files changed, 26 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 3830f908e215..5e04ba7fa403 100644 > --- a/fs/xfs/xfs_notify_failure.c > +++ b/fs/xfs/xfs_notify_failure.c > @@ -22,6 +22,7 @@ > > #include <linux/mm.h> > #include <linux/dax.h> > +#include <linux/fs.h> > > struct xfs_failure_info { > xfs_agblock_t startblock; > @@ -77,6 +78,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))) { > + /* The device is about to be removed. Not a really failure. */ > + if (notify->mf_flags & MF_MEM_PRE_REMOVE) > + return 0; > notify->want_shutdown = true; > return 0; > } > @@ -182,12 +186,23 @@ xfs_dax_notify_failure( > struct xfs_mount *mp = dax_holder(dax_dev); > u64 ddev_start; > u64 ddev_end; > + int error; > > if (!(mp->m_super->s_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); > + drop_pagecache_sb(mp->m_super, NULL); > + 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_debug(mp, > "notify_failure() not supported on realtime device!"); > @@ -196,6 +211,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; > @@ -209,6 +226,12 @@ xfs_dax_notify_failure( > ddev_start = mp->m_ddev_targp->bt_dax_part_off; > ddev_end = ddev_start + bdev_nr_bytes(mp->m_ddev_targp->bt_bdev) - 1; > > + /* Notify failure on the whole device */ > + if (offset == 0 && len == U64_MAX) { > + offset = ddev_start; > + len = bdev_nr_bytes(mp->m_ddev_targp->bt_bdev); > + } I wonder, won't the trimming code below take care of this? The rest of the patch looks ok to me. --D > + > /* Ignore the range out of filesystem area */ > if (offset + len - 1 < ddev_start) > return -ENXIO; > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 21f8b27bd9fd..9122a1c57dd2 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -3183,6 +3183,7 @@ enum mf_flags { > MF_UNPOISON = 1 << 4, > MF_SW_SIMULATED = 1 << 5, > MF_NO_RETRY = 1 << 6, > + MF_MEM_PRE_REMOVE = 1 << 7, > }; > int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index, > unsigned long count, int mf_flags); > -- > 2.37.2 >
在 2022/9/15 2:15, Darrick J. Wong 写道: > On Fri, Sep 02, 2022 at 10:36:01AM +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> >> --- >> drivers/dax/super.c | 3 ++- >> fs/xfs/xfs_notify_failure.c | 23 +++++++++++++++++++++++ >> include/linux/mm.h | 1 + >> 3 files changed, 26 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 3830f908e215..5e04ba7fa403 100644 >> --- a/fs/xfs/xfs_notify_failure.c >> +++ b/fs/xfs/xfs_notify_failure.c >> @@ -22,6 +22,7 @@ >> >> #include <linux/mm.h> >> #include <linux/dax.h> >> +#include <linux/fs.h> >> >> struct xfs_failure_info { >> xfs_agblock_t startblock; >> @@ -77,6 +78,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))) { >> + /* The device is about to be removed. Not a really failure. */ >> + if (notify->mf_flags & MF_MEM_PRE_REMOVE) >> + return 0; >> notify->want_shutdown = true; >> return 0; >> } >> @@ -182,12 +186,23 @@ xfs_dax_notify_failure( >> struct xfs_mount *mp = dax_holder(dax_dev); >> u64 ddev_start; >> u64 ddev_end; >> + int error; >> >> if (!(mp->m_super->s_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); >> + drop_pagecache_sb(mp->m_super, NULL); >> + 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_debug(mp, >> "notify_failure() not supported on realtime device!"); >> @@ -196,6 +211,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; >> @@ -209,6 +226,12 @@ xfs_dax_notify_failure( >> ddev_start = mp->m_ddev_targp->bt_dax_part_off; >> ddev_end = ddev_start + bdev_nr_bytes(mp->m_ddev_targp->bt_bdev) - 1; >> >> + /* Notify failure on the whole device */ >> + if (offset == 0 && len == U64_MAX) { >> + offset = ddev_start; >> + len = bdev_nr_bytes(mp->m_ddev_targp->bt_bdev); >> + } > > I wonder, won't the trimming code below take care of this? The len is U64_MAX, so 'offset + len - 1' will overflow. That can't be handled correctly by the trimming code below. -- Thanks, Ruan. > > The rest of the patch looks ok to me. > > --D > >> + >> /* Ignore the range out of filesystem area */ >> if (offset + len - 1 < ddev_start) >> return -ENXIO; >> diff --git a/include/linux/mm.h b/include/linux/mm.h >> index 21f8b27bd9fd..9122a1c57dd2 100644 >> --- a/include/linux/mm.h >> +++ b/include/linux/mm.h >> @@ -3183,6 +3183,7 @@ enum mf_flags { >> MF_UNPOISON = 1 << 4, >> MF_SW_SIMULATED = 1 << 5, >> MF_NO_RETRY = 1 << 6, >> + MF_MEM_PRE_REMOVE = 1 << 7, >> }; >> int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index, >> unsigned long count, int mf_flags); >> -- >> 2.37.2 >>
On Tue, Sep 20, 2022 at 12:45:19PM +1000, Dave Chinner wrote: > On Fri, Sep 02, 2022 at 10:36:01AM +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> > > --- > > drivers/dax/super.c | 3 ++- > > fs/xfs/xfs_notify_failure.c | 23 +++++++++++++++++++++++ > > include/linux/mm.h | 1 + > > 3 files changed, 26 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 3830f908e215..5e04ba7fa403 100644 > > --- a/fs/xfs/xfs_notify_failure.c > > +++ b/fs/xfs/xfs_notify_failure.c > > @@ -22,6 +22,7 @@ > > > > #include <linux/mm.h> > > #include <linux/dax.h> > > +#include <linux/fs.h> > > > > struct xfs_failure_info { > > xfs_agblock_t startblock; > > @@ -77,6 +78,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))) { > > + /* The device is about to be removed. Not a really failure. */ > > + if (notify->mf_flags & MF_MEM_PRE_REMOVE) > > + return 0; > > notify->want_shutdown = true; > > return 0; > > } > > @@ -182,12 +186,23 @@ xfs_dax_notify_failure( > > struct xfs_mount *mp = dax_holder(dax_dev); > > u64 ddev_start; > > u64 ddev_end; > > + int error; > > > > if (!(mp->m_super->s_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); > > + drop_pagecache_sb(mp->m_super, NULL); > > + up_write(&mp->m_super->s_umount); > > + if (error) > > + return error; > > If the device is about to go away unexpectedly, shouldn't this shut > down the filesystem after syncing it here? If the filesystem has > been shut down, then everything will fail before removal finally > triggers, and the act of unmounting the filesystem post device > removal will clean up the page cache and all the other caches. IIRC they want to kill all the processes with MAP_SYNC mappings sooner than whenever the admin gets around to unmounting the filesystem, which is why PRE_REMOVE will then go walk the rmapbt to find processes to shoot down. I'm not sure, though, if drop_pagecache_sb only touches DRAM page cache or if it'll shoot down fsdax mappings too? > IOWs, I don't understand why the page cache is considered special > here (as opposed to, say, the inode or dentry caches), nor why we > aren't shutting down the filesystem directly after syncing it to > disk to ensure that we don't end up with applications losing data as > a result of racing with the removal.... But yeah, we might as well shut down the fs at the end of PRE_REMOVE handling, if the rmap walk hasn't already done that. --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
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 3830f908e215..5e04ba7fa403 100644 --- a/fs/xfs/xfs_notify_failure.c +++ b/fs/xfs/xfs_notify_failure.c @@ -22,6 +22,7 @@ #include <linux/mm.h> #include <linux/dax.h> +#include <linux/fs.h> struct xfs_failure_info { xfs_agblock_t startblock; @@ -77,6 +78,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))) { + /* The device is about to be removed. Not a really failure. */ + if (notify->mf_flags & MF_MEM_PRE_REMOVE) + return 0; notify->want_shutdown = true; return 0; } @@ -182,12 +186,23 @@ xfs_dax_notify_failure( struct xfs_mount *mp = dax_holder(dax_dev); u64 ddev_start; u64 ddev_end; + int error; if (!(mp->m_super->s_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); + drop_pagecache_sb(mp->m_super, NULL); + 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_debug(mp, "notify_failure() not supported on realtime device!"); @@ -196,6 +211,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; @@ -209,6 +226,12 @@ xfs_dax_notify_failure( ddev_start = mp->m_ddev_targp->bt_dax_part_off; ddev_end = ddev_start + bdev_nr_bytes(mp->m_ddev_targp->bt_bdev) - 1; + /* Notify failure on the whole device */ + if (offset == 0 && len == U64_MAX) { + offset = ddev_start; + len = bdev_nr_bytes(mp->m_ddev_targp->bt_bdev); + } + /* Ignore the range out of filesystem area */ if (offset + len - 1 < ddev_start) return -ENXIO; diff --git a/include/linux/mm.h b/include/linux/mm.h index 21f8b27bd9fd..9122a1c57dd2 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -3183,6 +3183,7 @@ enum mf_flags { MF_UNPOISON = 1 << 4, MF_SW_SIMULATED = 1 << 5, MF_NO_RETRY = 1 << 6, + MF_MEM_PRE_REMOVE = 1 << 7, }; 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. [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 | 23 +++++++++++++++++++++++ include/linux/mm.h | 1 + 3 files changed, 26 insertions(+), 1 deletion(-)