Re: [PATCH] Prevent extent btree block allocation failures

[Lachlan McIlroy <lachlan@sgi.com>]

Dave Chinner wrote:
> On Mon, Jun 23, 2008 at 03:57:22PM +1000, Lachlan McIlroy wrote:
>> Dave Chinner wrote:
>>> On Fri, Jun 20, 2008 at 03:21:20PM +1000, Dave Chinner wrote:
>>>> On Thu, Jun 19, 2008 at 05:28:50PM +1000, Lachlan McIlroy wrote:
>>>>> There's something else that looks suspicious to me - this code in
>>>>> xfs_bmap_btalloc() is setting minleft to 0.  Doesn't this go against
>>>>> what you were saying about setting minleft to be the space we might
>>>>> need for the btree operations?
>>>>>
>>>>> 	if (args.fsbno == NULLFSBLOCK && nullfb) {
>>>>> 		args.fsbno = 0;
>>>>> 		args.type = XFS_ALLOCTYPE_FIRST_AG;
>>>>> 		args.total = ap->minlen;
>>>>> 		args.minleft = 0;
>>>>> 		if ((error = xfs_alloc_vextent(&args)))
>>>>> 			return error;
>>>>> 		ap->low = 1;
>>>>> 	}
>>>> Hmmm - that looks suspicious. In xfs_bmapi(), when we are doing a
>>>> write and *firstblock == NULLFSBLOCK (which leads to nullfb being
>>>> set in the above code), we do:
>>>>
>>>>         if (wr && *firstblock == NULLFSBLOCK) {
>>>>                 if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_BTREE)
>>>>                         minleft = be16_to_cpu(ifp->if_broot->bb_level) + 1;
>>>>                 else
>>>>                         minleft = 1;
>>>>         } else
>>>>                 minleft = 0;
>>>>
>>>> If we are in btree format we set the minleft to the number of blocks needed
>>>> for a split. If we are in extent or local format, change to extent of btree
>>>> format requires one extra block.
>>>>
>>>> The above code you point out definitely breaks this - we haven't done a
>>>> previous allocation so we can start from the first AG, but we sure as
>>>> hell still need minleft set to the number of blocks needed for a
>>>> format change or btree split.
>>> Just to point out yet another problem in this code (one that's just
>>> been tripped over @ agami) is unwritten extent conversion.
>>>
>>> Basically, we don't do an allocation here, so when we end up in
>>> xfs_bmap_add_extent_unwritten_real() with a null firstblock. Hence
>>> the cases where conversion can cause a split - case
>>> MASK(LEFT_FILLING), MASK(RIGHT_FILLING) and 0 (convert the middle of
>>> an extent) - we can select an AG that doesn't have enough space for
>>> the entire split as we've ignored the number of blocks we might
>>> need to allocate in the split (the minleft parameter) entirely.
>>>
>>> I suspect that xfs_bmbt_split() needs to handle the null first block
>>> case slightly differently - the minleft parameter passed to the
>>> allocation should not be zero - it should be the number of levels
>>> above the current level left in the tree. i.e:
>>>
>>> 	minleft = be16_to_cpu(ifp->if_broot->bb_level) + 1;
>>>
>>> If we've already got a firstblock set, then this should have already
>>> been taken into account (i.e. we still need to fix the low space
>>> case where it got ignored as we were discussing).
>> Funny.  I tested the exact same change last week to try to fix the same
>> problem.  Seemed to work okay.
> 
> Cool. Got a patch for review?

I couldn't find the original patch that calculated minleft as above - instead
here's a variant that addresses the double insert problem by retrieving the
reservation amount from the transaction.  It could very well be overkill though.

--- fs/xfs/xfs_bmap_btree.c_1.169	2008-06-16 17:25:10.000000000 +1000
+++ fs/xfs/xfs_bmap_btree.c	2008-06-16 18:32:45.000000000 +1000
@@ -1496,9 +1496,12 @@ xfs_bmbt_split(
 	if (args.fsbno == NULLFSBLOCK) {
 		args.fsbno = lbno;
 		args.type = XFS_ALLOCTYPE_START_BNO;
-	} else
+		args.minleft = xfs_trans_get_block_res(args.tp);
+	} else {
 		args.type = XFS_ALLOCTYPE_NEAR_BNO;
-	args.mod = args.minleft = args.alignment = args.total = args.isfl =
+		args.minleft = 0;
+	}
+	args.mod = args.alignment = args.total = args.isfl =
 		args.userdata = args.minalignslop = 0;
 	args.minlen = args.maxlen = args.prod = 1;
 	args.wasdel = cur->bc_private.b.flags & XFS_BTCUR_BPRV_WASDEL;


> 
>> In the case where we convert the middle of an existing unwritten extent
>> we need to insert two new extents.  I might be paranoid here but I'll
>> assume the worst case scenario and that we'll need space for two complete
>> tree splits.
> 
> Yes, I think so. Certainly, if you look at the block reservation in
> xfs_iomap_write_unwritten():
> 
> 892         resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0) << 1;
> 
> #define XFS_DIOSTRAT_SPACE_RES(mp, v)   \
>         (XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK) + (v))
> 
> #define XFS_EXTENTADD_SPACE_RES(mp,w)   (XFS_BM_MAXLEVELS(mp,w) - 1)
> 
> It reserves enough blocks for 2 bmbt splits so I think this is
> definitely a possibility we need to handle.
> 
>> The first allocation for the first insert will set minleft
>> correctly but what about the allocations for splits during the second
>> insert?  We could run out of space in the chosen AG because minleft wasn't
>> enough.
> 
> Yeah, so we probably need pass a flag in the cursor to indicate
> it's a double split case when doing the first allocation in
> xfs_bmbt_split....
> 
> Cheers,
> 
> Dave.