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@xxxxxx>
> > >
> > > 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@xxxxxxx>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
