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

[Stefani Seibold <stefani@seibold.net>]

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