[PATCH 0/3] UBI: block: Some fixes for static volumes

[PATCH 0/3] UBI: block: Some fixes for static volumes

[Ezequiel Garcia]

Hi all,

This patchset addresses some issues found when using static volumes
and playing with volume resize, update, and truncate operations.

The first patch changes the block interface to use the volume 'used_bytes'
which is the actual data size. It fixes a current issue with static volumes
which produces a nasty error when they are read past the data offset.

$ 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]

It's an annoying error, but on the other side, it should never be produced
if the block is used to mount a file system, so I'm not sure it's serious
enough to go the -stable.

The second and third patches add the required changes for the block interface
to catch the "volume updated" notification. For static volumes, the
'used_bytes' parameter can be changed when updating the volume.

Comments and feedback welcome!

Ezequiel Garcia (3):
  UBI: block: Fix block device size setting
  UBI: block: Add support for the UBI_VOLUME_UPDATED notification
  UBI: Dispatch update notification if the volume is updated

 drivers/mtd/ubi/block.c | 24 ++++++++++++++++++------
 drivers/mtd/ubi/cdev.c  |  4 +++-
 2 files changed, 21 insertions(+), 7 deletions(-)

-- 
2.0.1

[PATCH 1/3] UBI: block: Fix block device size setting

[Ezequiel Garcia]

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.

In particular, this produces I/O errors on static volumes 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

It's worth noticing that the 512-byte sector representation of the volume
is only correct if the volume size is multiple of 512-bytes. This is true for
virtually any NAND device, given eraseblocks and pages are 512-byte multiple
and hence so is the LEB size.

Fixes: 9d54c8a33eec ("UBI: R/O block driver on top of UBI volumes")
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 33c6495..a29d41c 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;
@@ -378,7 +378,7 @@ int ubiblock_create(struct ubi_volume_info *vi)
 {
 	struct ubiblock *dev;
 	struct gendisk *gd;
-	u64 disk_capacity = ((u64)vi->size * vi->usable_leb_size) >> 9;
+	u64 disk_capacity = vi->used_bytes >> 9;
 	int ret;
 
 	if ((sector_t)disk_capacity != disk_capacity)
@@ -502,7 +502,7 @@ int ubiblock_remove(struct ubi_volume_info *vi)
 static int ubiblock_resize(struct ubi_volume_info *vi)
 {
 	struct ubiblock *dev;
-	u64 disk_capacity = ((u64)vi->size * vi->usable_leb_size) >> 9;
+	u64 disk_capacity = vi->used_bytes >> 9;
 
 	if ((sector_t)disk_capacity != disk_capacity) {
 		ubi_warn("%s: the volume is too big, cannot resize (%d LEBs)",
@@ -523,7 +523,7 @@ static int ubiblock_resize(struct ubi_volume_info *vi)
 
 	mutex_lock(&dev->dev_mutex);
 	set_capacity(dev->gd, disk_capacity);
-	ubi_msg("%s resized to %d LEBs", dev->gd->disk_name, vi->size);
+	ubi_msg("%s resized to %lld bytes", dev->gd->disk_name, vi->used_bytes);
 	mutex_unlock(&dev->dev_mutex);
 	mutex_unlock(&devices_mutex);
 	return 0;
-- 
2.0.1

Re: [PATCH 1/3] UBI: block: Fix block device size setting

[Artem Bityutskiy]

On Fri, 2014-08-29 at 18:42 -0300, Ezequiel Garcia wrote:
> 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.
> 
> In particular, this produces I/O errors on static volumes 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
> 
> It's worth noticing that the 512-byte sector representation of the volume
> is only correct if the volume size is multiple of 512-bytes. This is true for
> virtually any NAND device, given eraseblocks and pages are 512-byte multiple
> and hence so is the LEB size.
> 
> Fixes: 9d54c8a33eec ("UBI: R/O block driver on top of UBI volumes")
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

Looks good, do we want this one in stable?

-- 
Best Regards,
Artem Bityutskiy

[PATCH 2/3] UBI: block: Add support for the UBI_VOLUME_UPDATED notification

[Ezequiel Garcia]

Static volumes can change its 'used_bytes' when they get updated,
and so the block interface must listen to the UBI_VOLUME_UPDATED
notification to resize the block device accordingly.

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

diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index a29d41c..4fafd49 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -522,8 +522,12 @@ static int ubiblock_resize(struct ubi_volume_info *vi)
 	}
 
 	mutex_lock(&dev->dev_mutex);
-	set_capacity(dev->gd, disk_capacity);
-	ubi_msg("%s resized to %lld bytes", dev->gd->disk_name, vi->used_bytes);
+
+	if (get_capacity(dev->gd) != disk_capacity) {
+		set_capacity(dev->gd, disk_capacity);
+		ubi_msg("%s resized to %lld bytes", dev->gd->disk_name,
+			vi->used_bytes);
+	}
 	mutex_unlock(&dev->dev_mutex);
 	mutex_unlock(&devices_mutex);
 	return 0;
@@ -547,6 +551,14 @@ static int ubiblock_notify(struct notifier_block *nb,
 	case UBI_VOLUME_RESIZED:
 		ubiblock_resize(&nt->vi);
 		break;
+	case UBI_VOLUME_UPDATED:
+		/*
+		 * If the volume is static, a content update might mean the
+		 * size (i.e. used_bytes) was also changed.
+		 */
+		if (nt->vi.vol_type == UBI_STATIC_VOLUME)
+			ubiblock_resize(&nt->vi);
+		break;
 	default:
 		break;
 	}
-- 
2.0.1

Re: [PATCH 2/3] UBI: block: Add support for the UBI_VOLUME_UPDATED notification

[Artem Bityutskiy]

On Fri, 2014-08-29 at 18:42 -0300, Ezequiel Garcia wrote:
> Static volumes can change its 'used_bytes' when they get updated,
> and so the block interface must listen to the UBI_VOLUME_UPDATED
> notification to resize the block device accordingly.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

Looks like a good fix. If you want this in stable, you please, let me
know and also make sure you actually tested it and verified that block
device size actually gets changed.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 2/3] UBI: block: Add support for the UBI_VOLUME_UPDATED notification

[Ezequiel Garcia]

On 08 Sep 01:54 PM, Artem Bityutskiy wrote:
> On Fri, 2014-08-29 at 18:42 -0300, Ezequiel Garcia wrote:
> > Static volumes can change its 'used_bytes' when they get updated,
> > and so the block interface must listen to the UBI_VOLUME_UPDATED
> > notification to resize the block device accordingly.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> 
> Looks like a good fix. If you want this in stable, you please, let me
> know and also make sure you actually tested it and verified that block
> device size actually gets changed.
> 

Yes, gave this series a good test, with gluebi and ubiblock enabled,
updating/resizing a volume and then reading back the contents from the
ubi0_0, ubiblock0_0 and mtdX (gluebi) devices.

However, it would be awesome if we could get someone else to test this
and report.

Regarding -stable, I think it's worth to pull the whole series.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 2/3] UBI: block: Add support for the UBI_VOLUME_UPDATED notification

[Artem Bityutskiy]

On Mon, 2014-09-08 at 08:18 -0300, Ezequiel Garcia wrote:
> Regarding -stable, I think it's worth to pull the whole series.

I've checked and the patches do not apply to 3.16 with many conflicts. 
Are you enthusiastic enough to try this and fined out the missing
dependencies?

The we could add the "Cc: stable@vger.kernel.org # v3.16+" tags, and
also mentioned the commits we depend on in the commit messages.

At this point I pushed your patches to the master branch. If you are
going to check the stable story, please, use this branch.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 2/3] UBI: block: Add support for the UBI_VOLUME_UPDATED notification

[Ezequiel Garcia]

On 08 Sep 03:47 PM, Artem Bityutskiy wrote:
> On Mon, 2014-09-08 at 08:18 -0300, Ezequiel Garcia wrote:
> > Regarding -stable, I think it's worth to pull the whole series.
> 
> I've checked and the patches do not apply to 3.16 with many conflicts. 
> Are you enthusiastic enough to try this and fined out the missing
> dependencies?
> 
> The we could add the "Cc: stable@vger.kernel.org # v3.16+" tags, and
> also mentioned the commits we depend on in the commit messages.
> 
> At this point I pushed your patches to the master branch. If you are
> going to check the stable story, please, use this branch.
> 

Sure, I'll prepare the list of commits and let you know.

Meanwhile, it would be very useful to have someone test these patches.

Rafał, do you think you can do some testing? It would be nice to check these
fixes are correct, and maybe you can pick them for OpenWRT.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 2/3] UBI: block: Add support for the UBI_VOLUME_UPDATED notification

[Artem Bityutskiy]

On Mon, 2014-09-08 at 15:27 -0300, Ezequiel Garcia wrote:
> On 08 Sep 03:47 PM, Artem Bityutskiy wrote:
> > On Mon, 2014-09-08 at 08:18 -0300, Ezequiel Garcia wrote:
> > > Regarding -stable, I think it's worth to pull the whole series.
> > 
> > I've checked and the patches do not apply to 3.16 with many conflicts. 
> > Are you enthusiastic enough to try this and fined out the missing
> > dependencies?
> > 
> > The we could add the "Cc: stable@vger.kernel.org # v3.16+" tags, and
> > also mentioned the commits we depend on in the commit messages.
> > 
> > At this point I pushed your patches to the master branch. If you are
> > going to check the stable story, please, use this branch.
> > 
> 
> Sure, I'll prepare the list of commits and let you know.

Should I wait some more or assume we won't add the stable tags and add
these patches to the 'next' branch? It is about time, I guess.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 2/3] UBI: block: Add support for the UBI_VOLUME_UPDATED notification

[Artem Bityutskiy]

On Tue, 2014-09-16 at 18:15 +0300, Artem Bityutskiy wrote:
> On Mon, 2014-09-08 at 15:27 -0300, Ezequiel Garcia wrote:
> > On 08 Sep 03:47 PM, Artem Bityutskiy wrote:
> > > On Mon, 2014-09-08 at 08:18 -0300, Ezequiel Garcia wrote:
> > > > Regarding -stable, I think it's worth to pull the whole series.
> > > 
> > > I've checked and the patches do not apply to 3.16 with many conflicts. 
> > > Are you enthusiastic enough to try this and fined out the missing
> > > dependencies?
> > > 
> > > The we could add the "Cc: stable@vger.kernel.org # v3.16+" tags, and
> > > also mentioned the commits we depend on in the commit messages.
> > > 
> > > At this point I pushed your patches to the master branch. If you are
> > > going to check the stable story, please, use this branch.
> > > 
> > 
> > Sure, I'll prepare the list of commits and let you know.
> 
> Should I wait some more or assume we won't add the stable tags and add
> these patches to the 'next' branch? It is about time, I guess.

I've moved your patch-set to the 'ubiblock' branch so far.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 2/3] UBI: block: Add support for the UBI_VOLUME_UPDATED notification

[Ezequiel Garcia]

On 16 September 2014 16:15, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Mon, 2014-09-08 at 15:27 -0300, Ezequiel Garcia wrote:
>> On 08 Sep 03:47 PM, Artem Bityutskiy wrote:
>> > On Mon, 2014-09-08 at 08:18 -0300, Ezequiel Garcia wrote:
>> > > Regarding -stable, I think it's worth to pull the whole series.
>> >
>> > I've checked and the patches do not apply to 3.16 with many conflicts.
>> > Are you enthusiastic enough to try this and fined out the missing
>> > dependencies?
>> >
>> > The we could add the "Cc: stable@vger.kernel.org # v3.16+" tags, and
>> > also mentioned the commits we depend on in the commit messages.
>> >
>> > At this point I pushed your patches to the master branch. If you are
>> > going to check the stable story, please, use this branch.
>> >
>>
>> Sure, I'll prepare the list of commits and let you know.
>
> Should I wait some more or assume we won't add the stable tags and add
> these patches to the 'next' branch? It is about time, I guess.
>

Yeah, I have the list of commits ready a couple weeks ago:

4fda380 UBI: Dispatch update notification if the volume is updated
46a5b80 UBI: block: Add support for the UBI_VOLUME_UPDATED notification
db95429 UBI: block: Fix block device size setting
3170403 UBI: block: Avoid disk size integer overflow
b6c7b28 UBI: block: Set disk_capacity out of the mutex
1860e37 Linux 3.15

I just got delayed thinking if it was OK to pick so many commits into
stable. As an alternative,
we can mark only the ones on this patchset, namely:

4fda380 UBI: Dispatch update notification if the volume is updated
46a5b80 UBI: block: Add support for the UBI_VOLUME_UPDATED notification
db95429 UBI: block: Fix block device size setting

And I can provide a backport when the -stable people ask for one. Of
course, it's a bit more work
on everyone, so... what do you think?
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 2/3] UBI: block: Add support for the UBI_VOLUME_UPDATED notification

[Ezequiel Garcia]

On 16 September 2014 16:32, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> On 16 September 2014 16:15, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> On Mon, 2014-09-08 at 15:27 -0300, Ezequiel Garcia wrote:
>>> On 08 Sep 03:47 PM, Artem Bityutskiy wrote:
>>> > On Mon, 2014-09-08 at 08:18 -0300, Ezequiel Garcia wrote:
>>> > > Regarding -stable, I think it's worth to pull the whole series.
>>> >
>>> > I've checked and the patches do not apply to 3.16 with many conflicts.
>>> > Are you enthusiastic enough to try this and fined out the missing
>>> > dependencies?
>>> >
>>> > The we could add the "Cc: stable@vger.kernel.org # v3.16+" tags, and
>>> > also mentioned the commits we depend on in the commit messages.
>>> >
>>> > At this point I pushed your patches to the master branch. If you are
>>> > going to check the stable story, please, use this branch.
>>> >
>>>
>>> Sure, I'll prepare the list of commits and let you know.
>>
>> Should I wait some more or assume we won't add the stable tags and add
>> these patches to the 'next' branch? It is about time, I guess.
>>
>
> Yeah, I have the list of commits ready a couple weeks ago:
>
> 4fda380 UBI: Dispatch update notification if the volume is updated
> 46a5b80 UBI: block: Add support for the UBI_VOLUME_UPDATED notification
> db95429 UBI: block: Fix block device size setting
> 3170403 UBI: block: Avoid disk size integer overflow
> b6c7b28 UBI: block: Set disk_capacity out of the mutex
> 1860e37 Linux 3.15
>
> I just got delayed thinking if it was OK to pick so many commits into
> stable. As an alternative,
> we can mark only the ones on this patchset, namely:
>
> 4fda380 UBI: Dispatch update notification if the volume is updated
> 46a5b80 UBI: block: Add support for the UBI_VOLUME_UPDATED notification
> db95429 UBI: block: Fix block device size setting
>

Of course, the commit hashes above are all wrong.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 2/3] UBI: block: Add support for the UBI_VOLUME_UPDATED notification

[Artem Bityutskiy]

On Tue, 2014-09-16 at 16:32 +0100, Ezequiel Garcia wrote:
> 4fda380 UBI: Dispatch update notification if the volume is updated
> 46a5b80 UBI: block: Add support for the UBI_VOLUME_UPDATED notification
> db95429 UBI: block: Fix block device size setting
> 3170403 UBI: block: Avoid disk size integer overflow
> b6c7b28 UBI: block: Set disk_capacity out of the mutex
> 1860e37 Linux 3.15
> 
> I just got delayed thinking if it was OK to pick so many commits into
> stable. As an alternative,
> we can mark only the ones on this patchset, namely:
> 
> 4fda380 UBI: Dispatch update notification if the volume is updated
> 46a5b80 UBI: block: Add support for the UBI_VOLUME_UPDATED notification
> db95429 UBI: block: Fix block device size setting

Yeah, these 3 look like the more important ones. So do we put 

Cc: stable@vger.kernel.org # v3.16+

or

Cc: stable@vger.kernel.org # v3.15+

?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 2/3] UBI: block: Add support for the UBI_VOLUME_UPDATED notification

[Ezequiel Garcia]

On 16 September 2014 16:53, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Tue, 2014-09-16 at 16:32 +0100, Ezequiel Garcia wrote:
>> 4fda380 UBI: Dispatch update notification if the volume is updated
>> 46a5b80 UBI: block: Add support for the UBI_VOLUME_UPDATED notification
>> db95429 UBI: block: Fix block device size setting
>> 3170403 UBI: block: Avoid disk size integer overflow
>> b6c7b28 UBI: block: Set disk_capacity out of the mutex
>> 1860e37 Linux 3.15
>>
>> I just got delayed thinking if it was OK to pick so many commits into
>> stable. As an alternative,
>> we can mark only the ones on this patchset, namely:
>>
>> 4fda380 UBI: Dispatch update notification if the volume is updated
>> 46a5b80 UBI: block: Add support for the UBI_VOLUME_UPDATED notification
>> db95429 UBI: block: Fix block device size setting
>
> Yeah, these 3 look like the more important ones. So do we put
>
> Cc: stable@vger.kernel.org # v3.16+
>
> or
>
> Cc: stable@vger.kernel.org # v3.15+
>

Cc: stable@vger.kernel.org # v3.15+

I'll backport them when -stable guys ask to.

-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 2/3] UBI: block: Add support for the UBI_VOLUME_UPDATED notification

[Artem Bityutskiy]

On Tue, 2014-09-16 at 16:56 +0100, Ezequiel Garcia wrote:
> Cc: stable@vger.kernel.org # v3.15+
> 
> I'll backport them when -stable guys ask to.

OK, just did it, and pushed out, thanks!

-- 
Best Regards,
Artem Bityutskiy

[PATCH 3/3] UBI: Dispatch update notification if the volume is updated

[Ezequiel Garcia]

The UBI_IOCVOLUP ioctl is used to start an update and also to
truncate a volume. In the first case, a "volume updated" notification
is dispatched when the update is done.

This commit adds the "volume updated" notification to be also sent when
the volume is truncated. This is required for UBI block and gluebi to get
notified about the new volume size.

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

diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index 7646220..20aeb27 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -425,8 +425,10 @@ static long vol_cdev_ioctl(struct file *file, unsigned int cmd,
 			break;
 
 		err = ubi_start_update(ubi, vol, bytes);
-		if (bytes == 0)
+		if (bytes == 0) {
+			ubi_volume_notify(ubi, vol, UBI_VOLUME_UPDATED);
 			revoke_exclusive(desc, UBI_READWRITE);
+		}
 		break;
 	}
 
-- 
2.0.1