Re: [patch 2/15] fs/logfs/logfs_abi.h

[Arnd Bergmann <arnd@arndb.de>]

Hi Jörn,

Great to see a new version finally posted again!

A few things that I spotted looking through this header,
I hope I can find some time to look throught the other files
as well.

On Tuesday 01 April 2008, joern@logfs.org wrote:
> --- /dev/null	2008-04-02 16:29:12.813336657 +0200
> +++ linux-2.6.24logfs/fs/logfs/logfs_abi.h	2008-04-01 21:02:34.980239877 +0200
> @@ -0,0 +1,523 @@
> +/*
> + * fs/logfs/logfs.h

The comment doesn't match the file name, and the file name doesn't
match the purpose -- you are not defining an "application" binary
interface but rather the medium format, with the small exception
of the chattr flags.

> +#ifndef fs_logfs_logfs_abi_h
> +#define fs_logfs_logfs_abi_h

Everyone else uses capital letters for these.

> +/*
> + * LOGFS_HEADERSIZE is the size of a single header in the object store,
> + * LOGFS_MAX_OBJECTSIZE the size of the largest possible object, including
> + * its header,
> + * LOGFS_SEGMENT_RESERVE is the amount of space reserved for each segment for
> + * its segment header and the padded space at the end when no further objects
> + * fit.
> + */
> +#define LOGFS_HEADERSIZE	(0x1c)
> +#define LOGFS_SEGMENT_HEADERSIZE (0x18)
> +#define LOGFS_MAX_OBJECTSIZE	(LOGFS_HEADERSIZE + LOGFS_BLOCKSIZE)
> +#define LOGFS_SEGMENT_RESERVE	(LOGFS_HEADERSIZE + LOGFS_MAX_OBJECTSIZE - 1)

The comment makes it sound like the last line should be

#define LOGFS_SEGMENT_RESERVE (LOGFS_SEGMENT_HEADERSIZE + LOGFS_MAX_OBJECTSIZE - 1)

> +/**
> + * struct logfs_disk_dentry - on-medium dentry structure
> + *
> + * @ino:			inode number
> + * @namelen:			length of file name
> + * @type:			file type, identical to bits 12..15 of mode
> + * @name:			file name
> + */
> +struct logfs_disk_dentry {
> +	__be64	ino;
> +	__be16	namelen;
> +	__u8	type;
> +	__u8	name[LOGFS_MAX_NAMELEN];
> +} __attribute__((packed));

With LOGFS_MAX_NAMELEN == 255, this data structure is not aligned to 64
bits, which means that accessing the inode number requires unaligned
memory accesses when you have an array of logfs_disk_dentry structures
on the medium. Is that intentional?

If you add another 32 bits here, you don't need the __packed any more.

> +/**
> + * struct logfs_object_header - per-object header in the ostore
> + *
> + * @crc:			crc32 of header, excluding data_crc
> + * @len:			length of data
> + * @type:			object type, see above
> + * @compr:			compression type
> + * @ino:			inode number
> + * @bix:			block index
> + * @data_crc:			crc32 of payload
> + */
> +struct logfs_object_header {
> +	__be32	crc;
> +	__be16	len;
> +	__u8	type;
> +	__u8	compr;
> +	__be64	ino;
> +	__be64	bix;
> +	__be32	data_crc;
> +} __attribute__((packed));

Similarly, this structure contains 8 byte members but has a smaller
size.

	Arnd <><