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

From: Dave Chinner <david@fromorbit.com>
To: Ben Myers <bpm@sgi.com>
Cc: Christoph Hellwig <hch@infradead.org>, xfs@oss.sgi.com
Subject: Re: [PATCH 4/6] xfs: swalloc doesn't align allocations properly
Date: Tue, 17 Dec 2013 14:39:41 +1100	[thread overview]
Message-ID: <20131217033941.GC31386@dastard> (raw)
In-Reply-To: <20131216231414.GQ1935@sgi.com>

On Mon, Dec 16, 2013 at 05:14:14PM -0600, Ben Myers wrote:
> Hi Gents,
> 
> On Fri, Dec 13, 2013 at 04:01:23AM -0800, Christoph Hellwig wrote:
> > Looks good.
> > 
> > Reviewed-by: Christoph Hellwig <hch@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....

> Quoting jpk from
> http://lwn.net/Articles/87526/
> "  o [XFS] Add support for allocating additional file space in stripe width
> sized chunks. A new fstab/mount option, "swalloc" has been defined. If
> specified when mounting a striped file system, allocation requests will be
> rounded up to a stripe width if the file size is >= stripe width, and the data
> is being appended to eof. The 'swalloc' option is "off" by default. "

You can find the actual commit in the oss archive tree:

http://oss.sgi.com/cgi-bin/gitweb.cgi?p=archive/xfs-import.git;a=commitdiff;h=2da28d20c986a1a16e266f9c3e90dd967267f2bd

> This feature was likely designed with volume stripes in mind as
> opposed to to raid stripes.

I'd say go look up PV783527 and see what the problem it was solving
was... ;)

But regardless, if someone has askked for swalloc, it's because they
have an application doing large sequential IO, and when that happens
there's no reason not to stripe width align the allocation.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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