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

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

[Shiyang Ruan]

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)
   -> pmem driver ->remove()
    -> kill_dax()
     -> dax_holder_notify_failure(dax_dev, 0, U64_MAX, MF_MEM_REMOVE)
      -> xfs_dax_notify_failure()

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

Based on "[PATCH v12] fsdax: introduce fs query to support reflink"[2]

[1]: https://lore.kernel.org/linux-mm/161604050314.1463742.14151665140035795571.stgit@dwillia2-desk3.amr.corp.intel.com/
[2]: https://lore.kernel.org/linux-xfs/20220410160904.3758789-1-ruansy.fnst@fujitsu.com/T/#t

Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
---
 drivers/dax/super.c         | 2 +-
 drivers/nvdimm/pmem.c       | 3 ++-
 fs/xfs/xfs_notify_failure.c | 6 +++++-
 include/linux/mm.h          | 1 +
 4 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index 619a05615497..52f820fdd8ff 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -312,7 +312,7 @@ 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_REMOVE);
 
 	clear_bit(DAXDEV_ALIVE, &dax_dev->flags);
 	synchronize_srcu(&dax_srcu);
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index bd502957cfdf..72d9e69aea98 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -359,7 +359,6 @@ static void pmem_release_disk(void *__pmem)
 	struct pmem_device *pmem = __pmem;
 
 	dax_remove_host(pmem->disk);
-	kill_dax(pmem->dax_dev);
 	put_dax(pmem->dax_dev);
 	del_gendisk(pmem->disk);
 
@@ -597,6 +596,8 @@ static void nd_pmem_remove(struct device *dev)
 		pmem->bb_state = NULL;
 	}
 	nvdimm_flush(to_nd_region(dev->parent), NULL);
+
+	kill_dax(pmem->dax_dev);
 }
 
 static void nd_pmem_shutdown(struct device *dev)
diff --git a/fs/xfs/xfs_notify_failure.c b/fs/xfs/xfs_notify_failure.c
index aac44f54feb4..f8b41085218b 100644
--- a/fs/xfs/xfs_notify_failure.c
+++ b/fs/xfs/xfs_notify_failure.c
@@ -73,7 +73,9 @@ xfs_dax_failure_fn(
 	struct failure_info		*notify = data;
 	int				error = 0;
 
-	if (XFS_RMAP_NON_INODE_OWNER(rec->rm_owner) ||
+	/* Do not shutdown so early when device is to be removed */
+	if (!(notify->mf_flags & MF_MEM_REMOVE) ||
+	    XFS_RMAP_NON_INODE_OWNER(rec->rm_owner) ||
 	    (rec->rm_flags & (XFS_RMAP_ATTR_FORK | XFS_RMAP_BMBT_BLOCK))) {
 		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_ONDISK);
 		return -EFSCORRUPTED;
@@ -181,6 +183,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_REMOVE)
+			return -EOPNOTSUPP;
 		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 742604feef28..b47c3745782d 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3225,6 +3225,7 @@ enum mf_flags {
 	MF_MUST_KILL = 1 << 2,
 	MF_SOFT_OFFLINE = 1 << 3,
 	MF_UNPOISON = 1 << 4,
+	MF_MEM_REMOVE = 1 << 5,
 };
 int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
 		      unsigned long count, int mf_flags);
-- 
2.35.1

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

[Christoph Hellwig]

On Mon, Apr 11, 2022 at 01:16:23AM +0800, Shiyang Ruan wrote:
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index bd502957cfdf..72d9e69aea98 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -359,7 +359,6 @@ static void pmem_release_disk(void *__pmem)
>  	struct pmem_device *pmem = __pmem;
>  
>  	dax_remove_host(pmem->disk);
> -	kill_dax(pmem->dax_dev);
>  	put_dax(pmem->dax_dev);
>  	del_gendisk(pmem->disk);
>  
> @@ -597,6 +596,8 @@ static void nd_pmem_remove(struct device *dev)
>  		pmem->bb_state = NULL;
>  	}
>  	nvdimm_flush(to_nd_region(dev->parent), NULL);
> +
> +	kill_dax(pmem->dax_dev);

I think the put_dax will have to move as well.

This part should probably also be a separate, well-documented
cleanup patch.

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

[Shiyang Ruan]

在 2022/4/11 15:06, Christoph Hellwig 写道:
> On Mon, Apr 11, 2022 at 01:16:23AM +0800, Shiyang Ruan wrote:
>> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
>> index bd502957cfdf..72d9e69aea98 100644
>> --- a/drivers/nvdimm/pmem.c
>> +++ b/drivers/nvdimm/pmem.c
>> @@ -359,7 +359,6 @@ static void pmem_release_disk(void *__pmem)
>>   	struct pmem_device *pmem = __pmem;
>>   
>>   	dax_remove_host(pmem->disk);
>> -	kill_dax(pmem->dax_dev);
>>   	put_dax(pmem->dax_dev);
>>   	del_gendisk(pmem->disk);
>>   
>> @@ -597,6 +596,8 @@ static void nd_pmem_remove(struct device *dev)
>>   		pmem->bb_state = NULL;
>>   	}
>>   	nvdimm_flush(to_nd_region(dev->parent), NULL);
>> +
>> +	kill_dax(pmem->dax_dev);
> 
> I think the put_dax will have to move as well.

After reading the implementation of 'devm_add_action_or_reset()', I 
think there is no need to move kill_dax() and put_dax() into ->remove().

In unbind, it will call both drv->remove() and devres_release_all(). 
The action, pmem_release_disk(), added in devm_add_action_or_reset() 
will be execute in devres_release_all().  So, during the unbind process, 
{kill,put}_dax() will finally be called to notify the REMOVE signal.

In addition, if devm_add_action_or_reset() fails in pmem_attach_disk(), 
pmem_release_disk() will be called to cleanup the pmem->dax_dev.


--
Thanks,
Ruan.

> 
> This part should probably also be a separate, well-documented
> cleanup patch.

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

[Shiyang Ruan]

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_REMOVE)
      -> xfs_dax_notify_failure()

Introduce MF_MEM_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/
[2]: https://lore.kernel.org/linux-xfs/20220508143620.1775214-1-ruansy.fnst@fujitsu.com/

Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>

==
Changes since v1:
  1. Drop the needless change of moving {kill,put}_dax()
  2. Rebased on '[PATCHSETS] v14 fsdax-rmap + v11 fsdax-reflink'[2]

---
 drivers/dax/super.c         | 2 +-
 fs/xfs/xfs_notify_failure.c | 6 +++++-
 include/linux/mm.h          | 1 +
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index 5ddb159c4653..44ca3b488e2a 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -313,7 +313,7 @@ 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_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 aa8dc27c599c..91d3f05d4241 100644
--- a/fs/xfs/xfs_notify_failure.c
+++ b/fs/xfs/xfs_notify_failure.c
@@ -73,7 +73,9 @@ xfs_dax_failure_fn(
 	struct failure_info		*notify = data;
 	int				error = 0;
 
-	if (XFS_RMAP_NON_INODE_OWNER(rec->rm_owner) ||
+	/* Do not shutdown so early when device is to be removed */
+	if (!(notify->mf_flags & MF_MEM_REMOVE) ||
+	    XFS_RMAP_NON_INODE_OWNER(rec->rm_owner) ||
 	    (rec->rm_flags & (XFS_RMAP_ATTR_FORK | XFS_RMAP_BMBT_BLOCK))) {
 		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_ONDISK);
 		return -EFSCORRUPTED;
@@ -182,6 +184,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_REMOVE)
+			return -EOPNOTSUPP;
 		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 e2c0f69f0660..ebcb5a7f3295 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3226,6 +3226,7 @@ enum mf_flags {
 	MF_MUST_KILL = 1 << 2,
 	MF_SOFT_OFFLINE = 1 << 3,
 	MF_UNPOISON = 1 << 4,
+	MF_MEM_REMOVE = 1 << 5,
 };
 int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
 		      unsigned long count, int mf_flags);
-- 
2.35.1

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

[Shiyang Ruan]

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_REMOVE)
      -> xfs_dax_notify_failure()

Introduce MF_MEM_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>

==
Changes since v2:
  1. Rebased on next-20220615

Changes since v1:
  1. Drop the needless change of moving {kill,put}_dax()
  2. Rebased on '[PATCHSETS] v14 fsdax-rmap + v11 fsdax-reflink'[2]

---
 drivers/dax/super.c         | 2 +-
 fs/xfs/xfs_notify_failure.c | 6 +++++-
 include/linux/mm.h          | 1 +
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index 9b5e2a5eb0ae..d4bc83159d46 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -323,7 +323,7 @@ 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_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 aa8dc27c599c..91d3f05d4241 100644
--- a/fs/xfs/xfs_notify_failure.c
+++ b/fs/xfs/xfs_notify_failure.c
@@ -73,7 +73,9 @@ xfs_dax_failure_fn(
 	struct failure_info		*notify = data;
 	int				error = 0;
 
-	if (XFS_RMAP_NON_INODE_OWNER(rec->rm_owner) ||
+	/* Do not shutdown so early when device is to be removed */
+	if (!(notify->mf_flags & MF_MEM_REMOVE) ||
+	    XFS_RMAP_NON_INODE_OWNER(rec->rm_owner) ||
 	    (rec->rm_flags & (XFS_RMAP_ATTR_FORK | XFS_RMAP_BMBT_BLOCK))) {
 		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_ONDISK);
 		return -EFSCORRUPTED;
@@ -182,6 +184,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_REMOVE)
+			return -EOPNOTSUPP;
 		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 623c2ee8330a..bbeb31883362 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3249,6 +3249,7 @@ enum mf_flags {
 	MF_SOFT_OFFLINE = 1 << 3,
 	MF_UNPOISON = 1 << 4,
 	MF_NO_RETRY = 1 << 5,
+	MF_MEM_REMOVE = 1 << 6,
 };
 int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
 		      unsigned long count, int mf_flags);
-- 
2.36.1

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

[Darrick J. Wong]

On Wed, Jun 15, 2022 at 08:54:00PM +0800, 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_REMOVE)
>       -> xfs_dax_notify_failure()
> 
> Introduce MF_MEM_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>
> 
> ==
> Changes since v2:
>   1. Rebased on next-20220615
> 
> Changes since v1:
>   1. Drop the needless change of moving {kill,put}_dax()
>   2. Rebased on '[PATCHSETS] v14 fsdax-rmap + v11 fsdax-reflink'[2]
> 
> ---
>  drivers/dax/super.c         | 2 +-
>  fs/xfs/xfs_notify_failure.c | 6 +++++-
>  include/linux/mm.h          | 1 +
>  3 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index 9b5e2a5eb0ae..d4bc83159d46 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c
> @@ -323,7 +323,7 @@ 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_REMOVE);

At the point we're initiating a MEM_REMOVE call, is the pmem already
gone, or is it about to be gone?

>  
>  	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 aa8dc27c599c..91d3f05d4241 100644
> --- a/fs/xfs/xfs_notify_failure.c
> +++ b/fs/xfs/xfs_notify_failure.c
> @@ -73,7 +73,9 @@ xfs_dax_failure_fn(
>  	struct failure_info		*notify = data;
>  	int				error = 0;
>  
> -	if (XFS_RMAP_NON_INODE_OWNER(rec->rm_owner) ||
> +	/* Do not shutdown so early when device is to be removed */
> +	if (!(notify->mf_flags & MF_MEM_REMOVE) ||
> +	    XFS_RMAP_NON_INODE_OWNER(rec->rm_owner) ||
>  	    (rec->rm_flags & (XFS_RMAP_ATTR_FORK | XFS_RMAP_BMBT_BLOCK))) {
>  		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_ONDISK);
>  		return -EFSCORRUPTED;
> @@ -182,6 +184,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_REMOVE)
> +			return -EOPNOTSUPP;

The reason I ask is that if the pmem is *about to be* but not yet
removed from the system, shouldn't we at least try to flush all dirty
files and the log to reduce data loss and minimize recovery time?

If it's already gone, then you might as well shut down immediately,
unless there's a chance the pmem will come back(?)

--D

>  		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 623c2ee8330a..bbeb31883362 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3249,6 +3249,7 @@ enum mf_flags {
>  	MF_SOFT_OFFLINE = 1 << 3,
>  	MF_UNPOISON = 1 << 4,
>  	MF_NO_RETRY = 1 << 5,
> +	MF_MEM_REMOVE = 1 << 6,
>  };
>  int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
>  		      unsigned long count, int mf_flags);
> -- 
> 2.36.1
> 
> 
> 

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

[Shiyang Ruan]

在 2022/6/23 0:49, Darrick J. Wong 写道:
> On Wed, Jun 15, 2022 at 08:54:00PM +0800, 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_REMOVE)
>>        -> xfs_dax_notify_failure()
>>
>> Introduce MF_MEM_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>
>>
>> ==
>> Changes since v2:
>>    1. Rebased on next-20220615
>>
>> Changes since v1:
>>    1. Drop the needless change of moving {kill,put}_dax()
>>    2. Rebased on '[PATCHSETS] v14 fsdax-rmap + v11 fsdax-reflink'[2]
>>
>> ---
>>   drivers/dax/super.c         | 2 +-
>>   fs/xfs/xfs_notify_failure.c | 6 +++++-
>>   include/linux/mm.h          | 1 +
>>   3 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
>> index 9b5e2a5eb0ae..d4bc83159d46 100644
>> --- a/drivers/dax/super.c
>> +++ b/drivers/dax/super.c
>> @@ -323,7 +323,7 @@ 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_REMOVE);
> 
> At the point we're initiating a MEM_REMOVE call, is the pmem already
> gone, or is it about to be gone?

It's about to be gone.

I found two cases:
   1. exec `unbind` by user, who wants to unplug the pmem
   2. handle failures during initialization

> 
>>   
>>   	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 aa8dc27c599c..91d3f05d4241 100644
>> --- a/fs/xfs/xfs_notify_failure.c
>> +++ b/fs/xfs/xfs_notify_failure.c
>> @@ -73,7 +73,9 @@ xfs_dax_failure_fn(
>>   	struct failure_info		*notify = data;
>>   	int				error = 0;
>>   
>> -	if (XFS_RMAP_NON_INODE_OWNER(rec->rm_owner) ||
>> +	/* Do not shutdown so early when device is to be removed */
>> +	if (!(notify->mf_flags & MF_MEM_REMOVE) ||
>> +	    XFS_RMAP_NON_INODE_OWNER(rec->rm_owner) ||
>>   	    (rec->rm_flags & (XFS_RMAP_ATTR_FORK | XFS_RMAP_BMBT_BLOCK))) {
>>   		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_ONDISK);
>>   		return -EFSCORRUPTED;
>> @@ -182,6 +184,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_REMOVE)
>> +			return -EOPNOTSUPP;
> 
> The reason I ask is that if the pmem is *about to be* but not yet
> removed from the system, shouldn't we at least try to flush all dirty
> files and the log to reduce data loss and minimize recovery time?

Yes, they should be flushed.  Will add it.


--
Thanks,
Ruan.

> 
> If it's already gone, then you might as well shut down immediately,
> unless there's a chance the pmem will come back(?)
> 
> --D
> 
>>   		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 623c2ee8330a..bbeb31883362 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -3249,6 +3249,7 @@ enum mf_flags {
>>   	MF_SOFT_OFFLINE = 1 << 3,
>>   	MF_UNPOISON = 1 << 4,
>>   	MF_NO_RETRY = 1 << 5,
>> +	MF_MEM_REMOVE = 1 << 6,
>>   };
>>   int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
>>   		      unsigned long count, int mf_flags);
>> -- 
>> 2.36.1
>>
>>
>>

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

[Shiyang Ruan]

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_REMOVE)
      -> xfs_dax_notify_failure()

Introduce MF_MEM_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 v3:
  1. Flush dirty files and logs when pmem is about to be removed.
  2. Rebased on next-20220701

Changes since v2:
  1. Rebased on next-20220615

Changes since v1:
  1. Drop the needless change of moving {kill,put}_dax()
  2. Rebased on '[PATCHSETS] v14 fsdax-rmap + v11 fsdax-reflink'[2]

[1]: https://lore.kernel.org/linux-mm/161604050314.1463742.14151665140035795571.stgit@dwillia2-desk3.amr.corp.intel.com/
[2]: https://lore.kernel.org/linux-xfs/20220508143620.1775214-1-ruansy.fnst@fujitsu.com/

Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
---
 drivers/dax/super.c         |  2 +-
 fs/xfs/xfs_notify_failure.c | 23 ++++++++++++++++++++++-
 include/linux/mm.h          |  1 +
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index 9b5e2a5eb0ae..d4bc83159d46 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -323,7 +323,7 @@ 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_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 aa8dc27c599c..269e21b3341c 100644
--- a/fs/xfs/xfs_notify_failure.c
+++ b/fs/xfs/xfs_notify_failure.c
@@ -18,6 +18,7 @@
 #include "xfs_rmap_btree.h"
 #include "xfs_rtalloc.h"
 #include "xfs_trans.h"
+#include "xfs_log.h"

 #include <linux/mm.h>
 #include <linux/dax.h>
@@ -75,6 +76,10 @@ 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_REMOVE) {
+			return 0;
+		}
 		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_ONDISK);
 		return -EFSCORRUPTED;
 	}
@@ -168,6 +173,7 @@ 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()!");
@@ -182,6 +188,13 @@ 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_REMOVE) {
+			/* Flush the log since device is about to be removed. */
+			error = xfs_log_force(mp, XFS_LOG_SYNC);
+			if (error)
+				return error;
+			return -EOPNOTSUPP;
+		}
 		xfs_err(mp, "ondisk log corrupt, shutting down fs!");
 		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_ONDISK);
 		return -EFSCORRUPTED;
@@ -211,8 +224,16 @@ xfs_dax_notify_failure(
 	if (offset + len > ddev_end)
 		len -= ddev_end - offset;

-	return xfs_dax_notify_ddev_failure(mp, BTOBB(offset), BTOBB(len),
+	error = xfs_dax_notify_ddev_failure(mp, BTOBB(offset), BTOBB(len),
 			mf_flags);
+	if (error)
+		return error;
+
+	if (mf_flags & MF_MEM_REMOVE) {
+		xfs_flush_inodes(mp);
+		error = xfs_log_force(mp, XFS_LOG_SYNC);
+	}
+	return error;
 }

 const struct dax_holder_operations xfs_dax_holder_operations = {
diff --git a/include/linux/mm.h b/include/linux/mm.h
index a2270e35a676..e66d23188323 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3236,6 +3236,7 @@ enum mf_flags {
 	MF_SOFT_OFFLINE = 1 << 3,
 	MF_UNPOISON = 1 << 4,
 	MF_SW_SIMULATED = 1 << 5,
+	MF_MEM_REMOVE = 1 << 6,
 };
 int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
 		      unsigned long count, int mf_flags);
--
2.36.1

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

[Darrick J. Wong]

On Sun, Jul 03, 2022 at 09:08:38PM +0800, 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_REMOVE)
>       -> xfs_dax_notify_failure()
> 
> Introduce MF_MEM_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 v3:
>   1. Flush dirty files and logs when pmem is about to be removed.
>   2. Rebased on next-20220701
> 
> Changes since v2:
>   1. Rebased on next-20220615
> 
> Changes since v1:
>   1. Drop the needless change of moving {kill,put}_dax()
>   2. Rebased on '[PATCHSETS] v14 fsdax-rmap + v11 fsdax-reflink'[2]
> 
> [1]: https://lore.kernel.org/linux-mm/161604050314.1463742.14151665140035795571.stgit@dwillia2-desk3.amr.corp.intel.com/
> [2]: https://lore.kernel.org/linux-xfs/20220508143620.1775214-1-ruansy.fnst@fujitsu.com/
> 
> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
> ---
>  drivers/dax/super.c         |  2 +-
>  fs/xfs/xfs_notify_failure.c | 23 ++++++++++++++++++++++-
>  include/linux/mm.h          |  1 +
>  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index 9b5e2a5eb0ae..d4bc83159d46 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c
> @@ -323,7 +323,7 @@ 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_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 aa8dc27c599c..269e21b3341c 100644
> --- a/fs/xfs/xfs_notify_failure.c
> +++ b/fs/xfs/xfs_notify_failure.c
> @@ -18,6 +18,7 @@
>  #include "xfs_rmap_btree.h"
>  #include "xfs_rtalloc.h"
>  #include "xfs_trans.h"
> +#include "xfs_log.h"
> 
>  #include <linux/mm.h>
>  #include <linux/dax.h>
> @@ -75,6 +76,10 @@ 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_REMOVE) {
> +			return 0;
> +		}
>  		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_ONDISK);
>  		return -EFSCORRUPTED;
>  	}
> @@ -168,6 +173,7 @@ 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()!");
> @@ -182,6 +188,13 @@ 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_REMOVE) {
> +			/* Flush the log since device is about to be removed. */

If MF_MEM_REMOVE means "storage is about to go away" then perhaps the
only thing we need to do in xfs_dax_notify_failure is log a message
about the pending failure and then call sync_filesystem()?  This I think
could come before we even start looking at which device -- if any of the
filesystem blockdevs are about to be removed, the best we can do is
flush all the dirty data to disk.

--D

> +			error = xfs_log_force(mp, XFS_LOG_SYNC);
> +			if (error)
> +				return error;
> +			return -EOPNOTSUPP;
> +		}
>  		xfs_err(mp, "ondisk log corrupt, shutting down fs!");
>  		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_ONDISK);
>  		return -EFSCORRUPTED;
> @@ -211,8 +224,16 @@ xfs_dax_notify_failure(
>  	if (offset + len > ddev_end)
>  		len -= ddev_end - offset;
> 
> -	return xfs_dax_notify_ddev_failure(mp, BTOBB(offset), BTOBB(len),
> +	error = xfs_dax_notify_ddev_failure(mp, BTOBB(offset), BTOBB(len),
>  			mf_flags);
> +	if (error)
> +		return error;
> +
> +	if (mf_flags & MF_MEM_REMOVE) {
> +		xfs_flush_inodes(mp);
> +		error = xfs_log_force(mp, XFS_LOG_SYNC);
> +	}
> +	return error;
>  }
> 
>  const struct dax_holder_operations xfs_dax_holder_operations = {
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a2270e35a676..e66d23188323 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3236,6 +3236,7 @@ enum mf_flags {
>  	MF_SOFT_OFFLINE = 1 << 3,
>  	MF_UNPOISON = 1 << 4,
>  	MF_SW_SIMULATED = 1 << 5,
> +	MF_MEM_REMOVE = 1 << 6,
>  };
>  int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
>  		      unsigned long count, int mf_flags);
> --
> 2.36.1
> 
> 
> 

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

[ruansy.fnst]

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_REMOVE)
      -> xfs_dax_notify_failure()

Introduce MF_MEM_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 v4:
  1. sync_filesystem() at the beginning when MF_MEM_REMOVE
  2. Rebased on next-20220706

Changes since v3:
  1. Flush dirty files and logs when pmem is about to be removed.
  2. Rebased on next-20220701

Changes since v2:
  1. Rebased on next-20220615

Changes since v1:
  1. Drop the needless change of moving {kill,put}_dax()
  2. Rebased on '[PATCHSETS] v14 fsdax-rmap + v11 fsdax-reflink'[2]

[1]: https://lore.kernel.org/linux-mm/161604050314.1463742.14151665140035795571.stgit@dwillia2-desk3.amr.corp.intel.com/
[2]: https://lore.kernel.org/linux-xfs/20220508143620.1775214-1-ruansy.fnst@fujitsu.com/

Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
---
 drivers/dax/super.c         |  2 +-
 fs/xfs/xfs_notify_failure.c | 16 ++++++++++++++++
 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..d4bc83159d46 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -323,7 +323,7 @@ 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_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 aa8dc27c599c..728b0c1d0ddf 100644
--- a/fs/xfs/xfs_notify_failure.c
+++ b/fs/xfs/xfs_notify_failure.c
@@ -18,6 +18,7 @@
 #include "xfs_rmap_btree.h"
 #include "xfs_rtalloc.h"
 #include "xfs_trans.h"
+#include "xfs_log.h"
 
 #include <linux/mm.h>
 #include <linux/dax.h>
@@ -75,6 +76,10 @@ 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_REMOVE) {
+			return 0;
+		}
 		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_ONDISK);
 		return -EFSCORRUPTED;
 	}
@@ -168,6 +173,14 @@ xfs_dax_notify_failure(
 	struct xfs_mount	*mp = dax_holder(dax_dev);
 	u64			ddev_start;
 	u64			ddev_end;
+	int			error;
+
+	if (mf_flags & MF_MEM_REMOVE) {
+		xfs_info(mp, "device is about to be removed!");
+		error = sync_filesystem(mp->m_super);
+		if (error)
+			return error;
+	}
 
 	if (!(mp->m_sb.sb_flags & SB_BORN)) {
 		xfs_warn(mp, "filesystem is not ready for notify_failure()!");
@@ -182,6 +195,9 @@ 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_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 794ad19b57f8..3eab2d7ba884 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3240,6 +3240,7 @@ enum mf_flags {
 	MF_UNPOISON = 1 << 4,
 	MF_SW_SIMULATED = 1 << 5,
 	MF_NO_RETRY = 1 << 6,
+	MF_MEM_REMOVE = 1 << 7,
 };
 int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
 		      unsigned long count, int mf_flags);
-- 
2.37.0

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

[Darrick J. Wong]

On Fri, Jul 08, 2022 at 05:42:22AM +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_REMOVE)
>       -> xfs_dax_notify_failure()
> 
> Introduce MF_MEM_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 v4:
>   1. sync_filesystem() at the beginning when MF_MEM_REMOVE
>   2. Rebased on next-20220706
> 
> Changes since v3:
>   1. Flush dirty files and logs when pmem is about to be removed.
>   2. Rebased on next-20220701
> 
> Changes since v2:
>   1. Rebased on next-20220615
> 
> Changes since v1:
>   1. Drop the needless change of moving {kill,put}_dax()
>   2. Rebased on '[PATCHSETS] v14 fsdax-rmap + v11 fsdax-reflink'[2]
> 
> [1]: https://lore.kernel.org/linux-mm/161604050314.1463742.14151665140035795571.stgit@dwillia2-desk3.amr.corp.intel.com/
> [2]: https://lore.kernel.org/linux-xfs/20220508143620.1775214-1-ruansy.fnst@fujitsu.com/
> 
> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
> ---
>  drivers/dax/super.c         |  2 +-
>  fs/xfs/xfs_notify_failure.c | 16 ++++++++++++++++
>  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..d4bc83159d46 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c
> @@ -323,7 +323,7 @@ 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_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 aa8dc27c599c..728b0c1d0ddf 100644
> --- a/fs/xfs/xfs_notify_failure.c
> +++ b/fs/xfs/xfs_notify_failure.c
> @@ -18,6 +18,7 @@
>  #include "xfs_rmap_btree.h"
>  #include "xfs_rtalloc.h"
>  #include "xfs_trans.h"
> +#include "xfs_log.h"
>  
>  #include <linux/mm.h>
>  #include <linux/dax.h>
> @@ -75,6 +76,10 @@ 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_REMOVE) {
> +			return 0;
> +		}

Nit: no curly braces needed here.

>  		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_ONDISK);
>  		return -EFSCORRUPTED;
>  	}
> @@ -168,6 +173,14 @@ xfs_dax_notify_failure(
>  	struct xfs_mount	*mp = dax_holder(dax_dev);
>  	u64			ddev_start;
>  	u64			ddev_end;
> +	int			error;
> +
> +	if (mf_flags & MF_MEM_REMOVE) {
> +		xfs_info(mp, "device is about to be removed!");
> +		error = sync_filesystem(mp->m_super);

sync_filesystem requires callers to hold s_umount.  Does the dax media
failure code take that lock for us, or is this missing a lock?

Also, I'm not sure it's a good idea to sync_filesystem() before checking
if SB_BORN has been set.

> +		if (error)
> +			return error;
> +	}
>  
>  	if (!(mp->m_sb.sb_flags & SB_BORN)) {
>  		xfs_warn(mp, "filesystem is not ready for notify_failure()!");
> @@ -182,6 +195,9 @@ 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_REMOVE) {
> +			return 0;
> +		}

Same nit about not needing curly braces.

>  		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 794ad19b57f8..3eab2d7ba884 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3240,6 +3240,7 @@ enum mf_flags {
>  	MF_UNPOISON = 1 << 4,
>  	MF_SW_SIMULATED = 1 << 5,
>  	MF_NO_RETRY = 1 << 6,
> +	MF_MEM_REMOVE = 1 << 7,

This is more of a pre-removal notification, right?  I think the flag
value ought to be named that way too (MF_MEM_PRE_REMOVE).

--D

>  };
>  int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
>  		      unsigned long count, int mf_flags);
> -- 
> 2.37.0

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

[ruansy.fnst]

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);
+		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

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

[Darrick J. Wong]

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

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

[Dan Williams]

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?

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

[Darrick J. Wong]

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

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

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

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

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

[ruansy.fnst]

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

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

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

[Shiyang Ruan]

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

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

[Darrick J. Wong]

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.