| Message ID | CABatt_wEa+jiSmutzZrjyRFXqypPTbfdo_Z8CVDhuuRMUXjA6w@mail.gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Heiko Schocher |
| Headers | show |
| Series | [U-Boot] ubi: Fix filesystem corruption on detach when fastmap enabled | expand |
Hello Martin, Am 12.01.2018 um 20:03 schrieb Martin Townsend: >>From d35b7ea298fbd6c9d08b1b7132d43b9289d2b914 Mon Sep 17 00:00:00 2001 > From: Martin Townsend <mtownsend1973@gmail.com> > Date: Fri, 12 Jan 2018 18:59:23 +0000 > Subject: [PATCH] ubi: Fix filesystem corruption on detach when fastmap enabled > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > When detaching using "ubi detach" it calls ubi_detach_mtd_dev which > calls ubi_update_fastmap twice when fastmap is enabled. The second > invocation was corrupting the ubifs as it was called after uif_close. > Moved all calls to ubi_wl_close before uif_close. > > Signed-off-by: Martin Townsend <mtownsend1973@gmail.com> > --- > drivers/mtd/ubi/build.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c > index baf4e2d..795ea34 100644 > --- a/drivers/mtd/ubi/build.c > +++ b/drivers/mtd/ubi/build.c > @@ -1082,9 +1082,9 @@ out_debugfs: > out_uif: > get_device(&ubi->dev); > ubi_assert(ref); > + ubi_wl_close(ubi); > uif_close(ubi); > out_detach: > - ubi_wl_close(ubi); > ubi_free_internal_volumes(ubi); > vfree(ubi->vtbl); > out_free: > @@ -1161,9 +1161,9 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway) > get_device(&ubi->dev); > > ubi_debugfs_exit_dev(ubi); > + ubi_wl_close(ubi); > uif_close(ubi); > > - ubi_wl_close(ubi); > ubi_free_internal_volumes(ubi); > vfree(ubi->vtbl); > put_mtd_device(ubi->mtd); > Could you please try the following patch: diff --git a/drivers/mtd/ubi/fastmap-wl.c b/drivers/mtd/ubi/fastmap-wl.c index a33d4063e0..2923d21836 100644 --- a/drivers/mtd/ubi/fastmap-wl.c +++ b/drivers/mtd/ubi/fastmap-wl.c @@ -339,8 +339,6 @@ static void ubi_fastmap_close(struct ubi_device *ubi) #ifndef __UBOOT__ flush_work(&ubi->fm_work); -#else - update_fastmap_work_fn(ubi); #endif return_unused_pool_pebs(ubi, &ubi->fm_pool); return_unused_pool_pebs(ubi, &ubi->fm_wl_pool); Your problem is (I think) because U-Boot Code accidentially calls update_fastmap_work_fn(ubi), but we do not need it here anymore, as U-Boot does all UBI work immediately. bye, Heiko
Hi Heiko, On Mon, Jan 15, 2018 at 11:30 AM, Heiko Schocher <hs@denx.de> wrote: > Hello Martin, > > > Am 12.01.2018 um 20:03 schrieb Martin Townsend: >>> >>> From d35b7ea298fbd6c9d08b1b7132d43b9289d2b914 Mon Sep 17 00:00:00 2001 >> >> From: Martin Townsend <mtownsend1973@gmail.com> >> Date: Fri, 12 Jan 2018 18:59:23 +0000 >> Subject: [PATCH] ubi: Fix filesystem corruption on detach when fastmap >> enabled >> MIME-Version: 1.0 >> Content-Type: text/plain; charset=UTF-8 >> Content-Transfer-Encoding: 8bit >> >> When detaching using "ubi detach" it calls ubi_detach_mtd_dev which >> calls ubi_update_fastmap twice when fastmap is enabled. The second >> invocation was corrupting the ubifs as it was called after uif_close. >> Moved all calls to ubi_wl_close before uif_close. >> >> Signed-off-by: Martin Townsend <mtownsend1973@gmail.com> >> --- >> drivers/mtd/ubi/build.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c >> index baf4e2d..795ea34 100644 >> --- a/drivers/mtd/ubi/build.c >> +++ b/drivers/mtd/ubi/build.c >> @@ -1082,9 +1082,9 @@ out_debugfs: >> out_uif: >> get_device(&ubi->dev); >> ubi_assert(ref); >> + ubi_wl_close(ubi); >> uif_close(ubi); >> out_detach: >> - ubi_wl_close(ubi); >> ubi_free_internal_volumes(ubi); >> vfree(ubi->vtbl); >> out_free: >> @@ -1161,9 +1161,9 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway) >> get_device(&ubi->dev); >> >> ubi_debugfs_exit_dev(ubi); >> + ubi_wl_close(ubi); >> uif_close(ubi); >> >> - ubi_wl_close(ubi); >> ubi_free_internal_volumes(ubi); >> vfree(ubi->vtbl); >> put_mtd_device(ubi->mtd); >> > > Could you please try the following patch: > > diff --git a/drivers/mtd/ubi/fastmap-wl.c b/drivers/mtd/ubi/fastmap-wl.c > index a33d4063e0..2923d21836 100644 > --- a/drivers/mtd/ubi/fastmap-wl.c > +++ b/drivers/mtd/ubi/fastmap-wl.c > @@ -339,8 +339,6 @@ static void ubi_fastmap_close(struct ubi_device *ubi) > > #ifndef __UBOOT__ > flush_work(&ubi->fm_work); > -#else > - update_fastmap_work_fn(ubi); > #endif > return_unused_pool_pebs(ubi, &ubi->fm_pool); > return_unused_pool_pebs(ubi, &ubi->fm_wl_pool); > > Your problem is (I think) because U-Boot Code accidentially calls > update_fastmap_work_fn(ubi), but we do not need it here anymore, as > U-Boot does all UBI work immediately. > > bye, > Heiko > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de That was my original fix so can confirm this also works. My reasoning for opting for the reordering was: I think the problem is uif_close frees up some UBI data structures so we have to ensure no updating of the filesystem occurs after this. What if ubi_fastmap_close or ubi_wl_close change in future and these changes result in updates to the filesystem, the same problem will occur and for our board it corrupts the UBIFS. So I opted to change the order in build.c. Cheers, Martin
Hello Martin, added Richard to cc Am 15.01.2018 um 13:13 schrieb Martin Townsend: > Hi Heiko, > > > On Mon, Jan 15, 2018 at 11:30 AM, Heiko Schocher <hs@denx.de> wrote: >> Hello Martin, >> >> >> Am 12.01.2018 um 20:03 schrieb Martin Townsend: >>>> >>>> From d35b7ea298fbd6c9d08b1b7132d43b9289d2b914 Mon Sep 17 00:00:00 2001 >>> >>> From: Martin Townsend <mtownsend1973@gmail.com> >>> Date: Fri, 12 Jan 2018 18:59:23 +0000 >>> Subject: [PATCH] ubi: Fix filesystem corruption on detach when fastmap >>> enabled >>> MIME-Version: 1.0 >>> Content-Type: text/plain; charset=UTF-8 >>> Content-Transfer-Encoding: 8bit >>> >>> When detaching using "ubi detach" it calls ubi_detach_mtd_dev which >>> calls ubi_update_fastmap twice when fastmap is enabled. The second >>> invocation was corrupting the ubifs as it was called after uif_close. >>> Moved all calls to ubi_wl_close before uif_close. >>> >>> Signed-off-by: Martin Townsend <mtownsend1973@gmail.com> >>> --- >>> drivers/mtd/ubi/build.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c >>> index baf4e2d..795ea34 100644 >>> --- a/drivers/mtd/ubi/build.c >>> +++ b/drivers/mtd/ubi/build.c >>> @@ -1082,9 +1082,9 @@ out_debugfs: >>> out_uif: >>> get_device(&ubi->dev); >>> ubi_assert(ref); >>> + ubi_wl_close(ubi); >>> uif_close(ubi); >>> out_detach: >>> - ubi_wl_close(ubi); >>> ubi_free_internal_volumes(ubi); >>> vfree(ubi->vtbl); >>> out_free: >>> @@ -1161,9 +1161,9 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway) >>> get_device(&ubi->dev); >>> >>> ubi_debugfs_exit_dev(ubi); >>> + ubi_wl_close(ubi); >>> uif_close(ubi); >>> >>> - ubi_wl_close(ubi); >>> ubi_free_internal_volumes(ubi); >>> vfree(ubi->vtbl); >>> put_mtd_device(ubi->mtd); >>> >> >> Could you please try the following patch: >> >> diff --git a/drivers/mtd/ubi/fastmap-wl.c b/drivers/mtd/ubi/fastmap-wl.c >> index a33d4063e0..2923d21836 100644 >> --- a/drivers/mtd/ubi/fastmap-wl.c >> +++ b/drivers/mtd/ubi/fastmap-wl.c >> @@ -339,8 +339,6 @@ static void ubi_fastmap_close(struct ubi_device *ubi) >> >> #ifndef __UBOOT__ >> flush_work(&ubi->fm_work); >> -#else >> - update_fastmap_work_fn(ubi); >> #endif >> return_unused_pool_pebs(ubi, &ubi->fm_pool); >> return_unused_pool_pebs(ubi, &ubi->fm_wl_pool); >> >> Your problem is (I think) because U-Boot Code accidentially calls >> update_fastmap_work_fn(ubi), but we do not need it here anymore, as >> U-Boot does all UBI work immediately. >> >> bye, >> Heiko >> -- >> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk >> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >> Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de > > That was my original fix so can confirm this also works. Ok, great. > My reasoning for opting for the reordering was: I think the problem is > uif_close frees up some UBI data structures so we have to ensure no > updating of the filesystem occurs after this. What if > ubi_fastmap_close or ubi_wl_close change in future and these changes > result in updates to the filesystem, the same problem will occur and > for our board it corrupts the UBIFS. So I opted to change the order in > build.c. Hmm... may Richard can comment here, because your change changes code from linux, so if there is a potentiall problem, we should fix it also in linux (or may this is fixed in mainline linux already?) BTW: your patch has checkpatch problems, see my weekly tbot tests: http://xeidos.ddns.net/tbot/id_590/tbot.txt search for "2018-01-16 02:42" to see the wget command to get your patch from patchwork search for "2018-01-16 02:42:19" to see the checkpatch cmd output [33mWARNING:[0m Possible unwrapped commit description (prefer a maximum 75 chars per line) #19: Subject: [PATCH] ubi: Fix filesystem corruption on detach when fastmap enabled [33mWARNING:[0m please, no spaces at the start of a line #42: FILE: drivers/mtd/ubi/build.c:1085: + ubi_wl_close(ubi);$ [33mWARNING:[0m please, no spaces at the start of a line #53: FILE: drivers/mtd/ubi/build.c:1164: + ubi_wl_close(ubi);$ Please fix this also in a v2, thanks! Huch, and search for "2018-01-16 02:42" ... your patch does not apply to mainline U-Boot: 2018-01-16 02:42:20,650:CON :tbotlib # tb_ctrl: Applying: ubi: Fix filesystem corruption on detach when fastmap enabled Using index info to reconstruct a base tree... error: patch failed: drivers/mtd/ubi/build.c:1082 error: drivers/mtd/ubi/build.c: patch does not apply error: Did you hand edit your patch? It does not apply to blobs recorded in its index. Patch failed at 0001 ubi: Fix filesystem corruption on detach when fastmap enabled The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". bye, Heiko
Hi, On Tue, Jan 16, 2018 at 5:43 AM, Heiko Schocher <hs@denx.de> wrote: > Hello Martin, > > added Richard to cc > > > Am 15.01.2018 um 13:13 schrieb Martin Townsend: >> >> Hi Heiko, >> >> >> On Mon, Jan 15, 2018 at 11:30 AM, Heiko Schocher <hs@denx.de> wrote: >>> >>> Hello Martin, >>> >>> >>> Am 12.01.2018 um 20:03 schrieb Martin Townsend: >>>>> >>>>> >>>>> From d35b7ea298fbd6c9d08b1b7132d43b9289d2b914 Mon Sep 17 00:00:00 2001 >>>> >>>> >>>> From: Martin Townsend <mtownsend1973@gmail.com> >>>> Date: Fri, 12 Jan 2018 18:59:23 +0000 >>>> Subject: [PATCH] ubi: Fix filesystem corruption on detach when fastmap >>>> enabled >>>> MIME-Version: 1.0 >>>> Content-Type: text/plain; charset=UTF-8 >>>> Content-Transfer-Encoding: 8bit >>>> >>>> When detaching using "ubi detach" it calls ubi_detach_mtd_dev which >>>> calls ubi_update_fastmap twice when fastmap is enabled. The second >>>> invocation was corrupting the ubifs as it was called after uif_close. >>>> Moved all calls to ubi_wl_close before uif_close. >>>> >>>> Signed-off-by: Martin Townsend <mtownsend1973@gmail.com> >>>> --- >>>> drivers/mtd/ubi/build.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c >>>> index baf4e2d..795ea34 100644 >>>> --- a/drivers/mtd/ubi/build.c >>>> +++ b/drivers/mtd/ubi/build.c >>>> @@ -1082,9 +1082,9 @@ out_debugfs: >>>> out_uif: >>>> get_device(&ubi->dev); >>>> ubi_assert(ref); >>>> + ubi_wl_close(ubi); >>>> uif_close(ubi); >>>> out_detach: >>>> - ubi_wl_close(ubi); >>>> ubi_free_internal_volumes(ubi); >>>> vfree(ubi->vtbl); >>>> out_free: >>>> @@ -1161,9 +1161,9 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway) >>>> get_device(&ubi->dev); >>>> >>>> ubi_debugfs_exit_dev(ubi); >>>> + ubi_wl_close(ubi); >>>> uif_close(ubi); >>>> >>>> - ubi_wl_close(ubi); >>>> ubi_free_internal_volumes(ubi); >>>> vfree(ubi->vtbl); >>>> put_mtd_device(ubi->mtd); >>>> >>> >>> Could you please try the following patch: >>> >>> diff --git a/drivers/mtd/ubi/fastmap-wl.c b/drivers/mtd/ubi/fastmap-wl.c >>> index a33d4063e0..2923d21836 100644 >>> --- a/drivers/mtd/ubi/fastmap-wl.c >>> +++ b/drivers/mtd/ubi/fastmap-wl.c >>> @@ -339,8 +339,6 @@ static void ubi_fastmap_close(struct ubi_device *ubi) >>> >>> #ifndef __UBOOT__ >>> flush_work(&ubi->fm_work); >>> -#else >>> - update_fastmap_work_fn(ubi); >>> #endif >>> return_unused_pool_pebs(ubi, &ubi->fm_pool); >>> return_unused_pool_pebs(ubi, &ubi->fm_wl_pool); >>> >>> Your problem is (I think) because U-Boot Code accidentially calls >>> update_fastmap_work_fn(ubi), but we do not need it here anymore, as >>> U-Boot does all UBI work immediately. >>> >>> bye, >>> Heiko >>> -- >>> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk >>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >>> Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de >> >> >> That was my original fix so can confirm this also works. > > > Ok, great. > >> My reasoning for opting for the reordering was: I think the problem is >> uif_close frees up some UBI data structures so we have to ensure no >> updating of the filesystem occurs after this. What if >> ubi_fastmap_close or ubi_wl_close change in future and these changes >> result in updates to the filesystem, the same problem will occur and >> for our board it corrupts the UBIFS. So I opted to change the order in >> build.c. > > > Hmm... may Richard can comment here, because your change changes > code from linux, so if there is a potentiall problem, we should fix it > also in linux (or may this is fixed in mainline linux already?) > > BTW: your patch has checkpatch problems, see my weekly tbot tests: > > http://xeidos.ddns.net/tbot/id_590/tbot.txt > search for "2018-01-16 02:42" to see the wget command to get your patch > from patchwork > search for "2018-01-16 02:42:19" to see the checkpatch cmd output > > > [33mWARNING: [0m Possible unwrapped commit description (prefer a maximum 75 > chars per line) > #19: > Subject: [PATCH] ubi: Fix filesystem corruption on detach when fastmap > enabled > > [33mWARNING: [0m please, no spaces at the start of a line > #42: FILE: drivers/mtd/ubi/build.c:1085: > + ubi_wl_close(ubi);$ > > [33mWARNING: [0m please, no spaces at the start of a line > #53: FILE: drivers/mtd/ubi/build.c:1164: > + ubi_wl_close(ubi);$ > > Please fix this also in a v2, thanks! > > Huch, and search for "2018-01-16 02:42" ... your patch does not apply > to mainline U-Boot: > > 2018-01-16 02:42:20,650:CON :tbotlib # tb_ctrl: Applying: ubi: Fix > filesystem corruption on detach when fastmap enabled > Using index info to reconstruct a base tree... > error: patch failed: drivers/mtd/ubi/build.c:1082 > error: drivers/mtd/ubi/build.c: patch does not apply > error: Did you hand edit your patch? > It does not apply to blobs recorded in its index. > Patch failed at 0001 ubi: Fix filesystem corruption on detach when fastmap > enabled > The copy of the patch that failed is found in: .git/rebase-apply/patch > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". > Ah. Must be the mail client. Sorry about that I'll setup git send-mail for v2. I'll wait to see what Richard has to say in case there is a better fix. > > bye, > Heiko > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de
Heiko, Martin, Am Dienstag, 16. Januar 2018, 10:11:41 CET schrieb Martin Townsend: > Ah. Must be the mail client. Sorry about that I'll setup git send-mail > for v2. I'll wait to see what Richard has to say in case there is a > better fix. Thanks for letting me know! Indeed, there is a problem. I'm a little astonished that nobody noticed so far. Most likely because while detaching UBI in Linux mostly no Fastmap work is scheduled, and therefore in most cases flush_work(&ubi->fm_work) does nothing. As you noticed in U-Boot the story is different, you always update the Fastmap upon detach. Martin, can you please explain what corruption you see? From reading the code I'd assume that you miss volumes but a full scan would recover everything. For Linux I suggest this fix: From 48287459cf8717cffac5aed423937cd7ba4360ab Mon Sep 17 00:00:00 2001 From: Richard Weinberger <richard@nod.at> Date: Tue, 16 Jan 2018 14:12:46 +0100 Subject: [PATCH] ubi: fastmap: Don't flush fastmap work on detach At this point UBI volumes have already been free()'ed and fastmap can no longer access these data structures. Fixes: 74cdaf24004a ("UBI: Fastmap: Fix memory leaks while closing the WL sub- system") Signed-off-by: Richard Weinberger <richard@nod.at> Cc: stable@vger.kernel.org --- drivers/mtd/ubi/fastmap-wl.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/mtd/ubi/fastmap-wl.c b/drivers/mtd/ubi/fastmap-wl.c index 4f0bd6b4422a..69dd21679a30 100644 --- a/drivers/mtd/ubi/fastmap-wl.c +++ b/drivers/mtd/ubi/fastmap-wl.c @@ -362,7 +362,6 @@ static void ubi_fastmap_close(struct ubi_device *ubi) { int i; - flush_work(&ubi->fm_work); return_unused_pool_pebs(ubi, &ubi->fm_pool); return_unused_pool_pebs(ubi, &ubi->fm_wl_pool);
Hi Richard, On Tue, Jan 16, 2018 at 1:22 PM, Richard Weinberger <richard@sigma-star.at> wrote: > Heiko, Martin, > > Am Dienstag, 16. Januar 2018, 10:11:41 CET schrieb Martin Townsend: >> Ah. Must be the mail client. Sorry about that I'll setup git send-mail >> for v2. I'll wait to see what Richard has to say in case there is a >> better fix. > > Thanks for letting me know! > Indeed, there is a problem. I'm a little astonished that nobody noticed so > far. > Most likely because while detaching UBI in Linux mostly no Fastmap work is > scheduled, > and therefore in most cases flush_work(&ubi->fm_work) does nothing. > > As you noticed in U-Boot the story is different, you always update the Fastmap > upon detach. > > Martin, can you please explain what corruption you see? > From reading the code I'd assume that you miss volumes but a full scan would > recover everything. I didn't do much analysis of the corruption I'm afraid. When I tried to reattach the the c->empty flag was set to true and indeed all the EBA tables were reporting an empty filesystem even after a reboot. Not sure if this was recoverable, how do you trigger a full scan? > > For Linux I suggest this fix: > > From 48287459cf8717cffac5aed423937cd7ba4360ab Mon Sep 17 00:00:00 2001 > From: Richard Weinberger <richard@nod.at> > Date: Tue, 16 Jan 2018 14:12:46 +0100 > Subject: [PATCH] ubi: fastmap: Don't flush fastmap work on detach > > At this point UBI volumes have already been free()'ed and fastmap can no > longer access these data structures. > > Fixes: 74cdaf24004a ("UBI: Fastmap: Fix memory leaks while closing the WL sub- > system") > Signed-off-by: Richard Weinberger <richard@nod.at> > Cc: stable@vger.kernel.org > --- > drivers/mtd/ubi/fastmap-wl.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/mtd/ubi/fastmap-wl.c b/drivers/mtd/ubi/fastmap-wl.c > index 4f0bd6b4422a..69dd21679a30 100644 > --- a/drivers/mtd/ubi/fastmap-wl.c > +++ b/drivers/mtd/ubi/fastmap-wl.c > @@ -362,7 +362,6 @@ static void ubi_fastmap_close(struct ubi_device *ubi) > { > int i; > > - flush_work(&ubi->fm_work); > return_unused_pool_pebs(ubi, &ubi->fm_pool); > return_unused_pool_pebs(ubi, &ubi->fm_wl_pool); > > -- > 2.13.6 > > In U-Boot you can do the same. > > Thanks, > //richard > > -- > sigma star gmbh - Eduard-Bodem-Gasse 6 - 6020 Innsbruck - Austria > ATU66964118 - FN 374287y Richard: This will work but just want to check that ubi_wl_close is supposed to never write out to the filesystem or will never do so in future, if so maybe a something in the comments above ubi_wl_close to ensure this?
Martin, Am Dienstag, 16. Januar 2018, 15:13:04 CET schrieb Martin Townsend: > > Martin, can you please explain what corruption you see? > > From reading the code I'd assume that you miss volumes but a full scan > > would recover everything. > > I didn't do much analysis of the corruption I'm afraid. When I tried > to reattach the the c->empty flag was set to true and indeed all the > EBA tables were reporting an empty filesystem even after a reboot. Sounds like what I'd expect. > Not sure if this was recoverable, how do you trigger a full scan? Booting a kernel without Fastmap support. B-) > > For Linux I suggest this fix: > > > > From 48287459cf8717cffac5aed423937cd7ba4360ab Mon Sep 17 00:00:00 2001 > > From: Richard Weinberger <richard@nod.at> > > Date: Tue, 16 Jan 2018 14:12:46 +0100 > > Subject: [PATCH] ubi: fastmap: Don't flush fastmap work on detach > > > > At this point UBI volumes have already been free()'ed and fastmap can no > > longer access these data structures. > > > > Fixes: 74cdaf24004a ("UBI: Fastmap: Fix memory leaks while closing the WL > > sub- system") > > Signed-off-by: Richard Weinberger <richard@nod.at> > > Cc: stable@vger.kernel.org > > --- > > > > drivers/mtd/ubi/fastmap-wl.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/mtd/ubi/fastmap-wl.c b/drivers/mtd/ubi/fastmap-wl.c > > index 4f0bd6b4422a..69dd21679a30 100644 > > --- a/drivers/mtd/ubi/fastmap-wl.c > > +++ b/drivers/mtd/ubi/fastmap-wl.c > > @@ -362,7 +362,6 @@ static void ubi_fastmap_close(struct ubi_device *ubi) > > > > { > > > > int i; > > > > - flush_work(&ubi->fm_work); > > > > return_unused_pool_pebs(ubi, &ubi->fm_pool); > > return_unused_pool_pebs(ubi, &ubi->fm_wl_pool); > > > > -- > > 2.13.6 > > > > In U-Boot you can do the same. > > Richard: This will work but just want to check that ubi_wl_close is > supposed to never write out to the filesystem or will never do so in > future, if so maybe a something in the comments above ubi_wl_close to > ensure this? Hmm, not sure if I got this question. The filesystem sits above UBI. If we reach ubi_wl_close() all users ontop of UBI are gone. There is no UBIFS mounted anymore. Thanks, //richard
Hi, On Wed, Jan 17, 2018 at 3:47 PM, Richard Weinberger <richard@sigma-star.at> wrote: > Martin, > > Am Dienstag, 16. Januar 2018, 15:13:04 CET schrieb Martin Townsend: >> > Martin, can you please explain what corruption you see? >> > From reading the code I'd assume that you miss volumes but a full scan >> > would recover everything. >> >> I didn't do much analysis of the corruption I'm afraid. When I tried >> to reattach the the c->empty flag was set to true and indeed all the >> EBA tables were reporting an empty filesystem even after a reboot. > > Sounds like what I'd expect. > >> Not sure if this was recoverable, how do you trigger a full scan? > > Booting a kernel without Fastmap support. B-) > >> > For Linux I suggest this fix: >> > >> > From 48287459cf8717cffac5aed423937cd7ba4360ab Mon Sep 17 00:00:00 2001 >> > From: Richard Weinberger <richard@nod.at> >> > Date: Tue, 16 Jan 2018 14:12:46 +0100 >> > Subject: [PATCH] ubi: fastmap: Don't flush fastmap work on detach >> > >> > At this point UBI volumes have already been free()'ed and fastmap can no >> > longer access these data structures. >> > >> > Fixes: 74cdaf24004a ("UBI: Fastmap: Fix memory leaks while closing the WL >> > sub- system") >> > Signed-off-by: Richard Weinberger <richard@nod.at> >> > Cc: stable@vger.kernel.org >> > --- >> > >> > drivers/mtd/ubi/fastmap-wl.c | 1 - >> > 1 file changed, 1 deletion(-) >> > >> > diff --git a/drivers/mtd/ubi/fastmap-wl.c b/drivers/mtd/ubi/fastmap-wl.c >> > index 4f0bd6b4422a..69dd21679a30 100644 >> > --- a/drivers/mtd/ubi/fastmap-wl.c >> > +++ b/drivers/mtd/ubi/fastmap-wl.c >> > @@ -362,7 +362,6 @@ static void ubi_fastmap_close(struct ubi_device *ubi) >> > >> > { >> > >> > int i; >> > >> > - flush_work(&ubi->fm_work); >> > >> > return_unused_pool_pebs(ubi, &ubi->fm_pool); >> > return_unused_pool_pebs(ubi, &ubi->fm_wl_pool); >> > >> > -- >> > 2.13.6 >> > >> > In U-Boot you can do the same. >> >> Richard: This will work but just want to check that ubi_wl_close is >> supposed to never write out to the filesystem or will never do so in >> future, if so maybe a something in the comments above ubi_wl_close to >> ensure this? > > Hmm, not sure if I got this question. > The filesystem sits above UBI. If we reach ubi_wl_close() all users ontop of > UBI are gone. There is no UBIFS mounted anymore. > Thanks for clarifying, I'll submit a version 2 patch that replicates this for U-Boot. > Thanks, > //richard > > -- > sigma star gmbh - Eduard-Bodem-Gasse 6 - 6020 Innsbruck - Austria > ATU66964118 - FN 374287y
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index baf4e2d..795ea34 100644 --- a/drivers/mtd/ubi/build.c +++ b/drivers/mtd/ubi/build.c @@ -1082,9 +1082,9 @@ out_debugfs: out_uif: get_device(&ubi->dev); ubi_assert(ref); + ubi_wl_close(ubi); uif_close(ubi); out_detach: - ubi_wl_close(ubi); ubi_free_internal_volumes(ubi); vfree(ubi->vtbl); out_free: @@ -1161,9 +1161,9 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway) get_device(&ubi->dev); ubi_debugfs_exit_dev(ubi); + ubi_wl_close(ubi); uif_close(ubi); - ubi_wl_close(ubi); ubi_free_internal_volumes(ubi); vfree(ubi->vtbl); put_mtd_device(ubi->mtd);