xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 4/6] xfs: swalloc doesn't align allocations properly
From: Ben Myers <bpm@xxxxxxx>
Date: Tue, 17 Dec 2013 08:59:22 -0600
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131217033941.GC31386@dastard>
References: <1386826478-13846-1-git-send-email-david@xxxxxxxxxxxxx> <1386826478-13846-5-git-send-email-david@xxxxxxxxxxxxx> <20131213120123.GA32749@xxxxxxxxxxxxx> <20131216231414.GQ1935@xxxxxxx> <20131217033941.GC31386@dastard>
User-agent: Mutt/1.5.20 (2009-06-14)
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>

<Prev in Thread] Current Thread [Next in Thread>