* 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
Powered by JetHome