[Top] [All Lists]

Re: [PATCH v2 6/9] xfsrestore: fix node table setup

To: aelder@xxxxxxx
Subject: Re: [PATCH v2 6/9] xfsrestore: fix node table setup
From: Bill Kendall <wkendall@xxxxxxx>
Date: Mon, 15 Nov 2010 15:30:56 -0600
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1289853515.2199.225.camel@doink>
References: <20101105163500.747192954@xxxxxxx> <20101105163644.182690256@xxxxxxx> <1289853515.2199.225.camel@doink>
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv: Gecko/20100915 Thunderbird/3.0.8
On 11/15/2010 02:38 PM, Alex Elder wrote:
On Fri, 2010-11-05 at 11:35 -0500, wkendall@xxxxxxx wrote:
plain text document attachment (winmap_max_fix)
The node table setup code unintentionally limits the amount of virtual
memory that will be used for the node table. Rather than setting a
limit based on the remaining virtual memory, the node table is limited
to the amount of memory it thinks it will need based on the dump's
inode count. But the node table stores directory entries, not inodes,
and so dumps with lots of hard links end up thrashing mapping and
unmapping segments of the node table to stay within the self-imposed
virtual memory limit.

This patch also changes the node table to ensure that there are a
power of two nodes per segment. Profiling revealed that while building
the node table, 50% of the restore cpu time was spent doing division
and modulo to convert a node index into its segment index and offset
within the segment. This change prepares the node table for another
patch which will change the lookup mechanism to use shift and

Also don't bother passing the estimated segment table size to the
window abstraction. It has to deal with resizing the table anyway and
can choose a reasonable starting size.

Signed-off-by: Bill Kendall<wkendall@xxxxxxx>

This looks good to me.

In your loop determining how many segments to
use, etc., you might as well start with the
minimum number of segments.  I.e.:

winmapmax = 0;
for ( segcount = WINMAP_MIN; winmapmax<  WINMAP_MIN; segcount<<= 1 ) {

I had a feeling there could be a pathological case
in which the the computation of winmapmax, etc. would
get stuck looping indefinitely, but I gave up trying
to see if I could identify such a case...

I had a similar thought while making the change, but then
remembered that we just need enough virtual memory to
map up to WINMAP_WIN segments. It's perfectly okay to
only use one segment assuming there's enough virtual

I did some unit testing here playing around with the
bounds of the inputs (inode count and virtual memory size)
just to be sure that all output params were sane.


And although I'm not that confident I understand the way
the windows on the tree file are used, I don't see anything
really wrong with your change.

Reviewed-by: Alex Elder<aelder@xxxxxxx>

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