xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxx>, xfs@xxxxxxxxxxx
Subject: Re: [PATCH 21/26] implement generic xfs_btree_in??ert/insrec
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Mon, 4 Aug 2008 21:29:43 -0400
In-reply-to: <20080805010557.GL6119@disturbed>
References: <20080804013535.GV8819@xxxxxx> <20080805010557.GL6119@disturbed>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.5.18 (2008-05-17)
On Tue, Aug 05, 2008 at 11:05:57AM +1000, Dave Chinner wrote:
> 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....

Yeah.  The name giving macro is already gone in my current tree.

> 
> > +   /* 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.

Done.

> > +   /*
> > +    * 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.
>        */

Updated.


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