On Thu, Dec 11, 2008 at 01:20:23PM -0800, Andrew Morton wrote:
> On Wed, 10 Dec 2008 08:28:50 +0100
> Nick Piggin <npiggin@xxxxxxx> wrote:
>
> >
> > Add a "radix_tree_gang_set_if_tagged" operation, which takes a range of indexes,
> > and sets a given tag, if any of the specified tags were found to be set. Used by
> > the next patch to set an "fsync" bit on any dirty and writeback pages.
> >
>
> Spent 15 seconds trying to decode the above, quickly gave up.
>
> Because it's the code comments which matter, not the changelog.
>
> Only there are no code comemnts. The rest of the radix-tree API is
> documented.
Fair call on comments and style. You needn't apply this one if you
don't apply the next one either. The first 4 were the more important
data integrity ones. I'll send updated 5/6 and 6/6 later, and we can
decide if we want the starvation fix.
> > + for (i = offset_start; i <= offset_end; i++) {
> > +
> > + if (height > 1) {
> > + struct radix_tree_node *slot;
> > +
> > + slot = node->slots[i];
> > + if (!slot)
> > + goto notset;
> > +
> > + if (node_tag_set_if_tagged(slot, height - 1,
> > + first, last,
> > + ifset, thentag)) {
>
> It's recursive.
>
> Considerable effort has been made to avoid recursion in the radix-tree
> code and here it gets added with nary a mention in the code or the
> changelog. It *at least* needs justification by showing a calculation
> of the maximum stack usage. Which is dependent upon the value of
> RADIX_TREE_MAP_SHIFT and is hence a constraint upon it.
>
> (I have a vague feeling that we broke the don't-recur design a while back, but
> re-breaking it is still bad).
Hmm. It's rather tricky code, so I found recursion was easier :( I'll
try to take another look at it... I guess it could go a maximum of
16 levels with CONFIG_SMALL and a really big file. It doesn't use a
heap of stack, although it's hard to say because some architectures
save a lot of things.
> The rest of the radix-tree API is exported to modules, so this addition
> should also be.
OK.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
