On Mon, Jun 23, 2008 at 03:24:45PM +1000, Lachlan McIlroy wrote:
> Dave Chinner wrote:
>> On Thu, Jun 19, 2008 at 05:28:50PM +1000, Lachlan McIlroy wrote:
>>> I see it sets a lowspace indicator which filters back up and into
>>> some btree operations. It appears the purpose of this feature is to
>>> allow allocations to search for space in other AGs as in this example
>>> from xfs_bmap_extents_to_btree():
>>>
>>> if (*firstblock == NULLFSBLOCK) {
>>> args.type = XFS_ALLOCTYPE_START_BNO;
>>> args.fsbno = XFS_INO_TO_FSB(mp, ip->i_ino);
>>> } else if (flist->xbf_low) {
>>> args.type = XFS_ALLOCTYPE_START_BNO;
>>> args.fsbno = *firstblock;
>>> } else {
>>> args.type = XFS_ALLOCTYPE_NEAR_BNO;
>>> args.fsbno = *firstblock;
>>> }
>>
>> Hmmm - the only place xbf_low is used in the extent-to-btree conversion. I
>> don't have access to the revision history anymore, so i can't find out what
>> bug the xbf_low condition was added for. It certainly looks like it is
>> allowing the btree block to be allocated in a different AG to data block.
>
> The lowspace algorithm was added way back in the early '90s and has been
> 'tweaked' many times since.
It's good to have a complete revision history around somewhere. ;)
>>> This is sort of what I was trying to do with my patch but without the
>>> special lowspace condition. This lowspace feature is probably broken
>>> because there was a similar special case in xfs_bmbt_split() that got
>>> removed with the changes that fixed the AG out-of-order locking problem.
>>>
>>> @@ -1569,12 +1569,11 @@
>>> lbno = XFS_DADDR_TO_FSB(args.mp, XFS_BUF_ADDR(lbp));
>>> left = XFS_BUF_TO_BMBT_BLOCK(lbp);
>>> args.fsbno = cur->bc_private.b.firstblock;
>>> + args.firstblock = args.fsbno;
>>> if (args.fsbno == NULLFSBLOCK) {
>>> args.fsbno = lbno;
>>> args.type = XFS_ALLOCTYPE_START_BNO;
>>> - } else if (cur->bc_private.b.flist->xbf_low)
>>> - args.type = XFS_ALLOCTYPE_FIRST_AG;
>>> - else
>>> + } else
>>> args.type = XFS_ALLOCTYPE_NEAR_BNO;
>>> args.mod = args.minleft = args.alignment = args.total = args.isfl =
>>> args.userdata = args.minalignslop = 0;
>>>
>>> This could be why we have allocations failing now.
>>
>> Hmmm - yes, could be. Well found, Lachlan. Was there an equivalent change
>> to the allocation of a new root block?
>
> Yeah but it got dropped 14 years ago in a code cleanup change! Looks like
> it was by mistake too.
Ouch.
> There used to be another special case for converting
> delayed allocations that had the same semantics as this low space trick - it
> used XFS_ALLOCTYPE_FIRST_AG instead - maybe to try a little harder to find
> space for cases where it is too late to return an error to the user.
Interesting. Any idea why that was removed? Or another accident?
[....]
>> Hmmmm - perhaps before allocating with minleft = 0 we need to
>> check if we can allocate the rest of the blocks from another AG and
>> lock both AGs in the correct order first, recheck we can allocate
>> from both of them after they are locked but before modifying anything
>> and only then proceed. If we can't find two AGs to allocate from then
>> we can safely ENOSPC without any problems. In this special case we'd then
>> be able to search the entire FS for space and hence only get an ENOSPC
>> if we are really at ENOSPC. Can you pick holes in this?
>
> Sounds like it should work. We may need to lock more than two AGs at once to
> find all the space we need. Since we can't lock AGs out of order then if we
> get
> to the last AG and we still don't have enough space then we will need to try
> again but start at an earlier AG (say AG 0 which should work).
In this case, I'd just start from XFS_ALLOCTYPE_FIRST_AG rather than
doing multiple passes and having to undo between them....
> So the code in xfs_alloc_vextent() that uses XFS_ALLOCTYPE_FIRST_AG and sets
> minleft to 0 should work.
Yes, as long as the next selected AG has the original minleft blocks
available in it.
> If we start at AG 0 and we've done the proper reservations
> then we should eventually find all the space we need - as long as everything
> that
> needs to allocate space obeys the lowspace algorithm and we always kick off
> each
> search for space from the AG we last allocated from.
Yup. Seems sane to me.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
|