[PATCH] [UBI] Volume table update fix

[PATCH] [UBI] Volume table update fix

[Brijesh Singh]

Hi,
  This patch fixes the consistency problem during volume update.
  UBI writes two copies of vtbl during volume update. If
the first copy was successfully written, and second copy fails,
UBI returns failure.
  But UBI recovers updated vtbl during next mount. So it should have
returned success. It should go to read only mode to avoid further failures.

Signed-off-by: Brijesh Singh <brij.singh@samsung.com>
---
diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
index 1afc61e..c776037 100644
--- a/drivers/mtd/ubi/vtbl.c
+++ b/drivers/mtd/ubi/vtbl.c
@@ -84,7 +84,7 @@ static struct ubi_vtbl_record empty_vtbl_record;
 int ubi_change_vtbl_record(struct ubi_device *ubi, int idx,
 			   struct ubi_vtbl_record *vtbl_rec)
 {
-	int i, err;
+	int copy, err, err1;
 	uint32_t crc;
 	struct ubi_volume *layout_vol;

@@ -99,19 +99,41 @@ int ubi_change_vtbl_record(struct ubi_device *ubi, int idx,
 	}

 	memcpy(&ubi->vtbl[idx], vtbl_rec, sizeof(struct ubi_vtbl_record));
-	for (i = 0; i < UBI_LAYOUT_VOLUME_EBS; i++) {
-		err = ubi_eba_unmap_leb(ubi, layout_vol, i);
+	for (copy = 0; copy < UBI_LAYOUT_VOLUME_EBS; copy++) {
+		err = ubi_eba_unmap_leb(ubi, layout_vol, copy);
 		if (err)
-			return err;
+			goto out_error;

-		err = ubi_eba_write_leb(ubi, layout_vol, i, ubi->vtbl, 0,
+		err = ubi_eba_write_leb(ubi, layout_vol, copy, ubi->vtbl, 0,
 					ubi->vtbl_size, UBI_LONGTERM);
 		if (err)
-			return err;
+			goto out_error;
 	}

 	paranoid_vtbl_check(ubi);
 	return 0;
+
+out_error:
+	/* If first copy was written,volume creation is successful.
+	* But switch to read only mode as we have only one copy.
+	* If first copy itself was not written, older version is in copy 2.
+	* Unmap first copy and call wl_flush.
+	* Volume creating is unsuccessful.
+	*/
+	ubi_err("Error writing volume table copy #%d", copy+1);
+	err1 = ubi_eba_unmap_leb(ubi, layout_vol, copy);
+	if (!err1) {
+		ubi_wl_flush(ubi);
+		/* Don't bother about error in flush
+		 * We are going read only any ways
+		 */
+	}
+	ubi_ro_mode(ubi);
+	ubi_msg("Try detaching and attaching UBI again");
+	if (copy > 0)
+		return 0;
+	else
+		return err;
 }

 /**

Re: [PATCH] [UBI] Volume table update fix

[Artem Bityutskiy]

Hi,

thanks for the patch. Below are my minor/stylistic notes.

> diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
> index 1afc61e..c776037 100644
> --- a/drivers/mtd/ubi/vtbl.c
> +++ b/drivers/mtd/ubi/vtbl.c
> @@ -84,7 +84,7 @@ static struct ubi_vtbl_record empty_vtbl_record;
>  int ubi_change_vtbl_record(struct ubi_device *ubi, int idx,
>  			   struct ubi_vtbl_record *vtbl_rec)
>  {
> -	int i, err;
> +	int copy, err, err1;
>  	uint32_t crc;
>  	struct ubi_volume *layout_vol;
> 
> @@ -99,19 +99,41 @@ int ubi_change_vtbl_record(struct ubi_device *ubi, int idx,
>  	}
> 
>  	memcpy(&ubi->vtbl[idx], vtbl_rec, sizeof(struct ubi_vtbl_record));
> -	for (i = 0; i < UBI_LAYOUT_VOLUME_EBS; i++) {
> -		err = ubi_eba_unmap_leb(ubi, layout_vol, i);
> +	for (copy = 0; copy < UBI_LAYOUT_VOLUME_EBS; copy++) {
> +		err = ubi_eba_unmap_leb(ubi, layout_vol, copy);
>  		if (err)
> -			return err;
> +			goto out_error;
> 
> -		err = ubi_eba_write_leb(ubi, layout_vol, i, ubi->vtbl, 0,
> +		err = ubi_eba_write_leb(ubi, layout_vol, copy, ubi->vtbl, 0,
>  					ubi->vtbl_size, UBI_LONGTERM);
>  		if (err)
> -			return err;
> +			goto out_error;
>  	}
> 
>  	paranoid_vtbl_check(ubi);
>  	return 0;
> +
> +out_error:
> +	/* If first copy was written,volume creation is successful.
> +	* But switch to read only mode as we have only one copy.
> +	* If first copy itself was not written, older version is in copy 2.
> +	* Unmap first copy and call wl_flush.
> +	* Volume creating is unsuccessful.
> +	*/

Please, clean-up the comment. Split lines nicer - you have 79 characters
per line, use the same starting '/*' as other UBI comments do. Just
glance at the other UBI comments. BTW, the commit message has somewhat
unclean line splitting as well.

> +	ubi_err("Error writing volume table copy #%d", copy+1);

UBI prints should not start with capital letters, because the printing
macros add prefixes. Take a look at other UBI prints.

> +	err1 = ubi_eba_unmap_leb(ubi, layout_vol, copy);
> +	if (!err1) {
> +		ubi_wl_flush(ubi);
> +		/* Don't bother about error in flush
> +		 * We are going read only any ways
> +		 */
Please. clean up this comment a little. You might as well just kill it. 

> +	}
> +	ubi_ro_mode(ubi);
> +	ubi_msg("Try detaching and attaching UBI again");

Please, remove this message. The kernel messages should not be used for
suggestions like this. They are not FAQ.

> +	if (copy > 0)
> +		return 0;
> +	else
> +		return err;
This is a tricky place, IMO, and deserves a comment. Could we have
something like:

/*
 * If the first volume table copy has been changed then overall the
 * operation has succeeded, because the change would be there if we now
 * re-attached the UBI device. Thus, return success in this case.
 */

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

Re: [PATCH] [UBI] Volume table update fix

[Artem Bityutskiy]

On Mon, 2009-06-22 at 10:29 +0300, Artem Bityutskiy wrote:
> > +	if (copy > 0)
> > +		return 0;
> > +	else
> > +		return err;
> This is a tricky place, IMO, and deserves a comment. Could we have
> something like:
> 
> /*
>  * If the first volume table copy has been changed then overall the
>  * operation has succeeded, because the change would be there if we now
>  * re-attached the UBI device. Thus, return success in this case.
>  */
> 
This looks a bit more elegant:

/*
 * If the first volume table copy has been changed then overall the
 * operation has succeeded, because the change would be there if we now
 * re-attached the UBI device. Thus, return success in this case.
 */
return copy ? 0 : err;

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

Re: [PATCH] [UBI] Volume table update fix

[Brijesh Singh]

Hi Artem,
    Thanks for the comments.Sending the cleaned up patch.

    This patch fixes the consistency problem during volume update.
UBI writes two copies of vtbl during volume update. If the first  copy  was
successfully written, and second copy fails, UBI returns failure. But UBI
recovers updated vtbl during next mount. So it should have returned success.
It should go to read only mode to avoid further failures.

Signed-off-by: Brijesh Singh <brij.singh@samsung.com>
---
diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
index 1afc61e..77659d4 100644
--- a/drivers/mtd/ubi/vtbl.c
+++ b/drivers/mtd/ubi/vtbl.c
@@ -84,7 +84,7 @@ static struct ubi_vtbl_record empty_vtbl_record;
 int ubi_change_vtbl_record(struct ubi_device *ubi, int idx,
 			   struct ubi_vtbl_record *vtbl_rec)
 {
-	int i, err;
+	int copy, err, err1;
 	uint32_t crc;
 	struct ubi_volume *layout_vol;

@@ -99,19 +99,35 @@ int ubi_change_vtbl_record(struct ubi_device *ubi, int idx,
 	}

 	memcpy(&ubi->vtbl[idx], vtbl_rec, sizeof(struct ubi_vtbl_record));
-	for (i = 0; i < UBI_LAYOUT_VOLUME_EBS; i++) {
-		err = ubi_eba_unmap_leb(ubi, layout_vol, i);
+	for (copy = 0; copy < UBI_LAYOUT_VOLUME_EBS; copy++) {
+		err = ubi_eba_unmap_leb(ubi, layout_vol, copy);
 		if (err)
-			return err;
+			goto out_error;

-		err = ubi_eba_write_leb(ubi, layout_vol, i, ubi->vtbl, 0,
+		err = ubi_eba_write_leb(ubi, layout_vol, copy, ubi->vtbl, 0,
 					ubi->vtbl_size, UBI_LONGTERM);
 		if (err)
-			return err;
+			goto out_error;
 	}

 	paranoid_vtbl_check(ubi);
 	return 0;
+
+out_error:
+	ubi_err("error writing volume table copy #%d", copy+1);
+	err1 = ubi_eba_unmap_leb(ubi, layout_vol, copy);
+	if (!err1)
+		ubi_wl_flush(ubi);
+
+	/* Only one valid copy of vtbl left, go to read only mode for safty. */
+	ubi_ro_mode(ubi);
+
+	/*
+	 * If first copy was written successfully, this copy will be recovered
+	 * during next mount. So vtbl updation should be successful.
+	 * But if first copy itself was not written, volume update should fail.
+	 */
+	return copy ? 0 : err;
 }

 /**


On Mon, Jun 22, 2009 at 1:04 PM, Artem Bityutskiy<dedekind@infradead.org> wrote:
> On Mon, 2009-06-22 at 10:29 +0300, Artem Bityutskiy wrote:
>> > +   if (copy > 0)
>> > +           return 0;
>> > +   else
>> > +           return err;
>> This is a tricky place, IMO, and deserves a comment. Could we have
>> something like:
>>
>> /*
>>  * If the first volume table copy has been changed then overall the
>>  * operation has succeeded, because the change would be there if we now
>>  * re-attached the UBI device. Thus, return success in this case.
>>  */
>>
> This looks a bit more elegant:
>
> /*
>  * If the first volume table copy has been changed then overall the
>  * operation has succeeded, because the change would be there if we now
>  * re-attached the UBI device. Thus, return success in this case.
>  */
> return copy ? 0 : err;
>
> --
> Best regards,
> Artem Bityutskiy (Битюцкий Артём)
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

Re: [PATCH] [UBI] Volume table update fix

[Artem Bityutskiy]

On Mon, 2009-06-22 at 16:44 +0530, Brijesh Singh wrote:
> Hi Artem,
>     Thanks for the comments.Sending the cleaned up patch.
> 
>     This patch fixes the consistency problem during volume update.
> UBI writes two copies of vtbl during volume update. If the first  copy  was
> successfully written, and second copy fails, UBI returns failure. But UBI
> recovers updated vtbl during next mount. So it should have returned success.
> It should go to read only mode to avoid further failures.
> 
> Signed-off-by: Brijesh Singh <brij.singh@samsung.com>

Applied with minor amendments, thanks.

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