UBI: refine wear leveling logic
Subsystems (2)
RFC PATCH 1/1 UBI: refine wear leveling logic
Hi,
This patch addresses the handling of blocks that are put by the user while they are moved by the wear leveling thread. The schedule_erase function is now called by put_peb() itself instead of notifying the wear leveling thread.
Comments are welcome...
--- drivers/mtd/ubi/ubi.h | 2 - drivers/mtd/ubi/wl.c | 77 ++++++++------------------------------------------ 2 files changed, 13 insertions(+), 66 deletions(-)
drivers/mtd/ubi/ubi.h +0 -2
@@ -316,8 +316,6 @@ struct ubi_device {unsigned long long abs_ec;struct ubi_wl_entry *move_from;struct ubi_wl_entry *move_to; -int move_from_put; -int move_to_put;struct list_head works;int works_count;struct task_struct *bgt_thread;
drivers/mtd/ubi/wl.c +13 -64
116 lines not on this page · raw message
Hi Alexander,
Quoted 4 lines · Alexander Schmidt
This patch addresses the handling of blocks that are put by the user while they are moved by the wear leveling thread. The
schedule_erasefunction is now called byput_peb()itself instead of notifying the wear leveling thread.
May I ask you for more explanation why you think your code is correct?
drivers/mtd/ubi/wl.c int ubi_wl_put_peb(struct ubi_device *ub · line 1165
Quoted 10 lines
drivers/mtd/ubi/wl.c +7
-0+ +if (in_wl_tree (e, &ubi->used)) +used_tree_del (ubi, e); +else if (unlikely (in_wl_tree (e, &ubi->scrub))) +scrub_tree_del (ubi, e); +else if (!in_wl_tree (e, &ubi->free)) +prot_tree_del (ubi, e->pnum);spin_unlock (&ubi->wl_lock); err =schedule_erase (ubi, e, torture);
Fine, you schedule this eraseblock for erasure. At the same time the the WL worker moves data in there. The copy_leb() function will notice that the LEB is unmapped, and won't do copy. Then the WL worker will insert the eraseblock to a tree. At the same time the erase worker will insert the same wl_entry to the free tree. One of the trees will be screwed-up.
Signature · 2 lines
Best regards, Artem Bityutskiy (Битюцкий Артём)
Hi Artem,
Quoted 1 line · Artem Bityutskiy
May I ask you for more explanation why you think your code is correct?
While writing this i assumed that erase and WL procedures are performed sequentially (either by the background thread or, if the bgt is disabled, by the produce_free_peb() function). Thank to your comment below I now realised that erase/wl procedures could happen concurrently if the free tree is empty and there are pending works (this is the only way that could lead to your error scenario, right?).
If so then i propose to make get_peb() wait until the bgt produces a free peb and not mix synchronous/asynchronous operations, as this would make the code easier, IMO.
Regards,
Alex
Quoted 23 lines
drivers/mtd/ubi/wl.c int ubi_wl_put_peb(struct ubi_device *ub · line 1165
Quoted 11 lines
drivers/mtd/ubi/wl.c +7
-0+ +if (in_wl_tree (e, &ubi->used)) +used_tree_del (ubi, e); +else if (unlikely (in_wl_tree (e, &ubi->scrub))) +scrub_tree_del (ubi, e); +else if (!in_wl_tree (e, &ubi->free)) +prot_tree_del (ubi, e->pnum);spin_unlock (&ubi->wl_lock); err =schedule_erase (ubi, e, torture);Fine, you schedule this eraseblock for erasure. At the same time the the WL worker moves data in there. The
copy_leb()function will notice that the LEB is unmapped, and won't do copy. Then the WL worker will insert the eraseblock to a tree. At the same time the erase worker will insert the samewl_entryto the free tree. One of the trees will be screwed-up.Signature · 2 lines
Best regards, Artem Bityutskiy (Битюцкий Артём)
Quoted 6 lines · Alexander Schmidt
While writing this i assumed that erase and WL procedures are performed sequentially (either by the background thread or, if the bgt is disabled, by the
produce_free_peb()function). Thank to your comment below I now realised that erase/wl procedures could happen concurrently if the free tree is empty and there are pending works (this is the only way that could lead to your error scenario, right?).
Yes, kind of.
Quoted 3 lines
If so then i propose to make
get_peb()wait until the bgt produces a free peb and not mix synchronous/asynchronous operations, as this would make the code easier, IMO.
It already does this AFAIK.
Signature · 2 lines
Best Regards, Artem Bityutskiy (Артём Битюцкий)
Quoted 5 lines · Artem Bityutskiy
Quoted 3 lines
If so then i propose to make
get_peb()wait until the bgt produces a free peb and not mix synchronous/asynchronous operations, as this would make the code easier, IMO.It already does this AFAIK.
In the current code, wl_get_peb() calls produce_free_peb() if the free tree is empty and thereby performs a synchronous work (even if the bgt is enabled). This is what I meant with "mixing" sync/async.
So I think making the user thread wait for the wl thread to free a peb (e.g. by sleeping while waiting for a semaphore) could lead to a better performance as the wl thread will likely finish its work before the user thread has performed a whole work.
Regards,
Alex
Quoted 3 lines · Alexander Schmidt
In the current code,
wl_get_peb()callsproduce_free_peb()if the free tree is empty and thereby performs a synchronous work (even if the bgt is enabled). This is what I meant with "mixing" sync/async.
OK, what's wring with this? We do not have eraseblocks, we produce them synchronously.
We cannot rely on the thread - what if it was killed or its priority is too low, but the requesting task needs eraseblocks ASAP.
Quoted 4 lines
So I think making the user thread wait for the wl thread to free a peb (e.g. by sleeping while waiting for a semaphore) could lead to a better performance as the wl thread will likely finish its work before the user thread has performed a whole work.
Not necessarily. The current approach is safer.
Signature · 2 lines
Best regards, Artem Bityutskiy (Битюцкий Артём)