lkml.org 
[lkml]   [2007]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch 14/23] inodes: Support generic defragmentation
    On Tue, 6 November 2007 17:11:44 -0800, Christoph Lameter wrote:
    >
    > +void *get_inodes(struct kmem_cache *s, int nr, void **v)
    > +{
    > + int i;
    > +
    > + spin_lock(&inode_lock);
    > + for (i = 0; i < nr; i++) {
    > + struct inode *inode = v[i];
    > +
    > + if (inode->i_state & (I_FREEING|I_CLEAR|I_WILL_FREE))
    > + v[i] = NULL;
    > + else
    > + __iget(inode);
    > + }
    > + spin_unlock(&inode_lock);
    > + return NULL;
    > +}
    > +EXPORT_SYMBOL(get_inodes);

    What purpose does the return type have?

    > +/*
    > + * Function for filesystems that embedd struct inode into their own
    > + * structures. The offset is the offset of the struct inode in the fs inode.
    > + */
    > +void *fs_get_inodes(struct kmem_cache *s, int nr, void **v,
    > + unsigned long offset)
    > +{
    > + int i;
    > +
    > + for (i = 0; i < nr; i++)
    > + v[i] += offset;
    > +
    > + return get_inodes(s, nr, v);
    > +}
    > +EXPORT_SYMBOL(fs_get_inodes);

    The fact that all pointers get changed makes me a bit uneasy:
    struct foo_inode v[20];
    ...
    fs_get_inodes(..., v, ...);
    ...
    v[0].foo_field = bar;

    No warning, but spectacular fireworks.

    > +void kick_inodes(struct kmem_cache *s, int nr, void **v, void *private)
    > +{
    > + struct inode *inode;
    > + int i;
    > + int abort = 0;
    > + LIST_HEAD(freeable);
    > + struct super_block *sb;
    > +
    > + for (i = 0; i < nr; i++) {
    > + inode = v[i];
    > + if (!inode)
    > + continue;

    NULL is legal here? Then fs_get_inodes should check for NULL as well
    and not add the offset to NULL pointers, I guess.

    > + if (inode_has_buffers(inode) || inode->i_data.nrpages) {
    > + if (remove_inode_buffers(inode))
    > + invalidate_mapping_pages(&inode->i_data,
    > + 0, -1);

    This linebreak can be removed.

    > + }
    > +
    > + /* Invalidate children and dentry */
    > + if (S_ISDIR(inode->i_mode)) {
    > + struct dentry *d = d_find_alias(inode);
    > +
    > + if (d) {
    > + d_invalidate(d);
    > + dput(d);
    > + }
    > + }
    > +
    > + if (inode->i_state & I_DIRTY)
    > + write_inode_now(inode, 1);

    Once more the three-bit I_DIRTY is used like a boolean value. I don't
    hold it against you, specifically. A general review/cleanup is
    necessary for that.

    Jörn

    --
    "[One] doesn't need to know [...] how to cause a headache in order
    to take an aspirin."
    -- Scott Culp, Manager of the Microsoft Security Response Center, 2001
    -
    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-11-07 11:25    [W:0.029 / U:30.096 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site