xfs
[Top] [All Lists]

Re: [PATCH 1/2] set minleft in xfs_bmbt_split()

To: Lachlan McIlroy <lachlan@xxxxxxx>
Subject: Re: [PATCH 1/2] set minleft in xfs_bmbt_split()
From: Dave Chinner <dchinner@xxxxxxxxx>
Date: Mon, 23 Jun 2008 22:25:56 -0700
Cc: xfs-dev <xfs-dev@xxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
In-reply-to: <4860847B.7070703@xxxxxxx>
Mail-followup-to: Lachlan McIlroy <lachlan@xxxxxxx>, xfs-dev <xfs-dev@xxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
References: <48605744.8070400@xxxxxxx> <20080624045841.GN16257@xxxxxxxxxxxxxxxxxxxxx> <4860847B.7070703@xxxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.4.2.3i
On Tue, Jun 24, 2008 at 03:22:03PM +1000, Lachlan McIlroy wrote:
> Dave Chinner wrote:
> >On Tue, Jun 24, 2008 at 12:09:08PM +1000, Lachlan McIlroy wrote:
> >>The bmap btree split code relies on a previous data extent allocation
> >>(from xfs_bmap_btalloc()) to find an AG that has sufficient space
> >>to perform a full btree split, when inserting the extent.  When
> >>converting unwritten extents we don't allocate a data extent so a
> >>btree split will be the first allocation.  In this case we need to
> >>set minleft so the allocator will pick an AG that has space to
> >>complete the split(s).
> >
> >looks good, execpt...
> >
> >>Lachlan
> >>
> >>--- 2.6.x-agno2.orig/fs/xfs/xfs_bmap_btree.c
> >>+++ 2.6.x-agno2/fs/xfs/xfs_bmap_btree.c
> >>@@ -1493,12 +1493,20 @@ xfs_bmbt_split(
> >>    left = XFS_BUF_TO_BMBT_BLOCK(lbp);
> >>    args.fsbno = cur->bc_private.b.firstblock;
> >>    args.firstblock = args.fsbno;
> >>+   args.minleft = 0;
> >>    if (args.fsbno == NULLFSBLOCK) {
> >>            args.fsbno = lbno;
> >>            args.type = XFS_ALLOCTYPE_START_BNO;
> >>+           /*
> >>+            * Make sure there is sufficient room left in the AG to
> >>+            * complete a full tree split for an extent insert.  If
> >>+            * we are converting the middle part of an extent then
> >>+            * we may need space for two tree splits.
> >>+            */
> >>+           args.minleft = xfs_trans_get_block_res(args.tp);
> >
> >I'd mention in this comment that transaction reservation needs to
> >take this specifically into account.
> How would transaction reservation take this into account?  Are you
> implying they could do something different if they knew about this
> fix?

No, I'm implying that the transaction reservation better be correct.
i.e. anywhere that unwritten extent conversion can take place needs
to supply a reservation of enough blocks to allow a double split to
occur.  i.e. we are relying on the caller to get this right, so we
need to ensure that a comment explaining the potential landmine is
present....

Cheers,

Dave.
-- 
Dave Chinner
dchinner@xxxxxxxxx


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