| Message ID | AANLkTimuYmRDCZCkoB9YHifngCwjp9_B2ucFQ1seaiM9@mail.gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
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.
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?
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.
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.
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
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.
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:
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(-) ubi_msg("read-only compatible internal volume %d:%d" --