Re: [PATCH 06/11] xfs: fully initialise temp leaf in xfs_attr3_leaf_unbalance

[Ben Myers <bpm@sgi.com>]

On Thu, May 23, 2013 at 09:54:50AM +1000, Dave Chinner wrote:
> On Wed, May 22, 2013 at 03:50:59PM -0500, Ben Myers wrote:
> > On Tue, May 21, 2013 at 06:02:05PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > xfs_attr3_leaf_unbalance() uses a temporary buffer for recombining
> > > the entries in two leaves when the destination leaf requires
> > > compaction. The temporary buffer ends up being copied back over the
> > > original destination buffer, so the header in the temporary buffer
> > > needs to contain all the information that is in the destination
> > > buffer.
> > > 
> > > To make sure the temporary buffer is fully initialised, once we've
> > > set up the temporary incore header appropriately, write is back to
> > > the temporary buffer before starting to move entries around.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > >
> > > ---
> > >  fs/xfs/xfs_attr_leaf.c |   16 +++++++++++++---
> > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_attr_leaf.c b/fs/xfs/xfs_attr_leaf.c
> > > index 0bce1b3..79ece72 100644
> > > --- a/fs/xfs/xfs_attr_leaf.c
> > > +++ b/fs/xfs/xfs_attr_leaf.c
> > > @@ -2181,14 +2181,24 @@ xfs_attr3_leaf_unbalance(
> > >  		struct xfs_attr_leafblock *tmp_leaf;
> > >  		struct xfs_attr3_icleaf_hdr tmphdr;
> > >  
> > > -		tmp_leaf = kmem_alloc(state->blocksize, KM_SLEEP);
> > > -		memset(tmp_leaf, 0, state->blocksize);
> > > -		memset(&tmphdr, 0, sizeof(tmphdr));
> > > +		tmp_leaf = kmem_zalloc(state->blocksize, KM_SLEEP);
> > > +
> > > +		/*
> > > +		 * Copy the header into the temp leaf so that all the stuff
> > > +		 * not in the incore header is present and gets copied back in
> > > +		 * once we've moved all the entries.
> > > +		 */
> > > +		memcpy(tmp_leaf, save_leaf, xfs_attr3_leaf_hdr_size(save_leaf));
> > >  
> > > +		memset(&tmphdr, 0, sizeof(tmphdr));
> > >  		tmphdr.magic = savehdr.magic;
> > >  		tmphdr.forw = savehdr.forw;
> > >  		tmphdr.back = savehdr.back;
> > >  		tmphdr.firstused = state->blocksize;
> > > +
> > > +		/* write the header to the temp buffer to initialise it */
> > > +		xfs_attr3_leaf_hdr_to_disk(tmp_leaf, &tmphdr);
> > > +
> > 
> > Hrm.  It seemed like the memset 0 for the incore temp header should have been
> > enough for the purposes of moveents.  Why did you have to add this additional
> > hdr_to_disk?
> 
> Because we've modified the tmphdr so it is different to what is in
> the tmp_leaf, and I'm being paranoid and not making any assumptions
> that a callee doesn't reread the tmphdr from the tmp_leaf.
> 
> i.e. I'm initialising both the on-disk and the incore headers to
> be identical before we do anything else. Once bitten, twice shy.

Reviewed-by: Ben Myers <bpm@sgi.com>

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