[PATCH v2 for v3.15 0/3] UBI: block: Support very large volumes

[PATCH v2 for v3.15 0/3] UBI: block: Support very large volumes

[Ezequiel Garcia]

Changes from v1:

  * Fixed ubiblock_resize so it returns something. I've kept a ubi_warn
    message in ubiblock_resize, because otherwise the failure is not reported
    to the user in any way.
    Thanks for the kbuild-robot for reporting this!

Here's the v1 cover letter:

This is a re-spin of Richard's work [1]. I've added a patch for some minor
clean-up, so the large volume fix is a less intrusive one.

The proposed returned value E2BIG has been changed for EFBIG, which results
in a slightly more accurate error. Also, the user messages have been removed
entirely; returning an error seems enough.

This bug was reported on #mtd IRC. I don't have to reporter name to give proper
credit. It should be applied as a fix for v3.15.

Richard: Thanks a lot for hunting this down, and feel free to disagree about
my changes.

[1] https://lkml.org/lkml/2014/3/19/333

Ezequiel Garcia (2):
  UBI: block: Make ubiblock_resize return something
  UBI: block: Set disk_capacity out of the mutex

Richard Weinberger (1):
  UBI: block: Avoid disk size integer overflow

 drivers/mtd/ubi/block.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

-- 
1.9.1

[PATCH v2 1/3] UBI: block: Make ubiblock_resize return something

[Ezequiel Garcia]

Currently, ubiblock_resize() can fail if the device is not found
in the list. This commit changes the return type, so the function can
return something meaningful on error paths.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/ubi/block.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index 8d659e6..a8e4b77 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -496,7 +496,7 @@ int ubiblock_remove(struct ubi_volume_info *vi)
 	return 0;
 }
 
-static void ubiblock_resize(struct ubi_volume_info *vi)
+static int ubiblock_resize(struct ubi_volume_info *vi)
 {
 	struct ubiblock *dev;
 	int disk_capacity;
@@ -510,7 +510,7 @@ static void ubiblock_resize(struct ubi_volume_info *vi)
 	dev = find_dev_nolock(vi->ubi_num, vi->vol_id);
 	if (!dev) {
 		mutex_unlock(&devices_mutex);
-		return;
+		return -ENODEV;
 	}
 
 	mutex_lock(&dev->dev_mutex);
@@ -519,6 +519,7 @@ static void ubiblock_resize(struct ubi_volume_info *vi)
 	ubi_msg("%s resized to %d LEBs", dev->gd->disk_name, vi->size);
 	mutex_unlock(&dev->dev_mutex);
 	mutex_unlock(&devices_mutex);
+	return 0;
 }
 
 static int ubiblock_notify(struct notifier_block *nb,
-- 
1.9.1

[PATCH v2 2/3] UBI: block: Set disk_capacity out of the mutex

[Ezequiel Garcia]

There's no need to set the disk capacity with the mutex held, so this
commit takes the variable setting out of the mutex. This simplifies
the disk capacity fix for very large volumes in a follow up commit.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/ubi/block.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index a8e4b77..502f508 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -378,7 +378,7 @@ int ubiblock_create(struct ubi_volume_info *vi)
 {
 	struct ubiblock *dev;
 	struct gendisk *gd;
-	int disk_capacity;
+	int disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
 	int ret;
 
 	/* Check that the volume isn't already handled */
@@ -412,7 +412,6 @@ int ubiblock_create(struct ubi_volume_info *vi)
 	gd->first_minor = dev->ubi_num * UBI_MAX_VOLUMES + dev->vol_id;
 	gd->private_data = dev;
 	sprintf(gd->disk_name, "ubiblock%d_%d", dev->ubi_num, dev->vol_id);
-	disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
 	set_capacity(gd, disk_capacity);
 	dev->gd = gd;
 
@@ -499,7 +498,7 @@ int ubiblock_remove(struct ubi_volume_info *vi)
 static int ubiblock_resize(struct ubi_volume_info *vi)
 {
 	struct ubiblock *dev;
-	int disk_capacity;
+	int disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
 
 	/*
 	 * Need to lock the device list until we stop using the device,
@@ -514,7 +513,6 @@ static int ubiblock_resize(struct ubi_volume_info *vi)
 	}
 
 	mutex_lock(&dev->dev_mutex);
-	disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
 	set_capacity(dev->gd, disk_capacity);
 	ubi_msg("%s resized to %d LEBs", dev->gd->disk_name, vi->size);
 	mutex_unlock(&dev->dev_mutex);
-- 
1.9.1

[PATCH v2 3/3] UBI: block: Avoid disk size integer overflow

[Ezequiel Garcia]

From: Richard Weinberger <richard@nod.at>

This patch fixes the issue that on very large UBI volumes
UBI block does not work correctly.

Signed-off-by: Richard Weinberger <richard@nod.at>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/ubi/block.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index 502f508..c1bb567 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -378,9 +378,11 @@ int ubiblock_create(struct ubi_volume_info *vi)
 {
 	struct ubiblock *dev;
 	struct gendisk *gd;
-	int disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
+	u64 disk_capacity = ((u64)vi->size * vi->usable_leb_size) >> 9;
 	int ret;
 
+	if ((sector_t)disk_capacity != disk_capacity)
+		return -EFBIG;
 	/* Check that the volume isn't already handled */
 	mutex_lock(&devices_mutex);
 	if (find_dev_nolock(vi->ubi_num, vi->vol_id)) {
@@ -498,8 +500,13 @@ int ubiblock_remove(struct ubi_volume_info *vi)
 static int ubiblock_resize(struct ubi_volume_info *vi)
 {
 	struct ubiblock *dev;
-	int disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
+	u64 disk_capacity = ((u64)vi->size * vi->usable_leb_size) >> 9;
 
+	if ((sector_t)disk_capacity != disk_capacity) {
+		ubi_warn("%s: the volume is too big, cannot resize (%d LEBs)",
+			 dev->gd->disk_name, vi->size);
+		return -EFBIG;
+	}
 	/*
 	 * Need to lock the device list until we stop using the device,
 	 * otherwise the device struct might get released in
-- 
1.9.1

Re: [PATCH v2 for v3.15 0/3] UBI: block: Support very large volumes

[Ezequiel Garcia]

Artem,

On 05 May 07:11 AM, Ezequiel Garcia wrote:
> Changes from v1:
> 
>   * Fixed ubiblock_resize so it returns something. I've kept a ubi_warn
>     message in ubiblock_resize, because otherwise the failure is not reported
>     to the user in any way.
>     Thanks for the kbuild-robot for reporting this!
> 

Gentle reminder :)

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v2 for v3.15 0/3] UBI: block: Support very large volumes

[Artem Bityutskiy]

On Mon, 2014-05-05 at 07:11 -0300, Ezequiel Garcia wrote:
> This bug was reported on #mtd IRC. I don't have to reporter name to give proper
> credit. It should be applied as a fix for v3.15.

I've taken them, thanks.

However, I do not think they are important enough to send to 3.15. The
first patch, may be, but I doubt there is really a user suffering from
that. The second is just an optimization. The third is a fix, but I am
not sure if anyone uses this driver with such a huge MTD device. So I
think 3.16 should be all-right, how does this sound to you?

Thanks!

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v2 for v3.15 0/3] UBI: block: Support very large volumes

[Ezequiel Garcia]

On 27 May 02:02 PM, Artem Bityutskiy wrote:
> On Mon, 2014-05-05 at 07:11 -0300, Ezequiel Garcia wrote:
> > This bug was reported on #mtd IRC. I don't have to reporter name to give proper
> > credit. It should be applied as a fix for v3.15.
> 
> I've taken them, thanks.
> 
> However, I do not think they are important enough to send to 3.15. The
> first patch, may be, but I doubt there is really a user suffering from
> that. The second is just an optimization. The third is a fix, but I am
> not sure if anyone uses this driver with such a huge MTD device. So I
> think 3.16 should be all-right, how does this sound to you?
> 

Sure, I'm fine with that.

Thanks,
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v2 for v3.15 0/3] UBI: block: Support very large volumes

[Ezequiel Garcia]

Hi Artem,

On 27 May 09:30 AM, Ezequiel Garcia wrote:
> On 27 May 02:02 PM, Artem Bityutskiy wrote:
> > On Mon, 2014-05-05 at 07:11 -0300, Ezequiel Garcia wrote:
> > > This bug was reported on #mtd IRC. I don't have to reporter name to give proper
> > > credit. It should be applied as a fix for v3.15.
> > 
> > I've taken them, thanks.
> > 
> > However, I do not think they are important enough to send to 3.15. The
> > first patch, may be, but I doubt there is really a user suffering from
> > that. The second is just an optimization. The third is a fix, but I am
> > not sure if anyone uses this driver with such a huge MTD device. So I
> > think 3.16 should be all-right, how does this sound to you?
> > 
> 
> Sure, I'm fine with that.
> 

I think this series got lost :-(

It's not in today's -next, and I'm looking at your pull for v3.16 and it's
not there either. It seems I overlooked it, and realised just now, just before
sending a new fix.

Any idea what happened?

Anyway, I have found a new issue with ubiblock and I think this may be suitable
for -stable. Can you take a quick look at the patch below?

If the fix below is suitable for -stable, then we should apply this one now
and then rebase the large volume support on top of it, to be applied for v3.17.

>From 72b390aa14c6d2c81dfd6f3940ceed1067b9ae15 Mon Sep 17 00:00:00 2001
From: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Date: Fri, 25 Jul 2014 19:37:03 -0300
Subject: [PATCH] UBI: block: Fix block device size set

We are currently taking the block device size from the ubi_volume_info.size
field. However, this is not the amount of data in the volume, but the
number of reserved physical eraseblocks, and hence leads to an incorrect
representation of the volume.

For static volumes, this leads to I/O errors as the block interface may
attempt to read unmapped PEBs:

$ cat /dev/ubiblock0_0 > /dev/null
UBI error: ubiblock_read_to_buf: ubiblock0_0 ubi_read error -22
end_request: I/O error, dev ubiblock0_0, sector 9536
Buffer I/O error on device ubiblock0_0, logical block 2384
[snip]

Fix this by using the ubi_volume_info.used_bytes field which is set to the
actual number of data bytes for both static and dynamic volumes.
While here, improve the error message to be less stupid and more useful:

UBI error: ubiblock_read_to_buf: ubiblock0_1 ubi_read error -9 on LEB=0, off=15872, len=512

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/ubi/block.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index 8457df7..295a234 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -188,8 +188,8 @@ static int ubiblock_read_to_buf(struct ubiblock *dev, char *buffer,
 
 	ret = ubi_read(dev->desc, leb, buffer, offset, len);
 	if (ret) {
-		ubi_err("%s ubi_read error %d",
-			dev->gd->disk_name, ret);
+		ubi_err("%s ubi_read error %d on LEB=%d, off=%d, len=%d",
+			dev->gd->disk_name, ret, leb, offset, len);
 		return ret;
 	}
 	return 0;
@@ -412,7 +412,7 @@ int ubiblock_create(struct ubi_volume_info *vi)
 	gd->first_minor = dev->ubi_num * UBI_MAX_VOLUMES + dev->vol_id;
 	gd->private_data = dev;
 	sprintf(gd->disk_name, "ubiblock%d_%d", dev->ubi_num, dev->vol_id);
-	disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
+	disk_capacity = vi->used_bytes >> 9;
 	set_capacity(gd, disk_capacity);
 	dev->gd = gd;
 
@@ -516,9 +516,9 @@ static void ubiblock_resize(struct ubi_volume_info *vi)
 	}
 
 	mutex_lock(&dev->dev_mutex);
-	disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
+	disk_capacity = vi->used_bytes >> 9;
 	set_capacity(dev->gd, disk_capacity);
-	ubi_msg("%s resized to %d LEBs", dev->gd->disk_name, vi->size);
+	ubi_msg("%s resized to %d bytes", dev->gd->disk_name, vi->used_bytes);
 	mutex_unlock(&dev->dev_mutex);
 	mutex_unlock(&devices_mutex);
 }
-- 
2.0.1

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v2 for v3.15 0/3] UBI: block: Support very large volumes

[Artem Bityutskiy]

On Fri, 2014-07-25 at 20:10 -0300, Ezequiel Garcia wrote:
> I think this series got lost :-(
> 
> It's not in today's -next, and I'm looking at your pull for v3.16 and it's
> not there either. It seems I overlooked it, and realised just now, just before
> sending a new fix.
> 
> Any idea what happened?

Shame, but no idea. May be I applied them on a laptop, and forgot to
push, and then never noticed. Anyway, sorry, just applied and pushed
out.

>  	return 0;
> @@ -412,7 +412,7 @@ int ubiblock_create(struct ubi_volume_info *vi)
>  	gd->first_minor = dev->ubi_num * UBI_MAX_VOLUMES + dev->vol_id;
>  	gd->private_data = dev;
>  	sprintf(gd->disk_name, "ubiblock%d_%d", dev->ubi_num, dev->vol_id);
> -	disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
> +	disk_capacity = vi->used_bytes >> 9;

I think you should first align up and then divide.  Something like

disk_capacity = ALIGN(vi->used_bytes, 512) / 512

may be?

By the way, do you really need to shift instead of just dividing, which
is more readable. I think with nowadays' compilers ">> 9" and "/ 512"
will give the same code, and the latter seems a bit more readable, no?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v2 for v3.15 0/3] UBI: block: Support very large volumes

[Ezequiel Garcia]

On 28 Jul 07:05 PM, Artem Bityutskiy wrote:
> On Fri, 2014-07-25 at 20:10 -0300, Ezequiel Garcia wrote:
> > I think this series got lost :-(
> > 
> > It's not in today's -next, and I'm looking at your pull for v3.16 and it's
> > not there either. It seems I overlooked it, and realised just now, just before
> > sending a new fix.
> > 
> > Any idea what happened?
> 
> Shame, but no idea. May be I applied them on a laptop, and forgot to
> push, and then never noticed. Anyway, sorry, just applied and pushed
> out.
> 

Cool, no problem.

> >  	return 0;
> > @@ -412,7 +412,7 @@ int ubiblock_create(struct ubi_volume_info *vi)
> >  	gd->first_minor = dev->ubi_num * UBI_MAX_VOLUMES + dev->vol_id;
> >  	gd->private_data = dev;
> >  	sprintf(gd->disk_name, "ubiblock%d_%d", dev->ubi_num, dev->vol_id);
> > -	disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
> > +	disk_capacity = vi->used_bytes >> 9;
> 
> I think you should first align up and then divide.  Something like
> 
> disk_capacity = ALIGN(vi->used_bytes, 512) / 512
> 
> may be?
> 

Yeah, of course.

> By the way, do you really need to shift instead of just dividing, which
> is more readable. I think with nowadays' compilers ">> 9" and "/ 512"
> will give the same code, and the latter seems a bit more readable, no?
> 

Probably. I'm not sure I was thinking in any compiler optimization, but rather
trying to follow current practice in other block code:

  $ git grep ">> 9" block/ drivers/block/ | wc -l
  133

  $ git grep "/ 512" block/ drivers/block/ | wc -l
  12

It sounds like ">> 9" is a frequent enough idiom for "block sector translation".
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com