Re: UBI: fix rb_tree node comparison in add_map commit buggy?

Re: UBI: fix rb_tree node comparison in add_map commit buggy?

[Richard Weinberger]

Hi!

Am 23.06.2014 13:04, schrieb Heiko Schocher:
> 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 <snitzer@redhat.com>
> 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 <snitzer@redhat.com>
>         Acked-by: Richard Weinberger <richard@nod.at>
>         Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 
> 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)
> 
> 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. :)

> 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?

> If I make in U-Boot and Linux in following patch [1]:
> $ git diff
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index b04e7d0..72f39da 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -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:
> 
>                 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

Re: UBI: fix rb_tree node comparison in add_map commit buggy?

[Richard Weinberger]

Am 23.06.2014 13:04, schrieb Heiko Schocher:
> 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
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index b04e7d0..72f39da 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -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;
> $

"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

Re: UBI: fix rb_tree node comparison in add_map commit buggy?

[Heiko Schocher]

Hello Richard,

Am 23.06.2014 14:41, schrieb Richard Weinberger:
> Am 23.06.2014 13:04, schrieb Heiko Schocher:
>> 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
>> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
>> index b04e7d0..72f39da 100644
>> --- a/drivers/mtd/ubi/fastmap.c
>> +++ b/drivers/mtd/ubi/fastmap.c
>> @@ -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;
>> $
>
> "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().

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

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

Re: UBI: fix rb_tree node comparison in add_map commit buggy?

[Richard Weinberger]

Am 23.06.2014 15:05, schrieb Heiko Schocher:
> Hello Richard,
> 
> Am 23.06.2014 14:41, schrieb Richard Weinberger:
>> Am 23.06.2014 13:04, schrieb Heiko Schocher:
>>> 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
>>> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
>>> index b04e7d0..72f39da 100644
>>> --- a/drivers/mtd/ubi/fastmap.c
>>> +++ b/drivers/mtd/ubi/fastmap.c
>>> @@ -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;
>>> $
>>
>> "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().
> 
> 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

Re: UBI: fix rb_tree node comparison in add_map commit buggy?

[Artem Bityutskiy]

On Mon, 2014-06-23 at 21:07 +0200, Richard Weinberger wrote:
> 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?

-- 
Best Regards,
Artem Bityutskiy

Re: UBI: fix rb_tree node comparison in add_map commit buggy?

[Heiko Schocher]

Hello Artem,

Am 24.06.2014 06:59, schrieb Artem Bityutskiy:
> On Mon, 2014-06-23 at 21:07 +0200, Richard Weinberger wrote:
>> 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 <hs@denx.de>
To: linux-mtd@lists.infradead.org
Cc: Heiko Schocher <hs@denx.de>,
	Artem Bityutskiy <artem.bityutskiy@linux.intel.com>,
	Richard Weinberger <richard@nod.at>,
	David Woodhouse <dwmw2@infradead.org>,
	Brian Norris <computersforpeace@gmail.com>,
	Mike Snitzer <snitzer@redhat.com>,
	Wolfgang Denk <wd@denx.de>,
	linux-kernel@vger.kernel.org
Subject: [PATCH] ubi: fix correct rb_tree node comparison in add_vol
Date: Mon, 23 Jun 2014 15:27:09 +0200
Message-Id: <1403530029-12151-1-git-send-email-hs@denx.de>
X-Mailer: git-send-email 1.8.3.1

...

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

Re: UBI: fix rb_tree node comparison in add_map commit buggy?

[Artem Bityutskiy]

On Tue, 2014-06-24 at 07:26 +0200, Heiko Schocher wrote:
> Hello Artem,
> 
> Am 24.06.2014 06:59, schrieb Artem Bityutskiy:
> > On Mon, 2014-06-23 at 21:07 +0200, Richard Weinberger wrote:
> >> 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 <hs@denx.de>
> To: linux-mtd@lists.infradead.org
> Cc: Heiko Schocher <hs@denx.de>,
> 	Artem Bityutskiy <artem.bityutskiy@linux.intel.com>,
> 	Richard Weinberger <richard@nod.at>,
> 	David Woodhouse <dwmw2@infradead.org>,
> 	Brian Norris <computersforpeace@gmail.com>,
> 	Mike Snitzer <snitzer@redhat.com>,
> 	Wolfgang Denk <wd@denx.de>,
> 	linux-kernel@vger.kernel.org
> Subject: [PATCH] ubi: fix correct rb_tree node comparison in add_vol
> Date: Mon, 23 Jun 2014 15:27:09 +0200
> Message-Id: <1403530029-12151-1-git-send-email-hs@denx.de>
> 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?

-- 
Best Regards,
Artem Bityutskiy

Re: UBI: fix rb_tree node comparison in add_map commit buggy?

[Richard Weinberger]

Am 24.06.2014 07:31, schrieb Artem Bityutskiy:
> On Tue, 2014-06-24 at 07:26 +0200, Heiko Schocher wrote:
>> Hello Artem,
>>
>> Am 24.06.2014 06:59, schrieb Artem Bityutskiy:
>>> On Mon, 2014-06-23 at 21:07 +0200, Richard Weinberger wrote:
>>>> 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 <hs@denx.de>
>> To: linux-mtd@lists.infradead.org
>> Cc: Heiko Schocher <hs@denx.de>,
>> 	Artem Bityutskiy <artem.bityutskiy@linux.intel.com>,
>> 	Richard Weinberger <richard@nod.at>,
>> 	David Woodhouse <dwmw2@infradead.org>,
>> 	Brian Norris <computersforpeace@gmail.com>,
>> 	Mike Snitzer <snitzer@redhat.com>,
>> 	Wolfgang Denk <wd@denx.de>,
>> 	linux-kernel@vger.kernel.org
>> Subject: [PATCH] ubi: fix correct rb_tree node comparison in add_vol
>> Date: Mon, 23 Jun 2014 15:27:09 +0200
>> Message-Id: <1403530029-12151-1-git-send-email-hs@denx.de>
>> 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?

Same here.

Thanks,
//richard

Re: UBI: fix rb_tree node comparison in add_map commit buggy?

[Richard Weinberger]

Am 24.06.2014 06:59, schrieb Artem Bityutskiy:
> On Mon, 2014-06-23 at 21:07 +0200, Richard Weinberger wrote:
>> 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