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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20071107101748.GC7374@lazybastard.org>
Date:	Wed, 7 Nov 2007 11:17:48 +0100
From:	Jörn Engel <joern@...fs.org>
To:	Christoph Lameter <clameter@....com>
Cc:	akpm@...ux-foundatin.org, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org, Mel Gorman <mel@...net.ie>
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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/