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

Stefani Seibold
2010-12-15T07:49:29Z

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