This patchset is aimed to support shared pages tracking for fsdax.
Changes since V10:
- Use cmpxchg() to prevent concurrent registration/unregistration
- Use phys_addr_t for ->memory_failure()
- Add dax_entry_lock() for dax_lock_mapping_entry()
- Fix offset and length calculation at the boundary of a filesystem
This patchset moves owner tracking from dax_assocaite_entry() to pmem
device driver, by introducing an interface ->memory_failure() for struct
pagemap. This interface is called by memory_failure() in mm, and
implemented by pmem device.
Then call holder operations to find the filesystem which the corrupted
data located in, and call filesystem handler to track files or metadata
associated with this page.
Finally we are able to try to fix the corrupted data in filesystem and
do other necessary processing, such as killing processes who are using
the files affected.
The call trace is like this:
memory_failure()
|* fsdax case
|------------
|pgmap->ops->memory_failure() => pmem_pgmap_memory_failure()
| dax_holder_notify_failure() =>
| dax_device->holder_ops->notify_failure() =>
| - xfs_dax_notify_failure()
| |* xfs_dax_notify_failure()
| |--------------------------
| | xfs_rmap_query_range()
| | xfs_dax_failure_fn()
| | * corrupted on metadata
| | try to recover data, call xfs_force_shutdown()
| | * corrupted on file data
| | try to recover data, call mf_dax_kill_procs()
|* normal case
|-------------
|mf_generic_kill_procs()
==
Shiyang Ruan (8):
dax: Introduce holder for dax_device
mm: factor helpers for memory_failure_dev_pagemap
pagemap,pmem: Introduce ->memory_failure()
fsdax: Introduce dax_lock_mapping_entry()
mm: move pgoff_address() to vma_pgoff_address()
mm: Introduce mf_dax_kill_procs() for fsdax case
xfs: Implement ->notify_failure() for XFS
fsdax: set a CoW flag when associate reflink mappings
drivers/dax/super.c | 89 +++++++++++++
drivers/nvdimm/pmem.c | 16 +++
fs/dax.c | 140 ++++++++++++++++++---
fs/xfs/Makefile | 1 +
fs/xfs/xfs_buf.c | 12 ++
fs/xfs/xfs_fsops.c | 3 +
fs/xfs/xfs_mount.h | 1 +
fs/xfs/xfs_notify_failure.c | 235 +++++++++++++++++++++++++++++++++++
fs/xfs/xfs_notify_failure.h | 10 ++
fs/xfs/xfs_super.c | 6 +
include/linux/dax.h | 47 +++++++
include/linux/memremap.h | 12 ++
include/linux/mm.h | 17 +++
include/linux/page-flags.h | 6 +
mm/memory-failure.c | 240 ++++++++++++++++++++++++++----------
15 files changed, 747 insertions(+), 88 deletions(-)
create mode 100644 fs/xfs/xfs_notify_failure.c
create mode 100644 fs/xfs/xfs_notify_failure.h
--
2.35.1
Since it is not a DAX-specific function, move it into mm and rename it
to be a generic helper.
Signed-off-by: Shiyang Ruan <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Reviewed-by: Dan Williams <[email protected]>
---
fs/dax.c | 12 +-----------
include/linux/mm.h | 13 +++++++++++++
2 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/fs/dax.c b/fs/dax.c
index 653a2f390b72..f164cf64c611 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -853,16 +853,6 @@ static void *dax_insert_entry(struct xa_state *xas,
return entry;
}
-static inline
-unsigned long pgoff_address(pgoff_t pgoff, struct vm_area_struct *vma)
-{
- unsigned long address;
-
- address = vma->vm_start + ((pgoff - vma->vm_pgoff) << PAGE_SHIFT);
- VM_BUG_ON_VMA(address < vma->vm_start || address >= vma->vm_end, vma);
- return address;
-}
-
/* Walk all mappings of a given index of a file and writeprotect them */
static void dax_entry_mkclean(struct address_space *mapping, pgoff_t index,
unsigned long pfn)
@@ -882,7 +872,7 @@ static void dax_entry_mkclean(struct address_space *mapping, pgoff_t index,
if (!(vma->vm_flags & VM_SHARED))
continue;
- address = pgoff_address(index, vma);
+ address = vma_pgoff_address(vma, index);
/*
* follow_invalidate_pte() will use the range to call
diff --git a/include/linux/mm.h b/include/linux/mm.h
index e1a84b1e6787..9b1d56c5c224 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2816,6 +2816,19 @@ static inline unsigned long vma_pages(struct vm_area_struct *vma)
return (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
}
+/*
+ * Get user virtual address at the specific offset within a vma.
+ */
+static inline unsigned long vma_pgoff_address(struct vm_area_struct *vma,
+ pgoff_t pgoff)
+{
+ unsigned long address;
+
+ address = vma->vm_start + ((pgoff - vma->vm_pgoff) << PAGE_SHIFT);
+ VM_BUG_ON_VMA(address < vma->vm_start || address >= vma->vm_end, vma);
+ return address;
+}
+
/* Look up the first VMA which exactly match the interval vm_start ... vm_end */
static inline struct vm_area_struct *find_exact_vma(struct mm_struct *mm,
unsigned long vm_start, unsigned long vm_end)
--
2.35.1
This function is called at the end of RMAP routine, i.e. filesystem
recovery function, to collect and kill processes using a shared page of
DAX file. The difference with mf_generic_kill_procs() is, it accepts
file's (mapping,offset) instead of struct page because different files'
mappings and offsets may share the same page in fsdax mode.
It will be called when filesystem's RMAP results are found.
Signed-off-by: Shiyang Ruan <[email protected]>
Reviewed-by: Dan Williams <[email protected]>
---
include/linux/mm.h | 4 ++
mm/memory-failure.c | 96 ++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 90 insertions(+), 10 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 9b1d56c5c224..0420189e4788 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3195,6 +3195,10 @@ enum mf_flags {
MF_SOFT_OFFLINE = 1 << 3,
MF_UNPOISON = 1 << 4,
};
+#if IS_ENABLED(CONFIG_FS_DAX)
+int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
+ unsigned long count, int mf_flags);
+#endif /* CONFIG_FS_DAX */
extern int memory_failure(unsigned long pfn, int flags);
extern void memory_failure_queue(unsigned long pfn, int flags);
extern void memory_failure_queue_kick(int cpu);
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 8dd2f58c1aa8..2ba79c3869ed 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -304,10 +304,9 @@ void shake_page(struct page *p)
}
EXPORT_SYMBOL_GPL(shake_page);
-static unsigned long dev_pagemap_mapping_shift(struct page *page,
- struct vm_area_struct *vma)
+static unsigned long dev_pagemap_mapping_shift(struct vm_area_struct *vma,
+ unsigned long address)
{
- unsigned long address = vma_address(page, vma);
unsigned long ret = 0;
pgd_t *pgd;
p4d_t *p4d;
@@ -346,10 +345,14 @@ static unsigned long dev_pagemap_mapping_shift(struct page *page,
/*
* Schedule a process for later kill.
* Uses GFP_ATOMIC allocations to avoid potential recursions in the VM.
+ *
+ * Notice: @fsdax_pgoff is used only when @p is a fsdax page.
+ * In other cases, such as anonymous and file-backend page, the address to be
+ * killed can be caculated by @p itself.
*/
static void add_to_kill(struct task_struct *tsk, struct page *p,
- struct vm_area_struct *vma,
- struct list_head *to_kill)
+ pgoff_t fsdax_pgoff, struct vm_area_struct *vma,
+ struct list_head *to_kill)
{
struct to_kill *tk;
@@ -360,9 +363,15 @@ static void add_to_kill(struct task_struct *tsk, struct page *p,
}
tk->addr = page_address_in_vma(p, vma);
- if (is_zone_device_page(p))
- tk->size_shift = dev_pagemap_mapping_shift(p, vma);
- else
+ if (is_zone_device_page(p)) {
+ /*
+ * Since page->mapping is not used for fsdax, we need
+ * calculate the address based on the vma.
+ */
+ if (p->pgmap->type == MEMORY_DEVICE_FS_DAX)
+ tk->addr = vma_pgoff_address(vma, fsdax_pgoff);
+ tk->size_shift = dev_pagemap_mapping_shift(vma, tk->addr);
+ } else
tk->size_shift = page_shift(compound_head(p));
/*
@@ -510,7 +519,7 @@ static void collect_procs_anon(struct page *page, struct list_head *to_kill,
if (!page_mapped_in_vma(page, vma))
continue;
if (vma->vm_mm == t->mm)
- add_to_kill(t, page, vma, to_kill);
+ add_to_kill(t, page, 0, vma, to_kill);
}
}
read_unlock(&tasklist_lock);
@@ -546,12 +555,40 @@ static void collect_procs_file(struct page *page, struct list_head *to_kill,
* to be informed of all such data corruptions.
*/
if (vma->vm_mm == t->mm)
- add_to_kill(t, page, vma, to_kill);
+ add_to_kill(t, page, 0, vma, to_kill);
+ }
+ }
+ read_unlock(&tasklist_lock);
+ i_mmap_unlock_read(mapping);
+}
+
+#if IS_ENABLED(CONFIG_FS_DAX)
+/*
+ * Collect processes when the error hit a fsdax page.
+ */
+static void collect_procs_fsdax(struct page *page,
+ struct address_space *mapping, pgoff_t pgoff,
+ struct list_head *to_kill)
+{
+ struct vm_area_struct *vma;
+ struct task_struct *tsk;
+
+ i_mmap_lock_read(mapping);
+ read_lock(&tasklist_lock);
+ for_each_process(tsk) {
+ struct task_struct *t = task_early_kill(tsk, true);
+
+ if (!t)
+ continue;
+ vma_interval_tree_foreach(vma, &mapping->i_mmap, pgoff, pgoff) {
+ if (vma->vm_mm == t->mm)
+ add_to_kill(t, page, pgoff, vma, to_kill);
}
}
read_unlock(&tasklist_lock);
i_mmap_unlock_read(mapping);
}
+#endif /* CONFIG_FS_DAX */
/*
* Collect the processes who have the corrupted page mapped to kill.
@@ -1574,6 +1611,45 @@ static int mf_generic_kill_procs(unsigned long long pfn, int flags,
return 0;
}
+#if IS_ENABLED(CONFIG_FS_DAX)
+/**
+ * mf_dax_kill_procs - Collect and kill processes who are using this file range
+ * @mapping: the file in use
+ * @index: start pgoff of the range within the file
+ * @count: length of the range, in unit of PAGE_SIZE
+ * @mf_flags: memory failure flags
+ */
+int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
+ unsigned long count, int mf_flags)
+{
+ LIST_HEAD(to_kill);
+ dax_entry_t cookie;
+ struct page *page;
+ size_t end = index + count;
+
+ mf_flags |= MF_ACTION_REQUIRED | MF_MUST_KILL;
+
+ for (; index < end; index++) {
+ page = NULL;
+ cookie = dax_lock_mapping_entry(mapping, index, &page);
+ if (!cookie)
+ return -EBUSY;
+ if (!page)
+ goto unlock;
+
+ SetPageHWPoison(page);
+
+ collect_procs_fsdax(page, mapping, index, &to_kill);
+ unmap_and_kill(&to_kill, page_to_pfn(page), mapping,
+ index, mf_flags);
+unlock:
+ dax_unlock_mapping_entry(mapping, index, cookie);
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(mf_dax_kill_procs);
+#endif /* CONFIG_FS_DAX */
+
static int memory_failure_hugetlb(unsigned long pfn, int flags)
{
struct page *p = pfn_to_page(pfn);
--
2.35.1
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 <[email protected]>
Reviewed-by: Darrick J. Wong <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Reviewed-by: Dan Williams <[email protected]>
---
mm/memory-failure.c | 141 ++++++++++++++++++++++++--------------------
1 file changed, 77 insertions(+), 64 deletions(-)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 14ae5c18e776..98b6144e4b9b 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1500,6 +1500,80 @@ 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) {
+ /*
+ * Unmap the largest mapping to avoid breaking up device-dax
+ * mappings which are constant size. The actual size of the
+ * mapping being torn down is communicated in siginfo, see
+ * kill_proc()
+ */
+ loff_t start = (index << PAGE_SHIFT) & ~(size - 1);
+
+ unmap_mapping_range(mapping, start, size, 0);
+ }
+
+ kill_procs(to_kill, flags & MF_MUST_KILL, false, pfn, flags);
+}
+
+static int mf_generic_kill_procs(unsigned long long pfn, int flags,
+ struct dev_pagemap *pgmap)
+{
+ struct page *page = pfn_to_page(pfn);
+ LIST_HEAD(to_kill);
+ dax_entry_t cookie;
+
+ /*
+ * Prevent the inode from being freed while we are interrogating
+ * the address_space, typically this would be handled by
+ * lock_page(), but dax pages do not use the page lock. This
+ * also prevents changes to the mapping of this pfn until
+ * poison signaling is complete.
+ */
+ cookie = dax_lock_page(page);
+ if (!cookie)
+ return -EBUSY;
+
+ if (hwpoison_filter(page))
+ return 0;
+
+ if (pgmap->type == MEMORY_DEVICE_PRIVATE) {
+ /*
+ * TODO: Handle HMM pages which may need coordination
+ * with device-side memory.
+ */
+ return -EBUSY;
+ }
+
+ /*
+ * Use this flag as an indication that the dax page has been
+ * remapped UC to prevent speculative consumption of poison.
+ */
+ SetPageHWPoison(page);
+
+ /*
+ * Unlike System-RAM there is no possibility to swap in a
+ * different physical page at a given virtual address, so all
+ * userspace consumption of ZONE_DEVICE memory necessitates
+ * SIGBUS (i.e. MF_MUST_KILL)
+ */
+ flags |= MF_ACTION_REQUIRED | MF_MUST_KILL;
+ collect_procs(page, &to_kill, true);
+
+ unmap_and_kill(&to_kill, pfn, page->mapping, page->index, flags);
+ dax_unlock_page(page, cookie);
+ return 0;
+}
+
static int memory_failure_hugetlb(unsigned long pfn, int flags)
{
struct page *p = pfn_to_page(pfn);
@@ -1576,12 +1650,8 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
struct dev_pagemap *pgmap)
{
struct page *page = pfn_to_page(pfn);
- unsigned long size = 0;
- struct to_kill *tk;
LIST_HEAD(tokill);
- int rc = -EBUSY;
- loff_t start;
- dax_entry_t cookie;
+ int rc = -ENXIO;
if (flags & MF_COUNT_INCREASED)
/*
@@ -1590,67 +1660,10 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
put_page(page);
/* device metadata space is not recoverable */
- if (!pgmap_pfn_valid(pgmap, pfn)) {
- rc = -ENXIO;
- goto out;
- }
-
- /*
- * Prevent the inode from being freed while we are interrogating
- * the address_space, typically this would be handled by
- * lock_page(), but dax pages do not use the page lock. This
- * also prevents changes to the mapping of this pfn until
- * poison signaling is complete.
- */
- cookie = dax_lock_page(page);
- if (!cookie)
+ if (!pgmap_pfn_valid(pgmap, pfn))
goto out;
- if (hwpoison_filter(page)) {
- rc = 0;
- goto unlock;
- }
-
- if (pgmap->type == MEMORY_DEVICE_PRIVATE) {
- /*
- * TODO: Handle HMM pages which may need coordination
- * with device-side memory.
- */
- goto unlock;
- }
-
- /*
- * Use this flag as an indication that the dax page has been
- * remapped UC to prevent speculative consumption of poison.
- */
- SetPageHWPoison(page);
-
- /*
- * Unlike System-RAM there is no possibility to swap in a
- * different physical page at a given virtual address, so all
- * userspace consumption of ZONE_DEVICE memory necessitates
- * SIGBUS (i.e. MF_MUST_KILL)
- */
- flags |= MF_ACTION_REQUIRED | MF_MUST_KILL;
- collect_procs(page, &tokill, flags & MF_ACTION_REQUIRED);
-
- list_for_each_entry(tk, &tokill, nd)
- if (tk->size_shift)
- size = max(size, 1UL << tk->size_shift);
- if (size) {
- /*
- * Unmap the largest mapping to avoid breaking up
- * device-dax mappings which are constant size. The
- * actual size of the mapping being torn down is
- * communicated in siginfo, see kill_proc()
- */
- start = (page->index << PAGE_SHIFT) & ~(size - 1);
- unmap_mapping_range(page->mapping, start, size, 0);
- }
- kill_procs(&tokill, flags & MF_MUST_KILL, false, pfn, flags);
- rc = 0;
-unlock:
- dax_unlock_page(page, cookie);
+ rc = mf_generic_kill_procs(pfn, flags, pgmap);
out:
/* drop pgmap ref acquired in caller */
put_dev_pagemap(pgmap);
--
2.35.1
Introduce a PAGE_MAPPING_DAX_COW flag to support association with CoW file
mappings. In this case, since the dax-rmap has already took the
responsibility to look up for shared files by given dax page,
the page->mapping is no longer to used for rmap but for marking that
this dax page is shared. And to make sure disassociation works fine, we
use page->index as refcount, and clear page->mapping to the initial
state when page->index is decreased to 0.
With the help of this new flag, it is able to distinguish normal case
and CoW case, and keep the warning in normal case.
==
PS: The @cow added for dax_associate_entry(), is used to let it know
whether the entry is to be shared during iomap operation. It is decided
by iomap,srcmap's flag, and will be used in another patchset(
fsdax,xfs: Add reflink&dedupe support for fsdax[1]).
In this patch, we set @cow always false for now.
[1] https://lore.kernel.org/linux-xfs/[email protected]/
==
Signed-off-by: Shiyang Ruan <[email protected]>
---
fs/dax.c | 65 ++++++++++++++++++++++++++++++++------
include/linux/page-flags.h | 6 ++++
2 files changed, 62 insertions(+), 9 deletions(-)
diff --git a/fs/dax.c b/fs/dax.c
index f164cf64c611..b97106a0a0b1 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -334,13 +334,46 @@ static unsigned long dax_end_pfn(void *entry)
for (pfn = dax_to_pfn(entry); \
pfn < dax_end_pfn(entry); pfn++)
+static inline void dax_mapping_set_cow_flag(struct address_space *mapping)
+{
+ mapping = (struct address_space *)PAGE_MAPPING_DAX_COW;
+}
+
+static inline bool dax_mapping_is_cow(struct address_space *mapping)
+{
+ return (unsigned long)mapping == PAGE_MAPPING_DAX_COW;
+}
+
/*
- * TODO: for reflink+dax we need a way to associate a single page with
- * multiple address_space instances at different linear_page_index()
- * offsets.
+ * Set or Update the page->mapping with FS_DAX_MAPPING_COW flag.
+ * Return true if it is an Update.
+ */
+static inline bool dax_mapping_set_cow(struct page *page)
+{
+ if (page->mapping) {
+ /* flag already set */
+ if (dax_mapping_is_cow(page->mapping))
+ return false;
+
+ /*
+ * This page has been mapped even before it is shared, just
+ * need to set this FS_DAX_MAPPING_COW flag.
+ */
+ dax_mapping_set_cow_flag(page->mapping);
+ return true;
+ }
+ /* Newly associate CoW mapping */
+ dax_mapping_set_cow_flag(page->mapping);
+ return false;
+}
+
+/*
+ * When it is called in dax_insert_entry(), the cow flag will indicate that
+ * whether this entry is shared by multiple files. If so, set the page->mapping
+ * to be FS_DAX_MAPPING_COW, and use page->index as refcount.
*/
static void dax_associate_entry(void *entry, struct address_space *mapping,
- struct vm_area_struct *vma, unsigned long address)
+ struct vm_area_struct *vma, unsigned long address, bool cow)
{
unsigned long size = dax_entry_size(entry), pfn, index;
int i = 0;
@@ -352,9 +385,17 @@ static void dax_associate_entry(void *entry, struct address_space *mapping,
for_each_mapped_pfn(entry, pfn) {
struct page *page = pfn_to_page(pfn);
- WARN_ON_ONCE(page->mapping);
- page->mapping = mapping;
- page->index = index + i++;
+ if (cow) {
+ if (dax_mapping_set_cow(page)) {
+ /* Was normal, now updated to CoW */
+ page->index = 2;
+ } else
+ page->index++;
+ } else {
+ WARN_ON_ONCE(page->mapping);
+ page->mapping = mapping;
+ page->index = index + i++;
+ }
}
}
@@ -370,7 +411,12 @@ static void dax_disassociate_entry(void *entry, struct address_space *mapping,
struct page *page = pfn_to_page(pfn);
WARN_ON_ONCE(trunc && page_ref_count(page) > 1);
- WARN_ON_ONCE(page->mapping && page->mapping != mapping);
+ if (!dax_mapping_is_cow(page->mapping)) {
+ /* keep the CoW flag if this page is still shared */
+ if (page->index-- > 0)
+ continue;
+ } else
+ WARN_ON_ONCE(page->mapping && page->mapping != mapping);
page->mapping = NULL;
page->index = 0;
}
@@ -829,7 +875,8 @@ static void *dax_insert_entry(struct xa_state *xas,
void *old;
dax_disassociate_entry(entry, mapping, false);
- dax_associate_entry(new_entry, mapping, vmf->vma, vmf->address);
+ dax_associate_entry(new_entry, mapping, vmf->vma, vmf->address,
+ false);
/*
* Only swap our new entry into the page cache if the current
* entry is a zero page or an empty entry. If a normal PTE or
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 1c3b6e5c8bfd..6370d279795a 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -572,6 +572,12 @@ __PAGEFLAG(Reported, reported, PF_NO_COMPOUND)
#define PAGE_MAPPING_KSM (PAGE_MAPPING_ANON | PAGE_MAPPING_MOVABLE)
#define PAGE_MAPPING_FLAGS (PAGE_MAPPING_ANON | PAGE_MAPPING_MOVABLE)
+/*
+ * Different with flags above, this flag is used only for fsdax mode. It
+ * indicates that this page->mapping is now under reflink case.
+ */
+#define PAGE_MAPPING_DAX_COW 0x1
+
static __always_inline int PageMappingFlags(struct page *page)
{
return ((unsigned long)page->mapping & PAGE_MAPPING_FLAGS) != 0;
--
2.35.1
Introduce xfs_notify_failure.c to handle failure related works, such as
implement ->notify_failure(), register/unregister dax holder in xfs, and
so on.
If the rmap feature of XFS enabled, we can query it to find files and
metadata which are associated with the corrupt data. For now all we do
is kill processes with that file mapped into their address spaces, but
future patches could actually do something about corrupt metadata.
After that, the memory failure needs to notify the processes who are
using those files.
Signed-off-by: Shiyang Ruan <[email protected]>
---
fs/xfs/Makefile | 1 +
fs/xfs/xfs_buf.c | 12 ++
fs/xfs/xfs_fsops.c | 3 +
fs/xfs/xfs_mount.h | 1 +
fs/xfs/xfs_notify_failure.c | 235 ++++++++++++++++++++++++++++++++++++
fs/xfs/xfs_notify_failure.h | 10 ++
fs/xfs/xfs_super.c | 6 +
7 files changed, 268 insertions(+)
create mode 100644 fs/xfs/xfs_notify_failure.c
create mode 100644 fs/xfs/xfs_notify_failure.h
diff --git a/fs/xfs/Makefile b/fs/xfs/Makefile
index 04611a1068b4..389970b3e13b 100644
--- a/fs/xfs/Makefile
+++ b/fs/xfs/Makefile
@@ -84,6 +84,7 @@ xfs-y += xfs_aops.o \
xfs_message.o \
xfs_mount.o \
xfs_mru_cache.o \
+ xfs_notify_failure.o \
xfs_pwork.o \
xfs_reflink.o \
xfs_stats.o \
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index b45e0d50a405..5f4984a5e108 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -19,6 +19,7 @@
#include "xfs_errortag.h"
#include "xfs_error.h"
#include "xfs_ag.h"
+#include "xfs_notify_failure.h"
static struct kmem_cache *xfs_buf_cache;
@@ -1892,6 +1893,8 @@ xfs_free_buftarg(
list_lru_destroy(&btp->bt_lru);
blkdev_issue_flush(btp->bt_bdev);
+ if (btp->bt_daxdev)
+ dax_unregister_holder(btp->bt_daxdev, btp->bt_mount);
fs_put_dax(btp->bt_daxdev);
kmem_free(btp);
@@ -1939,6 +1942,7 @@ xfs_alloc_buftarg(
struct block_device *bdev)
{
xfs_buftarg_t *btp;
+ int error;
btp = kmem_zalloc(sizeof(*btp), KM_NOFS);
@@ -1946,6 +1950,14 @@ xfs_alloc_buftarg(
btp->bt_dev = bdev->bd_dev;
btp->bt_bdev = bdev;
btp->bt_daxdev = fs_dax_get_by_bdev(bdev, &btp->bt_dax_part_off);
+ if (btp->bt_daxdev) {
+ error = dax_register_holder(btp->bt_daxdev, mp,
+ &xfs_dax_holder_operations);
+ if (error) {
+ xfs_err(mp, "DAX device already in use?!");
+ goto error_free;
+ }
+ }
/*
* Buffer IO error rate limiting. Limit it to no more than 10 messages
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 33e26690a8c4..d4d36c5bef11 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -542,6 +542,9 @@ xfs_do_force_shutdown(
} else if (flags & SHUTDOWN_CORRUPT_INCORE) {
tag = XFS_PTAG_SHUTDOWN_CORRUPT;
why = "Corruption of in-memory data";
+ } else if (flags & SHUTDOWN_CORRUPT_ONDISK) {
+ tag = XFS_PTAG_SHUTDOWN_CORRUPT;
+ why = "Corruption of on-disk metadata";
} else {
tag = XFS_PTAG_SHUTDOWN_IOERROR;
why = "Metadata I/O Error";
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 00720a02e761..47ff4ac53c4c 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -435,6 +435,7 @@ void xfs_do_force_shutdown(struct xfs_mount *mp, int flags, char *fname,
#define SHUTDOWN_LOG_IO_ERROR 0x0002 /* write attempt to the log failed */
#define SHUTDOWN_FORCE_UMOUNT 0x0004 /* shutdown from a forced unmount */
#define SHUTDOWN_CORRUPT_INCORE 0x0008 /* corrupt in-memory data structures */
+#define SHUTDOWN_CORRUPT_ONDISK 0x0010 /* corrupt metadata on device */
#define XFS_SHUTDOWN_STRINGS \
{ SHUTDOWN_META_IO_ERROR, "metadata_io" }, \
diff --git a/fs/xfs/xfs_notify_failure.c b/fs/xfs/xfs_notify_failure.c
new file mode 100644
index 000000000000..b057e9f7eb31
--- /dev/null
+++ b/fs/xfs/xfs_notify_failure.c
@@ -0,0 +1,235 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2022 Fujitsu. All Rights Reserved.
+ */
+
+#include "xfs.h"
+#include "xfs_shared.h"
+#include "xfs_format.h"
+#include "xfs_log_format.h"
+#include "xfs_trans_resv.h"
+#include "xfs_mount.h"
+#include "xfs_alloc.h"
+#include "xfs_bit.h"
+#include "xfs_btree.h"
+#include "xfs_inode.h"
+#include "xfs_icache.h"
+#include "xfs_rmap.h"
+#include "xfs_rmap_btree.h"
+#include "xfs_rtalloc.h"
+#include "xfs_trans.h"
+
+#include <linux/mm.h>
+#include <linux/dax.h>
+
+struct failure_info {
+ xfs_agblock_t startblock;
+ xfs_extlen_t blockcount;
+ int mf_flags;
+};
+
+#if IS_ENABLED(CONFIG_MEMORY_FAILURE) && IS_ENABLED(CONFIG_FS_DAX)
+static pgoff_t
+xfs_failure_pgoff(
+ struct xfs_mount *mp,
+ const struct xfs_rmap_irec *rec,
+ const struct failure_info *notify)
+{
+ uint64_t pos = rec->rm_offset;
+
+ if (notify->startblock > rec->rm_startblock)
+ pos += XFS_FSB_TO_B(mp,
+ notify->startblock - rec->rm_startblock);
+ return pos >> PAGE_SHIFT;
+}
+
+static unsigned long
+xfs_failure_pgcnt(
+ struct xfs_mount *mp,
+ const struct xfs_rmap_irec *rec,
+ const struct failure_info *notify)
+{
+ xfs_agblock_t end_rec;
+ xfs_agblock_t end_notify;
+ xfs_agblock_t start_cross;
+ xfs_agblock_t end_cross;
+
+ start_cross = max(rec->rm_startblock, notify->startblock);
+
+ end_rec = rec->rm_startblock + rec->rm_blockcount;
+ end_notify = notify->startblock + notify->blockcount;
+ end_cross = min(end_rec, end_notify);
+
+ return XFS_FSB_TO_B(mp, end_cross - start_cross) >> PAGE_SHIFT;
+}
+
+static int
+xfs_dax_failure_fn(
+ struct xfs_btree_cur *cur,
+ const struct xfs_rmap_irec *rec,
+ void *data)
+{
+ struct xfs_mount *mp = cur->bc_mp;
+ struct xfs_inode *ip;
+ struct failure_info *notify = data;
+ int error = 0;
+
+ if (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;
+ }
+
+ /* Get files that incore, filter out others that are not in use. */
+ error = xfs_iget(mp, cur->bc_tp, rec->rm_owner, XFS_IGET_INCORE,
+ 0, &ip);
+ /* Continue the rmap query if the inode isn't incore */
+ if (error == -ENODATA)
+ return 0;
+ if (error)
+ return error;
+
+ error = mf_dax_kill_procs(VFS_I(ip)->i_mapping,
+ xfs_failure_pgoff(mp, rec, notify),
+ xfs_failure_pgcnt(mp, rec, notify),
+ notify->mf_flags);
+ xfs_irele(ip);
+ return error;
+}
+#else
+static int
+xfs_dax_failure_fn(
+ struct xfs_btree_cur *cur,
+ const struct xfs_rmap_irec *rec,
+ void *data)
+{
+ struct xfs_mount *mp = cur->bc_mp;
+
+ /* No other option besides shutting down the fs. */
+ xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_ONDISK);
+ return -EFSCORRUPTED;
+}
+#endif /* CONFIG_MEMORY_FAILURE && CONFIG_FS_DAX */
+
+static int
+xfs_dax_notify_ddev_failure(
+ struct xfs_mount *mp,
+ xfs_daddr_t daddr,
+ xfs_daddr_t bblen,
+ int mf_flags)
+{
+ struct xfs_trans *tp = NULL;
+ struct xfs_btree_cur *cur = NULL;
+ struct xfs_buf *agf_bp = NULL;
+ int error = 0;
+ xfs_fsblock_t fsbno = XFS_DADDR_TO_FSB(mp, daddr);
+ xfs_agnumber_t agno = XFS_FSB_TO_AGNO(mp, fsbno);
+ xfs_fsblock_t end_fsbno = XFS_DADDR_TO_FSB(mp, daddr + bblen);
+ xfs_agnumber_t end_agno = XFS_FSB_TO_AGNO(mp, end_fsbno);
+
+ error = xfs_trans_alloc_empty(mp, &tp);
+ if (error)
+ return error;
+
+ for (; agno <= end_agno; agno++) {
+ struct xfs_rmap_irec ri_low = { };
+ struct xfs_rmap_irec ri_high;
+ struct failure_info notify;
+ struct xfs_agf *agf;
+ xfs_agblock_t agend;
+
+ error = xfs_alloc_read_agf(mp, tp, agno, 0, &agf_bp);
+ if (error)
+ break;
+
+ cur = xfs_rmapbt_init_cursor(mp, tp, agf_bp, agf_bp->b_pag);
+
+ /*
+ * Set the rmap range from ri_low to ri_high, which represents
+ * a [start, end] where we looking for the files or metadata.
+ * The part of range out of a AG will be ignored. So, it's fine
+ * to set ri_low to "startblock" in all loops. When it reaches
+ * the last AG, set the ri_high to "endblock" to make sure we
+ * actually end at the end.
+ */
+ memset(&ri_high, 0xFF, sizeof(ri_high));
+ ri_low.rm_startblock = XFS_FSB_TO_AGBNO(mp, fsbno);
+ if (agno == end_agno)
+ ri_high.rm_startblock = XFS_FSB_TO_AGBNO(mp, end_fsbno);
+
+ agf = agf_bp->b_addr;
+ agend = min(be32_to_cpu(agf->agf_length),
+ ri_high.rm_startblock);
+ notify.startblock = ri_low.rm_startblock;
+ notify.blockcount = agend - ri_low.rm_startblock;
+
+ error = xfs_rmap_query_range(cur, &ri_low, &ri_high,
+ xfs_dax_failure_fn, ¬ify);
+ xfs_btree_del_cursor(cur, error);
+ xfs_trans_brelse(tp, agf_bp);
+ if (error)
+ break;
+
+ fsbno = XFS_AGB_TO_FSB(mp, agno + 1, 0);
+ }
+
+ xfs_trans_cancel(tp);
+ return error;
+}
+
+static int
+xfs_dax_notify_failure(
+ struct dax_device *dax_dev,
+ u64 offset,
+ u64 len,
+ int mf_flags)
+{
+ struct xfs_mount *mp = dax_holder(dax_dev);
+ u64 ddev_start;
+ u64 ddev_end;
+
+ if (mp->m_rtdev_targp && mp->m_rtdev_targp->bt_daxdev == dax_dev) {
+ xfs_warn(mp,
+ "notify_failure() not supported on realtime device!");
+ return -EOPNOTSUPP;
+ }
+
+ if (mp->m_logdev_targp && mp->m_logdev_targp->bt_daxdev == dax_dev &&
+ mp->m_logdev_targp != mp->m_ddev_targp) {
+ xfs_err(mp, "ondisk log corrupt, shutting down fs!");
+ xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_ONDISK);
+ return -EFSCORRUPTED;
+ }
+
+ if (!xfs_has_rmapbt(mp)) {
+ xfs_warn(mp, "notify_failure() needs rmapbt enabled!");
+ return -EOPNOTSUPP;
+ }
+
+ ddev_start = mp->m_ddev_targp->bt_dax_part_off;
+ ddev_end = ddev_start +
+ (mp->m_ddev_targp->bt_bdev->bd_nr_sectors << SECTOR_SHIFT) - 1;
+
+ /* Ignore the range out of filesystem area */
+ if ((offset + len) < ddev_start)
+ return -ENXIO;
+ if (offset > ddev_end)
+ return -ENXIO;
+
+ /* Calculate the real range when it touches the boundary */
+ if (offset > ddev_start)
+ offset -= ddev_start;
+ else {
+ len -= ddev_start - offset;
+ offset = 0;
+ }
+ if ((offset + len) > ddev_end)
+ len -= ddev_end - offset;
+
+ return xfs_dax_notify_ddev_failure(mp, BTOBB(offset), BTOBB(len),
+ mf_flags);
+}
+
+const struct dax_holder_operations xfs_dax_holder_operations = {
+ .notify_failure = xfs_dax_notify_failure,
+};
diff --git a/fs/xfs/xfs_notify_failure.h b/fs/xfs/xfs_notify_failure.h
new file mode 100644
index 000000000000..76187b9620f9
--- /dev/null
+++ b/fs/xfs/xfs_notify_failure.h
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2022 Fujitsu. All Rights Reserved.
+ */
+#ifndef __XFS_NOTIFY_FAILURE_H__
+#define __XFS_NOTIFY_FAILURE_H__
+
+extern const struct dax_holder_operations xfs_dax_holder_operations;
+
+#endif /* __XFS_NOTIFY_FAILURE_H__ */
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index e8f37bdc8354..b8de6ed2c888 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -353,6 +353,12 @@ xfs_setup_dax_always(
return -EINVAL;
}
+ if (xfs_has_reflink(mp) && !xfs_has_rmapbt(mp)) {
+ xfs_alert(mp,
+ "need rmapbt when both DAX and reflink enabled.");
+ return -EINVAL;
+ }
+
xfs_warn(mp, "DAX enabled. Warning: EXPERIMENTAL, use at your own risk");
return 0;
--
2.35.1
To easily track filesystem from a pmem device, we introduce a holder for
dax_device structure, and also its operation. This holder is used to
remember who is using this dax_device:
- When it is the backend of a filesystem, the holder will be the
instance of this filesystem.
- When this pmem device is one of the targets in a mapped device, the
holder will be this mapped device. In this case, the mapped device
has its own dax_device and it will follow the first rule. So that we
can finally track to the filesystem we needed.
The holder and holder_ops will be set when filesystem is being mounted,
or an target device is being activated.
Signed-off-by: Shiyang Ruan <[email protected]>
---
drivers/dax/super.c | 89 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/dax.h | 32 ++++++++++++++++
2 files changed, 121 insertions(+)
diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index e3029389d809..da5798e19d57 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -21,6 +21,9 @@
* @cdev: optional character interface for "device dax"
* @private: dax driver private data
* @flags: state and boolean properties
+ * @ops: operations for dax_device
+ * @holder_data: holder of a dax_device: could be filesystem or mapped device
+ * @holder_ops: operations for the inner holder
*/
struct dax_device {
struct inode inode;
@@ -28,6 +31,8 @@ struct dax_device {
void *private;
unsigned long flags;
const struct dax_operations *ops;
+ void *holder_data;
+ const struct dax_holder_operations *holder_ops;
};
static dev_t dax_devt;
@@ -193,6 +198,29 @@ int dax_zero_page_range(struct dax_device *dax_dev, pgoff_t pgoff,
}
EXPORT_SYMBOL_GPL(dax_zero_page_range);
+int dax_holder_notify_failure(struct dax_device *dax_dev, u64 off,
+ u64 len, int mf_flags)
+{
+ int rc, id;
+
+ id = dax_read_lock();
+ if (!dax_alive(dax_dev)) {
+ rc = -ENXIO;
+ goto out;
+ }
+
+ if (!dax_dev->holder_ops) {
+ rc = -EOPNOTSUPP;
+ goto out;
+ }
+
+ rc = dax_dev->holder_ops->notify_failure(dax_dev, off, len, mf_flags);
+out:
+ dax_read_unlock(id);
+ return rc;
+}
+EXPORT_SYMBOL_GPL(dax_holder_notify_failure);
+
#ifdef CONFIG_ARCH_HAS_PMEM_API
void arch_wb_cache_pmem(void *addr, size_t size);
void dax_flush(struct dax_device *dax_dev, void *addr, size_t size)
@@ -268,6 +296,10 @@ void kill_dax(struct dax_device *dax_dev)
clear_bit(DAXDEV_ALIVE, &dax_dev->flags);
synchronize_srcu(&dax_srcu);
+
+ /* clear holder data */
+ dax_dev->holder_ops = NULL;
+ dax_dev->holder_data = NULL;
}
EXPORT_SYMBOL_GPL(kill_dax);
@@ -409,6 +441,63 @@ void put_dax(struct dax_device *dax_dev)
}
EXPORT_SYMBOL_GPL(put_dax);
+/**
+ * dax_holder() - obtain the holder of a dax device
+ * @dax_dev: a dax_device instance
+
+ * Return: the holder's data which represents the holder if registered,
+ * otherwize NULL.
+ */
+void *dax_holder(struct dax_device *dax_dev)
+{
+ if (!dax_alive(dax_dev))
+ return NULL;
+
+ return dax_dev->holder_data;
+}
+EXPORT_SYMBOL_GPL(dax_holder);
+
+/**
+ * dax_register_holder() - register a holder to a dax device
+ * @dax_dev: a dax_device instance
+ * @holder: a pointer to a holder's data which represents the holder
+ * @ops: operations of this holder
+
+ * Return: negative errno if an error occurs, otherwise 0.
+ */
+int dax_register_holder(struct dax_device *dax_dev, void *holder,
+ const struct dax_holder_operations *ops)
+{
+ if (!dax_alive(dax_dev))
+ return -ENXIO;
+
+ if (cmpxchg(&dax_dev->holder_data, NULL, holder))
+ return -EBUSY;
+
+ dax_dev->holder_ops = ops;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(dax_register_holder);
+
+/**
+ * dax_unregister_holder() - unregister the holder for a dax device
+ * @dax_dev: a dax_device instance
+ * @holder: the holder to be unregistered
+ *
+ * Return: negative errno if an error occurs, otherwise 0.
+ */
+int dax_unregister_holder(struct dax_device *dax_dev, void *holder)
+{
+ if (!dax_alive(dax_dev))
+ return -ENXIO;
+
+ if (cmpxchg(&dax_dev->holder_data, holder, NULL) != holder)
+ return -EBUSY;
+ dax_dev->holder_ops = NULL;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(dax_unregister_holder);
+
/**
* inode_dax: convert a public inode into its dax_dev
* @inode: An inode with i_cdev pointing to a dax_dev
diff --git a/include/linux/dax.h b/include/linux/dax.h
index 9fc5f99a0ae2..262d7bad131a 100644
--- a/include/linux/dax.h
+++ b/include/linux/dax.h
@@ -32,8 +32,24 @@ struct dax_operations {
int (*zero_page_range)(struct dax_device *, pgoff_t, size_t);
};
+struct dax_holder_operations {
+ /*
+ * notify_failure - notify memory failure into inner holder device
+ * @dax_dev: the dax device which contains the holder
+ * @offset: offset on this dax device where memory failure occurs
+ * @len: length of this memory failure event
+ * @flags: action flags for memory failure handler
+ */
+ int (*notify_failure)(struct dax_device *dax_dev, u64 offset,
+ u64 len, int mf_flags);
+};
+
#if IS_ENABLED(CONFIG_DAX)
struct dax_device *alloc_dax(void *private, const struct dax_operations *ops);
+int dax_register_holder(struct dax_device *dax_dev, void *holder,
+ const struct dax_holder_operations *ops);
+int dax_unregister_holder(struct dax_device *dax_dev, void *holder);
+void *dax_holder(struct dax_device *dax_dev);
void put_dax(struct dax_device *dax_dev);
void kill_dax(struct dax_device *dax_dev);
void dax_write_cache(struct dax_device *dax_dev, bool wc);
@@ -53,6 +69,20 @@ static inline bool daxdev_mapping_supported(struct vm_area_struct *vma,
return dax_synchronous(dax_dev);
}
#else
+static inline int dax_register_holder(struct dax_device *dax_dev, void *holder,
+ const struct dax_holder_operations *ops)
+{
+ return 0;
+}
+static inline int dax_unregister_holder(struct dax_device *dax_dev,
+ void *holder)
+{
+ return 0;
+}
+static inline void *dax_holder(struct dax_device *dax_dev)
+{
+ return NULL;
+}
static inline struct dax_device *alloc_dax(void *private,
const struct dax_operations *ops)
{
@@ -185,6 +215,8 @@ size_t dax_copy_to_iter(struct dax_device *dax_dev, pgoff_t pgoff, void *addr,
size_t bytes, struct iov_iter *i);
int dax_zero_page_range(struct dax_device *dax_dev, pgoff_t pgoff,
size_t nr_pages);
+int dax_holder_notify_failure(struct dax_device *dax_dev, u64 off, u64 len,
+ int mf_flags);
void dax_flush(struct dax_device *dax_dev, void *addr, size_t size);
ssize_t dax_iomap_rw(struct kiocb *iocb, struct iov_iter *iter,
--
2.35.1
Hi Shiyang,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on xfs-linux/for-next]
[also build test ERROR on linux/master]
[cannot apply to hnaz-mm/master linus/master v5.17-rc5 next-20220225]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Shiyang-Ruan/fsdax-introduce-fs-query-to-support-reflink/20220227-200849
base: https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git for-next
config: i386-randconfig-a003 (https://download.01.org/0day-ci/archive/20220227/[email protected]/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/9f4bfbd2bae60e9f172e0b7332b2af32aa5baa87
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Shiyang-Ruan/fsdax-introduce-fs-query-to-support-reflink/20220227-200849
git checkout 9f4bfbd2bae60e9f172e0b7332b2af32aa5baa87
# save the config file to linux build tree
mkdir build_dir
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
ld: fs/xfs/xfs_buf.o: in function `xfs_free_buftarg':
>> fs/xfs/xfs_buf.c:1897: undefined reference to `dax_unregister_holder'
ld: fs/xfs/xfs_notify_failure.o: in function `xfs_dax_notify_failure':
>> fs/xfs/xfs_notify_failure.c:187: undefined reference to `dax_holder'
vim +1897 fs/xfs/xfs_buf.c
1885
1886 void
1887 xfs_free_buftarg(
1888 struct xfs_buftarg *btp)
1889 {
1890 unregister_shrinker(&btp->bt_shrinker);
1891 ASSERT(percpu_counter_sum(&btp->bt_io_count) == 0);
1892 percpu_counter_destroy(&btp->bt_io_count);
1893 list_lru_destroy(&btp->bt_lru);
1894
1895 blkdev_issue_flush(btp->bt_bdev);
1896 if (btp->bt_daxdev)
> 1897 dax_unregister_holder(btp->bt_daxdev, btp->bt_mount);
1898 fs_put_dax(btp->bt_daxdev);
1899
1900 kmem_free(btp);
1901 }
1902
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Shiyang,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on xfs-linux/for-next]
[also build test ERROR on linux/master]
[cannot apply to hnaz-mm/master linus/master v5.17-rc5 next-20220225]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Shiyang-Ruan/fsdax-introduce-fs-query-to-support-reflink/20220227-200849
base: https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git for-next
config: hexagon-buildonly-randconfig-r005-20220227 (https://download.01.org/0day-ci/archive/20220227/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/9f4bfbd2bae60e9f172e0b7332b2af32aa5baa87
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Shiyang-Ruan/fsdax-introduce-fs-query-to-support-reflink/20220227-200849
git checkout 9f4bfbd2bae60e9f172e0b7332b2af32aa5baa87
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> ld.lld: error: undefined symbol: dax_unregister_holder
>>> referenced by xfs_buf.c
>>> xfs/xfs_buf.o:(xfs_free_buftarg) in archive fs/built-in.a
>>> referenced by xfs_buf.c
>>> xfs/xfs_buf.o:(xfs_free_buftarg) in archive fs/built-in.a
--
>> ld.lld: error: undefined symbol: dax_holder
>>> referenced by xfs_notify_failure.c
>>> xfs/xfs_notify_failure.o:(xfs_dax_notify_failure) in archive fs/built-in.a
>>> referenced by xfs_notify_failure.c
>>> xfs/xfs_notify_failure.o:(xfs_dax_notify_failure) in archive fs/built-in.a
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Shiyang,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on xfs-linux/for-next]
[also build test ERROR on linux/master]
[cannot apply to hnaz-mm/master linus/master v5.17-rc5 next-20220225]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Shiyang-Ruan/fsdax-introduce-fs-query-to-support-reflink/20220227-200849
base: https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git for-next
config: riscv-randconfig-p001-20220227 (https://download.01.org/0day-ci/archive/20220227/[email protected]/config)
compiler: riscv64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/9f4bfbd2bae60e9f172e0b7332b2af32aa5baa87
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Shiyang-Ruan/fsdax-introduce-fs-query-to-support-reflink/20220227-200849
git checkout 9f4bfbd2bae60e9f172e0b7332b2af32aa5baa87
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=riscv SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
riscv64-linux-ld: fs/xfs/xfs_buf.o: in function `.L828':
>> xfs_buf.c:(.text+0x3f7c): undefined reference to `dax_unregister_holder'
riscv64-linux-ld: fs/xfs/xfs_notify_failure.o: in function `xfs_dax_notify_failure':
>> xfs_notify_failure.c:(.text+0x2b0): undefined reference to `dax_holder'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Shiyang,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on xfs-linux/for-next]
[also build test WARNING on linux/master]
[cannot apply to hnaz-mm/master linus/master v5.17-rc5 next-20220225]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Shiyang-Ruan/fsdax-introduce-fs-query-to-support-reflink/20220227-200849
base: https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git for-next
config: i386-randconfig-a013 (https://download.01.org/0day-ci/archive/20220227/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/a0ac78065bbb4fbb3e5477c32686eca3b9f0e1ef
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Shiyang-Ruan/fsdax-introduce-fs-query-to-support-reflink/20220227-200849
git checkout a0ac78065bbb4fbb3e5477c32686eca3b9f0e1ef
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> fs/dax.c:337:67: warning: parameter 'mapping' set but not used [-Wunused-but-set-parameter]
static inline void dax_mapping_set_cow_flag(struct address_space *mapping)
^
1 warning generated.
vim +/mapping +337 fs/dax.c
328
329 /*
330 * Iterate through all mapped pfns represented by an entry, i.e. skip
331 * 'empty' and 'zero' entries.
332 */
333 #define for_each_mapped_pfn(entry, pfn) \
334 for (pfn = dax_to_pfn(entry); \
335 pfn < dax_end_pfn(entry); pfn++)
336
> 337 static inline void dax_mapping_set_cow_flag(struct address_space *mapping)
338 {
339 mapping = (struct address_space *)PAGE_MAPPING_DAX_COW;
340 }
341
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
ping ;)
在 2022/2/27 20:07, Shiyang Ruan 写道:
> This patchset is aimed to support shared pages tracking for fsdax.
>
> Changes since V10:
> - Use cmpxchg() to prevent concurrent registration/unregistration
> - Use phys_addr_t for ->memory_failure()
> - Add dax_entry_lock() for dax_lock_mapping_entry()
> - Fix offset and length calculation at the boundary of a filesystem
>
> This patchset moves owner tracking from dax_assocaite_entry() to pmem
> device driver, by introducing an interface ->memory_failure() for struct
> pagemap. This interface is called by memory_failure() in mm, and
> implemented by pmem device.
>
> Then call holder operations to find the filesystem which the corrupted
> data located in, and call filesystem handler to track files or metadata
> associated with this page.
>
> Finally we are able to try to fix the corrupted data in filesystem and
> do other necessary processing, such as killing processes who are using
> the files affected.
>
> The call trace is like this:
> memory_failure()
> |* fsdax case
> |------------
> |pgmap->ops->memory_failure() => pmem_pgmap_memory_failure()
> | dax_holder_notify_failure() =>
> | dax_device->holder_ops->notify_failure() =>
> | - xfs_dax_notify_failure()
> | |* xfs_dax_notify_failure()
> | |--------------------------
> | | xfs_rmap_query_range()
> | | xfs_dax_failure_fn()
> | | * corrupted on metadata
> | | try to recover data, call xfs_force_shutdown()
> | | * corrupted on file data
> | | try to recover data, call mf_dax_kill_procs()
> |* normal case
> |-------------
> |mf_generic_kill_procs()
>
> ==
> Shiyang Ruan (8):
> dax: Introduce holder for dax_device
> mm: factor helpers for memory_failure_dev_pagemap
> pagemap,pmem: Introduce ->memory_failure()
> fsdax: Introduce dax_lock_mapping_entry()
> mm: move pgoff_address() to vma_pgoff_address()
> mm: Introduce mf_dax_kill_procs() for fsdax case
> xfs: Implement ->notify_failure() for XFS
> fsdax: set a CoW flag when associate reflink mappings
>
> drivers/dax/super.c | 89 +++++++++++++
> drivers/nvdimm/pmem.c | 16 +++
> fs/dax.c | 140 ++++++++++++++++++---
> fs/xfs/Makefile | 1 +
> fs/xfs/xfs_buf.c | 12 ++
> fs/xfs/xfs_fsops.c | 3 +
> fs/xfs/xfs_mount.h | 1 +
> fs/xfs/xfs_notify_failure.c | 235 +++++++++++++++++++++++++++++++++++
> fs/xfs/xfs_notify_failure.h | 10 ++
> fs/xfs/xfs_super.c | 6 +
> include/linux/dax.h | 47 +++++++
> include/linux/memremap.h | 12 ++
> include/linux/mm.h | 17 +++
> include/linux/page-flags.h | 6 +
> mm/memory-failure.c | 240 ++++++++++++++++++++++++++----------
> 15 files changed, 747 insertions(+), 88 deletions(-)
> create mode 100644 fs/xfs/xfs_notify_failure.c
> create mode 100644 fs/xfs/xfs_notify_failure.h
>
On Sun, Feb 27, 2022 at 4:08 AM Shiyang Ruan <[email protected]> wrote:
>
> To easily track filesystem from a pmem device, we introduce a holder for
> dax_device structure, and also its operation. This holder is used to
> remember who is using this dax_device:
> - When it is the backend of a filesystem, the holder will be the
> instance of this filesystem.
> - When this pmem device is one of the targets in a mapped device, the
> holder will be this mapped device. In this case, the mapped device
> has its own dax_device and it will follow the first rule. So that we
> can finally track to the filesystem we needed.
>
> The holder and holder_ops will be set when filesystem is being mounted,
> or an target device is being activated.
>
> Signed-off-by: Shiyang Ruan <[email protected]>
> ---
> drivers/dax/super.c | 89 +++++++++++++++++++++++++++++++++++++++++++++
> include/linux/dax.h | 32 ++++++++++++++++
> 2 files changed, 121 insertions(+)
>
> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index e3029389d809..da5798e19d57 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c
> @@ -21,6 +21,9 @@
> * @cdev: optional character interface for "device dax"
> * @private: dax driver private data
> * @flags: state and boolean properties
> + * @ops: operations for dax_device
> + * @holder_data: holder of a dax_device: could be filesystem or mapped device
> + * @holder_ops: operations for the inner holder
> */
> struct dax_device {
> struct inode inode;
> @@ -28,6 +31,8 @@ struct dax_device {
> void *private;
> unsigned long flags;
> const struct dax_operations *ops;
> + void *holder_data;
> + const struct dax_holder_operations *holder_ops;
> };
>
> static dev_t dax_devt;
> @@ -193,6 +198,29 @@ int dax_zero_page_range(struct dax_device *dax_dev, pgoff_t pgoff,
> }
> EXPORT_SYMBOL_GPL(dax_zero_page_range);
>
> +int dax_holder_notify_failure(struct dax_device *dax_dev, u64 off,
> + u64 len, int mf_flags)
> +{
> + int rc, id;
> +
> + id = dax_read_lock();
> + if (!dax_alive(dax_dev)) {
> + rc = -ENXIO;
> + goto out;
> + }
> +
> + if (!dax_dev->holder_ops) {
> + rc = -EOPNOTSUPP;
I think it is ok to return success (0) for this case. All the caller
of dax_holder_notify_failure() wants to know is if the notification
was successfully delivered to the holder. If there is no holder
present then there is nothing to report. This is minor enough for me
to fix up locally if nothing else needs to be changed.
> + goto out;
> + }
> +
> + rc = dax_dev->holder_ops->notify_failure(dax_dev, off, len, mf_flags);
> +out:
> + dax_read_unlock(id);
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(dax_holder_notify_failure);
> +
> #ifdef CONFIG_ARCH_HAS_PMEM_API
> void arch_wb_cache_pmem(void *addr, size_t size);
> void dax_flush(struct dax_device *dax_dev, void *addr, size_t size)
> @@ -268,6 +296,10 @@ void kill_dax(struct dax_device *dax_dev)
>
> clear_bit(DAXDEV_ALIVE, &dax_dev->flags);
> synchronize_srcu(&dax_srcu);
> +
> + /* clear holder data */
> + dax_dev->holder_ops = NULL;
> + dax_dev->holder_data = NULL;
Isn't this another failure scenario? If kill_dax() is called while a
holder is still holding the dax_device that seems to be another
->notify_failure scenario to tell the holder that the device is going
away and the holder has not released the device yet.
> }
> EXPORT_SYMBOL_GPL(kill_dax);
>
> @@ -409,6 +441,63 @@ void put_dax(struct dax_device *dax_dev)
> }
> EXPORT_SYMBOL_GPL(put_dax);
>
> +/**
> + * dax_holder() - obtain the holder of a dax device
> + * @dax_dev: a dax_device instance
> +
> + * Return: the holder's data which represents the holder if registered,
> + * otherwize NULL.
> + */
> +void *dax_holder(struct dax_device *dax_dev)
> +{
> + if (!dax_alive(dax_dev))
> + return NULL;
It's safe for the holder to assume that it can de-reference
->holder_data freely in its notify_handler callback because
dax_holder_notify_failure() arranges for the callback to run in
dax_read_lock() context.
This is another minor detail that I can fixup locally.
> +
> + return dax_dev->holder_data;
> +}
> +EXPORT_SYMBOL_GPL(dax_holder);
> +
> +/**
> + * dax_register_holder() - register a holder to a dax device
> + * @dax_dev: a dax_device instance
> + * @holder: a pointer to a holder's data which represents the holder
> + * @ops: operations of this holder
> +
> + * Return: negative errno if an error occurs, otherwise 0.
> + */
> +int dax_register_holder(struct dax_device *dax_dev, void *holder,
> + const struct dax_holder_operations *ops)
> +{
> + if (!dax_alive(dax_dev))
> + return -ENXIO;
> +
> + if (cmpxchg(&dax_dev->holder_data, NULL, holder))
> + return -EBUSY;
> +
> + dax_dev->holder_ops = ops;
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(dax_register_holder);
> +
> +/**
> + * dax_unregister_holder() - unregister the holder for a dax device
> + * @dax_dev: a dax_device instance
> + * @holder: the holder to be unregistered
> + *
> + * Return: negative errno if an error occurs, otherwise 0.
> + */
> +int dax_unregister_holder(struct dax_device *dax_dev, void *holder)
> +{
> + if (!dax_alive(dax_dev))
> + return -ENXIO;
> +
> + if (cmpxchg(&dax_dev->holder_data, holder, NULL) != holder)
> + return -EBUSY;
> + dax_dev->holder_ops = NULL;
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(dax_unregister_holder);
> +
> /**
> * inode_dax: convert a public inode into its dax_dev
> * @inode: An inode with i_cdev pointing to a dax_dev
> diff --git a/include/linux/dax.h b/include/linux/dax.h
> index 9fc5f99a0ae2..262d7bad131a 100644
> --- a/include/linux/dax.h
> +++ b/include/linux/dax.h
> @@ -32,8 +32,24 @@ struct dax_operations {
> int (*zero_page_range)(struct dax_device *, pgoff_t, size_t);
> };
>
> +struct dax_holder_operations {
> + /*
> + * notify_failure - notify memory failure into inner holder device
> + * @dax_dev: the dax device which contains the holder
> + * @offset: offset on this dax device where memory failure occurs
> + * @len: length of this memory failure event
Forgive me if this has been discussed before, but since dax_operations
are in terms of pgoff and nr pages and memory_failure() is in terms of
pfns what was the rationale for making the function signature byte
based?
I want to get this series merged into linux-next shortly after
v5.18-rc1. Then we can start working on incremental fixups rather
resending the full series with these long reply cycles.
在 2022/3/12 7:35, Dan Williams 写道:
> On Sun, Feb 27, 2022 at 4:08 AM Shiyang Ruan <[email protected]> wrote:
>>
>> To easily track filesystem from a pmem device, we introduce a holder for
>> dax_device structure, and also its operation. This holder is used to
>> remember who is using this dax_device:
>> - When it is the backend of a filesystem, the holder will be the
>> instance of this filesystem.
>> - When this pmem device is one of the targets in a mapped device, the
>> holder will be this mapped device. In this case, the mapped device
>> has its own dax_device and it will follow the first rule. So that we
>> can finally track to the filesystem we needed.
>>
>> The holder and holder_ops will be set when filesystem is being mounted,
>> or an target device is being activated.
>>
>> Signed-off-by: Shiyang Ruan <[email protected]>
>> ---
>> drivers/dax/super.c | 89 +++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/dax.h | 32 ++++++++++++++++
>> 2 files changed, 121 insertions(+)
>>
>> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
>> index e3029389d809..da5798e19d57 100644
>> --- a/drivers/dax/super.c
>> +++ b/drivers/dax/super.c
>> @@ -21,6 +21,9 @@
>> * @cdev: optional character interface for "device dax"
>> * @private: dax driver private data
>> * @flags: state and boolean properties
>> + * @ops: operations for dax_device
>> + * @holder_data: holder of a dax_device: could be filesystem or mapped device
>> + * @holder_ops: operations for the inner holder
>> */
>> struct dax_device {
>> struct inode inode;
>> @@ -28,6 +31,8 @@ struct dax_device {
>> void *private;
>> unsigned long flags;
>> const struct dax_operations *ops;
>> + void *holder_data;
>> + const struct dax_holder_operations *holder_ops;
>> };
>>
>> static dev_t dax_devt;
>> @@ -193,6 +198,29 @@ int dax_zero_page_range(struct dax_device *dax_dev, pgoff_t pgoff,
>> }
>> EXPORT_SYMBOL_GPL(dax_zero_page_range);
>>
>> +int dax_holder_notify_failure(struct dax_device *dax_dev, u64 off,
>> + u64 len, int mf_flags)
>> +{
>> + int rc, id;
>> +
>> + id = dax_read_lock();
>> + if (!dax_alive(dax_dev)) {
>> + rc = -ENXIO;
>> + goto out;
>> + }
>> +
>> + if (!dax_dev->holder_ops) {
>> + rc = -EOPNOTSUPP;
>
> I think it is ok to return success (0) for this case. All the caller
> of dax_holder_notify_failure() wants to know is if the notification
> was successfully delivered to the holder. If there is no holder
> present then there is nothing to report. This is minor enough for me
> to fix up locally if nothing else needs to be changed.
I thought it could fall back to generic memory failure handler:
mf_generic_kill_procs(), if holder_ops not exists.
>
>> + goto out;
>> + }
>> +
>> + rc = dax_dev->holder_ops->notify_failure(dax_dev, off, len, mf_flags);
>> +out:
>> + dax_read_unlock(id);
>> + return rc;
>> +}
>> +EXPORT_SYMBOL_GPL(dax_holder_notify_failure);
>> +
>> #ifdef CONFIG_ARCH_HAS_PMEM_API
>> void arch_wb_cache_pmem(void *addr, size_t size);
>> void dax_flush(struct dax_device *dax_dev, void *addr, size_t size)
>> @@ -268,6 +296,10 @@ void kill_dax(struct dax_device *dax_dev)
>>
>> clear_bit(DAXDEV_ALIVE, &dax_dev->flags);
>> synchronize_srcu(&dax_srcu);
>> +
>> + /* clear holder data */
>> + dax_dev->holder_ops = NULL;
>> + dax_dev->holder_data = NULL;
>
> Isn't this another failure scenario? If kill_dax() is called while a
> holder is still holding the dax_device that seems to be another
> ->notify_failure scenario to tell the holder that the device is going
> away and the holder has not released the device yet.
Yes. I should call dax_holder_notify_failure() and then unregister the
holder.
>
>> }
>> EXPORT_SYMBOL_GPL(kill_dax);
>>
>> @@ -409,6 +441,63 @@ void put_dax(struct dax_device *dax_dev)
>> }
>> EXPORT_SYMBOL_GPL(put_dax);
>>
>> +/**
>> + * dax_holder() - obtain the holder of a dax device
>> + * @dax_dev: a dax_device instance
>> +
>> + * Return: the holder's data which represents the holder if registered,
>> + * otherwize NULL.
>> + */
>> +void *dax_holder(struct dax_device *dax_dev)
>> +{
>> + if (!dax_alive(dax_dev))
>> + return NULL;
>
> It's safe for the holder to assume that it can de-reference
> ->holder_data freely in its notify_handler callback because
> dax_holder_notify_failure() arranges for the callback to run in
> dax_read_lock() context.
>
> This is another minor detail that I can fixup locally.
>
>> +
>> + return dax_dev->holder_data;
>> +}
>> +EXPORT_SYMBOL_GPL(dax_holder);
>> +
>> +/**
>> + * dax_register_holder() - register a holder to a dax device
>> + * @dax_dev: a dax_device instance
>> + * @holder: a pointer to a holder's data which represents the holder
>> + * @ops: operations of this holder
>> +
>> + * Return: negative errno if an error occurs, otherwise 0.
>> + */
>> +int dax_register_holder(struct dax_device *dax_dev, void *holder,
>> + const struct dax_holder_operations *ops)
>> +{
>> + if (!dax_alive(dax_dev))
>> + return -ENXIO;
>> +
>> + if (cmpxchg(&dax_dev->holder_data, NULL, holder))
>> + return -EBUSY;
>> +
>> + dax_dev->holder_ops = ops;
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(dax_register_holder);
>> +
>> +/**
>> + * dax_unregister_holder() - unregister the holder for a dax device
>> + * @dax_dev: a dax_device instance
>> + * @holder: the holder to be unregistered
>> + *
>> + * Return: negative errno if an error occurs, otherwise 0.
>> + */
>> +int dax_unregister_holder(struct dax_device *dax_dev, void *holder)
>> +{
>> + if (!dax_alive(dax_dev))
>> + return -ENXIO;
>> +
>> + if (cmpxchg(&dax_dev->holder_data, holder, NULL) != holder)
>> + return -EBUSY;
>> + dax_dev->holder_ops = NULL;
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(dax_unregister_holder);
>> +
>> /**
>> * inode_dax: convert a public inode into its dax_dev
>> * @inode: An inode with i_cdev pointing to a dax_dev
>> diff --git a/include/linux/dax.h b/include/linux/dax.h
>> index 9fc5f99a0ae2..262d7bad131a 100644
>> --- a/include/linux/dax.h
>> +++ b/include/linux/dax.h
>> @@ -32,8 +32,24 @@ struct dax_operations {
>> int (*zero_page_range)(struct dax_device *, pgoff_t, size_t);
>> };
>>
>> +struct dax_holder_operations {
>> + /*
>> + * notify_failure - notify memory failure into inner holder device
>> + * @dax_dev: the dax device which contains the holder
>> + * @offset: offset on this dax device where memory failure occurs
>> + * @len: length of this memory failure event
>
> Forgive me if this has been discussed before, but since dax_operations
> are in terms of pgoff and nr pages and memory_failure() is in terms of
> pfns what was the rationale for making the function signature byte
> based?
Maybe I didn't describe it clearly... The @offset and @len here are
byte-based. And so is ->memory_failure().
You can find the implementation of ->memory_failure() in 3rd patch:
+static int pmem_pagemap_memory_failure(struct dev_pagemap *pgmap,
+ phys_addr_t addr, u64 len, int mf_flags)
+{
+ struct pmem_device *pmem =
+ container_of(pgmap, struct pmem_device, pgmap);
+ u64 offset = addr - pmem->phys_addr - pmem->data_offset;
+
+ return dax_holder_notify_failure(pmem->dax_dev, offset, len, mf_flags);
+}
>
> I want to get this series merged into linux-next shortly after
> v5.18-rc1. Then we can start working on incremental fixups rather
> resending the full series with these long reply cycles.
Thanks. That really helps.
--
Ruan.
> @@ -1892,6 +1893,8 @@ xfs_free_buftarg(
> list_lru_destroy(&btp->bt_lru);
>
> blkdev_issue_flush(btp->bt_bdev);
> + if (btp->bt_daxdev)
> + dax_unregister_holder(btp->bt_daxdev, btp->bt_mount);
> fs_put_dax(btp->bt_daxdev);
>
> kmem_free(btp);
> @@ -1939,6 +1942,7 @@ xfs_alloc_buftarg(
> struct block_device *bdev)
> {
> xfs_buftarg_t *btp;
> + int error;
>
> btp = kmem_zalloc(sizeof(*btp), KM_NOFS);
>
> @@ -1946,6 +1950,14 @@ xfs_alloc_buftarg(
> btp->bt_dev = bdev->bd_dev;
> btp->bt_bdev = bdev;
> btp->bt_daxdev = fs_dax_get_by_bdev(bdev, &btp->bt_dax_part_off);
> + if (btp->bt_daxdev) {
> + error = dax_register_holder(btp->bt_daxdev, mp,
> + &xfs_dax_holder_operations);
> + if (error) {
> + xfs_err(mp, "DAX device already in use?!");
> + goto error_free;
> + }
> + }
It seems to me that just passing the holder and holder ops to
fs_dax_get_by_bdev and the holder to dax_unregister_holder would
significantly simply the interface here.
Dan, what do you think?
> +#if IS_ENABLED(CONFIG_MEMORY_FAILURE) && IS_ENABLED(CONFIG_FS_DAX)
No real need for the IS_ENABLED. Also any reason to even build this
file if the options are not set? It seems like
xfs_dax_holder_operations should just be defined to NULL and the
whole file not supported if we can't support the functionality.
Dan: not for this series, but is there any reason not to require
MEMORY_FAILURE for DAX to start with?
> +
> + ddev_start = mp->m_ddev_targp->bt_dax_part_off;
> + ddev_end = ddev_start +
> + (mp->m_ddev_targp->bt_bdev->bd_nr_sectors << SECTOR_SHIFT) - 1;
This should use bdev_nr_bytes.
But didn't we say we don't want to support notifications on partitioned
devices and thus don't actually need all this?
> +
> + /* Ignore the range out of filesystem area */
> + if ((offset + len) < ddev_start)
No need for the inner braces.
> + if ((offset + len) > ddev_end)
No need for the braces either.
> diff --git a/fs/xfs/xfs_notify_failure.h b/fs/xfs/xfs_notify_failure.h
> new file mode 100644
> index 000000000000..76187b9620f9
> --- /dev/null
> +++ b/fs/xfs/xfs_notify_failure.h
> @@ -0,0 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 Fujitsu. All Rights Reserved.
> + */
> +#ifndef __XFS_NOTIFY_FAILURE_H__
> +#define __XFS_NOTIFY_FAILURE_H__
> +
> +extern const struct dax_holder_operations xfs_dax_holder_operations;
> +
> +#endif /* __XFS_NOTIFY_FAILURE_H__ */
Dowe really need a new header for this vs just sequeezing it into
xfs_super.h or something like that?
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index e8f37bdc8354..b8de6ed2c888 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -353,6 +353,12 @@ xfs_setup_dax_always(
> return -EINVAL;
> }
>
> + if (xfs_has_reflink(mp) && !xfs_has_rmapbt(mp)) {
> + xfs_alert(mp,
> + "need rmapbt when both DAX and reflink enabled.");
> + return -EINVAL;
> + }
Right now we can't even enable reflink with DAX yet, so adding this
here seems premature - it should go into the patch allowing DAX+reflink.
在 2022/3/30 18:13, Christoph Hellwig 写道:
> On Wed, Mar 30, 2022 at 06:03:01PM +0800, Shiyang Ruan wrote:
>>
>> Because I am not sure if the offset between each layer is page aligned. For
>> example, when pmem dirver handles ->memory_failure(), it should subtract its
>> ->data_offset when it calls dax_holder_notify_failure().
>
> If they aren't, none of the DAX machinery would work.
OK. Got it.
So, use page-based function signature for ->memory_failure():
int (*memory_failure)(struct dev_pagemap *pgmap, unsigned long pfn,
unsigned long nr_pfns, int flags);
As the code I pasted before, pmem driver will subtract its
->data_offset, which is byte-based. And the filesystem who implements
->notify_failure() will calculate the offset in unit of byte again.
So, leave its function signature byte-based, to avoid repeated conversions.
int (*notify_failure)(struct dax_device *dax_dev, u64 offset,
u64 len, int mf_flags);
What do you think?
--
Thanks,
Ruan.
在 2022/3/30 13:41, Christoph Hellwig 写道:
> On Wed, Mar 16, 2022 at 09:46:07PM +0800, Shiyang Ruan wrote:
>>> Forgive me if this has been discussed before, but since dax_operations
>>> are in terms of pgoff and nr pages and memory_failure() is in terms of
>>> pfns what was the rationale for making the function signature byte
>>> based?
>>
>> Maybe I didn't describe it clearly... The @offset and @len here are
>> byte-based. And so is ->memory_failure().
>
> Yes, but is there a good reason for that when the rest of the DAX code
> tends to work in page chunks?
Because I am not sure if the offset between each layer is page aligned.
For example, when pmem dirver handles ->memory_failure(), it should
subtract its ->data_offset when it calls dax_holder_notify_failure().
The implementation of ->memory_failure() by pmem driver:
+static int pmem_pagemap_memory_failure(struct dev_pagemap *pgmap,
+ phys_addr_t addr, u64 len, int mf_flags)
+{
+ struct pmem_device *pmem =
+ container_of(pgmap, struct pmem_device, pgmap);
+ u64 offset = addr - pmem->phys_addr - pmem->data_offset;
+
+ return dax_holder_notify_failure(pmem->dax_dev, offset, len, mf_flags);
+}
So, I choose u64 as the type of @len. And for consistency, the @addr is
using byte-based type as well.
> memory_failure()
> |* fsdax case
> |------------
> |pgmap->ops->memory_failure() => pmem_pgmap_memory_failure()
> | dax_holder_notify_failure() =>
the offset from 'pmem driver' to 'dax holder'
> | dax_device->holder_ops->notify_failure() =>
> | - xfs_dax_notify_failure()
> | |* xfs_dax_notify_failure()
> | |--------------------------
> | | xfs_rmap_query_range()
> | | xfs_dax_failure_fn()
> | | * corrupted on metadata
> | | try to recover data, call xfs_force_shutdown()
> | | * corrupted on file data
> | | try to recover data, call mf_dax_kill_procs()
> |* normal case
> |-------------
> |mf_generic_kill_procs()
--
Thanks,
Ruan.
在 2022/3/30 14:00, Christoph Hellwig 写道:
>> @@ -1892,6 +1893,8 @@ xfs_free_buftarg(
>> list_lru_destroy(&btp->bt_lru);
>>
>> blkdev_issue_flush(btp->bt_bdev);
>> + if (btp->bt_daxdev)
>> + dax_unregister_holder(btp->bt_daxdev, btp->bt_mount);
>> fs_put_dax(btp->bt_daxdev);
>>
>> kmem_free(btp);
>> @@ -1939,6 +1942,7 @@ xfs_alloc_buftarg(
>> struct block_device *bdev)
>> {
>> xfs_buftarg_t *btp;
>> + int error;
>>
>> btp = kmem_zalloc(sizeof(*btp), KM_NOFS);
>>
>> @@ -1946,6 +1950,14 @@ xfs_alloc_buftarg(
>> btp->bt_dev = bdev->bd_dev;
>> btp->bt_bdev = bdev;
>> btp->bt_daxdev = fs_dax_get_by_bdev(bdev, &btp->bt_dax_part_off);
>> + if (btp->bt_daxdev) {
>> + error = dax_register_holder(btp->bt_daxdev, mp,
>> + &xfs_dax_holder_operations);
>> + if (error) {
>> + xfs_err(mp, "DAX device already in use?!");
>> + goto error_free;
>> + }
>> + }
>
> It seems to me that just passing the holder and holder ops to
> fs_dax_get_by_bdev and the holder to dax_unregister_holder would
> significantly simply the interface here.
>
> Dan, what do you think?
>
>> +#if IS_ENABLED(CONFIG_MEMORY_FAILURE) && IS_ENABLED(CONFIG_FS_DAX)
>
> No real need for the IS_ENABLED. Also any reason to even build this
> file if the options are not set? It seems like
> xfs_dax_holder_operations should just be defined to NULL and the
> whole file not supported if we can't support the functionality.
Got it. These two CONFIG seem not related for now. So, I think I
should wrap these code with #ifdef CONFIG_MEMORY_FAILURE here, and add
`xfs-$(CONFIG_FS_DAX) += xfs_notify_failure.o` in the makefile.
>
> Dan: not for this series, but is there any reason not to require
> MEMORY_FAILURE for DAX to start with?
>
>> +
>> + ddev_start = mp->m_ddev_targp->bt_dax_part_off;
>> + ddev_end = ddev_start +
>> + (mp->m_ddev_targp->bt_bdev->bd_nr_sectors << SECTOR_SHIFT) - 1;
>
> This should use bdev_nr_bytes.
OK.
>
> But didn't we say we don't want to support notifications on partitioned
> devices and thus don't actually need all this?
>
>> +
>> + /* Ignore the range out of filesystem area */
>> + if ((offset + len) < ddev_start)
>
> No need for the inner braces.
>
>> + if ((offset + len) > ddev_end)
>
> No need for the braces either.
Really no need? It is to make sure the range to be handled won't out of
the filesystem area. And make sure the @offset and @len are valid and
correct after subtract the bbdev_start.
>
>> diff --git a/fs/xfs/xfs_notify_failure.h b/fs/xfs/xfs_notify_failure.h
>> new file mode 100644
>> index 000000000000..76187b9620f9
>> --- /dev/null
>> +++ b/fs/xfs/xfs_notify_failure.h
>> @@ -0,0 +1,10 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2022 Fujitsu. All Rights Reserved.
>> + */
>> +#ifndef __XFS_NOTIFY_FAILURE_H__
>> +#define __XFS_NOTIFY_FAILURE_H__
>> +
>> +extern const struct dax_holder_operations xfs_dax_holder_operations;
>> +
>> +#endif /* __XFS_NOTIFY_FAILURE_H__ */
>
> Dowe really need a new header for this vs just sequeezing it into
> xfs_super.h or something like that?
Yes, I'll move it into xfs_super.h. The xfs_notify_failure.c was
splitted from xfs_super.c in the old patch. There is no need to create
a header file for only single line of code.
>
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index e8f37bdc8354..b8de6ed2c888 100644
>> --- a/fs/xfs/xfs_super.c
>> +++ b/fs/xfs/xfs_super.c
>> @@ -353,6 +353,12 @@ xfs_setup_dax_always(
>> return -EINVAL;
>> }
>>
>> + if (xfs_has_reflink(mp) && !xfs_has_rmapbt(mp)) {
>> + xfs_alert(mp,
>> + "need rmapbt when both DAX and reflink enabled.");
>> + return -EINVAL;
>> + }
>
> Right now we can't even enable reflink with DAX yet, so adding this
> here seems premature - it should go into the patch allowing DAX+reflink.
>
Yes. I'll remove it for now.
--
Thanks,
Ruan.
On Wed, Mar 30, 2022 at 06:03:01PM +0800, Shiyang Ruan wrote:
>
> Because I am not sure if the offset between each layer is page aligned. For
> example, when pmem dirver handles ->memory_failure(), it should subtract its
> ->data_offset when it calls dax_holder_notify_failure().
If they aren't, none of the DAX machinery would work.
On Wed, Mar 30, 2022 at 06:58:21PM +0800, Shiyang Ruan wrote:
> As the code I pasted before, pmem driver will subtract its ->data_offset,
> which is byte-based. And the filesystem who implements ->notify_failure()
> will calculate the offset in unit of byte again.
>
> So, leave its function signature byte-based, to avoid repeated conversions.
I'm actually fine either way, so I'll wait for Dan to comment.
On Wed, Mar 30, 2022 at 08:49:29AM -0700, Christoph Hellwig wrote:
> On Wed, Mar 30, 2022 at 06:58:21PM +0800, Shiyang Ruan wrote:
> > As the code I pasted before, pmem driver will subtract its ->data_offset,
> > which is byte-based. And the filesystem who implements ->notify_failure()
> > will calculate the offset in unit of byte again.
> >
> > So, leave its function signature byte-based, to avoid repeated conversions.
>
> I'm actually fine either way, so I'll wait for Dan to comment.
FWIW I'd convinced myself that the reason for using byte units is to
make it possible to reduce the pmem failure blast radius to subpage
units... but then I've also been distracted for months. :/
--D
在 2022/3/30 13:46, Christoph Hellwig 写道:
> On Sun, Feb 27, 2022 at 08:07:44PM +0800, Shiyang Ruan wrote:
>> Since it is not a DAX-specific function, move it into mm and rename it
>> to be a generic helper.
>
> FYI, there is a patch in -mm and linux-next:
>
> "mm: rmap: introduce pfn_mkclean_range() to cleans PTEs"
>
> that adds a vma_pgoff_address which seems like a bit of a superset of
> the one added in this patch, but only is in mm/internal.h.
Yes. The function in this patch handles more cases.
So, let me rebase onto this patch and move the function into
/include/linux/mm.h, so that fs/dax.c can also use it. Is this ok?
--
Thanks,
Ruan.
On Sun, Feb 27, 2022 at 08:07:45PM +0800, Shiyang Ruan wrote:
> This function is called at the end of RMAP routine, i.e. filesystem
> recovery function, to collect and kill processes using a shared page of
> DAX file.
I think just throwing RMAP inhere is rather confusing.
> The difference with mf_generic_kill_procs() is, it accepts
> file's (mapping,offset) instead of struct page because different files'
> mappings and offsets may share the same page in fsdax mode.
> It will be called when filesystem's RMAP results are found.
So maybe I'd word the whole log as something like:
This new function is a variant of mf_generic_kill_procs that accepts
a file, offset pair instead o a struct to support multiple files sharing
a DAX mapping. It is intended to be called by the file systems as
part of the memory_failure handler after the file system performed
a reverse mapping from the storage address to the file and file offset.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
> index 9b1d56c5c224..0420189e4788 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3195,6 +3195,10 @@ enum mf_flags {
> MF_SOFT_OFFLINE = 1 << 3,
> MF_UNPOISON = 1 << 4,
> };
> +#if IS_ENABLED(CONFIG_FS_DAX)
> +int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
> + unsigned long count, int mf_flags);
> +#endif /* CONFIG_FS_DAX */
No need for the ifdef here, having the stable declaration around is
just fine.
> +#if IS_ENABLED(CONFIG_FS_DAX)
No need for the IS_ENABLED as CONFIG_FS_DAX can't be modular.
A good old #ifdef will do it.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
On Wed, Mar 16, 2022 at 09:46:07PM +0800, Shiyang Ruan wrote:
> > Forgive me if this has been discussed before, but since dax_operations
> > are in terms of pgoff and nr pages and memory_failure() is in terms of
> > pfns what was the rationale for making the function signature byte
> > based?
>
> Maybe I didn't describe it clearly... The @offset and @len here are
> byte-based. And so is ->memory_failure().
Yes, but is there a good reason for that when the rest of the DAX code
tends to work in page chunks?
On Sun, Feb 27, 2022 at 08:07:44PM +0800, Shiyang Ruan wrote:
> Since it is not a DAX-specific function, move it into mm and rename it
> to be a generic helper.
FYI, there is a patch in -mm and linux-next:
"mm: rmap: introduce pfn_mkclean_range() to cleans PTEs"
that adds a vma_pgoff_address which seems like a bit of a superset of
the one added in this patch, but only is in mm/internal.h.
On Fri, Mar 11, 2022 at 03:35:13PM -0800, Dan Williams wrote:
> > + if (!dax_dev->holder_ops) {
> > + rc = -EOPNOTSUPP;
>
> I think it is ok to return success (0) for this case. All the caller
> of dax_holder_notify_failure() wants to know is if the notification
> was successfully delivered to the holder. If there is no holder
> present then there is nothing to report. This is minor enough for me
> to fix up locally if nothing else needs to be changed.
The caller needs to know there are no holder ops to fall back to
different path.
> Isn't this another failure scenario? If kill_dax() is called while a
> holder is still holding the dax_device that seems to be another
> ->notify_failure scenario to tell the holder that the device is going
> away and the holder has not released the device yet.
Yes.
On Wed, Mar 30, 2022 at 11:16:10PM +0800, Shiyang Ruan wrote:
> > > +#if IS_ENABLED(CONFIG_MEMORY_FAILURE) && IS_ENABLED(CONFIG_FS_DAX)
> >
> > No real need for the IS_ENABLED. Also any reason to even build this
> > file if the options are not set? It seems like
> > xfs_dax_holder_operations should just be defined to NULL and the
> > whole file not supported if we can't support the functionality.
>
> Got it. These two CONFIG seem not related for now. So, I think I should
> wrap these code with #ifdef CONFIG_MEMORY_FAILURE here, and add
> `xfs-$(CONFIG_FS_DAX) += xfs_notify_failure.o` in the makefile.
I'd do
ifeq ($(CONFIG_MEMORY_FAILURE),y)
xfs-$(CONFIG_FS_DAX) += xfs_notify_failure.o
endif
in the makefile and keep it out of the actual source file entirely.
> > > +
> > > + /* Ignore the range out of filesystem area */
> > > + if ((offset + len) < ddev_start)
> >
> > No need for the inner braces.
> >
> > > + if ((offset + len) > ddev_end)
> >
> > No need for the braces either.
>
> Really no need? It is to make sure the range to be handled won't out of the
> filesystem area. And make sure the @offset and @len are valid and correct
> after subtract the bbdev_start.
Yes, but there is no need for the braces per the precedence rules in
C. So these could be:
if (offset + len < ddev_start)
and
if (offset + len > ddev_end)
On 3/30/2022 9:18 AM, Darrick J. Wong wrote:
> On Wed, Mar 30, 2022 at 08:49:29AM -0700, Christoph Hellwig wrote:
>> On Wed, Mar 30, 2022 at 06:58:21PM +0800, Shiyang Ruan wrote:
>>> As the code I pasted before, pmem driver will subtract its ->data_offset,
>>> which is byte-based. And the filesystem who implements ->notify_failure()
>>> will calculate the offset in unit of byte again.
>>>
>>> So, leave its function signature byte-based, to avoid repeated conversions.
>>
>> I'm actually fine either way, so I'll wait for Dan to comment.
>
> FWIW I'd convinced myself that the reason for using byte units is to
> make it possible to reduce the pmem failure blast radius to subpage
> units... but then I've also been distracted for months. :/
>
Yes, thanks Darrick! I recall that.
Maybe just add a comment about why byte unit is used?
thanks!
-jane
> --D
On Tue, Apr 5, 2022 at 5:55 PM Jane Chu <[email protected]> wrote:
>
> On 3/30/2022 9:18 AM, Darrick J. Wong wrote:
> > On Wed, Mar 30, 2022 at 08:49:29AM -0700, Christoph Hellwig wrote:
> >> On Wed, Mar 30, 2022 at 06:58:21PM +0800, Shiyang Ruan wrote:
> >>> As the code I pasted before, pmem driver will subtract its ->data_offset,
> >>> which is byte-based. And the filesystem who implements ->notify_failure()
> >>> will calculate the offset in unit of byte again.
> >>>
> >>> So, leave its function signature byte-based, to avoid repeated conversions.
> >>
> >> I'm actually fine either way, so I'll wait for Dan to comment.
> >
> > FWIW I'd convinced myself that the reason for using byte units is to
> > make it possible to reduce the pmem failure blast radius to subpage
> > units... but then I've also been distracted for months. :/
> >
>
> Yes, thanks Darrick! I recall that.
> Maybe just add a comment about why byte unit is used?
I think we start with page failure notification and then figure out
how to get finer grained through the dax interface in follow-on
changes. Otherwise, for finer grained error handling support,
memory_failure() would also need to be converted to stop upcasting
cache-line granularity to page granularity failures. The native MCE
notification communicates a 'struct mce' that can be in terms of
sub-page bytes, but the memory management implications are all page
based. I assume the FS implications are all FS-block-size based?
On Tue, Apr 05, 2022 at 06:22:48PM -0700, Dan Williams wrote:
> On Tue, Apr 5, 2022 at 5:55 PM Jane Chu <[email protected]> wrote:
> >
> > On 3/30/2022 9:18 AM, Darrick J. Wong wrote:
> > > On Wed, Mar 30, 2022 at 08:49:29AM -0700, Christoph Hellwig wrote:
> > >> On Wed, Mar 30, 2022 at 06:58:21PM +0800, Shiyang Ruan wrote:
> > >>> As the code I pasted before, pmem driver will subtract its ->data_offset,
> > >>> which is byte-based. And the filesystem who implements ->notify_failure()
> > >>> will calculate the offset in unit of byte again.
> > >>>
> > >>> So, leave its function signature byte-based, to avoid repeated conversions.
> > >>
> > >> I'm actually fine either way, so I'll wait for Dan to comment.
> > >
> > > FWIW I'd convinced myself that the reason for using byte units is to
> > > make it possible to reduce the pmem failure blast radius to subpage
> > > units... but then I've also been distracted for months. :/
> > >
> >
> > Yes, thanks Darrick! I recall that.
> > Maybe just add a comment about why byte unit is used?
>
> I think we start with page failure notification and then figure out
> how to get finer grained through the dax interface in follow-on
> changes. Otherwise, for finer grained error handling support,
> memory_failure() would also need to be converted to stop upcasting
> cache-line granularity to page granularity failures. The native MCE
> notification communicates a 'struct mce' that can be in terms of
> sub-page bytes, but the memory management implications are all page
> based. I assume the FS implications are all FS-block-size based?
I wouldn't necessarily make that assumption -- for regular files, the
user program is in a better position to figure out how to reset the file
contents.
For fs metadata, it really depends. In principle, if (say) we could get
byte granularity poison info, we could look up the space usage within
the block to decide if the poisoned part was actually free space, in
which case we can correct the problem by (re)zeroing the affected bytes
to clear the poison.
Obviously, if the blast radius hits the internal space info or something
that was storing useful data, then you'd have to rebuild the whole block
(or the whole data structure), but that's not necessarily a given.
--D
[ add Mauro and Tony for RAS discussion ]
On Wed, Apr 6, 2022 at 1:39 PM Darrick J. Wong <[email protected]> wrote:
>
> On Tue, Apr 05, 2022 at 06:22:48PM -0700, Dan Williams wrote:
> > On Tue, Apr 5, 2022 at 5:55 PM Jane Chu <[email protected]> wrote:
> > >
> > > On 3/30/2022 9:18 AM, Darrick J. Wong wrote:
> > > > On Wed, Mar 30, 2022 at 08:49:29AM -0700, Christoph Hellwig wrote:
> > > >> On Wed, Mar 30, 2022 at 06:58:21PM +0800, Shiyang Ruan wrote:
> > > >>> As the code I pasted before, pmem driver will subtract its ->data_offset,
> > > >>> which is byte-based. And the filesystem who implements ->notify_failure()
> > > >>> will calculate the offset in unit of byte again.
> > > >>>
> > > >>> So, leave its function signature byte-based, to avoid repeated conversions.
> > > >>
> > > >> I'm actually fine either way, so I'll wait for Dan to comment.
> > > >
> > > > FWIW I'd convinced myself that the reason for using byte units is to
> > > > make it possible to reduce the pmem failure blast radius to subpage
> > > > units... but then I've also been distracted for months. :/
> > > >
> > >
> > > Yes, thanks Darrick! I recall that.
> > > Maybe just add a comment about why byte unit is used?
> >
> > I think we start with page failure notification and then figure out
> > how to get finer grained through the dax interface in follow-on
> > changes. Otherwise, for finer grained error handling support,
> > memory_failure() would also need to be converted to stop upcasting
> > cache-line granularity to page granularity failures. The native MCE
> > notification communicates a 'struct mce' that can be in terms of
> > sub-page bytes, but the memory management implications are all page
> > based. I assume the FS implications are all FS-block-size based?
>
> I wouldn't necessarily make that assumption -- for regular files, the
> user program is in a better position to figure out how to reset the file
> contents.
>
> For fs metadata, it really depends. In principle, if (say) we could get
> byte granularity poison info, we could look up the space usage within
> the block to decide if the poisoned part was actually free space, in
> which case we can correct the problem by (re)zeroing the affected bytes
> to clear the poison.
>
> Obviously, if the blast radius hits the internal space info or something
> that was storing useful data, then you'd have to rebuild the whole block
> (or the whole data structure), but that's not necessarily a given.
tl;dr: dax_holder_notify_failure() != fs->notify_failure()
So I think I see some confusion between what DAX->notify_failure()
needs, memory_failure() needs, the raw information provided by the
hardware, and the failure granularity the filesystem can make use of.
DAX and memory_failure() need to make immediate page granularity
decisions. They both need to map out whole pages (in the direct map
and userspace respectively) to prevent future poison consumption, at
least until the poison is repaired.
The event that leads to a page being failed can be triggered by a
hardware error as small as an individual cacheline. While that is
interesting to a filesystem it isn't information that memory_failure()
and DAX can utilize.
The reason DAX needs to have a callback into filesystem code is to map
the page failure back to all the processes that might have that page
mapped because reflink means that page->mapping is not sufficient to
find all the affected 'struct address_space' instances. So it's more
of an address-translation / "help me kill processes" service than a
general failure notification service.
Currently when raw hardware event happens there are mechanisms like
arch-specific notifier chains, like powerpc::mce_register_notifier()
and x86::mce_register_decode_chain(), or other platform firmware code
like ghes_edac_report_mem_error() that uplevel the error to a coarse
page granularity failure, while emitting the fine granularity error
event to userspace.
All of this to say that the interface to ask the fs to do the bottom
half of memory_failure() (walking affected 'struct address_space'
instances and killing processes (mf_dax_kill_procs())) is different
than the general interface to tell the filesystem that memory has gone
bad relative to a device. So if the only caller of
fs->notify_failure() handler is this code:
+ if (pgmap->ops->memory_failure) {
+ rc = pgmap->ops->memory_failure(pgmap, PFN_PHYS(pfn), PAGE_SIZE,
+ flags);
...then you'll never get fine-grained reports. So, I still think the
DAX, pgmap and memory_failure() interface should be pfn based. The
interface to the *filesystem* ->notify_failure() can still be
byte-based, but the trigger for that byte based interface will likely
need to be something driven by another agent. Perhaps like rasdaemon
in userspace translating all the arch specific physical address events
back into device-relative offsets and then calling a new ABI that is
serviced by fs->notify_failure() on the backend.
在 2022/4/8 9:38, Dan Williams 写道:
> [ add Mauro and Tony for RAS discussion ]
>
> On Wed, Apr 6, 2022 at 1:39 PM Darrick J. Wong <[email protected]> wrote:
>>
>> On Tue, Apr 05, 2022 at 06:22:48PM -0700, Dan Williams wrote:
>>> On Tue, Apr 5, 2022 at 5:55 PM Jane Chu <[email protected]> wrote:
>>>>
>>>> On 3/30/2022 9:18 AM, Darrick J. Wong wrote:
>>>>> On Wed, Mar 30, 2022 at 08:49:29AM -0700, Christoph Hellwig wrote:
>>>>>> On Wed, Mar 30, 2022 at 06:58:21PM +0800, Shiyang Ruan wrote:
>>>>>>> As the code I pasted before, pmem driver will subtract its ->data_offset,
>>>>>>> which is byte-based. And the filesystem who implements ->notify_failure()
>>>>>>> will calculate the offset in unit of byte again.
>>>>>>>
>>>>>>> So, leave its function signature byte-based, to avoid repeated conversions.
>>>>>>
>>>>>> I'm actually fine either way, so I'll wait for Dan to comment.
>>>>>
>>>>> FWIW I'd convinced myself that the reason for using byte units is to
>>>>> make it possible to reduce the pmem failure blast radius to subpage
>>>>> units... but then I've also been distracted for months. :/
>>>>>
>>>>
>>>> Yes, thanks Darrick! I recall that.
>>>> Maybe just add a comment about why byte unit is used?
>>>
>>> I think we start with page failure notification and then figure out
>>> how to get finer grained through the dax interface in follow-on
>>> changes. Otherwise, for finer grained error handling support,
>>> memory_failure() would also need to be converted to stop upcasting
>>> cache-line granularity to page granularity failures. The native MCE
>>> notification communicates a 'struct mce' that can be in terms of
>>> sub-page bytes, but the memory management implications are all page
>>> based. I assume the FS implications are all FS-block-size based?
>>
>> I wouldn't necessarily make that assumption -- for regular files, the
>> user program is in a better position to figure out how to reset the file
>> contents.
>>
>> For fs metadata, it really depends. In principle, if (say) we could get
>> byte granularity poison info, we could look up the space usage within
>> the block to decide if the poisoned part was actually free space, in
>> which case we can correct the problem by (re)zeroing the affected bytes
>> to clear the poison.
>>
>> Obviously, if the blast radius hits the internal space info or something
>> that was storing useful data, then you'd have to rebuild the whole block
>> (or the whole data structure), but that's not necessarily a given.
>
> tl;dr: dax_holder_notify_failure() != fs->notify_failure()
>
> So I think I see some confusion between what DAX->notify_failure()
> needs, memory_failure() needs, the raw information provided by the
> hardware, and the failure granularity the filesystem can make use of.
>
> DAX and memory_failure() need to make immediate page granularity
> decisions. They both need to map out whole pages (in the direct map
> and userspace respectively) to prevent future poison consumption, at
> least until the poison is repaired.
>
> The event that leads to a page being failed can be triggered by a
> hardware error as small as an individual cacheline. While that is
> interesting to a filesystem it isn't information that memory_failure()
> and DAX can utilize.
>
> The reason DAX needs to have a callback into filesystem code is to map
> the page failure back to all the processes that might have that page
> mapped because reflink means that page->mapping is not sufficient to
> find all the affected 'struct address_space' instances. So it's more
> of an address-translation / "help me kill processes" service than a
> general failure notification service.
>
> Currently when raw hardware event happens there are mechanisms like
> arch-specific notifier chains, like powerpc::mce_register_notifier()
> and x86::mce_register_decode_chain(), or other platform firmware code
> like ghes_edac_report_mem_error() that uplevel the error to a coarse
> page granularity failure, while emitting the fine granularity error
> event to userspace.
>
> All of this to say that the interface to ask the fs to do the bottom
> half of memory_failure() (walking affected 'struct address_space'
> instances and killing processes (mf_dax_kill_procs())) is different
> than the general interface to tell the filesystem that memory has gone
> bad relative to a device. So if the only caller of
> fs->notify_failure() handler is this code:
>
> + if (pgmap->ops->memory_failure) {
> + rc = pgmap->ops->memory_failure(pgmap, PFN_PHYS(pfn), PAGE_SIZE,
> + flags);
>
> ...then you'll never get fine-grained reports. So, I still think the
> DAX, pgmap and memory_failure() interface should be pfn based. The
> interface to the *filesystem* ->notify_failure() can still be
> byte-based, but the trigger for that byte based interface will likely
> need to be something driven by another agent. Perhaps like rasdaemon
> in userspace translating all the arch specific physical address events
> back into device-relative offsets and then calling a new ABI that is
> serviced by fs->notify_failure() on the backend.
Understood. I'll do as your advise. Thanks!
--
Ruan.
在 2022/3/30 14:00, Christoph Hellwig 写道:
>> @@ -1892,6 +1893,8 @@ xfs_free_buftarg(
>> list_lru_destroy(&btp->bt_lru);
>>
>> blkdev_issue_flush(btp->bt_bdev);
>> + if (btp->bt_daxdev)
>> + dax_unregister_holder(btp->bt_daxdev, btp->bt_mount);
>> fs_put_dax(btp->bt_daxdev);
>>
>> kmem_free(btp);
>> @@ -1939,6 +1942,7 @@ xfs_alloc_buftarg(
>> struct block_device *bdev)
>> {
>> xfs_buftarg_t *btp;
>> + int error;
>>
>> btp = kmem_zalloc(sizeof(*btp), KM_NOFS);
>>
>> @@ -1946,6 +1950,14 @@ xfs_alloc_buftarg(
>> btp->bt_dev = bdev->bd_dev;
>> btp->bt_bdev = bdev;
>> btp->bt_daxdev = fs_dax_get_by_bdev(bdev, &btp->bt_dax_part_off);
>> + if (btp->bt_daxdev) {
>> + error = dax_register_holder(btp->bt_daxdev, mp,
>> + &xfs_dax_holder_operations);
>> + if (error) {
>> + xfs_err(mp, "DAX device already in use?!");
>> + goto error_free;
>> + }
>> + }
>
> It seems to me that just passing the holder and holder ops to
> fs_dax_get_by_bdev and the holder to dax_unregister_holder would
> significantly simply the interface here.
>
> Dan, what do you think?
Hi Dan,
Could you give some advise on this API? Is it needed to move
dax_register_holder's job into fs_dax_get_by_bdev()?
--
Thanks,
Ruan
>
>> +#if IS_ENABLED(CONFIG_MEMORY_FAILURE) && IS_ENABLED(CONFIG_FS_DAX)
>
> No real need for the IS_ENABLED. Also any reason to even build this
> file if the options are not set? It seems like
> xfs_dax_holder_operations should just be defined to NULL and the
> whole file not supported if we can't support the functionality.
>
> Dan: not for this series, but is there any reason not to require
> MEMORY_FAILURE for DAX to start with?
>
>> +
>> + ddev_start = mp->m_ddev_targp->bt_dax_part_off;
>> + ddev_end = ddev_start +
>> + (mp->m_ddev_targp->bt_bdev->bd_nr_sectors << SECTOR_SHIFT) - 1;
>
> This should use bdev_nr_bytes.
>
> But didn't we say we don't want to support notifications on partitioned
> devices and thus don't actually need all this?
>
>> +
>> + /* Ignore the range out of filesystem area */
>> + if ((offset + len) < ddev_start)
>
> No need for the inner braces.
>
>> + if ((offset + len) > ddev_end)
>
> No need for the braces either.
>
>> diff --git a/fs/xfs/xfs_notify_failure.h b/fs/xfs/xfs_notify_failure.h
>> new file mode 100644
>> index 000000000000..76187b9620f9
>> --- /dev/null
>> +++ b/fs/xfs/xfs_notify_failure.h
>> @@ -0,0 +1,10 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2022 Fujitsu. All Rights Reserved.
>> + */
>> +#ifndef __XFS_NOTIFY_FAILURE_H__
>> +#define __XFS_NOTIFY_FAILURE_H__
>> +
>> +extern const struct dax_holder_operations xfs_dax_holder_operations;
>> +
>> +#endif /* __XFS_NOTIFY_FAILURE_H__ */
>
> Dowe really need a new header for this vs just sequeezing it into
> xfs_super.h or something like that?
>
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index e8f37bdc8354..b8de6ed2c888 100644
>> --- a/fs/xfs/xfs_super.c
>> +++ b/fs/xfs/xfs_super.c
>> @@ -353,6 +353,12 @@ xfs_setup_dax_always(
>> return -EINVAL;
>> }
>>
>> + if (xfs_has_reflink(mp) && !xfs_has_rmapbt(mp)) {
>> + xfs_alert(mp,
>> + "need rmapbt when both DAX and reflink enabled.");
>> + return -EINVAL;
>> + }
>
> Right now we can't even enable reflink with DAX yet, so adding this
> here seems premature - it should go into the patch allowing DAX+reflink.
>
On Tue, Mar 29, 2022 at 11:01 PM Christoph Hellwig <[email protected]> wrote:
>
> > @@ -1892,6 +1893,8 @@ xfs_free_buftarg(
> > list_lru_destroy(&btp->bt_lru);
> >
> > blkdev_issue_flush(btp->bt_bdev);
> > + if (btp->bt_daxdev)
> > + dax_unregister_holder(btp->bt_daxdev, btp->bt_mount);
> > fs_put_dax(btp->bt_daxdev);
> >
> > kmem_free(btp);
> > @@ -1939,6 +1942,7 @@ xfs_alloc_buftarg(
> > struct block_device *bdev)
> > {
> > xfs_buftarg_t *btp;
> > + int error;
> >
> > btp = kmem_zalloc(sizeof(*btp), KM_NOFS);
> >
> > @@ -1946,6 +1950,14 @@ xfs_alloc_buftarg(
> > btp->bt_dev = bdev->bd_dev;
> > btp->bt_bdev = bdev;
> > btp->bt_daxdev = fs_dax_get_by_bdev(bdev, &btp->bt_dax_part_off);
> > + if (btp->bt_daxdev) {
> > + error = dax_register_holder(btp->bt_daxdev, mp,
> > + &xfs_dax_holder_operations);
> > + if (error) {
> > + xfs_err(mp, "DAX device already in use?!");
> > + goto error_free;
> > + }
> > + }
>
> It seems to me that just passing the holder and holder ops to
> fs_dax_get_by_bdev and the holder to dax_unregister_holder would
> significantly simply the interface here.
>
> Dan, what do you think?
Yes, makes sense, just like the optional holder arguments to blkdev_get_by_*().
>
> > +#if IS_ENABLED(CONFIG_MEMORY_FAILURE) && IS_ENABLED(CONFIG_FS_DAX)
>
> No real need for the IS_ENABLED. Also any reason to even build this
> file if the options are not set? It seems like
> xfs_dax_holder_operations should just be defined to NULL and the
> whole file not supported if we can't support the functionality.
>
> Dan: not for this series, but is there any reason not to require
> MEMORY_FAILURE for DAX to start with?
Given that DAX ties some storage semantics to memory and storage
supports EIO I can see an argument to require memory_failure() for
DAX, and especially for DAX on CXL where hotplug is supported it will
be necessary. Linux currently has no facility to consult PCI drivers
about removal actions, so the only recourse for a force removed CXL
device is mass memory_failure().
>
> > +
> > + ddev_start = mp->m_ddev_targp->bt_dax_part_off;
> > + ddev_end = ddev_start +
> > + (mp->m_ddev_targp->bt_bdev->bd_nr_sectors << SECTOR_SHIFT) - 1;
>
> This should use bdev_nr_bytes.
>
> But didn't we say we don't want to support notifications on partitioned
> devices and thus don't actually need all this?
Right.
On Thu, Apr 7, 2022 at 11:05 PM Shiyang Ruan <[email protected]> wrote:
>
>
>
> 在 2022/3/30 14:00, Christoph Hellwig 写道:
> >> @@ -1892,6 +1893,8 @@ xfs_free_buftarg(
> >> list_lru_destroy(&btp->bt_lru);
> >>
> >> blkdev_issue_flush(btp->bt_bdev);
> >> + if (btp->bt_daxdev)
> >> + dax_unregister_holder(btp->bt_daxdev, btp->bt_mount);
> >> fs_put_dax(btp->bt_daxdev);
> >>
> >> kmem_free(btp);
> >> @@ -1939,6 +1942,7 @@ xfs_alloc_buftarg(
> >> struct block_device *bdev)
> >> {
> >> xfs_buftarg_t *btp;
> >> + int error;
> >>
> >> btp = kmem_zalloc(sizeof(*btp), KM_NOFS);
> >>
> >> @@ -1946,6 +1950,14 @@ xfs_alloc_buftarg(
> >> btp->bt_dev = bdev->bd_dev;
> >> btp->bt_bdev = bdev;
> >> btp->bt_daxdev = fs_dax_get_by_bdev(bdev, &btp->bt_dax_part_off);
> >> + if (btp->bt_daxdev) {
> >> + error = dax_register_holder(btp->bt_daxdev, mp,
> >> + &xfs_dax_holder_operations);
> >> + if (error) {
> >> + xfs_err(mp, "DAX device already in use?!");
> >> + goto error_free;
> >> + }
> >> + }
> >
> > It seems to me that just passing the holder and holder ops to
> > fs_dax_get_by_bdev and the holder to dax_unregister_holder would
> > significantly simply the interface here.
> >
> > Dan, what do you think?
>
> Hi Dan,
>
> Could you give some advise on this API? Is it needed to move
> dax_register_holder's job into fs_dax_get_by_bdev()?
Yes, works for me to just add them as optional arguments.