[PATCH 1/2] Add new spin_lock for i_flags of xfs_inode [try #2]

[PATCH 1/2] Add new spin_lock for i_flags of xfs_inode [try #2]

[Masayuki Saito]

It is the problem that i_flags of xfs_inode has no consistent
locking protection.

For the reason, I define a new spin_lock(i_flags_lock) for i_flags
of xfs_inode.  And I add this spin_lock for appropriate places.

I divided into two patches.  A summary of patches is as follows.

 [1/2] add a new lock for i_flags of xfs_inode
 [2/2] fix i_state of the inode is changed after the inode is freed

The following changes were made in [try #2]:
 [1/2] Nothing
 [2/2] wake_up() is done before iput() in xfs_iunpin().

I have tested the two patches and confirmed with these patches that
the inode of i_state is not changed after the inode is freed.

Appreciate any comments and feedbacks.

Signed-off-by: Masayuki Saito <m-saito@tnes.nec.co.jp>
Signed-off-by: ASANO Masahiro <masano@tnes.nec.co.jp>
---

--- linux-2.6.17.7/fs/xfs/xfs_inode.h.orig	2006-07-29 15:41:28.570341641 +0900
+++ linux-2.6.17.7/fs/xfs/xfs_inode.h	2006-08-01 02:51:53.598587423 +0900
@@ -267,6 +267,7 @@ typedef struct xfs_inode {
 	sema_t			i_flock;	/* inode flush lock */
 	atomic_t		i_pincount;	/* inode pin count */
 	wait_queue_head_t	i_ipin_wait;	/* inode pinning wait queue */
+	spinlock_t		i_flags_lock;	/* inode i_flags lock */
 #ifdef HAVE_REFCACHE
 	struct xfs_inode	**i_refcache;	/* ptr to entry in ref cache */
 	struct xfs_inode	*i_release;	/* inode to unref */
--- linux-2.6.17.7/fs/xfs/xfs_inode.c.orig	2006-07-29 15:41:39.050936181 +0900
+++ linux-2.6.17.7/fs/xfs/xfs_inode.c	2006-08-01 14:13:51.615782806 +0900
@@ -861,6 +861,7 @@ xfs_iread(
 	ip = kmem_zone_zalloc(xfs_inode_zone, KM_SLEEP);
 	ip->i_ino = ino;
 	ip->i_mount = mp;
+	spin_lock_init(&ip->i_flags_lock);
 
 	/*
 	 * Get pointer's to the on-disk inode and the buffer containing it.
@@ -2204,7 +2205,9 @@ xfs_ifree_cluster(
 
 			if (ip == free_ip) {
 				if (xfs_iflock_nowait(ip)) {
+					spin_lock(&ip->i_flags_lock);
 					ip->i_flags |= XFS_ISTALE;
+					spin_unlock(&ip->i_flags_lock);
 
 					if (xfs_inode_clean(ip)) {
 						xfs_ifunlock(ip);
@@ -2218,7 +2221,9 @@ xfs_ifree_cluster(
 
 			if (xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) {
 				if (xfs_iflock_nowait(ip)) {
+					spin_lock(&ip->i_flags_lock);
 					ip->i_flags |= XFS_ISTALE;
+					spin_unlock(&ip->i_flags_lock);
 
 					if (xfs_inode_clean(ip)) {
 						xfs_ifunlock(ip);
@@ -2248,7 +2253,9 @@ xfs_ifree_cluster(
 				AIL_LOCK(mp,s);
 				iip->ili_flush_lsn = iip->ili_item.li_lsn;
 				AIL_UNLOCK(mp, s);
+				spin_lock(&iip->ili_inode->i_flags_lock);
 				iip->ili_inode->i_flags |= XFS_ISTALE;
+				spin_unlock(&iip->ili_inode->i_flags_lock);
 				pre_flushed++;
 			}
 			lip = lip->li_bio_list;
--- linux-2.6.17.7/fs/xfs/xfs_itable.c.orig	2006-07-29 15:41:52.071674818 +0900
+++ linux-2.6.17.7/fs/xfs/xfs_itable.c	2006-08-01 02:51:53.698593096 +0900
@@ -559,6 +559,7 @@ xfs_bulkstat(
 								      KM_SLEEP);
 						ip->i_ino = ino;
 						ip->i_mount = mp;
+						spin_lock_init(&ip->i_flags_lock);
 						if (bp)
 							xfs_buf_relse(bp);
 						error = xfs_itobp(mp, NULL, ip,
--- linux-2.6.17.7/fs/xfs/xfs_iget.c.orig	2006-07-29 15:42:03.992351051 +0900
+++ linux-2.6.17.7/fs/xfs/xfs_iget.c	2006-08-01 21:17:58.173213088 +0900
@@ -246,7 +246,9 @@ again:
 
 				XFS_STATS_INC(xs_ig_found);
 
+				spin_lock(&ip->i_flags_lock);
 				ip->i_flags &= ~XFS_IRECLAIMABLE;
+				spin_unlock(&ip->i_flags_lock);
 				version = ih->ih_version;
 				read_unlock(&ih->ih_lock);
 				xfs_ihash_promote(ih, ip, version);
@@ -300,7 +302,9 @@ finish_inode:
 			if (lock_flags != 0)
 				xfs_ilock(ip, lock_flags);
 
+			spin_lock(&ip->i_flags_lock);
 			ip->i_flags &= ~XFS_ISTALE;
+			spin_unlock(&ip->i_flags_lock);
 
 			vn_trace_exit(vp, "xfs_iget.found",
 						(inst_t *)__return_address);
@@ -371,7 +375,9 @@ finish_inode:
 	ih->ih_next = ip;
 	ip->i_udquot = ip->i_gdquot = NULL;
 	ih->ih_version++;
+	spin_lock(&ip->i_flags_lock);
 	ip->i_flags |= XFS_INEW;
+	spin_unlock(&ip->i_flags_lock);
 
 	write_unlock(&ih->ih_lock);
 
--- linux-2.6.17.7/fs/xfs/xfs_vnodeops.c.orig	2006-07-29 15:42:40.182404031 +0900
+++ linux-2.6.17.7/fs/xfs/xfs_vnodeops.c	2006-08-02 03:43:12.163597123 +0900
@@ -3836,7 +3836,9 @@ xfs_reclaim(
 		XFS_MOUNT_ILOCK(mp);
 		vn_bhv_remove(VN_BHV_HEAD(vp), XFS_ITOBHV(ip));
 		list_add_tail(&ip->i_reclaim, &mp->m_del_inodes);
+		spin_lock(&ip->i_flags_lock);
 		ip->i_flags |= XFS_IRECLAIMABLE;
+		spin_unlock(&ip->i_flags_lock);
 		XFS_MOUNT_IUNLOCK(mp);
 	}
 	return 0;
@@ -3861,8 +3863,10 @@ xfs_finish_reclaim(
 	 * us.
 	 */
 	write_lock(&ih->ih_lock);
+	spin_lock(&ip->i_flags_lock);
 	if ((ip->i_flags & XFS_IRECLAIM) ||
 	    (!(ip->i_flags & XFS_IRECLAIMABLE) && vp == NULL)) {
+		spin_unlock(&ip->i_flags_lock);
 		write_unlock(&ih->ih_lock);
 		if (locked) {
 			xfs_ifunlock(ip);
@@ -3871,6 +3875,7 @@ xfs_finish_reclaim(
 		return 1;
 	}
 	ip->i_flags |= XFS_IRECLAIM;
+	spin_unlock(&ip->i_flags_lock);
 	write_unlock(&ih->ih_lock);
 
 	/*
--- linux-2.6.17.7/fs/xfs/linux-2.6/xfs_super.c.orig	2006-07-29 15:43:17.768536207 +0900
+++ linux-2.6.17.7/fs/xfs/linux-2.6/xfs_super.c	2006-08-01 13:23:09.934740893 +0900
@@ -230,7 +230,9 @@ xfs_initialize_vnode(
 		xfs_revalidate_inode(XFS_BHVTOM(bdp), vp, ip);
 		xfs_set_inodeops(inode);
 
+		spin_lock(&ip->i_flags_lock);
 		ip->i_flags &= ~XFS_INEW;
+		spin_unlock(&ip->i_flags_lock);
 		barrier();
 
 		unlock_new_inode(inode);

Re: [PATCH 1/2] Add new spin_lock for i_flags of xfs_inode [try #2]

[Andrew Morton]

On Wed, 23 Aug 2006 20:12:51 +0900
Masayuki Saito <m-saito@tnes.nec.co.jp> wrote:

> It is the problem that i_flags of xfs_inode has no consistent
> locking protection.
> 
> For the reason, I define a new spin_lock(i_flags_lock) for i_flags
> of xfs_inode.  And I add this spin_lock for appropriate places.

You could simply use inode.i_lock for this.  i_lock is a general-purpose
per-inode lock.  Its mandate is "use it for whatever you like, but it must
always be `innermost'"

Re: [PATCH 1/2] Add new spin_lock for i_flags of xfs_inode [try #2]

[Nathan Scott]

On Wed, Aug 23, 2006 at 09:38:17PM -0700, Andrew Morton wrote:
> On Wed, 23 Aug 2006 20:12:51 +0900
> Masayuki Saito <m-saito@tnes.nec.co.jp> wrote:
> 
> > It is the problem that i_flags of xfs_inode has no consistent
> > locking protection.
> > 
> > For the reason, I define a new spin_lock(i_flags_lock) for i_flags
> > of xfs_inode.  And I add this spin_lock for appropriate places.
> 
> You could simply use inode.i_lock for this.  i_lock is a general-purpose
> per-inode lock.  Its mandate is "use it for whatever you like, but it must
> always be `innermost'"

Sounds spot on for our needs here, and has the added benefit of
not increasing the size of the inode (as well as not adding to
our locking complexity).  Thanks!

cheers.

-- 
Nathan

Re: [PATCH 1/2] Add new spin_lock for i_flags of xfs_inode [try #2]

[Nathan Scott]

On Thu, Aug 24, 2006 at 03:42:45PM +1000, Nathan Scott wrote:
> On Wed, Aug 23, 2006 at 09:38:17PM -0700, Andrew Morton wrote:
> > On Wed, 23 Aug 2006 20:12:51 +0900
> > Masayuki Saito <m-saito@tnes.nec.co.jp> wrote:
> > 
> > > It is the problem that i_flags of xfs_inode has no consistent
> > > locking protection.
> > > 
> > > For the reason, I define a new spin_lock(i_flags_lock) for i_flags
> > > of xfs_inode.  And I add this spin_lock for appropriate places.
> > 
> > You could simply use inode.i_lock for this.  i_lock is a general-purpose
> > per-inode lock.  Its mandate is "use it for whatever you like, but it must
> > always be `innermost'"
> 
> Sounds spot on for our needs here, and has the added benefit of
> not increasing the size of the inode (as well as not adding to
> our locking complexity).  Thanks!

Oh, except that the generic inode has a different lifecycle to the
xfs_inode_t, which is going to prevent using this.  Doh.  I had also
looked at the other xfs_inode locks before, but not seen an easy way
to piggyback on those... perhaps a way could be found though.

cheers.

-- 
Nathan

Re: [PATCH 1/2] Add new spin_lock for i_flags of xfs_inode [try #2]

[Masayuki Saito]

Thank you for your comment.

Andrew Morton <akpm@osdl.org> wrote:
>On Wed, 23 Aug 2006 20:12:51 +0900
>Masayuki Saito <m-saito@tnes.nec.co.jp> wrote:
>
>> It is the problem that i_flags of xfs_inode has no consistent
>> locking protection.
>> 
>> For the reason, I define a new spin_lock(i_flags_lock) for i_flags
>> of xfs_inode.  And I add this spin_lock for appropriate places.
>
>You could simply use inode.i_lock for this.  i_lock is a general-purpose
>per-inode lock.  Its mandate is "use it for whatever you like, but it must
>always be `innermost'"
>

I think that inode.i_lock isn't appropriate for this case.
Because there is the situation that no inode is attached to an xfs_inode.


Masayuki