[lustre-devel] [PATCH] staging: lustre: Change return type to vm_fault_t

[lustre-devel] [PATCH] staging: lustre: Change return type to vm_fault_t

[Souptick Joarder]

Use new return type vm_fault_t for fault handler. For
now, this is just documenting that the function returns
a VM_FAULT value rather than an errno. Once all instances
are converted, vm_fault_t will become a distinct type.

Commit 1c8f422059ae ("mm: change return type to vm_fault_t")

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
---
 drivers/staging/lustre/lustre/llite/llite_mmap.c | 29 ++++++++++++------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/llite_mmap.c b/drivers/staging/lustre/lustre/llite/llite_mmap.c
index c0533bd..6aafe7c 100644
--- a/drivers/staging/lustre/lustre/llite/llite_mmap.c
+++ b/drivers/staging/lustre/lustre/llite/llite_mmap.c
@@ -231,7 +231,7 @@ static int ll_page_mkwrite0(struct vm_area_struct *vma, struct page *vmpage,
 	return result;
 }
 
-static inline int to_fault_error(int result)
+static inline vm_fault_t to_fault_error(int result)
 {
 	switch (result) {
 	case 0:
@@ -261,7 +261,7 @@ static inline int to_fault_error(int result)
  * \retval VM_FAULT_ERROR on general error
  * \retval NOPAGE_OOM not have memory for allocate new page
  */
-static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
+static vm_fault_t ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
 	struct lu_env	   *env;
 	struct cl_io	    *io;
@@ -269,7 +269,7 @@ static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
 	struct page	     *vmpage;
 	unsigned long	    ra_flags;
 	int		      result = 0;
-	int		      fault_ret = 0;
+	vm_fault_t		fault_ret = 0;
 	u16 refcheck;
 
 	env = cl_env_get(&refcheck);
@@ -278,7 +278,7 @@ static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
 
 	io = ll_fault_io_init(env, vma, vmf->pgoff, &ra_flags);
 	if (IS_ERR(io)) {
-		result = to_fault_error(PTR_ERR(io));
+		fault_ret = to_fault_error(PTR_ERR(io));
 		goto out;
 	}
 
@@ -319,7 +319,7 @@ static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
 	if (result != 0 && !(fault_ret & VM_FAULT_RETRY))
 		fault_ret |= to_fault_error(result);
 
-	CDEBUG(D_MMAP, "%s fault %d/%d\n", current->comm, fault_ret, result);
+	CDEBUG(D_MMAP, "%s fault %x/%d\n", current->comm, fault_ret, result);
 	return fault_ret;
 }
 
@@ -327,7 +327,7 @@ static int ll_fault(struct vm_fault *vmf)
 {
 	int count = 0;
 	bool printed = false;
-	int result;
+	vm_fault_t result;
 	sigset_t set;
 
 	/* Only SIGKILL and SIGTERM are allowed for fault/nopage/mkwrite
@@ -370,12 +370,13 @@ static int ll_page_mkwrite(struct vm_fault *vmf)
 	int count = 0;
 	bool printed = false;
 	bool retry;
-	int result;
+	int err;
+	vm_fault_t ret;
 
 	file_update_time(vma->vm_file);
 	do {
 		retry = false;
-		result = ll_page_mkwrite0(vma, vmf->page, &retry);
+		err = ll_page_mkwrite0(vma, vmf->page, &retry);
 
 		if (!printed && ++count > 16) {
 			const struct dentry *de = vma->vm_file->f_path.dentry;
@@ -387,25 +388,25 @@ static int ll_page_mkwrite(struct vm_fault *vmf)
 		}
 	} while (retry);
 
-	switch (result) {
+	switch (err) {
 	case 0:
 		LASSERT(PageLocked(vmf->page));
-		result = VM_FAULT_LOCKED;
+		ret = VM_FAULT_LOCKED;
 		break;
 	case -ENODATA:
 	case -EAGAIN:
 	case -EFAULT:
-		result = VM_FAULT_NOPAGE;
+		ret = VM_FAULT_NOPAGE;
 		break;
 	case -ENOMEM:
-		result = VM_FAULT_OOM;
+		ret = VM_FAULT_OOM;
 		break;
 	default:
-		result = VM_FAULT_SIGBUS;
+		ret = VM_FAULT_SIGBUS;
 		break;
 	}
 
-	return result;
+	return ret;
 }
 
 /**
-- 
1.9.1

[lustre-devel] [PATCH] staging: lustre: Change return type to vm_fault_t

[Matthew Wilcox]

On Sun, Apr 22, 2018 at 03:47:24AM +0530, Souptick Joarder wrote:
> @@ -261,7 +261,7 @@ static inline int to_fault_error(int result)
>   * \retval VM_FAULT_ERROR on general error
>   * \retval NOPAGE_OOM not have memory for allocate new page
>   */
> -static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
> +static vm_fault_t ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
>  {
>  	struct lu_env	   *env;
>  	struct cl_io	    *io;

Did you compile-test this with the sparse changes?  Because I can see
a problem here:

        env = cl_env_get(&refcheck);
        if (IS_ERR(env))
                return PTR_ERR(env);

> @@ -269,7 +269,7 @@ static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
>  	struct page	     *vmpage;
>  	unsigned long	    ra_flags;
>  	int		      result = 0;
> -	int		      fault_ret = 0;
> +	vm_fault_t		fault_ret = 0;

What odd indentation ...

[lustre-devel] [PATCH] staging: lustre: Change return type to vm_fault_t

[Souptick Joarder]

On Sun, Apr 22, 2018 at 8:50 AM, Matthew Wilcox <willy@infradead.org> wrote:
> On Sun, Apr 22, 2018 at 03:47:24AM +0530, Souptick Joarder wrote:
>> @@ -261,7 +261,7 @@ static inline int to_fault_error(int result)
>>   * \retval VM_FAULT_ERROR on general error
>>   * \retval NOPAGE_OOM not have memory for allocate new page
>>   */
>> -static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
>> +static vm_fault_t ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
>>  {
>>       struct lu_env      *env;
>>       struct cl_io        *io;
>
> Did you compile-test this with the sparse changes?  Because I can see
> a problem here:

Yes, compile-tested. Sparse didn't throw any warning/error.

>
>         env = cl_env_get(&refcheck);
>         if (IS_ERR(env))
>                 return PTR_ERR(env);
>
>> @@ -269,7 +269,7 @@ static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
>>       struct page          *vmpage;
>>       unsigned long       ra_flags;
>>       int                   result = 0;
>> -     int                   fault_ret = 0;
>> +     vm_fault_t              fault_ret = 0;
>
> What odd indentation ...

Sorry, will correct it.
>

[lustre-devel] [PATCH] staging: lustre: Change return type to vm_fault_t

[Matthew Wilcox]

On Mon, Apr 23, 2018 at 10:12:30PM +0530, Souptick Joarder wrote:
> On Sun, Apr 22, 2018 at 8:50 AM, Matthew Wilcox <willy@infradead.org> wrote:
> > On Sun, Apr 22, 2018 at 03:47:24AM +0530, Souptick Joarder wrote:
> >> @@ -261,7 +261,7 @@ static inline int to_fault_error(int result)
> >>   * \retval VM_FAULT_ERROR on general error
> >>   * \retval NOPAGE_OOM not have memory for allocate new page
> >>   */
> >> -static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
> >> +static vm_fault_t ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
> >>  {
> >>       struct lu_env      *env;
> >>       struct cl_io        *io;
> >
> > Did you compile-test this with the sparse changes?  Because I can see
> > a problem here:
> 
> Yes, compile-tested. Sparse didn't throw any warning/error.

I think you need to check your setup ... I get this:

drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31: warning: incorrect type in return expression (different base types)
drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31:    expected restricted vm_fault_t
drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31:    got long

[lustre-devel] [PATCH] staging: lustre: Change return type to vm_fault_t

[Souptick Joarder]

On Mon, Apr 23, 2018 at 11:05 PM, Matthew Wilcox <willy@infradead.org> wrote:
> On Mon, Apr 23, 2018 at 10:12:30PM +0530, Souptick Joarder wrote:
>> On Sun, Apr 22, 2018 at 8:50 AM, Matthew Wilcox <willy@infradead.org> wrote:
>> > On Sun, Apr 22, 2018 at 03:47:24AM +0530, Souptick Joarder wrote:
>> >> @@ -261,7 +261,7 @@ static inline int to_fault_error(int result)
>> >>   * \retval VM_FAULT_ERROR on general error
>> >>   * \retval NOPAGE_OOM not have memory for allocate new page
>> >>   */
>> >> -static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
>> >> +static vm_fault_t ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
>> >>  {
>> >>       struct lu_env      *env;
>> >>       struct cl_io        *io;
>> >
>> > Did you compile-test this with the sparse changes?  Because I can see
>> > a problem here:
>>
>> Yes, compile-tested. Sparse didn't throw any warning/error.
>
> I think you need to check your setup ... I get this:
>
> drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31: warning: incorrect type in return expression (different base types)
> drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31:    expected restricted vm_fault_t
> drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31:    got long

What I am trying is -
after applying the patch in 4.17-rc1, enable the configuration
for this driver and do "make c=1/2 -j8"

Let me verify again.

[lustre-devel] [PATCH] staging: lustre: Change return type to vm_fault_t

[Matthew Wilcox]

On Mon, Apr 23, 2018 at 11:38:17PM +0530, Souptick Joarder wrote:
> On Mon, Apr 23, 2018 at 11:05 PM, Matthew Wilcox <willy@infradead.org> wrote:
> > I think you need to check your setup ... I get this:
> >
> > drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31: warning: incorrect type in return expression (different base types)
> > drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31:    expected restricted vm_fault_t
> > drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31:    got long
> 
> What I am trying is -
> after applying the patch in 4.17-rc1, enable the configuration
> for this driver and do "make c=1/2 -j8"
> 
> Let me verify again.

Yep, but you need to add the sparse annotations to make vm_fault_t a
separate type.  In case you've lost that patch ...

diff --git a/include/linux/mm.h b/include/linux/mm.h
index ad06d42adb1a..9ac6d2eb633f 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1174,52 +1175,6 @@ static inline void clear_page_pfmemalloc(struct page *page)
 	page->index = 0;
 }
 
-/*
- * Different kinds of faults, as returned by handle_mm_fault().
- * Used to decide whether a process gets delivered SIGBUS or
- * just gets major/minor fault counters bumped up.
- */
-
-#define VM_FAULT_OOM	0x0001
-#define VM_FAULT_SIGBUS	0x0002
-#define VM_FAULT_MAJOR	0x0004
-#define VM_FAULT_WRITE	0x0008	/* Special case for get_user_pages */
-#define VM_FAULT_HWPOISON 0x0010	/* Hit poisoned small page */
-#define VM_FAULT_HWPOISON_LARGE 0x0020  /* Hit poisoned large page. Index encoded in upper bits */
-#define VM_FAULT_SIGSEGV 0x0040
-
-#define VM_FAULT_NOPAGE	0x0100	/* ->fault installed the pte, not return page */
-#define VM_FAULT_LOCKED	0x0200	/* ->fault locked the returned page */
-#define VM_FAULT_RETRY	0x0400	/* ->fault blocked, must retry */
-#define VM_FAULT_FALLBACK 0x0800	/* huge page fault failed, fall back to small */
-#define VM_FAULT_DONE_COW   0x1000	/* ->fault has fully handled COW */
-#define VM_FAULT_NEEDDSYNC  0x2000	/* ->fault did not modify page tables
-					 * and needs fsync() to complete (for
-					 * synchronous page faults in DAX) */
-
-#define VM_FAULT_ERROR	(VM_FAULT_OOM | VM_FAULT_SIGBUS | VM_FAULT_SIGSEGV | \
-			 VM_FAULT_HWPOISON | VM_FAULT_HWPOISON_LARGE | \
-			 VM_FAULT_FALLBACK)
-
-#define VM_FAULT_RESULT_TRACE \
-	{ VM_FAULT_OOM,			"OOM" }, \
-	{ VM_FAULT_SIGBUS,		"SIGBUS" }, \
-	{ VM_FAULT_MAJOR,		"MAJOR" }, \
-	{ VM_FAULT_WRITE,		"WRITE" }, \
-	{ VM_FAULT_HWPOISON,		"HWPOISON" }, \
-	{ VM_FAULT_HWPOISON_LARGE,	"HWPOISON_LARGE" }, \
-	{ VM_FAULT_SIGSEGV,		"SIGSEGV" }, \
-	{ VM_FAULT_NOPAGE,		"NOPAGE" }, \
-	{ VM_FAULT_LOCKED,		"LOCKED" }, \
-	{ VM_FAULT_RETRY,		"RETRY" }, \
-	{ VM_FAULT_FALLBACK,		"FALLBACK" }, \
-	{ VM_FAULT_DONE_COW,		"DONE_COW" }, \
-	{ VM_FAULT_NEEDDSYNC,		"NEEDDSYNC" }
-
-/* Encode hstate index for a hwpoisoned large page */
-#define VM_FAULT_SET_HINDEX(x) ((x) << 12)
-#define VM_FAULT_GET_HINDEX(x) (((x) >> 12) & 0xf)
-
 /*
  * Can be called by the pagefault handler when it gets a VM_FAULT_OOM.
  */
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index fd1af6b9591d..5265f425fe88 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -610,6 +610,54 @@ static inline bool mm_tlb_flush_nested(struct mm_struct *mm)
 
 struct vm_fault;
 
+/*
+ * Different kinds of faults, as returned from fault handlers.
+ * Used to decide whether a process gets delivered SIGBUS or
+ * just gets major/minor fault counters bumped up.
+ */
+typedef __bitwise unsigned int vm_fault_t;
+enum {
+	VM_FAULT_OOM		= (__force vm_fault_t)0x000001,	/* Out Of Memory */
+	VM_FAULT_SIGBUS		= (__force vm_fault_t)0x000002,	/* Bad access */
+	VM_FAULT_MAJOR		= (__force vm_fault_t)0x000004,	/* Page read from storage */
+	VM_FAULT_WRITE		= (__force vm_fault_t)0x000008,	/* Special case for get_user_pages */
+	VM_FAULT_HWPOISON	= (__force vm_fault_t)0x000010,	/* Hit poisoned small page */
+	VM_FAULT_HWPOISON_LARGE = (__force vm_fault_t)0x000020,	/* Hit poisoned large page. Index encoded in upper bits */
+	VM_FAULT_SIGSEGV	= (__force vm_fault_t)0x000040,
+	VM_FAULT_NOPAGE		= (__force vm_fault_t)0x000100,	/* ->fault installed the pte, not return page */
+	VM_FAULT_LOCKED		= (__force vm_fault_t)0x000200,	/* ->fault locked the returned page */
+	VM_FAULT_RETRY		= (__force vm_fault_t)0x000400,	/* ->fault blocked, must retry */
+	VM_FAULT_FALLBACK	= (__force vm_fault_t)0x000800,	/* huge page fault failed, fall back to small */
+	VM_FAULT_DONE_COW	= (__force vm_fault_t)0x001000,	/* ->fault has fully handled COW */
+	VM_FAULT_NEEDDSYNC	= (__force vm_fault_t)0x002000,	/* ->fault did not modify page tables
+					 * and needs fsync() to complete (for
+					 * synchronous page faults in DAX) */
+	VM_FAULT_HINDEX_MASK	= (__force vm_fault_t)0x0f0000,
+};
+
+/* Encode hstate index for a hwpoisoned large page */
+#define VM_FAULT_SET_HINDEX(x) ((x) << 16)
+#define VM_FAULT_GET_HINDEX(x) (((x) >> 16) & 0xf)
+
+#define VM_FAULT_ERROR	(VM_FAULT_OOM | VM_FAULT_SIGBUS | VM_FAULT_SIGSEGV | \
+			 VM_FAULT_HWPOISON | VM_FAULT_HWPOISON_LARGE | \
+			 VM_FAULT_FALLBACK)
+
+#define VM_FAULT_RESULT_TRACE \
+	{ VM_FAULT_OOM,			"OOM" }, \
+	{ VM_FAULT_SIGBUS,		"SIGBUS" }, \
+	{ VM_FAULT_MAJOR,		"MAJOR" }, \
+	{ VM_FAULT_WRITE,		"WRITE" }, \
+	{ VM_FAULT_HWPOISON,		"HWPOISON" }, \
+	{ VM_FAULT_HWPOISON_LARGE,	"HWPOISON_LARGE" }, \
+	{ VM_FAULT_SIGSEGV,		"SIGSEGV" }, \
+	{ VM_FAULT_NOPAGE,		"NOPAGE" }, \
+	{ VM_FAULT_LOCKED,		"LOCKED" }, \
+	{ VM_FAULT_RETRY,		"RETRY" }, \
+	{ VM_FAULT_FALLBACK,		"FALLBACK" }, \
+	{ VM_FAULT_DONE_COW,		"DONE_COW" }, \
+	{ VM_FAULT_NEEDDSYNC,		"NEEDDSYNC" }
+
 struct vm_special_mapping {
 	const char *name;	/* The name, e.g. "[vdso]". */