xfs
[Top] [All Lists]

Re: [PATCH 01/11] xfs: reshuffle dir2 headers

To: Alex Elder <aelder@xxxxxxx>
Subject: Re: [PATCH 01/11] xfs: reshuffle dir2 headers
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Tue, 12 Jul 2011 05:06:09 -0400
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <1310423560.7019.53.camel@doink>
References: <20110710204916.856267100@xxxxxxxxxxxxxxxxxxxxxx> <20110710205016.890382263@xxxxxxxxxxxxxxxxxxxxxx> <1310423560.7019.53.camel@doink>
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Jul 11, 2011 at 05:32:40PM -0500, Alex Elder wrote:
> - 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?

It's more or less intentional.  If actually had non-opaque data types
that weren't part of the disk format we might have them here, but the
dir2 code doesn't have any of those.

> - 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.  

The only type not part of the disk format is xfs_dir2_data_aoff_t,
which is a type that we probably should get rid of anyway.

> - 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.

We haven't done that for any of the XFS headers, so I don't
plan to change it with this patch.

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

These lines were taken as-is from the old headers, but your variants
are beter, I'll change it.

> > +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).

We can fix this up later, but I don't want to change identifiers in
addition to the header consolidation.

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

Ok.

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

It doesn't seem overly useful, so I'll remove it.

> 
> > + *
> > + * 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?

I'll remove it.

<Prev in Thread] Current Thread [Next in Thread>