On Sun, 2011-07-10 at 16:49 -0400, Christoph Hellwig wrote:
> Replace the current mess of dir2 headers with just three that have a clear
> purpose:
>
> - xfs_dir2_format.h for all format defintions, including the inline helpers
definitions
> to access our variable size structures
> - xfs_dir2_priv.h for all prototypes that are internal to the dir2 code
> and no needed by anything outside of the directory code. For this
not
> purpose xfs_da_btree.c, and phase6.c in xfs_repair are considered part
> of the directory code.
> - xfs_dir2.h for the public interface to the directory code
>
> In addition to the reshuffle I have also update the comments to not only
> match the new file structure, but also to describe the directory format
> better.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
OK, I looked over this probably more closely than necessary.
In any case, it all looks good, but I'll mention just a few
things, but really they're mostly curiosity and observations
rather than anything all that pressing.
- It looks like the three files are:
- external interface (just function prototypes)
- internal interface (just function prototypes)
- structures and accessors, other types, and constants
Did it just happen to turn out that that the two interface
files had nothing but prototypes? Or was that what you
intended to do?
- The contents of "xfs_dir2_format.h" comprise more than
just the on-disk format, it really seems to capture all
substantive data types and definitions related to directory
structures in XFS.
- None of the dir2 header files themselves #included
anything else. The same is true for your (new) three
header files. However, the internal interface file
(xfs_dir2_priv.h) seems to *always* require the data
types file (xfs_dir2_format.h) to be included first.
What are your thoughts about just putting the #include
of "xfs_dir2_format.h" into "xfs_dir2_priv.h". I
realize that's really a philosophical question, broader
than this particular case.
A few other details, below. I'll look for whatever your
response is, but I found no real problems and I trust
you'll do what you think is best with my suggestions here.
Reviewed-by: Alex Elder <aelder@xxxxxxx>
. . .
> Index: xfs/fs/xfs/xfs_dir2_format.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ xfs/fs/xfs/xfs_dir2_format.h 2011-07-09 13:29:33.488251514 +0200
> @@ -0,0 +1,602 @@
> +/*
> + * Copyright (c) 2000-2001,2005 Silicon Graphics, Inc.
. . .
> + * Directory version 2.
> + *
> + * There are 4 possible formats:
> + * - shortform - embedded into the inode
> + * - single block - data with embedded leaf at the end
> + * - multiple data blocks, single leaf+freeindex block
> + * - data blocks, node and leaf blocks (btree), freeindex blocks
> + *
> + * Note: many node blocks structures and constants are shared with the attributes
> + * code and defined in xfs_da_btree.h.
> + */
> +
> +#define XFS_DIR2_BLOCK_MAGIC 0x58443242 /* XD2B: for one block dirs */
/* XD2B: for single block dirs */
> +#define XFS_DIR2_DATA_MAGIC 0x58443244 /* XD2D: for multiblock dirs */
> +#define XFS_DIR2_FREE_MAGIC 0x58443246 /* XD2F */
/* XD2F: for free index blocks */
> +
> +/*
> + * Byte offset in data block and shortform entry.
> + */
> +typedef __uint16_t xfs_dir2_data_off_t;
. . .
> +
> +typedef union {
> + xfs_dir2_ino8_t i8;
> + xfs_dir2_ino4_t i4;
> +} xfs_dir2_inou_t;
> +#define XFS_DIR2_MAX_SHORT_INUM ((xfs_ino_t)0xffffffffULL)
I know this is historical, but I don't like the use of
"SHORT" here to mean "4-byte," since "short" in the
context of directories has a very different meaning
(i.e., shortform).
> +
> +/*
> + * Directory layout when stored internal to an inode.
> + *
> + * Small directories are packed as tightly as possible so as to fit into the
> + * literal area of the inode. They consist of a single xfs_dir2_sf_hdr header
...of the inode. These "shortform" directories consist...
> + * followed by zero or more xfs_dir2_sf_entry structures. Due the different
> + * inode number storage size and the variable length name field in
> + * the xfs_dir2_sf_entry all these structure are variable length, and the
> + * accessors in this file should be used to iterate over them.
> + *
> + *
> + * The parent directory has a dedicated field, and the self-pointer must
> + * be calculated on the fly.
This sentence is not very meaningful standing by itself here.
I think it either needs a bit more context, or maybe it can
just be removed.
> + *
> + * Entries are packed toward the top as tightly as possible, and thus may
> + * be misaligned. Care needs to be taken to access them through special
> + * helpers or copy them into aligned variables first.
Can this last sentence just be deleted, since above you
now say that the accessors here should be used?
> + */
> +typedef struct xfs_dir2_sf_hdr {
> + __uint8_t count; /* count of entries */
> + __uint8_t i8count; /* count of 8-byte inode #s */
> + xfs_dir2_inou_t parent; /* parent dir inode number */
> +} __arch_pack xfs_dir2_sf_hdr_t;
. . .
> + * | xfs_dir2_data_entry_t OR xfs_dir2_data_unused_t |
> + * | xfs_dir2_data_entry_t OR xfs_dir2_data_unused_t |
> + * | ... |
> + * +-------------------------------------------------+
> + * | unused space |
> + * +-------------------------------------------------+
> + *
> + * As all the entries are variable size structures the accessors below should
> + * be used to iterate over them.
> + *
> + * In addition to the pure data blocks for the data and node formats it,
and node formats,
> + * most structures are also used for the combined data/freespace "block"
. . .
> + * +---------------------------+
> + * | xfs_dir2_leaf_tail_t |
> + * +---------------------------+
> + *
> + * The xfs_dir2_data_off_t members (bests) and tail are at the end of the block
> + * for single-leaf (magic = XFS_DIR2_LEAF1_MAGIC) blocks only, but not present
> + * for directories with separate leaf nodes and free space blocks
> + * (magic = XFS_DIR2_LEAFN_MAGIC).
This was nicely reworded. Not a lot different, but better.
. . .
> + * Leaf block.
> + */
> +typedef struct xfs_dir2_leaf {
> + xfs_dir2_leaf_hdr_t hdr; /* leaf header */
> + xfs_dir2_leaf_entry_t ents[]; /* entries */
^^^ insert a tab here
> +} xfs_dir2_leaf_t;
> +
. . .
That's it.
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
