[PATCH] iget-locked [2/6]

[PATCH] iget-locked [2/6]

[Jan Harkes]

Now we introduce iget_locked and iget5_locked. These are similar to
iget, but return a locked inode and read_inode has not been called. So
the FS has to call read_inode to initialize the inode and then unlock
it with unlock_new_inode().

This patch is based on the icreate patch from the XFS group, i.e.
it is still pretty much identical except for function naming.

======

diff -urN iget_locked-1/fs/Makefile iget_locked-2/fs/Makefile
--- iget_locked-1/fs/Makefile	Wed May  1 00:27:34 2002
+++ iget_locked-2/fs/Makefile	Fri May 10 10:33:12 2002
@@ -7,7 +7,7 @@
 
 O_TARGET := fs.o
 
-export-objs :=	filesystems.o open.o dcache.o buffer.o bio.o
+export-objs :=	filesystems.o open.o dcache.o buffer.o bio.o inode.o
 mod-subdirs :=	nls
 
 obj-y :=	open.o read_write.o devices.o file_table.o buffer.o \
diff -urN iget_locked-1/fs/inode.c iget_locked-2/fs/inode.c
--- iget_locked-1/fs/inode.c	Fri May 10 10:32:03 2002
+++ iget_locked-2/fs/inode.c	Fri May 10 10:33:12 2002
@@ -12,6 +12,7 @@
 #include <linux/quotaops.h>
 #include <linux/slab.h>
 #include <linux/writeback.h>
+#include <linux/module.h>
 
 /*
  * New inode.c implementation.
@@ -501,6 +502,21 @@
 	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.
  *
@@ -527,7 +543,7 @@
 				inodes_stat.nr_inodes++;
 				list_add(&inode->i_list, &inode_in_use);
 				list_add(&inode->i_hash, head);
-				inode->i_state = I_LOCK;
+				inode->i_state = I_LOCK|I_NEW;
 			}
 			spin_unlock(&inode_lock);
 
@@ -536,28 +552,9 @@
 				return NULL;
 			}
 
-			/* 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.
-			** -- mason@suse.com 
-			*/
-			if (sb->s_op->read_inode2) {
-				sb->s_op->read_inode2(inode, data) ;
-			} 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;
 		}
 
@@ -637,8 +634,12 @@
 	return inode;
 }
 
-
-struct inode *iget4(struct super_block *sb, unsigned long ino, int (*test)(struct inode *, void *), int (*set)(struct inode *, void *), void *data)
+/*
+ * This is iget without the read_inode portion of get_new_inode
+ * the filesystem gets back a new locked and hashed inode and gets
+ * to fill it in before unlocking it via unlock_new_inode().
+ */
+struct inode *iget5_locked(struct super_block *sb, unsigned long ino, int (*test)(struct inode *, void *), int (*set)(struct inode *, void *), void *data)
 {
 	struct list_head * head = inode_hashtable + hash(sb,ino);
 	struct inode * inode;
@@ -658,6 +659,36 @@
 	 * in case it had to block at any point.
 	 */
 	return get_new_inode(sb, ino, head, test, set, data);
+}
+
+struct inode *iget_locked(struct super_block *sb, unsigned long ino)
+{
+	return iget5_locked(sb, ino, NULL, NULL, NULL);
+}
+
+EXPORT_SYMBOL(iget5_locked);
+EXPORT_SYMBOL(iget_locked);
+EXPORT_SYMBOL(unlock_new_inode);
+
+struct inode *iget4(struct super_block *sb, unsigned long ino, int (*test)(struct inode *, void *), int (*set)(struct inode *, void *), void *data)
+{
+	struct inode *inode = iget5_locked(sb, ino, test, set, data);
+
+	if (inode && (inode->i_state & I_NEW)) {
+		/* 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.
+		** -- mason@suse.com 
+		*/
+		if (sb->s_op->read_inode2) {
+			sb->s_op->read_inode2(inode, data);
+		} else {
+			sb->s_op->read_inode(inode);
+		}
+		unlock_new_inode(inode);
+	}
+
+	return inode;
 }
 
 /**
diff -urN iget_locked-1/include/linux/fs.h iget_locked-2/include/linux/fs.h
--- iget_locked-1/include/linux/fs.h	Fri May 10 10:32:04 2002
+++ iget_locked-2/include/linux/fs.h	Fri May 10 10:33:12 2002
@@ -832,6 +832,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)
 
@@ -1239,6 +1240,10 @@
 extern void force_delete(struct inode *);
 extern struct inode * igrab(struct inode *);
 extern ino_t iunique(struct super_block *, ino_t);
+
+extern struct inode * iget5_locked(struct super_block *, unsigned long, int (*test)(struct inode *, void *), int (*set)(struct inode *, void *), void *);
+extern struct inode * iget_locked(struct super_block *, unsigned long);
+extern void unlock_new_inode(struct inode *);
 
 extern struct inode * iget4(struct super_block *, unsigned long, int (*test)(struct inode *, void *), int (*set)(struct inode *, void *), void *);
 static inline struct inode *iget(struct super_block *sb, unsigned long ino)

Re: [PATCH] iget-locked [2/6]

[Alexander Viro]

On Fri, 10 May 2002, Jan Harkes wrote:

> 
> Now we introduce iget_locked and iget5_locked. These are similar to
> iget, but return a locked inode and read_inode has not been called. So
> the FS has to call read_inode to initialize the inode and then unlock
> it with unlock_new_inode().
> 
> This patch is based on the icreate patch from the XFS group, i.e.
> it is still pretty much identical except for function naming.

No problems, except for putting exports in inode.c.  ISTR Linus saying that
additional files with exports seriously increase the build time...  Linus?

Re: [PATCH] iget-locked [2/6]

[Linus Torvalds]

On Fri, 10 May 2002, Alexander Viro wrote:
> 
> No problems, except for putting exports in inode.c.  ISTR Linus saying that
> additional files with exports seriously increase the build time...  Linus?

A few additional ones are fine - especially for "core" stuff like inode.c 
I don't see any problem at all.

And keeping EXPORT_SYMBOL close to the place that defines it makes some 
things clearer. I would certainly not mind moving some of the 
kernel/ksym.c stuff out to the places that actually define the functions.

If it becomes an issue where _most_ files in export symbols, our build 
times will suck, but fs/inode.c is certainly central enough that I don't 
find any problem with it.

		Linus

Re: [PATCH] iget-locked [2/6]

[Alexander Viro]

On Fri, 10 May 2002, Linus Torvalds wrote:

> 
> On Fri, 10 May 2002, Alexander Viro wrote:
> > 
> > No problems, except for putting exports in inode.c.  ISTR Linus saying that
> > additional files with exports seriously increase the build time...  Linus?
> 
> A few additional ones are fine - especially for "core" stuff like inode.c 
> I don't see any problem at all.
> 
> And keeping EXPORT_SYMBOL close to the place that defines it makes some 
> things clearer. I would certainly not mind moving some of the 
> kernel/ksym.c stuff out to the places that actually define the functions.
> 
> If it becomes an issue where _most_ files in export symbols, our build 
> times will suck, but fs/inode.c is certainly central enough that I don't 
> find any problem with it.

OK.  BTW, would you accept ->getattr() patchset if I start to feed it to
you today?

Re: [PATCH] iget-locked [2/6]

[Linus Torvalds]

On Fri, 10 May 2002, Alexander Viro wrote:
> 
> OK.  BTW, would you accept ->getattr() patchset if I start to feed it to
> you today?

Go wild.

		Linus

Re: [PATCH] iget-locked [2/6]

[Keith Owens]

On Fri, 10 May 2002 15:57:03 -0400 (EDT), 
Alexander Viro <viro@math.psu.edu> wrote:
>On Fri, 10 May 2002, Jan Harkes wrote:
>> Now we introduce iget_locked and iget5_locked. These are similar to
>> iget, but return a locked inode and read_inode has not been called. So
>> the FS has to call read_inode to initialize the inode and then unlock
>> it with unlock_new_inode().
>
>No problems, except for putting exports in inode.c.  ISTR Linus saying that
>additional files with exports seriously increase the build time...  Linus?

Build time is the least of your worries here.  All objects that export
symbols must have unique basenames, all the modversion crud goes in
include/linux/modules under the object's basename.  This is the main
reason that many subsystems have a subsystem_ksyms.c file, to get a
unique base name.

There are 34 files called inode in 2.4.18.  None currently export
symbols so adding EXPORT_SYMBOL to fs/inode.c is safe, until somebody
else decides they want their inode.c to export symbols.  You will not
notice until you build both systems with MODVERSIONS=y.

Re: [PATCH] iget-locked [2/6]

[Kai Germaschewski]

On Sat, 11 May 2002, Keith Owens wrote:

> Build time is the least of your worries here.  All objects that export
> symbols must have unique basenames, all the modversion crud goes in
> include/linux/modules under the object's basename.

This is not true anymore in 2.5, this limitation was removed when ALSA 
went in.

--Kai

Re: [PATCH] iget-locked [2/6]

[Keith Owens]

On Fri, 10 May 2002 21:21:16 -0500 (CDT), 
Kai Germaschewski <kai@tp1.ruhr-uni-bochum.de> wrote:
>On Sat, 11 May 2002, Keith Owens wrote:
>
>> Build time is the least of your worries here.  All objects that export
>> symbols must have unique basenames, all the modversion crud goes in
>> include/linux/modules under the object's basename.
>
>This is not true anymore in 2.5, this limitation was removed when ALSA 
>went in.

True, but if the iget change goes into 2.5 it will probably be
backported to 2.4 later, 2.4 still has the restriction.

As for modversions on 2.5, well you know my opinion ;).

Re: [PATCH] iget-locked [2/6]

[Jan Harkes]

On Sat, May 11, 2002 at 12:48:46PM +1000, Keith Owens wrote:
> On Fri, 10 May 2002 21:21:16 -0500 (CDT), 
> Kai Germaschewski <kai@tp1.ruhr-uni-bochum.de> wrote:
> >This is not true anymore in 2.5, this limitation was removed when ALSA 
> >went in.
> 
> True, but if the iget change goes into 2.5 it will probably be
> backported to 2.4 later, 2.4 still has the restriction.
> 
> As for modversions on 2.5, well you know my opinion ;).

A backport is not that likely. The patch removes iget4 and as a result
breaks compatibility for binary-only kernel modules that use iget and/or
iget4. So, I don't believe this patch is appropriate for a stable series.

I'm going to fix the iget4 race in 2.4 by adding a per-superblock
semaphore around the call to iget4 in Coda. My guess is that NFS and
ReiserFS will have to do something similar. Filesystems that do not
use a special 'find_actor' (i.e. iget) don't have a problem in 2.4.

Jan

Re: [PATCH] iget-locked [2/6]

[Kai Germaschewski]

On Sat, 11 May 2002, Keith Owens wrote:

> True, but if the iget change goes into 2.5 it will probably be
> backported to 2.4 later, 2.4 still has the restriction.

It's a two line change to Rules.make, so I suppose that can be backported
as well ;-) 

It's needed anyway IIRC, since s390 and ISDN both have fsm.o, and both do
export symbols.

--Kai

Re: [PATCH] iget-locked [2/6]

[Keith Owens]

On Fri, 10 May 2002 22:06:46 -0500 (CDT), 
Kai Germaschewski <kai@tp1.ruhr-uni-bochum.de> wrote:
>On Sat, 11 May 2002, Keith Owens wrote:
>
>> True, but if the iget change goes into 2.5 it will probably be
>> backported to 2.4 later, 2.4 still has the restriction.
>
>It's a two line change to Rules.make, so I suppose that can be backported
>as well ;-) 

The change looks safe enough for the kernel but I worry about its
impact on distributors who play silly buggers with the modversion
files (R**H** springs to mind).  It would be polite to check with the
major distributors before backporting the change to 2.4.

>It's needed anyway IIRC, since s390 and ISDN both have fsm.o, and both do
>export symbols.

ISDN on s390.  The mind boggles.

Re: [PATCH] iget-locked [2/6]

[Alexander Viro]

On Fri, 10 May 2002, Jan Harkes wrote:

> On Sat, May 11, 2002 at 12:48:46PM +1000, Keith Owens wrote:
> > On Fri, 10 May 2002 21:21:16 -0500 (CDT), 
> > Kai Germaschewski <kai@tp1.ruhr-uni-bochum.de> wrote:
> > >This is not true anymore in 2.5, this limitation was removed when ALSA 
> > >went in.
> > 
> > True, but if the iget change goes into 2.5 it will probably be
> > backported to 2.4 later, 2.4 still has the restriction.
> > 
> > As for modversions on 2.5, well you know my opinion ;).
> 
> A backport is not that likely. The patch removes iget4 and as a result
> breaks compatibility for binary-only kernel modules that use iget and/or
> iget4. So, I don't believe this patch is appropriate for a stable series.

It will need decent testing + backport of knfsd changes to 2.4 to become
a candidate for merge.

As for the binary compatibility... as long as we are source-compatible
(i.e. keep ->read_inode2 and provide a compatible iget4()) - compatibility
is not a problem.  Anyone who ships binary-only modules is playing in the
traffic and if they become a roadkill - it's Not Our Problem(tm).  Think
of it as evolution in action...

Re: [PATCH] iget-locked [2/6]

[Christoph Hellwig]

On Fri, May 10, 2002 at 11:04:37PM -0400, Jan Harkes wrote:
> A backport is not that likely. The patch removes iget4 and as a result
> breaks compatibility for binary-only kernel modules that use iget and/or
> iget4. So, I don't believe this patch is appropriate for a stable series.

Part of that patchseries (an not, it's not iget4 removal :)) is likely to be
backported at least into the 2.4 XFS tree.  I guess it will also end up in
the mainline at some point.

Re: [PATCH] iget-locked [2/6]

[Christoph Hellwig]

On Fri, May 10, 2002 at 11:44:23PM -0400, Alexander Viro wrote:
> As for the binary compatibility... as long as we are source-compatible
> (i.e. keep ->read_inode2 and provide a compatible iget4()) - compatibility
> is not a problem.  Anyone who ships binary-only modules is playing in the
> traffic and if they become a roadkill - it's Not Our Problem(tm).  Think
> of it as evolution in action...

We need to keep iget4, but I don't think keeping ->read_inode2 source
compatiblitly is needed, it always was marked as a reiserfs cludge and
no one was supposed to use it.