On Wed, 7 Nov 2007, Jörn Engel wrote:
> 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?
The pointer is for communication between the get and kick methods. get()
can modify kick() behavior by returning a pointer to a data structure or
using the pointer to set a flag. F.e. get() may discover that there is an
unreclaimable object and set a flag that causes kick to simply undo the
refcount increment. get() may build a map for the objects and indicate in
the map special treatment.
> > +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.
As far as I can remember: The core code always passes pointers to struct
inode to the filesystems. The filesystems will then recalculate the
pointers to point to the fs ide of an inode.
> > +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.
The get() method may have set a pointer to NULL. The fs_get_inodes() is
run at a time when all pointers are valid.
> > + }
> > +
> > + /* 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.
Yeah. I'd be glad if someone could take this piece off my hands.
