Re: [PATCH 8/9] xfs: introduce new internal log vector structure

[Dave Chinner <david@xxxxxxxxxxxxx>]

On Sat, Mar 06, 2010 at 10:46:10AM -0500, Christoph Hellwig wrote:
> On Sat, Mar 06, 2010 at 12:51:23PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > The current log IO vector structure is a flat array and not
> > extensible. To make it possible to keep separate log IO vectors for
> > individual log items, we need a method of chaining log IO vectors
> > together.
> > 
> > Introduce a new log vector type that can be used to wrap the
> > existing log IO vectors on use that internally to the log. This
> > means that the existing external interface (xfs_log_write) does not
> > change and hence no changes to the transaction commit code are
> > required.
> > 
> > This initial use of the new log vectors does not use the chaining
> > capability of the new log vector structure - it is not needed to
> > implement the flat vector array the current transaction commit path
> > creates.
> 
> Looks good to me,
> 
> 
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> 
> A few comments below:
> 
> > +                   /* the data section must be 32 bit size aligned */
> > +                   struct {
> > +                       __uint16_t magic;
> > +                       __uint16_t pad1;
> > +                       __uint32_t pad2; /* may as well make it 64 bits */
> > +                   } magic = {
> > +                           .magic = XLOG_UNMOUNT_TYPE,
> > +                   };
> > +                   struct xfs_log_iovec reg = {
> > +                           .i_addr = (void *)&magic,
> > +                           .i_len = sizeof(magic),
> > +                           .i_type = XLOG_REG_TYPE_UNMOUNT,
> > +                   };
> > +                   struct xfs_log_vec vec = {
> > +                           .lv_niovecs = 1,
> > +                   };
> > +                   /* sigh. c99 initialisers don't work on anon unions */
> > +                   vec.lv_iovecp = &reg;
> 
> I can't see an anonymous union involved anywhere here, and initializing
> these normally works just fine (see patch below).

Ah, I killed that union for this patch set - I was using embedded
arrays to cut down on memory allocations for the delayed logging so
there was a:

        union {
                struct xfs_log_iovec    *lv_iovecp;
                struct xfs_log_iovec    lv_iovec[0];
        };

in the structure. I decided that the union wasn't necessary for
these patches (and can be avoided entirely, anyway) but I forgot to
fix up the setup of the structure to remove those comments. I'll
update it as you've suggested.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx