xfs
[Top] [All Lists]

Re: [PATCH 21/26] implement generic xfs_btree_inÑert/insrec

To: Christoph Hellwig <hch@xxxxxx>
Subject: Re: [PATCH 21/26] implement generic xfs_btree_inѕert/insrec
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 5 Aug 2008 11:05:57 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20080804013535.GV8819@xxxxxx>
Mail-followup-to: Christoph Hellwig <hch@xxxxxx>, xfs@xxxxxxxxxxx
References: <20080804013535.GV8819@xxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.5.18 (2008-05-17)
On Mon, Aug 04, 2008 at 03:35:35AM +0200, Christoph Hellwig wrote:
> Make the btree insert code generic.  Based on a patch from David Chinner
> with lots of changes to follow the original btree implementations more
> closely.  While this loses some of the generic helper routines for
> inserting/moving/removing records it also solves some of the one off
> bugs in the original code and makes it easier to verify.
....
> +     if (cur->bc_flags & XFS_BTREE_ROOT_IN_INODE) {
> +             if (numrecs < cur->bc_ops->get_dmaxrecs(cur, level)) {
> +                     /* A resizeable root block that can be made bigger. */
> +                     cur->bc_ops->realloc_root(cur, 1);
> +                     return 0;
> +             }

I think that ->get_dmaxrecs is probably badly named. I called it
that originally because it matched the macro name it was wrapping.
Realisitically it should be ->get_root_maxrecs....

> +     /* Make a key out of the record data to be inserted, and save it. */
> +     cur->bc_ops->init_key_from_rec(cur, &key, recp);
> +
> +     /* If we're off the left edge, return failure. */
> +     ptr = cur->bc_ptrs[level];
> +     if (ptr == 0) {
> +             XFS_BTREE_TRACE_CURSOR(cur, XBT_EXIT);
> +             *stat = 0;
> +             return 0;
> +     }

You can probably move the key initialisation till after then initial
'in this block' checks.

> +     /*
> +      * If the block is full, we can't insert the new entry until we
> +      * make the block un-full.
> +      */
> +     xfs_btree_set_ptr_null(cur, &nptr);
> +     if (numrecs == cur->bc_ops->get_maxrecs(cur, level)) {
> +             error = xfs_btree_make_block_unfull(cur, level, numrecs,
> +                                     &optr, &ptr, &nptr, &ncur, &nrec, stat);
> +             if (error || *stat == 0)
> +                     goto error0;
> +     }
> +
> +     /* The current block may have changed during the split. */
> +     block = xfs_btree_get_block(cur, level, &bp);
> +     numrecs = xfs_btree_get_numrecs(block);

The comment here should probably refer to the unfull call, not a
'split'. i.e.:

        /*
         * the current block may have changed if the block was
         * previously full and we have just made space in it.
         */


Otherwise looks good.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx


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