lkml.org 
[lkml]   [2008]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [patch 2/15] fs/logfs/logfs_abi.h
Date
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 <><
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2008-04-08 02:27    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site