Re: Odd NFS related SIGBUS (& possible fix)

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Fri, 2010-10-01 at 14:35 -0400, Trond Myklebust wrote:
> On Fri, 2010-10-01 at 14:12 -0400, Trond Myklebust wrote:
> > However, it looks to me as if the right thing to do when the
> > page->mapping has changed would be to do the same thing as
> > block_page_mkwrite(), and just return VM_FAULT_NOPAGE so that the VM can
> > retry the fault.
> > IMO: We should only SIGBUS if the calls to nfs_flush_incompatible()
> > and/or nfs_updatepage() fail.
> > 
> > Cheers
> >   Trond
> 
> IOW: Something like the following patch.

Thanks, I'll try it but it will have to wait about 10 days as I'm off on
vacation.

I'll see if somebody here can do some tests while I'm away

Cheers,
Ben.

> Cheers
>   Trond
> 
> ---------------------------------------------------------------------------
> NFS: Don't SIGBUS if nfs_vm_page_mkwrite races with a cache invalidation
> 
> From: Trond Myklebust <Trond.Myklebust@netapp.com>
> 
> In the case where we lock the page, and then find out that the page has
> been thrown out of the page cache, we should just return VM_FAULT_NOPAGE.
> This is what block_page_mkwrite() does in these situations.
> 
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
> 
>  fs/nfs/file.c |   17 ++++++++---------
>  1 files changed, 8 insertions(+), 9 deletions(-)
> 
> 
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index 05bf3c0..6d95e24 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -551,7 +551,7 @@ static int nfs_vm_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
>  	struct file *filp = vma->vm_file;
>  	struct dentry *dentry = filp->f_path.dentry;
>  	unsigned pagelen;
> -	int ret = -EINVAL;
> +	int ret = VM_FAULT_NOPAGE;
>  	struct address_space *mapping;
>  
>  	dfprintk(PAGECACHE, "NFS: vm_page_mkwrite(%s/%s(%ld), offset %lld)\n",
> @@ -567,21 +567,20 @@ static int nfs_vm_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
>  	if (mapping != dentry->d_inode->i_mapping)
>  		goto out_unlock;
>  
> -	ret = 0;
>  	pagelen = nfs_page_length(page);
>  	if (pagelen == 0)
>  		goto out_unlock;
>  
> -	ret = nfs_flush_incompatible(filp, page);
> -	if (ret != 0)
> -		goto out_unlock;
> +	ret = VM_FAULT_LOCKED;
> +	if (nfs_flush_incompatible(filp, page) == 0 &&
> +	    nfs_updatepage(filp, page, 0, pagelen) == 0)
> +		goto out;
>  
> -	ret = nfs_updatepage(filp, page, 0, pagelen);
> +	ret = VM_FAULT_SIGBUS;
>  out_unlock:
> -	if (!ret)
> -		return VM_FAULT_LOCKED;
>  	unlock_page(page);
> -	return VM_FAULT_SIGBUS;
> +out:
> +	return ret;
>  }
>  
>  static const struct vm_operations_struct nfs_file_vm_ops = {
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/