* Re: + git-nfs-vs-nfs-convert-to-new-aops.patch added to -mm tree
[not found] <200708202301.l7KN1GAH028291@imap1.linux-foundation.org>
@ 2007-09-20 11:20 ` Peter Zijlstra
2007-09-20 12:37 ` Trond Myklebust
2007-09-24 13:53 ` Peter Zijlstra
0 siblings, 2 replies; 3+ messages in thread
From: Peter Zijlstra @ 2007-09-20 11:20 UTC (permalink / raw)
To: linux-kernel; +Cc: akpm, mm-commits, nickpiggin, trond.myklebust
On Mon, 20 Aug 2007 15:56:10 -0700 akpm@linux-foundation.org wrote:
> ------------------------------------------------------
> Subject: git-nfs vs nfs-convert-to-new-aops
> From: Andrew Morton <akpm@linux-foundation.org>
>
> nfi if this is correct. How am I supposed to know how to work out what to put
> in `copied' in write_end?
I can has broken NFS :-)
nfs_write_begin wants to lock the page itself, but we pass it a locked
page.
> Cc: Nick Piggin <nickpiggin@yahoo.com.au>
> Cc: Trond Myklebust <trond.myklebust@fys.uio.no>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
> 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;
}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: + git-nfs-vs-nfs-convert-to-new-aops.patch added to -mm tree
2007-09-20 11:20 ` + git-nfs-vs-nfs-convert-to-new-aops.patch added to -mm tree Peter Zijlstra
@ 2007-09-20 12:37 ` Trond Myklebust
2007-09-24 13:53 ` Peter Zijlstra
1 sibling, 0 replies; 3+ messages in thread
From: Trond Myklebust @ 2007-09-20 12:37 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: linux-kernel, akpm, mm-commits, nickpiggin
On Thu, 2007-09-20 at 13:20 +0200, Peter Zijlstra wrote:
> > Cc: Nick Piggin <nickpiggin@yahoo.com.au>
> > Cc: Trond Myklebust <trond.myklebust@fys.uio.no>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> >
> > 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: + git-nfs-vs-nfs-convert-to-new-aops.patch added to -mm tree
2007-09-20 11:20 ` + git-nfs-vs-nfs-convert-to-new-aops.patch added to -mm tree Peter Zijlstra
2007-09-20 12:37 ` Trond Myklebust
@ 2007-09-24 13:53 ` Peter Zijlstra
1 sibling, 0 replies; 3+ messages in thread
From: Peter Zijlstra @ 2007-09-24 13:53 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, akpm, mm-commits, nickpiggin, trond.myklebust
On Thu, 20 Sep 2007 13:20:47 +0200 Peter Zijlstra
<peterz@infradead.org> wrote:
> /me continues the mmap write on nfs adventure...
My test prog reliably hangs like so:
mm_tester D 000000000040b305 0 2701 2699
6042cef0 602ca520 617dfa50 617de000 617dfa90 60010b62 617dfa80 6002785d
617de000 60583840 6042bcc0 6042ca40 617dfad0 6019e3c2 00080000 617de000
617dfae0 ffffc0e7 617dfb38 00000002 617dfb60 6019eb20 616e7ae0 6048bd00 Call Trace:
Call Trace:
617dfa58: [<60010b62>] _switch_to+0x81/0xf5
617dfa68: [<6002785d>] pick_next_entity+0x1a/0x38
617dfa98: [<6019e3c2>] schedule+0x1b8/0x23f
617dfad8: [<6019eb20>] schedule_timeout+0xa2/0xcb
617dfaf8: [<60035dc3>] process_timeout+0x0/0xb
617dfb10: [<6019eb1b>] schedule_timeout+0x9d/0xcb
617dfb68: [<6019ea62>] io_schedule_timeout+0xf/0x17
617dfb78: [<6004f0d1>] sync_page+0x6b/0x6f
617dfb88: [<6019ecd3>] __wait_on_bit_lock+0x42/0x78
617dfbb0: [<6004fa2a>] find_lock_page+0xb4/0x155
617dfbc8: [<6004f806>] __lock_page+0x73/0xb3
617dfbf0: [<60040585>] wake_bit_function+0x0/0x2a
617dfc38: [<6004fa3c>] find_lock_page+0xc6/0x155
617dfc48: [<600570c9>] do_page_cache_readahead+0x52/0x5f
617dfc78: [<600508ea>] filemap_fault+0x151/0x2c2
617dfce8: [<6005e9c8>] __do_fault+0x6c/0x444
617dfd68: [<6005edd1>] do_linear_fault+0x31/0x33
617dfd88: [<6005f04e>] handle_mm_fault+0x130/0x228
617dfda8: [<6011e2d7>] __up_read+0x73/0x7b
617dfde8: [<600131d4>] handle_page_fault+0x120/0x2d9
617dfe08: [<601242c8>] tty_write+0x1f7/0x212
617dfe48: [<60013513>] segv+0xac/0x286
617dff28: [<60013461>] segv_handler+0x68/0x6e
617dff48: [<600232c9>] get_skas_faultinfo+0x9c/0xa1
617dff68: [<6002386f>] userspace+0x13a/0x19d
617dffc8: [<60010d4c>] fork_handler+0x86/0x8d
A new nfs_sync_page() method tells me:
sleeping on page: 0000000060ba05c0 held by: [<000000006004f5d1>] add_to_page_cache_lru+0xf/0x3a
And a rather crude printk() and dump_stack() in add_to_page_cache_lru()
match:
page: 0000000060ba05c0
Call Trace:
605eda88: [<6004f5f4>] add_to_page_cache_lru+0x32/0x3a
605edaa8: [<60056ddc>] read_cache_pages+0x4a/0x8f
605edae8: [<600f8e49>] nfs_readpages+0x116/0x164
605edb38: [<600f86bb>] nfs_pagein_one+0x0/0xd2
605edb98: [<60056e58>] read_pages+0x37/0x9b
605edbd8: [<60056fbc>] __do_page_cache_readahead+0x100/0x146
605edc48: [<600570dd>] do_page_cache_readahead+0x52/0x5f
605edc78: [<600508f4>] filemap_fault+0x145/0x2c2
605edca8: [<60022b7d>] run_syscall_stub+0xd1/0xdd
605edce8: [<6005e9dc>] __do_fault+0x6c/0x444
605edd68: [<6005ede5>] do_linear_fault+0x31/0x33
605edd88: [<6005f062>] handle_mm_fault+0x130/0x228
605edda8: [<6011e2eb>] __up_read+0x73/0x7b
605edde8: [<600131d4>] handle_page_fault+0x120/0x2d9
605ede08: [<601242dc>] tty_write+0x1f7/0x212
605ede48: [<60013513>] segv+0xac/0x286
605edf28: [<60013461>] segv_handler+0x68/0x6e
605edf48: [<600232c9>] get_skas_faultinfo+0x9c/0xa1
605edf68: [<6002386f>] userspace+0x13a/0x19d
605edfc8: [<60010d4c>] fork_handler+0x86/0x8d
/me wonders, missing RPC request or locking mistake...
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2007-09-24 13:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <200708202301.l7KN1GAH028291@imap1.linux-foundation.org>
2007-09-20 11:20 ` + git-nfs-vs-nfs-convert-to-new-aops.patch added to -mm tree Peter Zijlstra
2007-09-20 12:37 ` Trond Myklebust
2007-09-24 13:53 ` Peter Zijlstra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox