Re: [PATCH 1/2] LogFS proper

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 2007-05-08 at 00:00 +0200, Jörn Engel wrote:
> The filesystem itself.

Very descriptive log entry.

> +config LOGFS
> +	tristate "Log Filesystem (EXPERIMENTAL)"
> +	depends on EXPERIMENTAL
> +	select ZLIB_INFLATE
> +	select ZLIB_DEFLATE
> +	help
> +	  Successor of JFFS2, using explicit filesystem hierarchy.

Why is it a successor ? Does it build upon JFFS2 ?

> +	  Continuing with the long tradition of calling the filesystem
> +	  exactly what it is not, LogFS is a journaled filesystem,
> +	  while JFFS and JFFS2 were true log-structured filesystems.
> +	  The hybrid structure of journaled filesystems promise to
> +	  scale better to larger sized.
> +
> +	  If unsure, say N.

...

> @@ -0,0 +1,14 @@
> +obj-$(CONFIG_LOGFS)	+= logfs.o
> +
> +logfs-y += compr.o
> +logfs-y	+= dir.o
> +logfs-y	+= file.o
> +logfs-y	+= gc.o
> +logfs-y	+= inode.o
> +logfs-y	+= journal.o
> +logfs-y += memtree.o
> +logfs-y	+= readwrite.o
> +logfs-y	+= segment.o
> +logfs-y	+= super.o
> +logfs-y += progs/fsck.o
> +logfs-y += progs/mkfs.o

Please use either tabs or spaces. Preferrably tabs

> --- /dev/null	2007-04-18 05:32:26.652341749 +0200
> +++ linux-2.6.21logfs/fs/logfs/logfs.h	2007-05-07 13:32:12.000000000 +0200
> @@ -0,0 +1,626 @@
> +#ifndef logfs_h
> +#define logfs_h
> +
> +#define __CHECK_ENDIAN__
> +
> +
> +#include <linux/crc32.h>
> +#include <linux/fs.h>
> +#include <linux/kallsyms.h>
> +#include <linux/kernel.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/pagemap.h>
> +#include <linux/statfs.h>

Please sort includes alphabetically and seperate the 
#include <linux/mtd/mtd.h> from the #include <linux/...> ones

> +typedef __be16 be16;
> +typedef __be32 be32;
> +typedef __be64 be64;

Why are those typedefs necessary ?

> +struct btree_head {
> +	struct btree_node *node;
> +	int height;
> +	void *null_ptr;
> +};

Please document structures

> +#define packed __attribute__((__packed__))

Please use the __attribute__((__packed__)) on your structs instead of
creating some extra "needs lookup" magic.

> +
> +#define TRACE() do {						\
> +	printk("trace: %s:%d: ", __FILE__, __LINE__);		\
> +	printk("->%s\n", __func__);				\
> +} while(0)

Oh no. Not again another "I'm in function X tracer". 

> +
> +#define LOGFS_MAGIC 0xb21f205ac97e8168ull
> +#define LOGFS_MAGIC_U32 0xc97e8168ull

why is an U32 constant ull ?

> +#define LOGFS_BLOCK_SECTORS	(8)
> +#define LOGFS_BLOCK_BITS	(9)	/* 512 pointers, used for shifts */
> +#define LOGFS_BLOCKSIZE		(4096ull)
> +#define LOGFS_BLOCK_FACTOR (LOGFS_BLOCKSIZE / sizeof(u64))
> +#define LOGFS_BLOCK_MASK (LOGFS_BLOCK_FACTOR-1)

for the whole defines:

Please align them so it does not look like a jigsaw puzzle.

Please avoid tail comments as it makes it harder to parse

> +#define I0_BLOCKS	(4+16)
> +#define I1_BLOCKS LOGFS_BLOCK_FACTOR
> +#define I2_BLOCKS (LOGFS_BLOCK_FACTOR * I1_BLOCKS)
> +#define I3_BLOCKS (LOGFS_BLOCK_FACTOR * I2_BLOCKS)
> +#define I4_BLOCKS (LOGFS_BLOCK_FACTOR * I3_BLOCKS)
> +#define I5_BLOCKS (LOGFS_BLOCK_FACTOR * I4_BLOCKS)

Some explanation for that magic math might be helpful

> +#define I1_INDEX	(4+16)

same constant as IO_BLOCKS. coincidence ?

> +#define I2_INDEX	(5+16)
> +#define I3_INDEX	(6+16)
> +#define I4_INDEX	(7+16)
> +#define I5_INDEX	(8+16)

#define I2_INDEX	(I1_INDEX + 1)
....

> +struct logfs_disk_super {
> +	be64	ds_magic;
> +	be32	ds_crc;			/* crc32 of everything below */
> +	u8	ds_ifile_levels;	/* max level of ifile */
> +	u8	ds_iblock_levels;	/* max level of regular files */
> +	u8	ds_data_levels;		/* number of segments to leaf blocks */
> +	u8	pad0;
> +
> +	be64	ds_feature_incompat;
> +	be64	ds_feature_ro_compat;
> +
> +	be64	ds_feature_compat;
> +	be64	ds_flags;
> +
> +	be64	ds_filesystem_size;	/* filesystem size in bytes */
> +	u8	ds_segment_shift;	/* log2 of segment size */
> +	u8	ds_block_shift;		/* log2 if block size */
> +	u8	ds_write_shift;		/* log2 of write size */
> +	u8	pad1[5];
> +
> +	/* the segments of the primary journal.  if fewer than 4 segments are
> +	 * used, some fields are set to 0 */
> +#define LOGFS_JOURNAL_SEGS 4

Please avoid defines inside of structures

> +	be64	ds_journal_seg[LOGFS_JOURNAL_SEGS];
> +
> +	be64	ds_root_reserve;	/* bytes reserved for root */
> +
> +	be64	pad2[19];		/* align to 256 bytes */
> +}packed;

Please comment the structure with kernel doc comments and avoid the tail
comments.

> +
> +#define LOGFS_IF_VALID		0x00000001 /* inode exists */
> +#define LOGFS_IF_EMBEDDED	0x00000002 /* data embedded in block pointers */
> +#define LOGFS_IF_ZOMBIE		0x00000004 /* inode was already deleted */
> +#define LOGFS_IF_STILLBORN	0x40000000 /* couldn't write inode in creat() */
> +#define LOGFS_IF_INVALID	0x80000000 /* inode does not exist */

Are these bit values or enum type ?

> +struct logfs_disk_inode {
> +	be16	di_mode;
> +	be16	di_pad;
> +	be32	di_flags;
> +	be32	di_uid;
> +	be32	di_gid;
> +
> +	be64	di_ctime;
> +	be64	di_mtime;
> +
> +	be32	di_refcount;
> +	be32	di_generation;
> +	be64	di_used_bytes;
> +
> +	be64	di_size;
> +	be64	di_data[LOGFS_EMBEDDED_FIELDS];
> +}packed;
> +
> +
> +#define LOGFS_MAX_NAMELEN 255

Please put define on top

> +struct logfs_disk_dentry {
> +	be64	ino;		/* inode pointer */
> +	be16	namelen;
> +	u8	type;
> +	u8	name[LOGFS_MAX_NAMELEN];
> +}packed;
> +
> +
> +#define OBJ_TOP_JOURNAL	1	/* segment header for master journal */
> +#define OBJ_JOURNAL	2	/* segment header for journal */
> +#define OBJ_OSTORE	3	/* segment header for ostore */
> +#define OBJ_BLOCK	4	/* data block */
> +#define OBJ_INODE	5	/* inode */
> +#define OBJ_DENTRY	6	/* dentry */

enum please

> +struct logfs_object_header {
> +	be32	crc;		/* checksum */
> +	be16	len;		/* length of object, header not included */
> +	u8	type;		/* node type */
> +	u8	compr;		/* compression type */
> +	be64	ino;		/* inode number */
> +	be64	pos;		/* file position */
> +}packed;

For all structs:

Please use kernel doc struct comments.

> +
> +struct logfs_segment_header {
> +	be32	crc;		/* checksum */
> +	be16	len;		/* length of object, header not included */
> +	u8	type;		/* node type */
> +	u8	level;		/* GC level */
> +	be32	segno;		/* segment number */
> +	be32	ec;		/* erase count */
> +	be64	gec;		/* global erase count (write time) */
> +}packed;
> +
> +enum {
> +	COMPR_NONE	= 0,
> +	COMPR_ZLIB	= 1,
> +};

Please name the enums and use the same enum for the according fields and
the function arguments.

> +
> +/* Journal entries come in groups of 16.  First group contains individual
> + * entries, next groups contain one entry per level */
> +enum {
> +	JEG_BASE	= 0,
> +	JE_FIRST	= 1,
> +
> +	JE_COMMIT	= 1,	/* commits all previous entries */
> +	JE_ABORT	= 2,	/* aborts all previous entries */
> +	JE_DYNSB	= 3,
> +	JE_ANCHOR	= 4,
> +	JE_ERASECOUNT	= 5,
> +	JE_SPILLOUT	= 6,
> +	JE_DELTA	= 7,
> +	JE_BADSEGMENTS	= 8,
> +	JE_AREAS	= 9,	/* area description sans wbuf */
> +	JEG_WBUF	= 0x10,	/* write buffer for segments */
> +
> +	JE_LAST		= 0x1f,
> +};

same here

> +
> +////////////////////////////////////////////////////////////////////////////////
> +////////////////////////////////////////////////////////////////////////////////

Eew.

> +
> +#define LOGFS_SUPER(sb) ((struct logfs_super*)(sb->s_fs_info))
> +#define LOGFS_INODE(inode) container_of(inode, struct logfs_inode, vfs_inode)

lowercase inlines please

> +
> +			/*	0	reserved for gc markers */
> +#define LOGFS_INO_MASTER	1	/* inode file */
> +#define LOGFS_INO_ROOT		2	/* root directory */
> +#define LOGFS_INO_ATIME		4	/* atime for all inodes */
> +#define LOGFS_INO_BAD_BLOCKS	5	/* bad blocks */
> +#define LOGFS_INO_OBSOLETE	6	/* obsolete block count */
> +#define LOGFS_INO_ERASE_COUNT	7	/* erase count */
> +#define LOGFS_RESERVED_INOS	16

enum ?

> +struct logfs_super {
> +	//struct super_block *s_sb;		/* should get removed... */

Please do so

> +	be64	*s_rblock;
> +	be64	*s_wblock[LOGFS_MAX_LEVELS];

Please comment the non obvious ones instead of the self explaining

> +	u64	 s_free_bytes;			/* number of free bytes */


> +#define journal_for_each(__i) for (__i=0; __i<LOGFS_JOURNAL_SEGS; __i++)

	__i = 0; __i < LOGFS_JOURNAL_SEGS;

> +void logfs_crash_dump(struct super_block *sb);
> +#define LOGFS_BUG(sb) do {		\
> +	struct super_block *__sb = sb;	\

Why do we need a local variable here ?

> +	logfs_crash_dump(__sb);		\
> +	BUG();				\
> +} while(0)

> +static inline u8 logfs_type(struct inode *inode)
> +{
> +	return (inode->i_mode >> 12) & 15;

What's 12 and 15 ? Constants perhaps ?

> +}
> +static inline struct logfs_disk_sum *alloc_disk_sum(struct super_block *sb)
> +{
> +	return kzalloc(sb->s_blocksize, GFP_ATOMIC);
> +}

No, please do not add another alias for kzalloc

> +static inline void free_disk_sum(struct logfs_disk_sum *sum)
> +{
> +	kfree(sum);
> +}

same here

> +
> +/* compr.c */
> +#define logfs_compress_none logfs_memcpy
> +#define logfs_uncompress_none logfs_memcpy

can you please use logfs_memcpy instead ?

> +int logfs_memcpy(void *in, void *out, size_t inlen, size_t outlen);
> +int logfs_compress(void *in, void *out, size_t inlen, size_t outlen);
> +int logfs_compress_vec(struct kvec *vec, int count, void *out, size_t outlen);
> +int logfs_uncompress(void *in, void *out, size_t inlen, size_t outlen);
> +int logfs_uncompress_vec(void *in, size_t inlen, struct kvec *vec, int count);

are those global ? If yes, please add extern, else remove

> +int __init logfs_compr_init(void);
> +void __exit logfs_compr_exit(void);

dito

> +
> +/* dir.c */
> +extern struct inode_operations logfs_dir_iops;
> +extern struct file_operations logfs_dir_fops;
> +int logfs_replay_journal(struct super_block *sb);

dito

> +
> +/* file.c */
> +extern struct inode_operations logfs_reg_iops;
> +extern struct file_operations logfs_reg_fops;
> +extern struct address_space_operations logfs_reg_aops;
> +
> +int logfs_setattr(struct dentry *dentry, struct iattr *iattr);

dito

> +
> +/* gc.c */
> +void logfs_gc_pass(struct super_block *sb);
> +int logfs_init_gc(struct logfs_super *super);
> +void logfs_cleanup_gc(struct logfs_super *super);

same here ......................

> +
> +/* inode.c */
> +/* progs/mkfs.c */
> +int logfs_fsck(struct super_block *sb);

down to this place

> +
> +static inline u64 dev_ofs(struct super_block *sb, u32 segno, u32 ofs)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);

Seperate variables and code by an empty line please

> +	return ((u64)segno << super->s_segshift) + ofs;
> +}
> +
> +
> +static inline void device_read(struct super_block *sb, u32 segno, u32 ofs,
> +		size_t len, void *buf)
> +{
> +	int err = mtdread(sb, dev_ofs(sb, segno, ofs), len, buf);

Same here.

> +	LOGFS_BUG_ON(err, sb);

Please open code this instead of nesting mtdread into device_read and
therefor avoid the error handling pathes in those places where
device_read is used.

> +}
> +
> +
> +#define EOF	256

1. very intuitive name
2. why is this constant not at the top, where the other constants are
3. why 256


> +
> +typedef int (*dir_callback)(struct inode *dir, struct dentry *dentry,
> +		struct logfs_disk_dentry *dd, loff_t pos);

Why is this in the middle of something else ?

> +
> +static s64 dir_seek_data(struct inode *inode, s64 pos)
> +{
> +	s64 new_pos = logfs_seek_data(inode, pos);

new line please

> +	return max((s64)pos, new_pos - 1);

	max_t please 

> +}
> +
> +
> +static int __logfs_dir_walk(struct inode *dir, struct dentry *dentry,
> +		dir_callback handler, struct logfs_disk_dentry *dd, loff_t *pos)
> +{
> +	struct qstr *name = dentry ? &dentry->d_name : NULL;
> +	int ret;
> +
> +	for (; ; (*pos)++) {
> +		ret = read_dir(dir, dd, *pos);
> +		if (ret == -EOF)
> +			return 0;
> +		if (ret == -ENODATA) {/* deleted dentry */

Please move the comment away. It makes parsing hard

> +			*pos = dir_seek_data(dir, *pos);
> +			continue;
> +		}
> +		if (ret)
> +			return ret;
> +		BUG_ON(dd->namelen == 0);
> +
> +		if (name) {
> +			if (name->len != be16_to_cpu(dd->namelen))
> +				continue;
> +			if (memcmp(name->name, dd->name, name->len))
> +				continue;
> +		}
> +
> +		return handler(dir, dentry, dd, *pos);
> +	}
> +	return ret;

	Where do you break out of the loop ?

> +}
> +
> +
> +static int logfs_dir_walk(struct inode *dir, struct dentry *dentry,
> +		dir_callback handler)
> +{
> +	struct logfs_disk_dentry dd;
> +	loff_t pos = 0;

New line please

> +	return __logfs_dir_walk(dir, dentry, handler, &dd, &pos);
> +}
> +
> +
> +static struct dentry *logfs_lookup(struct inode *dir, struct dentry *dentry,
> +		struct nameidata *nd)
> +{
> +	struct dentry *ret;
> +
> +	ret = ERR_PTR(logfs_dir_walk(dir, dentry, logfs_lookup_handler));
> +	return ret;

	return ERR_PTR(.....);

> +}
> +
> +static int logfs_unlink(struct inode *dir, struct dentry *dentry)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(dir->i_sb);
> +	struct inode *inode = dentry->d_inode;
> +	int ret;
> +
> +	mutex_lock(&super->s_victim_mutex);
> +	super->s_victim_ino = inode->i_ino;
> +
> +	/* remove dentry */
> +	if (inode->i_mode & S_IFDIR)
> +		dir->i_nlink--;
> +	inode->i_ctime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
> +	ret = logfs_dir_walk(dir, dentry, logfs_unlink_handler);
> +	super->s_victim_ino = 0;
> +	if (ret)
> +		goto out;
> +
> +	/* remove inode */
> +	ret = logfs_remove_inode(inode);

Please remove this goto / label construct and  do

	if (likely(!ret))
		ret = logfs_remove_inode(inode);

instead

> +out:
> +	mutex_unlock(&super->s_victim_mutex);
> +	return ret;
> +}
> +
> +
> +/* FIXME: readdir currently has it's own dir_walk code.  I don't see a good
> + * way to combine the two copies */
> +#define IMPLICIT_NODES 2
> +static int __logfs_readdir(struct file *file, void *buf, filldir_t filldir)
> +{
> +	struct logfs_disk_dentry dd;
> +	loff_t pos = file->f_pos - IMPLICIT_NODES;
> +	int err;
> +
> +	BUG_ON(pos<0);
> +	for (;; pos++) {
> +		struct inode *dir = file->f_dentry->d_inode;

new line please

> +		err = read_dir(dir, &dd, pos);
> +		if (err == -EOF)
> +			break;

	-EOF results in a return code 0 ?

> +		if (err == -ENODATA) {/* deleted dentry */
> +			pos = dir_seek_data(dir, pos);
> +			continue;
> +		}
> +		if (err)
> +			return err;
> +		BUG_ON(dd.namelen == 0);
> +
> +		if (filldir(buf, dd.name, be16_to_cpu(dd.namelen), pos,
> +					be64_to_cpu(dd.ino), dd.type))
> +			break;
> +	}
> +
> +	file->f_pos = pos + IMPLICIT_NODES;
> +	return 0;
> +}
> +
> +
> +static int logfs_readdir(struct file *file, void *buf, filldir_t filldir)
> +{
> +	struct inode *inode = file->f_dentry->d_inode;
> +	int err;
> +
> +	if (file->f_pos < 0)
> +		return -EINVAL;
> +
> +	if (file->f_pos == 0) {
> +		if (filldir(buf, ".", 1, 1, inode->i_ino, DT_DIR) < 0)
> +			return 0;
> +		file->f_pos++;
> +	}
> +	if (file->f_pos == 1) {
> +		ino_t pino = parent_ino(file->f_dentry);

empty line

> +		if (filldir(buf, "..", 2, 2, pino, DT_DIR) < 0)
> +			return 0;
> +		file->f_pos++;
> +	}
> +
> +	err = __logfs_readdir(file, buf, filldir);
> +	if (err)
> +		printk("LOGFS readdir error=%x, pos=%llx\n", err, file->f_pos);
> +	return err;
> +}

> +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. */

Please use 

	/*
	 * kernel style 
	 * multi line comments
	 */

> +	err = write_dir(dir, &dd, file_end(dir));
> +	if (err)
> +		return err;
> +	d_instantiate(dentry, inode);
> +	return 0;
> +}
> +
> +
> +static int __logfs_create(struct inode *dir, struct dentry *dentry,
> +		struct inode *inode, const char *dest, long destlen)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(dir->i_sb);
> +	struct logfs_inode *li = LOGFS_INODE(inode);
> +	int ret;
> +
> +	mutex_lock(&super->s_victim_mutex);
> +	super->s_victim_ino = inode->i_ino;
> +	if (inode->i_mode & S_IFDIR)
> +		inode->i_nlink++;
> +
> +	if (dest) /* symlink */
> +		ret = logfs_inode_write(inode, dest, destlen, 0);
> +	else /* creat/mkdir/mknod */
> +		ret = __logfs_write_inode(inode);


Please remove this confusing tail comments

> +	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++;
> +	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;
> +}
> +
> +
> +/* FIXME: This should really be somewhere in the 64bit area. */
> +#define LOGFS_LINK_MAX (1<<30)

Please move the define to the header file or some other useful place

> +static int logfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
> +{
> +	struct inode *inode;
> +
> +	if (dir->i_nlink >= LOGFS_LINK_MAX)
> +		return -EMLINK;
> +
> +	/* FIXME: why do we have to fill in S_IFDIR, while the mode is
> +	 * correct for mknod, creat, etc.?  Smells like the vfs *should*
> +	 * do it for us but for some reason fails to do so.
> +	 */

Comment style

> +
> +static struct inode_operations ext2_symlink_iops = {
> +	.readlink	= generic_readlink,
> +	.follow_link	= page_follow_link_light,
> +};

s/ext2/logfs/ maybe ?

> +static int logfs_nop_handler(struct inode *dir, struct dentry *dentry,
> +		struct logfs_disk_dentry *dd, loff_t pos)
> +{
> +	return 0;
> +}

New line

> +static inline int logfs_get_dd(struct inode *dir, struct dentry *dentry,
> +		struct logfs_disk_dentry *dd, loff_t *pos)
> +{
> +	*pos = 0;
> +	return __logfs_dir_walk(dir, dentry, logfs_nop_handler, dd, pos);
> +}
> +

> +static int logfs_delete_dd(struct inode *dir, struct logfs_disk_dentry *dd,
> +		loff_t pos)
> +{
> +	int err;
> +
> +	err = read_dir(dir, dd, pos);
> +	if (err == -EOF) /* don't expose internal errnos */
> +		err = -EIO;

Interesting. Why is EOF morphed to EIO ?

> +	if (err)
> +		return err;
> +
> +	dir->i_ctime = dir->i_mtime = CURRENT_TIME;
> +	if (dd->type == DT_DIR)
> +		dir->i_nlink--;
> +	return logfs_delete(dir, pos);
> +}

> +
> +static int logfs_rename(struct inode *old_dir, struct dentry *old_dentry,
> +		struct inode *new_dir, struct dentry *new_dentry)
> +{
> +	if (new_dentry->d_inode) /* target exists */
> +		return logfs_rename_target(old_dir, old_dentry, new_dir, new_dentry);
> +	else if (old_dir == new_dir) /* local rename */
> +		return logfs_rename_local(old_dir, old_dentry, new_dentry);

Comment style

> +	return logfs_rename_cross(old_dir, old_dentry, new_dir, new_dentry);
> +}
> +
> --- /dev/null	2007-04-18 05:32:26.652341749 +0200
> +++ linux-2.6.21logfs/fs/logfs/file.c	2007-05-07 13:32:12.000000000 +0200
> @@ -0,0 +1,82 @@

Comment missing. License missing.

> +#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;

	Self explaining logic ?

> +	return logfs_readpage_nolock(page);
> +}
> +
> +
> +static int logfs_readpage(struct file *file, struct page *page)
> +{
> +	int ret = logfs_readpage_nolock(page);

empty line

> +	unlock_page(page);
> +	return ret;
> +}
> +
> +
> +static int logfs_writepage(struct page *page, struct writeback_control *wbc)
> +{
> +	BUG();

Is this a permanent solution ?

> +	return 0;
> +}

> --- /dev/null	2007-04-18 05:32:26.652341749 +0200
> +++ linux-2.6.21logfs/fs/logfs/gc.c	2007-05-07 13:32:12.000000000 +0200
> @@ -0,0 +1,350 @@

Comment and license please.

> +#include "logfs.h"
> +
> +#if 0

Can you please remove this ?

> +/**
> + * When deciding which segment to use next, calculate the resistance
> + * of each segment and pick the lowest.  Segments try to resist usage
> + * if
> + * o they are full,
> + * o they have a high erase count or
> + * o they have recently been written.
> + *
> + * Full segments should not get reused, as there is little space to
> + * gain from them.  Segments with high erase count should be left
> + * aside as they can wear out sooner than others.  Freshly-written
> + * segments contain many blocks that will get obsoleted fairly soon,
> + * so it helps to wait a little before reusing them.
> + *
> + * Total resistance is expressed in erase counts.  Formula is:
> + *
> + * R = EC + K1*F + K2*e^(-t/theta)
> + *
> + * R:	Resistance
> + * EC:	Erase count
> + * K1:	Constant, 10,000 might be a good value
> + * K2:	Constant,  1,000 might be a good value
> + * F:	Segment fill level
> + * t:	Time since segment was written to (in number of segments written)
> + * theta: Time constant.  Total number of segments might be a good value
> + *
> + * Since the kernel is not allowed to use floating point, the function
> + * decay() is used to approximate exponential decay in fixed point.
> + */

Interestingly enough this unused function is better commented than
anything else in this patch.

> +static long decay(long t0, long t, long theta)
> +{
> +	long shift, fac;
> +
> +	if (t >= 32*theta)
> +		return 0;
> +
> +	shift = t/theta;
> +	fac = theta - (t%theta)/2;
> +	return (t0 >> shift) * fac / theta;
> +}
> +#endif
> +
> +
> +static u32 logfs_valid_bytes(struct super_block *sb, u32 segno)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	struct logfs_object_header h;
> +	u64 ofs, ino, pos;
> +	u32 seg_ofs, valid, size;
> +	void *reserved;
> +	int i;
> +
> +	/* Some segments are reserved.  Just pretend they were all valid */
> +	reserved = btree_lookup(&super->s_reserved_segments, segno);
> +	if (reserved)
> +		return super->s_segsize;
> +
> +	/* Currently open segments */
> +	/* FIXME: just reserve open areas and remove this code */
> +	for (i=0; i<LOGFS_NO_AREAS; i++) {
> +		struct logfs_area *area = super->s_area[i];
> +		if (area->a_is_open && (area->a_segno == segno)) {
> +			return super->s_segsize;
> +		}
> +	}
> +
> +	device_read(sb, segno, 0, sizeof(h), &h);

	See above comment about device_read() implementation.

> +	if (all_ff(&h, sizeof(h)))
> +		return 0;
> +
> +	valid = 0; /* segment header not counted as valid bytes */
> +	for (seg_ofs = sizeof(h); seg_ofs + sizeof(h) < super->s_segsize; ) {
> +		device_read(sb, segno, seg_ofs, sizeof(h), &h);
> +		if (all_ff(&h, sizeof(h)))
> +			break;
> +
> +		ofs = dev_ofs(sb, segno, seg_ofs);
> +		ino = be64_to_cpu(h.ino);
> +		pos = be64_to_cpu(h.pos);
> +		size = (u32)be16_to_cpu(h.len) + sizeof(h);
> +		//printk("%x %x (%llx, %llx, %llx)(%x, %x)\n", h.type, h.compr, ofs, ino, pos, valid, size);

	Please remove

> +		if (logfs_is_valid_block(sb, ofs, ino, pos))
> +			valid += size;
> +		seg_ofs += size;
> +	}
> +	printk("valid(%x) = %x\n", segno, valid);
> +	return valid;
> +}
> +
> +static void __logfs_gc_segment(struct super_block *sb, u32 segno)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	struct logfs_object_header h;
> +	struct logfs_segment_header *sh;
> +	u64 ofs, ino, pos;
> +	u32 seg_ofs;
> +	int level;
> +
> +	device_read(sb, segno, 0, sizeof(h), &h);


	See above comment about device_read() implementation.

> +	sh = (void*)&h;

Please use proper type casting !

> +	level = sh->level;
> +
> +	for (seg_ofs = sizeof(h); seg_ofs + sizeof(h) < super->s_segsize; ) {
> +		ofs = dev_ofs(sb, segno, seg_ofs);
> +		device_read(sb, segno, seg_ofs, sizeof(h), &h);

	See above comment about device_read() implementation.

> +		ino = be64_to_cpu(h.ino);
> +		pos = be64_to_cpu(h.pos);
> +		if (logfs_is_valid_block(sb, ofs, ino, pos))
> +			logfs_cleanse_block(sb, ofs, ino, pos, level);
> +		seg_ofs += sizeof(h);
> +		seg_ofs += be16_to_cpu(h.len);
> +	}
> +}
> +

> +static void __add_segment(struct list_head *list, int *count, u32 segno,
> +		int valid)
> +{
> +	struct logfs_segment *seg = kzalloc(sizeof(*seg), GFP_KERNEL);

empty line

> +	if (!seg)
> +		return;
> +
> +	seg->segno = segno;
> +	seg->valid = valid;
> +	list_add(&seg->list, list);
> +	*count += 1;
> +}

Also __add_segment() can fail. Why is there no return code ?

> +
> +
> +static void add_segment(struct list_head *list, int *count, u32 segno,
> +		int valid)
> +{
> +	struct logfs_segment *seg;
> +	list_for_each_entry(seg, list, list)
> +		if (seg->segno == segno)
> +			return;
> +	__add_segment(list, count, segno, valid);

	Can fail. Error handling ?

> +}
> +
> +
> +static void del_segment(struct list_head *list, int *count, u32 segno)
> +{
> +	struct logfs_segment *seg;

Empty line

> +	list_for_each_entry(seg, list, list)
> +		if (seg->segno == segno) {
> +			list_del(&seg->list);
> +			*count -= 1;
> +			kfree(seg);
> +			return;
> +		}
> +}
> +
> +
> +static void add_free_segment(struct super_block *sb, u32 segno)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	add_segment(&super->s_free_list, &super->s_free_count, segno, 0);
> +}

Empty line

> +static void add_low_segment(struct super_block *sb, u32 segno, int valid)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);

Empty line

> +	add_segment(&super->s_low_list, &super->s_low_count, segno, valid);

	Can fail

> +}
> +static void del_low_segment(struct super_block *sb, u32 segno)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);

Empty line

> +	del_segment(&super->s_low_list, &super->s_low_count, segno);
> +}
> +
> +
> +static void scan_segment(struct super_block *sb, u32 segno)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	u32 full = super->s_segsize - sb->s_blocksize - 0x18; /* one header */

Please use a understandable constant instead of 0x18

> +	int valid;
> +
> +	valid = logfs_valid_bytes(sb, segno);
> +	if (valid == 0) {
> +		del_low_segment(sb, segno);
> +		add_free_segment(sb, segno);
> +	} else if (valid < full)
> +		add_low_segment(sb, segno, valid);

		Can fail
> +}
> +
> +
> +static void free_all_segments(struct logfs_super *super)
> +{
> +	struct logfs_segment *seg, *next;
> +
> +	list_for_each_entry_safe(seg, next, &super->s_free_list, list) {
> +		list_del(&seg->list);
> +		kfree(seg);
> +	}
> +	list_for_each_entry_safe(seg, next, &super->s_low_list, list) {
> +		list_del(&seg->list);
> +		kfree(seg);
> +	}
> +}
> +
> +
> +static void logfs_scan_pass(struct super_block *sb)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	int i;
> +
> +	for (i = super->s_sweeper+1; i != super->s_sweeper; i++) {

	for (i = super->s_sweeper + 1; i != super->s_sweeper; i++) {


> +		if (i >= super->s_no_segs)
> +			i=1;	/* skip superblock */

			i = 1;
	and remove tail comment

> +
> +		scan_segment(sb, i);
> +
> +		if (super->s_free_count >= super->s_total_levels) {
> +			super->s_sweeper = i;
> +			return;
> +		}
> +	}
> +	scan_segment(sb, super->s_sweeper);
> +}
> +

> +/* GC all the low-count segments.  If necessary, rescan the medium.
> + * If we made enough room, return */
> +static void logfs_gc_several(struct super_block *sb)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	int rounds;
> +
> +	rounds = super->s_low_count;
> +
> +	for (; rounds; rounds--) {
> +		if (super->s_free_count >= super->s_total_levels)
> +			return;
> +		if (super->s_free_count < 3) {
> +			logfs_scan_pass(sb);
> +			printk("s");

	Debug leftover ?

> +		}
> +		logfs_gc_once(sb);
> +#if 1
> +		if (super->s_free_count >= super->s_total_levels)
> +			return;
> +		printk(".");
> +#endif

	Dito ?

> +	}
> +}
> +
> +
> +void logfs_gc_pass(struct super_block *sb)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	int i;
> +
> +	for (i=4; i; i--) {

	(i = 4; ...

	Please use a constant instead of 4


> +		if (super->s_free_count >= super->s_total_levels)
> +			return;
> +		logfs_scan_pass(sb);
> +
> +		if (super->s_free_count >= super->s_total_levels)
> +			return;
> +		printk("free:%8d, low:%8d, sweeper:%8lld\n",
> +				super->s_free_count, super->s_low_count,
> +				super->s_sweeper);

Debug leftover ? Otherwise please add loglevel and some hint from which
code this originates

> +		logfs_gc_several(sb);
> +		printk("free:%8d, low:%8d, sweeper:%8lld\n",
> +				super->s_free_count, super->s_low_count,
> +				super->s_sweeper);

Same here

> +	}
> +	logfs_fsck(sb);
> +	LOGFS_BUG(sb);
> +}
> +
> +
> +
> +void logfs_cleanup_gc(struct logfs_super *super)
> +{
> +	free_all_segments(super);
> +}

Can we add another wrapper to this please ?

> --- /dev/null	2007-04-18 05:32:26.652341749 +0200
> +++ linux-2.6.21logfs/fs/logfs/inode.c	2007-05-07 13:32:12.000000000 +0200
> @@ -0,0 +1,468 @@

Comment + license missing

> +#include "logfs.h"
> +#include <linux/backing-dev.h>
> +#include <linux/writeback.h> /* for inode_lock */

Please remove the stupid comment

> +
> +static struct kmem_cache *logfs_inode_cache;
> +
> +
> +static int __logfs_read_inode(struct inode *inode);
> +
> +
> +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) /* never iget this "inode"! */

	comment style

> +		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);
> +}
> +
> +
> +void logfs_iput(struct inode *inode, int cookie)
> +{
> +	if (inode->i_ino == LOGFS_INO_MASTER) /* never iput it either! */

	comment style

> +		return;
> +
> +	if (cookie)
> +		return;
> +
> +	iput(inode);
> +}
> +
> +
> +static void logfs_init_inode(struct inode *inode)
> +{
> +	struct logfs_inode *li = LOGFS_INODE(inode);
> +	int i;
> +
> +	li->li_flags	= LOGFS_IF_VALID;
> +	li->li_used_bytes = 0;
> +	inode->i_uid	= 0;
> +	inode->i_gid	= 0;
> +	inode->i_size	= 0;
> +	inode->i_blocks	= 0;
> +	inode->i_ctime	= CURRENT_TIME;
> +	inode->i_mtime	= CURRENT_TIME;
> +	inode->i_nlink	= 1;
> +	INIT_LIST_HEAD(&li->li_freeing_list);
> +
> +	for (i=0; i<LOGFS_EMBEDDED_FIELDS; i++)

	i = 0; .....

> +		li->li_data[i] = 0;
> +
> +	return;
> +}
> +
> +
> +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;

Please remove the brackets and move the variables to the top of the
fucntion

> +		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;
> +}
> +
> +
> +static struct timespec be64_to_timespec(be64 betime)
> +{
> +	u64 time = be64_to_cpu(betime);
> +	struct timespec tsp;

Empty line

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

tsp.tv_nsec & 0xffffffff ????

timespecs need to be normalized, so tv_nsec can never be greater than
999999999 == 0x3B9AC9FF

> +	return cpu_to_be64(time);
> +}
> +
> +
> +static void logfs_disk_to_inode(struct logfs_disk_inode *di, struct inode*inode)
> +{
> +	struct logfs_inode *li = LOGFS_INODE(inode);
> +	int i;
> +
> +	inode->i_mode	= be16_to_cpu(di->di_mode);
> +	li->li_flags	= be32_to_cpu(di->di_flags);
> +	inode->i_uid	= be32_to_cpu(di->di_uid);
> +	inode->i_gid	= be32_to_cpu(di->di_gid);
> +	inode->i_size	= be64_to_cpu(di->di_size);
> +	logfs_set_blocks(inode, be64_to_cpu(di->di_used_bytes));
> +	inode->i_ctime	= be64_to_timespec(di->di_ctime);
> +	inode->i_mtime	= be64_to_timespec(di->di_mtime);
> +	inode->i_nlink	= be32_to_cpu(di->di_refcount);
> +	inode->i_generation = be32_to_cpu(di->di_generation);
> +
> +	switch (inode->i_mode & S_IFMT) {
> +	case S_IFCHR: /* fall through */

Sigh. Could you please add useful comments ?

> +	case S_IFBLK: /* fall through */
> +	case S_IFIFO:
> +		inode->i_rdev = be64_to_cpu(di->di_data[0]);
> +		break;
> +	default:
> +		for (i=0; i<LOGFS_EMBEDDED_FIELDS; i++)

		i = 0; i < L.....

> +			li->li_data[i] = be64_to_cpu(di->di_data[i]);
> +		break;
> +	}
> +}
> +
> +
> +static void logfs_inode_to_disk(struct inode *inode, struct logfs_disk_inode*di)
> +{
> +	struct logfs_inode *li = LOGFS_INODE(inode);
> +	int i;
> +
> +	di->di_mode	= cpu_to_be16(inode->i_mode);
> +	di->di_pad	= 0;
> +	di->di_flags	= cpu_to_be32(li->li_flags);
> +	di->di_uid	= cpu_to_be32(inode->i_uid);
> +	di->di_gid	= cpu_to_be32(inode->i_gid);
> +	di->di_size	= cpu_to_be64(i_size_read(inode));
> +	di->di_used_bytes = cpu_to_be64(li->li_used_bytes);
> +	di->di_ctime	= timespec_to_be64(inode->i_ctime);
> +	di->di_mtime	= timespec_to_be64(inode->i_mtime);
> +	di->di_refcount	= cpu_to_be32(inode->i_nlink);
> +	di->di_generation = cpu_to_be32(inode->i_generation);
> +
> +	switch (inode->i_mode & S_IFMT) {
> +	case S_IFCHR: /* fall through */

See above

> +	case S_IFBLK: /* fall through */
> +	case S_IFIFO:
> +		di->di_data[0] = cpu_to_be64(inode->i_rdev);
> +		break;
> +	default:
> +		for (i=0; i<LOGFS_EMBEDDED_FIELDS; i++)

See above

> +			di->di_data[i] = cpu_to_be64(li->li_data[i]);
> +		break;
> +	}
> +}
> +
> +
> +static int logfs_read_disk_inode(struct logfs_disk_inode *di,
> +		struct inode *inode)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(inode->i_sb);
> +	ino_t ino = inode->i_ino;
> +	int ret;
> +
> +	BUG_ON(!super->s_master_inode);
> +	ret = logfs_inode_read(super->s_master_inode, di, sizeof(*di), ino);
> +	if (ret)
> +		return ret;
> +
> +	if ( !(be32_to_cpu(di->di_flags) & LOGFS_IF_VALID))
> +		return -EIO;
> +
> +	if (be32_to_cpu(di->di_flags) & LOGFS_IF_INVALID)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +
> +static int __logfs_read_inode(struct inode *inode)
> +{
> +	struct logfs_inode *li = LOGFS_INODE(inode);
> +	struct logfs_disk_inode di;
> +	int ret;
> +
> +	ret = logfs_read_disk_inode(&di, inode);
> +	/* FIXME: move back to mkfs when format has settled */
> +	if (ret == -ENODATA && inode->i_ino == LOGFS_INO_ROOT) {
> +		memset(&di, 0, sizeof(di));
> +		di.di_flags	= cpu_to_be32(LOGFS_IF_VALID);
> +		di.di_mode	= cpu_to_be16(S_IFDIR | 0755);
> +		di.di_refcount	= cpu_to_be32(2);
> +		ret = 0;
> +	}
> +	if (ret)
> +		return ret;
> +	logfs_disk_to_inode(&di, inode);
> +
> +	if ( !(li->li_flags&LOGFS_IF_VALID) || (li->li_flags&LOGFS_IF_INVALID))
> +		return -EIO;

	Is this really an IO error ?

> +	switch (inode->i_mode & S_IFMT) {
> +	case S_IFDIR:
> +		inode->i_op = &logfs_dir_iops;
> +		inode->i_fop = &logfs_dir_fops;
> +		break;
> +	case S_IFREG:
> +		inode->i_op = &logfs_reg_iops;
> +		inode->i_fop = &logfs_reg_fops;
> +		inode->i_mapping->a_ops = &logfs_reg_aops;
> +		break;
> +	default:
> +		;
> +	}
> +
> +	return 0;
> +}
> +

> +int __logfs_write_inode(struct inode *inode)
> +{
> +	struct logfs_disk_inode old, new; /* FIXME: move these off the stack */
> +
> +	BUG_ON(inode->i_ino == LOGFS_INO_MASTER);
> +
> +	/* read and compare the inode first.  If it hasn't changed, don't
> +	 * bother writing it. */

Comment style

> +	logfs_inode_to_disk(inode, &new);
> +	if (logfs_read_disk_inode(&old, inode))
> +		return logfs_write_disk_inode(&new, inode);
> +	if (memcmp(&old, &new, sizeof(old)))
> +		return logfs_write_disk_inode(&new, inode);
> +	return 0;
> +}
> +
> +
> +
> +/**

Do not use kernel doc comment start sequence for non kernel doc comments
please 

> + * We need to remember which inodes are currently being dropped.  They
> + * would deadlock the cleaner, if it were to iget() them.  So
> + * logfs_drop_inode() adds them to super->s_freeing_list,
> + * logfs_destroy_inode() removes them again and logfs_iget() checks the
> + * list.
> + */
> +static void logfs_destroy_inode(struct 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
> +	 */

Comment style

> +	spin_lock(&super->s_ino_lock);
> +	ino = super->s_last_ino; /* ifile shouldn't be too sparse */
> +	super->s_last_ino++;
> +	spin_unlock(&super->s_ino_lock);
> +	return ino;
> +}
> +

> --- /dev/null	2007-04-18 05:32:26.652341749 +0200
> +++ linux-2.6.21logfs/fs/logfs/journal.c	2007-05-07 13:32:12.000000000 +0200
> @@ -0,0 +1,696 @@

Comment and license missing

> +#include "logfs.h"
> +
> +
> +static void clear_retired(struct super_block *sb)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	int i;
> +
> +	for (i=0; i<JE_LAST; i++)

	i = 0; ....

> +		super->s_retired[i].used = 0;
> +	super->s_first.used = 0;
> +}
> +
> +
> +static void clear_speculatives(struct super_block *sb)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	int i;
> +
> +	for (i=0; i<JE_LAST; i++)

	dito

> +		super->s_speculative[i].used = 0;
> +}
> +
> +
> +static void retire_speculatives(struct super_block *sb)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	int i;
> +
> +	for (i=0; i<JE_LAST; i++) {
> +		struct logfs_journal_entry *spec = super->s_speculative + i;
> +		struct logfs_journal_entry *retired = super->s_retired + i;

empty line

> +		if (! spec->used)
> +			continue;
> +		if (retired->used && (spec->version <= retired->version))
> +			continue;
> +		retired->used = 1;
> +		retired->version = spec->version;
> +		retired->offset = spec->offset;
> +		retired->len = spec->len;
> +	}
> +	clear_speculatives(sb);
> +}
> +
> +
> +static void __logfs_scan_journal(struct super_block *sb, void *block,
> +		u32 segno, u64 block_ofs, int block_index)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	struct logfs_journal_header *h;
> +	struct logfs_area *area = super->s_journal_area;
> +
> +	for (h = block; (void*)h - block < sb->s_blocksize; h++) {
> +		struct logfs_journal_entry *spec, *retired;
> +		unsigned long ofs = (void*)h - block;
> +		unsigned long remainder = sb->s_blocksize - ofs;
> +		u16 len = be16_to_cpu(h->h_len);
> +		u16 type = be16_to_cpu(h->h_type);
> +		s16 version = be16_to_cpu(h->h_version);
> +
> +		if ((len < 16) || (len > remainder))
> +			continue;
> +		if ((type < JE_FIRST) || (type > JE_LAST))
> +			continue;
> +		if (h->h_crc != logfs_crc32(h, len, 4))
> +			continue;
> +
> +		if (!super->s_first.used) { /* remember first version */

	Comment style

> +			super->s_first.used = 1;
> +			super->s_first.version = version;
> +		}
> +		version -= super->s_first.version;
> +
> +		if (abs(version) > 1<<14) /* all versions should be near */
> +			LOGFS_BUG(sb);
> +
> +		spec = &super->s_speculative[type];
> +		retired = &super->s_retired[type];
> +		switch (type) {
> +		default: /* store speculative entry */

	Comment style

> +			if (spec->used && (version <= spec->version))
> +				break;
> +			spec->used = 1;
> +			spec->version = version;
> +			spec->offset = block_ofs + ofs;
> +			spec->len = len;
> +			break;
> +		case JE_COMMIT: /* retire speculative entries */

	Comment style

> +			if (retired->used && (version <= retired->version))
> +				break;
> +			retired->used = 1;
> +			retired->version = version;
> +			retired->offset = block_ofs + ofs;
> +			retired->len = len;
> +			retire_speculatives(sb);
> +			/* and set up journal area */
> +			area->a_segno = segno;
> +			area->a_used_objects = block_index;
> +			area->a_is_open = 0; /* never reuse same segment after
> +						mount - wasteful but safe */

	Comment style

> +			break;
> +		}
> +	}
> +}
> +
> +
> +static int logfs_scan_journal(struct super_block *sb)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	void *block = super->s_compressed_je;
> +	u64 ofs;
> +	u32 segno;
> +	int i, k, err;
> +
> +	clear_speculatives(sb);
> +	clear_retired(sb);
> +	journal_for_each(i) {
> +		segno = super->s_journal_seg[i];
> +		if (!segno)
> +			continue;
> +		for (k=0; k<super->s_no_blocks; k++) {

	k = 0;..........

> +			ofs = logfs_block_ofs(sb, segno, k);
> +			err = mtdread(sb, ofs, sb->s_blocksize, block);
> +			if (err)
> +				return err;
> +			__logfs_scan_journal(sb, block, segno, ofs, k);
> +		}
> +	}
> +	return 0;
> +}
> +

> +static void logfs_calc_free(struct super_block *sb)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	u64 no_segs = super->s_no_segs;
> +	u64 no_blocks = super->s_no_blocks;
> +	u64 blocksize = sb->s_blocksize;
> +	u64 free;
> +	int i, reserved_segs;
> +
> +	reserved_segs = 1; /* super_block */
> +	reserved_segs += super->s_bad_segments;
> +	journal_for_each(i)
> +		if (super->s_journal_seg[i])
> +			reserved_segs++;
> +
> +	free = no_segs * no_blocks * blocksize;		/* total size */
> +	free -= reserved_segs * no_blocks * blocksize;	/* sb & journal */
> +	free -= (no_segs - reserved_segs) * blocksize;	/* block summary */
> +	free -= super->s_used_bytes;			/* stored data */
> +	super->s_free_bytes = free;

	comments all over the function

> +}
> +

> +static void reserve_sb_and_journal(struct super_block *sb)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	struct btree_head *head = &super->s_reserved_segments;
> +	int i, err;
> +
> +	err = btree_insert(head, 0, (void*)1);

What stands 1 for ?

> +	BUG_ON(err);
> +
> +	journal_for_each(i) {
> +		if (! super->s_journal_seg[i])
> +			continue;
> +		err = btree_insert(head, super->s_journal_seg[i], (void*)1);
> +		BUG_ON(err);
> +	}
> +}
> +

> +static void logfs_read_anchor(struct super_block *sb, struct logfs_anchor *da)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	struct inode *inode = super->s_master_inode;
> +	struct logfs_inode *li = LOGFS_INODE(inode);
> +	int i;
> +
> +	super->s_last_ino = be64_to_cpu(da->da_last_ino);
> +	li->li_flags	= LOGFS_IF_VALID;
> +	i_size_write(inode, be64_to_cpu(da->da_size));
> +	li->li_used_bytes = be64_to_cpu(da->da_used_bytes);
> +
> +	for (i=0; i<LOGFS_EMBEDDED_FIELDS; i++)

	i = 0; ...

> +		li->li_data[i] = be64_to_cpu(da->da_data[i]);
> +}
> +

> +static void logfs_read_areas(struct super_block *sb, struct logfs_je_areas *a)
> +{
> +	struct logfs_area *area;
> +	int i;
> +
> +	for (i=0; i<LOGFS_NO_AREAS; i++) {

	Sigh

> +		area = LOGFS_SUPER(sb)->s_area[i];
> +		area->a_used_bytes = be32_to_cpu(a->used_bytes[i]);
> +		area->a_segno = be32_to_cpu(a->segno[i]);
> +		if (area->a_segno)
> +			area->a_is_open = 1;
> +	}
> +}
> +

> +/* FIXME: make sure there are enough per-area objects in journal */
> +static int logfs_read_journal(struct super_block *sb)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	void *block = super->s_compressed_je;
> +	void *scratch = super->s_je;
> +	int i, err, level;
> +	struct logfs_area *area;
> +
> +	for (i=0; i<JE_LAST; i++) {

i..

> +		struct logfs_journal_entry *je = super->s_retired + i;
> +		if (!super->s_retired[i].used)

		if (!super->s_retired[i].used) {

> +			switch (i) {
> +			case JE_COMMIT:
> +			case JE_DYNSB:
> +			case JE_ANCHOR:
> +				printk("LogFS: Missing journal entry %x?\n",
> +						i);
> +				return -EIO;
> +			default:
> +				continue;
> +			}

		}

> +		err = mtdread(sb, je->offset, sb->s_blocksize, block);
> +		if (err)
> +			return err;

> +		level = i & 0xf;

what is 0xf ?

> +		area = super->s_area[level];
> +		switch (i & ~0xf) {
> +		case JEG_BASE:
> +			switch (i) {

Represents I an enum or a bitfield or both ?

> +			case JE_COMMIT:
> +				/* just reads the latest version number */
> +				logfs_read_commit(super, block);
> +				break;
> +			case JE_DYNSB:
> +				logfs_read_dynsb(sb, unpack(block, scratch));
> +				break;
> +
> +static void journal_get_free_segment(struct logfs_area *area)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(area->a_sb);
> +	int i;
> +
> +	journal_for_each(i) {
> +		if (area->a_segno != super->s_journal_seg[i])
> +			continue;
> +empty_seg:
> +		i++;
> +		if (i == LOGFS_JOURNAL_SEGS)
> +			i = 0;
> +		if (!super->s_journal_seg[i])
> +			goto empty_seg;


Does this loop for ever or is there a guranteed exit ?
Please use a do while loop instead of the goto

> +		area->a_segno = super->s_journal_seg[i];
> +		++(super->s_journal_ec[i]);
> +		return;
> +	}
> +	BUG();
> +}
> +
> +
> +/**
> + * logfs_get_free_entry - return free space for journal entry
> + */
> +static s64 logfs_get_free_entry(struct super_block *sb)
> +{
> +	s64 ret;
> +
> +	mutex_lock(&LOGFS_SUPER(sb)->s_log_mutex);
> +	ret = __logfs_get_free_entry(sb);
> +	mutex_unlock(&LOGFS_SUPER(sb)->s_log_mutex);
> +	BUG_ON(ret <= 0); /* not sure, but it's safer to BUG than to accept */

It might be safer to do proper error handling.

> +	return ret;
> +}
> +

> +static size_t logfs_write_header(struct logfs_super *super,
> +		struct logfs_journal_header *h, size_t datalen, u16 type)
> +{
> +	size_t len = datalen + sizeof(*h);

Empty line

> +	return __logfs_write_header(super, h, len, datalen, type, COMPR_NONE);
> +}
> +
> +
> +static void *logfs_write_bb(struct super_block *sb, void *h,
> +		u16 *type, size_t *len)
> +{
> +	*type = JE_BADSEGMENTS;
> +	*len = sb->s_blocksize;
> +	return LOGFS_SUPER(sb)->s_bb_array;
> +}
> +
> +
> +static inline size_t logfs_journal_erasecount_size(struct logfs_super *super)
> +{
> +	return LOGFS_JOURNAL_SEGS * sizeof(be32);
> +}

E,pty line

> +static void *logfs_write_erasecount(struct super_block *sb, void *_ec,
> +		u16 *type, size_t *len)
> +{

> +
> +static void *__logfs_write_anchor(struct super_block *sb, void *_da,
> +		u16 *type, size_t *len)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	struct logfs_anchor *da = _da;
> +	struct inode *inode = super->s_master_inode;
> +	struct logfs_inode *li = LOGFS_INODE(inode);
> +	int i;
> +
> +	da->da_last_ino = cpu_to_be64(super->s_last_ino);
> +	da->da_size	= cpu_to_be64(i_size_read(inode));
> +	da->da_used_bytes = cpu_to_be64(li->li_used_bytes);
> +	for (i=0; i<LOGFS_EMBEDDED_FIELDS; i++)

	i = 0; ....

> +		da->da_data[i] = cpu_to_be64(li->li_data[i]);
> +	*type = JE_ANCHOR;
> +	*len = sizeof(*da);
> +	return da;
> +}
> +

> +
> +static void *logfs_write_areas(struct super_block *sb, void *_a,
> +		u16 *type, size_t *len)
> +{
> +	struct logfs_area *area;
> +	struct logfs_je_areas *a = _a;
> +	int i;
> +
> +	for (i=0; i<16; i++) { /* FIXME: have all 16 areas */
> +		a->used_bytes[i] = 0;
> +		a->segno[i] = 0;
> +	}

	memset perhaps ?

> +	for (i=0; i<LOGFS_NO_AREAS; i++) {

	i = 0; ...

> +		area = LOGFS_SUPER(sb)->s_area[i];
> +		a->used_bytes[i] = cpu_to_be32(area->a_used_bytes);
> +		a->segno[i] = cpu_to_be32(area->a_segno);
> +	}
> +	*type = JE_AREAS;
> +	*len = sizeof(*a);
> +	return a;
> +}
> +

> +int logfs_write_anchor(struct inode *inode)
> +{
> +	struct super_block *sb = inode->i_sb;
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	void *block = super->s_compressed_je;
> +	u64 ofs;
> +	size_t jpos;
> +	int i, ret;
> +
> +	ofs = logfs_get_free_entry(sb);
> +	BUG_ON(ofs >= super->s_size);
> +
> +	memset(block, 0, sb->s_blocksize);
> +	jpos = 0;
> +	for (i=0; i<LOGFS_NO_AREAS; i++) {

	i = 0; ...
> +		super->s_sum_index = i;
> +		jpos += logfs_write_je(sb, jpos, logfs_write_wbuf);
> +	}
> +	jpos += logfs_write_je(sb, jpos, logfs_write_bb);
> +	jpos += logfs_write_je(sb, jpos, logfs_write_erasecount);
> +	jpos += logfs_write_je(sb, jpos, __logfs_write_anchor);
> +	jpos += logfs_write_je(sb, jpos, logfs_write_dynsb);
> +	jpos += logfs_write_je(sb, jpos, logfs_write_areas);
> +	jpos += logfs_write_je(sb, jpos, logfs_write_commit);
> +
> +	BUG_ON(jpos > sb->s_blocksize);
> +
> +	ret = mtdwrite(sb, ofs, sb->s_blocksize, block);
> +	if (ret)
> +		return ret;
> +	return 0;

	Interesting way to reyl on compiler smartness

> +}
> +

> +int logfs_init_journal(struct super_block *sb)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	int ret;
> +
> +	mutex_init(&super->s_log_mutex);
> +
> +	super->s_je = kzalloc(sb->s_blocksize, GFP_KERNEL);
> +	if (!super->s_je)
> +		goto err0;
> +
> +	super->s_compressed_je = kzalloc(sb->s_blocksize, GFP_KERNEL);
> +	if (!super->s_compressed_je)
> +		goto err1;
> +
> +	super->s_bb_array = kzalloc(sb->s_blocksize, GFP_KERNEL);
> +	if (!super->s_bb_array)
> +		goto err2;
> +
> +	super->s_master_inode = logfs_new_meta_inode(sb, LOGFS_INO_MASTER);
> +	if (!super->s_master_inode)
> +		goto err3;
> +
> +	super->s_master_inode->i_nlink = 1; /* lock it in ram */
> +
> +	/* logfs_scan_journal() is looking for the latest journal entries, but
> +	 * doesn't copy them into data structures yet.  logfs_read_journal()
> +	 * then re-reads those entries and copies their contents over. */
> +	ret = logfs_scan_journal(sb);
> +	if (ret)
> +		return ret;

	what about the allocated buffers ?

> +	ret = logfs_read_journal(sb);
> +	if (ret)
> +		return ret;

	dito

> +	reserve_sb_and_journal(sb);
> +	logfs_calc_free(sb);
> +
> +	super->s_journal_area->a_ops = &journal_area_ops;
> +	return 0;
> +err3:
> +	kfree(super->s_bb_array);
> +err2:
> +	kfree(super->s_compressed_je);
> +err1:
> +	kfree(super->s_je);
> +err0:
> +	return -ENOMEM;
> +}
> +
> +
> --- /dev/null	2007-04-18 05:32:26.652341749 +0200
> +++ linux-2.6.21logfs/fs/logfs/readwrite.c	2007-05-07 20:37:05.000000000 +0200
> @@ -0,0 +1,1125 @@
> +/**
> + * fs/logfs/readwrite.c
> + *
> + * Actually contains five sets of very similar functions:
> + * read		read blocks from a file
> + * write	write blocks to a file
> + * valid	check whether a block still belongs to a file
> + * truncate	truncate a file
> + * rewrite	move existing blocks of a file to a new location (gc helper)

License ?

> + */
> +#include "logfs.h"
> +
> +
> +static int logfs_read_empty(void *buf, int read_zero)
> +{
> +	if (!read_zero)
> +		return -ENODATA;
> +
> +	memset(buf, 0, PAGE_CACHE_SIZE);

	Is buf guaranteed to be at least sizeof(PAGE_CACHE_SIZE) ?

> +	return 0;
> +}

> +static int logfs_read_direct(struct inode *inode, pgoff_t index, void *buf,
> +		int read_zero)
> +{
> +	struct logfs_inode *li = LOGFS_INODE(inode);
> +	u64 block;
> +
> +	block = li->li_data[index];
> +	if (!block)
> +		return logfs_read_empty(buf, read_zero);
> +
> +	//printk("ino=%lx, index=%lx, blocks=%llx\n", inode->i_ino, index, block);

Please remove

> +	return logfs_segment_read(inode->i_sb, buf, block);
> +}
> +
> +
> +
> +static unsigned long get_bits(u64 val, int skip, int no)
> +{
> +	u64 ret = val;
> +
> +	ret >>= skip * no;
> +	ret <<= 64 - no;
> +	ret >>= 64 - no;
> +	BUG_ON((unsigned long)ret != ret);

	????

> +	return ret;
> +}
> +
> +
> +
> +static u64 seek_data_loop(struct inode *inode, u64 pos, int count)
> +{
> +	struct logfs_inode *li = LOGFS_INODE(inode);
> +	struct logfs_super *super = LOGFS_SUPER(inode->i_sb);
> +	be64 *rblock;
> +	u64 bofs = li->li_data[I1_INDEX + count];
> +	int bits = LOGFS_BLOCK_BITS;
> +	int i, ret, slot;
> +
> +	BUG_ON(!bofs);
> +
> +	rblock = logfs_get_rblock(super);
> +
> +	for (i=count; i>=0; i--) {
> +		ret = logfs_segment_read(inode->i_sb, rblock, bofs);
> +		if (ret)
> +			goto out;

	break;

> +		slot = get_bits(pos, i, bits);
> +		while (slot < LOGFS_BLOCK_FACTOR && rblock[slot] == 0) {
> +			slot++;
> +			pos += 1 << (LOGFS_BLOCK_BITS * i);
> +		}
> +		if (slot >= LOGFS_BLOCK_FACTOR)
> +			goto out;

	break;

> +		bofs = be64_to_cpu(rblock[slot]);
> +	}
> +out:
> +	logfs_put_rblock(super);
> +	return pos;
> +}
> +

> +static int logfs_is_valid_loop(struct inode *inode, pgoff_t index,
> +		int count, u64 ofs)
> +{
> +	struct logfs_inode *li = LOGFS_INODE(inode);
> +	struct logfs_super *super = LOGFS_SUPER(inode->i_sb);
> +	be64 *rblock;
> +	u64 bofs = li->li_data[I1_INDEX + count];
> +	int bits = LOGFS_BLOCK_BITS;
> +	int i, ret;
> +
> +	if (!bofs)
> +		return 0;
> +
> +	if (bofs == ofs)
> +		return 1;
> +
> +	rblock = logfs_get_rblock(super);
> +
> +	for (i=count; i>=0; i--) {

	....

> +		ret = logfs_segment_read(inode->i_sb, rblock, bofs);
> +		if (ret)
> +			goto fail;

	please use break and do a return !ret;

> +		bofs = be64_to_cpu(rblock[get_bits(index, i, bits)]);
> +		if (!bofs)
> +			goto fail;
> +
> +		if (bofs == ofs) {
> +			ret = 1;
> +			goto out;
> +		}
> +	}
> +
> +fail:
> +	ret = 0;

	

> +out:
> +	logfs_put_rblock(super);
> +	return ret;
> +}
> +
> +
> +static int __logfs_is_valid_block(struct inode *inode, pgoff_t index, u64 ofs)
> +{
> +	struct logfs_inode *li = LOGFS_INODE(inode);
> +
> +	//printk("%lx, %x, %x\n", inode->i_ino, inode->i_nlink, atomic_read(&inode->i_count));

	Sigh

> +	if ((inode->i_nlink == 0) && atomic_read(&inode->i_count) == 1)
> +		return 0;
> +
> +	if (li->li_flags & LOGFS_IF_EMBEDDED)
> +		return 0;
> +
> +	if (index < I0_BLOCKS)
> +		return logfs_is_valid_direct(li, index, ofs);
> +	else if (index < I1_BLOCKS)
> +		return logfs_is_valid_loop(inode, index, 0, ofs);
> +	else if (index < I2_BLOCKS)
> +		return logfs_is_valid_loop(inode, index, 1, ofs);
> +	else if (index < I3_BLOCKS)
> +		return logfs_is_valid_loop(inode, index, 2, ofs);
> +
> +	BUG();
> +	return 0;
> +}
> +
> +
> +int logfs_is_valid_block(struct super_block *sb, u64 ofs, u64 ino, u64 pos)
> +{
> +	struct inode *inode;
> +	int ret, cookie;
> +
> +	/* Umount closes a segment with free blocks remaining.  Those
> +	 * blocks are by definition invalid. */
> +	if (ino == -1)
> +		return 0;
> +
> +	if ((u64)(u_long)ino != ino) {
> +		printk("%llx, %llx, %llx\n", ofs, ino, pos);

	more sigh

> +		LOGFS_BUG(sb);
> +	}
> +	inode = logfs_iget(sb, ino, &cookie);
> +	if (!inode)
> +		return 0;
> +
> +#if 0
> +	/* Any data belonging to dirty inodes must be considered valid until
> +	 * the inode is written back.  If we prematurely deleted old blocks
> +	 * and crashed before the inode is written, the filesystem goes boom.
> +	 */
> +	if (inode->i_state & I_DIRTY)
> +		ret = 2;
> +	else

There seems to be a patternm, that unused code is surprisingly well
commented.

> +#endif
> +		ret = __logfs_is_valid_block(inode, pos, ofs);
> +
> +	logfs_iput(inode, cookie);
> +	return ret;
> +}
> +
> +
> +
> +/**
> + * logfs_file_read - generic_file_read for in-kernel buffers
> + */
> +static ssize_t __logfs_inode_read(struct inode *inode, char *buf, size_t count,
> +		loff_t *ppos, int read_zero)
> +{
> +	void *block_data = NULL;
> +	loff_t size = i_size_read(inode);
> +	int err = -ENOMEM;
> +
> +	pr_debug("read from %lld, count %zd\n", *ppos, count);

Loglevel missing

> +	if (*ppos >= size)
> +		return 0;
> +	if (count > size - *ppos)
> +		count = size - *ppos;
> +
> +	BUG_ON(logfs_index(*ppos) != logfs_index(*ppos + count - 1));
> +
> +	block_data = kzalloc(LOGFS_BLOCKSIZE, GFP_KERNEL);
> +	if (!block_data)
> +		goto fail;
> +
> +	err = logfs_read_block(inode, logfs_index(*ppos), block_data,
> +			read_zero);
> +	if (err)
> +		goto fail;
> +
> +	memcpy(buf, block_data + (*ppos % LOGFS_BLOCKSIZE), count);
> +	*ppos += count;
> +	kfree(block_data);
> +	return count;

	err = count; and fall trough ?

> +fail:
> +	kfree(block_data);
> +	return err;
> +}
> +

> +static int logfs_alloc_bytes(struct inode *inode, int bytes)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(inode->i_sb);
> +
> +	if (!bytes)
> +		return 0;
> +
> +	if (super->s_free_bytes < bytes + super->s_gc_reserve) {
> +		//TRACE();

	Sigh.

> +		return -ENOSPC;
> +	}
> +
> +	/* Actual allocation happens later.  Make sure we don't drop the
> +	 * lock before then! */
> +
> +	return 0;
> +}
> +

> +
> +/*
> + * File is too large for embedded data when called.  Move data to first
> + * block and clear embedded area
> + */
> +static int logfs_move_embedded(struct inode *inode, be64 **wblocks)
> +{
> +	struct logfs_inode *li = LOGFS_INODE(inode);
> +	void *buf;
> +	s64 block;
> +	int i;
> +
> +	if (! (li->li_flags & LOGFS_IF_EMBEDDED))
> +		return 0;
> +
> +	if (logfs_alloc_blocks(inode, 1)) {
> +		//TRACE();

	more sigh
> +		return -ENOSPC;
> +	}
> +
> +	buf = wblocks[0];
> +
> +	memcpy(buf, li->li_data, LOGFS_EMBEDDED_SIZE);
> +	block = logfs_segment_write(inode, buf, 0, 0, 1);
> +	if (block < 0)
> +		return block;
> +
> +	li->li_data[0] = block;
> +
> +	li->li_flags &= ~LOGFS_IF_EMBEDDED;
> +	for (i=1; i<LOGFS_EMBEDDED_FIELDS; i++)
> +		li->li_data[i] = 0;
> +
> +	return logfs_dirty_inode(inode);
> +}
> +
> +
> +static int logfs_write_direct(struct inode *inode, pgoff_t index, void *buf)
> +{
> +	struct logfs_inode *li = LOGFS_INODE(inode);
> +	s64 block;
> +
> +	if (li->li_data[index] == 0) {
> +		if (logfs_alloc_blocks(inode, 1)) {
> +			//TRACE();

	again
	
> +			return -ENOSPC;
> +		}
> +	}
> +	block = logfs_segment_write(inode, buf, index, 0, 1);
> +	if (block < 0)
> +		return block;
> +
> +	if (li->li_data[index])
> +		logfs_segment_delete(inode, li->li_data[index], index, 0);
> +	li->li_data[index] = block;
> +
> +	return logfs_dirty_inode(inode);
> +}
> +
> +
> +static int logfs_write_loop(struct inode *inode, pgoff_t index, void *buf,
> +		be64 **wblocks, int count)
> +{
> +	struct logfs_inode *li = LOGFS_INODE(inode);
> +	u64 bofs = li->li_data[I1_INDEX + count];
> +	s64 block;
> +	int bits = LOGFS_BLOCK_BITS;
> +	int allocs = 0;
> +	int i, ret;
> +
> +	for (i=count; i>=0; i--) {
> +		if (bofs) {
> +			ret = logfs_segment_read(inode->i_sb, wblocks[i], bofs);
> +			if (ret)
> +				return ret;
> +		} else {
> +			allocs++;
> +			memset(wblocks[i], 0, LOGFS_BLOCKSIZE);
> +		}
> +		bofs = be64_to_cpu(wblocks[i][get_bits(index, i, bits)]);
> +	}
> +
> +	if (! wblocks[0][get_bits(index, 0, bits)])
> +		allocs++;
> +	if (logfs_alloc_blocks(inode, allocs)) {
> +		//TRACE();

	yet more

> +		return -ENOSPC;
> +	}
> +
> +	block = logfs_segment_write(inode, buf, index, 0, allocs);
> +	allocs = allocs ? allocs-1 : 0;
> +	if (block < 0)
> +		return block;
> +
> +	for (i=0; i<=count; i++) {

	i = 0; ....

> +		wblocks[i][get_bits(index, i, bits)] = cpu_to_be64(block);
> +		block = logfs_segment_write(inode, wblocks[i], index, i+1,
> +				allocs);
> +		allocs = allocs ? allocs-1 : 0;
> +		if (block < 0)
> +			return block;
> +	}
> +
> +	li->li_data[I1_INDEX + count] = block;
> +
> +	return logfs_dirty_inode(inode);
> +}
> +
> +
> +
> +
> +int logfs_rewrite_block(struct inode *inode, pgoff_t index, u64 ofs, int level)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(inode->i_sb);
> +	be64 **wblocks;
> +	void *buf;
> +	int ret;
> +
> +	//printk("(%lx, %lx, %llx, %x)\n", inode->i_ino, index, ofs, level);

	yay !

> +	wblocks = super->s_wblock;
> +	buf = wblocks[LOGFS_MAX_INDIRECT];
> +	ret = __logfs_rewrite_block(inode, index, buf, wblocks, level);
> +	return ret;
> +}
> +
> +
> +/**

Please do not use /** here, it is the start sequence for kernel doc
comments

> + * Three cases exist:
> + * size <= pos			- remove full block
> + * size >= pos + chunk		- do nothing
> + * pos < size < pos + chunk	- truncate, rewrite
> + */
> +static s64 __logfs_truncate_i0(struct inode *inode, u64 size, u64 bofs,
> +		u64 pos, be64 **wblocks)
> +{
> +	size_t len = size - pos;
> +	void *buf = wblocks[LOGFS_MAX_INDIRECT];
> +	int err;
> +
> +	if (size <= pos) {	/* remove whole block */
> +		logfs_segment_delete(inode, bofs,
> +				pos >> inode->i_sb->s_blocksize_bits, 0);
> +		return 0;
> +	}
> +
> +	/* truncate this block, rewrite it */
> +	err = logfs_segment_read(inode->i_sb, buf, bofs);
> +	if (err)
> +		return err;
> +
> +	memset(buf + len, 0, LOGFS_BLOCKSIZE - len);
> +	return logfs_segment_write_pos(inode, buf, pos, 0, 0);
> +}
> +
> +
> +/* FIXME: move to super */

Please do so

> +static u64 logfs_factor[] = {
> +	LOGFS_BLOCKSIZE,
> +	LOGFS_I1_SIZE,
> +	LOGFS_I2_SIZE,
> +	LOGFS_I3_SIZE
> +};
> +

> +
> +static ssize_t __logfs_inode_write(struct inode *inode, const char *buf,
> +		size_t count, loff_t *ppos)
> +{
> +	void *block_data = NULL;
> +	int err = -ENOMEM;
> +
> +	pr_debug("write to 0x%llx, count %zd\n", *ppos, count);
> +
> +	BUG_ON(logfs_index(*ppos) != logfs_index(*ppos + count - 1));
> +
> +	block_data = kzalloc(LOGFS_BLOCKSIZE, GFP_KERNEL);
> +	if (!block_data)
> +		goto fail;
> +
> +	err = logfs_read_block(inode, logfs_index(*ppos), block_data, 1);
> +	if (err)
> +		goto fail;
> +
> +	memcpy(block_data + (*ppos % LOGFS_BLOCKSIZE), buf, count);
> +
> +	if (i_size_read(inode) < *ppos + count)
> +		i_size_write(inode, *ppos + count);
> +
> +	err = logfs_write_buf(inode, logfs_index(*ppos), block_data);
> +	if (err)
> +		goto fail;
> +
> +	*ppos += count;
> +	pr_debug("write to %lld, count %zd\n", *ppos, count);

	Please add some hint, where this comes from

> +	kfree(block_data);
> +	return count;

	err = count; fall trhough ?

> +fail:
> +	kfree(block_data);
> +	return err;
> +}
> +
> +
> +int logfs_inode_read(struct inode *inode, void *buf, size_t n, loff_t _pos)
> +{
> +	loff_t pos = _pos << inode->i_sb->s_blocksize_bits;
> +	ssize_t ret;
> +
> +	if (pos >= i_size_read(inode))
> +		return -EOF;
> +	ret = __logfs_inode_read(inode, buf, n, &pos, 0);
> +	if (ret < 0)
> +		return ret;
> +	ret = ret==n ? 0 : -EIO;

	return ret == n ? ..... perhaps ?

> +	return ret;
> +}
> +
> +
> +
> +int logfs_init_rw(struct logfs_super *super)
> +{
> +	int i;
> +
> +	mutex_init(&super->s_r_mutex);
> +	mutex_init(&super->s_w_mutex);
> +	super->s_rblock = kmalloc(LOGFS_BLOCKSIZE, GFP_KERNEL);
> +	if (!super->s_wblock)
> +		return -ENOMEM;
> +	for (i=0; i<=LOGFS_MAX_INDIRECT; i++) {

	i = 0; ...

> +		super->s_wblock[i] = kmalloc(LOGFS_BLOCKSIZE, GFP_KERNEL);
> +		if (!super->s_wblock) {
> +			logfs_cleanup_rw(super);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +
> +void logfs_cleanup_rw(struct logfs_super *super)
> +{
> +	int i;
> +
> +	for (i=0; i<=LOGFS_MAX_INDIRECT; i++)

	dito

> +		kfree(super->s_wblock[i]);
> +	kfree(super->s_rblock);
> +}
> --- /dev/null	2007-04-18 05:32:26.652341749 +0200
> +++ linux-2.6.21logfs/fs/logfs/super.c	2007-05-07 13:32:12.000000000 +0200
> @@ -0,0 +1,490 @@

Comment, license please

> +#include "logfs.h"
> +
> +
> +#define FAIL_ON(cond) do { if (unlikely((cond))) return -EINVAL; } while(0)

Please open code

> +int mtdread(struct super_block *sb, loff_t ofs, size_t len, void *buf)
> +{
> +	struct mtd_info *mtd = LOGFS_SUPER(sb)->s_mtd;
> +	size_t retlen;
> +	int ret;
> +
> +	ret = mtd->read(mtd, ofs, len, &retlen, buf);
> +	if (ret || (retlen != len)) {
> +		printk("ret: %x\n", ret);
> +		printk("retlen: %x, len: %x\n", retlen, len);
> +		printk("ofs: %llx, mtd->size: %x\n", ofs, mtd->size);

	Sigh

> +		dump_stack();
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +
> +static void check(void *buf, size_t len)
> +{
> +	char value[8] = {0x5a, 0x5a, 0x5a, 0x5a, 0x5a, 0x5a, 0x5a, 0x5a};
> +	void *poison = buf, *end = buf + len;
> +
> +	while (poison) {
> +		poison = memchr(poison, value[0], end-poison);
> +		if (!poison || poison + 8 > end)
> +			return;
> +		if (! memcmp(poison, value, 8)) {
> +			printk("%p %p %p\n", buf, poison, end);

	More sigh

> +			BUG();
> +		}
> +		poison++;
> +	}
> +}
> +
> +
> +int mtdwrite(struct super_block *sb, loff_t ofs, size_t len, void *buf)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	struct mtd_info *mtd = super->s_mtd;
> +	struct inode *inode = super->s_dev_inode;
> +	size_t retlen;
> +	loff_t page_start, page_end;
> +	int ret;
> +
> +	if (0) /* FIXME: this should be a debugging option */
> +		check(buf, len);
> +
> +	//printk("write ofs=%llx, len=%x\n", ofs, len);

	hrmpf

> +	BUG_ON((ofs >= mtd->size) || (len > mtd->size - ofs));
> +	BUG_ON(ofs != (ofs >> super->s_writeshift) << super->s_writeshift);
> +	//BUG_ON(len != (len >> super->s_blockshift) << super->s_blockshift);


	hrmpf

> +	/* FIXME: fix all callers to write PAGE_CACHE_SIZE'd chunks */
> +	BUG_ON(len > PAGE_CACHE_SIZE);
> +	page_start = ofs & PAGE_CACHE_MASK;
> +	page_end = PAGE_CACHE_ALIGN(ofs + len) - 1;
> +	truncate_inode_pages_range(&inode->i_data, page_start, page_end);
> +	ret = mtd->write(mtd, ofs, len, &retlen, buf);
> +	if (ret || (retlen != len))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +
> +static DECLARE_COMPLETION(logfs_erase_complete);

empty line

> +static void logfs_erase_callback(struct erase_info *ei)
> +{
> +	complete(&logfs_erase_complete);
> +}

dito

> +int mtderase(struct super_block *sb, loff_t ofs, size_t len)
> +{
> +	struct mtd_info *mtd = LOGFS_SUPER(sb)->s_mtd;
> +	struct inode *inode = LOGFS_SUPER(sb)->s_dev_inode;
> +	struct erase_info ei;
> +	int ret;
> +
> +	BUG_ON(len % mtd->erasesize);
> +
> +	truncate_inode_pages_range(&inode->i_data, ofs, ofs+len-1);
> +	if (mtd->block_isbad(mtd, ofs))
> +		return -EIO;

this actually leads to a double check of block_isbad for blocks which
are not bad. 

> +	memset(&ei, 0, sizeof(ei));
> +	ei.mtd = mtd;
> +	ei.addr = ofs;
> +	ei.len = len;
> +	ei.callback = logfs_erase_callback;
> +	ret = mtd->erase(mtd, &ei);
> +	if (ret)
> +		return -EIO;
> +
> +	wait_for_completion(&logfs_erase_complete);
> +	if (ei.state != MTD_ERASE_DONE)
> +		return -EIO;
> +	return 0;
> +}
> +
> +
> +
> +void *logfs_device_getpage(struct super_block *sb, u64 offset,
> +		struct page **page)
> +{
> +	struct inode *inode = LOGFS_SUPER(sb)->s_dev_inode;
> +
> +	*page = read_cache_page(inode->i_mapping, offset >> PAGE_CACHE_SHIFT,
> +			logfs_readdevice, NULL);
> +	BUG_ON(IS_ERR(*page));	/* TODO: use mempool here */

				For the BUG ?

> +	return kmap(*page);
> +}
> +
> +
> +static int logfs_get_sb_final(struct super_block *sb, struct vfsmount *mnt)
> +{
> +	struct inode *rootdir;
> +	int err;
> +
> +	/* root dir */
> +	rootdir = iget(sb, LOGFS_INO_ROOT);
> +	if (!rootdir)
> +		goto fail;
> +
> +	sb->s_root = d_alloc_root(rootdir);
> +	if (!sb->s_root)
> +		goto fail;
> +
> +#if 1
> +	err = logfs_fsck(sb);
> +#else
> +	err = 0;
> +#endif

	Please cleanup

> +	if (err) {
> +		printk(KERN_ERR "LOGFS: fsck failed, refusing to mount\n");
> +		goto fail;
> +	}
> +
> +	return simple_set_mnt(mnt, sb);
> +
> +fail:
> +	iput(LOGFS_SUPER(sb)->s_master_inode);
> +	return -EIO;
> +}
> +
> +
> +
> +
> +
> +static int logfs_get_sb(struct file_system_type *type, int flags,
> +		const char *devname, void *data, struct vfsmount *mnt)
> +{
> +	ulong mtdnr;
> +	struct mtd_info *mtd;
> +
> +#if 0
> +	if (!devname)
> +		return ERR_PTR(-EINVAL);
> +	if (strncmp(devname, "mtd", 3))
> +		return ERR_PTR(-EINVAL);
> +
> +	{
> +		char *garbage;
> +		mtdnr = simple_strtoul(devname+3, &garbage, 0);
> +		if (*garbage)
> +			return ERR_PTR(-EINVAL);
> +	}
> +#else
> +	mtdnr = 0;
> +#endif
> +

	Please cleanup

> +	mtd = get_mtd_device(NULL, mtdnr);
> +	if (!mtd)
> +		return -EINVAL;
> +
> +	return logfs_get_sb_mtd(type, flags, mtd, mnt);
> +}
> +
> +-- /dev/null	2007-04-18 05:32:26.652341749 +0200
> +++ linux-2.6.21logfs/fs/logfs/progs/mkfs.c	2007-05-07 13:32:12.000000000 +0200

why needs this to be in a sub directory ? And shouldn't this be user
space tools - or what I'm missing here ?

> @@ -0,0 +1,319 @@

Comment, license

> +#include "../logfs.h"
> +
> +#define OFS_SB		0
> +#define OFS_JOURNAL	1
> +#define OFS_ROOTDIR	3
> +#define OFS_IFILE	4
> +#define OFS_COUNT	5

enum ?

> +static u64 segment_offset[OFS_COUNT];
> +
> +static u64 fssize;
> +static u64 no_segs;
> +static u64 free_blocks;
> +
> +static u32 segsize;
> +static u32 blocksize;
> +static int segshift;
> +static int blockshift;
> +static int writeshift;
> +
> +static u32 blocks_per_seg;
> +static u16 version;
> +
> +static be32 bb_array[1024];
> +static int bb_count;
> +
> +
> +#if 0
> +/* rootdir */
> +static int make_rootdir(struct super_block *sb)
> +{
> +	struct logfs_disk_inode *di;
> +	int ret;
> +
> +	di = kzalloc(blocksize, GFP_KERNEL);
> +	if (!di)
> +		return -ENOMEM;
> +
> +	di->di_flags	= cpu_to_be32(LOGFS_IF_VALID);
> +	di->di_mode	= cpu_to_be16(S_IFDIR | 0755);
> +	di->di_refcount	= cpu_to_be32(2);
> +	ret = mtdwrite(sb, segment_offset[OFS_ROOTDIR], blocksize, di);
> +	kfree(di);
> +	return ret;
> +}
> +
> +
> +/* summary */
> +static int make_summary(struct super_block *sb)
> +{
> +	struct logfs_disk_sum *sum;
> +	u64 sum_ofs;
> +	int ret;
> +
> +	sum = kzalloc(LOGFS_BLOCKSIZE, GFP_KERNEL);
> +	if (!sum)
> +		return -ENOMEM;
> +	memset(sum, 0xff, LOGFS_BLOCKSIZE);
> +
> +	sum->oids[0].ino = cpu_to_be64(LOGFS_INO_MASTER);
> +	sum->oids[0].pos = cpu_to_be64(LOGFS_INO_ROOT);
> +	sum_ofs = segment_offset[OFS_ROOTDIR];
> +	sum_ofs += segsize - blocksize;
> +	sum->level = LOGFS_MAX_LEVELS;
> +	ret = mtdwrite(sb, sum_ofs, LOGFS_BLOCKSIZE, sum);
> +	kfree(sum);
> +	return ret;
> +}
> +#endif

Please remove

> +
> +/* journal */
> +static size_t __write_header(struct logfs_journal_header *h, size_t len,
> +		size_t datalen, u16 type, u8 compr)
> +{
> +	h->h_len	= cpu_to_be16(len);
> +	h->h_type	= cpu_to_be16(type);
> +	h->h_version	= cpu_to_be16(++version);
> +	h->h_datalen	= cpu_to_be16(datalen);
> +	h->h_compr	= compr;
> +	h->h_pad[0]	= 'h';
> +	h->h_pad[1]	= 'a';
> +	h->h_pad[2]	= 't';
> +	h->h_crc	= logfs_crc32(h, len, 4);
> +	return len;
> +}
> +static size_t write_header(struct logfs_journal_header *h, size_t datalen,
> +		u16 type)
> +{
> +	size_t len = datalen + sizeof(*h);
> +	return __write_header(h, len, datalen, type, COMPR_NONE);
> +}
> +static size_t je_badsegments(void *data, u16 *type)
> +{
> +	memcpy(data, bb_array, blocksize);
> +	*type = JE_BADSEGMENTS;
> +	return blocksize;
> +}
> +static size_t je_anchor(void *_da, u16 *type)
> +{
> +	struct logfs_anchor *da = _da;
> +
> +	memset(da, 0, sizeof(*da));
> +	da->da_last_ino	= cpu_to_be64(LOGFS_RESERVED_INOS);
> +	da->da_size	= cpu_to_be64((LOGFS_INO_ROOT+1) * blocksize);
> +#if 0
> +	da->da_used_bytes = cpu_to_be64(blocksize);
> +	da->da_data[LOGFS_INO_ROOT] = cpu_to_be64(3*segsize);
> +#else
> +	da->da_data[LOGFS_INO_ROOT] = 0;
> +#endif

Please cleanup

> +	*type = JE_ANCHOR;
> +	return sizeof(*da);
> +}

Empty line

> +static size_t je_dynsb(void *_dynsb, u16 *type)
> +{
> +	struct logfs_dynsb *dynsb = _dynsb;
> +
> +	memset(dynsb, 0, sizeof(*dynsb));
> +	dynsb->ds_used_bytes	= cpu_to_be64(blocksize);
> +	*type = JE_DYNSB;
> +	return sizeof(*dynsb);
> +}

Same

> +static size_t je_commit(void *h, u16 *type)
> +{
> +	*type = JE_COMMIT;
> +	return 0;
> +}

Same

> +static size_t write_je(size_t jpos, void *scratch, void *header,
> +		size_t (*write)(void *scratch, u16 *type))
> +{
> +	void *data;
> +	ssize_t len, max, compr_len, pad_len, full_len;
> +	u16 type;
> +	u8 compr = COMPR_ZLIB;
> +
> +	header += jpos;
> +	data = header + sizeof(struct logfs_journal_header);
> +
> +	len = write(scratch, &type);
> +	if (len == 0)
> +		return write_header(header, 0, type);
> +
> +	max = blocksize - jpos;
> +	compr_len = logfs_compress(scratch, data, len, max);
> +	if ((compr_len < 0) || (type == JE_ANCHOR)) {
> +		compr_len = logfs_memcpy(scratch, data, len, max);
> +		compr = COMPR_NONE;
> +	}
> +	BUG_ON(compr_len < 0);
> +
> +	pad_len = ALIGN(compr_len, 16);
> +	memset(data + compr_len, 0, pad_len - compr_len);
> +	full_len = pad_len + sizeof(struct logfs_journal_header);
> +
> +	return __write_header(header, full_len, len, type, compr);
> +}

Same

> +static int make_journal(struct super_block *sb)
> +{
> +	void *journal, *scratch;
> +	size_t jpos;
> +	int ret;
> +
> +	journal = kzalloc(2*blocksize, GFP_KERNEL);
> +	if (!journal)
> +		return -ENOMEM;
> +
> +	scratch = journal + blocksize;
> +
> +	jpos = 0;
> +	/* erasecount is not written - implicitly set to 0 */
> +	/* neither are summary, index, wbuf */
> +	jpos += write_je(jpos, scratch, journal, je_badsegments);
> +	jpos += write_je(jpos, scratch, journal, je_anchor);
> +	jpos += write_je(jpos, scratch, journal, je_dynsb);
> +	jpos += write_je(jpos, scratch, journal, je_commit);
> +	ret = mtdwrite(sb, segment_offset[OFS_JOURNAL], blocksize, journal);
> +	kfree(journal);
> +	return ret;
> +}
> +
> +
> +/* superblock */
> +static int make_super(struct super_block *sb, struct logfs_disk_super *ds)
> +{
> +	void *sector;
> +	int ret;
> +
> +	sector = kzalloc(4096, GFP_KERNEL);
> +	if (!sector)
> +		return -ENOMEM;
> +
> +	memset(ds, 0, sizeof(*ds));
> +
> +	ds->ds_magic		= cpu_to_be64(LOGFS_MAGIC);
> +#if 0	/* sane defaults */
> +	ds->ds_ifile_levels	= 3; /* 2+1, 1GiB */
> +	ds->ds_iblock_levels	= 4; /* 3+1, 512GiB */
> +	ds->ds_data_levels	= 3; /* old, young, unknown */
> +#else
> +	ds->ds_ifile_levels	= 1; /* 0+1, 80kiB */
> +	ds->ds_iblock_levels	= 4; /* 3+1, 512GiB */
> +	ds->ds_data_levels	= 1; /* unknown */
> +#endif

Please cleanup

> +	ds->ds_feature_incompat	= 0;
> +	ds->ds_feature_ro_compat= 0;
> +
> +	ds->ds_feature_compat	= 0;
> +	ds->ds_flags		= 0;
> +
> +	ds->ds_filesystem_size	= cpu_to_be64(fssize);
> +	ds->ds_segment_shift	= segshift;
> +	ds->ds_block_shift	= blockshift;
> +	ds->ds_write_shift	= writeshift;
> +
> +	ds->ds_journal_seg[0]	= cpu_to_be64(1);
> +	ds->ds_journal_seg[1]	= cpu_to_be64(2);
> +	ds->ds_journal_seg[2]	= 0;
> +	ds->ds_journal_seg[3]	= 0;
> +
> +	ds->ds_root_reserve	= 0;
> +
> +	ds->ds_crc = logfs_crc32(ds, sizeof(*ds), 12);
> +
> +	memcpy(sector, ds, sizeof(*ds));
> +	ret = mtdwrite(sb, segment_offset[OFS_SB], 4096, sector);
> +	kfree(sector);
> +	return ret;
> +}
> +
> +
> +int logfs_mkfs(struct super_block *sb, struct logfs_disk_super *ds)
> +{
> +	int ret = 0;
> +
> +	segshift = 17;
> +	blockshift = 12;
> +	writeshift = 8;
> +
> +	segsize = 1 << segshift;
> +	blocksize = 1 << blockshift;
> +	version = 0;
> +
> +	getsize(sb, &fssize, &no_segs);
> +
> +	/* 3 segs for sb and journal,
> +	 * 1 block per seg extra,
> +	 * 1 block for rootdir
> +	 */
> +	blocks_per_seg = 1 << (segshift - blockshift);
> +	free_blocks = (no_segs - 3) * (blocks_per_seg - 1) - 1;
> +
> +	ret = bad_block_scan(sb);
> +	if (ret)
> +		return ret;
> +
> +	{
> +		int i;
> +		for (i=0; i<OFS_COUNT; i++)
> +			printk("%x->%llx\n", i, segment_offset[i]);
> +	}
> +
> +#if 0
> +	ret = make_rootdir(sb);
> +	if (ret)
> +		return ret;
> +
> +	ret = make_summary(sb);
> +	if (ret)
> +		return ret;
> +#endif

Same

> +	ret = make_journal(sb);
> +	if (ret)
> +		return ret;
> +
> +	ret = make_super(sb, ds);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> --- /dev/null	2007-04-18 05:32:26.652341749 +0200
> +++ linux-2.6.21logfs/fs/logfs/progs/fsck.c	2007-05-07 13:32:12.000000000 +0200
> @@ -0,0 +1,323 @@

Comment, license

> +#include "../logfs.h"
> +
> +static u64 used_bytes;
> +static u64 free_bytes;
> +static u64 last_ino;
> +static u64 *inode_bytes;
> +static u64 *inode_links;
> +
> +
> +/**
> + * Pass 1: blocks
> + */
> +
> +
> +static void safe_read(struct super_block *sb, u32 segno, u32 ofs,
> +		size_t len, void *buf)
> +{
> +	BUG_ON(wbuf_read(sb, dev_ofs(sb, segno, ofs), len, buf));
> +}

Empty line

> +static u32 logfs_free_bytes(struct super_block *sb, u32 segno)
> +{

> +static void logfsck_blocks(struct super_block *sb)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	int i;
> +	int free;
> +
> +	for (i=0; i<super->s_no_segs; i++) {
> +		free = logfs_free_bytes(sb, i);
> +		free_bytes += free;
> +		printk(" %3x", free);
> +		if (i % 8 == 7)
> +			printk(" : ");
> +		if (i % 16 == 15)
> +			printk("\n");
> +	}
> +	printk("\n");

printk with loglevels and identifiable origin please

> +
> +
> +static s64 dir_seek_data(struct inode *inode, s64 pos)
> +{
> +	s64 new_pos = logfs_seek_data(inode, pos);

new line

> +	return max((s64)pos, new_pos - 1);
> +}
> +
> +
> +static int __logfsck_dirs(struct inode *dir)
> +{
> +	struct inode *inode;
> +	loff_t pos;
> +	u64 ino;
> +	u8 type;
> +	int cookie, err, ret = 0;
> +
> +	for (pos=0; ; pos++) {
> +		err = read_one_dd(dir, pos, &ino, &type);
> +		//yield();

	great. cond_resched() if you really need to

> +		if (err == -ENODATA) { /* dentry was deleted */
> +			pos = dir_seek_data(dir, pos);
> +			continue;
> +		}
> +		if (err == -EOF)
> +			break;
> +		if (err)
> +			goto error0;
> +
> +		err = -EIO;
> +		if (ino > last_ino) {
> +			printk("ino %llx > last_ino %llx\n", ino, last_ino);

	loglevel .....

> +			goto error0;
> +		}
> +		inode = logfs_iget(dir->i_sb, ino, &cookie);
> +		if (!inode) {
> +			printk("Could not find inode #%llx\n", ino);
> +			goto error0;
> +		}
> +		if (type != logfs_type(inode)) {
> +			printk("dd type %x != inode type %x\n", type,
> +					logfs_type(inode));

	dito

> +			goto error1;
> +		}
> +		inode_links[ino]++;
> +		err = 0;
> +		if (type == DT_DIR) {
> +			inode_links[dir->i_ino]++;
> +			inode_links[ino]++;
> +			err = __logfsck_dirs(inode);
> +		}
> +error1:
> +		logfs_iput(inode, cookie);
> +error0:
> +		if (!ret)
> +			ret = err;
> +		continue;
> +	}
> +	return 1;
> +}
> +
> +
> +/**
> + * Pass 3: inodes
> + */
> +
> +
> +static int logfs_check_inode(struct inode *inode)
> +{
> +	struct logfs_inode *li = LOGFS_INODE(inode);
> +	u64 bytes0 = li->li_used_bytes;
> +	u64 bytes1 = inode_bytes[inode->i_ino];
> +	u64 links0 = inode->i_nlink;
> +	u64 links1 = inode_links[inode->i_ino];
> +
> +	if (bytes0 || bytes1 || links0 || links1
> +			|| inode->i_ino == LOGFS_SUPER(inode->i_sb)->s_last_ino)
> +		printk("%lx: %llx(%llx) bytes, %llx(%llx) links\n",
> +				inode->i_ino, bytes0, bytes1, links0, links1);

	Sigh

> +	used_bytes += bytes0;
> +	return (bytes0 == bytes1) && (links0 == links1);
> +}
> +
> +
> +static int logfs_check_ino(struct super_block *sb, u64 ino)
> +{
> +	struct inode *inode;
> +	int ret, cookie;
> +
> +	//yield();

	See above instance of //yield();

> +	inode = logfs_iget(sb, ino, &cookie);
> +	if (!inode)
> +		return 1;
> +	ret = logfs_check_inode(inode);
> +	logfs_iput(inode, cookie);
> +	return ret;
> +}
> +
> +
> +
> +static int logfsck_stats(struct super_block *sb)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	u64 ostore_segs, total, expected;
> +	int i, reserved_segs;
> +
> +	reserved_segs = 1; /* super_block */
> +	journal_for_each(i)
> +		if (super->s_journal_seg[i])
> +			reserved_segs++;
> +	reserved_segs += super->s_bad_segments;
> +
> +	ostore_segs = super->s_no_segs - reserved_segs;
> +	expected = ostore_segs << super->s_segshift;
> +	total = free_bytes + used_bytes;
> +
> +	printk("free:%8llx, used:%8llx, total:%8llx",
> +			free_bytes, used_bytes, expected);

	loglevel

> +	if (total > expected)
> +		printk(" + %llx\n", total - expected);
> +	else if (total < expected)
> +		printk(" - %llx\n", expected - total);
> +	else
> +		printk("\n");
> +
> +	return total == expected;
> +}
> +
> +
> +static int __logfs_fsck(struct super_block *sb)
> +{
> +	int ret;
> +	int err = 0;
> +
> +	/* pass 1: check blocks */
> +	logfsck_blocks(sb);
> +	/* pass 2: check directories */
> +	ret = logfsck_dirs(sb);
> +	if (!ret) {
> +		printk("Pass 2: directory check failed\n");

	same

> +		err = -EIO;
> +	}
> +	/* pass 3: check inodes */
> +	ret = logfsck_inodes(sb);
> +	if (!ret) {
> +		printk("Pass 3: inode check failed\n");

	same

> +		err = -EIO;
> +	}
> +	/* Pass 4: Total blocks */
> +	ret = logfsck_stats(sb);
> +	if (!ret) {
> +		printk("Pass 4: statistic check failed\n");

	same

> +		err = -EIO;
> +	}
> +
> +	return err;
> +}
> +
> +
> +int logfs_fsck(struct super_block *sb)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	int ret = -ENOMEM;
> +
> +	used_bytes = 0;
> +	free_bytes = 0;
> +	last_ino = super->s_last_ino;
> +	inode_bytes = kzalloc(last_ino * sizeof(be64), GFP_KERNEL);
> +	if (!inode_bytes)
> +		goto out0;

	return ret;

> +	inode_links = kzalloc(last_ino * sizeof(be64), GFP_KERNEL);
> +	if (!inode_links)
> +		goto out1;
> +
> +	ret = __logfs_fsck(sb);
> +
> +	kfree(inode_links);
> +	inode_links = NULL;
> +out1:
> +	kfree(inode_bytes);
> +	inode_bytes = NULL;
> +out0:
> +	return ret;
> +}
> --- /dev/null	2007-04-18 05:32:26.652341749 +0200
> +++ linux-2.6.21logfs/fs/logfs/Locking	2007-05-07 13:32:12.000000000 +0200
> @@ -0,0 +1,45 @@

Can you move this into documentation please

> --- /dev/null	2007-04-18 05:32:26.652341749 +0200
> +++ linux-2.6.21logfs/fs/logfs/compr.c	2007-05-07 13:32:12.000000000 +0200
> @@ -0,0 +1,198 @@

Comment, license

> +#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_memcpy(void *in, void *out, size_t inlen, size_t outlen)
> +{
> +	if (outlen < inlen)
> +		return -EIO;
> +	memcpy(out, in, inlen);
> +	return inlen;
> +}
> +
> +
> +int logfs_compress_vec(struct kvec *vec, int count, void *out, size_t outlen)
> +{
> +	int i, ret;
> +
> +	mutex_lock(&compr_mutex);
> +	ret = zlib_deflateInit(&stream, COMPR_LEVEL);
> +	if (ret != Z_OK)
> +		goto error;
> +
> +	stream.total_in = 0;
> +	stream.total_out = 0;
> +
> +	for (i=0; i<count-1; i++) {
> +		stream.next_in = vec[i].iov_base;
> +		stream.avail_in = vec[i].iov_len;
> +		stream.next_out = out + stream.total_out;
> +		stream.avail_out = outlen - stream.total_out;
> +
> +		ret = zlib_deflate(&stream, Z_NO_FLUSH);
> +		if (ret != Z_OK)
> +			goto error;
> +		/* if (stream.total_out >= outlen)
> +			goto error; */

	???

> +	}
> +
> +	stream.next_in = vec[count-1].iov_base;
> +	stream.avail_in = vec[count-1].iov_len;
> +	stream.next_out = out + stream.total_out;
> +	stream.avail_out = outlen - stream.total_out;
> +
> +	ret = zlib_deflate(&stream, Z_FINISH);
> +	if (ret != Z_STREAM_END)
> +		goto error;
> +	/* if (stream.total_out >= outlen)
> +		goto error; */

	???

> +	ret = zlib_deflateEnd(&stream);
> +	if (ret != Z_OK)
> +		goto error;
> +
> +	if (stream.total_out >= stream.total_in)
> +		goto error;
> +
> +	ret = stream.total_out;
> +	mutex_unlock(&compr_mutex);
> +	return ret;
> +error:
> +	mutex_unlock(&compr_mutex);
> +	return -EIO;
> +}
> +
> +

> +int logfs_uncompress_vec(void *in, size_t inlen, struct kvec *vec, int count)
> +{
> +	int i, ret;
> +
> +	mutex_lock(&compr_mutex);
> +	ret = zlib_inflateInit(&stream);
> +	if (ret != Z_OK)
> +		goto error;
> +
> +	stream.total_in = 0;
> +	stream.total_out = 0;
> +
> +	for (i=0; i<count-1; i++) {
> +		stream.next_in = in + stream.total_in;
> +		stream.avail_in = inlen - stream.total_in;
> +		stream.next_out = vec[i].iov_base;
> +		stream.avail_out = vec[i].iov_len;
> +
> +		ret = zlib_inflate(&stream, Z_NO_FLUSH);
> +		if (ret != Z_OK)
> +			goto error;
> +	}
> +	stream.next_in = in + stream.total_in;
> +	stream.avail_in = inlen - stream.total_in;
> +	stream.next_out = vec[count-1].iov_base;
> +	stream.avail_out = vec[count-1].iov_len;
> +
> +	ret = zlib_inflate(&stream, Z_FINISH);
> +	if (ret != Z_STREAM_END)
> +		goto error;
> +
> +	ret = zlib_inflateEnd(&stream);
> +	if (ret != Z_OK)
> +		goto error;
> +
> +	mutex_unlock(&compr_mutex);
> +	return ret;
> +error:
> +	mutex_unlock(&compr_mutex);
> +	return -EIO;

	Sigh. Can you please make this a bit more clever  ?

> +}
> +
> +
> +int logfs_uncompress(void *in, void *out, size_t inlen, size_t outlen)
> +{
> +	int ret;
> +
> +	mutex_lock(&compr_mutex);
> +	ret = zlib_inflateInit(&stream);
> +	if (ret != 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;
> +
> +	ret = zlib_inflate(&stream, Z_FINISH);
> +	if (ret != Z_STREAM_END)
> +		goto error;
> +
> +	ret = zlib_inflateEnd(&stream);
> +	if (ret != Z_OK)
> +		goto error;
> +
> +	mutex_unlock(&compr_mutex);
> +	return ret;
> +error:
> +	mutex_unlock(&compr_mutex);
> +	return -EIO;

	Same here

> +}


> +
> +int __init logfs_compr_init(void)
> +{
> +	size_t size = max(zlib_deflate_workspacesize(),
> +			zlib_inflate_workspacesize());
> +	printk("deflate size: %x\n", zlib_deflate_workspacesize());
> +	printk("inflate size: %x\n", zlib_inflate_workspacesize());

	loglevel

> +	stream.workspace = vmalloc(size);
> +	if (!stream.workspace)
> +		return -ENOMEM;
> +	return 0;
> +}
> +
> +void __exit logfs_compr_exit(void)
> +{
> +	vfree(stream.workspace);
> +}
> --- /dev/null	2007-04-18 05:32:26.652341749 +0200
> +++ linux-2.6.21logfs/fs/logfs/segment.c	2007-05-07 20:41:17.000000000 +0200
> @@ -0,0 +1,533 @@

Comment, license

> +#include "logfs.h"
> +
> +
> +
> +#define HEADER_SIZE sizeof(struct logfs_object_header)

empty line

> +s64 __logfs_segment_write(struct inode *inode, void *buf, u64 pos, int level,
> +		int alloc, int len, int compr)
> +{
> +	struct logfs_area *area;
> +	struct super_block *sb = inode->i_sb;
> +	u64 ofs;
> +	u64 ino = inode->i_ino;
> +	int err;
> +	struct logfs_object_header h;
> +
> +	h.crc	= cpu_to_be32(0xcccccccc);
> +	h.len	= cpu_to_be16(len);
> +	h.type	= OBJ_BLOCK;
> +	h.compr	= compr;
> +	h.ino	= cpu_to_be64(inode->i_ino);
> +	h.pos	= cpu_to_be64(pos);
> +
> +	level = adj_level(ino, level);
> +	area = get_area(sb, level);
> +	ofs = __logfs_get_free_bytes(area, ino, pos, len + HEADER_SIZE);
> +	LOGFS_BUG_ON(ofs <= 0, sb);
> +	//printk("alloc: (%llx, %llx, %llx, %x)\n", ino, pos, ret, level);

	clean up

> +	err = buf_write(area, ofs, &h, sizeof(h));
> +	if (!err)
> +		err = buf_write(area, ofs + HEADER_SIZE, buf, len);
> +	BUG_ON(err);
> +	if (err)
> +		return err;
> +	if (alloc) {
> +		int acc_len = (level==0) ? len : sb->s_blocksize;
> +		logfs_consume_bytes(inode, acc_len + HEADER_SIZE);
> +	}
> +
> +	logfs_close_area(area); /* FIXME merge with open_area */
> +
> +	//printk("    (%llx, %llx, %llx)\n", ofs, ino, pos);

	same

> +	return ofs;
> +}
> +
> +
> +
> +
> +int wbuf_read(struct super_block *sb, u64 ofs, size_t len, void *buf)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	struct logfs_area *area;
> +	u32 segno = ofs >> super->s_segshift;
> +	int i, err;
> +
> +	err = mtdread(sb, ofs, len, buf);
> +	if (err)
> +		return err;
> +
> +	for (i=0; i<LOGFS_NO_AREAS; i++) {

	i = 0; ...

> +		area = super->s_area[i];
> +		if (area->a_segno == segno) {
> +			fixup_from_wbuf(sb, area, buf, ofs, len);
> +			break;
> +		}
> +	}
> +	return 0;
> +}
> +
> +
> +int logfs_segment_read(struct super_block *sb, void *buf, u64 ofs)
> +{
> +	struct logfs_object_header *h;
> +	u16 len;
> +	int err, bs = sb->s_blocksize;
> +
> +	mutex_lock(&compr_mutex);
> +	err = wbuf_read(sb, ofs, bs+24, compressor_buf);
> +	if (err)
> +		goto out;
> +	h = (void*)compressor_buf;


	please use proper typecasts

> +	len = be16_to_cpu(h->len);
> +
> +	switch (h->compr) {
> +	case COMPR_NONE:
> +		logfs_memcpy(compressor_buf+24, buf, bs, bs);
> +		break;
> +	case COMPR_ZLIB:
> +		err = logfs_uncompress(compressor_buf+24, buf, len, bs);
> +		BUG_ON(err);
> +		break;
> +	default:
> +		LOGFS_BUG(sb);
> +	}
> +out:
> +	mutex_unlock(&compr_mutex);
> +	return err;
> +}
> +
> +
> +static u64 logfs_block_mask[] = {
> +	~0,
> +	~(I1_BLOCKS-1),
> +	~(I2_BLOCKS-1),
> +	~(I3_BLOCKS-1)
> +};

Empty line please

> +static int check_pos(struct super_block *sb, u64 pos1, u64 pos2, int level)
> +{
> +	LOGFS_BUG_ON(	(pos1 & logfs_block_mask[level]) !=
> +			(pos2 & logfs_block_mask[level]), sb);
> +}

empty line

> +int logfs_segment_delete(struct inode *inode, u64 ofs, u64 pos, int level)
> +{
> +	struct super_block *sb = inode->i_sb;
> +	struct logfs_object_header *h;
> +	u16 len;
> +	int err;
> +
> +
> +	mutex_lock(&compr_mutex);
> +	err = wbuf_read(sb, ofs, 4096+24, compressor_buf);
> +	LOGFS_BUG_ON(err, sb);
> +	h = (void*)compressor_buf;

	proper typecast

> +	len = be16_to_cpu(h->len);
> +	check_pos(sb, pos, be64_to_cpu(h->pos), level);
> +	mutex_unlock(&compr_mutex);
> +
> +	level = adj_level(inode->i_ino, level);
> +	len = (level==0) ? len : sb->s_blocksize;
> +	logfs_remove_bytes(inode, len + sizeof(*h));
> +	return 0;
> +}
> +
> +
> +int logfs_open_area(struct logfs_area *area)
> +{
> +	if (area->a_is_open)
> +		return 0; /* nothing to do */

	yeah, another really helpful comment

> +	area->a_ops->get_free_segment(area);
> +	area->a_used_objects = 0;
> +	area->a_used_bytes = 0;
> +	area->a_ops->get_erase_count(area);
> +
> +	area->a_ops->clear_blocks(area);
> +	area->a_is_open = 1;
> +
> +	return area->a_ops->erase_segment(area);
> +}
> +

> +static void ostore_get_free_segment(struct logfs_area *area)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(area->a_sb);
> +	struct logfs_segment *seg;
> +
> +	BUG_ON(list_empty(&super->s_free_list));
> +
> +	seg = list_entry(super->s_free_list.prev, struct logfs_segment, list);
> +	list_del(&seg->list);
> +	area->a_segno = seg->segno;
> +	kfree(seg);
> +	super->s_free_count -= 1;

get_free_segment actually kfree's a segment ? Please use a less
misleading function name

> +}
> +
> +
> +static void ostore_get_erase_count(struct logfs_area *area)
> +{
> +	struct logfs_segment_header h;
> +
> +	device_read(area->a_sb, area->a_segno, 0, sizeof(h), &h);

	error handling

> +	area->a_erase_count = be32_to_cpu(h.ec) + 1;
> +}
> +
> +
> +
> +static int ostore_erase_segment(struct logfs_area *area)
> +{
> +	struct logfs_segment_header h;
> +	u64 ofs;
> +	int err;
> +
> +	err = logfs_erase_segment(area->a_sb, area->a_segno);
> +	if (err)
> +		return err;
> +
> +	h.len = 0;
> +	h.type = OBJ_OSTORE;
> +	h.level = area->a_level;
> +	h.segno = cpu_to_be32(area->a_segno);
> +	h.ec = cpu_to_be32(area->a_erase_count);
> +	h.gec = cpu_to_be64(LOGFS_SUPER(area->a_sb)->s_gec);
> +	h.crc = logfs_crc32(&h, sizeof(h), 4);
> +	/* FIXME: write it out */

	isn't that what buf_write() does ?

> +	ofs = dev_ofs(area->a_sb, area->a_segno, 0);
> +	area->a_used_bytes = sizeof(h);
> +	return buf_write(area, ofs, &h, sizeof(h));
> +}
> +
> +
> +static void flush_buf(struct logfs_area *area)
> +{
> +	struct super_block *sb = area->a_sb;
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	u32 used, free;
> +	u64 ofs;
> +	u32 writemask = super->s_writesize - 1;
> +	int err;
> +
> +	ofs = dev_ofs(sb, area->a_segno, area->a_used_bytes);
> +	ofs &= ~writemask;
> +	used = area->a_used_bytes & writemask;
> +	free = super->s_writesize - area->a_used_bytes;
> +	free &= writemask;
> +	//printk("flush(%llx, %x, %x)\n", ofs, used, free);

	sigh

> +	if (used == 0)
> +		return;
> +
> +	TRACE();

	sigh more

> +	memset(area->a_wbuf + used, 0xff, free);
> +	err = mtdwrite(sb, ofs, super->s_writesize, area->a_wbuf);
> +	LOGFS_BUG_ON(err, sb);
> +}
> +

> +
> +
> +int logfs_init_areas(struct super_block *sb)
> +{
> +	struct logfs_super *super = LOGFS_SUPER(sb);
> +	int i;
> +
> +	super->s_journal_area = kzalloc(sizeof(struct logfs_area), GFP_KERNEL);
> +	if (!super->s_journal_area)
> +		return -ENOMEM;
> +	super->s_journal_area->a_sb = sb;
> +
> +	for (i=0; i<LOGFS_NO_AREAS; i++) {
	i = 0; ..

> +		super->s_area[i] = init_ostore_area(sb, i);
> +		if (!super->s_area[i])
> +			goto err;
> +	}
> +	return 0;
> +
> +err:
> +	for (i--; i>=0; i--)

	same here

> +		cleanup_ostore_area(super->s_area[i]);
> +	kfree(super->s_journal_area);
> +	return -ENOMEM;
> +}
> +
> +
> +void logfs_cleanup_areas(struct logfs_super *super)
> +{
> +	int i;
> +
> +	for (i=0; i<LOGFS_NO_AREAS; i++)

	adnd here

> +		cleanup_ostore_area(super->s_area[i]);
> +	kfree(super->s_journal_area);
> +}
> --- /dev/null	2007-04-18 05:32:26.652341749 +0200
> +++ linux-2.6.21logfs/fs/logfs/memtree.c	2007-05-07 13:32:12.000000000 +0200
> @@ -0,0 +1,199 @@
> +/* In-memory B+Tree. */

license and a little bit more description

> +#include "logfs.h"
> +
> +#define BTREE_NODES 16	/* 32bit, 128 byte cacheline */
> +//#define BTREE_NODES 8	/* 32bit,  64 byte cacheline */

Please cleanup

> +void *btree_lookup(struct btree_head *head, long val)
> +{
> +	int i, height = head->height;
> +	struct btree_node *node = head->node;
> +
> +	if (val == 0)
> +		return head->null_ptr;
> +
> +	if (height == 0)
> +		return NULL;
> +
> +	for ( ; height > 1; height--) {
> +		for (i=0; i<BTREE_NODES; i++)
> +			if (node[i].val <= val)
> +				break;
> +		node = node[i].node;
> +	}
> +
> +	for (i=0; i<BTREE_NODES; i++)

	i = 0; ...

> +		if (node[i].val == val)
> +			return node[i].node;
> +
> +	return NULL;
> +}
> +
> +
> +static void find_pos(struct btree_node *node, long val, int *pos, int *fill)
> +{
> +	int i;
> +
> +	for (i=0; i<BTREE_NODES; i++)
	
same

> +		if (node[i].val <= val)
> +			break;
> +	*pos = i;
> +	for (i=*pos; i<BTREE_NODES; i++)

same

> +		if (node[i].val == 0)
> +			break;
> +	*fill = i;
> +}
> +
> +
> +static struct btree_node *find_level(struct btree_head *head, long val,
> +		int level)
> +{
> +	struct btree_node *node = head->node;
> +	int i, height = head->height;
> +
> +	for ( ; height > level; height--) {
> +		for (i=0; i<BTREE_NODES; i++)

same

> +			if (node[i].val <= val)
> +				break;
> +		node = node[i].node;
> +	}
> +	return node;
> +}
> +
> +
> +
> +static int btree_remove_level(struct btree_head *head, long val, int level)
> +{
> +	struct btree_node *node;
> +	int i, pos, fill;
> +
> +	if (val == 0) { /* 0 identifies empty slots, so special-case this */
> +		head->null_ptr = NULL;
> +		return 0;
> +	}
> +
> +	node = find_level(head, val, level);
> +	find_pos(node, val, &pos, &fill);
> +	if (level == 1)
> +		BUG_ON(node[pos].val != val);
> +
> +	/* remove and shift */
> +	for (i=pos; i<fill-1; i++) {
> +		node[i].val = node[i+1].val;
> +		node[i].node = node[i+1].node;
> +	}
> +	node[fill-1].val = 0;
> +	node[fill-1].node = NULL;
> +
> +	if (fill-1 < BTREE_NODES/2) {
> +		/* XXX */

	YYYY perhaps ?

> +	}
> +	if (fill-1 == 0) {
> +		btree_remove_level(head, val, level+1);
> +		kfree(node);
> +		return 0;
> +	}
> +
> +	return 0;
> +}


-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux