Messages in this thread | | | Subject | Re: [PATCH] cramfs: generate unique inode number for better inode cache usage | From | Stefani Seibold <> | Date | Wed, 15 Dec 2010 08:50:18 +0100 |
| |
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
| |