Re: [PATCH v3 01/10] fsdax: Factor helpers to simplify dax fault code

2021-03-23 Thread Ritesh Harjani




On 3/19/21 7:22 AM, Shiyang Ruan wrote:

The dax page fault code is too long and a bit difficult to read. And it
is hard to understand when we trying to add new features. Some of the
PTE/PMD codes have similar logic. So, factor them as helper functions to
simplify the code.

Signed-off-by: Shiyang Ruan 
Reviewed-by: Christoph Hellwig 
---
  fs/dax.c | 152 ++-
  1 file changed, 84 insertions(+), 68 deletions(-)



Refactoring & the changes looks good to me.
Feel free to add.

Reviewed-by: Ritesh Harjani 


[PATCH v3 01/10] fsdax: Factor helpers to simplify dax fault code

2021-03-18 Thread Shiyang Ruan
The dax page fault code is too long and a bit difficult to read. And it
is hard to understand when we trying to add new features. Some of the
PTE/PMD codes have similar logic. So, factor them as helper functions to
simplify the code.

Signed-off-by: Shiyang Ruan 
Reviewed-by: Christoph Hellwig 
---
 fs/dax.c | 152 ++-
 1 file changed, 84 insertions(+), 68 deletions(-)

diff --git a/fs/dax.c b/fs/dax.c
index 26d5dcd2d69e..7031e4302b13 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -1243,6 +1243,52 @@ static bool dax_fault_is_synchronous(unsigned long flags,
&& (iomap->flags & IOMAP_F_DIRTY);
 }
 
+/*
+ * If we are doing synchronous page fault and inode needs fsync, we can insert
+ * PTE/PMD into page tables only after that happens. Skip insertion for now and
+ * return the pfn so that caller can insert it after fsync is done.
+ */
+static vm_fault_t dax_fault_synchronous_pfnp(pfn_t *pfnp, pfn_t pfn)
+{
+   if (WARN_ON_ONCE(!pfnp))
+   return VM_FAULT_SIGBUS;
+
+   *pfnp = pfn;
+   return VM_FAULT_NEEDDSYNC;
+}
+
+static int dax_fault_cow_page(struct vm_fault *vmf, struct iomap *iomap,
+   loff_t pos, vm_fault_t *ret)
+{
+   int error = 0;
+   unsigned long vaddr = vmf->address;
+   sector_t sector = dax_iomap_sector(iomap, pos);
+
+   switch (iomap->type) {
+   case IOMAP_HOLE:
+   case IOMAP_UNWRITTEN:
+   clear_user_highpage(vmf->cow_page, vaddr);
+   break;
+   case IOMAP_MAPPED:
+   error = copy_cow_page_dax(iomap->bdev, iomap->dax_dev,
+   sector, vmf->cow_page, vaddr);
+   break;
+   default:
+   WARN_ON_ONCE(1);
+   error = -EIO;
+   break;
+   }
+
+   if (error)
+   return error;
+
+   __SetPageUptodate(vmf->cow_page);
+   *ret = finish_fault(vmf);
+   if (!*ret)
+   *ret = VM_FAULT_DONE_COW;
+   return 0;
+}
+
 static vm_fault_t dax_iomap_pte_fault(struct vm_fault *vmf, pfn_t *pfnp,
   int *iomap_errp, const struct iomap_ops *ops)
 {
@@ -1311,30 +1357,9 @@ static vm_fault_t dax_iomap_pte_fault(struct vm_fault 
*vmf, pfn_t *pfnp,
}
 
if (vmf->cow_page) {
-   sector_t sector = dax_iomap_sector(, pos);
-
-   switch (iomap.type) {
-   case IOMAP_HOLE:
-   case IOMAP_UNWRITTEN:
-   clear_user_highpage(vmf->cow_page, vaddr);
-   break;
-   case IOMAP_MAPPED:
-   error = copy_cow_page_dax(iomap.bdev, iomap.dax_dev,
- sector, vmf->cow_page, vaddr);
-   break;
-   default:
-   WARN_ON_ONCE(1);
-   error = -EIO;
-   break;
-   }
-
+   error = dax_fault_cow_page(vmf, , pos, );
if (error)
-   goto error_finish_iomap;
-
-   __SetPageUptodate(vmf->cow_page);
-   ret = finish_fault(vmf);
-   if (!ret)
-   ret = VM_FAULT_DONE_COW;
+   ret = dax_fault_return(error);
goto finish_iomap;
}
 
@@ -1354,19 +1379,8 @@ static vm_fault_t dax_iomap_pte_fault(struct vm_fault 
*vmf, pfn_t *pfnp,
entry = dax_insert_entry(, mapping, vmf, entry, pfn,
 0, write && !sync);
 
-   /*
-* If we are doing synchronous page fault and inode needs fsync,
-* we can insert PTE into page tables only after that happens.
-* Skip insertion for now and return the pfn so that caller can
-* insert it after fsync is done.
-*/
if (sync) {
-   if (WARN_ON_ONCE(!pfnp)) {
-   error = -EIO;
-   goto error_finish_iomap;
-   }
-   *pfnp = pfn;
-   ret = VM_FAULT_NEEDDSYNC | major;
+   ret = dax_fault_synchronous_pfnp(pfnp, pfn);
goto finish_iomap;
}
trace_dax_insert_mapping(inode, vmf, entry);
@@ -1465,13 +1479,45 @@ static vm_fault_t dax_pmd_load_hole(struct xa_state 
*xas, struct vm_fault *vmf,
return VM_FAULT_FALLBACK;
 }
 
+static bool dax_fault_check_fallback(struct vm_fault *vmf, struct xa_state 
*xas,
+   pgoff_t max_pgoff)
+{
+   unsigned long pmd_addr = vmf->address & PMD_MASK;
+   bool write = vmf->flags & FAULT_FLAG_WRITE;
+
+   /*
+* Make sure that the faulting address's PMD offset (color) matches
+* the PMD offset from the start of the file.  This is necessary so
+