Messages in this thread | | | Date | Wed, 7 Nov 2007 11:17:48 +0100 | From | Jörn Engel <> | Subject | Re: [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/
| |