UBI: Fastmap fixes - round one

UBI: Fastmap fixes - round one

[Richard Weinberger]

This is the first set of fastmap fixes for v3.18.
I have more in my local queue but these depend on non-trivial
fastmap changes I made. As soon they have been fully tested I'll
submitt them too.

Thanks,
//richard

[PATCH 1/4] UBI: Ensure that all fastmap work is done upon WL
[PATCH 2/4] UBI: Fastmap: Calc fastmap size correctly
[PATCH 3/4] UBI: Fastmap: Care about the protection queue
[PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is

[PATCH 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown

[Richard Weinberger]

...otherwise the deferred work might run after datastructures
got freed and corrupt memory.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/wl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 20f491713..dc01b1f 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -2029,6 +2029,9 @@ static void protection_queue_destroy(struct ubi_device *ubi)
 void ubi_wl_close(struct ubi_device *ubi)
 {
 	dbg_wl("close the WL sub-system");
+#ifdef CONFIG_MTD_UBI_FASTMAP
+	flush_work(&ubi->fm_work);
+#endif
 	cancel_pending(ubi);
 	protection_queue_destroy(ubi);
 	tree_destroy(&ubi->used);
-- 
2.1.0

[PATCH 2/4] UBI: Fastmap: Calc fastmap size correctly

[Richard Weinberger]

We need to add fm_sb too.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 0431b46..2b0d8d6 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -24,7 +24,8 @@ size_t ubi_calc_fm_size(struct ubi_device *ubi)
 {
 	size_t size;
 
-	size = sizeof(struct ubi_fm_hdr) + \
+	size = sizeof(struct ubi_fm_sb) + \
+		sizeof(struct ubi_fm_hdr) + \
 		sizeof(struct ubi_fm_scan_pool) + \
 		sizeof(struct ubi_fm_scan_pool) + \
 		(ubi->peb_count * sizeof(struct ubi_fm_ec)) + \
-- 
2.1.0

[PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Richard Weinberger]

Fastmap can miss a PEB if it is in the protection queue
and not jet in the used tree.
Treat every protected PEB as used.

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 2b0d8d6..2853a69 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -1195,6 +1195,19 @@ static int ubi_write_fastmap(struct ubi_device *ubi,
 		fm_pos += sizeof(*fec);
 		ubi_assert(fm_pos <= ubi->fm_size);
 	}
+
+	for (i = 0; i < UBI_PROT_QUEUE_LEN; i++) {
+		list_for_each_entry(wl_e, &ubi->pq[i], u.list) {
+			fec = (struct ubi_fm_ec *)(fm_raw + fm_pos);
+
+			fec->pnum = cpu_to_be32(wl_e->pnum);
+			fec->ec = cpu_to_be32(wl_e->ec);
+
+			used_peb_count++;
+			fm_pos += sizeof(*fec);
+			ubi_assert(fm_pos <= ubi->fm_size);
+		}
+	}
 	fmh->used_peb_count = cpu_to_be32(used_peb_count);
 
 	for (node = rb_first(&ubi->scrub); node; node = rb_next(node)) {
-- 
2.1.0

[PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled

[Richard Weinberger]

If the WL pool runs out of PEBs we schedule a fastmap write
to refill it was soon as possible.
Ensure that only one at a time is scheduled otherwise we might end in
a fastmap write storm was writing the fastmap can schedule another
write if bitflips were detected.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/ubi.h | 2 ++
 drivers/mtd/ubi/wl.c  | 8 +++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 7bf4163..529dfb0 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -426,6 +426,7 @@ struct ubi_debug_info {
  * @fm_size: fastmap size in bytes
  * @fm_sem: allows ubi_update_fastmap() to block EBA table changes
  * @fm_work: fastmap work queue
+ * @fm_work_scheduled: non-zero if fastmap work was scheduled
  *
  * @used: RB-tree of used physical eraseblocks
  * @erroneous: RB-tree of erroneous used physical eraseblocks
@@ -531,6 +532,7 @@ struct ubi_device {
 	void *fm_buf;
 	size_t fm_size;
 	struct work_struct fm_work;
+	int fm_work_scheduled;
 
 	/* Wear-leveling sub-system's stuff */
 	struct rb_root used;
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index dc01b1f..4c02a6e 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -149,6 +149,9 @@ static void update_fastmap_work_fn(struct work_struct *wrk)
 {
 	struct ubi_device *ubi = container_of(wrk, struct ubi_device, fm_work);
 	ubi_update_fastmap(ubi);
+	spin_lock(&ubi->wl_lock);
+	ubi->fm_work_scheduled = 0;
+	spin_unlock(&ubi->wl_lock);
 }
 
 /**
@@ -657,7 +660,10 @@ static struct ubi_wl_entry *get_peb_for_wl(struct ubi_device *ubi)
 		/* We cannot update the fastmap here because this
 		 * function is called in atomic context.
 		 * Let's fail here and refill/update it as soon as possible. */
-		schedule_work(&ubi->fm_work);
+		if (!ubi->fm_work_scheduled) {
+			ubi->fm_work_scheduled = 1;
+			schedule_work(&ubi->fm_work);
+		}
 		return NULL;
 	} else {
 		pnum = pool->pebs[pool->used++];
-- 
2.1.0

Re: [PATCH 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown

[Artem Bityutskiy]

On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote:
> ...otherwise the deferred work might run after datastructures
> got freed and corrupt memory.

How can this happend? The background thread is stopped by this time
already, so what are the other possibilities? And why is this
fastmap-only?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled

[Bityutskiy, Artem]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1077 bytes --]

On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote:
> +       spin_lock(&ubi->wl_lock);
> +       ubi->fm_work_scheduled = 0;
> +       spin_unlock(&ubi->wl_lock);

Andrew Morton once said me that if I am protecting an integer change
like this with a spinlock, I have a problem in my locking design. He was
right for my particular case.

Integer is changes atomic. The only other thing spinlock adds are the
barriers.

-- 
Best Regards,
Artem Bityutskiy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown

[Richard Weinberger]

Am 30.09.2014 08:26, schrieb Artem Bityutskiy:
> On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote:
>> ...otherwise the deferred work might run after datastructures
>> got freed and corrupt memory.
> 
> How can this happend? The background thread is stopped by this time
> already, so what are the other possibilities? And why is this
> fastmap-only?

This has nothing do to with the background thread.
Fastmap has a work queue. If one fastmap work has been
scheuled we have to wait for it.

Thanks,
//richard

Re: [PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled

[Richard Weinberger]

Am 30.09.2014 08:45, schrieb Bityutskiy, Artem:
> On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote:
>> +       spin_lock(&ubi->wl_lock);
>> +       ubi->fm_work_scheduled = 0;
>> +       spin_unlock(&ubi->wl_lock);
> 
> Andrew Morton once said me that if I am protecting an integer change
> like this with a spinlock, I have a problem in my locking design. He was
> right for my particular case.
> 
> Integer is changes atomic. The only other thing spinlock adds are the
> barriers.

I've added the spinlock to have a barrier in any case.

Thanks,
//richard

Re: [PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled

[Bityutskiy, Artem]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1307 bytes --]

On Tue, 2014-09-30 at 08:59 +0200, Richard Weinberger wrote:
> Am 30.09.2014 08:45, schrieb Bityutskiy, Artem:
> > On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote:
> >> +       spin_lock(&ubi->wl_lock);
> >> +       ubi->fm_work_scheduled = 0;
> >> +       spin_unlock(&ubi->wl_lock);
> > 
> > Andrew Morton once said me that if I am protecting an integer change
> > like this with a spinlock, I have a problem in my locking design. He was
> > right for my particular case.
> > 
> > Integer is changes atomic. The only other thing spinlock adds are the
> > barriers.
> 
> I've added the spinlock to have a barrier in any case.

Examples of any?

-- 
Best Regards,
Artem Bityutskiy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled

[Richard Weinberger]

Am 30.09.2014 09:39, schrieb Bityutskiy, Artem:
> On Tue, 2014-09-30 at 08:59 +0200, Richard Weinberger wrote:
>> Am 30.09.2014 08:45, schrieb Bityutskiy, Artem:
>>> On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote:
>>>> +       spin_lock(&ubi->wl_lock);
>>>> +       ubi->fm_work_scheduled = 0;
>>>> +       spin_unlock(&ubi->wl_lock);
>>>
>>> Andrew Morton once said me that if I am protecting an integer change
>>> like this with a spinlock, I have a problem in my locking design. He was
>>> right for my particular case.
>>>
>>> Integer is changes atomic. The only other thing spinlock adds are the
>>> barriers.
>>
>> I've added the spinlock to have a barrier in any case.
> 
> Examples of any?

You mean a case where the compiler would reorder code and the barrier is needed?
I don't have one, but I'm not that creative as a modern C compiler.
If you say that no barrier is needed I'll trust you. :-)

Thanks,
//richard

Re: [PATCH 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown

[Bityutskiy, Artem]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2310 bytes --]

On Tue, 2014-09-30 at 08:58 +0200, Richard Weinberger wrote:
> Am 30.09.2014 08:26, schrieb Artem Bityutskiy:
> > On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote:
> >> ...otherwise the deferred work might run after datastructures
> >> got freed and corrupt memory.
> > 
> > How can this happend? The background thread is stopped by this time
> > already, so what are the other possibilities? And why is this
> > fastmap-only?
> 
> This has nothing do to with the background thread.
> Fastmap has a work queue. If one fastmap work has been
> scheuled we have to wait for it.

I expected a bit more explanation. But OK, here is what I think.

UBI consists of subsystems. Subsystems try to be more or less
independent, whenever possible. They expose interface functions for
other subsystems. Of course the split is not ideal, but we do our best.

* wl.c does wear-levelling.
* wl.c does not do fastmap.
* fastmap.c does fastmap.
* I am unhappy seeing yet another ifdef to wl.c
* I am unhappy seeing wl.c calling 'flush_work(&ubi->fm_work)', because
fastmap.c should deal with 'fm_work'. Or said differently, wl.c is not a
fastmap queue baby-sitter. fastmap.c is.

Most UB subsystems have the init and close function. May be adding one
for fastmap would help? Then you could flush whatever from
'ubi_wl_close()' ?

Historically the work queue was implemented in wl.c because wl.c was the
only user of it.

If this layout is not good enough, we should probably extend it, may be
separate work queue management out of wl.c.

But populating wl.c with macros and little "take care of this fatmap
bit" stuff is a not going to lead to better code structure.

-- 
Best Regards,
Artem Bityutskiy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown

[Richard Weinberger]

Am 30.09.2014 09:53, schrieb Bityutskiy, Artem:
> On Tue, 2014-09-30 at 08:58 +0200, Richard Weinberger wrote:
>> Am 30.09.2014 08:26, schrieb Artem Bityutskiy:
>>> On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote:
>>>> ...otherwise the deferred work might run after datastructures
>>>> got freed and corrupt memory.
>>>
>>> How can this happend? The background thread is stopped by this time
>>> already, so what are the other possibilities? And why is this
>>> fastmap-only?
>>
>> This has nothing do to with the background thread.
>> Fastmap has a work queue. If one fastmap work has been
>> scheuled we have to wait for it.
> 
> I expected a bit more explanation. But OK, here is what I think.
> 
> UBI consists of subsystems. Subsystems try to be more or less
> independent, whenever possible. They expose interface functions for
> other subsystems. Of course the split is not ideal, but we do our best.
> 
> * wl.c does wear-levelling.
> * wl.c does not do fastmap.
> * fastmap.c does fastmap.
> * I am unhappy seeing yet another ifdef to wl.c

I can warp it.

> * I am unhappy seeing wl.c calling 'flush_work(&ubi->fm_work)', because
> fastmap.c should deal with 'fm_work'. Or said differently, wl.c is not a
> fastmap queue baby-sitter. fastmap.c is.

But wl.c has to trigger the deferred fastmap work as only wl.c detects when it is
needed. I you want I can implement a ubi_fastmap_shutdown() in fastmap.c which
calls flush_work().
But I did it in wl.c to have it balanced. wl.c triggers and stops the fastmap work.

> Most UB subsystems have the init and close function. May be adding one
> for fastmap would help? Then you could flush whatever from
> 'ubi_wl_close()' ?
> 
> Historically the work queue was implemented in wl.c because wl.c was the
> only user of it.

What work queue are you talking about?
The fastmap work queue was added by me and has nothing to do with the UBI background
worker thread.

> If this layout is not good enough, we should probably extend it, may be
> separate work queue management out of wl.c.

One thing I can think of is getting completely rid of the UBI background thread
and convert it to a work queue. But I'm not sure if this would make things easier
for fastmap.

> But populating wl.c with macros and little "take care of this fatmap
> bit" stuff is a not going to lead to better code structure.

s/fatmap/fastmap. A Freudian slip? ;)

I spent already a full week on refactoring that code. My goal was
making ubi_update_fastmap() callable from within the wear_leveling_worker() to get
rid of the fastmap work queue completely.
After one week the new code was more complicated and ugly than the current one. :-\

Some background info:
Fastmap needs to create a snapshot of the UBI state.
This is why you cannot call it within an UBI work. As UBI works can run in parallel.
The fastmap creation does many things which can sleep. Most stuff in the wear_leveling_worker()
happens in atmoic context.

Thanks,
//richard

Re: [PATCH 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown

[Tanya Brokhman]

Hi Richard,

On 9/30/2014 1:20 AM, Richard Weinberger wrote:
> ...otherwise the deferred work might run after datastructures
> got freed and corrupt memory.
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>   drivers/mtd/ubi/wl.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
> index 20f491713..dc01b1f 100644
> --- a/drivers/mtd/ubi/wl.c
> +++ b/drivers/mtd/ubi/wl.c
> @@ -2029,6 +2029,9 @@ static void protection_queue_destroy(struct ubi_device *ubi)
>   void ubi_wl_close(struct ubi_device *ubi)
>   {
>   	dbg_wl("close the WL sub-system");
> +#ifdef CONFIG_MTD_UBI_FASTMAP
> +	flush_work(&ubi->fm_work);

flush_work returns bool. It might be useful to print that value for 
debugging.

> +#endif
>   	cancel_pending(ubi);
>   	protection_queue_destroy(ubi);
>   	tree_destroy(&ubi->used);
>

Thanks
Tanya Brokhman
-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation

Re: [PATCH 2/4] UBI: Fastmap: Calc fastmap size correctly

[Tanya Brokhman]

On 9/30/2014 1:20 AM, Richard Weinberger wrote:
> We need to add fm_sb too.
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>   drivers/mtd/ubi/fastmap.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index 0431b46..2b0d8d6 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -24,7 +24,8 @@ size_t ubi_calc_fm_size(struct ubi_device *ubi)
>   {
>   	size_t size;
>
> -	size = sizeof(struct ubi_fm_hdr) + \
> +	size = sizeof(struct ubi_fm_sb) + \
> +		sizeof(struct ubi_fm_hdr) + \
>   		sizeof(struct ubi_fm_scan_pool) + \
>   		sizeof(struct ubi_fm_scan_pool) + \
>   		(ubi->peb_count * sizeof(struct ubi_fm_ec)) + \
>

Not sure what's the proper way doing this (Reviewed-by/Acked-by) but I 
agree this patch is required. I would just elaborate a bit more on the 
commit message.

Thanks,
Tanya Brokhman
-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation

Re: [PATCH 2/4] UBI: Fastmap: Calc fastmap size correctly

[Richard Weinberger]

Am 02.10.2014 15:14, schrieb Tanya Brokhman:
> On 9/30/2014 1:20 AM, Richard Weinberger wrote:
>> We need to add fm_sb too.
>>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> ---
>>   drivers/mtd/ubi/fastmap.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
>> index 0431b46..2b0d8d6 100644
>> --- a/drivers/mtd/ubi/fastmap.c
>> +++ b/drivers/mtd/ubi/fastmap.c
>> @@ -24,7 +24,8 @@ size_t ubi_calc_fm_size(struct ubi_device *ubi)
>>   {
>>       size_t size;
>>
>> -    size = sizeof(struct ubi_fm_hdr) + \
>> +    size = sizeof(struct ubi_fm_sb) + \
>> +        sizeof(struct ubi_fm_hdr) + \
>>           sizeof(struct ubi_fm_scan_pool) + \
>>           sizeof(struct ubi_fm_scan_pool) + \
>>           (ubi->peb_count * sizeof(struct ubi_fm_ec)) + \
>>
> 
> Not sure what's the proper way doing this (Reviewed-by/Acked-by) but I agree this patch is required. I would just elaborate a bit more on the commit message.

If you read this patch and find it good, please add your Reviewed-by.
Thanks a lot for reviewing my patches!

Thanks,
//richard

Re: [PATCH 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown

[Richard Weinberger]

Am 02.10.2014 15:05, schrieb Tanya Brokhman:
>> +#ifdef CONFIG_MTD_UBI_FASTMAP
>> +    flush_work(&ubi->fm_work);
> 
> flush_work returns bool. It might be useful to print that value for debugging.

Why would this be useful?
The sole purpose of this flush is having a barrier.
If flush_work() had to wait (returns true) for a pending work, fine.
If where was no work (returns false) to wait for, also fine. :)

Thanks,
//richard

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Tanya Brokhman]

Hi Richard

On 9/30/2014 1:20 AM, Richard Weinberger wrote:
> Fastmap can miss a PEB if it is in the protection queue
> and not jet in the used tree.
> Treat every protected PEB as used.
>
> 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 2b0d8d6..2853a69 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -1195,6 +1195,19 @@ static int ubi_write_fastmap(struct ubi_device *ubi,
>   		fm_pos += sizeof(*fec);
>   		ubi_assert(fm_pos <= ubi->fm_size);
>   	}
> +
> +	for (i = 0; i < UBI_PROT_QUEUE_LEN; i++) {
> +		list_for_each_entry(wl_e, &ubi->pq[i], u.list) {

why not list_for_each_entry_safe?

> +			fec = (struct ubi_fm_ec *)(fm_raw + fm_pos);
> +
> +			fec->pnum = cpu_to_be32(wl_e->pnum);
> +			fec->ec = cpu_to_be32(wl_e->ec);
> +
> +			used_peb_count++;
> +			fm_pos += sizeof(*fec);
> +			ubi_assert(fm_pos <= ubi->fm_size);

Is fm_size ok with this addition or does it needs updating as well?

> +		}
> +	}
>   	fmh->used_peb_count = cpu_to_be32(used_peb_count);
>
>   	for (node = rb_first(&ubi->scrub); node; node = rb_next(node)) {
>

Thanks
Tanya Brokhman
-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Richard Weinberger]

Am 02.10.2014 15:28, schrieb Tanya Brokhman:
> Hi Richard
> 
> On 9/30/2014 1:20 AM, Richard Weinberger wrote:
>> Fastmap can miss a PEB if it is in the protection queue
>> and not jet in the used tree.
>> Treat every protected PEB as used.
>>
>> 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 2b0d8d6..2853a69 100644
>> --- a/drivers/mtd/ubi/fastmap.c
>> +++ b/drivers/mtd/ubi/fastmap.c
>> @@ -1195,6 +1195,19 @@ static int ubi_write_fastmap(struct ubi_device *ubi,
>>           fm_pos += sizeof(*fec);
>>           ubi_assert(fm_pos <= ubi->fm_size);
>>       }
>> +
>> +    for (i = 0; i < UBI_PROT_QUEUE_LEN; i++) {
>> +        list_for_each_entry(wl_e, &ubi->pq[i], u.list) {
> 
> why not list_for_each_entry_safe?

Because we don't delete elements from this list while iterating over it.

>> +            fec = (struct ubi_fm_ec *)(fm_raw + fm_pos);
>> +
>> +            fec->pnum = cpu_to_be32(wl_e->pnum);
>> +            fec->ec = cpu_to_be32(wl_e->ec);
>> +
>> +            used_peb_count++;
>> +            fm_pos += sizeof(*fec);
>> +            ubi_assert(fm_pos <= ubi->fm_size);
> 
> Is fm_size ok with this addition or does it needs updating as well?

It is okay. The fastmap size calculation reserves enough space for all possible
PEBs.

Thanks,
//richard

Re: [PATCH 2/4] UBI: Fastmap: Calc fastmap size correctly

[Tanya Brokhman]

On 9/30/2014 1:20 AM, Richard Weinberger wrote:
> We need to add fm_sb too.
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>   drivers/mtd/ubi/fastmap.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index 0431b46..2b0d8d6 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -24,7 +24,8 @@ size_t ubi_calc_fm_size(struct ubi_device *ubi)
>   {
>   	size_t size;
>
> -	size = sizeof(struct ubi_fm_hdr) + \
> +	size = sizeof(struct ubi_fm_sb) + \
> +		sizeof(struct ubi_fm_hdr) + \
>   		sizeof(struct ubi_fm_scan_pool) + \
>   		sizeof(struct ubi_fm_scan_pool) + \
>   		(ubi->peb_count * sizeof(struct ubi_fm_ec)) + \
>

Reviewed-by: Tanya Brokhman <tlinder@codeaurora.org>

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Tanya Brokhman]

On 10/2/2014 4:32 PM, Richard Weinberger wrote:
> Am 02.10.2014 15:28, schrieb Tanya Brokhman:
>> Hi Richard
>>
>> On 9/30/2014 1:20 AM, Richard Weinberger wrote:
>>> Fastmap can miss a PEB if it is in the protection queue
>>> and not jet in the used tree.
>>> Treat every protected PEB as used.
>>>
>>> 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 2b0d8d6..2853a69 100644
>>> --- a/drivers/mtd/ubi/fastmap.c
>>> +++ b/drivers/mtd/ubi/fastmap.c
>>> @@ -1195,6 +1195,19 @@ static int ubi_write_fastmap(struct ubi_device *ubi,
>>>            fm_pos += sizeof(*fec);
>>>            ubi_assert(fm_pos <= ubi->fm_size);
>>>        }
>>> +
>>> +    for (i = 0; i < UBI_PROT_QUEUE_LEN; i++) {
>>> +        list_for_each_entry(wl_e, &ubi->pq[i], u.list) {
>>
>> why not list_for_each_entry_safe?
>
> Because we don't delete elements from this list while iterating over it.
>
>>> +            fec = (struct ubi_fm_ec *)(fm_raw + fm_pos);
>>> +
>>> +            fec->pnum = cpu_to_be32(wl_e->pnum);
>>> +            fec->ec = cpu_to_be32(wl_e->ec);
>>> +
>>> +            used_peb_count++;
>>> +            fm_pos += sizeof(*fec);
>>> +            ubi_assert(fm_pos <= ubi->fm_size);
>>
>> Is fm_size ok with this addition or does it needs updating as well?
>
> It is okay. The fastmap size calculation reserves enough space for all possible
> PEBs.
>
> Thanks,
> //richard
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

Reviewed-by: Tanya Brokhman <tlinder@codeaurora.org>

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation

Re: [PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled

[Tanya Brokhman]

On 9/30/2014 10:44 AM, Richard Weinberger wrote:
> Am 30.09.2014 09:39, schrieb Bityutskiy, Artem:
>> On Tue, 2014-09-30 at 08:59 +0200, Richard Weinberger wrote:
>>> Am 30.09.2014 08:45, schrieb Bityutskiy, Artem:
>>>> On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote:
>>>>> +       spin_lock(&ubi->wl_lock);
>>>>> +       ubi->fm_work_scheduled = 0;
>>>>> +       spin_unlock(&ubi->wl_lock);
>>>>
>>>> Andrew Morton once said me that if I am protecting an integer change
>>>> like this with a spinlock, I have a problem in my locking design. He was
>>>> right for my particular case.
>>>>
>>>> Integer is changes atomic. The only other thing spinlock adds are the
>>>> barriers.
>>>
>>> I've added the spinlock to have a barrier in any case.
>>
>> Examples of any?
>
> You mean a case where the compiler would reorder code and the barrier is needed?
> I don't have one, but I'm not that creative as a modern C compiler.
> If you say that no barrier is needed I'll trust you. :-)

we just implemented the same thing :) It's being tested....
Why not use atomic_t fm_work_scheduled and save the spin_lock?

>
> Thanks,
> //richard
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>


-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation

Re: [PATCH 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown

[Artem Bityutskiy]

On Tue, 2014-09-30 at 10:07 +0200, Richard Weinberger wrote:
> > UBI consists of subsystems. Subsystems try to be more or less
> > independent, whenever possible. They expose interface functions for
> > other subsystems. Of course the split is not ideal, but we do our best.
> > 
> > * wl.c does wear-levelling.
> > * wl.c does not do fastmap.
> > * fastmap.c does fastmap.
> > * I am unhappy seeing yet another ifdef to wl.c
> 
> I can warp it.

What I am looking forward to is to see fastmap.c and wl.c to be
separated in a better way. Would your answer be "this is impossible", or
"I've got some ideas"?

The end result should be no #ifdefs in wl.c, or very few of them.

> > * I am unhappy seeing wl.c calling 'flush_work(&ubi->fm_work)', because
> > fastmap.c should deal with 'fm_work'. Or said differently, wl.c is not a
> > fastmap queue baby-sitter. fastmap.c is.
> 
> But wl.c has to trigger the deferred fastmap work as only wl.c detects when it is
> needed. I you want I can implement a ubi_fastmap_shutdown() in fastmap.c which
> calls flush_work().
> But I did it in wl.c to have it balanced. wl.c triggers and stops the fastmap work.

wl.c should be responsible for wear levelling.

wl.c should not be responsible for managing the fastmap pool.

Architecture-wise, is the pull something above or below the WL level?

I think above.

In the past, the EBA layer, which sits on top of the WL layer, called
the WL functions directly to get an and put the LEB.

Then you added the Fastmap pool inbetween EBA and WL. And you added it
to wl.c, it seems (note, I do not know the fastmap well enough, so be
patient and carefully explain me things when I am wrong, please).

What I think is that the pool should live in fastmap.c instead. EBA
should call the pool functions, instead of the WL functions. The pool
code should call the WL functions whenever it needs to refill the pool.

Can something like this be done?

> Some background info:
> Fastmap needs to create a snapshot of the UBI state.
> This is why you cannot call it within an UBI work. As UBI works can run in parallel.

My point is, let's do it in fastmap.c. Let's keep each layer as pure a
possible.

Right now wl.c is populated by fastmap-specific stuff too much. It is
hard to make wear-levelling improvements there because of this.

I want the complexity to be partitioned.

I want WL complexity be in wl.c

I do want to try hard to have _no_ fastmap complexity in wl.c.

I am looking for a discussion about how we could do this.

This patch adds a little bit, but more fastmap complexity to the WL
subsystem. I am trying to block this.

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Artem Bityutskiy]

On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote:
> +
> +	for (i = 0; i < UBI_PROT_QUEUE_LEN; i++) {
> +		list_for_each_entry(wl_e, &ubi->pq[i], u.list) {
> +			fec = (struct ubi_fm_ec *)(fm_raw + fm_pos);

Similarly to my other posts, is it possible to not touch the WL-internal
things like 'ubi->pq[]' from fastmap.c? Can we come up with a function
at wl.c which fastmap.c could call instead?

Re: [PATCH 2/4] UBI: Fastmap: Calc fastmap size correctly

[Artem Bityutskiy]

On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote:
> We need to add fm_sb too.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>

Pushed this one, thank you!

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Richard Weinberger]

Am 03.10.2014 16:31, schrieb Artem Bityutskiy:
> On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote:
>> +
>> +	for (i = 0; i < UBI_PROT_QUEUE_LEN; i++) {
>> +		list_for_each_entry(wl_e, &ubi->pq[i], u.list) {
>> +			fec = (struct ubi_fm_ec *)(fm_raw + fm_pos);
> 
> Similarly to my other posts, is it possible to not touch the WL-internal
> things like 'ubi->pq[]' from fastmap.c? Can we come up with a function
> at wl.c which fastmap.c could call instead?

Fastmap needs basically access to all internal state of UBI, which lives mostly
within wl.c
It needs to iterate over the used, free, erase, scrub RB-trees, the protection queue, etc. to
collect the exact state of all PEBs.

In C++ or Java I would pass an iterator to fastmap.c using a function in wl.c but I'm not sure
how to do such a thing in a sane way in C.

What about having a wl-fastmap.c file which acts as glue layer between fastmap.c and wl.c?

Thanks,
//richard

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Artem Bityutskiy]

On Fri, 2014-10-03 at 21:06 +0200, Richard Weinberger wrote:
> Fastmap needs basically access to all internal state of UBI, which
> lives mostly
> within wl.c

Sounds like a very strong assertion, smells a bit fishy, need the
details.

> It needs to iterate over the used, free, erase, scrub RB-trees, the
> protection queue, etc. to
> collect the exact state of all PEBs.

When? In 'ubi_write_fastmap()' when forming the FM pool data structures?

I think you can just add a function like this to wl.c:

int ubi_wl_get_peb_info(int pnum, struct ubi_wl_peb_info *pebinfo)

Yoy will give it the PEB number you are interested in, it will return
you the information you need in 'pebinfo'. The information will contain
the EC, the state (used, free, etc).

If you need just the EC, then you do not even need the ubi_wl_peb_info
data structure.

Then you just iterate over all PEBs and fill the FM pool data
structures.

Would something like this work?

Artem.

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Richard Weinberger]

Am 13.10.2014 um 15:17 schrieb Artem Bityutskiy:
> On Fri, 2014-10-03 at 21:06 +0200, Richard Weinberger wrote:
>> Fastmap needs basically access to all internal state of UBI, which
>> lives mostly
>> within wl.c
> 
> Sounds like a very strong assertion, smells a bit fishy, need the
> details.

Fastmap need to know the exact state of each PEB.
I.e. is it free, used, scheduled for erase, etc...

>> It needs to iterate over the used, free, erase, scrub RB-trees, the
>> protection queue, etc. to
>> collect the exact state of all PEBs.
> 
> When? In 'ubi_write_fastmap()' when forming the FM pool data structures?

Yes.

> I think you can just add a function like this to wl.c:
> 
> int ubi_wl_get_peb_info(int pnum, struct ubi_wl_peb_info *pebinfo)
> 
> Yoy will give it the PEB number you are interested in, it will return
> you the information you need in 'pebinfo'. The information will contain
> the EC, the state (used, free, etc).
> 
> If you need just the EC, then you do not even need the ubi_wl_peb_info
> data structure.
> 
> Then you just iterate over all PEBs and fill the FM pool data
> structures.
> 
> Would something like this work?

The interface would work but some work in wl.c is needed.
For example if I want to find out in which state PEB 1 is wl.c would have to
look int free free, used free, protection queue, etc.. to tell me the state. This is slow.

But we could add the state information to struct ubi_wl_entry by adding a single integer attribute called "state" or "flags".
Then wl.c can set the state every time the PEB moves between internal data structures.
I.e. upon it moves from the used to free rb tree the flag UBI_STATE_FREE will be cleared and UBI_STATE_USED set.
This will also give us a very efficient way to ensure a sane state.
In fact, I have already written such a feature. I needed it to hunt down a Fastmap bug which lead to a state where a PEB existed
in both the used tree and the protection queue. Using the ->state attribute in ubi_wl_entry I was able to add various
cheap asserts to the code and found the incorrect transaction.

The cost of this bloating the ubi_wl_entry struct by sizeof(int) bytes.
But we'll get very efficient variants of self_check_in_wl_tree() and friends.

Also the implementation of ubi_wl_get_peb_info() would be easy.
It will be mostly like:
int ubi_wl_get_peb_info(int pnum, struct ubi_wl_peb_info *pebinfo)
{
...
e = ubi->lookuptbl[pnum];
fill_pebinfo(pebinfo, e->state);
...
}

Would this make you happy? :)

Thanks,
//richard

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Artem Bityutskiy]

On Mon, 2014-10-13 at 16:30 +0200, Richard Weinberger wrote:
> Am 13.10.2014 um 15:17 schrieb Artem Bityutskiy:
> > On Fri, 2014-10-03 at 21:06 +0200, Richard Weinberger wrote:
> >> Fastmap needs basically access to all internal state of UBI, which
> >> lives mostly
> >> within wl.c
> > 
> > Sounds like a very strong assertion, smells a bit fishy, need the
> > details.
> 
> Fastmap need to know the exact state of each PEB.
> I.e. is it free, used, scheduled for erase, etc...
> 
> >> It needs to iterate over the used, free, erase, scrub RB-trees, the
> >> protection queue, etc. to
> >> collect the exact state of all PEBs.
> > 
> > When? In 'ubi_write_fastmap()' when forming the FM pool data structures?
> 
> Yes.
> 
> > I think you can just add a function like this to wl.c:
> > 
> > int ubi_wl_get_peb_info(int pnum, struct ubi_wl_peb_info *pebinfo)
> > 
> > Yoy will give it the PEB number you are interested in, it will return
> > you the information you need in 'pebinfo'. The information will contain
> > the EC, the state (used, free, etc).
> > 
> > If you need just the EC, then you do not even need the ubi_wl_peb_info
> > data structure.
> > 
> > Then you just iterate over all PEBs and fill the FM pool data
> > structures.
> > 
> > Would something like this work?
> 
> The interface would work but some work in wl.c is needed.
> For example if I want to find out in which state PEB 1 is wl.c would have to
> look int free free, used free, protection queue, etc.. to tell me the state. This is slow.

Well, used and free are RB-trees, looking them up is slow.

If what you need is to go through all used and free PEBs, then you can
introduce some kind of

struct ubi_wl_entry *ubi_wl_get_next_used(struct ubi_wl_entry *prev)

function, and similar functions for the free.

I would return you the next entry (or NULL would indicate the end), and
it would take the previous entry as the input, or NULL for the first
call.

We'd need to take the locks before calling this function. This is
cleaner than what we do now, right?

> But we could add the state information to struct ubi_wl_entry by adding a single integer attribute called "state" or "flags".

But there is a price - memory consumption. We do not want to pay it just
for making the inter-subsystems boundaries better, there ought to be a
better reason.

Say, for an (imaginary) 8GiB NAND chip with 128KiB PEB size this would
cost 256KiB of RAM.

Squeezing the state into the last 2 bits a memory reference would be
another possibility, BTW. Not elegant, though...

> Would this make you happy? :)

Not very, I'd save this for the last resort solution.

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Bityutskiy, Artem]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1052 bytes --]

On Mon, 2014-10-13 at 18:23 +0300, Artem Bityutskiy wrote:
> > The interface would work but some work in wl.c is needed.
> > For example if I want to find out in which state PEB 1 is wl.c would have to
> > look int free free, used free, protection queue, etc.. to tell me the state. This is slow.
> 
> Well, used and free are RB-trees, looking them up is slow.

I wanted to say "not slow" here, sorry.

-- 
Best Regards,
Artem Bityutskiy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Richard Weinberger]

Am 13.10.2014 um 17:23 schrieb Artem Bityutskiy:
> Well, used and free are RB-trees, looking them up is slow.

This is true but we'd have to look it up in multiple trees and the protection queue...

> If what you need is to go through all used and free PEBs, then you can
> introduce some kind of
> 
> struct ubi_wl_entry *ubi_wl_get_next_used(struct ubi_wl_entry *prev)
> 
> function, and similar functions for the free.
> 
> I would return you the next entry (or NULL would indicate the end), and
> it would take the previous entry as the input, or NULL for the first
> call.
> 
> We'd need to take the locks before calling this function. This is
> cleaner than what we do now, right?

ubi_update_fastmap() takes ubi->wl_lock anyway to block any changes in the free, used, etc. trees
to make sure that the to be taken state snapshot is consistent.

>> But we could add the state information to struct ubi_wl_entry by adding a single integer attribute called "state" or "flags".
> 
> But there is a price - memory consumption. We do not want to pay it just
> for making the inter-subsystems boundaries better, there ought to be a
> better reason.
> 
> Say, for an (imaginary) 8GiB NAND chip with 128KiB PEB size this would
> cost 256KiB of RAM.

Is 128KiB PEB size still realistic on modern NANDs?
Even if, 256KiB are not much and the kernel consumes this additionally with
every new release.
But I can understand your concerns.

> Squeezing the state into the last 2 bits a memory reference would be
> another possibility, BTW. Not elegant, though...
> 
>> Would this make you happy? :)
> 
> Not very, I'd save this for the last resort solution.

Okay, I'll try harder to make you happy.

Thanks,
//richard

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Artem Bityutskiy]

On Mon, 2014-10-13 at 23:04 +0200, Richard Weinberger wrote:
> Am 13.10.2014 um 17:23 schrieb Artem Bityutskiy:
> > Well, used and free are RB-trees, looking them up is slow.
> 
> This is true but we'd have to look it up in multiple trees and the protection queue...

Right. 2 RB-trees, and one list. The list is empty most of the time, or
contains one element. 

So we'd look-up 2 RB-trees most of the time. Very rarely we'd need to
look at the list containing very few elements.

Not that bad, I think.

> ubi_update_fastmap() takes ubi->wl_lock anyway to block any changes in the free, used, etc. trees
> to make sure that the to be taken state snapshot is consistent.

I think this is fine.

> > But there is a price - memory consumption. We do not want to pay it just
> > for making the inter-subsystems boundaries better, there ought to be a
> > better reason.
> > 
> > Say, for an (imaginary) 8GiB NAND chip with 128KiB PEB size this would
> > cost 256KiB of RAM.
> 
> Is 128KiB PEB size still realistic on modern NANDs?
> Even if, 256KiB are not much and the kernel consumes this additionally with
> every new release.

Right, but the point is that bigger systems use eMMC and wouldn't bother
with raw flash. Raw flash trending down the smaller systems, where a
hundred of kilobytes matters.

> But I can understand your concerns.

Thanks, yes, my point is to be careful about the RAM we consume, and try
to avoid growing this data structure, an only do it if we have to.

> Okay, I'll try harder to make you happy.

Well, making me happy is not the point of course :-)

Thanks!

--
Artem

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Tanya Brokhman]

On 10/14/2014 1:23 PM, Artem Bityutskiy wrote:
> On Mon, 2014-10-13 at 23:04 +0200, Richard Weinberger wrote:
>> Am 13.10.2014 um 17:23 schrieb Artem Bityutskiy:
>>> Well, used and free are RB-trees, looking them up is slow.
>>
>> This is true but we'd have to look it up in multiple trees and the protection queue...
>
> Right. 2 RB-trees, and one list. The list is empty most of the time, or
> contains one element.
>
> So we'd look-up 2 RB-trees most of the time. Very rarely we'd need to
> look at the list containing very few elements.
>
> Not that bad, I think.
>
>> ubi_update_fastmap() takes ubi->wl_lock anyway to block any changes in the free, used, etc. trees
>> to make sure that the to be taken state snapshot is consistent.
>
> I think this is fine.
>
>>> But there is a price - memory consumption. We do not want to pay it just
>>> for making the inter-subsystems boundaries better, there ought to be a
>>> better reason.
>>>
>>> Say, for an (imaginary) 8GiB NAND chip with 128KiB PEB size this would
>>> cost 256KiB of RAM.
>>
>> Is 128KiB PEB size still realistic on modern NANDs?
>> Even if, 256KiB are not much and the kernel consumes this additionally with
>> every new release.
>
> Right, but the point is that bigger systems use eMMC and wouldn't bother
> with raw flash. Raw flash trending down the smaller systems, where a
> hundred of kilobytes matters.
>
>> But I can understand your concerns.
>
> Thanks, yes, my point is to be careful about the RAM we consume, and try
> to avoid growing this data structure, an only do it if we have to.
>
>> Okay, I'll try harder to make you happy.
>
> Well, making me happy is not the point of course :-)
>
> Thanks!
>
> --
> Artem
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

Hi Artem/Richard

I think your discussion here stopped being relevant to this specific 
patch but went on to the fastmap feature design in general :)
This patch fixes a real bug in the current implementation of the 
feature. What you're discussing requires a re-writing and re-design of 
the feature. Perhaps this one can be merged and will be "fixed" later on 
when you agree on how you would like FM to access WL data structures in 
general?

Thanks,
Tanya Brokhman
-- 
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Artem Bityutskiy]

On Tue, 2014-10-14 at 15:21 +0300, Tanya Brokhman wrote:
> Hi Artem/Richard
> 
> I think your discussion here stopped being relevant to this specific 
> patch but went on to the fastmap feature design in general :)
> This patch fixes a real bug in the current implementation of the 
> feature. What you're discussing requires a re-writing and re-design of 
> the feature. Perhaps this one can be merged and will be "fixed" later on 
> when you agree on how you would like FM to access WL data structures in 
> general?

First of all, "re-writing and re-design of the feature" is an
overstatement. So far this is on the "cleaning things up" side of the
spectrum, closer to the "re-factoring" area.

WRT "merge the fix now and improve later" - this is a good argument for
an "inside a company" discussion, where the primary TTM is the driving
factor.

For the community TTM is a good thing, but quality comes first.

Now, if this was about a regression, one could apply time pressure on
the maintainer. But we are talking about a problem which was there from
day 0.

It is completely normal for the maintainer to push back various
hot-fixes for the problem and request some reasonable re-factoring
first. This is what I do. This is very very usual thing in the Linux
community.

So far I did not ask anything huge and unreasonable, I think. Just
cleaner inter-subsystem APIs, less of the "fastmap uses the other
subsystems' internals" kind of things.

--
Artem.

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Tanya Brokhman]

On 10/14/2014 4:02 PM, Artem Bityutskiy wrote:
> On Tue, 2014-10-14 at 15:21 +0300, Tanya Brokhman wrote:
>> Hi Artem/Richard
>>
>> I think your discussion here stopped being relevant to this specific
>> patch but went on to the fastmap feature design in general :)
>> This patch fixes a real bug in the current implementation of the
>> feature. What you're discussing requires a re-writing and re-design of
>> the feature. Perhaps this one can be merged and will be "fixed" later on
>> when you agree on how you would like FM to access WL data structures in
>> general?
>
> First of all, "re-writing and re-design of the feature" is an
> overstatement. So far this is on the "cleaning things up" side of the
> spectrum, closer to the "re-factoring" area.
>
> WRT "merge the fix now and improve later" - this is a good argument for
> an "inside a company" discussion, where the primary TTM is the driving
> factor.
>
> For the community TTM is a good thing, but quality comes first.
>
> Now, if this was about a regression, one could apply time pressure on
> the maintainer. But we are talking about a problem which was there from
> day 0.
>
> It is completely normal for the maintainer to push back various
> hot-fixes for the problem and request some reasonable re-factoring
> first. This is what I do. This is very very usual thing in the Linux
> community.
>
> So far I did not ask anything huge and unreasonable, I think. Just
> cleaner inter-subsystem APIs, less of the "fastmap uses the other
> subsystems' internals" kind of things.
>
> --
> Artem.
>

Ok, accepted. It was just a suggestion. I'm all for quality coming 
first, even if you were asking for something "huge".

Thanks,
Tanya Brokhman
-- 
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Richard Weinberger]

Am 14.10.2014 um 12:23 schrieb Artem Bityutskiy:
> On Mon, 2014-10-13 at 23:04 +0200, Richard Weinberger wrote:
>> Am 13.10.2014 um 17:23 schrieb Artem Bityutskiy:
>>> Well, used and free are RB-trees, looking them up is slow.
>>
>> This is true but we'd have to look it up in multiple trees and the protection queue...
> 
> Right. 2 RB-trees, and one list. The list is empty most of the time, or
> contains one element. 

Actually it is the free, used and scrub tree plus the protection queue.
Then there is another place where PEBs can hide, the erase work queue.
This brings me to an issue I've recently identified and fixed in my local queue.

ubi_wl_put_peb() does the following:
                if (in_wl_tree(e, &ubi->used)) {
                        self_check_in_wl_tree(ubi, e, &ubi->used);
                        rb_erase(&e->u.rb, &ubi->used);
                } else if (in_wl_tree(e, &ubi->scrub)) {
                        self_check_in_wl_tree(ubi, e, &ubi->scrub);
                        rb_erase(&e->u.rb, &ubi->scrub);
                } else if (in_wl_tree(e, &ubi->erroneous)) {
                        self_check_in_wl_tree(ubi, e, &ubi->erroneous);
                        rb_erase(&e->u.rb, &ubi->erroneous);
                        ubi->erroneous_peb_count -= 1;
                        ubi_assert(ubi->erroneous_peb_count >= 0);
                        /* Erroneous PEBs should be tortured */
                        torture = 1;
                } else {
                        err = prot_queue_del(ubi, e->pnum);
                        if (err) {
                                ubi_err("PEB %d not found", pnum);
                                ubi_ro_mode(ubi);
                                spin_unlock(&ubi->wl_lock);
                                return err;
                        }
                }
        }
        spin_unlock(&ubi->wl_lock);

        err = schedule_erase(ubi, e, vol_id, lnum, torture);

If it happens that a fastmap write is needed between spin_unlock() and schedule_erase(), fastmap
will miss this PEB. Because it go already deleted from one of the RB trees or the protection queue but not
jet queued to the work list. Yes, I hit this bug in real world.

My solution is marking the ubi_wl_entry's ->state attribute with a flag like UBI_WLE_STATE_ERASE.
This way I was also able to get rid of the ubi_is_erase_work() wart.

> So we'd look-up 2 RB-trees most of the time. Very rarely we'd need to
> look at the list containing very few elements.
> 
> Not that bad, I think.
> 
>> ubi_update_fastmap() takes ubi->wl_lock anyway to block any changes in the free, used, etc. trees
>> to make sure that the to be taken state snapshot is consistent.
> 
> I think this is fine.
> 
>>> But there is a price - memory consumption. We do not want to pay it just
>>> for making the inter-subsystems boundaries better, there ought to be a
>>> better reason.
>>>
>>> Say, for an (imaginary) 8GiB NAND chip with 128KiB PEB size this would
>>> cost 256KiB of RAM.
>>
>> Is 128KiB PEB size still realistic on modern NANDs?
>> Even if, 256KiB are not much and the kernel consumes this additionally with
>> every new release.
> 
> Right, but the point is that bigger systems use eMMC and wouldn't bother
> with raw flash. Raw flash trending down the smaller systems, where a
> hundred of kilobytes matters.
> 
>> But I can understand your concerns.
> 
> Thanks, yes, my point is to be careful about the RAM we consume, and try
> to avoid growing this data structure, an only do it if we have to.

Currently I'm using an integer variable. But a char would also do it. I need only
a few flags.

What I'm trying to say is, state tracking would solve the "internal state accessing" problem in a clean and
sane way.

I gave the ubi_get_peb_state() idea a try. Unfortunately it won't work that well as expected.
The UBI Fastmap on-flash layout has a section which contains the state and EC counter of each PEB.
But the PEBs are not stored on flash starting with 0 to ubi->peb_count, they are stored by their state.
First free, then used, then scrub and finally to be erased PEBs. I've chosen this layout to avoid an
addition on-flash state attribute to keep the overall structure small.

So, writing a fastmap like:
for (i = 0; i < ubi->peb_count; i++)
  ubi_get_peb_state(i);

...won't work. I need to iterate first over the free RB tree, then over the used tree, and so on.
This way I'd have to allocate memory and store the state somewhere or iterate multiple times over the same RB trees.

What about this:
Let's create a file like fastmap-wl.c, it will contain all Fastmap functions which deal with internal
wl.c stuff. In wl.c we can add:
#ifdef CONFIG_MTD_UBI_FASTMAP
#include "fastmap-wl.c"
#endif

This way we can get rid of almost all #ifdefs in wl.c and don't have to export a lot of new fastmap specific helper functions in wl.c
to global scope.
Including a c file is common practice in Linux.
A prominent example is USB. See drivers/usb/host/ehci-hcd.c.

What do you think?

Thanks,
//richard

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Artem Bityutskiy]

On Thu, 2014-10-16 at 12:06 +0200, Richard Weinberger wrote:
> What I'm trying to say is, state tracking would solve the "internal state accessing" problem in a clean and
> sane way.

Can you squeeze the stat to the lookup talbe? It contains pointers, so
the last 2 bits could be used for the state.

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Richard Weinberger]

Am 16.10.2014 um 12:15 schrieb Artem Bityutskiy:
> On Thu, 2014-10-16 at 12:06 +0200, Richard Weinberger wrote:
>> What I'm trying to say is, state tracking would solve the "internal state accessing" problem in a clean and
>> sane way.
> 
> Can you squeeze the stat to the lookup talbe? It contains pointers, so
> the last 2 bits could be used for the state.

IMHO this is beyond horrible. :)
The goal is the make things clean in terms of not accessing internal state
from fastmap.c. If the price for that is abusing pointers in magic ways
I'd say no.

Rather consider the fastmap-wl.c idea...

Thanks,
//richard

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Artem Bityutskiy]

On Thu, 2014-10-16 at 12:06 +0200, Richard Weinberger wrote:
> Am 14.10.2014 um 12:23 schrieb Artem Bityutskiy:
> > On Mon, 2014-10-13 at 23:04 +0200, Richard Weinberger wrote:
> >> Am 13.10.2014 um 17:23 schrieb Artem Bityutskiy:
> >>> Well, used and free are RB-trees, looking them up is slow.
> >>
> >> This is true but we'd have to look it up in multiple trees and the protection queue...
> > 
> > Right. 2 RB-trees, and one list. The list is empty most of the time, or
> > contains one element. 
> 
> Actually it is the free, used and scrub tree plus the protection queue.

OK, still does not change my point: 3 trees and a list which is mostly
empty or short. Not that bad.

Besides, the used tree is the large one, contains most of the stuff. The
scrub tree is usually small, and mostly empty. The erroneous tree also
mostly empty.

So this in most of the cases, this will be about looking up a single
tree.

And again, all you need is:

1. all used
2. all scrub
3. all erroneous

> Then there is another place where PEBs can hide, the erase work queue.

That's just fastmap code not doing the right thing. We should not touch
the work queue directly at all. What we _should_ do instead is to make
it empty by asking the subsystem which manages it to flush it.

1. Lock the work queue to prevent anyone from submitting new jobs while
we are in process of writing the fastmap.
2. Flush all works
3. do all the fastmap write stuff
4. Unlock

> This brings me to an issue I've recently identified and fixed in my local queue.
> 
> ubi_wl_put_peb() does the following:
>                 if (in_wl_tree(e, &ubi->used)) {
>                         self_check_in_wl_tree(ubi, e, &ubi->used);
>                         rb_erase(&e->u.rb, &ubi->used);
>                 } else if (in_wl_tree(e, &ubi->scrub)) {
>                         self_check_in_wl_tree(ubi, e, &ubi->scrub);
>                         rb_erase(&e->u.rb, &ubi->scrub);
>                 } else if (in_wl_tree(e, &ubi->erroneous)) {
>                         self_check_in_wl_tree(ubi, e, &ubi->erroneous);
>                         rb_erase(&e->u.rb, &ubi->erroneous);
>                         ubi->erroneous_peb_count -= 1;
>                         ubi_assert(ubi->erroneous_peb_count >= 0);
>                         /* Erroneous PEBs should be tortured */
>                         torture = 1;
>                 } else {
>                         err = prot_queue_del(ubi, e->pnum);
>                         if (err) {
>                                 ubi_err("PEB %d not found", pnum);
>                                 ubi_ro_mode(ubi);
>                                 spin_unlock(&ubi->wl_lock);
>                                 return err;
>                         }
>                 }
>         }
>         spin_unlock(&ubi->wl_lock);
> 
>         err = schedule_erase(ubi, e, vol_id, lnum, torture);
> 
> If it happens that a fastmap write is needed between spin_unlock() and schedule_erase(), fastmap
> will miss this PEB.

You say is that LEBs change while you are writing fastmap. Of course
they do. We should have a locking mechanism in place which would prevent
this.

Mat be wl_lock needs to become a mutex, or may be there should be
separate mutex.

Or may be 'ubi_wl_put_peb()' should go through the fatmap subsystem
which should be able to block it if it is writing fastmap.

Ideally, you should be able to "freeze" the state (of course for a short
time), prepare fastmap data structures in memory, unfreeze, let users to
further IO, and wirte fastmap.

This is roughly what UBIFS does when committing. It freezes all writers,
builds the commit list, unfreezes the writers, and continues the commit
process.

Now, to be that advanced, you need a journal. Without journal, you do
freeze, prepare and write, unfreeze. The end results is that writers are
blocked for longer time, but this may be good enough.

> What do you think?

I think that UBI is memory consumption grows linearly with the flash
growth. Mount time was linear too, fastmap is supposed to improve this.

I know that there are people, who also reported their issues in this
mailing list, who are very concerned about UBI memory consumption.

And I think that fastmap should try really hard to not only improve the
mount time, but also not to make the memory consumption problem worse.

So yes, I understand code niceness argument. I really do. But this
should not make UBI problem to be an even worse problem.

Please, let's try much harder to go without increasing the memory
footprint.

Thanks!

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Richard Weinberger]

Am 20.10.2014 um 16:46 schrieb Artem Bityutskiy:
> On Thu, 2014-10-16 at 12:06 +0200, Richard Weinberger wrote:
>> Am 14.10.2014 um 12:23 schrieb Artem Bityutskiy:
>>> On Mon, 2014-10-13 at 23:04 +0200, Richard Weinberger wrote:
>>>> Am 13.10.2014 um 17:23 schrieb Artem Bityutskiy:
>>>>> Well, used and free are RB-trees, looking them up is slow.
>>>>
>>>> This is true but we'd have to look it up in multiple trees and the protection queue...
>>>
>>> Right. 2 RB-trees, and one list. The list is empty most of the time, or
>>> contains one element. 
>>
>> Actually it is the free, used and scrub tree plus the protection queue.
> 
> OK, still does not change my point: 3 trees and a list which is mostly
> empty or short. Not that bad.
> 
> Besides, the used tree is the large one, contains most of the stuff. The
> scrub tree is usually small, and mostly empty. The erroneous tree also
> mostly empty.
> 
> So this in most of the cases, this will be about looking up a single
> tree.
> 
> And again, all you need is:
> 
> 1. all used
> 2. all scrub
> 3. all erroneous

Agreed.

>> Then there is another place where PEBs can hide, the erase work queue.
> 
> That's just fastmap code not doing the right thing. We should not touch
> the work queue directly at all. What we _should_ do instead is to make
> it empty by asking the subsystem which manages it to flush it.
> 
> 1. Lock the work queue to prevent anyone from submitting new jobs while
> we are in process of writing the fastmap.
> 2. Flush all works
> 3. do all the fastmap write stuff
> 4. Unlock

Are you sure? Flushing all works before every fastmap write will slow UBI down.
The fastmap write is not cheap. The goal should be to make it faster.

>> This brings me to an issue I've recently identified and fixed in my local queue.
>>
>> ubi_wl_put_peb() does the following:
>>                 if (in_wl_tree(e, &ubi->used)) {
>>                         self_check_in_wl_tree(ubi, e, &ubi->used);
>>                         rb_erase(&e->u.rb, &ubi->used);
>>                 } else if (in_wl_tree(e, &ubi->scrub)) {
>>                         self_check_in_wl_tree(ubi, e, &ubi->scrub);
>>                         rb_erase(&e->u.rb, &ubi->scrub);
>>                 } else if (in_wl_tree(e, &ubi->erroneous)) {
>>                         self_check_in_wl_tree(ubi, e, &ubi->erroneous);
>>                         rb_erase(&e->u.rb, &ubi->erroneous);
>>                         ubi->erroneous_peb_count -= 1;
>>                         ubi_assert(ubi->erroneous_peb_count >= 0);
>>                         /* Erroneous PEBs should be tortured */
>>                         torture = 1;
>>                 } else {
>>                         err = prot_queue_del(ubi, e->pnum);
>>                         if (err) {
>>                                 ubi_err("PEB %d not found", pnum);
>>                                 ubi_ro_mode(ubi);
>>                                 spin_unlock(&ubi->wl_lock);
>>                                 return err;
>>                         }
>>                 }
>>         }
>>         spin_unlock(&ubi->wl_lock);
>>
>>         err = schedule_erase(ubi, e, vol_id, lnum, torture);
>>
>> If it happens that a fastmap write is needed between spin_unlock() and schedule_erase(), fastmap
>> will miss this PEB.
> 
> You say is that LEBs change while you are writing fastmap. Of course
> they do. We should have a locking mechanism in place which would prevent
> this.

Not really. As fastmap will take the work semaphore no work is processed while
writing the fastmap. In this case the PEB is in flight between RB trees and the
work queue. Fastmap can miss it. But it won't change.

> Mat be wl_lock needs to become a mutex, or may be there should be
> separate mutex.
> 
> Or may be 'ubi_wl_put_peb()' should go through the fatmap subsystem
> which should be able to block it if it is writing fastmap.
> 
> Ideally, you should be able to "freeze" the state (of course for a short
> time), prepare fastmap data structures in memory, unfreeze, let users to
> further IO, and wirte fastmap.

Fastmap does this already. It takes various locks to freeze the state.

> This is roughly what UBIFS does when committing. It freezes all writers,
> builds the commit list, unfreezes the writers, and continues the commit
> process.
> 
> Now, to be that advanced, you need a journal. Without journal, you do
> freeze, prepare and write, unfreeze. The end results is that writers are
> blocked for longer time, but this may be good enough.

I think we're becoming to scientific now.

>> What do you think?
> 
> I think that UBI is memory consumption grows linearly with the flash
> growth. Mount time was linear too, fastmap is supposed to improve this.

With fastmap you have anyways a higher memory consumption.
To begin with the vmalloc()'ed ubi->fm_buf which holds the complete fastmap
in memory while reading/writing it.

> I know that there are people, who also reported their issues in this
> mailing list, who are very concerned about UBI memory consumption.
> 
> And I think that fastmap should try really hard to not only improve the
> mount time, but also not to make the memory consumption problem worse.
> 
> So yes, I understand code niceness argument. I really do. But this
> should not make UBI problem to be an even worse problem.

You can up with the niceness arguments, and now you're suddenly extremely
focusing on the memory footprint.

> Please, let's try much harder to go without increasing the memory
> footprint.

I need to go back to the drawing board. But first I have to fix/analyze
various bugs. Then I'll maybe have time again for memory/design nicefications.
Don't await any new patches for fastmap within the next weeks.

Thanks,
//richard

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Artem Bityutskiy]

On Mon, 2014-10-20 at 17:17 +0200, Richard Weinberger wrote:
> > That's just fastmap code not doing the right thing. We should not touch
> > the work queue directly at all. What we _should_ do instead is to make
> > it empty by asking the subsystem which manages it to flush it.
> > 
> > 1. Lock the work queue to prevent anyone from submitting new jobs while
> > we are in process of writing the fastmap.
> > 2. Flush all works
> > 3. do all the fastmap write stuff
> > 4. Unlock
> 
> Are you sure? Flushing all works before every fastmap write will slow UBI down.
> The fastmap write is not cheap. The goal should be to make it faster.

Yes, you are right. So instead, we wanna "freeze" it, and save all PEBs
the jobs in fastmap too.

Why 'ubi_write_fastmap()' only cares about the erase jobs, and saves the
PEBs from the job as "must be erased".

What about the "move" jobs?

Also, say, PEB X is in the work queue waiting for erasure. Fastmap comes
along and saves it as "must be erased" in the fastmap. Fastmap finishes
its job, PEB X gets erased, and I write my data there, so PEB X is
referred to by LEB Y. Now I have power cut. Then I attach the flash
again. Surely it is not that fastmap just erases PEB X and I lose the
contents of LEB Y?

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Richard Weinberger]

Am 20.10.2014 um 17:40 schrieb Artem Bityutskiy:
> On Mon, 2014-10-20 at 17:17 +0200, Richard Weinberger wrote:
>>> That's just fastmap code not doing the right thing. We should not touch
>>> the work queue directly at all. What we _should_ do instead is to make
>>> it empty by asking the subsystem which manages it to flush it.
>>>
>>> 1. Lock the work queue to prevent anyone from submitting new jobs while
>>> we are in process of writing the fastmap.
>>> 2. Flush all works
>>> 3. do all the fastmap write stuff
>>> 4. Unlock
>>
>> Are you sure? Flushing all works before every fastmap write will slow UBI down.
>> The fastmap write is not cheap. The goal should be to make it faster.
> 
> Yes, you are right. So instead, we wanna "freeze" it, and save all PEBs
> the jobs in fastmap too.
> 
> Why 'ubi_write_fastmap()' only cares about the erase jobs, and saves the
> PEBs from the job as "must be erased".
> 
> What about the "move" jobs?

There are no move jobs. Do you mean ubi->move_from/to used in wear_leveling_worker()?
How could it happen that a fastmap is written between these? IIRC everything is done
under wl_lock. And the PEBs used have to go through the pool.

> Also, say, PEB X is in the work queue waiting for erasure. Fastmap comes
> along and saves it as "must be erased" in the fastmap. Fastmap finishes
> its job, PEB X gets erased, and I write my data there, so PEB X is
> referred to by LEB Y. Now I have power cut. Then I attach the flash
> again. Surely it is not that fastmap just erases PEB X and I lose the
> contents of LEB Y?

This cannot happen. If X is erased you cannot write data do it. I must first go
thought the pool and the pool is scanned while attaching.

Thanks,
//richard

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Artem Bityutskiy]

On Mon, 2014-10-20 at 17:59 +0200, Richard Weinberger wrote:
> > Also, say, PEB X is in the work queue waiting for erasure. Fastmap comes
> > along and saves it as "must be erased" in the fastmap. Fastmap finishes
> > its job, PEB X gets erased, and I write my data there, so PEB X is
> > referred to by LEB Y. Now I have power cut. Then I attach the flash
> > again. Surely it is not that fastmap just erases PEB X and I lose the
> > contents of LEB Y?
> 
> This cannot happen. If X is erased you cannot write data do it. I must first go
> thought the pool and the pool is scanned while attaching.

Just noticed from the code that we first add PEBs to the erase list, and
then we go an scan the pools.

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Richard Weinberger]

Am 20.10.2014 um 18:09 schrieb Artem Bityutskiy:
> On Mon, 2014-10-20 at 17:59 +0200, Richard Weinberger wrote:
>>> Also, say, PEB X is in the work queue waiting for erasure. Fastmap comes
>>> along and saves it as "must be erased" in the fastmap. Fastmap finishes
>>> its job, PEB X gets erased, and I write my data there, so PEB X is
>>> referred to by LEB Y. Now I have power cut. Then I attach the flash
>>> again. Surely it is not that fastmap just erases PEB X and I lose the
>>> contents of LEB Y?
>>
>> This cannot happen. If X is erased you cannot write data do it. I must first go
>> thought the pool and the pool is scanned while attaching.
> 
> Just noticed from the code that we first add PEBs to the erase list, and
> then we go an scan the pools.

In the attach code, yes. But this does not matter.
It cannot happen that a fastmap is written with a PEB being in a pool
and the erase job list.

If you figure out such a state, please tell me. I'll fix it. :-)

Thanks,
//richard

Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue

[Richard Weinberger]

Am 20.10.2014 um 17:40 schrieb Artem Bityutskiy:
> Also, say, PEB X is in the work queue waiting for erasure. Fastmap comes
> along and saves it as "must be erased" in the fastmap. Fastmap finishes
> its job, PEB X gets erased, and I write my data there, so PEB X is
> referred to by LEB Y. Now I have power cut. Then I attach the flash
> again. Surely it is not that fastmap just erases PEB X and I lose the
> contents of LEB Y?

After reading my mail again, I think my answer was not clear.
Please let me explain in detail.

PEB X waits for erasure. In this state it can get erased and will be moved into
the free RB tree.
If a new PEB is requested from UBI it will not get served from the free RB tree,
instead it will come from the pool. I.e. There is no way that the freshly erased PEB
will immediately used. If a a fastmap was written while PEB was queued for erasure
and a power cut happens after the erasure it will be erased again. This does not harm.
But it can never ever happen that the same PEB will be in a pool (and therefore maybe used)
and scheduled for erase. A freshly erased PEB can only be used after wandering into the pool.
But this would require a refill-pool operation first an a write of the fastmap, the said PEB
cannot be listed as erase work in the fastmap then.

Thanks,
//richard