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
|