Re: [RFC PATCH 2/2] mtd: ubi: wl: avoid erasing a PEB which is empty

[Richard Weinberger <richard@nod.at>]

Sebastian,

Am 23.11.2015 um 19:09 schrieb Sebastian Andrzej Siewior:
> wear_leveling_worker() currently unconditionally puts a PEB on erase in
> the error case even it just been taken from the free_list and never
> used.
> In case the PEB was never used it can be put back on the free list
> saving an erase cycle.

Makes sense, thanks for pointing out this optimization!

> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/mtd/ubi/wl.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
> index eb4489f9082f..709ca27e103c 100644
> --- a/drivers/mtd/ubi/wl.c
> +++ b/drivers/mtd/ubi/wl.c
> @@ -631,6 +631,7 @@ static int do_sync_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
>  	return erase_worker(ubi, wl_wrk, 0);
>  }
>  
> +static int ensure_wear_leveling(struct ubi_device *ubi, int nested);
>  /**
>   * wear_leveling_worker - wear-leveling worker function.
>   * @ubi: UBI device description object
> @@ -652,6 +653,7 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk,
>  #endif
>  	struct ubi_wl_entry *e1, *e2;
>  	struct ubi_vid_hdr *vid_hdr;
> +	int to_leb_clean = 0;

Can we please rename this variable?
I know it means "the `to` LEB is clean". But for everybody else this reads as "I'm going to clean a LEB". ;)

>  	kfree(wrk);
>  	if (shutdown)
> @@ -756,6 +758,7 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk,
>  
>  	err = ubi_io_read_vid_hdr(ubi, e1->pnum, vid_hdr, 0);
>  	if (err && err != UBI_IO_BITFLIPS) {
> +		to_leb_clean = 1;
>  		if (err == UBI_IO_FF) {
>  			/*
>  			 * We are trying to move PEB without a VID header. UBI
> @@ -801,10 +804,12 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk,
>  			 * protection queue.
>  			 */
>  			protect = 1;
> +			to_leb_clean = 1;
>  			goto out_not_moved;
>  		}
>  		if (err == MOVE_RETRY) {
>  			scrubbing = 1;
> +			to_leb_clean = 1;
>  			goto out_not_moved;
>  		}
>  		if (err == MOVE_TARGET_BITFLIPS || err == MOVE_TARGET_WR_ERR ||
> @@ -830,6 +835,7 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk,
>  					ubi->erroneous_peb_count);
>  				goto out_error;
>  			}
> +			to_leb_clean = 1;
>  			erroneous = 1;
>  			goto out_not_moved;
>  		}
> @@ -900,15 +906,26 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk,
>  		wl_tree_add(e1, &ubi->scrub);
>  	else
>  		wl_tree_add(e1, &ubi->used);
> +	if (to_leb_clean) {
> +		wl_tree_add(e2, &ubi->free);
> +		ubi->free_count++;
> +	}
> +
>  	ubi_assert(!ubi->move_to_put);
>  	ubi->move_from = ubi->move_to = NULL;
>  	ubi->wl_scheduled = 0;
>  	spin_unlock(&ubi->wl_lock);
>  
>  	ubi_free_vid_hdr(ubi, vid_hdr);
> -	err = do_sync_erase(ubi, e2, vol_id, lnum, torture);
> -	if (err)
> -		goto out_ro;
> +	if (to_leb_clean) {
> +		ensure_wear_leveling(ubi, 0);

Why are you rescheduling wear leveling?
And the nested variable has to be set to no-zero as you're calling it from a UBI worker.

> +	} else {
> +		err = do_sync_erase(ubi, e2, vol_id, lnum, torture);
> +		if (err) {
> +			wl_entry_destroy(ubi, e2);

Why that? The erase_worker will free e2 if it encounters
a fatal error and gives up this PEB. You're introducing a double free.

I think you have copy&pasted from:
        err = do_sync_erase(ubi, e1, vol_id, lnum, 0);
        if (err) {
                if (e2)
                        wl_entry_destroy(ubi, e2);
                goto out_ro;
        }

...which looks wrong too. I'll double check tomorrow with a fresh brain.

Thanks,
//richard