Re: [patch 14/23] inodes: Support generic defragmentation

[Christoph Lameter]

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.