diff mbox

[2/2] dax: Fix error returns in PMD fault handler

Message ID 1449179922-502-3-git-send-email-matthew.r.wilcox@intel.com (mailing list archive)
State Superseded
Headers show

Commit Message

Wilcox, Matthew R Dec. 3, 2015, 9:58 p.m. UTC
From: Matthew Wilcox <willy@linux.intel.com>

When we need to bail at this point, we do not hold the mutex, so jumping
to the error path which will drop the mutex is the wrong approach.
Just return the fallback instead.  This mostly reverts commit fd6b9f6393.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
---
 fs/dax.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

Comments

Dan Williams Dec. 4, 2015, 2:27 a.m. UTC | #1
On Thu, Dec 3, 2015 at 1:58 PM, Matthew Wilcox
<matthew.r.wilcox@intel.com> wrote:
> From: Matthew Wilcox <willy@linux.intel.com>
>
> When we need to bail at this point, we do not hold the mutex, so jumping
> to the error path which will drop the mutex is the wrong approach.
> Just return the fallback instead.  This mostly reverts commit fd6b9f6393.
>
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> ---
>  fs/dax.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/fs/dax.c b/fs/dax.c
> index 727af65..71ba5f7 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -581,28 +581,22 @@ int __dax_pmd_fault(struct vm_area_struct *vma, unsigned long address,
>         /* Fall back to PTEs if we're going to COW */
>         if (write && !(vma->vm_flags & VM_SHARED)) {
>                 split_huge_page_pmd(vma, address, pmd);
> -               reason = "cow write";
>                 return VM_FAULT_FALLBACK;
>         }
> +
>         /* If the PMD would extend outside the VMA */
> -       if (pmd_addr < vma->vm_start) {
> -               reason = "vma start unaligned";
> -               goto fallback;
> -       }
> -       if ((pmd_addr + PMD_SIZE) > vma->vm_end) {
> -               reason = "vma end unaligned";
> -               goto fallback;
> -       }
> +       if (pmd_addr < vma->vm_start)
> +               return VM_FAULT_FALLBACK;
> +       if ((pmd_addr + PMD_SIZE) > vma->vm_end)
> +               return VM_FAULT_FALLBACK;
>
>         pgoff = linear_page_index(vma, pmd_addr);
>         size = (i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT;
>         if (pgoff >= size)
>                 return VM_FAULT_SIGBUS;
>         /* If the PMD would cover blocks out of the file */
> -       if ((pgoff | PG_PMD_COLOUR) >= size) {
> -               reason = "offset + huge page size > file size";
> +       if ((pgoff | PG_PMD_COLOUR) >= size)
>                 return VM_FAULT_FALLBACK;
> -       }
>
>         memset(&bh, 0, sizeof(bh));
>         block = (sector_t)pgoff << (PAGE_SHIFT - blkbits);

Having the pmd setup failure reasons is still handy since we have no
other indication .  I'll fix this up to handle the lock correctly.
Dan Williams Dec. 4, 2015, 2:57 a.m. UTC | #2
On Thu, Dec 3, 2015 at 6:27 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Thu, Dec 3, 2015 at 1:58 PM, Matthew Wilcox
> <matthew.r.wilcox@intel.com> wrote:
>> From: Matthew Wilcox <willy@linux.intel.com>
>>
>> When we need to bail at this point, we do not hold the mutex, so jumping
>> to the error path which will drop the mutex is the wrong approach.
>> Just return the fallback instead.  This mostly reverts commit fd6b9f6393.
>>
>> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
[..]
> Having the pmd setup failure reasons is still handy since we have no
> other indication .  I'll fix this up to handle the lock correctly.

Here's what I came up with... no control flow changes.

https://git.kernel.org/cgit/linux/kernel/git/djbw/nvdimm.git/commit/?h=libnvdimm-pending&id=fd6b9f639309774cbd6d0a02befe257711a84cfb
Dan Williams Dec. 4, 2015, 2:59 a.m. UTC | #3
On Thu, Dec 3, 2015 at 6:57 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Thu, Dec 3, 2015 at 6:27 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>> On Thu, Dec 3, 2015 at 1:58 PM, Matthew Wilcox
>> <matthew.r.wilcox@intel.com> wrote:
>>> From: Matthew Wilcox <willy@linux.intel.com>
>>>
>>> When we need to bail at this point, we do not hold the mutex, so jumping
>>> to the error path which will drop the mutex is the wrong approach.
>>> Just return the fallback instead.  This mostly reverts commit fd6b9f6393.
>>>
>>> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> [..]
>> Having the pmd setup failure reasons is still handy since we have no
>> other indication .  I'll fix this up to handle the lock correctly.
>
> Here's what I came up with... no control flow changes.
>
> https://git.kernel.org/cgit/linux/kernel/git/djbw/nvdimm.git/commit/?h=libnvdimm-pending&id=fd6b9f639309774cbd6d0a02befe257711a84cfb

Ah, sorry should have sent the patch.  kernel.org hadn't refreshed yet:

https://git.kernel.org/cgit/linux/kernel/git/djbw/nvdimm.git/commit/?h=libnvdimm-pending&id=a6cea26fcbe0
diff mbox

Patch

diff --git a/fs/dax.c b/fs/dax.c
index 727af65..71ba5f7 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -581,28 +581,22 @@  int __dax_pmd_fault(struct vm_area_struct *vma, unsigned long address,
 	/* Fall back to PTEs if we're going to COW */
 	if (write && !(vma->vm_flags & VM_SHARED)) {
 		split_huge_page_pmd(vma, address, pmd);
-		reason = "cow write";
 		return VM_FAULT_FALLBACK;
 	}
+
 	/* If the PMD would extend outside the VMA */
-	if (pmd_addr < vma->vm_start) {
-		reason = "vma start unaligned";
-		goto fallback;
-	}
-	if ((pmd_addr + PMD_SIZE) > vma->vm_end) {
-		reason = "vma end unaligned";
-		goto fallback;
-	}
+	if (pmd_addr < vma->vm_start)
+		return VM_FAULT_FALLBACK;
+	if ((pmd_addr + PMD_SIZE) > vma->vm_end)
+		return VM_FAULT_FALLBACK;
 
 	pgoff = linear_page_index(vma, pmd_addr);
 	size = (i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT;
 	if (pgoff >= size)
 		return VM_FAULT_SIGBUS;
 	/* If the PMD would cover blocks out of the file */
-	if ((pgoff | PG_PMD_COLOUR) >= size) {
-		reason = "offset + huge page size > file size";
+	if ((pgoff | PG_PMD_COLOUR) >= size)
 		return VM_FAULT_FALLBACK;
-	}
 
 	memset(&bh, 0, sizeof(bh));
 	block = (sector_t)pgoff << (PAGE_SHIFT - blkbits);