LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jan Kara <jack@suse.cz>
To: Souptick Joarder <jrdr.linux@gmail.com>
Cc: viro@zeniv.linux.org.uk, mawilcox@microsoft.com,
	ross.zwisler@linux.intel.com, akpm@linux-foundation.org,
	dan.j.williams@intel.com, mhocko@suse.com, jack@suse.cz,
	kirill.shutemov@linux.intel.com, linux-fsdevel@vger.kernel.org,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] fs: dax: Adding new return type vm_fault_t
Date: Mon, 23 Apr 2018 01:09:48 +0200	[thread overview]
Message-ID: <20180422230948.2mvimlf3zspry4ji@quack2.suse.cz> (raw)
In-Reply-To: <20180421210529.GA27238@jordon-HP-15-Notebook-PC>

On Sun 22-04-18 02:35:29, Souptick Joarder wrote:
> 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")
> 
> There was an existing bug inside dax_load_hole()
> if vm_insert_mixed had failed to allocate a page table,
> we'd return VM_FAULT_NOPAGE instead of VM_FAULT_OOM.
> With new vmf_insert_mixed() this issue is addressed.
> 
> vm_insert_mixed_mkwrite has inefficiency when it returns
> an error value, driver has to convert it to vm_fault_t
> type. With new vmf_insert_mixed_mkwrite() this limitation
> will be addressed.
> 
> As new function vmf_insert_mixed_mkwrite() only called
> from fs/dax.c, so keeping both the changes in a single
> patch.
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>

The patch looks good to me. Just one question:

> diff --git a/mm/memory.c b/mm/memory.c
> index 01f5464..721cfd5 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1955,12 +1955,19 @@ int vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr,
>  }
>  EXPORT_SYMBOL(vm_insert_mixed);
>  
> -int vm_insert_mixed_mkwrite(struct vm_area_struct *vma, unsigned long addr,
> -			pfn_t pfn)
> +vm_fault_t vmf_insert_mixed_mkwrite(struct vm_area_struct *vma,
> +		unsigned long addr, pfn_t pfn)
>  {
> -	return __vm_insert_mixed(vma, addr, pfn, true);
> +	int err;
> +
> +	err =  __vm_insert_mixed(vma, addr, pfn, true);
> +	if (err == -ENOMEM)
> +		return VM_FAULT_OOM;
> +	if (err < 0 && err != -EBUSY)
> +		return VM_FAULT_SIGBUS;
> +	return VM_FAULT_NOPAGE;
>  }
> -EXPORT_SYMBOL(vm_insert_mixed_mkwrite);
> +EXPORT_SYMBOL(vmf_insert_mixed_mkwrite);

So are we sure that all the callers of this function (and also of
vmf_insert_mixed()) are OK with EBUSY? Because especially in the
vmf_insert_mixed() case other page than the caller provided is in page
tables and thus possibly the caller needs to do some error recovery (such
as drop page refcount) in such case...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

  parent reply	other threads:[~2018-04-22 23:09 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-21 21:05 Souptick Joarder
2018-04-21 21:34 ` Matthew Wilcox
2018-04-21 21:54   ` Souptick Joarder
2018-04-21 23:56     ` Dan Williams
2018-04-22 23:09 ` Jan Kara [this message]
2018-04-23  2:25   ` Matthew Wilcox
2018-04-23 13:59     ` Jan Kara
2018-04-23 16:12       ` Souptick Joarder
2018-04-23 17:28         ` Matthew Wilcox

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180422230948.2mvimlf3zspry4ji@quack2.suse.cz \
    --to=jack@suse.cz \
    --cc=akpm@linux-foundation.org \
    --cc=dan.j.williams@intel.com \
    --cc=jrdr.linux@gmail.com \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mawilcox@microsoft.com \
    --cc=mhocko@suse.com \
    --cc=ross.zwisler@linux.intel.com \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH v3] fs: dax: Adding new return type vm_fault_t' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).