* [PATCH] nfs: init req_lock in nfs_alloc_inode @ 2007-02-16 17:05 Olof Johansson 2007-02-17 0:15 ` Andrew Morton ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Olof Johansson @ 2007-02-16 17:05 UTC (permalink / raw) To: trond.myklebust; +Cc: linux-kernel Seems like req_lock is never initialized. CONFIG_DEBUG_SPINLOCK reported: BUG: spinlock bad magic on CPU#0, mount/1073 lock: c00000007fdca108, .magic: 00000000, .owner: /24576, .owner_cpu: 0 Call Trace: [C00000007E913750] [C0000000000107B4] .show_stack+0x54/0x1f0 (unreliable) [C00000007E913800] [C0000000001D2234] .spin_bug+0xa4/0x120 [C00000007E913890] [C0000000001D247C] ._raw_spin_lock+0xdc/0x1d0 [C00000007E913930] [C0000000004DF990] ._spin_lock+0x10/0x30 [C00000007E9139B0] [C00000000017623C] .nfs_sync_mapping_wait+0xac/0x310 [C00000007E913AA0] [C000000000176F18] .nfs_sync_mapping_range+0x98/0x110 [C00000007E913B80] [C00000000016CAA0] .nfs_getattr+0x40/0xf0 [C00000007E913C20] [C0000000000BE668] .vfs_getattr+0x38/0x70 [C00000007E913CA0] [C0000000000BF05C] .vfs_fstat+0x3c/0x70 [C00000007E913D30] [C0000000000BF0B8] .sys_fstat64+0x28/0x60 [C00000007E913E30] [C000000000008608] syscall_exit+0x0/0x40 Signed-off-by: Olof Johansson <olof@lixom.net> --- Trond, is your MAINTAINERS entry up to date? Seems like you mostly post from @netapp.com these days. Index: linux-2.6/fs/nfs/inode.c =================================================================== --- linux-2.6.orig/fs/nfs/inode.c +++ linux-2.6/fs/nfs/inode.c @@ -1123,6 +1123,7 @@ struct inode *nfs_alloc_inode(struct sup nfsi->flags = 0UL; nfsi->cache_validity = 0UL; nfsi->cache_change_attribute = jiffies; + nfsi->req_lock = SPIN_LOCK_UNLOCKED; #ifdef CONFIG_NFS_V3_ACL nfsi->acl_access = ERR_PTR(-EAGAIN); nfsi->acl_default = ERR_PTR(-EAGAIN); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfs: init req_lock in nfs_alloc_inode 2007-02-16 17:05 [PATCH] nfs: init req_lock in nfs_alloc_inode Olof Johansson @ 2007-02-17 0:15 ` Andrew Morton 2007-02-17 1:24 ` Andrew Morton 2007-02-20 15:10 ` Trond Myklebust 2 siblings, 0 replies; 8+ messages in thread From: Andrew Morton @ 2007-02-17 0:15 UTC (permalink / raw) To: Olof Johansson; +Cc: trond.myklebust, linux-kernel On Fri, 16 Feb 2007 11:05:32 -0600 olof@lixom.net (Olof Johansson) wrote: > + nfsi->req_lock = SPIN_LOCK_UNLOCKED; This can confound lockdep. Please use spin_lock_init(). ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfs: init req_lock in nfs_alloc_inode 2007-02-16 17:05 [PATCH] nfs: init req_lock in nfs_alloc_inode Olof Johansson 2007-02-17 0:15 ` Andrew Morton @ 2007-02-17 1:24 ` Andrew Morton 2007-02-18 20:32 ` Olof Johansson 2007-02-20 15:10 ` Trond Myklebust 2 siblings, 1 reply; 8+ messages in thread From: Andrew Morton @ 2007-02-17 1:24 UTC (permalink / raw) To: Olof Johansson; +Cc: trond.myklebust, linux-kernel On Fri, 16 Feb 2007 11:05:32 -0600 olof@lixom.net (Olof Johansson) wrote: > Seems like req_lock is never initialized. CONFIG_DEBUG_SPINLOCK reported: > > BUG: spinlock bad magic on CPU#0, mount/1073 > lock: c00000007fdca108, .magic: 00000000, .owner: /24576, .owner_cpu: 0 > Call Trace: > [C00000007E913750] [C0000000000107B4] .show_stack+0x54/0x1f0 (unreliable) > [C00000007E913800] [C0000000001D2234] .spin_bug+0xa4/0x120 > [C00000007E913890] [C0000000001D247C] ._raw_spin_lock+0xdc/0x1d0 > [C00000007E913930] [C0000000004DF990] ._spin_lock+0x10/0x30 > [C00000007E9139B0] [C00000000017623C] .nfs_sync_mapping_wait+0xac/0x310 > [C00000007E913AA0] [C000000000176F18] .nfs_sync_mapping_range+0x98/0x110 > [C00000007E913B80] [C00000000016CAA0] .nfs_getattr+0x40/0xf0 > [C00000007E913C20] [C0000000000BE668] .vfs_getattr+0x38/0x70 > [C00000007E913CA0] [C0000000000BF05C] .vfs_fstat+0x3c/0x70 > [C00000007E913D30] [C0000000000BF0B8] .sys_fstat64+0x28/0x60 > [C00000007E913E30] [C000000000008608] syscall_exit+0x0/0x40 > > > Signed-off-by: Olof Johansson <olof@lixom.net> > > --- > > Trond, is your MAINTAINERS entry up to date? Seems like you mostly post > from @netapp.com these days. > > > Index: linux-2.6/fs/nfs/inode.c > =================================================================== > --- linux-2.6.orig/fs/nfs/inode.c > +++ linux-2.6/fs/nfs/inode.c > @@ -1123,6 +1123,7 @@ struct inode *nfs_alloc_inode(struct sup > nfsi->flags = 0UL; > nfsi->cache_validity = 0UL; > nfsi->cache_change_attribute = jiffies; > + nfsi->req_lock = SPIN_LOCK_UNLOCKED; > #ifdef CONFIG_NFS_V3_ACL > nfsi->acl_access = ERR_PTR(-EAGAIN); > nfsi->acl_default = ERR_PTR(-EAGAIN); req_lock is initialsied in fs/nfs/inode.c:init_once(). What kernel version were you using? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfs: init req_lock in nfs_alloc_inode 2007-02-17 1:24 ` Andrew Morton @ 2007-02-18 20:32 ` Olof Johansson 0 siblings, 0 replies; 8+ messages in thread From: Olof Johansson @ 2007-02-18 20:32 UTC (permalink / raw) To: Andrew Morton; +Cc: trond.myklebust, linux-kernel On Fri, Feb 16, 2007 at 05:24:42PM -0800, Andrew Morton wrote: > req_lock is initialsied in fs/nfs/inode.c:init_once(). Oh, it is indeed. Grmbl. > What kernel version were you using? I've reproduced this on a base 2.6.20 g5_defconfig + NFS root and serial console options on a G5 here. The steps I have used are: * Boot with NFS root, default mount options * mount /dev/sda3 /mnt ... that's all. I have not seen it happen without NFS root, even with quite active NFS activity. So it seems to be a factor. I'll continue debugging tomorrow. -Olof ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfs: init req_lock in nfs_alloc_inode 2007-02-16 17:05 [PATCH] nfs: init req_lock in nfs_alloc_inode Olof Johansson 2007-02-17 0:15 ` Andrew Morton 2007-02-17 1:24 ` Andrew Morton @ 2007-02-20 15:10 ` Trond Myklebust 2007-02-20 17:23 ` Olof Johansson 2 siblings, 1 reply; 8+ messages in thread From: Trond Myklebust @ 2007-02-20 15:10 UTC (permalink / raw) To: Olof Johansson; +Cc: linux-kernel On Fri, 2007-02-16 at 11:05 -0600, Olof Johansson wrote: > Seems like req_lock is never initialized. CONFIG_DEBUG_SPINLOCK reported: > > BUG: spinlock bad magic on CPU#0, mount/1073 > lock: c00000007fdca108, .magic: 00000000, .owner: /24576, .owner_cpu: 0 > Call Trace: > [C00000007E913750] [C0000000000107B4] .show_stack+0x54/0x1f0 (unreliable) > [C00000007E913800] [C0000000001D2234] .spin_bug+0xa4/0x120 > [C00000007E913890] [C0000000001D247C] ._raw_spin_lock+0xdc/0x1d0 > [C00000007E913930] [C0000000004DF990] ._spin_lock+0x10/0x30 > [C00000007E9139B0] [C00000000017623C] .nfs_sync_mapping_wait+0xac/0x310 > [C00000007E913AA0] [C000000000176F18] .nfs_sync_mapping_range+0x98/0x110 > [C00000007E913B80] [C00000000016CAA0] .nfs_getattr+0x40/0xf0 > [C00000007E913C20] [C0000000000BE668] .vfs_getattr+0x38/0x70 > [C00000007E913CA0] [C0000000000BF05C] .vfs_fstat+0x3c/0x70 > [C00000007E913D30] [C0000000000BF0B8] .sys_fstat64+0x28/0x60 > [C00000007E913E30] [C000000000008608] syscall_exit+0x0/0x40 > > > Signed-off-by: Olof Johansson <olof@lixom.net> > > --- > > Trond, is your MAINTAINERS entry up to date? Seems like you mostly post > from @netapp.com these days. I tend to be easier to get hold of via the fys.uio.no address, since that isn't hidden behind a VPN. I use the netapp.com address when posting patches etc since that is sort of required by the developers certificate of origin. > Index: linux-2.6/fs/nfs/inode.c > =================================================================== > --- linux-2.6.orig/fs/nfs/inode.c > +++ linux-2.6/fs/nfs/inode.c > @@ -1123,6 +1123,7 @@ struct inode *nfs_alloc_inode(struct sup > nfsi->flags = 0UL; > nfsi->cache_validity = 0UL; > nfsi->cache_change_attribute = jiffies; > + nfsi->req_lock = SPIN_LOCK_UNLOCKED; > #ifdef CONFIG_NFS_V3_ACL > nfsi->acl_access = ERR_PTR(-EAGAIN); > nfsi->acl_default = ERR_PTR(-EAGAIN); As Andrew implied, this spin lock should already be initialised by the slab allocator in the "init_once" callback. Is this a vanilla kernel, or do you have any extra patches applied? Cheers Trond ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfs: init req_lock in nfs_alloc_inode 2007-02-20 15:10 ` Trond Myklebust @ 2007-02-20 17:23 ` Olof Johansson 2007-02-21 0:37 ` Trond Myklebust 0 siblings, 1 reply; 8+ messages in thread From: Olof Johansson @ 2007-02-20 17:23 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-kernel On Tue, Feb 20, 2007 at 10:10:00AM -0500, Trond Myklebust wrote: > > Trond, is your MAINTAINERS entry up to date? Seems like you mostly post > > from @netapp.com these days. > > I tend to be easier to get hold of via the fys.uio.no address, since > that isn't hidden behind a VPN. I use the netapp.com address when > posting patches etc since that is sort of required by the developers > certificate of origin. Ok, no problem. Just figured I'd ask. > > Index: linux-2.6/fs/nfs/inode.c > > =================================================================== > > --- linux-2.6.orig/fs/nfs/inode.c > > +++ linux-2.6/fs/nfs/inode.c > > @@ -1123,6 +1123,7 @@ struct inode *nfs_alloc_inode(struct sup > > nfsi->flags = 0UL; > > nfsi->cache_validity = 0UL; > > nfsi->cache_change_attribute = jiffies; > > + nfsi->req_lock = SPIN_LOCK_UNLOCKED; > > #ifdef CONFIG_NFS_V3_ACL > > nfsi->acl_access = ERR_PTR(-EAGAIN); > > nfsi->acl_default = ERR_PTR(-EAGAIN); > > As Andrew implied, this spin lock should already be initialised by the > slab allocator in the "init_once" callback. Is this a vanilla kernel, or > do you have any extra patches applied? Plain 2.6.20 on an old dual g5 with g5_defconfig + NFS_ROOT + serial port enabled. To see the error instead of lockup I enable CONFIG_DEBUG_SPINLOCK. In my original reproduction, I had to boot with nfs root, and try to mount my sata drive (/dev/sda3). This is with a static /dev, no udev. Seems like it happens when trying to mount any block device that's located on NFS. Since this is what nfs_sync_mapping_wait does: long nfs_sync_mapping_wait(struct address_space *mapping, struct writeback_control *wbc, int how) { struct inode *inode = mapping->host; struct nfs_inode *nfsi = NFS_I(inode); [...] spin_lock(&nfsi->req_lock); [...] I added this and it pops when mounting: @@ -421,6 +421,10 @@ int nfs_getattr(struct vfsmount *mnt, st int need_atime = NFS_I(inode)->cache_validity & NFS_INO_INVALID_ATIME; int err; + if (inode->i_mapping->host != inode) { + printk("inode %p host %p\n", inode, inode->i_mapping->host); + printk("inode_nfs %p host_nfs %p\n", NFS_I(inode), NFS_I(inode->i_mapping->host)); + } /* Flush out writes to the server in order to update c/mtime */ nfs_sync_mapping_range(inode->i_mapping, 0, 0, FLUSH_NOCOMMIT); I don't claim to know VFS internals, but doesn't it make sense that the device node is backed against the actual device, not an NFS inode? And if so, NFS can't expect to do nfs_sync_mapping_range() on it, or at least not dereference ->host and use it as an NFS inode, right? What I'm not sure I understand is why it disappears in the first place when I add the spin lock init -- I never even see the i_mapping->host pointer being allocated as an nfs inode. Maybe I just messed that one up somehow. -Olof ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfs: init req_lock in nfs_alloc_inode 2007-02-20 17:23 ` Olof Johansson @ 2007-02-21 0:37 ` Trond Myklebust 2007-02-22 20:13 ` Olof Johansson 0 siblings, 1 reply; 8+ messages in thread From: Trond Myklebust @ 2007-02-21 0:37 UTC (permalink / raw) To: Olof Johansson; +Cc: linux-kernel [-- Attachment #1: Type: text/plain, Size: 2189 bytes --] On Tue, 2007-02-20 at 11:23 -0600, Olof Johansson wrote: > In my original reproduction, I had to boot with nfs root, and try to mount > my sata drive (/dev/sda3). This is with a static /dev, no udev. Seems like it > happens when trying to mount any block device that's located on NFS. > > Since this is what nfs_sync_mapping_wait does: > > long nfs_sync_mapping_wait(struct address_space *mapping, struct > writeback_control *wbc, int how) { > struct inode *inode = mapping->host; > struct nfs_inode *nfsi = NFS_I(inode); > [...] > spin_lock(&nfsi->req_lock); > [...] > > I added this and it pops when mounting: > > @@ -421,6 +421,10 @@ int nfs_getattr(struct vfsmount *mnt, st > int need_atime = NFS_I(inode)->cache_validity & NFS_INO_INVALID_ATIME; > int err; > > + if (inode->i_mapping->host != inode) { > + printk("inode %p host %p\n", inode, inode->i_mapping->host); > + printk("inode_nfs %p host_nfs %p\n", NFS_I(inode), NFS_I(inode->i_mapping->host)); > + } > /* Flush out writes to the server in order to update c/mtime */ > nfs_sync_mapping_range(inode->i_mapping, 0, 0, FLUSH_NOCOMMIT); > > > I don't claim to know VFS internals, but doesn't it make sense that the > device node is backed against the actual device, not an NFS inode? And > if so, NFS can't expect to do nfs_sync_mapping_range() on it, or at > least not dereference ->host and use it as an NFS inode, right? NFS still has to manage the inode attributes and handle permissions. It is only when you open the device that the VFS takes over (see the call to init_special_inode() in nfs_fhget()). > What I'm not sure I understand is why it disappears in the first place > when I add the spin lock init -- I never even see the i_mapping->host > pointer being allocated as an nfs inode. Maybe I just messed that one > up somehow. Looks like we need a check in nfs_getattr() for a regular file. It makes no sense to call nfs_sync_mapping_range() on anything else. I think that should fix your problem: it will stop the NFS client from interfering with dirty pages on that inode's mapping. Cheers Trond [-- Attachment #2: linux-2.6.20-000-fix_block_device_getattr.dif --] [-- Type: message/rfc822, Size: 967 bytes --] From: Trond Myklebust <Trond.Myklebust@netapp.com> Subject: No Subject Date: Tue, 20 Feb 2007 19:28:07 -0500 Message-ID: <1172017790.6421.19.camel@heimdal.trondhjem.org> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> --- fs/nfs/inode.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c index af53c02..93d046c 100644 --- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -429,7 +429,8 @@ int nfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat) int err; /* Flush out writes to the server in order to update c/mtime */ - nfs_sync_mapping_range(inode->i_mapping, 0, 0, FLUSH_NOCOMMIT); + if (S_ISREG(inode->i_mode)) + nfs_sync_mapping_range(inode->i_mapping, 0, 0, FLUSH_NOCOMMIT); /* * We may force a getattr if the user cares about atime. ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] nfs: init req_lock in nfs_alloc_inode 2007-02-21 0:37 ` Trond Myklebust @ 2007-02-22 20:13 ` Olof Johansson 0 siblings, 0 replies; 8+ messages in thread From: Olof Johansson @ 2007-02-22 20:13 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-kernel On Tue, Feb 20, 2007 at 07:37:18PM -0500, Trond Myklebust wrote: > Looks like we need a check in nfs_getattr() for a regular file. It makes > no sense to call nfs_sync_mapping_range() on anything else. I think that > should fix your problem: it will stop the NFS client from interfering > with dirty pages on that inode's mapping. Yep, that works here. I also verified that my previous patch really didn't change the behaviour. I wonder why it did once. Probably just pure luck with memory contents. > From: Trond Myklebust <Trond.Myklebust@netapp.com> > Date: Tue, 20 Feb 2007 19:28:07 -0500 > Subject: No Subject > > Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> Acked-by: Olof Johansson <olof@lixom.net> -Olof ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2007-02-22 20:03 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-02-16 17:05 [PATCH] nfs: init req_lock in nfs_alloc_inode Olof Johansson 2007-02-17 0:15 ` Andrew Morton 2007-02-17 1:24 ` Andrew Morton 2007-02-18 20:32 ` Olof Johansson 2007-02-20 15:10 ` Trond Myklebust 2007-02-20 17:23 ` Olof Johansson 2007-02-21 0:37 ` Trond Myklebust 2007-02-22 20:13 ` Olof Johansson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.