Re: [PATCH] UBIFS: add tolerance to use variable writesize

Re: [PATCH] UBIFS: add tolerance to use variable writesize

[Dirk Behme]

Am 15.05.2013 17:41, schrieb Dirk Behme:
> From: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com>
>
> UBIfs adapts it‘s structures according to the minimum writeSize reported
> by the MTD device. For NOR flash devices, this is normally 1. For NOR
> devices with internal hardware-ECC it might be more, such as the S-Die
> flash chips of Micron which can use an internal ECC if the chip is
> accessed in 32 byte chunks.
>
> The UBIfs mount process checks and compares the writeSize set in the
> image and the writeSize reported from the /dev/mtd . UBIfs will fail
> to mount if the values differ. It should, though, not be a problem to
> mount an image which was created with a writeSize larger than that of
> the MTD, if it is larger by an integer factor.
>
> This commit changes the check in a way so it will allow the image
> writeSize to be larger than that of the MTD by an integer factor.
> It will allow to create images and deploy them on different devices
> using different writeSizes. writeSize found using values of 1, 8 and 32.
>
> Signed-off-by: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com>
> ---
>   fs/ubifs/sb.c |   12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
> index 4c37607..69dd794 100644
> --- a/fs/ubifs/sb.c
> +++ b/fs/ubifs/sb.c
> @@ -351,6 +351,7 @@ static int validate_sb(struct ubifs_info *c, struct ubifs_sb_node *sup)
>   {
>   	long long max_bytes;
>   	int err = 1, min_leb_cnt;
> +	int chk_tmp;
>
>   	if (!c->key_hash) {
>   		err = 2;
> @@ -362,9 +363,16 @@ static int validate_sb(struct ubifs_info *c, struct ubifs_sb_node *sup)
>   		goto failed;
>   	}
>
> -	if (le32_to_cpu(sup->min_io_size) != c->min_io_size) {
> +	/*
> +	 * Allow a min_io_size mismatch in the way that
> +	 * the size in the superblock (the image) is larger by
> +	 * an integer factor. If image-IOsize mod real-IOsize
> +	 * is zero, it should be ok to mount this.
> +	 */
> +	chk_tmp = le32_to_cpu(sup->min_io_size);
> +	if (chk_tmp != c->min_io_size && ((chk_tmp%c->min_io_size) != 0)) {
>   		ubifs_err("min. I/O unit mismatch: %d in superblock, %d real",
> -			  le32_to_cpu(sup->min_io_size), c->min_io_size);
> +			  chk_tmp, c->min_io_size);
>   		goto failed;
>   	}

Any comments on this?

Many thanks

Dirk

[PATCH] UBIFS: add tolerance to use variable writesize

[Dirk Behme]

From: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com>

UBIfs adapts it‘s structures according to the minimum writeSize reported
by the MTD device. For NOR flash devices, this is normally 1. For NOR
devices with internal hardware-ECC it might be more, such as the S-Die
flash chips of Micron which can use an internal ECC if the chip is
accessed in 32 byte chunks.

The UBIfs mount process checks and compares the writeSize set in the
image and the writeSize reported from the /dev/mtd . UBIfs will fail
to mount if the values differ. It should, though, not be a problem to
mount an image which was created with a writeSize larger than that of
the MTD, if it is larger by an integer factor.

This commit changes the check in a way so it will allow the image
writeSize to be larger than that of the MTD by an integer factor.
It will allow to create images and deploy them on different devices
using different writeSizes. writeSize found using values of 1, 8 and 32.

Signed-off-by: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com>
---
 fs/ubifs/sb.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
index 4c37607..69dd794 100644
--- a/fs/ubifs/sb.c
+++ b/fs/ubifs/sb.c
@@ -351,6 +351,7 @@ static int validate_sb(struct ubifs_info *c, struct ubifs_sb_node *sup)
 {
 	long long max_bytes;
 	int err = 1, min_leb_cnt;
+	int chk_tmp;
 
 	if (!c->key_hash) {
 		err = 2;
@@ -362,9 +363,16 @@ static int validate_sb(struct ubifs_info *c, struct ubifs_sb_node *sup)
 		goto failed;
 	}
 
-	if (le32_to_cpu(sup->min_io_size) != c->min_io_size) {
+	/*
+	 * Allow a min_io_size mismatch in the way that
+	 * the size in the superblock (the image) is larger by
+	 * an integer factor. If image-IOsize mod real-IOsize
+	 * is zero, it should be ok to mount this.
+	 */
+	chk_tmp = le32_to_cpu(sup->min_io_size);
+	if (chk_tmp != c->min_io_size && ((chk_tmp%c->min_io_size) != 0)) {
 		ubifs_err("min. I/O unit mismatch: %d in superblock, %d real",
-			  le32_to_cpu(sup->min_io_size), c->min_io_size);
+			  chk_tmp, c->min_io_size);
 		goto failed;
 	}
 
-- 
1.7.10.4

Re: [PATCH] UBIFS: add tolerance to use variable writesize

[Artem Bityutskiy]

On Wed, 2013-05-15 at 18:21 +0200, Dirk Behme wrote:
> From: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com>
> 
> UBIfs adapts it‘s structures according to the minimum writeSize reported
> by the MTD device. For NOR flash devices, this is normally 1. For NOR
> devices with internal hardware-ECC it might be more, such as the S-Die
> flash chips of Micron which can use an internal ECC if the chip is
> accessed in 32 byte chunks.
> 
> The UBIfs mount process checks and compares the writeSize set in the
> image and the writeSize reported from the /dev/mtd . UBIfs will fail
> to mount if the values differ. It should, though, not be a problem to
> mount an image which was created with a writeSize larger than that of
> the MTD, if it is larger by an integer factor.
> 
> This commit changes the check in a way so it will allow the image
> writeSize to be larger than that of the MTD by an integer factor.
> It will allow to create images and deploy them on different devices
> using different writeSizes. writeSize found using values of 1, 8 and 32.
> 
> Signed-off-by: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com>

Hi Dirk,

first of all, sorry for long delay.

In UBIFS we have 2 flash parameters which are relevant to this
discussion:

1. min_io_size - minimum size of the I/O buffer, comes from
mtd->writesize
2. max_write_size - optimzl size of the I/O buffer, comes from
mtd->writebufsize

Take a look at io.c.

So it looks like what you actually want to do is to make sure you have
the right max_write_size, in which case UBIFS will do most of its writes
in max_wirte_size units. E.g., make it to be 32.

This can be done by teaching your flash driver to export correct
'mtd->writebufsize'. My guess is that you do not do this, and it
defaults to 'mtd->writesize'.

-- 
Best Regards,
Artem Bityutskiy

RE: [PATCH] UBIFS: add tolerance to use variable writesize

[Dahlhoff Achim (CM-AI/PJ-CF32)]

Hello.

No, I do not want to touch the max_write_size or writebufsize. The max_write_size is the size which can be transferred in one piece to the chip and does not modify the UBIfs data structures. The min size is relevant and can cause images to become incompatible with a specific NOR chip. If the NOR driver should accidentally choose a writesize which is too small, this will not cause any compatibility issues or data loss. It will only reduce write performance.

Without the tolerance patch, you cannot mount one UBSfs image on different NOR devices with different writesize values of 1 or 32.


Achim.


-----Original Message-----
From: Artem Bityutskiy [mailto:dedekind1@gmail.com] 
Sent: Monday, July 01, 2013 9:55 AM
To: Dirk Behme
Cc: linux-mtd@lists.infradead.org; Behme Dirk (CM-AI/PJ-CF32); Dahlhoff Achim (CM-AI/PJ-CF32)
Subject: Re: [PATCH] UBIFS: add tolerance to use variable writesize

On Wed, 2013-05-15 at 18:21 +0200, Dirk Behme wrote:
> From: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com>
> 
> UBIfs adapts it‘s structures according to the minimum writeSize reported
> by the MTD device. For NOR flash devices, this is normally 1. For NOR
> devices with internal hardware-ECC it might be more, such as the S-Die
> flash chips of Micron which can use an internal ECC if the chip is
> accessed in 32 byte chunks.
> 
> The UBIfs mount process checks and compares the writeSize set in the
> image and the writeSize reported from the /dev/mtd . UBIfs will fail
> to mount if the values differ. It should, though, not be a problem to
> mount an image which was created with a writeSize larger than that of
> the MTD, if it is larger by an integer factor.
> 
> This commit changes the check in a way so it will allow the image
> writeSize to be larger than that of the MTD by an integer factor.
> It will allow to create images and deploy them on different devices
> using different writeSizes. writeSize found using values of 1, 8 and 32.
> 
> Signed-off-by: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com>

Hi Dirk,

first of all, sorry for long delay.

In UBIFS we have 2 flash parameters which are relevant to this
discussion:

1. min_io_size - minimum size of the I/O buffer, comes from
mtd->writesize
2. max_write_size - optimzl size of the I/O buffer, comes from
mtd->writebufsize

Take a look at io.c.

So it looks like what you actually want to do is to make sure you have
the right max_write_size, in which case UBIFS will do most of its writes
in max_wirte_size units. E.g., make it to be 32.

This can be done by teaching your flash driver to export correct
'mtd->writebufsize'. My guess is that you do not do this, and it
defaults to 'mtd->writesize'.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH] UBIFS: add tolerance to use variable writesize

[Artem Bityutskiy]

On Mon, 2013-07-01 at 10:32 +0200, Dahlhoff Achim (CM-AI/PJ-CF32) wrote:
> Hello.
> 
> No, I do not want to touch the max_write_size or writebufsize. The
> max_write_size is the size which can be transferred in one piece to
> the chip and does not modify the UBIfs data structures. 

Not sure I understand what you mean. Max_write_size, which is the same
as writebufsize, despite the name, is the _optimal_ I/O size. Sometimes
UBI/UBIFS _will_ write in smaller chunks. But it will try to write in
'max_write_size'.

Sounds like what you need - you flash does allow 1-byte writes, but will
be more efficient when writes are 32-bytes instead.

> The min size is relevant and can cause images to become incompatible
> with a specific NOR chip. If the NOR driver should accidentally choose
> a writesize which is too small, this will not cause any compatibility
> issues or data loss. It will only reduce write performance.

Right, increasing min_io_size breaks compatibility. Although I believe
this is fixable, and I already suggested people to fix this several
times in the past.

But increasing max_write_size should not lead to incompatibilites.
> 
> Without the tolerance patch, you cannot mount one UBSfs image on
> different NOR devices with different writesize values of 1 or 32.

I still think that you should just change writebufsize in your driver.
May be you could find some other view-angle to show my why you think I
am wrong?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH] UBIFS: add tolerance to use variable writesize

[Dirk Behme]

On 01.07.2013 11:33, Artem Bityutskiy wrote:
> On Mon, 2013-07-01 at 10:32 +0200, Dahlhoff Achim (CM-AI/PJ-CF32) wrote:
>> Hello.
>>
>> No, I do not want to touch the max_write_size or writebufsize. The
>> max_write_size is the size which can be transferred in one piece to
>> the chip and does not modify the UBIfs data structures.
>
> Not sure I understand what you mean. Max_write_size, which is the same
> as writebufsize, despite the name, is the _optimal_ I/O size. Sometimes
> UBI/UBIFS _will_ write in smaller chunks. But it will try to write in
> 'max_write_size'.
>
> Sounds like what you need - you flash does allow 1-byte writes, but will
> be more efficient when writes are 32-bytes instead.
>
>> The min size is relevant and can cause images to become incompatible
>> with a specific NOR chip. If the NOR driver should accidentally choose
>> a writesize which is too small, this will not cause any compatibility
>> issues or data loss. It will only reduce write performance.
>
> Right, increasing min_io_size breaks compatibility. Although I believe
> this is fixable, and I already suggested people to fix this several
> times in the past.
>
> But increasing max_write_size should not lead to incompatibilites.
>>
>> Without the tolerance patch, you cannot mount one UBSfs image on
>> different NOR devices with different writesize values of 1 or 32.
>
> I still think that you should just change writebufsize in your driver.
> May be you could find some other view-angle to show my why you think I
> am wrong?

I'm no expert on this, but let me try to explain what I've understood 
from Achim:

We are talking about UBIFS images which are created on the PC 
("mkfs.ubifs" and "ubinize") and then written to real physical NOR 
devices on the target hardware.

Different NOR devices can have different writesize values of 1, 8 or 32. 
On the target, this is reported from the physical NOR chip and used 
there for the UBIFS image. But, if the UBIFS image is created on the PC, 
the PC tools take the writesize as a parameter (--min-io-size) due to 
the unknown hardware device. This writesize is recorded in the UBIFS 
created on the PC, e.g. writesize == 1.

Now, this UBIFS image created on the PC should be written on the target 
to a real physical NOR device. This reports e.g. a writesize == 32. 
While writing the UBIFS image to the NOR device works, mounting it 
fails. Due to the mismatch in the writesize: The UBIFS image has recored 
1, while the physical NOR device reports 32.

As technically there is no reason why this shouldn't work, the patch we 
are discussing here tries to fix that.

I hope above is roughly correct ;)

Best regards

Dirk