[PATCH 2.6.6-mm2] vfs iput in inode critical region

[PATCH 2.6.6-mm2] vfs iput in inode critical region

[FabF]

[-- Attachment #1: Type: text/plain, Size: 231 bytes --]

Hi,

	AFAICS, block_dev.c : do_open calls bdput after an unlock_kernel.bdput
calls iput and iput plays with some parameters and locks in iput final
case only.Here's a patch to have a spinlock around the whole iput.

Regards,
FabF 

[-- Attachment #2: inodeputspin1.diff --]
[-- Type: text/x-patch, Size: 653 bytes --]

diff -Naur orig/fs/inode.c edited/fs/inode.c
--- orig/fs/inode.c	2004-05-11 10:13:15.000000000 +0200
+++ edited/fs/inode.c	2004-05-13 21:10:48.000000000 +0200
@@ -1081,16 +1081,20 @@
 void iput(struct inode *inode)
 {
 	if (inode) {
+		spin_lock(&inode_lock);
 		struct super_operations *op = inode->i_sb->s_op;
 
 		if (inode->i_state == I_CLEAR)
 			BUG();
 
+		/*Calling fs relevant put_inode*/
 		if (op && op->put_inode)
 			op->put_inode(inode);
 
-		if (atomic_dec_and_lock(&inode->i_count, &inode_lock))
+		/*finalize if inode is unused*/
+		if (atomic_dec_and_test(&inode->i_count))
 			iput_final(inode);
+		spin_unlock(&inode_lock);
 	}
 }
 

Re: [PATCH 2.6.6-mm2] vfs iput in inode critical region

[viro]

On Thu, May 13, 2004 at 09:26:36PM +0200, FabF wrote:
> Hi,
> 
> 	AFAICS, block_dev.c : do_open calls bdput after an unlock_kernel.bdput
> calls iput and iput plays with some parameters and locks in iput final
> case only.Here's a patch to have a spinlock around the whole iput.

Huh?  Of course iput() is called without BKL (and in a lot more places than
just that, actually), but why does it imply that we suddenly need to hold
inode_lock over the entire function?

Re: [PATCH 2.6.6-mm2] vfs iput in inode critical region

[FabF]

On Sat, 2004-05-15 at 00:47, viro@parcelfarce.linux.theplanet.co.uk
wrote:
> On Thu, May 13, 2004 at 09:26:36PM +0200, FabF wrote:
> > Hi,
> > 
> > 	AFAICS, block_dev.c : do_open calls bdput after an unlock_kernel.bdput
> > calls iput and iput plays with some parameters and locks in iput final
> > case only.Here's a patch to have a spinlock around the whole iput.
> 
> Huh?  Of course iput() is called without BKL (and in a lot more places than
> just that, actually), but why does it imply that we suddenly need to hold
> inode_lock over the entire function?
> 
What can avoid inode->i_state to change during fs put_inode is done plus
super_operations get assigned something unprotected as well.

Re: [PATCH 2.6.6-mm2] vfs iput in inode critical region

[viro]

On Thu, May 13, 2004 at 05:02:19PM +0200, FabF wrote:
> On Sat, 2004-05-15 at 00:47, viro@parcelfarce.linux.theplanet.co.uk
> wrote:
> > On Thu, May 13, 2004 at 09:26:36PM +0200, FabF wrote:
> > > Hi,
> > > 
> > > 	AFAICS, block_dev.c : do_open calls bdput after an unlock_kernel.bdput
> > > calls iput and iput plays with some parameters and locks in iput final
> > > case only.Here's a patch to have a spinlock around the whole iput.
> > 
> > Huh?  Of course iput() is called without BKL (and in a lot more places than
> > just that, actually), but why does it imply that we suddenly need to hold
> > inode_lock over the entire function?
> > 
> What can avoid inode->i_state to change during fs put_inode is done plus
> super_operations get assigned something unprotected as well.

1) Nothing whatsoever; why does ->put_inode() need protection for ->i_state
of all things?
2) ->i_sb never changes through the lifetime of inode; sb->s_op should never
change too.

Re: [PATCH 2.6.6-mm2] vfs iput in inode critical region

[FabF]

On Sat, 2004-05-15 at 08:33, viro@parcelfarce.linux.theplanet.co.uk
wrote:
> On Thu, May 13, 2004 at 05:02:19PM +0200, FabF wrote:
> > On Sat, 2004-05-15 at 00:47, viro@parcelfarce.linux.theplanet.co.uk
> > wrote:
> > > On Thu, May 13, 2004 at 09:26:36PM +0200, FabF wrote:
> > > > Hi,
> > > > 
> > > > 	AFAICS, block_dev.c : do_open calls bdput after an unlock_kernel.bdput
> > > > calls iput and iput plays with some parameters and locks in iput final
> > > > case only.Here's a patch to have a spinlock around the whole iput.
> > > 
> > > Huh?  Of course iput() is called without BKL (and in a lot more places than
> > > just that, actually), but why does it imply that we suddenly need to hold
> > > inode_lock over the entire function?
> > > 
> > What can avoid inode->i_state to change during fs put_inode is done plus
> > super_operations get assigned something unprotected as well.
> 
> 1) Nothing whatsoever; why does ->put_inode() need protection for ->i_state
> of all things?
Theorical question of atomicity I guess like stat.c set_bytes asking for
"sufficient locking" should use atomic_t values or critical region and
don't ask sub-layer to process the right locking.Btw set_bytes is used
once (in reiser AFAICS).

> 2) ->i_sb never changes through the lifetime of inode; sb->s_op should never
> change too.
Yep, it should'nt ...