[PATCH] UBI: fix delete compatible internal volume scan

[PATCH] UBI: fix delete compatible internal volume scan

[Brijesh Singh]

This patch resolves a possible bug. Scan is adding delete compatible
blocks to both corr list and used list.
It should return after adding the block to corr list.

Signed-off-by: Brijesh Singh <brijesh.s.singh@gmail.com>
---
 drivers/mtd/ubi/scan.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
index 6b7c0c4..fde15f1 100644
--- a/drivers/mtd/ubi/scan.c
+++ b/drivers/mtd/ubi/scan.c
@@ -845,7 +845,7 @@ static int process_eb(struct ubi_device *ubi,
struct ubi_scan_info *si,
                        err = add_to_list(si, pnum, ec, &si->corr);
                        if (err)
                                return err;
-                       break;
+                       return 0;

                case UBI_COMPAT_RO:
                        ubi_msg("read-only compatible internal volume %d:%d"
--

Re: [PATCH] UBI: fix delete compatible internal volume scan

[Artem Bityutskiy]

On Tue, 2010-05-18 at 23:10 +0530, Brijesh Singh wrote:
> This patch resolves a possible bug. Scan is adding delete compatible
> blocks to both corr list and used list.
> It should return after adding the block to corr list.
> 
> Signed-off-by: Brijesh Singh <brijesh.s.singh@gmail.com>

I'm not sure this patch is enough. I think we should add a 'compat' flag
to the 'struct ubi_mkvol_req' structure, to make it possible to create
volumes with different compatibility flags. Then we need to write a test
and add it to the UBI test-suite at 'mtd-utils/tests/ubi-tests/'.

Then once we have tested and fixed this, we should merge this and also
send to the '-stable' trees.

I remember I did give the compat option a test, but probably not that
good.

I'll try to do what I've described as soon as I have time, but you guys
could do this as well, if you have bandwidth.
 
-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH] UBI: fix delete compatible internal volume scan

[Artem Bityutskiy]

On Sun, 2010-05-23 at 09:16 +0300, Artem Bityutskiy wrote:
> On Tue, 2010-05-18 at 23:10 +0530, Brijesh Singh wrote:
> > This patch resolves a possible bug. Scan is adding delete compatible
> > blocks to both corr list and used list.
> > It should return after adding the block to corr list.
> > 
> > Signed-off-by: Brijesh Singh <brijesh.s.singh@gmail.com>
> 
> I'm not sure this patch is enough. I think we should add a 'compat' flag
> to the 'struct ubi_mkvol_req' structure, to make it possible to create
> volumes with different compatibility flags. Then we need to write a test
> and add it to the UBI test-suite at 'mtd-utils/tests/ubi-tests/'.
> 
> Then once we have tested and fixed this, we should merge this and also
> send to the '-stable' trees.
> 
> I remember I did give the compat option a test, but probably not that
> good.
> 
> I'll try to do what I've described as soon as I have time, but you guys
> could do this as well, if you have bandwidth.

I did not have time to do this so far, but still planning to do. Did you
go any further with your ubil?

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

Re: [PATCH] UBI: fix delete compatible internal volume scan

[Artem Bityutskiy]

On Sat, 2010-06-12 at 17:17 +0300, Artem Bityutskiy wrote:
> On Sun, 2010-05-23 at 09:16 +0300, Artem Bityutskiy wrote:
> > On Tue, 2010-05-18 at 23:10 +0530, Brijesh Singh wrote:
> > > This patch resolves a possible bug. Scan is adding delete compatible
> > > blocks to both corr list and used list.
> > > It should return after adding the block to corr list.
> > > 
> > > Signed-off-by: Brijesh Singh <brijesh.s.singh@gmail.com>
> > 
> > I'm not sure this patch is enough. I think we should add a 'compat' flag
> > to the 'struct ubi_mkvol_req' structure, to make it possible to create
> > volumes with different compatibility flags. Then we need to write a test
> > and add it to the UBI test-suite at 'mtd-utils/tests/ubi-tests/'.
> > 
> > Then once we have tested and fixed this, we should merge this and also
> > send to the '-stable' trees.
> > 
> > I remember I did give the compat option a test, but probably not that
> > good.
> > 
> > I'll try to do what I've described as soon as I have time, but you guys
> > could do this as well, if you have bandwidth.
> 
> I did not have time to do this so far, but still planning to do. Did you
> go any further with your ubil?

Actually, what I'm thinking to do is to create a test that creates
various UBI images with different types of volume compatibility flags,
and then tries to attach them. We have libubigen in mtd-utils, and with
this library it should be is quite easy to create different images.

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

Re: [PATCH] UBI: fix delete compatible internal volume scan

[Artem Bityutskiy]

On Sun, 2010-05-23 at 09:16 +0300, Artem Bityutskiy wrote:
> On Tue, 2010-05-18 at 23:10 +0530, Brijesh Singh wrote:
> > This patch resolves a possible bug. Scan is adding delete compatible
> > blocks to both corr list and used list.
> > It should return after adding the block to corr list.
> > 
> > Signed-off-by: Brijesh Singh <brijesh.s.singh@gmail.com>
> 
> I'm not sure this patch is enough. I think we should add a 'compat' flag
> to the 'struct ubi_mkvol_req' structure, to make it possible to create
> volumes with different compatibility flags. Then we need to write a test
> and add it to the UBI test-suite at 'mtd-utils/tests/ubi-tests/'.

Ok, that was nonsense. First of all, compatibility flags are applicable
only for internal volumes, and we do not want to allow users creating
internal volumes, even for testing.

Secondly, your patch is right.

Thirdly, even without your patch everything works just fine, because
even though we also add this PEB to the used tree, we'll never ever use
it, so it will not cause problems.

Thus, I'll just apply your patch, thank you.

> Then once we have tested and fixed this, we should merge this and also
> send to the '-stable' trees.

And not need to send it to '-stable'.

> I remember I did give the compat option a test, but probably not that
> good.

They actually seem to work fine.

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

Re: [PATCH] UBI: fix delete compatible internal volume scan

[Brijesh Singh]

Hi,
On Wed, Jun 16, 2010 at 11:52 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Sun, 2010-05-23 at 09:16 +0300, Artem Bityutskiy wrote:
>> On Tue, 2010-05-18 at 23:10 +0530, Brijesh Singh wrote:
>> > This patch resolves a possible bug. Scan is adding delete compatible
>> > blocks to both corr list and used list.
>> > It should return after adding the block to corr list.
>> >
>> > Signed-off-by: Brijesh Singh <brijesh.s.singh@gmail.com>
>>
>> I'm not sure this patch is enough. I think we should add a 'compat' flag
>> to the 'struct ubi_mkvol_req' structure, to make it possible to create
>> volumes with different compatibility flags. Then we need to write a test
>> and add it to the UBI test-suite at 'mtd-utils/tests/ubi-tests/'.
>
> Ok, that was nonsense. First of all, compatibility flags are applicable
> only for internal volumes, and we do not want to allow users creating
> internal volumes, even for testing.
>
> Secondly, your patch is right.
>
> Thirdly, even without your patch everything works just fine, because
> even though we also add this PEB to the used tree, we'll never ever use
> it, so it will not cause problems.
I created internal delete compatible volume. My assumption was that
this volume gets deleted in older versions of UBI. So, UBI should not
add this PEB to used tree. Please correct me if my assumption is
wrong.
I have one question: Should UBI ensure that delete compatible volume
is deleted? If yes, should these erase blocks be deleted at
initialization (not lazily)?
> Thus, I'll just apply your patch, thank you.
>
>> Then once we have tested and fixed this, we should merge this and also
>> send to the '-stable' trees.
>
> And not need to send it to '-stable'.
>
>> I remember I did give the compat option a test, but probably not that
>> good.
>
> They actually seem to work fine.

Thanks,
Brijesh

Re: [PATCH] UBI: fix delete compatible internal volume scan

[Artem Bityutskiy]

On Wed, 2010-06-16 at 14:22 +0530, Brijesh Singh wrote:
> > Thirdly, even without your patch everything works just fine, because
> > even though we also add this PEB to the used tree, we'll never ever use
> > it, so it will not cause problems.
> I created internal delete compatible volume. My assumption was that
> this volume gets deleted in older versions of UBI.

Right.

>  So, UBI should not
> add this PEB to used tree.

As I described, it should not indeed. But adding it to the used tree
does not cause problems in practice. I mean that old UBI binaries which
do not contain your patch will also work just fine.

I've pushed your patch to the UBI tree.

>  Please correct me if my assumption is
> wrong.

It is right.

> I have one question: Should UBI ensure that delete compatible volume
> is deleted? If yes, should these erase blocks be deleted at
> initialization (not lazily)?

Hmm, good question. I think it is fine to delete them asynchronously. I
do not see any problems with that. If we have, e.g., a power cut before
they are actually erased, we will erased them next time we mount.

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