Re: [PATCH 02/12] repair: allocate and free inode records individually

[Dave Chinner <david@fromorbit.com>]

On Fri, Dec 02, 2011 at 12:46:21PM -0500, Christoph Hellwig wrote:
> Instead of allocating inode records in chunks and keeping a freelist of them
> which never gets released to the system memory allocator use plain malloc
> and free for them.  The freelist just means adding a global lock instead
> of relying on malloc and free which could be implemented lockless, and the
> freelist is almost completely worthless as we are done allocating new
> inode records once we start freeing them in major quantities.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

OK, the freelist has been there since at least before the start of
the git tree history, but the locking was added as part of the
threading of repair back in 2007....

It looks to me like the allocation strategy is similar to the way
log tickets used to be allocated - a roll-your-own slab cache to
work around an inefficient memory allocator....

.....
> -	/* initialize node */
> -
> -	ino_rec->ino_startnum = 0;
> -	ino_rec->ino_confirmed = 0;
> -	ino_rec->ino_isa_dir = 0;
> -	ino_rec->ir_free = (xfs_inofree_t) - 1;
> -	ino_rec->ino_un.ex_data = NULL;
> -	ino_rec->nlinkops = &nlinkops[0];
> -	ino_rec->disk_nlinks = calloc(1, nlinkops[0].nlink_size);
> -	if (ino_rec->disk_nlinks == NULL)
> +	irec = malloc(sizeof(*irec));
> +	if (!irec)
> +		do_error(_("inode map malloc failed\n"));
> +
> +	irec->avl_node.avl_nextino = NULL;
> +	irec->avl_node.avl_forw = NULL;
> +	irec->avl_node.avl_back = NULL;
> +
> +	irec->ino_startnum = starting_ino;

OK, so you moved this initialisation into the init function instead
of doing it externally. Seems OK - both callers do post-init of
this, so no reason why not to do it here.

Everything else looks fine. the renaming of mk_ino_tree_nodes()
makes it kind of obvious now what that function is doing, too, so
that is good...

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs