2014-10-25 18:39:26

by Richard Weinberger

[permalink] [raw]
Subject: [PATCH] UBI: vtbl: Use ubi_eba_atomic_leb_change()

This is more a cosmetic change than a fix.
By using ubi_eba_atomic_leb_change()
we can guarantee that the first VTBL record is always
correct and we don't really need the second one anymore.
But we have to keep the second one to not break anything.

Signed-off-by: Richard Weinberger <[email protected]>
---
drivers/mtd/ubi/vtbl.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
index 07cac5f..0d26051 100644
--- a/drivers/mtd/ubi/vtbl.c
+++ b/drivers/mtd/ubi/vtbl.c
@@ -96,12 +96,8 @@ int ubi_change_vtbl_record(struct ubi_device *ubi, int idx,

memcpy(&ubi->vtbl[idx], vtbl_rec, sizeof(struct ubi_vtbl_record));
for (i = 0; i < UBI_LAYOUT_VOLUME_EBS; i++) {
- err = ubi_eba_unmap_leb(ubi, layout_vol, i);
- if (err)
- return err;
-
- err = ubi_eba_write_leb(ubi, layout_vol, i, ubi->vtbl, 0,
- ubi->vtbl_size);
+ err = ubi_eba_atomic_leb_change(ubi, layout_vol, i, ubi->vtbl,
+ ubi->vtbl_size);
if (err)
return err;
}
@@ -148,12 +144,8 @@ int ubi_vtbl_rename_volumes(struct ubi_device *ubi,

layout_vol = ubi->volumes[vol_id2idx(ubi, UBI_LAYOUT_VOLUME_ID)];
for (i = 0; i < UBI_LAYOUT_VOLUME_EBS; i++) {
- err = ubi_eba_unmap_leb(ubi, layout_vol, i);
- if (err)
- return err;
-
- err = ubi_eba_write_leb(ubi, layout_vol, i, ubi->vtbl, 0,
- ubi->vtbl_size);
+ err = ubi_eba_atomic_leb_change(ubi, layout_vol, i, ubi->vtbl,
+ ubi->vtbl_size);
if (err)
return err;
}
--
1.8.4.5


2014-10-30 08:55:55

by Artem Bityutskiy

[permalink] [raw]
Subject: Re: [PATCH] UBI: vtbl: Use ubi_eba_atomic_leb_change()

On Sat, 2014-10-25 at 19:43 +0200, Richard Weinberger wrote:
> This is more a cosmetic change than a fix.
> By using ubi_eba_atomic_leb_change()
> we can guarantee that the first VTBL record is always
> correct and we don't really need the second one anymore.
> But we have to keep the second one to not break anything.
>
> Signed-off-by: Richard Weinberger <[email protected]>

Yeah, this atomic change stuff was added later, and we had not
envisioned originally. Your patch adds robustness, but makes volume
creation slower, which is probably not a problem.

I've added a small comment and pushed it, thanks!

Artem

2014-10-31 04:04:11

by hujianyang

[permalink] [raw]
Subject: Re: [PATCH] UBI: vtbl: Use ubi_eba_atomic_leb_change()

On 2014/10/30 16:55, Artem Bityutskiy wrote:
> On Sat, 2014-10-25 at 19:43 +0200, Richard Weinberger wrote:
>> This is more a cosmetic change than a fix.
>> By using ubi_eba_atomic_leb_change()
>> we can guarantee that the first VTBL record is always
>> correct and we don't really need the second one anymore.
>> But we have to keep the second one to not break anything.
>>
>> Signed-off-by: Richard Weinberger <[email protected]>
>
> Yeah, this atomic change stuff was added later, and we had not
> envisioned originally. Your patch adds robustness, but makes volume
> creation slower, which is probably not a problem.
>
> I've added a small comment and pushed it, thanks!
>
> Artem
>
>

Hi Artem and Richard,

We are using atomic operation, leb_change(), for master_node
in ubifs-level. We use two lebs for master_node even if they
are changed with atomic operation.

I think volume_table and master_node play similar roles. Do
you think changing VTBL record into one peb is OK? I just
what to know if I missed something. Could you please take
some time to explain that?

Thanks very much~!

Hu

2014-10-31 08:09:55

by Richard Weinberger

[permalink] [raw]
Subject: Re: [PATCH] UBI: vtbl: Use ubi_eba_atomic_leb_change()

Hujianyang,

Am 31.10.2014 um 05:03 schrieb hujianyang:
> Hi Artem and Richard,
>
> We are using atomic operation, leb_change(), for master_node
> in ubifs-level. We use two lebs for master_node even if they
> are changed with atomic operation.
>
> I think volume_table and master_node play similar roles. Do
> you think changing VTBL record into one peb is OK? I just
> what to know if I missed something. Could you please take
> some time to explain that?

I'm not sure if I correctly understand your question.

If we use only one PEB for the VTBL existing UBI implementations
would break as they assume we have two.

Thanks,
//richard

2014-10-31 10:45:29

by hujianyang

[permalink] [raw]
Subject: Re: [PATCH] UBI: vtbl: Use ubi_eba_atomic_leb_change()

On 2014/10/31 16:09, Richard Weinberger wrote:
> Hujianyang,
>
> Am 31.10.2014 um 05:03 schrieb hujianyang:
>> Hi Artem and Richard,
>>
>> We are using atomic operation, leb_change(), for master_node
>> in ubifs-level. We use two lebs for master_node even if they
>> are changed with atomic operation.
>>
>> I think volume_table and master_node play similar roles. Do
>> you think changing VTBL record into one peb is OK? I just
>> what to know if I missed something. Could you please take
>> some time to explain that?
>
> I'm not sure if I correctly understand your question.
>
> If we use only one PEB for the VTBL existing UBI implementations
> would break as they assume we have two.
>
> Thanks,
> //richard
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
>

This question is basing on your comment for this patch:

"""
we can guarantee that the first VTBL record is always
correct and we don't really need the second one anymore.
"""

I think that means one PEB is enough in your considering.
So I want to know if you are sure about this. Because
we use two leb for master_node in ubifs-level. So maybe
VTBL is like super_node, not master_node, right?

2014-10-31 10:57:42

by Richard Weinberger

[permalink] [raw]
Subject: Re: [PATCH] UBI: vtbl: Use ubi_eba_atomic_leb_change()

Am 31.10.2014 um 11:45 schrieb hujianyang:
> This question is basing on your comment for this patch:
>
> """
> we can guarantee that the first VTBL record is always
> correct and we don't really need the second one anymore.
> """
>
> I think that means one PEB is enough in your considering.
> So I want to know if you are sure about this. Because
> we use two leb for master_node in ubifs-level. So maybe
> VTBL is like super_node, not master_node, right?

Yes, technically one PEB is enough if atomic leb change is used.
But existing UBI implementations want a second one
and a backup VTBL PEB is good for robustness.
i.e. if the PEB turns bad we have a backup and do not lose
all volume meta information.

Thanks,
//richard