From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nick Piggin Subject: Re: [patch 5/6] radix-tree: gang set if tagged operation Date: Thu, 11 Dec 2008 23:10:12 +0100 Message-ID: <20081211221012.GB8294@wotan.suse.de> References: <20081210072454.GB27096@wotan.suse.de> <20081210072850.GF27096@wotan.suse.de> <20081211132023.6d9914bb.akpm@linux-foundation.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-fsdevel@vger.kernel.org, mpatocka@redhat.com To: Andrew Morton Return-path: Received: from mail.suse.de ([195.135.220.2]:44713 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755710AbYLKWKR (ORCPT ); Thu, 11 Dec 2008 17:10:17 -0500 Content-Disposition: inline In-Reply-To: <20081211132023.6d9914bb.akpm@linux-foundation.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, Dec 11, 2008 at 01:20:23PM -0800, Andrew Morton wrote: > On Wed, 10 Dec 2008 08:28:50 +0100 > Nick Piggin 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.