[PATCH] UBI: fastmap convert used list to rb tree

[Shmulik Ladkani]

Hi Brian,

Nice!
Few tiny comments.

On Sun,  5 May 2013 17:47:47 -0700 Brian Pomerantz <bapper at gmail.com> wrote:
> +/**
> + * add_aeb - create and add a attach erase block to a given rb tree root.

s/add_aeb/add_aeb_rb/

> + * @ai: UBI attach info object
> + * @root: the target rb tree
> + * @pnum: PEB number of the new attach erase block
> + * @ec: erease counter of the new LEB
> + * @scrub: scrub this PEB after attaching
> + *
> + * Returns 0 on success, < 0 indicates an internal error.
> + */
> +static int add_aeb_rb(struct ubi_attach_info *ai, struct rb_root *root,
> +		   int pnum, int ec, int scrub)
> +{
> +	struct ubi_ainf_peb *aeb;
> +	struct ubi_ainf_peb *tmp_aeb;
> +	struct rb_node **p = &root->rb_node, *parent = NULL;
> +
> +	aeb = kmem_cache_alloc(ai->aeb_slab_cache, GFP_KERNEL);
> +	if (!aeb)
> +		return -ENOMEM;
> +
> +	aeb->pnum = pnum;
> +	aeb->ec = ec;
> +	aeb->lnum = -1;
> +	aeb->scrub = scrub;
> +	aeb->copy_flag = aeb->sqnum = 0;
> +
> +	ai->ec_sum += aeb->ec;
> +	ai->ec_count++;
> +
> +	if (ai->max_ec < aeb->ec)
> +		ai->max_ec = aeb->ec;
> +
> +	if (ai->min_ec > aeb->ec)
> +		ai->min_ec = aeb->ec;

Up until here, the code is identical to 'add_aeb'.
Suggest unify the above 'aeb' construction code.

>  /**
> - * assign_aeb_to_av - assigns a SEB to a given ainf_volume and removes it
> - * from it's original list.
> + * assign_aeb_to_av - assigns a SEB to a given ainf_volume

Maybe comment that 'aeb' should be already unlinked from any prior
aeb->u.list or aeb->u.rb?

> @@ -726,10 +803,7 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
>  				continue;
>  
>  			aeb = NULL;
> -			list_for_each_entry(tmp_aeb, &used, u.list) {
> -				if (tmp_aeb->pnum == pnum)
> -					aeb = tmp_aeb;
> -			}
> +			aeb = find_aeb_in_rb(&used, pnum);

The above 'aeb = NULL' is now redundant.

From ubi.h, struct ubi_ainf_peb documentation:
 * @u.rb: link in the per-volume RB-tree of &struct ubi_ainf_peb objects
 * @u.list: link in one of the eraseblock lists

Maybe we should amend the documentation, now that u.rb serves other
purposes than the per-volume RB-tree link?

Regards,
Shmulik