[PATCH] remove v_number

[PATCH] remove v_number

[Christoph Hellwig]

v_number is unused except for the naming some locks (which is a
functionality totally unused by Linux), so remove it and assorted
crap.  Besides saving two words in struct vnode this also gets rid
of a spinlock per inode allocation.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/xfs/linux-2.6/xfs_vnode.c
===================================================================
--- linux-2.6.orig/fs/xfs/linux-2.6/xfs_vnode.c	2006-11-29 16:37:23.000000000 +0100
+++ linux-2.6/fs/xfs/linux-2.6/xfs_vnode.c	2006-11-29 16:38:09.000000000 +0100
@@ -17,8 +17,6 @@
  */
 #include "xfs.h"
 
-uint64_t vn_generation;		/* vnode generation number */
-DEFINE_SPINLOCK(vnumber_lock);
 
 /*
  * Dedicated vnode inactive/reclaim sync semaphores.
@@ -82,12 +80,6 @@
 	vp->v_flag = VMODIFIED;
 	spinlock_init(&vp->v_lock, "v_lock");
 
-	spin_lock(&vnumber_lock);
-	if (!++vn_generation)	/* v_number shouldn't be zero */
-		vn_generation++;
-	vp->v_number = vn_generation;
-	spin_unlock(&vnumber_lock);
-
 	ASSERT(VN_CACHED(vp) == 0);
 
 	/* Initialize the first behavior and the behavior chain head. */
Index: linux-2.6/fs/xfs/linux-2.6/xfs_vnode.h
===================================================================
--- linux-2.6.orig/fs/xfs/linux-2.6/xfs_vnode.h	2006-11-29 16:38:13.000000000 +0100
+++ linux-2.6/fs/xfs/linux-2.6/xfs_vnode.h	2006-11-29 16:39:08.000000000 +0100
@@ -41,7 +41,6 @@
 typedef struct bhv_vnode {
 	bhv_vflags_t	v_flag;			/* vnode flags (see above) */
 	bhv_vfs_t	*v_vfsp;		/* ptr to containing VFS */
-	bhv_vnumber_t	v_number;		/* in-core vnode number */
 	bhv_head_t	v_bh;			/* behavior head */
 	spinlock_t	v_lock;			/* VN_LOCK/VN_UNLOCK */
 	atomic_t	v_iocount;		/* outstanding I/O count */
Index: linux-2.6/fs/xfs/xfs_iget.c
===================================================================
--- linux-2.6.orig/fs/xfs/xfs_iget.c	2006-11-29 16:36:19.000000000 +0100
+++ linux-2.6/fs/xfs/xfs_iget.c	2006-11-29 16:37:14.000000000 +0100
@@ -570,8 +570,8 @@
 	bhv_vnode_t	*vp)
 {
 	mrlock_init(&ip->i_lock, MRLOCK_ALLOW_EQUAL_PRI|MRLOCK_BARRIER,
-		     "xfsino", (long)vp->v_number);
-	mrlock_init(&ip->i_iolock, MRLOCK_BARRIER, "xfsio", vp->v_number);
+		     "xfsino", ip->i_ino);
+	mrlock_init(&ip->i_iolock, MRLOCK_BARRIER, "xfsio", ip->i_ino);
 	init_waitqueue_head(&ip->i_ipin_wait);
 	atomic_set(&ip->i_pincount, 0);
 	initnsema(&ip->i_flock, 1, "xfsfino");

Re: [PATCH] remove v_number

[David Chinner]

On Wed, Nov 29, 2006 at 04:47:29PM +0100, Christoph Hellwig wrote:
> v_number is unused except for the naming some locks (which is a
> functionality totally unused by Linux), so remove it and assorted
> crap.  Besides saving two words in struct vnode this also gets rid
> of a spinlock per inode allocation.

Hmm - given that I've just used the v_number in post-mortem analysis
of a nasty bug to correlate the sequence of events during a series
of mkdir operations (i.e. transactions in the incore log buffers,
the resulting xfs_inodes and some screwed up dentries) that lead to
a BUG_ON being tripped in d_instantiate.

So, while it appears to be unused, it is _very_ useful for
determining the SOE that has occurred in certain types of problems.

FWIW, while analysing this crash dump a couple of days ago I was
wishing that dentries had an equivalent sequence number because
there is no way to tell what dentry was supposed to be related to
what inode after it got screwed up...

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [PATCH] remove v_number

[Christoph Hellwig]

On Thu, Nov 30, 2006 at 11:30:50AM +1100, David Chinner wrote:
> On Wed, Nov 29, 2006 at 04:47:29PM +0100, Christoph Hellwig wrote:
> > v_number is unused except for the naming some locks (which is a
> > functionality totally unused by Linux), so remove it and assorted
> > crap.  Besides saving two words in struct vnode this also gets rid
> > of a spinlock per inode allocation.
> 
> Hmm - given that I've just used the v_number in post-mortem analysis
> of a nasty bug to correlate the sequence of events during a series
> of mkdir operations (i.e. transactions in the incore log buffers,
> the resulting xfs_inodes and some screwed up dentries) that lead to
> a BUG_ON being tripped in d_instantiate.
> 
> So, while it appears to be unused, it is _very_ useful for
> determining the SOE that has occurred in certain types of problems.
> 
> FWIW, while analysing this crash dump a couple of days ago I was
> wishing that dentries had an equivalent sequence number because
> there is no way to tell what dentry was supposed to be related to
> what inode after it got screwed up...

Putting in sequence counting is trivial using kprobes.  Will you put
in this patch after I write you a kprobes modules to do the sequence
numbering?

Re: [PATCH] remove v_number

[David Chinner]

On Mon, Dec 04, 2006 at 12:34:06PM +0100, Christoph Hellwig wrote:
> On Thu, Nov 30, 2006 at 11:30:50AM +1100, David Chinner wrote:
> > On Wed, Nov 29, 2006 at 04:47:29PM +0100, Christoph Hellwig wrote:
> > > v_number is unused except for the naming some locks (which is a
> > > functionality totally unused by Linux), so remove it and assorted
> > > crap.  Besides saving two words in struct vnode this also gets rid
> > > of a spinlock per inode allocation.
> > 
> > Hmm - given that I've just used the v_number in post-mortem analysis
> > of a nasty bug to correlate the sequence of events during a series
> > of mkdir operations (i.e. transactions in the incore log buffers,
> > the resulting xfs_inodes and some screwed up dentries) that lead to
> > a BUG_ON being tripped in d_instantiate.
> > 
> > So, while it appears to be unused, it is _very_ useful for
> > determining the SOE that has occurred in certain types of problems.
> > 
> > FWIW, while analysing this crash dump a couple of days ago I was
> > wishing that dentries had an equivalent sequence number because
> > there is no way to tell what dentry was supposed to be related to
> > what inode after it got screwed up...
> 
> Putting in sequence counting is trivial using kprobes.

Never used kprobes - I'll take your word that it's easy....

> Will you put
> in this patch after I write you a kprobes modules to do the sequence
> numbering?

I wasn't NACKing your patch - I was making the point that this
apparently unused code does have some useful, non-obvious, redeeming
features.

Pardon my lack of kprobes knowledge, but what is the point of
removing this code if we have to then go and add kprobes
instrumentation to every XFS installation we support to provide the
same information? Where does the kprobes data get kept? In
the kernel, or partially in kernel and in userspace? How
would you correlate the sequence number and the vnode from the
crash dump? How much extra runtime memory does it use?

I'm not saying no, Christoph - I'm just trying to understand
the implications of what you are suggesting....

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group