| Message ID | 1412029248-22454-2-git-send-email-richard@nod.at |
|---|---|
| State | Accepted |
| Headers | show |
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?
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
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.
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
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
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
On 10/2/2014 4:18 PM, Richard Weinberger wrote: > 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. :) That's the thing - when debugging FS corruptions that occur on next boot, it's very useful to know exactly what happened on last power down. (Painful experience with other filesystems...) We don't flush the fm on each write operation. We never flush on reads (and we will, as part of the read-disturb implementation. working on it), so it will be useful to know if fm was flashed at this point or wasn't. > > Thanks, > //richard > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ > Thanks Tanya Brokhman
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.
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);
...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(+)