Re: [PATCH 13/19] xfs: vectorise directory data operations

From: Dave Chinner <david@fromorbit.com>
To: Ben Myers <bpm@sgi.com>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 13/19] xfs: vectorise directory data operations
Date: Mon, 28 Oct 2013 10:01:41 +1100	[thread overview]
Message-ID: <20131027230141.GC2797@dastard> (raw)
In-Reply-To: <20131024222811.GA10553@sgi.com>

On Thu, Oct 24, 2013 at 05:28:11PM -0500, Ben Myers wrote:
> On Fri, Oct 25, 2013 at 09:08:44AM +1100, Dave Chinner wrote:
> > On Thu, Oct 24, 2013 at 04:41:12PM -0500, Ben Myers wrote:
> > > On Fri, Oct 25, 2013 at 08:31:17AM +1100, Dave Chinner wrote:
> > > > On Thu, Oct 24, 2013 at 01:39:09PM -0500, Ben Myers wrote:
> > > > > On Tue, Oct 15, 2013 at 09:17:59AM +1100, Dave Chinner wrote:
> > > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > > > 
> > > > > > Following from the initial patches to vectorise the shortform
> > > > > > directory encode/decode operations, convert half the data block
> > > > > > operations to use the vector. The rest will be done in a second
> > > > > > patch.
> > > > > > 
> > > > > > This further reduces the size of the built binary:
> > > > > > 
> > > > > >    text    data     bss     dec     hex filename
> > > > > >  794490   96802    1096  892388   d9de4 fs/xfs/xfs.o.orig
> > > > > >  792986   96802    1096  890884   d9804 fs/xfs/xfs.o.p1
> > > > > >  792350   96802    1096  890248   d9588 fs/xfs/xfs.o.p2
> > > > > >  789293   96802    1096  887191   d8997 fs/xfs/xfs.o.p3
> > > > > > 
> > > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > > > 
> > > > > Generally looks pretty good, I have a question below...
> > > > > 
> > > > > >  const struct xfs_dir_ops xfs_dir2_ftype_ops = {
> > > > > > @@ -223,6 +415,18 @@ const struct xfs_dir_ops xfs_dir2_ftype_ops = {
> > > > > >  	.sf_put_ino = xfs_dir3_sfe_put_ino,
> > > > > >  	.sf_get_parent_ino = xfs_dir2_sf_get_parent_ino,
> > > > > >  	.sf_put_parent_ino = xfs_dir2_sf_put_parent_ino,
> > > > > > +
> > > > > > +	.data_entsize = xfs_dir3_data_entsize,
> > > > > > +	.data_get_ftype = xfs_dir3_data_get_ftype,
> > > > > > +	.data_put_ftype = xfs_dir3_data_put_ftype,
> > > > > > +	.data_entry_tag_p = xfs_dir3_data_entry_tag_p,
> > > > > > +
> > > > > > +	.data_dot_offset = xfs_dir2_data_dot_offset,
> > > > > > +	.data_dotdot_offset = xfs_dir2_data_dotdot_offset,
> > > > > > +	.data_first_offset = xfs_dir2_data_first_offset,
> > > > > > +	.data_dot_entry_p = xfs_dir2_data_dot_entry_p,
> > > > > > +	.data_dotdot_entry_p = xfs_dir2_data_dotdot_entry_p,
> > > > > > +	.data_first_entry_p = xfs_dir2_data_first_entry_p,
> > > > > >  };
> > > > > 
> > > > > I think there may be a problem here.  Although the dirv2 functions for
> > > > > ., .., and first entry offset account for the v2 header size, they
> > > > > appear not to be accounting for the modified entry size due to the file
> > > > > type field.  Am I missing something?
> > > > 
> > > > The ftype field is handled by the alignment roundup. i.e. namelen is
> > > > 1 or two bytes, plus ftype is 2 or 3 bytes, roundup is to 8 bytes.
> > > > Hence adding a byte for the ftype field is not a problem for these
> > > > first entries because of their small, fixed size.
> > 
> > I should point out that this code is functionally identical to the
> > way the original macros treated the v4 ftype code. You reviewed that
> > code and tested it and it as such this implicit use of padding was
> > considered perfectly OK just a couple of months ago.....
> 
> It is a detail that I overlooked.  I try to do a thorough review... but
> sometimes things don't register.
> 
> > > It should either be explicitly correct (and I think it is today), or we
> > > need a comment to explain why it's not.  I would prefer the former.
> > 
> > Well, I'll add a patch at the end of the series to change it. I don't want to
> > have to rebase the rest of the patches in the series just because of the
> > don't apply because of context mismatches.
> 
> Sounds great.
> 
> > 
> > Besides, the last patch in the series it replaces the offset functions with
> > precalculated values. That replacement fixes the offset calculation to
> > explicitly use dir2 hdrs and dir3 entsizes, so the problem goes away for
> > those entries.
> 
> Ok, I'll look out for it then.

Given you found a few minor problems in the other patches (e.g.
hunks in the wrong patches) I'll fix up all the patches and resend
the series with all the updated tags on it...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs