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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 15 Dec 2010 10:15:42 +0200
From:	Pekka Enberg <penberg@...nel.org>
To:	Stefani Seibold <stefani@...bold.net>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
	Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH] cramfs: generate unique inode number for better inode
 cache usage

On Wed, Dec 15, 2010 at 9:50 AM, Stefani Seibold <stefani@...bold.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/