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    [W:0.026 / U:90.796 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site