UBI: fix rb_tree node comparison in add_map commit buggy?

DISCUSSION Richard Weinberger 4 participants 9 messages Jun 23, 2014 – Jun 24, 2014 2 messages not in the archive Memory technology devices (MTD)

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Richard Weinberger

author maintainer

2014-06-23 11:58 UTC #2 lore raw

Hi!

Am 23.06.2014 13:04, schrieb Heiko Schocher:

Quoted 28 lines

Hello all,
I just ported the linux v3.14 MTD/UBI/UBIFS sources to U-Boot to get in U-Boot also UBI fastmap support. This works now nice in U-Boot with linux v3.14 kernels :-)
Now just booted a linux v3.16-rc1 kernel and I could not mount the ubifs based rootfs anymore (using UBI Fastmap support active and burned the ubi rootfs image in U-Boot) ...
I found this patch in the linux tree:
commit 604b592e6fd3c98f21435e1181ba7723ffc24715
Author: Mike Snitzer <[email protected]>
Date:   Fri Mar 21 15:54:03 2014 -0400
UBI: fix rb_tree node comparison in add_map
The comparisons used in add_vol() shouldn't be identical. Pretty sure the following is correct but it is completely untested.
Signed-off-by Mike Snitzer <[email protected]>
Acked-by Richard Weinberger <[email protected]>
Signed-off-by Artem Bityutskiy <[email protected]>
Reverted it in the linux tree, and my rootfs boots again ... (This patch is not yet in the u-boot sources, as they based on v3.14)

Quoted 7 lines

So, this change seems at least to break backwardcompatibility... (I must admit, I am not a ubi nor ubi fastmap expert ...)
I tried this patch also in the U-Boot code, without getting a ubifs rootfs (burned with u-boot into the ubi volume) bootable in linux ... which worked before... maybe v3.14 and v3.16-rc1 are not compatible? Do anybody know from such problems?

No. If there is a problem it needs fixing. :)

Quoted 5 lines

If I revert commit 604b592e6fd3c98f21435e1181ba7723ffc24715 in Linux v3.16-rc1 only (so I have the same code in U-Boot at this place), I can boot Linux with the ubifs based rootfs, and after a reboot the U-Boot ubi attach time is short ... all works fine, as I can see it ...

So, Linux can no longer attach UBI Fastmap images which are older than 3.16-rc1? How odd. :(

Attaching newer ones works?

Quoted 29 lines

If I make in U-Boot and Linux in following patch [1]:

$ git diff

drivers/mtd/ubi/fastmap.c +1 -1

index b04e7d0..72f39da 100644
@@ -125,7 +125,7 @@ static struct ubi_ainf_volume *add_vol(struct ubi_attach_info *ai, int vol_id,
                parent = *p;
                av = rb_entry(parent, struct ubi_ainf_volume, rb);
 
-               if (vol_id < av->vol_id)
+               if (vol_id > av->vol_id)
                        p = &(*p)->rb_left;
                else
                        p = &(*p)->rb_right;

I can boot the ubifs based rootfs (burned with U-Boot), and after reboot to U-Boot U-Boot ubi attach works again fine.

As in process_pool_aeb() looks:
[...]

               if (be32_to_cpu(new_vh->vol_id) > tmp_av->vol_id)
                        p = &(*p)->rb_left;
                else if (be32_to_cpu(new_vh->vol_id) < tmp_av->vol_id)
                        p = &(*p)->rb_right;

so, I think the old code:

Quoted 7 lines

                if (vol_id > av->vol_id)
                        p = &(*p)->rb_left;
                else if (vol_id > av->vol_id)
                        p = &(*p)->rb_right;

is an copy&paste error ... and I think the right fix should be[1] ...

Forgot to attach it?

Thanks,
//richard

Richard Weinberger

author maintainer

2014-06-23 12:41 UTC #3 lore raw

In reply to a message not in the archive

Am 23.06.2014 13:04, schrieb Heiko Schocher:

Quoted 26 lines

I tried this patch also in the U-Boot code, without getting a ubifs rootfs (burned with u-boot into the ubi volume) bootable in linux ... which worked before... maybe v3.14 and v3.16-rc1 are not compatible? Do anybody know from such problems?
If I revert commit 604b592e6fd3c98f21435e1181ba7723ffc24715 in Linux v3.16-rc1 only (so I have the same code in U-Boot at this place), I can boot Linux with the ubifs based rootfs, and after a reboot the U-Boot ubi attach time is short ... all works fine, as I can see it ...
If I make in U-Boot and Linux in following patch [1]:
$ git diff
drivers/mtd/ubi/fastmap.c +1 -1
index b04e7d0..72f39da 100644
@@ -125,7 +125,7 @@ static struct ubi_ainf_volume *add_vol(struct ubi_attach_info *ai, int vol_id,
                parent = *p;
                av = rb_entry(parent, struct ubi_ainf_volume, rb);
 
-               if (vol_id < av->vol_id)
+               if (vol_id > av->vol_id)
                        p = &(*p)->rb_left;
                else
                        p = &(*p)->rb_right;

Quoted 1 line

$

"Found" your patch. ;)
Looks like I need new glasses.

Yes, it looks correct as the logic is now balanced with the logic in ubi_find_av().

Thanks,
//richard

Heiko Schocher

2014-06-23 13:05 UTC #4 lore raw

Hello Richard,

Am 23.06.2014 14:41, schrieb Richard Weinberger:

Quoted 22 lines

Am 23.06.2014 13:04, schrieb Heiko Schocher:
Quoted 21 lines
I tried this patch also in the U-Boot code, without getting a ubifs rootfs (burned with u-boot into the ubi volume) bootable in linux ... which worked before... maybe v3.14 and v3.16-rc1 are not compatible? Do anybody know from such problems?
If I revert commit 604b592e6fd3c98f21435e1181ba7723ffc24715 in Linux v3.16-rc1 only (so I have the same code in U-Boot at this place), I can boot Linux with the ubifs based rootfs, and after a reboot the U-Boot ubi attach time is short ... all works fine, as I can see it ...
If I make in U-Boot and Linux in following patch [1]:
$ git diff
drivers/mtd/ubi/fastmap.c +0 -0
index b04e7d0..72f39da 100644
@@ -125,7 +125,7 @@ static struct ubi_ainf_volume *add_vol(struct ubi_attach_info *ai, int vol_id,
                 parent = *p;
                 av = rb_entry(parent, struct ubi_ainf_volume, rb);
 

Quoted 9 lines

Quoted 6 lines

drivers/mtd/ubi/fastmap.c +1 -1

-               if (vol_id<  av->vol_id)
+               if (vol_id>  av->vol_id)
                         p =&(*p)->rb_left;
                 else
                         p =&(*p)->rb_right;

"Found" your patch. ;)
Looks like I need new glasses.

;-)

Quoted 1 line

Yes, it looks correct as the logic is now balanced with the logic in ubi_find_av().

Ah, yes, right! So should I post a correct patch for this?

bye,
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Richard Weinberger

author maintainer

2014-06-23 19:07 UTC #5 lore raw

Am 23.06.2014 15:05, schrieb Heiko Schocher:

Quoted 35 lines

Hello Richard,
Am 23.06.2014 14:41, schrieb Richard Weinberger:
Quoted 22 lines
Am 23.06.2014 13:04, schrieb Heiko Schocher:
Quoted 21 lines
I tried this patch also in the U-Boot code, without getting a ubifs rootfs (burned with u-boot into the ubi volume) bootable in linux ... which worked before... maybe v3.14 and v3.16-rc1 are not compatible? Do anybody know from such problems?
If I revert commit 604b592e6fd3c98f21435e1181ba7723ffc24715 in Linux v3.16-rc1 only (so I have the same code in U-Boot at this place), I can boot Linux with the ubifs based rootfs, and after a reboot the U-Boot ubi attach time is short ... all works fine, as I can see it ...
If I make in U-Boot and Linux in following patch [1]:
$ git diff
drivers/mtd/ubi/fastmap.c +0 -0
index b04e7d0..72f39da 100644
@@ -125,7 +125,7 @@ static struct ubi_ainf_volume *add_vol(struct ubi_attach_info *ai, int vol_id,
                 parent = *p;
                 av = rb_entry(parent, struct ubi_ainf_volume, rb);
 
Quoted 9 lines
Quoted 6 lines
drivers/mtd/ubi/fastmap.c +1 -1
-               if (vol_id<  av->vol_id)
+               if (vol_id>  av->vol_id)
                         p =&(*p)->rb_left;
                 else
                         p =&(*p)->rb_right;
$
"Found" your patch. ;)
Looks like I need new glasses.

Quoted 5 lines

;-)
Quoted 1 line
Yes, it looks correct as the logic is now balanced with the logic in ubi_find_av().
Ah, yes, right! So should I post a correct patch for this?

I think it would be better to "fix" the whole logic such that low volume ids go into the left rb node instead of the right.
Just to avoid further confusion.
Artem, do you know why UBI does it the other way around for volume ids?

Thanks,
//richard

Artem Bityutskiy

2014-06-24 05:00 UTC #6 lore raw

Quoted 4 lines · Richard Weinberger

I think it would be better to "fix" the whole logic such that low volume ids go into the left rb node instead of the right.
Just to avoid further confusion.
Artem, do you know why UBI does it the other way around for volume ids?

I do not remember. Probably no particular reason.

I'd suggest to do a quick fix first and submit to Linus for 3.16, and then people can do whatever logic streamlining for 3.17, makes sense?

Signature · 2 lines

Best Regards,
Artem Bityutskiy

Heiko Schocher

2014-06-24 05:26 UTC #7 lore raw

Hello Artem,

Am 24.06.2014 06:59, schrieb Artem Bityutskiy:

Quoted 10 lines · Richard Weinberger

Quoted 4 lines
I think it would be better to "fix" the whole logic such that low volume ids go into the left rb node instead of the right.
Just to avoid further confusion.
Artem, do you know why UBI does it the other way around for volume ids?
I do not remember. Probably no particular reason.
I'd suggest to do a quick fix first and submit to Linus for 3.16, and then people can do whatever logic streamlining for 3.17, makes sense?

I posted such a patch, but for some reason, I could not find it in
http://lists.infradead.org/pipermail/linux-mtd/2014-June/date.html

Here an extract from the header I got, as I was on Cc ...

From: Heiko Schocher <[email protected]>
To: [email protected]
Cc: Heiko Schocher <[email protected]>,
Artem Bityutskiy <[email protected]>, Richard Weinberger <[email protected]>,
David Woodhouse <[email protected]>,
Brian Norris <[email protected]>,
Mike Snitzer <[email protected]>,
Wolfgang Denk <[email protected]>, [email protected]
Subject: [PATCH] ubi: fix correct rb_tree node comparison in add_vol
Date: Mon, 23 Jun 2014 15:27:09 +0200
Message-Id: <[email protected]>
X-Mailer: git-send-email 1.8.3.1

...

bye,
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Artem Bityutskiy

2014-06-24 05:34 UTC #8 lore raw

Quoted 19 lines · Heiko Schocher

Hello Artem,
Am 24.06.2014 06:59, schrieb Artem Bityutskiy:
Quoted 10 lines · Richard Weinberger
Quoted 4 lines
I think it would be better to "fix" the whole logic such that low volume ids go into the left rb node instead of the right.
Just to avoid further confusion.
Artem, do you know why UBI does it the other way around for volume ids?
I do not remember. Probably no particular reason.
I'd suggest to do a quick fix first and submit to Linus for 3.16, and then people can do whatever logic streamlining for 3.17, makes sense?
I posted such a patch, but for some reason, I could not find it in
http://lists.infradead.org/pipermail/linux-mtd/2014-June/date.html
Here an extract from the header I got, as I was on Cc ...

Quoted 14 lines

From: Heiko Schocher <[email protected]>
To: [email protected]
Cc: Heiko Schocher <[email protected]>,
Artem Bityutskiy <[email protected]>, Richard Weinberger <[email protected]>,
David Woodhouse <[email protected]>,
Brian Norris <[email protected]>,
Mike Snitzer <[email protected]>,
Wolfgang Denk <[email protected]>, [email protected]
Subject: [PATCH] ubi: fix correct rb_tree node comparison in add_vol
Date: Mon, 23 Jun 2014 15:27:09 +0200
Message-Id: <[email protected]>
X-Mailer: git-send-email 1.8.3.1

I do not see it in my @linux.intel.com mailbox either, probably something went wrong on your side?

Signature · 2 lines

Best Regards,
Artem Bityutskiy

Richard Weinberger

author maintainer

2014-06-24 06:53 UTC #9 lore raw

Am 24.06.2014 07:31, schrieb Artem Bityutskiy:

Quoted 35 lines · Heiko Schocher

Quoted 19 lines
Hello Artem,
Am 24.06.2014 06:59, schrieb Artem Bityutskiy:
Quoted 10 lines · Richard Weinberger
Quoted 4 lines
I think it would be better to "fix" the whole logic such that low volume ids go into the left rb node instead of the right.
Just to avoid further confusion.
Artem, do you know why UBI does it the other way around for volume ids?
I do not remember. Probably no particular reason.
I'd suggest to do a quick fix first and submit to Linus for 3.16, and then people can do whatever logic streamlining for 3.17, makes sense?
I posted such a patch, but for some reason, I could not find it in
http://lists.infradead.org/pipermail/linux-mtd/2014-June/date.html
Here an extract from the header I got, as I was on Cc ...
Quoted 14 lines
From: Heiko Schocher <[email protected]>
To: [email protected]
Cc: Heiko Schocher <[email protected]>,
Artem Bityutskiy <[email protected]>, Richard Weinberger <[email protected]>,
David Woodhouse <[email protected]>,
Brian Norris <[email protected]>,
Mike Snitzer <[email protected]>,
Wolfgang Denk <[email protected]>, [email protected]
Subject: [PATCH] ubi: fix correct rb_tree node comparison in add_vol
Date: Mon, 23 Jun 2014 15:27:09 +0200
Message-Id: <[email protected]>
X-Mailer: git-send-email 1.8.3.1

Quoted 2 lines

I do not see it in my @linux.intel.com mailbox either, probably something went wrong on your side?

Same here.

Thanks,
//richard

Richard Weinberger

author maintainer

2014-06-24 06:53 UTC #10 lore raw

In reply to Artem Bityutskiy

Am 24.06.2014 06:59, schrieb Artem Bityutskiy:

Quoted 10 lines · Richard Weinberger

Quoted 4 lines
I think it would be better to "fix" the whole logic such that low volume ids go into the left rb node instead of the right.
Just to avoid further confusion.
Artem, do you know why UBI does it the other way around for volume ids?
I do not remember. Probably no particular reason.
I'd suggest to do a quick fix first and submit to Linus for 3.16, and then people can do whatever logic streamlining for 3.17, makes sense?

Agreed!

Thanks,
//richard