Re: Review: Multi-File Data Streams V2

[David Chinner <dgc@sgi.com>]

On Fri, Jun 22, 2007 at 06:44:42PM +1000, Timothy Shimmin wrote:
> Hi Dave,
> 
> For the xfs_bmap.c/xfs_bmap_btalloc()
> 
> *
> Might be clearer something like this:
> ------------------
> if (nullfb) {
> 	if (ap->userdata && xfs_inode_is_filestream(ap->ip)) {
> 		ag = xfs_filestream_lookup_ag(ap->ip);
> 		ag = (ag != NULLAGNUMBER) ? ag : 0;
> 		ap->rval = XFS_AGB_TO_FSB(mp, ag, 0);		
> 	} else {
> 		ap->rval = XFS_INO_TO_FSB(mp, ap->ip->i_ino);
>         }
> } else
> 	ap->rval = ap->firstblock;
> -------------------
> Unless we need "ag" set for the non-userdata && filestream case.
> I think Barry was questioning this today.

ag gets overwritten from args.fsbno, which is set up from
ap->rval, which comes from XFS_AGB_TO_FSB(mp, ag, 0) which means
that ag is not needed outside this check. So yes, it would
probably be cleaner.

> *
> It is interesting that at the start we set up the fsb
> for (userdata & filestreams) and then in a bunch of other places
> it tests just for filestreams - although, there is one spot further
> down which also tests for userdata.

Yes, because the userdata case you refer to is the need to select
a new stream ag - failing to allocate metadata, which may be in a
different AG, shouldn't cause the data stream to move.

In other cases, we check for filestreams to set up the fallbacks
on failure correctly. Those are the same for data and metadata
for filestreams.

> I find this a bit confusing (as usual:) - I thought we were only interested
> in changing the allocation of userdata for the filestream.

Mostly, yes, but metadata and data tend to be allocated with the
same locality principles which filestreams does not follow.
Hence there are small differences in the way we treat them.

> *
> As we talked about before, this code seems to come up in a few places:
> 
> 	need = XFS_MIN_FREELIST_PAG(pag, mp);
> 	delta = need > pag->pagf_flcount ?
> 		need - pag->pagf_flcount : 0;
> 	longest = (pag->pagf_longest > delta) ?
> 		(pag->pagf_longest - delta) :
> 		(pag->pagf_flcount > 0 ||
> 		pag->pagf_longest > 0);
> 
> Perhaps we could macroize/inline-function it?

Sure. I'll do that as a separate patch, though.

> It confused me in _xfs_filestream_pick_ag() when I was trying
> to understand it and so could do with a comment for it too.
> As I said then, I don't like the way it uses a boolean as
> the number of blocks, in the case when the longest extent is
> is smaller than the excess over the freelist which
> the fresspace-btree-splits-overhead needs.

Actually, the logic statement is correct. If we have a delta greater
than the longest extent, we cannot find out what the next longest
extent is without searching the btree.  Hence we assume that the
longest extent is a single block, which means if the have free
extents in the tree (pag->pagf_longest > 0) or we have blocks in the
freelist (pag->pagf_flcount > 0) we are guaranteed to be able to
alocate a single block if there is space available. So the logic is:

	longest = 0;
	if pag->pagf_longest > delta
		longest = pag->pagf_longest - delta;
	else if pag->pagf_flcount > 0
		longest = 1;
	else if pag->pagf_longest > 0
		longest = 1;

And the above is simply more compact.

> Also, the variables "need" and "delta" look pretty local to it.

*nod*

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group