Re: [PATCH v7 7/8] xfs: Implement ->notify_failure() for XFS
On Fri, Sep 24, 2021 at 09:09:58PM +0800, Shiyang Ruan wrote:
> +void fs_dax_register_holder(struct dax_device *dax_dev, void *holder,
> + const struct dax_holder_operations *ops)
> +{
> + dax_set_holder(dax_dev, holder, ops);
> +}
> +EXPORT_SYMBOL_GPL(fs_dax_register_holder);
> +
> +void fs_dax_unregister_holder(struct dax_device *dax_dev)
> +{
> + dax_set_holder(dax_dev, NULL, NULL);
> +}
> +EXPORT_SYMBOL_GPL(fs_dax_unregister_holder);
> +
> +void *fs_dax_get_holder(struct dax_device *dax_dev)
> +{
> + return dax_get_holder(dax_dev);
> +}
> +EXPORT_SYMBOL_GPL(fs_dax_get_holder);
These should not be in a XFS patch. But why do we even need this
wrappers?
> @@ -377,6 +385,8 @@ xfs_close_devices(
>
> xfs_free_buftarg(mp->m_logdev_targp);
> xfs_blkdev_put(logdev);
> + if (dax_logdev)
> + fs_dax_unregister_holder(dax_logdev);
> fs_put_dax(dax_logdev);
I'd prefer to include the fs_dax_unregister_holder in the fs_put_dax
call to avoid callers failing to unregister it.
> @@ -411,6 +425,9 @@ xfs_open_devices(
> struct block_device *logdev = NULL, *rtdev = NULL;
> int error;
>
> + if (dax_ddev)
> + fs_dax_register_holder(dax_ddev, mp,
> + _dax_holder_operations);
I'd include the holder registration with fs_dax_get_by_bdev as well.
Re: [PATCH v7 8/8] fsdax: add exception for reflinked files
On Thu, Oct 14, 2021 at 12:24:50PM -0700, Darrick J. Wong wrote: > It feels a little dangerous to have page->mapping for shared storage > point to an actual address_space when there are really multiple > potential address_spaces out there. If the mm or dax folks are ok with > doing this this way then I'll live with it, but it seems like you'd want > to leave /some/ kind of marker once you know that the page has multiple > owners and therefore regular mm rmap via page->mapping won't work. Yes, I thing poisoning page->mapping for the rmap enabled case seems like a better idea.
Re: [PATCH v7 4/8] pagemap,pmem: Introduce ->memory_failure()
Except for the error code inversion noticed by Darrick this looks fine to me: Reviewed-by: Christoph Hellwig
Re: [PATCH v7 3/8] mm: factor helpers for memory_failure_dev_pagemap
On Fri, Sep 24, 2021 at 09:09:54PM +0800, Shiyang Ruan wrote:
> memory_failure_dev_pagemap code is a bit complex before introduce RMAP
> feature for fsdax. So it is needed to factor some helper functions to
> simplify these code.
>
> Signed-off-by: Shiyang Ruan
> ---
> mm/memory-failure.c | 140
> 1 file changed, 76 insertions(+), 64 deletions(-)
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 54879c339024..8ff9b52823c0 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1430,6 +1430,79 @@ static int try_to_split_thp_page(struct page *page,
> const char *msg)
> return 0;
> }
>
> +static void unmap_and_kill(struct list_head *to_kill, unsigned long pfn,
> + struct address_space *mapping, pgoff_t index, int flags)
> +{
> + struct to_kill *tk;
> + unsigned long size = 0;
> +
> + list_for_each_entry(tk, to_kill, nd)
> + if (tk->size_shift)
> + size = max(size, 1UL << tk->size_shift);
> + if (size) {
Nit: an empty line here would be nice for readability.
> + if (pgmap->type == MEMORY_DEVICE_PRIVATE) {
> + /*
> + * TODO: Handle HMM pages which may need coordination
> + * with device-side memory.
> + */
> + return -EBUSY;
We've got rid of the HMM terminology for device private memory, so
I'd reword this update the comment to follow that while you're at it.
Otherwise looks good:
Reviewed-by: Christoph Hellwig
Re: [PATCH v7 1/8] dax: Use rwsem for dax_{read,write}_lock()
On Fri, Sep 24, 2021 at 09:09:52PM +0800, Shiyang Ruan wrote: > In order to introduce dax holder registration, we need a write lock for > dax. Because of the rarity of notification failures and the infrequency > of registration events, it would be better to be a global lock rather > than per-device. So, change the current lock to rwsem and introduce a > write lock for registration. I don't think taking the rw_semaphore everywhere will scale, as basically any DAX based I/O will take it (in read mode). So at a minimum we'd need a per-device percpu_rw_semaphore if we want to go there.
