[PATCH] factor out common code around ->follow_link invocation

[PATCH] factor out common code around ->follow_link invocation

[Christoph Hellwig]

--- 1.116/fs/namei.c	2005-01-05 03:48:11 +01:00
+++ edited/fs/namei.c	2005-01-12 19:41:49 +01:00
@@ -481,6 +482,24 @@
 	return PTR_ERR(link);
 }
 
+static inline int __do_follow_link(struct dentry *dentry, struct nameidata *nd)
+{
+	int error;
+
+	touch_atime(nd->mnt, dentry);
+	nd_set_link(nd, NULL);
+	error = dentry->d_inode->i_op->follow_link(dentry, nd);
+	if (!error) {
+		char *s = nd_get_link(nd);
+		if (s)
+			error = __vfs_follow_link(nd, s);
+		if (dentry->d_inode->i_op->put_link)
+			dentry->d_inode->i_op->put_link(dentry, nd);
+	}
+
+	return error;
+}
+
 /*
  * This limits recursive symlink follows to 8, while
  * limiting consecutive symlinks to 40.
@@ -503,16 +522,7 @@
 	current->link_count++;
 	current->total_link_count++;
 	nd->depth++;
-	touch_atime(nd->mnt, dentry);
-	nd_set_link(nd, NULL);
-	err = dentry->d_inode->i_op->follow_link(dentry, nd);
-	if (!err) {
-		char *s = nd_get_link(nd);
-		if (s)
-			err = __vfs_follow_link(nd, s);
-		if (dentry->d_inode->i_op->put_link)
-			dentry->d_inode->i_op->put_link(dentry, nd);
-	}
+	err = __do_follow_link(dentry, nd);
 	current->link_count--;
 	nd->depth--;
 	return err;
@@ -1469,16 +1479,7 @@
 	error = security_inode_follow_link(dentry, nd);
 	if (error)
 		goto exit_dput;
-	touch_atime(nd->mnt, dentry);
-	nd_set_link(nd, NULL);
-	error = dentry->d_inode->i_op->follow_link(dentry, nd);
-	if (!error) {
-		char *s = nd_get_link(nd);
-		if (s)
-			error = __vfs_follow_link(nd, s);
-		if (dentry->d_inode->i_op->put_link)
-			dentry->d_inode->i_op->put_link(dentry, nd);
-	}
+	error = __do_follow_link(dentry, nd);
 	dput(dentry);
 	if (error)
 		return error;

file system writes

[jenn]

Hello,

I had a question I hoped someone could answer.

I am modifying reiserfs code. I have added code to the fs that writes
kernel data to a file on the same system. On mount, I make a file by
creating a dentry, and then calling i_op->create. I then release the
dentry. Everything appears to work fine I can see the file on the system
and open it etc... 

I try to write data to this file after creating it like this: 

I make a file pointer
I call filp_open with the name of the file
I call set_fs (to tell it it is a kernel address)
I call fp->f_op->write 
I call filp_close

The write operation returns successfully but no data appears in the
file. I open it and it is empty. It does work if I try to write to a
file on another system though... like I can write to etc/ or /opt.

Brahhhh...

If anyone has some ideas thanks!

Jenn

Re: file system writes

[Jan Hudec]

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

On Thu, Jan 13, 2005 at 10:35:44 -0800, jenn wrote:
> Hello,
> 
> I had a question I hoped someone could answer.
> 
> I am modifying reiserfs code. I have added code to the fs that writes
> kernel data to a file on the same system. On mount, I make a file by
> creating a dentry, and then calling i_op->create. I then release the
> dentry. Everything appears to work fine I can see the file on the system
> and open it etc... 
> 
> I try to write data to this file after creating it like this: 
> 
> I make a file pointer
> I call filp_open with the name of the file
> I call set_fs (to tell it it is a kernel address)
> I call fp->f_op->write 
> I call filp_close
> 
> The write operation returns successfully but no data appears in the
> file. I open it and it is empty. It does work if I try to write to a
> file on another system though... like I can write to etc/ or /opt.
> 
> Brahhhh...
> 
> If anyone has some ideas thanks!

Is the whole world gone mad? Why is everybody trying to write files from
kernel?? I was said many times here, that it's forbidden to write to
files in kernel. What are you actualy trying to do??

That being said, it's doable and you are on the right track. Some stupid
mistake like forgetting to call fput perhaps?

But of course, I can't repeat it enough: FILES MUST NOT BE READ NOR
WRITTEN BY KERNEL!

-------------------------------------------------------------------------------
						 Jan 'Bulb' Hudec <bulb@ucw.cz>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: file system writes

[jenn]

> 
> That being said, it's doable and you are on the right track. Some stupid
> mistake like forgetting to call fput perhaps?

Okay... I changed my calls to use dentry_open and then fput (rather than
filp open and close) and I call dput to release the dentry. The file is
getting written to but I am getting this:

/home/jenn/linux-2.6.6/fs/reiserfs/bitmap.c:915: spin_is_locked on
uninitialized spinlock d48e4e60.
/home/jenn/linux-2.6.6/fs/reiserfs/bitmap.c:917: spin_is_locked on
uninitialized spinlock d48e4e60.
/home/jenn/linux-2.6.6/fs/reiserfs/bitmap.c:883: spin_is_locked on
uninitialized spinlock d48e4e60.
/home/jenn/linux-2.6.6/fs/reiserfs/bitmap.c:885: spin_is_locked on
uninitialized spinlock d48e4e60.
/home/jenn/linux-2.6.6/fs/reiserfs/bitmap.c:902: spin_is_locked on
uninitialized spinlock d48e4e60.
/home/jenn/linux-2.6.6/fs/reiserfs/bitmap.c:904: spin_is_locked on
uninitialized spinlock d48e4e60.

and when I unmount the system I get the dreaded: 

Busy inodes after unmount. Self-destruct in 5 seconds.  Have a nice
day...
slab error in kmem_cache_destroy(): cache `reiser_inode_cache': Can't
free all objects
> 
> But of course, I can't repeat it enough: FILES MUST NOT BE READ NOR
> WRITTEN BY KERNEL!

Well I do have a reason. I wish there were another way.

Jenn
> 
> -------------------------------------------------------------------------------
> 						 Jan 'Bulb' Hudec <bulb@ucw.cz>

Re: [PATCH] factor out common code around ->follow_link invocation

[Andrew Morton]

Christoph Hellwig <hch@lst.de> wrote:
>
> +static inline int __do_follow_link(struct dentry *dentry, struct nameidata *nd)

Now there's a fairly insane amount of inlining going on around
do_follow_link() and __do_follow_link().  It makes my fingers itchy.

ow.  __do_follow_link inlines __vfs_follow_link inlines walk_init_root and
__do_follow_link has maybe three callsites.

This:

--- 25/fs/namei.c~namei-uninlining	Thu Jan 13 16:01:24 2005
+++ 25-akpm/fs/namei.c	Thu Jan 13 16:04:21 2005
@@ -447,7 +447,7 @@ walk_init_root(const char *name, struct 
 	return 1;
 }
 
-static inline int __vfs_follow_link(struct nameidata *nd, const char *link)
+static int __vfs_follow_link(struct nameidata *nd, const char *link)
 {
 	int res = 0;
 	char *name;
@@ -482,7 +482,7 @@ fail:
 	return PTR_ERR(link);
 }
 
-static inline int __do_follow_link(struct dentry *dentry, struct nameidata *nd)
+static int __do_follow_link(struct dentry *dentry, struct nameidata *nd)
 {
 	int error;
 
@@ -507,7 +507,7 @@ static inline int __do_follow_link(struc
  * Without that kind of total limit, nasty chains of consecutive
  * symlinks can cause almost arbitrarily long lookups. 
  */
-static inline int do_follow_link(struct dentry *dentry, struct nameidata *nd)
+static int do_follow_link(struct dentry *dentry, struct nameidata *nd)
 {
 	int err = -ELOOP;
 	if (current->link_count >= MAX_NESTED_LINKS)


does this:

   text    data     bss     dec     hex filename
  16602      96       0   16698    413a fs/namei.o
  14986      96       0   15082    3aea fs/namei.o

Is it still the case that we need inlining in the link-following code to
prevent stack windup?  If so, some commentary is needed to protect it. 
what are the guidelines?

Re: [PATCH] factor out common code around ->follow_link invocation

[Al Viro]

On Thu, Jan 13, 2005 at 04:06:34PM -0800, Andrew Morton wrote:
> Christoph Hellwig <hch@lst.de> wrote:
> >
> > +static inline int __do_follow_link(struct dentry *dentry, struct nameidata *nd)
> 
> Now there's a fairly insane amount of inlining going on around
> do_follow_link() and __do_follow_link().  It makes my fingers itchy.
> 
> ow.  __do_follow_link inlines __vfs_follow_link inlines walk_init_root and
> __do_follow_link has maybe three callsites.

No.  These guys are in the middle of mutual recursion.  So in this
particular case we *do* need inlining.

Re: [PATCH] factor out common code around ->follow_link invocation

[Andrew Morton]

Al Viro <viro@parcelfarce.linux.theplanet.co.uk> wrote:
>
> On Thu, Jan 13, 2005 at 04:06:34PM -0800, Andrew Morton wrote:
> > Christoph Hellwig <hch@lst.de> wrote:
> > >
> > > +static inline int __do_follow_link(struct dentry *dentry, struct nameidata *nd)
> > 
> > Now there's a fairly insane amount of inlining going on around
> > do_follow_link() and __do_follow_link().  It makes my fingers itchy.
> > 
> > ow.  __do_follow_link inlines __vfs_follow_link inlines walk_init_root and
> > __do_follow_link has maybe three callsites.
> 
> No.  These guys are in the middle of mutual recursion.  So in this
> particular case we *do* need inlining.

<head spins> How can you do mutually-recursive code with inline functions
without generating an infinite amount of code?

Wanna spell it out a bit more please?

Re: [PATCH] factor out common code around ->follow_link invocation

[Al Viro]

On Thu, Jan 13, 2005 at 04:34:49PM -0800, Andrew Morton wrote:
> > > Now there's a fairly insane amount of inlining going on around
> > > do_follow_link() and __do_follow_link().  It makes my fingers itchy.
> > > 
> > > ow.  __do_follow_link inlines __vfs_follow_link inlines walk_init_root and
> > > __do_follow_link has maybe three callsites.
> > 
> > No.  These guys are in the middle of mutual recursion.  So in this
> > particular case we *do* need inlining.
> 
> <head spins> How can you do mutually-recursive code with inline functions
> without generating an infinite amount of code?

"In the middle of" != "entire"

> Wanna spell it out a bit more please?

link_path_walk -> do_follow_link -> __vfs_follow_link -> link_path_walk

link_path_walk() is not inline.  The rest of the thing would better be.