Re: + git-nfs-vs-nfs-convert-to-new-aops.patch added to -mm tree

[Trond Myklebust]

On Thu, 2007-09-20 at 13:20 +0200, Peter Zijlstra wrote:

> > Cc: Nick Piggin <nickpiggin@xxxxxxxxxxxx>
> > Cc: Trond Myklebust <trond.myklebust@xxxxxxxxxx>
> > Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > ---
> > 
> >  fs/nfs/file.c |    9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff -puN fs/nfs/file.c~git-nfs-vs-nfs-convert-to-new-aops fs/nfs/file.c
> > --- a/fs/nfs/file.c~git-nfs-vs-nfs-convert-to-new-aops
> > +++ a/fs/nfs/file.c
> > @@ -392,6 +392,7 @@ static int nfs_vm_page_mkwrite(struct vm
> >  	struct file *filp = vma->vm_file;
> >  	unsigned pagelen;
> >  	int ret = -EINVAL;
> > +	void *fsdata;
> >  
> >  	lock_page(page);
> >  	if (page->mapping != vma->vm_file->f_path.dentry->d_inode->i_mapping)
> > @@ -399,9 +400,13 @@ static int nfs_vm_page_mkwrite(struct vm
> >  	pagelen = nfs_page_length(page);
> >  	if (pagelen == 0)
> >  		goto out_unlock;
> > -	ret = nfs_prepare_write(filp, page, 0, pagelen);
> > +	ret = nfs_write_begin(filp, page->mapping,
> > +				(loff_t)page->index << PAGE_CACHE_SHIFT,
> > +				pagelen, 0, &page, &fsdata);
> >  	if (!ret)
> > -		ret = nfs_commit_write(filp, page, 0, pagelen);
> > +		ret = nfs_write_end(filp, page->mapping,
> > +				(loff_t)page->index << PAGE_CACHE_SHIFT,
> > +				pagelen, pagelen, page, fsdata);
> >  out_unlock:
> >  	unlock_page(page);
> >  	return ret;
> > _
> 
> But even with this patch I deadlock on page lock, just not here
> anymore :-/
> 
> /me continues the mmap write on nfs adventure...
> 
> ---
>  fs/nfs/file.c |   36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> Index: linux-2.6/fs/nfs/file.c
> ===================================================================
> --- linux-2.6.orig/fs/nfs/file.c
> +++ linux-2.6/fs/nfs/file.c
> @@ -393,22 +393,34 @@ static int nfs_vm_page_mkwrite(struct vm
>  	unsigned pagelen;
>  	int ret = -EINVAL;
>  	void *fsdata;
> +	struct address_space *mapping;
> +	loff_t offset;
>  
>  	lock_page(page);
> -	if (page->mapping != vma->vm_file->f_path.dentry->d_inode->i_mapping)
> -		goto out_unlock;
> +	mapping = page->mapping;
> +	if (mapping != vma->vm_file->f_path.dentry->d_inode->i_mapping) {
> +		unlock_page(page);
> +		return -EINVAL;
> +	}
>  	pagelen = nfs_page_length(page);
> -	if (pagelen == 0)
> -		goto out_unlock;
> -	ret = nfs_write_begin(filp, page->mapping,
> -				(loff_t)page->index << PAGE_CACHE_SHIFT,
> -				pagelen, 0, &page, &fsdata);
> -	if (!ret)
> -		ret = nfs_write_end(filp, page->mapping,
> -				(loff_t)page->index << PAGE_CACHE_SHIFT,
> -				pagelen, pagelen, page, fsdata);
> -out_unlock:
> +	offset = (loff_t)page->index << PAGE_CACHE_SHIFT;
>  	unlock_page(page);
> +
> +	/*
> +	 * we can use mapping after releasing the page lock, because:
> +	 * we hold mmap_sem on the fault path, which should pin the vma
> +	 * which should pin the file, which pins the dentry which should
> +	 * hold a reference on inode.
> +	 */
> +
> +	if (pagelen) {
> +		struct page *page2 = NULL;
> +		ret = nfs_write_begin(filp, mapping, offset, pagelen,
> +			       	0, &page2, &fsdata);
> +		if (!ret)
> +			ret = nfs_write_end(filp, mapping, offset, pagelen,
> +				       	pagelen, page2, fsdata);
> +	}
>  	return ret;
>  }

BTW: Ideally, we want to replace this with a "generic_vm_page_mkwrite()"
in mm/filemap.c(?). There is nothing here which is NFS-specific (except
for the fact that we hard-code the callbacks).

Cheers
  Trond

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/