[PATCH 4/6] xfs: swalloc doesn't align allocations properly

[Ben Myers]

On Tue, Dec 17, 2013 at 02:39:41PM +1100, Dave Chinner wrote:
> On Mon, Dec 16, 2013 at 05:14:14PM -0600, Ben Myers wrote:
> > On Fri, Dec 13, 2013 at 04:01:23AM -0800, Christoph Hellwig wrote:
> > > Looks good.
> > > 
> > > Reviewed-by: Christoph Hellwig <hch at lst.de>
> > > 
> > > Two very minor nitpicks below:
> > > 
> > > > +	int		stripe_align;
> > > >  
> > > >  	ASSERT(ap->length);
> > > >  
> > > >  	mp = ap->ip->i_mount;
> > > > +
> > > > +	/* stripe alignment for allocation is determined by mount parameters */
> > > > +	stripe_align = 0;
> > > > +	if (mp->m_swidth && (mp->m_flags & XFS_MOUNT_SWALLOC))
> > > > +		stripe_align = mp->m_swidth;
> > > > +	else if (mp->m_dalign)
> > > > +		stripe_align = mp->m_dalign;
> > > 
> > > nipick: I'd either initialize the variable to zero at the point of the
> > > declaration or do if .. else if .. else here.
> > > 
> > > >  	}
> > > > +
> > > > +
> > > >  	nullfb = *ap->firstblock == NULLFSBLOCK;
> > > 
> > > Two newlines seem odd here.  I'd support one even if that's an unrelated
> > > change :)
> > 
> > This is probably not the right thing to do for small files.  They will all end
> > up in the first stripe unit.
> 
> You're right, it's not the right thing to do for small files. And we
> don't, because the ap->aeof that triggers aligned allocation only
> when:
> 
> 	/*
> 	 * Only want to do the alignment at the eof if it is userdata and
> 	 * allocation length is larger than a stripe unit.
> 	 */
> 	if (mp->m_dalign && bma->length >= mp->m_dalign &&
> 	    !(bma->flags & XFS_BMAPI_METADATA) && whichfork == XFS_DATA_FORK) {
> 		error = xfs_bmap_isaeof(bma, whichfork);
> 		if (error)
> 			return error;
> 	}
> 
> The requested allocation length is greater than the stripe unit that
> is configured.
> 
> So, we never align small files, regardless of the mount option....

That addresses my concerns, thanks.  ;)

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