[PATCH] cramfs: generate unique inode number for better inode cache usage

[PATCH] cramfs: generate unique inode number for better inode cache usage

[stefani]

From: Stefani Seibold <stefani@seibold.net>

This is second version, hope this is less ugly code :-)

This patch generates a unique inode numbers for any entries in the cram file
system. For files which did not contain data's (device nodes, fifos and
sockets) the offset of the directory entry inside the cramfs plus 1 will be
used as inode number.

The + 1 for the inode will it make possible to distinguish between
a file which contains no data and files which has data, the later one has
a inode value where the lower two bits are always 0.

It also reimplement the behavoir to set the size and the number of block
to 0 for special file, which is the right value for empty files,
devices, fifos and sockets

As a little benefit it will be also more compatible which older
mkcramfs, because it will never use the cramfs_inode->offset for
creating a inode number for special files.

The patch is aginst 2.6.37-rc5

Please apply it.

Thanks,
Stefani

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 fs/cramfs/inode.c |   85 ++++++++++++++++++++++++++++++++++------------------
 1 files changed, 55 insertions(+), 30 deletions(-)

diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
index 32fd5fe..0b39167 100644
--- a/fs/cramfs/inode.c
+++ b/fs/cramfs/inode.c
@@ -36,16 +36,29 @@ static DEFINE_MUTEX(read_mutex);
 
 /* These two macros may change in future, to provide better st_ino
    semantics. */
-#define CRAMINO(x)	(((x)->offset && (x)->size)?(x)->offset<<2:1)
+#define CRAMINO_UNIQ(x)	((x) + 1)
 #define OFFSET(x)	((x)->i_ino)
 
-static void setup_inode(struct inode *inode, struct cramfs_inode * cramfs_inode)
+static unsigned long cramino(struct cramfs_inode * cino,
+					unsigned int offset)
+{
+	if (!cino->offset)
+		return CRAMINO_UNIQ(offset);
+	if (!cino->size)
+		return CRAMINO_UNIQ(offset);
+
+	return cino->offset << 2;
+}
+
+static void setup_inode(struct inode *inode, struct cramfs_inode * cramfs_inode, u32 size)
 {
 	static struct timespec zerotime;
 	inode->i_mode = cramfs_inode->mode;
 	inode->i_uid = cramfs_inode->uid;
-	inode->i_size = cramfs_inode->size;
-	inode->i_blocks = (cramfs_inode->size - 1) / 512 + 1;
+	if (size) {
+		inode->i_size = size;
+		inode->i_blocks = (size - 1) / 512 + 1;
+	}
 	inode->i_gid = cramfs_inode->gid;
 	/* Struct copy intentional */
 	inode->i_mtime = inode->i_atime = inode->i_ctime = zerotime;
@@ -53,36 +66,41 @@ static void setup_inode(struct inode *inode, struct cramfs_inode * cramfs_inode)
 	   but it's the best we can do without reading the directory
 	   contents.  1 yields the right result in GNU find, even
 	   without -noleaf option. */
-	if (S_ISREG(inode->i_mode)) {
-		inode->i_fop = &generic_ro_fops;
-		inode->i_data.a_ops = &cramfs_aops;
-	} else if (S_ISDIR(inode->i_mode)) {
-		inode->i_op = &cramfs_dir_inode_operations;
-		inode->i_fop = &cramfs_directory_operations;
-	} else if (S_ISLNK(inode->i_mode)) {
-		inode->i_op = &page_symlink_inode_operations;
-		inode->i_data.a_ops = &cramfs_aops;
-	} else {
-		init_special_inode(inode, inode->i_mode,
-			old_decode_dev(cramfs_inode->size));
-	}
+	unlock_new_inode(inode);
 }
 
 static struct inode *get_cramfs_inode(struct super_block *sb,
-				struct cramfs_inode * cramfs_inode)
+	struct cramfs_inode * cramfs_inode, unsigned int offset)
 {
 	struct inode *inode;
-	if (CRAMINO(cramfs_inode) == 1) {
-		inode = new_inode(sb);
-		if (inode) {
-			inode->i_ino = 1;
-			setup_inode(inode, cramfs_inode);
+
+	if (S_ISREG(cramfs_inode->mode)) {
+		inode = iget_locked(sb, cramino(cramfs_inode, offset));
+		if (inode && (inode->i_state & I_NEW)) {
+			inode->i_fop = &generic_ro_fops;
+			inode->i_data.a_ops = &cramfs_aops;
+			setup_inode(inode, cramfs_inode, cramfs_inode->size);
+		}
+	} else if (S_ISDIR(cramfs_inode->mode)) {
+		inode = iget_locked(sb, cramino(cramfs_inode, offset));
+		if (inode && (inode->i_state & I_NEW)) {
+			inode->i_op = &cramfs_dir_inode_operations;
+			inode->i_fop = &cramfs_directory_operations;
+			setup_inode(inode, cramfs_inode, cramfs_inode->size);
+		}
+	} else if (S_ISLNK(cramfs_inode->mode)) {
+		inode = iget_locked(sb, cramino(cramfs_inode, offset));
+		if (inode && (inode->i_state & I_NEW)) {
+			inode->i_op = &page_symlink_inode_operations;
+			inode->i_data.a_ops = &cramfs_aops;
+			setup_inode(inode, cramfs_inode, cramfs_inode->size);
 		}
 	} else {
-		inode = iget_locked(sb, CRAMINO(cramfs_inode));
+		inode = iget_locked(sb, CRAMINO_UNIQ(offset));
 		if (inode && (inode->i_state & I_NEW)) {
-			setup_inode(inode, cramfs_inode);
-			unlock_new_inode(inode);
+			init_special_inode(inode, cramfs_inode->mode,
+				old_decode_dev(cramfs_inode->size));
+			setup_inode(inode, cramfs_inode, 0);
 		}
 	}
 	return inode;
@@ -265,6 +283,9 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 		printk(KERN_ERR "cramfs: root is not a directory\n");
 		goto out;
 	}
+	/* correct strange, hard-coded permissions of mkcramfs */
+	super.root.mode |= (S_IRUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
+
 	root_offset = super.root.offset << 2;
 	if (super.flags & CRAMFS_FLAG_FSID_VERSION_2) {
 		sbi->size=super.size;
@@ -289,7 +310,7 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 
 	/* Set it all up.. */
 	sb->s_op = &cramfs_ops;
-	root = get_cramfs_inode(sb, &super.root);
+	root = get_cramfs_inode(sb, &super.root, 0);
 	if (!root)
 		goto out;
 	sb->s_root = d_alloc_root(root);
@@ -365,8 +386,11 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 		 */
 		namelen = de->namelen << 2;
 		memcpy(buf, name, namelen);
-		ino = CRAMINO(de);
 		mode = de->mode;
+		if (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode))
+			ino = cramino(de, OFFSET(inode) + offset);
+		else
+			ino = CRAMINO_UNIQ(OFFSET(inode) + offset);
 		mutex_unlock(&read_mutex);
 		nextoffset = offset + sizeof(*de) + namelen;
 		for (;;) {
@@ -404,8 +428,9 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 		struct cramfs_inode *de;
 		char *name;
 		int namelen, retval;
+		int dir_off = OFFSET(dir) + offset;
 
-		de = cramfs_read(dir->i_sb, OFFSET(dir) + offset, sizeof(*de)+CRAMFS_MAXPATHLEN);
+		de = cramfs_read(dir->i_sb, dir_off, sizeof(*de)+CRAMFS_MAXPATHLEN);
 		name = (char *)(de+1);
 
 		/* Try to take advantage of sorted directories */
@@ -436,7 +461,7 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 		if (!retval) {
 			struct cramfs_inode entry = *de;
 			mutex_unlock(&read_mutex);
-			d_add(dentry, get_cramfs_inode(dir->i_sb, &entry));
+			d_add(dentry, get_cramfs_inode(dir->i_sb, &entry, dir_off));
 			return NULL;
 		}
 		/* else (retval < 0) */
-- 
1.7.3.3

Re: [PATCH] cramfs: generate unique inode number for better inode cache usage

[Linus Torvalds]

Why do you repeat that

    inode = iget_locked(sb, cramino(cramfs_inode, offset));
    if (inode && (inode->i_state & I_NEW)) {

so many times?

Wouldn't it be nicer to just do it once at the top?

     Linus

Re: [PATCH] cramfs: generate unique inode number for better inode cache usage

[Linus Torvalds]

On Tue, Dec 14, 2010 at 3:32 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Why do you repeat that
>
>    inode = iget_locked(sb, cramino(cramfs_inode, offset));
>    if (inode && (inode->i_state & I_NEW)) {
>
> so many times?
>
> Wouldn't it be nicer to just do it once at the top?

Oh, and I think it's probably nicer to then do the if-statements as a

   switch (cram_inode->mode & S_IFMT) {
   case S_IFREG:
   ..

but what I objected to in the previous patch was doing it multiple
times, and moving the nice separate helper function into the caller.

So I _think_ you should be able to just do something like this:

   ino = cramino(cramfs_inode, offset);
   switch (cram_inode->mode & S_IFMT) {
   case S_IFREG:
      fops = generic_ro_fops;
      aops = &cramfs_aops;
      break;
   case S_IFDIR:
      ...
   default:
       ino = CRAMINO_UNIQ(offset);
   }
   inode = iget_locked(sb, ino);
   if (inode && (inode->i_state & I_NEW)) {
      inode->i_ops = iops;
      inode->i_fops = fops;
      inode->i_data.a_ops = aops;
      setup_inode(inode, cramfs_inode, cramfs_inode->size);
    }

or whatever?

                   Linus

Re: [PATCH] cramfs: generate unique inode number for better inode cache usage

[Stefani Seibold]

Am Dienstag, den 14.12.2010, 15:43 -0800 schrieb Linus Torvalds:
> On Tue, Dec 14, 2010 at 3:32 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > Why do you repeat that
> >
> >    inode = iget_locked(sb, cramino(cramfs_inode, offset));
> >    if (inode && (inode->i_state & I_NEW)) {
> >
> > so many times?
> >
> > Wouldn't it be nicer to just do it once at the top?
> 
> Oh, and I think it's probably nicer to then do the if-statements as a
> 
>    switch (cram_inode->mode & S_IFMT) {
>    case S_IFREG:
>    ..	
> 

This is easy to fix.

> but what I objected to in the previous patch was doing it multiple
> times, and moving the nice separate helper function into the caller.
> 
> So I _think_ you should be able to just do something like this:
> 
>    ino = cramino(cramfs_inode, offset);
>    switch (cram_inode->mode & S_IFMT) {
>    case S_IFREG:
>       fops = generic_ro_fops;
>       aops = &cramfs_aops;
>       break;
>    case S_IFDIR:
>       ...
>    default:
>        ino = CRAMINO_UNIQ(offset);
>    }
>    inode = iget_locked(sb, ino);
>    if (inode && (inode->i_state & I_NEW)) {
>       inode->i_ops = iops;
>       inode->i_fops = fops;
>       inode->i_data.a_ops = aops;
>       setup_inode(inode, cramfs_inode, cramfs_inode->size);
>     }
> 

No, it makes no sense, for following reasons:

- Why pre-setup variables like iops, fops and aops which are in the most
cases not needed? Because the inode is still in the cache.

- Each branch is a little bit different.

- The default branch is completly different, because in this case the
init_special_inode() will be called, which sets the inode->fop.

What do you think about this solution:

static void setup_inode(struct inode *inode, struct cramfs_inode * cramfs_inode)
{
	static struct timespec zerotime;
	inode->i_mode = cramfs_inode->mode;
	inode->i_uid = cramfs_inode->uid;
	/* if the lower 2 bits are zero, the inode contains data */
	if (!(inode->i_ino & 3)) {
		inode->i_size = size;
		inode->i_blocks = (size - 1) / 512 + 1;
	}
	inode->i_gid = cramfs_inode->gid;
	/* Struct copy intentional */
	inode->i_mtime = inode->i_atime = inode->i_ctime = zerotime;
	/* inode->i_nlink is left 1 - arguably wrong for directories,
	   but it's the best we can do without reading the directory
	   contents.  1 yields the right result in GNU find, even
	   without -noleaf option. */
	unlock_new_inode(inode);
}

static struct inode *get_cramfs_inode(struct super_block *sb,
	struct cramfs_inode * cramfs_inode, unsigned int offset)
{
	struct inode *inode;

	switch (cram_inode->mode & S_IFMT) {
	case S_IFREG:
		inode = iget_locked(sb, cramino(cramfs_inode, offset));
		if (inode && (inode->i_state & I_NEW)) {
			inode->i_fop = &generic_ro_fops;
			inode->i_data.a_ops = &cramfs_aops;
			setup_inode(inode, cramfs_inode);
		}
		break;
	case S_IFDIR:
		inode = iget_locked(sb, cramino(cramfs_inode, offset));
		if (inode && (inode->i_state & I_NEW)) {
			inode->i_op = &cramfs_dir_inode_operations;
			inode->i_fop = &cramfs_directory_operations;
			setup_inode(inode, cramfs_inode);
		}
		break;
	case S_IFLNK:
		inode = iget_locked(sb, cramino(cramfs_inode, offset));
		if (inode && (inode->i_state & I_NEW)) {
			inode->i_op = &page_symlink_inode_operations;
			inode->i_data.a_ops = &cramfs_aops;
			setup_inode(inode, cramfs_inode);
		}
		break;
	default:
		inode = iget_locked(sb, CRAMINO_UNIQ(offset));
		if (inode && (inode->i_state & I_NEW)) {
			init_special_inode(inode, cramfs_inode->mode,
				old_decode_dev(cramfs_inode->size));
			setup_inode(inode, cramfs_inode);
		}
		break;
	}
	return inode;
}

- Stefani

Re: [PATCH] cramfs: generate unique inode number for better inode cache usage

[Pekka Enberg]

On Wed, Dec 15, 2010 at 9:50 AM, Stefani Seibold <stefani@seibold.net> wrote:
> No, it makes no sense, for following reasons:
>
> - Why pre-setup variables like iops, fops and aops which are in the most
> cases not needed? Because the inode is still in the cache.
>
> - Each branch is a little bit different.
>
> - The default branch is completly different, because in this case the
> init_special_inode() will be called, which sets the inode->fop.
>
> What do you think about this solution:
>
> static void setup_inode(struct inode *inode, struct cramfs_inode * cramfs_inode)
> {
>        static struct timespec zerotime;
>        inode->i_mode = cramfs_inode->mode;
>        inode->i_uid = cramfs_inode->uid;
>        /* if the lower 2 bits are zero, the inode contains data */
>        if (!(inode->i_ino & 3)) {
>                inode->i_size = size;
>                inode->i_blocks = (size - 1) / 512 + 1;
>        }
>        inode->i_gid = cramfs_inode->gid;
>        /* Struct copy intentional */
>        inode->i_mtime = inode->i_atime = inode->i_ctime = zerotime;
>        /* inode->i_nlink is left 1 - arguably wrong for directories,
>           but it's the best we can do without reading the directory
>           contents.  1 yields the right result in GNU find, even
>           without -noleaf option. */
>        unlock_new_inode(inode);
> }
>
> static struct inode *get_cramfs_inode(struct super_block *sb,
>        struct cramfs_inode * cramfs_inode, unsigned int offset)
> {
>        struct inode *inode;
>
>        switch (cram_inode->mode & S_IFMT) {
>        case S_IFREG:
>                inode = iget_locked(sb, cramino(cramfs_inode, offset));
>                if (inode && (inode->i_state & I_NEW)) {
>                        inode->i_fop = &generic_ro_fops;
>                        inode->i_data.a_ops = &cramfs_aops;
>                        setup_inode(inode, cramfs_inode);
>                }
>                break;
>        case S_IFDIR:
>                inode = iget_locked(sb, cramino(cramfs_inode, offset));
>                if (inode && (inode->i_state & I_NEW)) {
>                        inode->i_op = &cramfs_dir_inode_operations;
>                        inode->i_fop = &cramfs_directory_operations;
>                        setup_inode(inode, cramfs_inode);
>                }
>                break;
>        case S_IFLNK:
>                inode = iget_locked(sb, cramino(cramfs_inode, offset));
>                if (inode && (inode->i_state & I_NEW)) {
>                        inode->i_op = &page_symlink_inode_operations;
>                        inode->i_data.a_ops = &cramfs_aops;
>                        setup_inode(inode, cramfs_inode);
>                }
>                break;
>        default:
>                inode = iget_locked(sb, CRAMINO_UNIQ(offset));
>                if (inode && (inode->i_state & I_NEW)) {
>                        init_special_inode(inode, cramfs_inode->mode,
>                                old_decode_dev(cramfs_inode->size));
>                        setup_inode(inode, cramfs_inode);
>                }
>                break;
>        }
>        return inode;
> }

I think Linus is after something like this:

static struct inode *get_cramfs_inode(struct super_block *sb, struct
cramfs_inode * cramfs_inode, unsigned int offset)
{
        struct inode *inode;
        unsigned long ino;

        switch (cram_inode->mode & S_IFMT) {
        case S_IFREG:
        case S_IFDIR:
        case S_IFLNK:
                ino = cramino(cramfs_inode, offset);
                break;
        default:
                ino = CRAMINO_UNIQ(offset);
                break;
        }

        inode = iget_locked(sb, ino);
        if (!inode)
                return ERR_PTR(-ENOMEM);
        if (!(inode->i_state & I_NEW))
                return inode;

       switch (cram_inode->mode & S_IFMT) {
       case S_IFREG:
               inode->i_fop = &generic_ro_fops;
               inode->i_data.a_ops = &cramfs_aops;
               setup_inode(inode, cramfs_inode);
               break;
       case S_IFDIR:
               inode->i_op = &cramfs_dir_inode_operations;
               inode->i_fop = &cramfs_directory_operations;
               setup_inode(inode, cramfs_inode);
               break;
       case S_IFLNK:
               inode->i_op = &page_symlink_inode_operations;
               inode->i_data.a_ops = &cramfs_aops;
               setup_inode(inode, cramfs_inode);
               break;
       default:
               init_special_inode(inode, cramfs_inode->mode,
old_decode_dev(cramfs_inode->size));
               setup_inode(inode, cramfs_inode);
               break;
       }
       return inode;
}

You should optimize for the in-cache case.

Re: [PATCH] cramfs: generate unique inode number for better inode cache usage

[Linus Torvalds]

On Wed, Dec 15, 2010 at 12:15 AM, Pekka Enberg <penberg@kernel.org> wrote:
>
> I think Linus is after something like this:

That works for me, but I hate the double "switch()" statement checking
the same thing.

Why is the camino() function not just doing something like

 static unsigned int cramino(struct cramfs_inode *ino, unsigned int
dirent_offset)
 {
     unsigned int data_offset = ino->offset;
     return data_offset ? data_offset << 2 : dirent_offset+1;
 }

and we can get rid of all the cruddy CRAMINO_UNIQ() and testing of the
mode entirely for that path.

That, together with Pekka's approach seems nice, simple and clean.

                            Linus

Re: [PATCH] cramfs: generate unique inode number for better inode cache usage

[stefani]

Zitat von Linus Torvalds <torvalds@linux-foundation.org>:

> On Wed, Dec 15, 2010 at 12:15 AM, Pekka Enberg <penberg@kernel.org> wrote:
>>
>> I think Linus is after something like this:
>
> That works for me, but I hate the double "switch()" statement checking
> the same thing.
>
> Why is the camino() function not just doing something like
>
>  static unsigned int cramino(struct cramfs_inode *ino, unsigned int
> dirent_offset)
>  {
>      unsigned int data_offset = ino->offset;
>      return data_offset ? data_offset << 2 : dirent_offset+1;
>  }
>
> and we can get rid of all the cruddy CRAMINO_UNIQ() and testing of the
> mode entirely for that path.
>
> That, together with Pekka's approach seems nice, simple and clean.
>
>                             Linus
>

Pekka's approach is not problem. But the "cruddy" CRAMINO_UNIQ() is  
exact what is needed. In your orginal design of cramfs there is no way  
to give entries with no data an unique inode number.

So for not to waste the inode cache and do useless lookups it it  
necessary to distinguish between entries with data and entries with no  
data. The later one need a special handling to create a unique inode  
number depending on the index of the directory entry inside the cramfs  
image.

I try my best to make best out of of the old cramfs design and fix the  
issues of this filesystem.

- Stefani

Re: [PATCH] cramfs: generate unique inode number for better inode cache usage

[Linus Torvalds]

On Wed, Dec 15, 2010 at 8:31 AM,  <stefani@seibold.net> wrote:
>
> Pekka's approach is not problem. But the "cruddy" CRAMINO_UNIQ() is exact
> what is needed. In your orginal design of cramfs there is no way to give
> entries with no data an unique inode number.

Umm. And my cramino() did exactly that. If it has a data pointer, it
uses that, otherwise it uses the directory offset.

It just doesn't care about the mode of the file.

(feel free to test the size too, for that matter - that's what the old
code does)

IOW, it uses the old CRAMINO() approach, except instead of "1" for the
non-offset case, it uses "dirent_offset+1"

                   Linus

Re: [PATCH] cramfs: generate unique inode number for better inode cache usage

[Stefani Seibold]

Am Mittwoch, den 15.12.2010, 08:45 -0800 schrieb Linus Torvalds:
> On Wed, Dec 15, 2010 at 8:31 AM,  <stefani@seibold.net> wrote:
> >
> > Pekka's approach is not problem. But the "cruddy" CRAMINO_UNIQ() is exact
> > what is needed. In your orginal design of cramfs there is no way to give
> > entries with no data an unique inode number.
> 
> Umm. And my cramino() did exactly that. If it has a data pointer, it
> uses that, otherwise it uses the directory offset.
> 

The current implementation does it not.

> It just doesn't care about the mode of the file.
> 

Right, but there are a buggy cramfs tools (which can create images for
different endian target or create on the fly devices nodes and so on)
out there, which set the offset of the cramfs_inode to a value not equal
0. This works well with older kernel like 2.6.12 but during the
evolution of the kernel this was broken.

I will resend a cleaned up patch. Hope you will apply it ;-)

Stefani

[PATCH] cramfs: generate unique inode number for better inode cache usage

[stefani]

From: Stefani Seibold <stefani@seibold.net>

This is fourth version. Changelog
 - 12.12.2010 first version
 - 15.12.2010 code cleanup suggest by Linus
 - 16.12.2010 code cleanup suggset by Pekka Enberg
 - 16.12.2010 fix comment, move setup_inode() code into get_cramfs_inode()

This patch generates a unique inode numbers for any entries in the cram file
system. For files which did not contain data's (device nodes, fifos and
sockets) the offset of the directory entry inside the cramfs plus 1 will be
used as inode number.

The + 1 for the inode will it make possible to distinguish between
a file which contains no data and files which has data, the later one has
a inode value where the lower two bits are always 0.

It also reimplement the behavoir to set the size and the number of block
to 0 for special file, which is the right value for empty files,
devices, fifos and sockets

As a little benefit it will be also more compatible which older
mkcramfs, because it will never use the cramfs_inode->offset for
creating a inode number for special files.

The patch is aginst 2.6.37-rc5

Please apply it.

Thanks,
Stefani

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 fs/cramfs/inode.c |  110 +++++++++++++++++++++++++++++++++--------------------
 1 files changed, 69 insertions(+), 41 deletions(-)

diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
index 32fd5fe..dfb0029 100644
--- a/fs/cramfs/inode.c
+++ b/fs/cramfs/inode.c
@@ -34,57 +34,81 @@ static const struct address_space_operations cramfs_aops;
 static DEFINE_MUTEX(read_mutex);
 
 
-/* These two macros may change in future, to provide better st_ino
-   semantics. */
-#define CRAMINO(x)	(((x)->offset && (x)->size)?(x)->offset<<2:1)
+/* These macros may change in future, to provide better st_ino semantics. */
 #define OFFSET(x)	((x)->i_ino)
 
-static void setup_inode(struct inode *inode, struct cramfs_inode * cramfs_inode)
+static unsigned long cramino(struct cramfs_inode * cino, unsigned int offset)
 {
+	if (!cino->offset)
+		return offset + 1;
+	if (!cino->size)
+		return offset + 1;
+
+	/*
+	 * The file mode test fix buggy mkcramfs implementations where
+	 * cramfs_inode->offset is set to a non zero value for entries
+	 * which did not contain data, like devices node and fifos.
+	 */
+	switch (cino->mode & S_IFMT) {
+	case S_IFREG:
+	case S_IFDIR:
+	case S_IFLNK:
+		return cino->offset << 2;
+	default:
+		break;
+	}
+	return offset + 1;
+}
+
+static struct inode *get_cramfs_inode(struct super_block *sb,
+	struct cramfs_inode * cramfs_inode, unsigned int offset)
+{
+	struct inode *inode;
 	static struct timespec zerotime;
+
+	inode = iget_locked(sb, cramino(cramfs_inode, offset));
+	if (!inode)
+		return ERR_PTR(-ENOMEM);
+	if (!(inode->i_state & I_NEW))
+		return inode;
+
+	switch (cramfs_inode->mode & S_IFMT) {
+	case S_IFREG:
+		inode->i_fop = &generic_ro_fops;
+		inode->i_data.a_ops = &cramfs_aops;
+		break;
+	case S_IFDIR:
+		inode->i_op = &cramfs_dir_inode_operations;
+		inode->i_fop = &cramfs_directory_operations;
+		break;
+	case S_IFLNK:
+		inode->i_op = &page_symlink_inode_operations;
+		inode->i_data.a_ops = &cramfs_aops;
+		break;
+	default:
+		init_special_inode(inode, cramfs_inode->mode,
+				old_decode_dev(cramfs_inode->size));
+	}
+
 	inode->i_mode = cramfs_inode->mode;
 	inode->i_uid = cramfs_inode->uid;
-	inode->i_size = cramfs_inode->size;
-	inode->i_blocks = (cramfs_inode->size - 1) / 512 + 1;
 	inode->i_gid = cramfs_inode->gid;
+
+	/* if the lower 2 bits are zero, the inode contains data */
+	if (!(inode->i_ino & 3)) {
+		inode->i_size = cramfs_inode->size;
+		inode->i_blocks = (cramfs_inode->size - 1) / 512 + 1;
+	}
+
 	/* Struct copy intentional */
 	inode->i_mtime = inode->i_atime = inode->i_ctime = zerotime;
 	/* inode->i_nlink is left 1 - arguably wrong for directories,
 	   but it's the best we can do without reading the directory
 	   contents.  1 yields the right result in GNU find, even
 	   without -noleaf option. */
-	if (S_ISREG(inode->i_mode)) {
-		inode->i_fop = &generic_ro_fops;
-		inode->i_data.a_ops = &cramfs_aops;
-	} else if (S_ISDIR(inode->i_mode)) {
-		inode->i_op = &cramfs_dir_inode_operations;
-		inode->i_fop = &cramfs_directory_operations;
-	} else if (S_ISLNK(inode->i_mode)) {
-		inode->i_op = &page_symlink_inode_operations;
-		inode->i_data.a_ops = &cramfs_aops;
-	} else {
-		init_special_inode(inode, inode->i_mode,
-			old_decode_dev(cramfs_inode->size));
-	}
-}
 
-static struct inode *get_cramfs_inode(struct super_block *sb,
-				struct cramfs_inode * cramfs_inode)
-{
-	struct inode *inode;
-	if (CRAMINO(cramfs_inode) == 1) {
-		inode = new_inode(sb);
-		if (inode) {
-			inode->i_ino = 1;
-			setup_inode(inode, cramfs_inode);
-		}
-	} else {
-		inode = iget_locked(sb, CRAMINO(cramfs_inode));
-		if (inode && (inode->i_state & I_NEW)) {
-			setup_inode(inode, cramfs_inode);
-			unlock_new_inode(inode);
-		}
-	}
+	unlock_new_inode(inode);
+
 	return inode;
 }
 
@@ -265,6 +289,9 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 		printk(KERN_ERR "cramfs: root is not a directory\n");
 		goto out;
 	}
+	/* correct strange, hard-coded permissions of mkcramfs */
+	super.root.mode |= (S_IRUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
+
 	root_offset = super.root.offset << 2;
 	if (super.flags & CRAMFS_FLAG_FSID_VERSION_2) {
 		sbi->size=super.size;
@@ -289,7 +316,7 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 
 	/* Set it all up.. */
 	sb->s_op = &cramfs_ops;
-	root = get_cramfs_inode(sb, &super.root);
+	root = get_cramfs_inode(sb, &super.root, 0);
 	if (!root)
 		goto out;
 	sb->s_root = d_alloc_root(root);
@@ -365,7 +392,7 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 		 */
 		namelen = de->namelen << 2;
 		memcpy(buf, name, namelen);
-		ino = CRAMINO(de);
+		ino = cramino(de, OFFSET(inode) + offset);
 		mode = de->mode;
 		mutex_unlock(&read_mutex);
 		nextoffset = offset + sizeof(*de) + namelen;
@@ -404,8 +431,9 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 		struct cramfs_inode *de;
 		char *name;
 		int namelen, retval;
+		int dir_off = OFFSET(dir) + offset;
 
-		de = cramfs_read(dir->i_sb, OFFSET(dir) + offset, sizeof(*de)+CRAMFS_MAXPATHLEN);
+		de = cramfs_read(dir->i_sb, dir_off, sizeof(*de)+CRAMFS_MAXPATHLEN);
 		name = (char *)(de+1);
 
 		/* Try to take advantage of sorted directories */
@@ -436,7 +464,7 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 		if (!retval) {
 			struct cramfs_inode entry = *de;
 			mutex_unlock(&read_mutex);
-			d_add(dentry, get_cramfs_inode(dir->i_sb, &entry));
+			d_add(dentry, get_cramfs_inode(dir->i_sb, &entry, dir_off));
 			return NULL;
 		}
 		/* else (retval < 0) */
-- 
1.7.3.3

[PATCH] cramfs: generate unique inode number for better inode cache usage

[stefani]

From: Stefani Seibold <stefani@seibold.net>

This is third version, hope this is less ugly code :-)

This patch generates a unique inode numbers for any entries in the cram file
system. For files which did not contain data's (device nodes, fifos and
sockets) the offset of the directory entry inside the cramfs plus 1 will be
used as inode number.

The + 1 for the inode will it make possible to distinguish between
a file which contains no data and files which has data, the later one has
a inode value where the lower two bits are always 0.

It also reimplement the behavoir to set the size and the number of block
to 0 for special file, which is the right value for empty files,
devices, fifos and sockets

As a little benefit it will be also more compatible which older
mkcramfs, because it will never use the cramfs_inode->offset for
creating a inode number for special files.

The patch is aginst 2.6.37-rc5

Please apply it.

Thanks,
Stefani

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 fs/cramfs/inode.c |   94 +++++++++++++++++++++++++++++++++-------------------
 1 files changed, 60 insertions(+), 34 deletions(-)

diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
index 32fd5fe..ced8912 100644
--- a/fs/cramfs/inode.c
+++ b/fs/cramfs/inode.c
@@ -34,18 +34,38 @@ static const struct address_space_operations cramfs_aops;
 static DEFINE_MUTEX(read_mutex);
 
 
-/* These two macros may change in future, to provide better st_ino
-   semantics. */
-#define CRAMINO(x)	(((x)->offset && (x)->size)?(x)->offset<<2:1)
+/* These macros may change in future, to provide better st_ino semantics. */
 #define OFFSET(x)	((x)->i_ino)
 
-static void setup_inode(struct inode *inode, struct cramfs_inode * cramfs_inode)
+static unsigned long cramino(struct cramfs_inode * cino, unsigned int offset)
+{
+	if (!cino->offset)
+		return offset + 1;
+	if (!cino->size)
+		return offset + 1;
+
+	/* the mode test fix buggy mkcramfs implementations */
+	switch (cino->mode & S_IFMT) {
+	case S_IFREG:
+	case S_IFDIR:
+	case S_IFLNK:
+		return cino->offset << 2;
+	default:
+		break;
+	}
+	return offset + 1;
+}
+
+static inline void setup_inode(struct inode *inode, struct cramfs_inode * cramfs_inode)
 {
 	static struct timespec zerotime;
 	inode->i_mode = cramfs_inode->mode;
 	inode->i_uid = cramfs_inode->uid;
-	inode->i_size = cramfs_inode->size;
-	inode->i_blocks = (cramfs_inode->size - 1) / 512 + 1;
+	/* if the lower 2 bits are zero, the inode contains data */
+	if (!(inode->i_ino & 3)) {
+		inode->i_size = cramfs_inode->size;
+		inode->i_blocks = (cramfs_inode->size - 1) / 512 + 1;
+	}
 	inode->i_gid = cramfs_inode->gid;
 	/* Struct copy intentional */
 	inode->i_mtime = inode->i_atime = inode->i_ctime = zerotime;
@@ -53,38 +73,40 @@ static void setup_inode(struct inode *inode, struct cramfs_inode * cramfs_inode)
 	   but it's the best we can do without reading the directory
 	   contents.  1 yields the right result in GNU find, even
 	   without -noleaf option. */
-	if (S_ISREG(inode->i_mode)) {
+	unlock_new_inode(inode);
+}
+
+static struct inode *get_cramfs_inode(struct super_block *sb,
+	struct cramfs_inode * cramfs_inode, unsigned int offset)
+{
+	struct inode *inode;
+
+	inode = iget_locked(sb, cramino(cramfs_inode, offset));
+	if (!inode)
+		return ERR_PTR(-ENOMEM);
+	if (!(inode->i_state & I_NEW))
+		return inode;
+
+	switch (cramfs_inode->mode & S_IFMT) {
+	case S_IFREG:
 		inode->i_fop = &generic_ro_fops;
 		inode->i_data.a_ops = &cramfs_aops;
-	} else if (S_ISDIR(inode->i_mode)) {
+		break;
+	case S_IFDIR:
 		inode->i_op = &cramfs_dir_inode_operations;
 		inode->i_fop = &cramfs_directory_operations;
-	} else if (S_ISLNK(inode->i_mode)) {
+		break;
+	case S_IFLNK:
 		inode->i_op = &page_symlink_inode_operations;
 		inode->i_data.a_ops = &cramfs_aops;
-	} else {
-		init_special_inode(inode, inode->i_mode,
-			old_decode_dev(cramfs_inode->size));
+		break;
+	default:
+		init_special_inode(inode, cramfs_inode->mode,
+				old_decode_dev(cramfs_inode->size));
 	}
-}
 
-static struct inode *get_cramfs_inode(struct super_block *sb,
-				struct cramfs_inode * cramfs_inode)
-{
-	struct inode *inode;
-	if (CRAMINO(cramfs_inode) == 1) {
-		inode = new_inode(sb);
-		if (inode) {
-			inode->i_ino = 1;
-			setup_inode(inode, cramfs_inode);
-		}
-	} else {
-		inode = iget_locked(sb, CRAMINO(cramfs_inode));
-		if (inode && (inode->i_state & I_NEW)) {
-			setup_inode(inode, cramfs_inode);
-			unlock_new_inode(inode);
-		}
-	}
+	setup_inode(inode, cramfs_inode);
+
 	return inode;
 }
 
@@ -265,6 +287,9 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 		printk(KERN_ERR "cramfs: root is not a directory\n");
 		goto out;
 	}
+	/* correct strange, hard-coded permissions of mkcramfs */
+	super.root.mode |= (S_IRUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
+
 	root_offset = super.root.offset << 2;
 	if (super.flags & CRAMFS_FLAG_FSID_VERSION_2) {
 		sbi->size=super.size;
@@ -289,7 +314,7 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 
 	/* Set it all up.. */
 	sb->s_op = &cramfs_ops;
-	root = get_cramfs_inode(sb, &super.root);
+	root = get_cramfs_inode(sb, &super.root, 0);
 	if (!root)
 		goto out;
 	sb->s_root = d_alloc_root(root);
@@ -365,7 +390,7 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 		 */
 		namelen = de->namelen << 2;
 		memcpy(buf, name, namelen);
-		ino = CRAMINO(de);
+		ino = cramino(de, OFFSET(inode) + offset);
 		mode = de->mode;
 		mutex_unlock(&read_mutex);
 		nextoffset = offset + sizeof(*de) + namelen;
@@ -404,8 +429,9 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 		struct cramfs_inode *de;
 		char *name;
 		int namelen, retval;
+		int dir_off = OFFSET(dir) + offset;
 
-		de = cramfs_read(dir->i_sb, OFFSET(dir) + offset, sizeof(*de)+CRAMFS_MAXPATHLEN);
+		de = cramfs_read(dir->i_sb, dir_off, sizeof(*de)+CRAMFS_MAXPATHLEN);
 		name = (char *)(de+1);
 
 		/* Try to take advantage of sorted directories */
@@ -436,7 +462,7 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 		if (!retval) {
 			struct cramfs_inode entry = *de;
 			mutex_unlock(&read_mutex);
-			d_add(dentry, get_cramfs_inode(dir->i_sb, &entry));
+			d_add(dentry, get_cramfs_inode(dir->i_sb, &entry, dir_off));
 			return NULL;
 		}
 		/* else (retval < 0) */
-- 
1.7.3.3

Re: [PATCH] cramfs: generate unique inode number for better inode cache usage

[Pekka Enberg]

On 12/16/10 11:52 AM, stefani@seibold.net wrote:
> +static unsigned long cramino(struct cramfs_inode * cino, unsigned int offset)
> +{
> +	if (!cino->offset)
> +		return offset + 1;
> +	if (!cino->size)
> +		return offset + 1;
> +
> +	/* the mode test fix buggy mkcramfs implementations */

I'm getting into the bikeshedding territory but a less terse comment 
would be in order here, I think.

> +	switch (cino->mode&  S_IFMT) {
> +	case S_IFREG:
> +	case S_IFDIR:
> +	case S_IFLNK:
> +		return cino->offset<<  2;
> +	default:
> +		break;
> +	}
> +	return offset + 1;
> +}

[PATCH] cramfs: generate unique inode number for better inode cache usage

[stefani]

From: Stefani Seibold <stefani@seibold.net>

This patch generates a unique inode numbers for any entries in the cram file
system. For files which did not contain data's (device nodes, fifos and
sockets) the offset of the directory entry inside + 1 will be used as the
inode number.

The + 1 for the inode will it make possible to distinguish between
a file which contains no data and files which has data, the later one has
a inode value where the lower two bits are always 0.

It also reimplement the behavoir to set the size and the number of block
to 0 for special file, which is the right value for devices, fifos and
sockets.

As a little benefit it will be also more compatible which older
mkcramfs, because it will never use the cramfs_inode->offset for
creating a inode number for special files.

Please apply it.

Thanks,
Stefani

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 fs/cramfs/inode.c |   99 +++++++++++++++++++++++++++++-----------------------
 1 files changed, 55 insertions(+), 44 deletions(-)

diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
index 32fd5fe..78ab713 100644
--- a/fs/cramfs/inode.c
+++ b/fs/cramfs/inode.c
@@ -36,54 +36,61 @@ static DEFINE_MUTEX(read_mutex);
 
 /* These two macros may change in future, to provide better st_ino
    semantics. */
-#define CRAMINO(x)	(((x)->offset && (x)->size)?(x)->offset<<2:1)
 #define OFFSET(x)	((x)->i_ino)
 
-static void setup_inode(struct inode *inode, struct cramfs_inode * cramfs_inode)
+static inline unsigned long cramino(struct cramfs_inode * cino,
+					unsigned int offset)
 {
-	static struct timespec zerotime;
-	inode->i_mode = cramfs_inode->mode;
-	inode->i_uid = cramfs_inode->uid;
-	inode->i_size = cramfs_inode->size;
-	inode->i_blocks = (cramfs_inode->size - 1) / 512 + 1;
-	inode->i_gid = cramfs_inode->gid;
-	/* Struct copy intentional */
-	inode->i_mtime = inode->i_atime = inode->i_ctime = zerotime;
-	/* inode->i_nlink is left 1 - arguably wrong for directories,
-	   but it's the best we can do without reading the directory
-	   contents.  1 yields the right result in GNU find, even
-	   without -noleaf option. */
-	if (S_ISREG(inode->i_mode)) {
-		inode->i_fop = &generic_ro_fops;
-		inode->i_data.a_ops = &cramfs_aops;
-	} else if (S_ISDIR(inode->i_mode)) {
-		inode->i_op = &cramfs_dir_inode_operations;
-		inode->i_fop = &cramfs_directory_operations;
-	} else if (S_ISLNK(inode->i_mode)) {
-		inode->i_op = &page_symlink_inode_operations;
-		inode->i_data.a_ops = &cramfs_aops;
-	} else {
-		init_special_inode(inode, inode->i_mode,
-			old_decode_dev(cramfs_inode->size));
-	}
+	if (!cino->offset)
+		return offset + 1;
+	if (!cino->size)
+		return offset + 1;
+	if (S_ISCHR(cino->mode))
+		return offset + 1;
+	if (S_ISBLK(cino->mode))
+		return offset + 1;
+	if (S_ISFIFO(cino->mode))
+		return offset + 1;
+	if (S_ISSOCK(cino->mode))
+		return offset + 1;
+
+	return cino->offset << 2;
 }
 
 static struct inode *get_cramfs_inode(struct super_block *sb,
-				struct cramfs_inode * cramfs_inode)
+	struct cramfs_inode * cramfs_inode, unsigned int offset)
 {
-	struct inode *inode;
-	if (CRAMINO(cramfs_inode) == 1) {
-		inode = new_inode(sb);
-		if (inode) {
-			inode->i_ino = 1;
-			setup_inode(inode, cramfs_inode);
-		}
-	} else {
-		inode = iget_locked(sb, CRAMINO(cramfs_inode));
-		if (inode && (inode->i_state & I_NEW)) {
-			setup_inode(inode, cramfs_inode);
-			unlock_new_inode(inode);
+	static struct timespec zerotime;
+	struct inode *inode = iget_locked(sb, cramino(cramfs_inode, offset));
+
+	if (inode && (inode->i_state & I_NEW)) {
+		inode->i_mode = cramfs_inode->mode;
+		inode->i_uid = cramfs_inode->uid;
+		inode->i_size = cramfs_inode->size;
+		inode->i_blocks = (cramfs_inode->size - 1) / 512 + 1;
+		inode->i_gid = cramfs_inode->gid;
+		/* Struct copy intentional */
+		inode->i_mtime = inode->i_atime = inode->i_ctime = zerotime;
+		/* inode->i_nlink is left 1 - arguably wrong for directories,
+		   but it's the best we can do without reading the directory
+		   contents.  1 yields the right result in GNU find, even
+		   without -noleaf option. */
+		if (S_ISREG(inode->i_mode)) {
+			inode->i_fop = &generic_ro_fops;
+			inode->i_data.a_ops = &cramfs_aops;
+		} else if (S_ISDIR(inode->i_mode)) {
+			inode->i_op = &cramfs_dir_inode_operations;
+			inode->i_fop = &cramfs_directory_operations;
+		} else if (S_ISLNK(inode->i_mode)) {
+			inode->i_op = &page_symlink_inode_operations;
+			inode->i_data.a_ops = &cramfs_aops;
+		} else {
+			inode->i_size = 0;
+			inode->i_blocks = 0;
+			init_special_inode(inode, inode->i_mode,
+				old_decode_dev(cramfs_inode->size));
 		}
+		unlock_new_inode(inode);
 	}
 	return inode;
 }
@@ -265,6 +272,9 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 		printk(KERN_ERR "cramfs: root is not a directory\n");
 		goto out;
 	}
+	/* correct strange, hard-coded permissions of mkcramfs */
+	super.root.mode |= (S_IRUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
+
 	root_offset = super.root.offset << 2;
 	if (super.flags & CRAMFS_FLAG_FSID_VERSION_2) {
 		sbi->size=super.size;
@@ -289,7 +299,7 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 
 	/* Set it all up.. */
 	sb->s_op = &cramfs_ops;
-	root = get_cramfs_inode(sb, &super.root);
+	root = get_cramfs_inode(sb, &super.root, 0);
 	if (!root)
 		goto out;
 	sb->s_root = d_alloc_root(root);
@@ -365,7 +375,7 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 		 */
 		namelen = de->namelen << 2;
 		memcpy(buf, name, namelen);
-		ino = CRAMINO(de);
+		ino = cramino(de, OFFSET(inode) + offset);
 		mode = de->mode;
 		mutex_unlock(&read_mutex);
 		nextoffset = offset + sizeof(*de) + namelen;
@@ -404,8 +414,9 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 		struct cramfs_inode *de;
 		char *name;
 		int namelen, retval;
+		int dir_off = OFFSET(dir) + offset;
 
-		de = cramfs_read(dir->i_sb, OFFSET(dir) + offset, sizeof(*de)+CRAMFS_MAXPATHLEN);
+		de = cramfs_read(dir->i_sb, dir_off, sizeof(*de)+CRAMFS_MAXPATHLEN);
 		name = (char *)(de+1);
 
 		/* Try to take advantage of sorted directories */
@@ -436,7 +447,7 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 		if (!retval) {
 			struct cramfs_inode entry = *de;
 			mutex_unlock(&read_mutex);
-			d_add(dentry, get_cramfs_inode(dir->i_sb, &entry));
+			d_add(dentry, get_cramfs_inode(dir->i_sb, &entry, dir_off));
 			return NULL;
 		}
 		/* else (retval < 0) */
-- 
1.7.3.3

Re: [PATCH] cramfs: generate unique inode number for better inode cache usage

[Andrew Morton]

On Sun, 12 Dec 2010 11:48:42 +0100
stefani@seibold.net wrote:

> This patch generates a unique inode numbers for any entries in the cram file
> system. For files which did not contain data's (device nodes, fifos and
> sockets) the offset of the directory entry inside + 1 will be used as the
> inode number.
> 
> The + 1 for the inode will it make possible to distinguish between
> a file which contains no data and files which has data, the later one has
> a inode value where the lower two bits are always 0.
> 
> It also reimplement the behavoir to set the size and the number of block
> to 0 for special file, which is the right value for devices, fifos and
> sockets.
> 
> As a little benefit it will be also more compatible which older
> mkcramfs, because it will never use the cramfs_inode->offset for
> creating a inode number for special files.

Did you look at using iunique() to generate cramfs inode numbers?

Re: [PATCH] cramfs: generate unique inode number for better inode cache usage

[Stefani Seibold]

Am Dienstag, den 14.12.2010, 12:51 -0800 schrieb Andrew Morton:
> On Sun, 12 Dec 2010 11:48:42 +0100
> stefani@seibold.net wrote:
> 
> > This patch generates a unique inode numbers for any entries in the cram file
> > system. For files which did not contain data's (device nodes, fifos and
> > sockets) the offset of the directory entry inside + 1 will be used as the
> > inode number.
> > 
> > The + 1 for the inode will it make possible to distinguish between
> > a file which contains no data and files which has data, the later one has
> > a inode value where the lower two bits are always 0.
> > 
> > It also reimplement the behavoir to set the size and the number of block
> > to 0 for special file, which is the right value for devices, fifos and
> > sockets.
> > 
> > As a little benefit it will be also more compatible which older
> > mkcramfs, because it will never use the cramfs_inode->offset for
> > creating a inode number for special files.
> 
> Did you look at using iunique() to generate cramfs inode numbers?

iunique() will create random inode numbers, depending on the order of
the accessed files. My solution returns always the same inode numbers
for the files. It is also faster, because there is no need to lookup
inode cache for the given super block. And at last the generating of the
inode number is very simple.

Re: [PATCH] cramfs: generate unique inode number for better inode cache usage

[Andrew Morton]

On Tue, 14 Dec 2010 22:02:30 +0100
Stefani Seibold <stefani@seibold.net> wrote:

> Am Dienstag, den 14.12.2010, 12:51 -0800 schrieb Andrew Morton:
> > On Sun, 12 Dec 2010 11:48:42 +0100
> > stefani@seibold.net wrote:
> > 
> > > This patch generates a unique inode numbers for any entries in the cram file
> > > system. For files which did not contain data's (device nodes, fifos and
> > > sockets) the offset of the directory entry inside + 1 will be used as the
> > > inode number.
> > > 
> > > The + 1 for the inode will it make possible to distinguish between
> > > a file which contains no data and files which has data, the later one has
> > > a inode value where the lower two bits are always 0.
> > > 
> > > It also reimplement the behavoir to set the size and the number of block
> > > to 0 for special file, which is the right value for devices, fifos and
> > > sockets.
> > > 
> > > As a little benefit it will be also more compatible which older
> > > mkcramfs, because it will never use the cramfs_inode->offset for
> > > creating a inode number for special files.
> > 
> > Did you look at using iunique() to generate cramfs inode numbers?
> 
> iunique() will create random inode numbers, depending on the order of
> the accessed files.

It generates non-unique inode numbers and uses Weird Nonstandard
Private Stuff in a place where we could use standard kernel facilities.

Re: [PATCH] cramfs: generate unique inode number for better inode cache usage

[Stefani Seibold]

Am Dienstag, den 14.12.2010, 13:12 -0800 schrieb Andrew Morton:
> On Tue, 14 Dec 2010 22:02:30 +0100
> Stefani Seibold <stefani@seibold.net> wrote:
> 
> > Am Dienstag, den 14.12.2010, 12:51 -0800 schrieb Andrew Morton:
> > > On Sun, 12 Dec 2010 11:48:42 +0100
> > > stefani@seibold.net wrote:
> > > 
> > > > This patch generates a unique inode numbers for any entries in the cram file
> > > > system. For files which did not contain data's (device nodes, fifos and
> > > > sockets) the offset of the directory entry inside + 1 will be used as the
> > > > inode number.
> > > > 
> > > > The + 1 for the inode will it make possible to distinguish between
> > > > a file which contains no data and files which has data, the later one has
> > > > a inode value where the lower two bits are always 0.
> > > > 
> > > > It also reimplement the behavoir to set the size and the number of block
> > > > to 0 for special file, which is the right value for devices, fifos and
> > > > sockets.
> > > > 
> > > > As a little benefit it will be also more compatible which older
> > > > mkcramfs, because it will never use the cramfs_inode->offset for
> > > > creating a inode number for special files.
> > > 
> > > Did you look at using iunique() to generate cramfs inode numbers?
> > 
> > iunique() will create random inode numbers, depending on the order of
> > the accessed files.
> 
> It generates non-unique inode numbers and uses Weird Nonstandard
> Private Stuff in a place where we could use standard kernel facilities.

Disagree, it will create uniqe inode. And if the "standard" kernel
facilities will not serve right, than it is necessary to do in a better
way. iunique() is not the right facility.

Stefani

Re: [PATCH] cramfs: generate unique inode number for better inode cache usage

[Linus Torvalds]

On Tue, Dec 14, 2010 at 12:51 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> Did you look at using iunique() to generate cramfs inode numbers?

That breaks the cramfs "hardlinking" (which is just files that have
the same data pointer), and now a hardlinked file wouldn't have the
same inode number any more.

Of course, I'm not sure the hardlinking really matters. cramfs
hardlinks aren't really traditional hardlinks anyway - since the
permissions etc are in the directory entry, you can have the data
hardlinked without having the same permissions, so it's not a "real"
hardlink even if the inode number were to be the same.

But this patch seems to roughly approximate the old pseudo-hardlink
behavior. It used to be that all non-data files showed up with the
same inode number, now they have separate inode numbers.

That said, I hate how it moves that "setup_inode" helper function
inline and then does the "if it's a character device" kinds of tests
twice. Once for the inode number logic, and once for the inode
operations structure assignment.

So I think the approach is fine, but I think the implementation is pretty ugly.

                          Linus

Re: [PATCH] cramfs: generate unique inode number for better inode cache usage

[Stefani Seibold]

Am Dienstag, den 14.12.2010, 13:08 -0800 schrieb Linus Torvalds:
> On Tue, Dec 14, 2010 at 12:51 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> >
> > Did you look at using iunique() to generate cramfs inode numbers?
> 
> That breaks the cramfs "hardlinking" (which is just files that have
> the same data pointer), and now a hardlinked file wouldn't have the
> same inode number any more.
> 
> Of course, I'm not sure the hardlinking really matters. cramfs
> hardlinks aren't really traditional hardlinks anyway - since the
> permissions etc are in the directory entry, you can have the data
> hardlinked without having the same permissions, so it's not a "real"
> hardlink even if the inode number were to be the same.
> 

In my opinion hardlinks doesn't matter, because cramfs has no real
hardlinks.

> But this patch seems to roughly approximate the old pseudo-hardlink
> behavior. It used to be that all non-data files showed up with the
> same inode number, now they have separate inode numbers.
> 
> That said, I hate how it moves that "setup_inode" helper function
> inline and then does the "if it's a character device" kinds of tests
> twice. Once for the inode number logic, and once for the inode
> operations structure assignment.
> 
> So I think the approach is fine, but I think the implementation is pretty ugly.
> 

Okay, i will see if i find a better solution. The problem is that the
inode number generation CRAMINO will be called from two different
functions, get_cramfs_inode() and cramfs_readdir(). So it is much more
readable to have one function which creates the inode than an two
optimized implementations.

Stefani