[PATCH] [MTD] UBI: Fix counting of ec value

[PATCH] [MTD] UBI: Fix counting of ec value

[Timo Lindhorst]

If the torture is triggered on a peb, the block is erased more than
once. So the ec value should be increased accordingly.
ubi_io_sync_erase returns the number of erasures made, so this value
can be used instead of a static 1.

Signed-off-by: Timo Lindhorst <lindhors@linux.vnet.ibm.com>
---
 drivers/mtd/ubi/wl.c |   27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

--- ubi-2.6.git.orig/drivers/mtd/ubi/wl.c
+++ ubi-2.6.git/drivers/mtd/ubi/wl.c
@@ -1395,34 +1395,37 @@ bitflip:
 static int sync_erase(const struct ubi_info *ubi, struct ubi_wl_entry *e,
 		      int torture)
 {
-	int err;
+	int err, ret;
 	struct ubi_ec_hdr *ec_hdr;
 	struct ubi_wl_info *wl = ubi->wl;
-	uint64_t ec = e->ec + 1;
+	uint64_t ec = e->ec;
 
-	dbg_wl("erase PEB %d, new EC %lld", e->pnum, (long long)ec);
+	dbg_wl("erase PEB %d, old EC %lld", e->pnum, (long long)ec);
 
 	err = paranoid_check_ec(ubi, e->pnum, e->ec);
 	if (unlikely(err > 0))
 		return -EINVAL;
 
+	ec_hdr = ubi_zalloc_ec_hdr(ubi);
+	if (unlikely(!ec_hdr))
+		return -ENOMEM;
+
+	ret = err = ubi_io_sync_erase(ubi, e->pnum, torture);
+	if (unlikely(err < 0))
+		goto out_free;
+
+	ec += ret;
 	if (unlikely(ec > UBI_MAX_ERASECOUNTER)) {
 		/*
 		 * Erase counter overflow. Upgrade UBI and use 64-bit
 		 * erase counters internally.
 		 */
 		ubi_err("erase counter overflow at PEB %d, EC %d",
-			e->pnum, e->ec);
+			e->pnum, ec);
 		return -EINVAL;
 	}
 
-	ec_hdr = ubi_zalloc_ec_hdr(ubi);
-	if (unlikely(!ec_hdr))
-		return -ENOMEM;
-
-	err = ubi_io_sync_erase(ubi, e->pnum, torture);
-	if (unlikely(err < 0))
-		goto out_free;
+	dbg_wl("erased PEB %d, new EC %lld", e->pnum, (long long)ec);
 
 	ec_hdr->ec = cpu_to_ubi64(ec);
 
@@ -1430,7 +1433,7 @@ static int sync_erase(const struct ubi_i
 	if (unlikely(err))
 		goto out_free;
 
-	e->ec += 1;
+	e->ec = ec;
 	if (e->ec > wl->max_ec)
 		wl->max_ec = e->ec;
 

Re: [PATCH] [MTD] UBI: Fix counting of ec value

[Artem Bityutskiy]

Hi Timo,

thanks for the patches. The comment-fixing one is fine - applied. This
one is fine in general, but i have questions.

On Mon, 2007-02-12 at 10:16 +0100, Timo Lindhorst wrote:
>  static int sync_erase(const struct ubi_info *ubi, struct ubi_wl_entry *e,
>  		      int torture)
>  {
> -	int err;
> +	int err, ret;
>  	struct ubi_ec_hdr *ec_hdr;
>  	struct ubi_wl_info *wl = ubi->wl;
> -	uint64_t ec = e->ec + 1;
> +	uint64_t ec = e->ec;
>  
> -	dbg_wl("erase PEB %d, new EC %lld", e->pnum, (long long)ec);
> +	dbg_wl("erase PEB %d, old EC %lld", e->pnum, (long long)ec);
>  
>  	err = paranoid_check_ec(ubi, e->pnum, e->ec);
>  	if (unlikely(err > 0))
>  		return -EINVAL;
>  
> +	ec_hdr = ubi_zalloc_ec_hdr(ubi);
> +	if (unlikely(!ec_hdr))
> +		return -ENOMEM;

So why have you moved this memory allocation here?

> +
> +	ret = err = ubi_io_sync_erase(ubi, e->pnum, torture);
> +	if (unlikely(err < 0))
> +		goto out_free;
> +
> +	ec += ret;
What's the point in the new 'ret' variable? Why ec += err does not work?

>  	if (unlikely(ec > UBI_MAX_ERASECOUNTER)) {
>  		/*
>  		 * Erase counter overflow. Upgrade UBI and use 64-bit
>  		 * erase counters internally.
>  		 */
>  		ubi_err("erase counter overflow at PEB %d, EC %d",
> -			e->pnum, e->ec);
> +			e->pnum, ec);
>  		return -EINVAL;
And now you do not free memory. Please do not move the allocation if it
is not really necessary.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH] [MTD] UBI: Fix counting of ec value

[Timo Lindhorst]

Hi Artem,

>>+	ec_hdr = ubi_zalloc_ec_hdr(ubi);
>>+	if (unlikely(!ec_hdr))
>>+		return -ENOMEM;
> 
> 
> So why have you moved this memory allocation here?
Actually I have not moved up the allocation, but moved down the
'if (unlikely(ec > UBI_MAX_ERASECOUNTER)) { ... ' block, since we need 
to know the new ec value before we can do this test.

I thought you want the allocation to be done before erasure, to prevent 
the erasure if the ec_hdr cannot be written due to a failure of memory 
allocation.
Otherwise, the allocation can be moved down again, but than the new ec 
is possibly not written to the EB if no memory can be allocated for ec_hdr.

Do I miss something? What do you think?

>>+
>>+	ret = err = ubi_io_sync_erase(ubi, e->pnum, torture);
>>+	if (unlikely(err < 0))
>>+		goto out_free;
>>+
>>+	ec += ret;
> 
> What's the point in the new 'ret' variable? Why ec += err does not work?
I just thought adding an error code to a counter seems odd. Do you 
prefer this way?

> 
>> 	if (unlikely(ec > UBI_MAX_ERASECOUNTER)) {
>> 		/*
>> 		 * Erase counter overflow. Upgrade UBI and use 64-bit
>> 		 * erase counters internally.
>> 		 */
>> 		ubi_err("erase counter overflow at PEB %d, EC %d",
>>-			e->pnum, e->ec);
>>+			e->pnum, ec);
>> 		return -EINVAL;
> 
> And now you do not free memory.
Yes, sorry! I have changed it to:

		err = -EINVAL;
		goto out_free;
	}


I will resend the patch when I have got your comments to the upper points.

Kind regards,
Timo

Re: [PATCH] [MTD] UBI: Fix counting of ec value

[Artem Bityutskiy]

On Mon, 2007-02-12 at 11:48 +0100, Timo Lindhorst wrote: 
> > So why have you moved this memory allocation here?
> Actually I have not moved up the allocation, but moved down the
> 'if (unlikely(ec > UBI_MAX_ERASECOUNTER)) { ... ' block, since we need 
> to know the new ec value before we can do this test.

OK, fine, just free the memory there.

> > What's the point in the new 'ret' variable? Why ec += err does not work?
> I just thought adding an error code to a counter seems odd. Do you 
> prefer this way?

Well, it's anyway better then a distinct variable IMO. Moreover, it is
used like this across the UBI code, so just assume it is UBI style :-)

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)