lkml.org 
[lkml]   [2007]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Subject[PATCH 00/31] Remove iget() and read_inode() [try #5]
    Date


    Here's a set of patches that remove all calls to iget() and all read_inode()
    functions. They should be removed for two reasons: firstly they don't lend
    themselves to good error handling, and secondly their presence is a temptation
    for code outside a filesystem to call iget() to access inodes within that
    filesystem.

    There are a few benefits to this:

    (1) Error handling gets simpler as you can return an error code rather than
    having to call is_bad_inode().

    (2) You can now tell the difference between ENOMEM and EIO occurring in the
    read_inode() path.

    (3) The code should get smaller. iget() is an inline function that is
    typically called 2-3 times per filesystem that uses it. By folding the
    iget code into the read_inode code for each filesystem, it eliminates
    some duplication.

    A tarball of the patches can be retrieved from:

    http://people.redhat.com/~dhowells/iget-remove.tar.bz2


    Additionally, there are a couple of patches that introduce an ERR_CAST() macro
    to be used instead of ERR_PTR(PTR_ERR(p)) constructs and apply it to all such
    instances in the kernel.

    Of the patches directly relevant to the subject:

    The first patch adds a function, iget_failed() that is a canned piece of code
    for killing an inode when the inode construction path fails.

    The second and third patches makes AFS and GFS2 use iget_failed() rather than
    interpolating the sequence directly.

    The final patch removes iget() and read_inode().

    Each of the other patches modify a filesystem that used iget() and read_inode()
    to use iget_locked() instead. The standard procedure was to convert:

    void thingyfs_read_inode(struct inode *inode)
    {
    ...
    }

    into:

    struct inode *thingyfs_iget(struct super_block *sp, unsigned long ino)
    {
    struct inode *inode;
    int ret;

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

    ...
    unlock_new_inode(inode);
    return inode;
    error:
    iget_failed(inode);
    return ERR_PTR(ret);
    }

    and then call thingyfs_iget() rather than iget():

    ret = -EINVAL;
    inode = iget(sb, ino);
    if (!inode || is_bad_inode(inode))
    goto error;

    becomes:

    inode = thingyfs_iget(sb, ino);
    if (IS_ERR(inode)) {
    ret = PTR_ERR(inode);
    goto error;
    }

    There were exceptions; most notably it appeared FAT should be calling ilookup()
    not iget().

    Additionally, HPPFS and HOSTFS (UM-specific filesystems) really need checking:

    hostfs_kern.c:

    (*) hostfs_iget() should perhaps subsume init_inode() and hostfs_read_inode().

    (*) It would appear that all hostfs inodes are the same inode because iget()
    was being called with inode number 0 - which forms the lookup key.

    hppfs_kern.c:

    (*) The HPPFS inode retains a pointer to the proc dentry it is shadowing, but
    whilst it does appear to retain a reference to it, it doesn't appear to
    destroy the reference if the inode goes away.

    (*) hppfs_iget() should perhaps subsume init_inode() and hppfs_read_inode().

    (*) It would appear that all hppfs inodes are the same inode because iget()
    was being called with inode number 0, which forms the lookup key.

    David
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-10-25 18:37    [W:0.027 / U:29.572 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site