| Message ID | 20210628000218.387833-6-ruansy.fnst@fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fsdax: introduce fs query to support reflink | expand |
Hi Shiyang,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.13 next-20210625]
[cannot apply to hnaz-linux-mm/master xfs-linux/for-next dm/for-next]
[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/20210628-080346
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 62fb9874f5da54fdb243003b386128037319b219
config: x86_64-randconfig-a006-20210628 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 4c92e31dd0f1bd152eda883af20ff7fbcaa14945)
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
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/dc92f6aa75da7bf5c6f974645a157759691ca36a
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/20210628-080346
git checkout dc92f6aa75da7bf5c6f974645a157759691ca36a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> mm/memory-failure.c:1325:22: error: implicit declaration of function 'dax_load_pfn' [-Werror,-Wimplicit-function-declaration]
unsigned long pfn = dax_load_pfn(mapping, index);
^
1 error generated.
vim +/dax_load_pfn +1325 mm/memory-failure.c
1320
1321 int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index, int flags)
1322 {
1323 LIST_HEAD(to_kill);
1324 /* load the pfn of the dax mapping file */
> 1325 unsigned long pfn = dax_load_pfn(mapping, index);
1326
1327 /*
1328 * Unlike System-RAM there is no possibility to swap in a
1329 * different physical page at a given virtual address, so all
1330 * userspace consumption of ZONE_DEVICE memory necessitates
1331 * SIGBUS (i.e. MF_MUST_KILL)
1332 */
1333 flags |= MF_ACTION_REQUIRED | MF_MUST_KILL;
1334 collect_procs_file(pfn_to_page(pfn), mapping, index, &to_kill,
1335 flags & MF_ACTION_REQUIRED);
1336
1337 unmap_and_kill(&to_kill, pfn, mapping, index, flags);
1338 return 0;
1339 }
1340 EXPORT_SYMBOL_GPL(mf_dax_kill_procs);
1341
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Mon, Jun 28, 2021 at 08:02:14AM +0800, Shiyang Ruan wrote: > +/* > + * dax_load_pfn - Load pfn of the DAX entry corresponding to a page > + * @mapping: The file whose entry we want to load > + * @index: offset where the DAX entry located in > + * > + * Return: pfn number of the DAX entry > + */ This is an externally visible function; why not add the second '*' and make this kernel-doc? > +unsigned long dax_load_pfn(struct address_space *mapping, unsigned long index) > +{ > + XA_STATE(xas, &mapping->i_pages, index); > + void *entry; > + unsigned long pfn; > + > + xas_lock_irq(&xas); > + entry = xas_load(&xas); > + pfn = dax_to_pfn(entry); > + xas_unlock_irq(&xas); Why do you need the i_pages lock to do this? is the rcu_read_lock() insufficient? For that matter, why use the xas functions? Why not simply: void *entry = xa_load(&mapping->i_pages, index); return dax_to_pfn(entry); Looking at it more though, how do you know this is a PFN entry? It could be locked, for example. Or the zero page, or empty. But I think this is unnecessary; why not just pass the PFN into mf_dax_kill_procs?
> -----Original Message----- > From: Matthew Wilcox <willy@infradead.org> > Subject: Re: [PATCH v5 5/9] mm: Introduce mf_dax_kill_procs() for fsdax case > > On Mon, Jun 28, 2021 at 08:02:14AM +0800, Shiyang Ruan wrote: > > +/* > > + * dax_load_pfn - Load pfn of the DAX entry corresponding to a page > > + * @mapping: The file whose entry we want to load > > + * @index: offset where the DAX entry located in > > + * > > + * Return: pfn number of the DAX entry > > + */ > > This is an externally visible function; why not add the second '*' and make this > kernel-doc? I'll fix this and add kernel-doc. > > > +unsigned long dax_load_pfn(struct address_space *mapping, unsigned > > +long index) { > > + XA_STATE(xas, &mapping->i_pages, index); > > + void *entry; > > + unsigned long pfn; > > + > > + xas_lock_irq(&xas); > > + entry = xas_load(&xas); > > + pfn = dax_to_pfn(entry); > > + xas_unlock_irq(&xas); > > Why do you need the i_pages lock to do this? is the rcu_read_lock() insufficient? I was misusing these locks, not very filmier with them... So, I think I should learn from dax_lock_page(): rcu_read_lock(), xas_load(&xas, index), and then wait_entry_unlocked(), finally get an unlocked entry, translate to PFN and return. > For that matter, why use the xas functions? Why not > simply: > > void *entry = xa_load(&mapping->i_pages, index); > return dax_to_pfn(entry); > > Looking at it more though, how do you know this is a PFN entry? > It could be locked, for example. Or the zero page, or empty. Yes, I didn't take these in consideration. If this file hasn't been mmapped and accessed, I can't get its PFN rightly. > > But I think this is unnecessary; why not just pass the PFN into mf_dax_kill_procs? Because the mf_dax_kill_procs() is called in filesystem recovery function, which is at the end of the RMAP routine. And the PFN has been translated to disk offset in pmem driver in order to do RMAP search in filesystem. So, if we have to pass it, every function in this routine needs to add an argument for this PFN. I was hoping I can avoid passing PFN through the whole stack with the help of this dax_load_pfn(). So, based on the above, if a file hasn't been mmapped and accessed, we can't get the right PFN number, which also means no process is associated with this PFN. Then we don't have to kill any process any more. Just return with an error code. mf_dax_kill_porcess() can also return immediately. How do you think? -- Thanks, Ruan.
On Tue, Jun 29, 2021 at 07:49:24AM +0000, ruansy.fnst@fujitsu.com wrote: > > But I think this is unnecessary; why not just pass the PFN into mf_dax_kill_procs? > > Because the mf_dax_kill_procs() is called in filesystem recovery function, which is at the end of the RMAP routine. And the PFN has been translated to disk offset in pmem driver in order to do RMAP search in filesystem. So, if we have to pass it, every function in this routine needs to add an argument for this PFN. I was hoping I can avoid passing PFN through the whole stack with the help of this dax_load_pfn(). OK, I think you need to create: struct memory_failure { phys_addr_t start; phys_addr_t end; unsigned long flags; }; (a memory failure might not be an entire page, so working in pfns isn't the best approach) Then that can be passed to ->memory_failure() and then deeper to ->notify_failure(), and finally into xfs_corrupt_helper().
> From: Matthew Wilcox <willy@infradead.org> > Subject: Re: [PATCH v5 5/9] mm: Introduce mf_dax_kill_procs() for fsdax case > > On Tue, Jun 29, 2021 at 07:49:24AM +0000, ruansy.fnst@fujitsu.com wrote: > > > But I think this is unnecessary; why not just pass the PFN into > mf_dax_kill_procs? > > > > Because the mf_dax_kill_procs() is called in filesystem recovery function, > which is at the end of the RMAP routine. And the PFN has been translated to > disk offset in pmem driver in order to do RMAP search in filesystem. So, if we > have to pass it, every function in this routine needs to add an argument for this > PFN. I was hoping I can avoid passing PFN through the whole stack with the > help of this dax_load_pfn(). > > OK, I think you need to create: > > struct memory_failure { > phys_addr_t start; > phys_addr_t end; > unsigned long flags; > }; > > (a memory failure might not be an entire page, so working in pfns isn't the best > approach) Do you mean the range of memory failure may less than one page size? I found those memory_failure* functions are using pfn as their parameter. So in which case it could be less than one page size? -- Thanks, Ruan. > > Then that can be passed to ->memory_failure() and then deeper to > ->notify_failure(), and finally into xfs_corrupt_helper().
diff --git a/fs/dax.c b/fs/dax.c index 62352cbcf0f4..7c5d8acd6bc5 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -389,6 +389,27 @@ static struct page *dax_busy_page(void *entry) return NULL; } +/* + * dax_load_pfn - Load pfn of the DAX entry corresponding to a page + * @mapping: The file whose entry we want to load + * @index: offset where the DAX entry located in + * + * Return: pfn number of the DAX entry + */ +unsigned long dax_load_pfn(struct address_space *mapping, unsigned long index) +{ + XA_STATE(xas, &mapping->i_pages, index); + void *entry; + unsigned long pfn; + + xas_lock_irq(&xas); + entry = xas_load(&xas); + pfn = dax_to_pfn(entry); + xas_unlock_irq(&xas); + + return pfn; +} + /* * dax_lock_mapping_entry - Lock the DAX entry corresponding to a page * @page: The page whose entry we want to lock @@ -789,16 +810,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) diff --git a/include/linux/dax.h b/include/linux/dax.h index 6f4b5c97ceb0..fc0c80f09b79 100644 --- a/include/linux/dax.h +++ b/include/linux/dax.h @@ -165,6 +165,7 @@ int dax_writeback_mapping_range(struct address_space *mapping, struct page *dax_layout_busy_page(struct address_space *mapping); struct page *dax_layout_busy_page_range(struct address_space *mapping, loff_t start, loff_t end); +unsigned long dax_load_pfn(struct address_space *mapping, unsigned long index); dax_entry_t dax_lock_page(struct page *page); void dax_unlock_page(struct page *page, dax_entry_t cookie); #else @@ -259,6 +260,15 @@ static inline bool dax_mapping(struct address_space *mapping) { return mapping->host && IS_DAX(mapping->host); } +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; +} #ifdef CONFIG_DEV_DAX_HMEM_DEVICES void hmem_register_device(int target_nid, struct resource *r); diff --git a/include/linux/mm.h b/include/linux/mm.h index 8ae31622deef..3f101caec30e 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1187,6 +1187,14 @@ static inline bool is_device_private_page(const struct page *page) page->pgmap->type == MEMORY_DEVICE_PRIVATE; } +static inline bool is_device_fsdax_page(const struct page *page) +{ + return IS_ENABLED(CONFIG_DEV_PAGEMAP_OPS) && + IS_ENABLED(CONFIG_FS_DAX) && + is_zone_device_page(page) && + page->pgmap->type == MEMORY_DEVICE_FS_DAX; +} + static inline bool is_pci_p2pdma_page(const struct page *page) { return IS_ENABLED(CONFIG_DEV_PAGEMAP_OPS) && @@ -3081,6 +3089,8 @@ enum mf_flags { MF_MUST_KILL = 1 << 2, MF_SOFT_OFFLINE = 1 << 3, }; +extern int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index, + int flags); 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 30ce1e653c0a..37ac15b223e0 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -120,6 +120,12 @@ static int hwpoison_filter_dev(struct page *p) if (PageSlab(p)) return -EINVAL; + if (pfn_valid(page_to_pfn(p))) { + if (is_device_fsdax_page(p)) + return 0; + } else + return -EINVAL; + mapping = page_mapping(p); if (mapping == NULL || mapping->host == NULL) return -EINVAL; @@ -290,10 +296,9 @@ void shake_page(struct page *p, int access) } EXPORT_SYMBOL_GPL(shake_page); -static unsigned long dev_pagemap_mapping_shift(struct page *page, +static unsigned long dev_pagemap_mapping_shift(unsigned long address, struct vm_area_struct *vma) { - unsigned long address = vma_address(page, vma); pgd_t *pgd; p4d_t *p4d; pud_t *pud; @@ -333,7 +338,7 @@ 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. */ -static void add_to_kill(struct task_struct *tsk, struct page *p, +static void add_to_kill(struct task_struct *tsk, struct page *p, pgoff_t pgoff, struct vm_area_struct *vma, struct list_head *to_kill) { @@ -346,9 +351,14 @@ 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 no more used for fsdax, we should + * calculate the address in a fsdax way. + */ + if (is_device_fsdax_page(p)) + tk->addr = pgoff_address(pgoff, vma); + tk->size_shift = dev_pagemap_mapping_shift(tk->addr, vma); + } else tk->size_shift = page_shift(compound_head(p)); /* @@ -496,7 +506,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); @@ -506,24 +516,20 @@ static void collect_procs_anon(struct page *page, struct list_head *to_kill, /* * Collect processes when the error hit a file mapped page. */ -static void collect_procs_file(struct page *page, struct list_head *to_kill, - int force_early) +static void collect_procs_file(struct page *page, struct address_space *mapping, + pgoff_t pgoff, struct list_head *to_kill, int force_early) { struct vm_area_struct *vma; struct task_struct *tsk; - struct address_space *mapping = page->mapping; - pgoff_t pgoff; i_mmap_lock_read(mapping); read_lock(&tasklist_lock); - pgoff = page_to_pgoff(page); for_each_process(tsk) { struct task_struct *t = task_early_kill(tsk, force_early); if (!t) continue; - vma_interval_tree_foreach(vma, &mapping->i_mmap, pgoff, - pgoff) { + vma_interval_tree_foreach(vma, &mapping->i_mmap, pgoff, pgoff) { /* * Send early kill signal to tasks where a vma covers * the page but the corrupted page is not necessarily @@ -532,7 +538,7 @@ 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, pgoff, vma, to_kill); } } read_unlock(&tasklist_lock); @@ -551,7 +557,8 @@ static void collect_procs(struct page *page, struct list_head *tokill, if (PageAnon(page)) collect_procs_anon(page, tokill, force_early); else - collect_procs_file(page, tokill, force_early); + collect_procs_file(page, page->mapping, page->index, tokill, + force_early); } static const char *action_name[] = { @@ -1297,6 +1304,27 @@ static int mf_generic_kill_procs(unsigned long long pfn, int flags) return 0; } +int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index, int flags) +{ + LIST_HEAD(to_kill); + /* load the pfn of the dax mapping file */ + unsigned long pfn = dax_load_pfn(mapping, index); + + /* + * 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_file(pfn_to_page(pfn), mapping, index, &to_kill, + flags & MF_ACTION_REQUIRED); + + unmap_and_kill(&to_kill, pfn, mapping, index, flags); + return 0; +} +EXPORT_SYMBOL_GPL(mf_dax_kill_procs); + static int memory_failure_hugetlb(unsigned long pfn, int flags) { struct page *p = pfn_to_page(pfn);
This function is called at the end of RMAP routine, i.e. filesystem recovery function. The difference between mf_generic_kill_procs() is, mf_dax_kill_procs() accepts file mapping and offset instead of struct page. It is because that different file mappings and offsets may share the same page in fsdax mode. So, it is called when filesystem RMAP results are found. Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> --- fs/dax.c | 31 +++++++++++++++-------- include/linux/dax.h | 10 ++++++++ include/linux/mm.h | 10 ++++++++ mm/memory-failure.c | 60 +++++++++++++++++++++++++++++++++------------ 4 files changed, 85 insertions(+), 26 deletions(-) -- 2.32.0