Skip to contentSkip to conversation

backport iget_locked from 2.5/2.6

PATCH In review Christoph Hellwig 2 participants 6 messages Aug 25, 2003 – Aug 26, 2003 VFS

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 backport iget_locked from 2.5/2.6

Provide an iget variant without unlocking the inode and ->read_inode call. This is needed for XFS and IIRC the reiserfs folks wanted it, too.

Tested in 2.5 for more than half a year and in 2.4-ac/-aa, and the varoius vendor trees for a long time.

fs/inode.c +18 -23
@@ -834,6 +839,20 @@
 	return inode;
 }
 
+void unlock_new_inode(struct inode *inode)
+{
+	/*
+	 * This is special!  We do not need the spinlock
+	 * when clearing I_LOCK, because we're guaranteed
+	 * that nobody else tries to do anything about the
+	 * state of the inode when it is locked, as we
+	 * just created it (so there can be no old holders
+	 * that haven't tested I_LOCK).
+	 */
+	inode->i_state &= ~(I_LOCK|I_NEW);
+	wake_up(&inode->i_wait);
+}
+
 /*
  * This is called without the inode lock held.. Be careful.
  *
@@ -856,31 +875,13 @@
 			list_add(&inode->i_list, &inode_in_use);
 			list_add(&inode->i_hash, head);
 			inode->i_ino = ino;
-			inode->i_state = I_LOCK;
+			inode->i_state = I_LOCK|I_NEW;
 			spin_unlock(&inode_lock);
 
-			/* reiserfs specific hack right here.  We don't
-			** want this to last, and are looking for VFS changes
-			** that will allow us to get rid of it.
-			** -- [email protected] 
-			*/
-			if (sb->s_op->read_inode2) {
-				sb->s_op->read_inode2(inode, opaque) ;
-			} else {
-				sb->s_op->read_inode(inode);
-			}
-
 			/*
-			 * This is special!  We do not need the spinlock
-			 * when clearing I_LOCK, because we're guaranteed
-			 * that nobody else tries to do anything about the
-			 * state of the inode when it is locked, as we
-			 * just created it (so there can be no old holders
-			 * that haven't tested I_LOCK).
+			 * Return the locked inode with I_NEW set, the
+			 * caller is responsible for filling in the contents
 			 */
-			inode->i_state &= ~I_LOCK;
-			wake_up(&inode->i_wait);
-
 			return inode;
 		}
 
@@ -960,8 +961,7 @@
 	return inode;
 }
 
-
-struct inode *iget4(struct super_block *sb, unsigned long ino, find_inode_t find_actor, void *opaque)
+struct inode *iget4_locked(struct super_block *sb, unsigned long ino, find_inode_t find_actor, void *opaque)
 {
 	struct list_head * head = inode_hashtable + hash(sb,ino);
 	struct inode * inode;
include/linux/fs.h +38 -2
@@ -966,6 +969,7 @@
 #define I_LOCK			8
 #define I_FREEING		16
 #define I_CLEAR			32
+#define I_NEW			64
 
 #define I_DIRTY (I_DIRTY_SYNC | I_DIRTY_DATASYNC | I_DIRTY_PAGES)
 
@@ -1391,12 +1396,47 @@
 extern void force_delete(struct inode *);
 extern struct inode * igrab(struct inode *);
 extern ino_t iunique(struct super_block *, ino_t);
+extern void unlock_new_inode(struct inode *);
 
 typedef int (*find_inode_t)(struct inode *, unsigned long, void *);
-extern struct inode * iget4(struct super_block *, unsigned long, find_inode_t, void *);
+
+extern struct inode * iget4_locked(struct super_block *, unsigned long,
+				   find_inode_t, void *);
+
+static inline struct inode *iget4(struct super_block *sb, unsigned long ino,
+				  find_inode_t find_actor, void *opaque)
+{
+	struct inode *inode = iget4_locked(sb, ino, find_actor, opaque);
+
+	if (inode && (inode->i_state & I_NEW)) {
+		/*
+		 * reiserfs-specific kludge that is expected to go away ASAP.
+		 */
+		if (sb->s_op->read_inode2)
+			sb->s_op->read_inode2(inode, opaque);
+		else
+			sb->s_op->read_inode(inode);
+		unlock_new_inode(inode);
+	}
+
+	return inode;
+}
+
 static inline struct inode *iget(struct super_block *sb, unsigned long ino)
 {
-	return iget4(sb, ino, NULL, NULL);
+	struct inode *inode = iget4_locked(sb, ino, NULL, NULL);
+
+	if (inode && (inode->i_state & I_NEW)) {
+		sb->s_op->read_inode(inode);
+		unlock_new_inode(inode);
+	}
+
+	return inode;
+}
+
+static inline struct inode *iget_locked(struct super_block *sb, unsigned long ino)
+{
+	return iget4_locked(sb, ino, NULL, NULL);
 }
 
 extern void clear_inode(struct inode *);
kernel/ksyms.c +2 -1
@@ -143,7 +143,8 @@
 EXPORT_SYMBOL(fget);
 EXPORT_SYMBOL(igrab);
 EXPORT_SYMBOL(iunique);
-EXPORT_SYMBOL(iget4);
+EXPORT_SYMBOL(iget4_locked);
+EXPORT_SYMBOL(unlock_new_inode);
 EXPORT_SYMBOL(iput);
 EXPORT_SYMBOL(inode_init_once);
 EXPORT_SYMBOL(force_delete);
#1 lore raw

Helllo!

Quoted 3 lines · Christoph Hellwig

Provide an iget variant without unlocking the inode and ->read_inode call. This is needed for XFS and IIRC the reiserfs folks wanted it, too.

No. This patch is useless for our purposes. (and coda/nfs ). We wanted to get rid of a race where inode allocation and filling fs specific parts are done non-atomically.
The patch below does not achieve this. We still fill inode private part outside of inode_lock locked region.

Bye,
Oleg

#2 lore raw
Quoted 2 lines · Oleg Drokin

The patch below does not achieve this. We still fill inode private part outside of inode_lock locked region.

-ENOPATCH :)

#3 lore raw

Hello!

Quoted 1 line

Mail-Followup-To: Christoph Hellwig <[email protected]>,

Hm, very interesting header, I'd say. No wonder I'm getting errors replying to your emails.

Quoted 3 lines · Christoph Hellwig
Quoted 2 lines

The patch below does not achieve this. We still fill inode private part outside of inode_lock locked region.

-ENOPATCH :)

I meant the patch in the email you sent and to which I answered originally ;)

Bye,
Oleg

#4 lore raw
Quoted 6 lines · Oleg Drokin

Hello!

Quoted 1 line

Mail-Followup-To: Christoph Hellwig <[email protected]>,

Hm, very interesting header, I'd say. No wonder I'm getting errors replying to your emails.

Well, I got the same from you although I though only in the Cc line which I removed.

Quoted 6 lines
Quoted 4 lines
Quoted 3 lines

The patch below does not achieve this. We still fill inode private part outside of inode_lock locked region.

-ENOPATCH :)

I meant the patch in the email you sent and to which I answered originally ;)

Sorry, I missed the 'not' when reading and though you had an alternate patch

#5 lore raw

Hello!

Quoted 5 lines · Christoph Hellwig
Quoted 3 lines
Quoted 1 line

Mail-Followup-To: Christoph Hellwig <[email protected]>,

Hm, very interesting header, I'd say. No wonder I'm getting errors replying to your emails.

Well, I got the same from you although I though only in the Cc line which I removed.

That's because I hit "reply" first time and have not looked at the CC list.

Quoted 6 lines
Quoted 4 lines
Quoted 3 lines
Quoted 2 lines

The patch below does not achieve this. We still fill inode private part outside of inode_lock locked region.

-ENOPATCH :)

I meant the patch in the email you sent and to which I answered originally ;)

Sorry, I missed the 'not' when reading and though you had an alternate patch

Actually, I have altenate patch (but no, I have not tried to attach it) ;)
I have full iget5_locked backport (that I was sending around some time ago). Only it will break every iget4 user not in the tree (by changing VFS API), so I guess it is not an option. But if people think it is good idea to adapt this change, I can easily resurrect that patch, of course.

Bye,
Oleg