Message-Id: <20070515133759.9ee434a2.akpm@linux-foundation.org>
Date:	Tue, 15 May 2007 13:37:59 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Jörn Engel <joern@...ybastard.org>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-mtd@...ts.infradead.org, Albert Cahalan <acahalan@...il.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Jan Engelhardt <jengelh@...ux01.gwdg.de>,
	Evgeniy Polyakov <johnpol@....mipt.ru>,
	Pekka Enberg <penberg@...helsinki.fi>,
	Greg KH <greg@...ah.com>, Ingo Oeser <ioe-lkml@...eria.de>
Subject: Re: [PATCH] LogFS take three

On Tue, 15 May 2007 17:19:20 +0200
J__rn Engel <joern@...ybastard.org> wrote:

> Add LogFS, a scalable flash filesystem.
>
> ...
>
>  
> +config LOGFS
> +	tristate "Log Filesystem (EXPERIMENTAL)"
> +	depends on EXPERIMENTAL
> +	select ZLIB_INFLATE
> +	select ZLIB_DEFLATE
> +	help
> +	  Flash filesystem aimed to scale efficiently to large devices.
> +	  In comparison to JFFS2 it offers significantly faster mount
> +	  times and potentially less RAM usage, although the latter has
> +	  not been measured yet.
> +
> +	  In its current state it is still very experimental and should
> +	  not be used for other than testing purposes.
> +
> +	  If unsure, say N.
> +
> +config LOGFS_FSCK
> +	bool "Run LogFS fsck at mount time"
> +	depends on LOGFS
> +	help
> +	  Run a full filesystem check on every mount.  If any errors are
> +	  found, mounting the filesystem will fail.  This is a debug option
> +	  for developers.
> +
> +	  If unsure, say N.
> +

No dependency on MTD,

> @@ -0,0 +1,373 @@
> +/*
> + * fs/logfs/logfs.h
> + *
> + * As should be obvious for Linux kernel code, license is GPLv2
> + *
> + * Copyright (c) 2005-2007 Joern Engel
> + *
> + * Private header for logfs.
> + */
> +#ifndef fs_logfs_logfs_h
> +#define fs_logfs_logfs_h
> +
> +#define __CHECK_ENDIAN__
> +
> +
> +#include <linux/crc32.h>
> +#include <linux/fs.h>
> +#include <linux/kallsyms.h>
> +#include <linux/kernel.h>
> +#include <linux/logfs.h>
> +#include <linux/pagemap.h>
> +#include <linux/statfs.h>
> +#include <linux/mtd/mtd.h>

But it includes an MTD header file.

Can this code be tested by people who don't have MTD hardware?  We used to
ahve a fake-mtd-on-a-blockdev thing, whcih was in a state of some
disrepair.  Maybe it got repaired.  Or removed.  I can't immediately locate
it...

It's strange and a bit regrettable that an fs would have dependency on MTD,
really.

> +
> +/**
> + * struct logfs_area - area management information
> + *
> + * @a_sb:			the superblock this area belongs to
> + * @a_is_open:			1 if the area is currently open, else 0
> + * @a_segno:			segment number of area
> + * @a_used_objects:		number of used objects (XXX: should get removed)
> + * @a_used_bytes:		number of used bytes
> + * @a_ops:			area operations (either journal or ostore)
> + * @a_wbuf:			write buffer
> + * @a_erase_count:		erase count
> + * @a_level:			GC level
> + */

ooh, documentation.  Quick, merge it!

> +/* memtree.c */
> +void btree_init(struct btree_head *head);
> +void *btree_lookup(struct btree_head *head, long val);
> +int btree_insert(struct btree_head *head, long val, void *ptr);
> +int btree_remove(struct btree_head *head, long val);

These names are too generic.  If we later add a btree library: blam.

> +
> +/* readwrite.c */
> +int logfs_inode_read(struct inode *inode, void *buf, size_t n, loff_t _pos);
> +int logfs_inode_write(struct inode *inode, const void *buf, size_t n,
> +		loff_t pos);

It's a bit rude stealing the logfs* namespace, but I guess you got there
first ;)

> +int logfs_readpage_nolock(struct page *page);
> +int logfs_write_buf(struct inode *inode, pgoff_t index, void *buf);
> +int logfs_delete(struct inode *inode, pgoff_t index);
> +int logfs_rewrite_block(struct inode *inode, pgoff_t index, u64 ofs, int level);
> +int logfs_is_valid_block(struct super_block *sb, u64 ofs, u64 ino, u64 pos);
> +void logfs_truncate(struct inode *inode);
> +u64 logfs_seek_data(struct inode *inode, u64 pos);
> +
> +int logfs_init_rw(struct logfs_super *super);
> +void logfs_cleanup_rw(struct logfs_super *super);
> +
> +/* segment.c */
> +int logfs_erase_segment(struct super_block *sb, u32 ofs);
> +int wbuf_read(struct super_block *sb, u64 ofs, size_t len, void *buf);
> +int logfs_segment_read(struct super_block *sb, void *buf, u64 ofs);
> +s64 logfs_segment_write(struct inode *inode, void *buf, u64 pos, int level,
> +		int alloc);
> +int logfs_segment_delete(struct inode *inode, u64 ofs, u64 pos, int level);
> +void logfs_set_blocks(struct inode *inode, u64 no);
> +void __logfs_set_blocks(struct inode *inode);
> +/* area handling */
> +int logfs_init_areas(struct super_block *sb);
> +void logfs_cleanup_areas(struct logfs_super *super);
> +int logfs_open_area(struct logfs_area *area);
> +void logfs_close_area(struct logfs_area *area);
> +
> +/* super.c */
> +int mtdread(struct super_block *sb, loff_t ofs, size_t len, void *buf);
> +int mtdwrite(struct super_block *sb, loff_t ofs, size_t len, void *buf);
> +int mtderase(struct super_block *sb, loff_t ofs, size_t len);
> +void logfs_crash_dump(struct super_block *sb);
> +int all_ff(void *buf, size_t len);
> +int logfs_statfs(struct dentry *dentry, struct kstatfs *stats);

Have you checked that all of this needs global scope?

> +
> +/* progs/fsck.c */
> +#ifdef CONFIG_LOGFS_FSCK
> +int logfs_fsck(struct super_block *sb);
> +#else
> +#define logfs_fsck(sb) ({ 0; })

static inline int logfs_fsck(struct super_block *sb)
{
	return 0;
}

is better: nicer to look at, has typechecking.

> +#endif

> +
> +/* progs/mkfs.c */
> +int logfs_mkfs(struct super_block *sb, struct logfs_disk_super *ds);
> +
> +
> +#define LOGFS_BUG(sb) do {		\
> +	struct super_block *__sb = sb;	\
> +	logfs_crash_dump(__sb);		\
> +	BUG();				\
> +} while(0)
> +
> +#define LOGFS_BUG_ON(condition, sb) \
> +	do { if (unlikely((condition)!=0)) LOGFS_BUG((sb)); } while(0)
> +
> +
> +static inline struct logfs_super *LOGFS_SUPER(struct super_block *sb)
> +{
> +	return sb->s_fs_info;
> +}
> +
> +static inline struct logfs_inode *LOGFS_INODE(struct inode *inode)
> +{
> +	return container_of(inode, struct logfs_inode, vfs_inode);
> +}

Do these need to be uppercase?

> +
> +static inline __be32 logfs_crc32(void *data, size_t len, size_t skip)
> +{
> +	return cpu_to_be32(crc32(~0, data+skip, len-skip));
> +}
> +
> +
> +static inline u8 logfs_type(struct inode *inode)
> +{
> +	return (inode->i_mode >> 12) & 15;
> +}
> +
> +
> +static inline pgoff_t logfs_index(u64 pos)
> +{
> +	return pos / LOGFS_BLOCKSIZE;
> +}

If the compiler goofs up here we'll end up trying to do a 64/32 divide and
it won't link on 32-bit machines.  It would be safer to do

	return pos >> LOGFS_BLOCKSHIFT;


> --- /dev/null	2007-04-18 05:32:26.652341749 +0200
> +++ linux-2.6.21logfs/fs/logfs/compr.c	2007-05-10 19:07:24.000000000 +0200
> @@ -0,0 +1,107 @@
> +/*
> + * fs/logfs/compr.c	- compression routines
> + *
> + * As should be obvious for Linux kernel code, license is GPLv2
> + *
> + * Copyright (c) 2005-2007 Joern Engel
> + */
> +#include "logfs.h"
> +#include <linux/vmalloc.h>
> +#include <linux/zlib.h>
> +
> +#define COMPR_LEVEL 3
> +
> +static DEFINE_MUTEX(compr_mutex);
> +static struct z_stream_s stream;
> +
> +
>
> ...
>
> +
> +int logfs_uncompress(void *in, void *out, size_t inlen, size_t outlen)
> +{
> +	int err, ret;
> +
> +	ret = -EIO;
> +	mutex_lock(&compr_mutex);

A per-superblock lock and stream would be nicer.

> +	err = zlib_inflateInit(&stream);
> +	if (err != Z_OK)
> +		goto error;
> +
> +	stream.next_in = in;
> +	stream.avail_in = inlen;
> +	stream.total_in = 0;
> +	stream.next_out = out;
> +	stream.avail_out = outlen;
> +	stream.total_out = 0;
> +
> +	err = zlib_inflate(&stream, Z_FINISH);
> +	if (err != Z_STREAM_END)
> +		goto error;
> +
> +	err = zlib_inflateEnd(&stream);
> +	if (err != Z_OK)
> +		goto error;
> +
> +	ret = 0;
> +error:
> +	mutex_unlock(&compr_mutex);
> +	return ret;
> +}
>
> ...
>
> --- /dev/null	2007-04-18 05:32:26.652341749 +0200
> +++ linux-2.6.21logfs/fs/logfs/dir.c	2007-05-10 19:57:46.000000000 +0200
> @@ -0,0 +1,725 @@
> +}
> +
> +
> +static inline loff_t file_end(struct inode *inode)
> +{
> +	return (i_size_read(inode) + inode->i_sb->s_blocksize - 1)
> +		>> inode->i_sb->s_blocksize_bits;
> +}
> +static void logfs_set_name(struct logfs_disk_dentry *dd, struct qstr *name)
> +{

The code has a strange mix of two-blank-lines-between-functions and
no-blank-lines-between-functions.  One blank line is usual.


> +	BUG_ON(name->len > LOGFS_MAX_NAMELEN);
> +	dd->namelen = cpu_to_be16(name->len);
> +	memcpy(dd->name, name->name, name->len);
> +}
> +static int logfs_write_dir(struct inode *dir, struct dentry *dentry,
> +		struct inode *inode)
> +{
> +	struct logfs_disk_dentry dd;
> +	int err;
> +
> +	memset(&dd, 0, sizeof(dd));
> +	dd.ino = cpu_to_be64(inode->i_ino);
> +	dd.type = logfs_type(inode);
> +	logfs_set_name(&dd, &dentry->d_name);
> +
> +	dir->i_ctime = dir->i_mtime = CURRENT_TIME;
> +	/*
> +	 * FIXME: the file size should actually get aligned when writing,
> +	 * not when reading.
> +	 */
> +	err = write_dir(dir, &dd, file_end(dir));
> +	if (err)
> +		return err;
> +	d_instantiate(dentry, inode);
> +	return 0;
> +}
> +
> +
>
> ...
>
> +
> +	if (dest) {
> +		/* symlink */
> +		ret = logfs_inode_write(inode, dest, destlen, 0);
> +	} else {
> +		/* creat/mkdir/mknod */
> +		ret = __logfs_write_inode(inode);
> +	}
> +	super->s_victim_ino = 0;
> +	if (ret) {
> +		if (!dest)
> +			li->li_flags |= LOGFS_IF_STILLBORN;
> +		/* FIXME: truncate symlink */
> +		inode->i_nlink--;
> +		iput(inode);
> +		goto out;
> +	}
> +
> +	if (inode->i_mode & S_IFDIR)
> +		dir->i_nlink++;

You have helper functions for i_nlink++, which remember to do
mark_inode_dirty()?

> +	ret = logfs_write_dir(dir, dentry, inode);
> +
> +	if (ret) {
> +		if (inode->i_mode & S_IFDIR)
> +			dir->i_nlink--;
> +		logfs_remove_inode(inode);
> +		iput(inode);
> +	}
> +out:
> +	mutex_unlock(&super->s_victim_mutex);
> +	return ret;
> +}
> +
>
> ...
>
> +
> +static struct inode_operations logfs_symlink_iops = {
> +	.readlink	= generic_readlink,
> +	.follow_link	= page_follow_link_light,
> +};

Should be const.

> +static int logfs_permission(struct inode *inode, int mask, struct nameidata *nd)
> +{
> +	return generic_permission(inode, mask, NULL);
> +}

Does this need to exist?

> +
> +struct inode_operations logfs_dir_iops = {
> +	.create		= logfs_create,
> +	.link		= logfs_link,
> +	.lookup		= logfs_lookup,
> +	.mkdir		= logfs_mkdir,
> +	.mknod		= logfs_mknod,
> +	.rename		= logfs_rename,
> +	.rmdir		= logfs_rmdir,
> +	.permission	= logfs_permission,
> +	.symlink	= logfs_symlink,
> +	.unlink		= logfs_unlink,
> +};

const

> +struct file_operations logfs_dir_fops = {
> +	.readdir	= logfs_readdir,
> +	.read		= generic_read_dir,
> +};

const

> --- /dev/null	2007-04-18 05:32:26.652341749 +0200
> +++ linux-2.6.21logfs/fs/logfs/file.c	2007-05-10 19:46:21.000000000 +0200
> @@ -0,0 +1,81 @@
> +/*
> + * fs/logfs/file.c	- prepare_write, commit_write and friends
> + *
> + * As should be obvious for Linux kernel code, license is GPLv2
> + *
> + * Copyright (c) 2005-2007 Joern Engel
> + */
> +#include "logfs.h"
> +
> +
> +static int logfs_prepare_write(struct file *file, struct page *page,
> +		unsigned start, unsigned end)
> +{
> +	if (PageUptodate(page))
> +		return 0;
> +
> +	if ((start == 0) && (end == PAGE_CACHE_SIZE))
> +		return 0;
> +
> +	return logfs_readpage_nolock(page);
> +}
> +
> +
> +static int logfs_commit_write(struct file *file, struct page *page,
> +		unsigned start, unsigned end)
> +{
> +	struct inode *inode = page->mapping->host;
> +	pgoff_t index = page->index;
> +	void *buf;
> +	int ret;
> +
> +	BUG_ON(PAGE_CACHE_SIZE != inode->i_sb->s_blocksize);

This check can be done once, at mount time.

> +	BUG_ON(page->index > I3_BLOCKS);
> +
> +	if (start == end)
> +		return 0; /* FIXME: do we need to update inode? */
> +
> +	if (i_size_read(inode) < (index << PAGE_CACHE_SHIFT) + end) {
> +		i_size_write(inode, (index << PAGE_CACHE_SHIFT) + end);
> +		mark_inode_dirty(inode);
> +	}
> +
> +	buf = kmap(page);
> +	ret = logfs_write_buf(inode, index, buf);
> +	kunmap(page);

kmap() is lame.  The preferred approach would be to pass the page* down to
the lower layers and to use kmap_atomic() at the lowest possible point.

> +	return ret;
> +}
> +
> +
> +static int logfs_readpage(struct file *file, struct page *page)
> +{
> +	int ret;
> +
> +	ret = logfs_readpage_nolock(page);
> +	unlock_page(page);
> +	return ret;
> +}
> +
> +
> +struct inode_operations logfs_reg_iops = {
> +	.truncate	= logfs_truncate,
> +};

const

> +
> +struct file_operations logfs_reg_fops = {
> +	.aio_read	= generic_file_aio_read,
> +	.aio_write	= generic_file_aio_write,
> +	.llseek		= generic_file_llseek,
> +	.mmap		= generic_file_readonly_mmap,
> +	.open		= generic_file_open,
> +	.read		= do_sync_read,
> +	.write		= do_sync_write,
> +};

const

> +
> +struct address_space_operations logfs_reg_aops = {
> +	.commit_write	= logfs_commit_write,
> +	.prepare_write	= logfs_prepare_write,
> +	.readpage	= logfs_readpage,
> +	.set_page_dirty	= __set_page_dirty_nobuffers,
> +};

const

> +/*
> + * cookie is set to 1 if we hand out a cached inode, 0 otherwise.
> + * this allows logfs_iput to do the right thing later
> + */
> +struct inode *logfs_iget(struct super_block *sb, ino_t ino, int *cookie)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	struct logfs_inode *li;
> +
> +	if (ino == LOGFS_INO_MASTER)
> +		return super->s_master_inode;
> +
> +	spin_lock(&inode_lock);
> +	list_for_each_entry(li, &super->s_freeing_list, li_freeing_list)
> +		if (li->vfs_inode.i_ino == ino) {
> +			spin_unlock(&inode_lock);
> +			*cookie = 1;
> +			return &li->vfs_inode;
> +		}
> +	spin_unlock(&inode_lock);
> +
> +	*cookie = 0;
> +	return __logfs_iget(sb, ino);
> +}

A filesystem playing with inode_lock: not good.  What's going on here?

As a minimum, the reasons for this should be clearly spelled out in code
comments, because this sticks out like a sore thumb.

> +
> +	li = kmem_cache_alloc(logfs_inode_cache, GFP_KERNEL);
> +	if (!li)
> +		return NULL;
> +	logfs_init_inode(&li->vfs_inode);
> +	return &li->vfs_inode;
> +}
> +
> +
> +struct inode *logfs_new_meta_inode(struct super_block *sb, u64 ino)
> +{
> +	struct inode *inode;
> +
> +	inode = logfs_alloc_inode(sb);
> +	if (!inode)
> +		return ERR_PTR(-ENOMEM);
> +
> +	logfs_init_inode(inode);
> +	inode->i_mode = 0;
> +	inode->i_ino = ino;
> +	inode->i_sb = sb;
> +
> +	/* This is a blatant copy of alloc_inode code.  We'd need alloc_inode
> +	 * to be nonstatic, alas. */
> +	{
> +		static const struct address_space_operations empty_aops;
> +		struct address_space * const mapping = &inode->i_data;
> +
> +		mapping->a_ops = &empty_aops;
> +		mapping->host = inode;
> +		mapping->flags = 0;
> +		mapping_set_gfp_mask(mapping, GFP_HIGHUSER);
> +		mapping->assoc_mapping = NULL;
> +		mapping->backing_dev_info = &default_backing_dev_info;
> +		inode->i_mapping = mapping;
> +	}
> +
> +	return inode;
> +}

This function would benefit from some comments.  What's it doing, and why
is it special?  I mean, new_inode() calls alloc_inode() anyway, so you're
unable to use new_inode().  The reader wonders why.


> +
> +/*
> + * We depend on the kernel to hand us proper time here.  If it has more
> + * nanoseconds than fit in a second, something's fishy.  Either the currently
> + * running kernel is confused or we read a wrong value.  The latter could be
> + * because whoever wrote the value messed up or we have undetected data
> + * corruption.
> + * Whatever the case, give a warning.
> + */
> +static struct timespec be64_to_timespec(__be64 betime)
> +{
> +	u64 time = be64_to_cpu(betime);
> +	struct timespec tsp;
> +
> +	tsp.tv_sec = time >> 32;
> +	tsp.tv_nsec = time & 0xffffffff;
> +	WARN_ON(tsp.tv_nsec > 999999999);
> +	return tsp;
> +}

Could use ns_to_timespec(be64_to_cpu(betime)) here.

Should use >= NSEC_PER_SEC here.

> +
> +static __be64 timespec_to_be64(struct timespec tsp)
> +{
> +	u64 time = ((u64)tsp.tv_sec << 32) + (tsp.tv_nsec & 0xffffffff);
> +
> +	WARN_ON(tsp.tv_nsec > 999999999);
> +	return cpu_to_be64(time);
> +}

Dittos.

> +/* called with inode_lock held */
> +static void logfs_drop_inode(struct inode *inode)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(inode->i_sb);
> +	struct logfs_inode *li = LOGFS_INODE(inode);
> +
> +	list_move(&li->li_freeing_list, &super->s_freeing_list);
> +	generic_drop_inode(inode);
> +}
> +
> +
> +static u64 logfs_get_ino(struct super_block *sb)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	u64 ino;
> +
> +	/*
> +	 * FIXME: ino allocation should work in two modes:
> +	 * o nonsparse - ifile is mostly occupied, just append
> +	 * o sparse - ifile has lots of holes, fill them up
> +	 *
> +	 * SEEK_HOLE would obviously help a lot here.
> +	 */
> +	spin_lock(&super->s_ino_lock);
> +	ino = super->s_last_ino;
> +	super->s_last_ino++;
> +	spin_unlock(&super->s_ino_lock);
> +	return ino;
> +}

Could use atomic64_add_return() here.

> +
> +struct inode *logfs_new_inode(struct inode *dir, int mode)
> +{
> +	struct super_block *sb = dir->i_sb;
> +	struct inode *inode;
> +
> +	inode = new_inode(sb);
> +	if (!inode)
> +		return ERR_PTR(-ENOMEM);
> +
> +	logfs_init_inode(inode);
> +
> +	inode->i_mode = mode;
> +	inode->i_ino = logfs_get_ino(sb);
> +
> +	insert_inode_hash(inode);
> +
> +	return inode;
> +}
> +
> +
> +static void logfs_init_once(void *_li, struct kmem_cache *cachep,
> +		unsigned long flags)
> +{
> +	struct logfs_inode *li = _li;
> +	int i;
> +
> +	if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
> +			SLAB_CTOR_CONSTRUCTOR) {

This won't compile in mainline (SLAB_CTOR_VERIFY has gone)

And it won't compile in -mm (SLAB_CTOR_CONSTRUCTOR has gone).

Just remove the test altogether.

> +		li->li_flags = 0;
> +		li->li_used_bytes = 0;
> +		for (i=0; i<LOGFS_EMBEDDED_FIELDS; i++)
> +			li->li_data[i] = 0;
> +		inode_init_once(&li->vfs_inode);
> +	}
> +
> +}
> +
> +
> +struct super_operations logfs_super_operations = {
> +	.alloc_inode	= logfs_alloc_inode,
> +	.delete_inode	= logfs_delete_inode,
> +	.destroy_inode	= logfs_destroy_inode,
> +	.drop_inode	= logfs_drop_inode,
> +	.read_inode	= logfs_read_inode,
> +	.write_inode	= logfs_write_inode,
> +	.statfs		= logfs_statfs,
> +};

const

> +
> +int logfs_init_inode_cache(void)
> +{
> +	logfs_inode_cache = kmem_cache_create("logfs_inode_cache",
> +			sizeof(struct logfs_inode), 0, SLAB_RECLAIM_ACCOUNT,
> +			logfs_init_once, NULL);

Use KMEM_CACHE() helper

> +	if (!logfs_inode_cache)
> +		return -ENOMEM;
> +	return 0;
> +}
> +
> +
> +void logfs_destroy_inode_cache(void)
> +{
> +	kmem_cache_destroy(logfs_inode_cache);
> +}

<attention span ran out, sorry>
-
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/

Powered by blists - more mailing lists