* UBI fastmap updates
@ 2013-09-28 13:55 Richard Weinberger
2013-09-28 13:55 ` [PATCH 1/7] UBI: fix refill_wl_user_pool() Richard Weinberger
` (7 more replies)
0 siblings, 8 replies; 19+ messages in thread
From: Richard Weinberger @ 2013-09-28 13:55 UTC (permalink / raw)
To: dedekind1; +Cc: richard.genoud, linux-mtd, linux-kernel
this series is a collection of all pending UBI fastmap updates.
Richard Genoud found issues in combination with U-Boot.
These issues also uncovered other minor issues in some error paths.
Only one fix is not really fastmap specific and touches generic
UBI code, "UBI: simplify image sequence test".
Thanks,
//richard
Richard Genoud (2):
UBI: fastmap: fix backward compatibility with image_seq
UBI: simplify image sequence test
Richard Weinberger (5):
UBI: fix refill_wl_user_pool()
UBI: Fix error path in scan_pool()
UBI: Call scan_all() with correct offset in error case
UBI: Fix memory leak in ubi_attach_fastmap() error path
UBI: Add some asserts to ubi_attach_fastmap()
drivers/mtd/ubi/attach.c | 11 ++++++-----
drivers/mtd/ubi/fastmap.c | 41 +++++++++++++++++++++++++++++++++++++----
drivers/mtd/ubi/wl.c | 4 ----
3 files changed, 43 insertions(+), 13 deletions(-)
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/7] UBI: fix refill_wl_user_pool()
2013-09-28 13:55 UBI fastmap updates Richard Weinberger
@ 2013-09-28 13:55 ` Richard Weinberger
2013-10-03 15:00 ` Artem Bityutskiy
2013-09-28 13:55 ` [PATCH 2/7] UBI: Fix error path in scan_pool() Richard Weinberger
` (6 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Richard Weinberger @ 2013-09-28 13:55 UTC (permalink / raw)
To: dedekind1; +Cc: richard.genoud, linux-mtd, linux-kernel, Richard Weinberger
If no free PEBs are available refill_wl_user_pool() must not
return with -ENOSPC immediately.
It has to block till produce_free_peb() produced a free PEB.
Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
drivers/mtd/ubi/wl.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index c95bfb1..02317c1 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -599,10 +599,6 @@ static void refill_wl_user_pool(struct ubi_device *ubi)
return_unused_pool_pebs(ubi, pool);
for (pool->size = 0; pool->size < pool->max_size; pool->size++) {
- if (!ubi->free.rb_node ||
- (ubi->free_count - ubi->beb_rsvd_pebs < 1))
- break;
-
pool->pebs[pool->size] = __wl_get_peb(ubi);
if (pool->pebs[pool->size] < 0)
break;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/7] UBI: Fix error path in scan_pool()
2013-09-28 13:55 UBI fastmap updates Richard Weinberger
2013-09-28 13:55 ` [PATCH 1/7] UBI: fix refill_wl_user_pool() Richard Weinberger
@ 2013-09-28 13:55 ` Richard Weinberger
2013-09-28 13:55 ` [PATCH 3/7] UBI: Call scan_all() with correct offset in error case Richard Weinberger
` (5 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Richard Weinberger @ 2013-09-28 13:55 UTC (permalink / raw)
To: dedekind1; +Cc: richard.genoud, linux-mtd, linux-kernel, Richard Weinberger
We have to set "ret", not "err" in case of an error.
Reported-and-tested-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
drivers/mtd/ubi/fastmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index f5aa4b0..9b42add 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -428,7 +428,7 @@ static int scan_pool(struct ubi_device *ubi, struct ubi_attach_info *ai,
if (be32_to_cpu(ech->image_seq) != ubi->image_seq) {
ubi_err("bad image seq: 0x%x, expected: 0x%x",
be32_to_cpu(ech->image_seq), ubi->image_seq);
- err = UBI_BAD_FASTMAP;
+ ret = UBI_BAD_FASTMAP;
goto out;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/7] UBI: Call scan_all() with correct offset in error case
2013-09-28 13:55 UBI fastmap updates Richard Weinberger
2013-09-28 13:55 ` [PATCH 1/7] UBI: fix refill_wl_user_pool() Richard Weinberger
2013-09-28 13:55 ` [PATCH 2/7] UBI: Fix error path in scan_pool() Richard Weinberger
@ 2013-09-28 13:55 ` Richard Weinberger
2013-09-28 13:55 ` [PATCH 4/7] UBI: fastmap: fix backward compatibility with image_seq Richard Weinberger
` (4 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Richard Weinberger @ 2013-09-28 13:55 UTC (permalink / raw)
To: dedekind1; +Cc: richard.genoud, linux-mtd, linux-kernel, Richard Weinberger
If we find an invalid fastmap we have to scan from the very beginning.
Otherwise we leak the first 64 PEBs.
Reported-and-tested-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
drivers/mtd/ubi/attach.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index c071d41..03b32b0 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -1417,9 +1417,11 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
ai = alloc_ai("ubi_aeb_slab_cache2");
if (!ai)
return -ENOMEM;
- }
- err = scan_all(ubi, ai, UBI_FM_MAX_START);
+ err = scan_all(ubi, ai, 0);
+ } else {
+ err = scan_all(ubi, ai, UBI_FM_MAX_START);
+ }
}
}
#else
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/7] UBI: fastmap: fix backward compatibility with image_seq
2013-09-28 13:55 UBI fastmap updates Richard Weinberger
` (2 preceding siblings ...)
2013-09-28 13:55 ` [PATCH 3/7] UBI: Call scan_all() with correct offset in error case Richard Weinberger
@ 2013-09-28 13:55 ` Richard Weinberger
2013-09-28 13:55 ` [PATCH 5/7] UBI: simplify image sequence test Richard Weinberger
` (3 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Richard Weinberger @ 2013-09-28 13:55 UTC (permalink / raw)
To: dedekind1; +Cc: richard.genoud, linux-mtd, linux-kernel, Richard Weinberger
From: Richard Genoud <richard.genoud@gmail.com>
Some old UBI implementations (e.g. U-Boot) have not implemented the image
sequence feature.
So, when erase blocks are written, the image sequence in the ec header
is lost (set to zero).
UBI scan_all() takes this case into account (commits
32bc4820287a1a03982979515949e8ea56eac641 and
2eadaad67b2b6bd132eda105128d2d466298b8e3)
But fastmap scan functions (ubi_scan_fastmap() and scan_pool()) didn't.
This patch fixes the issue.
Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
drivers/mtd/ubi/fastmap.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 9b42add..05067f5 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -407,6 +407,7 @@ static int scan_pool(struct ubi_device *ubi, struct ubi_attach_info *ai,
*/
for (i = 0; i < pool_size; i++) {
int scrub = 0;
+ int image_seq;
pnum = be32_to_cpu(pebs[i]);
@@ -425,7 +426,13 @@ static int scan_pool(struct ubi_device *ubi, struct ubi_attach_info *ai,
} else if (ret == UBI_IO_BITFLIPS)
scrub = 1;
- if (be32_to_cpu(ech->image_seq) != ubi->image_seq) {
+ /*
+ * Older UBI implementations have image_seq set to zero, so
+ * we shouldn't fail if image_seq == 0.
+ */
+ image_seq = be32_to_cpu(ech->image_seq);
+
+ if (image_seq && (image_seq != ubi->image_seq)) {
ubi_err("bad image seq: 0x%x, expected: 0x%x",
be32_to_cpu(ech->image_seq), ubi->image_seq);
ret = UBI_BAD_FASTMAP;
@@ -923,6 +930,8 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
}
for (i = 0; i < used_blocks; i++) {
+ int image_seq;
+
pnum = be32_to_cpu(fmsb->block_loc[i]);
if (ubi_io_is_bad(ubi, pnum)) {
@@ -940,10 +949,17 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
} else if (ret == UBI_IO_BITFLIPS)
fm->to_be_tortured[i] = 1;
+ image_seq = be32_to_cpu(ech->image_seq);
if (!ubi->image_seq)
- ubi->image_seq = be32_to_cpu(ech->image_seq);
+ ubi->image_seq = image_seq;
- if (be32_to_cpu(ech->image_seq) != ubi->image_seq) {
+ /*
+ * Older UBI implementations have image_seq set to zero, so
+ * we shouldn't fail if image_seq == 0.
+ */
+ if (image_seq && (image_seq != ubi->image_seq)) {
+ ubi_err("wrong image seq:%d instead of %d",
+ be32_to_cpu(ech->image_seq), ubi->image_seq);
ret = UBI_BAD_FASTMAP;
goto free_hdr;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 5/7] UBI: simplify image sequence test
2013-09-28 13:55 UBI fastmap updates Richard Weinberger
` (3 preceding siblings ...)
2013-09-28 13:55 ` [PATCH 4/7] UBI: fastmap: fix backward compatibility with image_seq Richard Weinberger
@ 2013-09-28 13:55 ` Richard Weinberger
2013-09-28 13:55 ` [PATCH 6/7] UBI: Fix memory leak in ubi_attach_fastmap() error path Richard Weinberger
` (2 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Richard Weinberger @ 2013-09-28 13:55 UTC (permalink / raw)
To: dedekind1; +Cc: richard.genoud, linux-mtd, linux-kernel, Richard Weinberger
From: Richard Genoud <richard.genoud@gmail.com>
The test:
if (!a && b)
a = b;
can be symplified in:
if (!a)
a = b;
And there's no need to test if ubi->image_seq is not null, because if it is,
it is set to image_seq.
So, we just test if image_seq is not null.
Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
drivers/mtd/ubi/attach.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 03b32b0..33bb1f2 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -900,10 +900,9 @@ static int scan_peb(struct ubi_device *ubi, struct ubi_attach_info *ai,
* number.
*/
image_seq = be32_to_cpu(ech->image_seq);
- if (!ubi->image_seq && image_seq)
+ if (!ubi->image_seq)
ubi->image_seq = image_seq;
- if (ubi->image_seq && image_seq &&
- ubi->image_seq != image_seq) {
+ if (image_seq && ubi->image_seq != image_seq) {
ubi_err("bad image sequence number %d in PEB %d, expected %d",
image_seq, pnum, ubi->image_seq);
ubi_dump_ec_hdr(ech);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 6/7] UBI: Fix memory leak in ubi_attach_fastmap() error path
2013-09-28 13:55 UBI fastmap updates Richard Weinberger
` (4 preceding siblings ...)
2013-09-28 13:55 ` [PATCH 5/7] UBI: simplify image sequence test Richard Weinberger
@ 2013-09-28 13:55 ` Richard Weinberger
2013-09-30 8:13 ` Richard Genoud
2013-09-28 13:55 ` [PATCH 7/7] UBI: Add some asserts to ubi_attach_fastmap() Richard Weinberger
2013-10-03 16:44 ` UBI fastmap updates Artem Bityutskiy
7 siblings, 1 reply; 19+ messages in thread
From: Richard Weinberger @ 2013-09-28 13:55 UTC (permalink / raw)
To: dedekind1; +Cc: richard.genoud, linux-mtd, linux-kernel, Richard Weinberger
On error we have to free all three temporary lists.
Reported-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
drivers/mtd/ubi/fastmap.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 05067f5..4cfc8da 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -841,6 +841,19 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
fail_bad:
ret = UBI_BAD_FASTMAP;
fail:
+ list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &used, u.list) {
+ kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
+ list_del(&tmp_aeb->u.list);
+ }
+ list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &eba_orphans, u.list) {
+ kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
+ list_del(&tmp_aeb->u.list);
+ }
+ list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &free, u.list) {
+ kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
+ list_del(&tmp_aeb->u.list);
+ }
+
return ret;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 7/7] UBI: Add some asserts to ubi_attach_fastmap()
2013-09-28 13:55 UBI fastmap updates Richard Weinberger
` (5 preceding siblings ...)
2013-09-28 13:55 ` [PATCH 6/7] UBI: Fix memory leak in ubi_attach_fastmap() error path Richard Weinberger
@ 2013-09-28 13:55 ` Richard Weinberger
2013-09-30 8:16 ` Richard Genoud
2013-10-03 16:44 ` UBI fastmap updates Artem Bityutskiy
7 siblings, 1 reply; 19+ messages in thread
From: Richard Weinberger @ 2013-09-28 13:55 UTC (permalink / raw)
To: dedekind1; +Cc: richard.genoud, linux-mtd, linux-kernel, Richard Weinberger
Add more paranioa asserts to make it easier to detect
implementation errors.
Signed-off-by: Richard Weinberger <richard@nod.at>
---
drivers/mtd/ubi/fastmap.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 4cfc8da..ead8613 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -826,6 +826,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &free, u.list)
list_move_tail(&tmp_aeb->u.list, &ai->free);
+ ubi_assert(list_empty(&used));
+ ubi_assert(list_empty(&eba_orphans));
+ ubi_assert(list_empty(&free));
+
/*
* If fastmap is leaking PEBs (must not happen), raise a
* fat warning and fall back to scanning mode.
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 6/7] UBI: Fix memory leak in ubi_attach_fastmap() error path
2013-09-28 13:55 ` [PATCH 6/7] UBI: Fix memory leak in ubi_attach_fastmap() error path Richard Weinberger
@ 2013-09-30 8:13 ` Richard Genoud
0 siblings, 0 replies; 19+ messages in thread
From: Richard Genoud @ 2013-09-30 8:13 UTC (permalink / raw)
To: Richard Weinberger; +Cc: Artem Bityutskiy, linux-mtd, linux-kernel
2013/9/28 Richard Weinberger <richard@nod.at>:
> On error we have to free all three temporary lists.
>
> Reported-by: Richard Genoud <richard.genoud@gmail.com>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> drivers/mtd/ubi/fastmap.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index 05067f5..4cfc8da 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -841,6 +841,19 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
> fail_bad:
> ret = UBI_BAD_FASTMAP;
> fail:
> + list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &used, u.list) {
> + kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
> + list_del(&tmp_aeb->u.list);
> + }
> + list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &eba_orphans, u.list) {
> + kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
> + list_del(&tmp_aeb->u.list);
> + }
> + list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &free, u.list) {
> + kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
> + list_del(&tmp_aeb->u.list);
> + }
> +
> return ret;
> }
>
> --
> 1.8.3.1
>
Works like a charm !
Tested-by: Richard Genoud <richard.genoud@gmail.com>
Thanks !
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 7/7] UBI: Add some asserts to ubi_attach_fastmap()
2013-09-28 13:55 ` [PATCH 7/7] UBI: Add some asserts to ubi_attach_fastmap() Richard Weinberger
@ 2013-09-30 8:16 ` Richard Genoud
0 siblings, 0 replies; 19+ messages in thread
From: Richard Genoud @ 2013-09-30 8:16 UTC (permalink / raw)
To: Richard Weinberger; +Cc: Artem Bityutskiy, linux-mtd, linux-kernel
2013/9/28 Richard Weinberger <richard@nod.at>:
> Add more paranioa asserts to make it easier to detect
> implementation errors.
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> drivers/mtd/ubi/fastmap.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index 4cfc8da..ead8613 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -826,6 +826,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
> list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &free, u.list)
> list_move_tail(&tmp_aeb->u.list, &ai->free);
>
> + ubi_assert(list_empty(&used));
> + ubi_assert(list_empty(&eba_orphans));
> + ubi_assert(list_empty(&free));
> +
> /*
> * If fastmap is leaking PEBs (must not happen), raise a
> * fat warning and fall back to scanning mode.
> --
> 1.8.3.1
>
Tested-by: Richard Genoud <richard.genoud@gmail.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] UBI: fix refill_wl_user_pool()
2013-09-28 13:55 ` [PATCH 1/7] UBI: fix refill_wl_user_pool() Richard Weinberger
@ 2013-10-03 15:00 ` Artem Bityutskiy
2013-10-03 15:08 ` Richard Weinberger
0 siblings, 1 reply; 19+ messages in thread
From: Artem Bityutskiy @ 2013-10-03 15:00 UTC (permalink / raw)
To: Richard Weinberger; +Cc: richard.genoud, linux-mtd, linux-kernel
On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
> If no free PEBs are available refill_wl_user_pool() must not
> return with -ENOSPC immediately.
> It has to block till produce_free_peb() produced a free PEB.
>
> Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
> Signed-off-by: Richard Weinberger <richard@nod.at>
What is pool size, I wonder?
--
Best Regards,
Artem Bityutskiy
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] UBI: fix refill_wl_user_pool()
2013-10-03 15:00 ` Artem Bityutskiy
@ 2013-10-03 15:08 ` Richard Weinberger
2013-10-03 15:27 ` Artem Bityutskiy
0 siblings, 1 reply; 19+ messages in thread
From: Richard Weinberger @ 2013-10-03 15:08 UTC (permalink / raw)
To: dedekind1; +Cc: richard.genoud, linux-mtd, linux-kernel
Am 03.10.2013 17:00, schrieb Artem Bityutskiy:
> On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
>> If no free PEBs are available refill_wl_user_pool() must not
>> return with -ENOSPC immediately.
>> It has to block till produce_free_peb() produced a free PEB.
>>
>> Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>
> What is pool size, I wonder?
Currently it's 25 (UBI_FM_WL_POOL_SIZE).
If experience shows that 25 is too low/big we can change this constant.
Maybe it's also worth making them configurable...
Thanks,
//richard
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] UBI: fix refill_wl_user_pool()
2013-10-03 15:08 ` Richard Weinberger
@ 2013-10-03 15:27 ` Artem Bityutskiy
2013-10-03 15:53 ` Richard Weinberger
0 siblings, 1 reply; 19+ messages in thread
From: Artem Bityutskiy @ 2013-10-03 15:27 UTC (permalink / raw)
To: Richard Weinberger; +Cc: richard.genoud, linux-mtd, linux-kernel
On Thu, 2013-10-03 at 17:08 +0200, Richard Weinberger wrote:
> Am 03.10.2013 17:00, schrieb Artem Bityutskiy:
> > On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
> >> If no free PEBs are available refill_wl_user_pool() must not
> >> return with -ENOSPC immediately.
> >> It has to block till produce_free_peb() produced a free PEB.
> >>
> >> Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
> >> Signed-off-by: Richard Weinberger <richard@nod.at>
> >
> > What is pool size, I wonder?
>
> Currently it's 25 (UBI_FM_WL_POOL_SIZE).
> If experience shows that 25 is too low/big we can change this constant.
> Maybe it's also worth making them configurable...
I if it is a possible scenario that this function will not return until
25 (or even 10) PEBs are erased?
--
Best Regards,
Artem Bityutskiy
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] UBI: fix refill_wl_user_pool()
2013-10-03 15:27 ` Artem Bityutskiy
@ 2013-10-03 15:53 ` Richard Weinberger
2013-10-03 16:00 ` Artem Bityutskiy
0 siblings, 1 reply; 19+ messages in thread
From: Richard Weinberger @ 2013-10-03 15:53 UTC (permalink / raw)
To: dedekind1; +Cc: richard.genoud, linux-mtd, linux-kernel
Am 03.10.2013 17:27, schrieb Artem Bityutskiy:
> On Thu, 2013-10-03 at 17:08 +0200, Richard Weinberger wrote:
>> Am 03.10.2013 17:00, schrieb Artem Bityutskiy:
>>> On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
>>>> If no free PEBs are available refill_wl_user_pool() must not
>>>> return with -ENOSPC immediately.
>>>> It has to block till produce_free_peb() produced a free PEB.
>>>>
>>>> Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
>>>> Signed-off-by: Richard Weinberger <richard@nod.at>
>>>
>>> What is pool size, I wonder?
>>
>> Currently it's 25 (UBI_FM_WL_POOL_SIZE).
>> If experience shows that 25 is too low/big we can change this constant.
>> Maybe it's also worth making them configurable...
>
> I if it is a possible scenario that this function will not return until
> 25 (or even 10) PEBs are erased?
>
Sure. It will try to gain up to 25 PEBs but return if it gets less.
That's why a pool has a current and a max size.
Thanks,
//richard
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] UBI: fix refill_wl_user_pool()
2013-10-03 15:53 ` Richard Weinberger
@ 2013-10-03 16:00 ` Artem Bityutskiy
2013-10-03 16:35 ` Richard Weinberger
0 siblings, 1 reply; 19+ messages in thread
From: Artem Bityutskiy @ 2013-10-03 16:00 UTC (permalink / raw)
To: Richard Weinberger; +Cc: richard.genoud, linux-mtd, linux-kernel
On Thu, 2013-10-03 at 17:53 +0200, Richard Weinberger wrote:
> Am 03.10.2013 17:27, schrieb Artem Bityutskiy:
> > On Thu, 2013-10-03 at 17:08 +0200, Richard Weinberger wrote:
> >> Am 03.10.2013 17:00, schrieb Artem Bityutskiy:
> >>> On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
> >>>> If no free PEBs are available refill_wl_user_pool() must not
> >>>> return with -ENOSPC immediately.
> >>>> It has to block till produce_free_peb() produced a free PEB.
> >>>>
> >>>> Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
> >>>> Signed-off-by: Richard Weinberger <richard@nod.at>
> >>>
> >>> What is pool size, I wonder?
> >>
> >> Currently it's 25 (UBI_FM_WL_POOL_SIZE).
> >> If experience shows that 25 is too low/big we can change this constant.
> >> Maybe it's also worth making them configurable...
> >
> > I if it is a possible scenario that this function will not return until
> > 25 (or even 10) PEBs are erased?
> >
>
> Sure. It will try to gain up to 25 PEBs but return if it gets less.
> That's why a pool has a current and a max size.
So if erasing speed is say, 250ms, then it would take 6.25 seconds?
--
Best Regards,
Artem Bityutskiy
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] UBI: fix refill_wl_user_pool()
2013-10-03 16:00 ` Artem Bityutskiy
@ 2013-10-03 16:35 ` Richard Weinberger
2013-10-03 16:41 ` Artem Bityutskiy
0 siblings, 1 reply; 19+ messages in thread
From: Richard Weinberger @ 2013-10-03 16:35 UTC (permalink / raw)
To: dedekind1; +Cc: richard.genoud, linux-mtd, linux-kernel
Am 03.10.2013 18:00, schrieb Artem Bityutskiy:
> On Thu, 2013-10-03 at 17:53 +0200, Richard Weinberger wrote:
>> Am 03.10.2013 17:27, schrieb Artem Bityutskiy:
>>> On Thu, 2013-10-03 at 17:08 +0200, Richard Weinberger wrote:
>>>> Am 03.10.2013 17:00, schrieb Artem Bityutskiy:
>>>>> On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
>>>>>> If no free PEBs are available refill_wl_user_pool() must not
>>>>>> return with -ENOSPC immediately.
>>>>>> It has to block till produce_free_peb() produced a free PEB.
>>>>>>
>>>>>> Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
>>>>>> Signed-off-by: Richard Weinberger <richard@nod.at>
>>>>>
>>>>> What is pool size, I wonder?
>>>>
>>>> Currently it's 25 (UBI_FM_WL_POOL_SIZE).
>>>> If experience shows that 25 is too low/big we can change this constant.
>>>> Maybe it's also worth making them configurable...
>>>
>>> I if it is a possible scenario that this function will not return until
>>> 25 (or even 10) PEBs are erased?
>>>
>>
>> Sure. It will try to gain up to 25 PEBs but return if it gets less.
>> That's why a pool has a current and a max size.
>
> So if erasing speed is say, 250ms, then it would take 6.25 seconds?
Only in the very worst case if we have to call 25 times produce_free_peb().
Of course we could add a check to return immediately if produce_free_peb()
got called a few times in series.
But I really would like to wait with such performance tweaks until fastmap
is more mature.
Thanks,
//richard
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] UBI: fix refill_wl_user_pool()
2013-10-03 16:35 ` Richard Weinberger
@ 2013-10-03 16:41 ` Artem Bityutskiy
2013-10-03 16:48 ` Richard Weinberger
0 siblings, 1 reply; 19+ messages in thread
From: Artem Bityutskiy @ 2013-10-03 16:41 UTC (permalink / raw)
To: Richard Weinberger; +Cc: richard.genoud, linux-mtd, linux-kernel
On Thu, 2013-10-03 at 18:35 +0200, Richard Weinberger wrote:
> Am 03.10.2013 18:00, schrieb Artem Bityutskiy:
> > On Thu, 2013-10-03 at 17:53 +0200, Richard Weinberger wrote:
> >> Am 03.10.2013 17:27, schrieb Artem Bityutskiy:
> >>> On Thu, 2013-10-03 at 17:08 +0200, Richard Weinberger wrote:
> >>>> Am 03.10.2013 17:00, schrieb Artem Bityutskiy:
> >>>>> On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
> >>>>>> If no free PEBs are available refill_wl_user_pool() must not
> >>>>>> return with -ENOSPC immediately.
> >>>>>> It has to block till produce_free_peb() produced a free PEB.
> >>>>>>
> >>>>>> Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
> >>>>>> Signed-off-by: Richard Weinberger <richard@nod.at>
> >>>>>
> >>>>> What is pool size, I wonder?
> >>>>
> >>>> Currently it's 25 (UBI_FM_WL_POOL_SIZE).
> >>>> If experience shows that 25 is too low/big we can change this constant.
> >>>> Maybe it's also worth making them configurable...
> >>>
> >>> I if it is a possible scenario that this function will not return until
> >>> 25 (or even 10) PEBs are erased?
> >>>
> >>
> >> Sure. It will try to gain up to 25 PEBs but return if it gets less.
> >> That's why a pool has a current and a max size.
> >
> > So if erasing speed is say, 250ms, then it would take 6.25 seconds?
>
> Only in the very worst case if we have to call 25 times produce_free_peb().
>
> Of course we could add a check to return immediately if produce_free_peb()
> got called a few times in series.
> But I really would like to wait with such performance tweaks until fastmap
> is more mature.
OK, but how about at least adding a comment talking about this unlikely
scenario?
--
Best Regards,
Artem Bityutskiy
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: UBI fastmap updates
2013-09-28 13:55 UBI fastmap updates Richard Weinberger
` (6 preceding siblings ...)
2013-09-28 13:55 ` [PATCH 7/7] UBI: Add some asserts to ubi_attach_fastmap() Richard Weinberger
@ 2013-10-03 16:44 ` Artem Bityutskiy
7 siblings, 0 replies; 19+ messages in thread
From: Artem Bityutskiy @ 2013-10-03 16:44 UTC (permalink / raw)
To: Richard Weinberger; +Cc: richard.genoud, linux-mtd, linux-kernel
On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
> this series is a collection of all pending UBI fastmap updates.
>
> Richard Genoud found issues in combination with U-Boot.
> These issues also uncovered other minor issues in some error paths.
> Only one fix is not really fastmap specific and touches generic
> UBI code, "UBI: simplify image sequence test".
>
Pushed to linux-ubi, thanks!
--
Best Regards,
Artem Bityutskiy
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] UBI: fix refill_wl_user_pool()
2013-10-03 16:41 ` Artem Bityutskiy
@ 2013-10-03 16:48 ` Richard Weinberger
0 siblings, 0 replies; 19+ messages in thread
From: Richard Weinberger @ 2013-10-03 16:48 UTC (permalink / raw)
To: dedekind1; +Cc: richard.genoud, linux-mtd, linux-kernel
Am 03.10.2013 18:41, schrieb Artem Bityutskiy:
> On Thu, 2013-10-03 at 18:35 +0200, Richard Weinberger wrote:
>> Am 03.10.2013 18:00, schrieb Artem Bityutskiy:
>>> On Thu, 2013-10-03 at 17:53 +0200, Richard Weinberger wrote:
>>>> Am 03.10.2013 17:27, schrieb Artem Bityutskiy:
>>>>> On Thu, 2013-10-03 at 17:08 +0200, Richard Weinberger wrote:
>>>>>> Am 03.10.2013 17:00, schrieb Artem Bityutskiy:
>>>>>>> On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
>>>>>>>> If no free PEBs are available refill_wl_user_pool() must not
>>>>>>>> return with -ENOSPC immediately.
>>>>>>>> It has to block till produce_free_peb() produced a free PEB.
>>>>>>>>
>>>>>>>> Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
>>>>>>>> Signed-off-by: Richard Weinberger <richard@nod.at>
>>>>>>>
>>>>>>> What is pool size, I wonder?
>>>>>>
>>>>>> Currently it's 25 (UBI_FM_WL_POOL_SIZE).
>>>>>> If experience shows that 25 is too low/big we can change this constant.
>>>>>> Maybe it's also worth making them configurable...
>>>>>
>>>>> I if it is a possible scenario that this function will not return until
>>>>> 25 (or even 10) PEBs are erased?
>>>>>
>>>>
>>>> Sure. It will try to gain up to 25 PEBs but return if it gets less.
>>>> That's why a pool has a current and a max size.
>>>
>>> So if erasing speed is say, 250ms, then it would take 6.25 seconds?
>>
>> Only in the very worst case if we have to call 25 times produce_free_peb().
>>
>> Of course we could add a check to return immediately if produce_free_peb()
>> got called a few times in series.
>> But I really would like to wait with such performance tweaks until fastmap
>> is more mature.
>
> OK, but how about at least adding a comment talking about this unlikely
> scenario?
I'm fine with this. Will send a patch. :-)
Thanks,
//richard
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2013-10-03 16:48 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-28 13:55 UBI fastmap updates Richard Weinberger
2013-09-28 13:55 ` [PATCH 1/7] UBI: fix refill_wl_user_pool() Richard Weinberger
2013-10-03 15:00 ` Artem Bityutskiy
2013-10-03 15:08 ` Richard Weinberger
2013-10-03 15:27 ` Artem Bityutskiy
2013-10-03 15:53 ` Richard Weinberger
2013-10-03 16:00 ` Artem Bityutskiy
2013-10-03 16:35 ` Richard Weinberger
2013-10-03 16:41 ` Artem Bityutskiy
2013-10-03 16:48 ` Richard Weinberger
2013-09-28 13:55 ` [PATCH 2/7] UBI: Fix error path in scan_pool() Richard Weinberger
2013-09-28 13:55 ` [PATCH 3/7] UBI: Call scan_all() with correct offset in error case Richard Weinberger
2013-09-28 13:55 ` [PATCH 4/7] UBI: fastmap: fix backward compatibility with image_seq Richard Weinberger
2013-09-28 13:55 ` [PATCH 5/7] UBI: simplify image sequence test Richard Weinberger
2013-09-28 13:55 ` [PATCH 6/7] UBI: Fix memory leak in ubi_attach_fastmap() error path Richard Weinberger
2013-09-30 8:13 ` Richard Genoud
2013-09-28 13:55 ` [PATCH 7/7] UBI: Add some asserts to ubi_attach_fastmap() Richard Weinberger
2013-09-30 8:16 ` Richard Genoud
2013-10-03 16:44 ` UBI fastmap updates Artem Bityutskiy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).