ubifs_sync_fs() may queue up a new UBI erase transaction, which is
processed in the background:
bash# sync
ubifs_sync_fs: enter
schedule_erase: enter
schedule_erase: exit
ubi_sync: enter
ubi_sync: exit
ubifs_sync_fs: exit
cfi_amdstd_erase_varsize: enter
bash# cfi_amdstd_erase_varsize: exit
Normally this is not a big deal. However, during the final sync before
rebooting, it initiates an erase operation that is potentially still
active when Linux restarts the machine:
bash# reboot -f
ubifs_sync_fs: enter
schedule_erase: enter
schedule_erase: exit
ubi_sync: enter
ubi_sync: exit
ubifs_sync_fs: exit
cfi_amdstd_erase_varsize: enter
Restarting system.
<Flash is stuck in FL_ERASE mode - system hangs>
This is easiest to observe on a NOR flash. One factor is the long erase
time. The other reason is because getting a NOR flash stuck in FL_ERASE
mode will prevent the bootloader from running, unless the board provides
a way for the processor to automatically reset the flash. In my
experience, many boards do not.
My proposal is to add a reboot notifier to let the UBI background thread
terminate gracefully. The new ordering looks like this:
bash# reboot -f
ubifs_sync_fs: enter
schedule_erase: enter
schedule_erase: exit
ubifs_sync_fs: exit
cfi_amdstd_erase_varsize: enter
ubi_reboot_notifier: enter
cfi_amdstd_erase_varsize: exit
ubi_reboot_notifier: exit
cfi_amdstd_reboot: enter
cfi_amdstd_reboot: exit
cfi_amdstd_reboot doesn't really exist, but I added a dummy notifier to
make sure that the ordering would be correct when using drivers that do
have this feature.
Signed-off-by: Kevin Cernekee <[email protected]>
---
drivers/mtd/ubi/build.c | 24 ++++++++++++++++++++++++
drivers/mtd/ubi/ubi.h | 2 ++
2 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 4048db8..ca88059 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -41,6 +41,7 @@
#include <linux/miscdevice.h>
#include <linux/log2.h>
#include <linux/kthread.h>
+#include <linux/reboot.h>
#include "ubi.h"
/* Maximum length of the 'mtd=' parameter */
@@ -726,6 +727,23 @@ static int autoresize(struct ubi_device *ubi, int vol_id)
}
/**
+ * ubi_reboot_notifier - halt UBI transactions immediately prior to a reboot
+ * @n: notifier_block struct (inside our struct ubi_device)
+ * @val: unused
+ * @v: unused
+ */
+static int ubi_reboot_notifier(struct notifier_block *n, unsigned long val,
+ void *v)
+{
+ struct ubi_device *ubi = container_of(n, struct ubi_device,
+ reboot_notifier);
+
+ if (ubi->bgt_thread)
+ kthread_stop(ubi->bgt_thread);
+ return NOTIFY_DONE;
+}
+
+/**
* ubi_attach_mtd_dev - attach an MTD device.
* @mtd: MTD device description object
* @ubi_num: number to assign to the new UBI device
@@ -876,6 +894,11 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, int vid_hdr_offset)
ubi->thread_enabled = 1;
wake_up_process(ubi->bgt_thread);
+ /* Flash device priority is 0. UBI needs to shut down first. */
+ ubi->reboot_notifier.priority = 1;
+ ubi->reboot_notifier.notifier_call = ubi_reboot_notifier;
+ register_reboot_notifier(&ubi->reboot_notifier);
+
ubi_devices[ubi_num] = ubi;
return ubi_num;
@@ -945,6 +968,7 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
* Before freeing anything, we have to stop the background thread to
* prevent it from doing anything on this device while we are freeing.
*/
+ unregister_reboot_notifier(&ubi->reboot_notifier);
if (ubi->bgt_thread)
kthread_stop(ubi->bgt_thread);
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index c055511..44a45b9 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -36,6 +36,7 @@
#include <linux/device.h>
#include <linux/string.h>
#include <linux/vmalloc.h>
+#include <linux/notifier.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/ubi.h>
@@ -420,6 +421,7 @@ struct ubi_device {
struct task_struct *bgt_thread;
int thread_enabled;
char bgt_name[sizeof(UBI_BGT_NAME_PATTERN)+2];
+ struct notifier_block reboot_notifier;
/* I/O sub-system's stuff */
long long flash_size;
--
1.6.3.1
On Mon, 2009-06-08 at 22:14 -0700, Kevin Cernekee wrote:
> ubifs_sync_fs() may queue up a new UBI erase transaction, which is
> processed in the background:
>
> bash# sync
> ubifs_sync_fs: enter
> schedule_erase: enter
> schedule_erase: exit
> ubi_sync: enter
> ubi_sync: exit
> ubifs_sync_fs: exit
> cfi_amdstd_erase_varsize: enter
> bash# cfi_amdstd_erase_varsize: exit
>
> Normally this is not a big deal. However, during the final sync before
> rebooting, it initiates an erase operation that is potentially still
> active when Linux restarts the machine:
>
> bash# reboot -f
> ubifs_sync_fs: enter
> schedule_erase: enter
> schedule_erase: exit
> ubi_sync: enter
> ubi_sync: exit
> ubifs_sync_fs: exit
> cfi_amdstd_erase_varsize: enter
> Restarting system.
> <Flash is stuck in FL_ERASE mode - system hangs>
>
> This is easiest to observe on a NOR flash. One factor is the long erase
> time. The other reason is because getting a NOR flash stuck in FL_ERASE
> mode will prevent the bootloader from running, unless the board provides
> a way for the processor to automatically reset the flash. In my
> experience, many boards do not.
>
> My proposal is to add a reboot notifier to let the UBI background thread
> terminate gracefully. The new ordering looks like this:
>
> bash# reboot -f
> ubifs_sync_fs: enter
> schedule_erase: enter
> schedule_erase: exit
> ubifs_sync_fs: exit
> cfi_amdstd_erase_varsize: enter
> ubi_reboot_notifier: enter
> cfi_amdstd_erase_varsize: exit
> ubi_reboot_notifier: exit
> cfi_amdstd_reboot: enter
> cfi_amdstd_reboot: exit
>
> cfi_amdstd_reboot doesn't really exist, but I added a dummy notifier to
> make sure that the ordering would be correct when using drivers that do
> have this feature.
The idea looks OK. So you basically make sure that if UBI bgt thread
started an erase operation, it finishes it (and only it) before we
reboot. Do I read the patch correctly?
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)
Looks good to me. I'm though wandering if UBIFS should also
register a reboot notifier an sync write-buffers in it...
But this is not related to your patch.
Some nit-picking.
On Mon, 2009-06-08 at 22:14 -0700, Kevin Cernekee wrote:
> /**
> + * ubi_reboot_notifier - halt UBI transactions immediately prior to a reboot
> + * @n: notifier_block struct (inside our struct ubi_device)
> + * @val: unused
> + * @v: unused
> + */
> +static int ubi_reboot_notifier(struct notifier_block *n, unsigned long val,
> + void *v)
> +{
> + struct ubi_device *ubi = container_of(n, struct ubi_device,
> + reboot_notifier);
> +
> + if (ubi->bgt_thread)
> + kthread_stop(ubi->bgt_thread);
> + return NOTIFY_DONE;
> +}
Several small things in comments and indentations. Take a look how
other UBI funcs do things, I'd be happier to keep the code consistent.
> +/**
> * ubi_attach_mtd_dev - attach an MTD device.
> * @mtd: MTD device description object
> * @ubi_num: number to assign to the new UBI device
> @@ -876,6 +894,11 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, int vid_hdr_offset)
> ubi->thread_enabled = 1;
> wake_up_process(ubi->bgt_thread);
>
> + /* Flash device priority is 0. UBI needs to shut down first. */
> + ubi->reboot_notifier.priority = 1;
> + ubi->reboot_notifier.notifier_call = ubi_reboot_notifier;
> + register_reboot_notifier(&ubi->reboot_notifier);
> +
> ubi_devices[ubi_num] = ubi;
> return ubi_num;
>
> @@ -945,6 +968,7 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
> * Before freeing anything, we have to stop the background thread to
> * prevent it from doing anything on this device while we are freeing.
> */
> + unregister_reboot_notifier(&ubi->reboot_notifier);
> if (ubi->bgt_thread)
> kthread_stop(ubi->bgt_thread);
>
> diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
> index c055511..44a45b9 100644
> --- a/drivers/mtd/ubi/ubi.h
> +++ b/drivers/mtd/ubi/ubi.h
> @@ -36,6 +36,7 @@
> #include <linux/device.h>
> #include <linux/string.h>
> #include <linux/vmalloc.h>
> +#include <linux/notifier.h>
> #include <linux/mtd/mtd.h>
> #include <linux/mtd/ubi.h>
>
> @@ -420,6 +421,7 @@ struct ubi_device {
> struct task_struct *bgt_thread;
> int thread_enabled;
> char bgt_name[sizeof(UBI_BGT_NAME_PATTERN)+2];
> + struct notifier_block reboot_notifier;
How about commenting this field above the struct ubi_device
definition?
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)
Terminate the UBI background thread prior to restarting the system.
Changes from v1:
Add ubi_sync() at the end, to make sure the underlying MTD device has
synchronized its buffers.
Clean up comments and formatting.
Signed-off-by: Kevin Cernekee <[email protected]>
---
drivers/mtd/ubi/build.c | 30 ++++++++++++++++++++++++++++++
drivers/mtd/ubi/ubi.h | 3 +++
2 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 4048db8..18b42c9 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -41,6 +41,7 @@
#include <linux/miscdevice.h>
#include <linux/log2.h>
#include <linux/kthread.h>
+#include <linux/reboot.h>
#include "ubi.h"
/* Maximum length of the 'mtd=' parameter */
@@ -726,6 +727,29 @@ static int autoresize(struct ubi_device *ubi, int vol_id)
}
/**
+ * ubi_reboot_notifier - halt UBI transactions immediately prior to a reboot.
+ * @n: reboot notifier object
+ * @state: SYS_RESTART, SYS_HALT, or SYS_POWER_OFF
+ * @cmd: pointer to command string for RESTART2
+ *
+ * This function stops the UBI background thread so that the flash device
+ * remains quiescent when Linux restarts the system. Any queued work will
+ * be discarded, but this function will block until do_work() finishes if
+ * an operation is already in progress.
+ */
+static int ubi_reboot_notifier(struct notifier_block *n, unsigned long state,
+ void *cmd)
+{
+ struct ubi_device *ubi;
+
+ ubi = container_of(n, struct ubi_device, reboot_notifier);
+ if (ubi->bgt_thread)
+ kthread_stop(ubi->bgt_thread);
+ ubi_sync(ubi->ubi_num);
+ return NOTIFY_DONE;
+}
+
+/**
* ubi_attach_mtd_dev - attach an MTD device.
* @mtd: MTD device description object
* @ubi_num: number to assign to the new UBI device
@@ -876,6 +900,11 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, int vid_hdr_offset)
ubi->thread_enabled = 1;
wake_up_process(ubi->bgt_thread);
+ /* Flash device priority is 0 - UBI needs to shut down first */
+ ubi->reboot_notifier.priority = 1;
+ ubi->reboot_notifier.notifier_call = ubi_reboot_notifier;
+ register_reboot_notifier(&ubi->reboot_notifier);
+
ubi_devices[ubi_num] = ubi;
return ubi_num;
@@ -945,6 +974,7 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
* Before freeing anything, we have to stop the background thread to
* prevent it from doing anything on this device while we are freeing.
*/
+ unregister_reboot_notifier(&ubi->reboot_notifier);
if (ubi->bgt_thread)
kthread_stop(ubi->bgt_thread);
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index c055511..c8fab4a 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -36,6 +36,7 @@
#include <linux/device.h>
#include <linux/string.h>
#include <linux/vmalloc.h>
+#include <linux/notifier.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/ubi.h>
@@ -339,6 +340,7 @@ struct ubi_wl_entry;
* @bgt_thread: background thread description object
* @thread_enabled: if the background thread is enabled
* @bgt_name: background thread name
+ * @reboot_notifier: notifier to terminate background thread before rebooting
*
* @flash_size: underlying MTD device size (in bytes)
* @peb_count: count of physical eraseblocks on the MTD device
@@ -420,6 +422,7 @@ struct ubi_device {
struct task_struct *bgt_thread;
int thread_enabled;
char bgt_name[sizeof(UBI_BGT_NAME_PATTERN)+2];
+ struct notifier_block reboot_notifier;
/* I/O sub-system's stuff */
long long flash_size;
--
1.6.3.1
On Tue, 2009-06-09 at 10:59 -0700, Kevin Cernekee wrote:
> Terminate the UBI background thread prior to restarting the system.
>
> Changes from v1:
>
> Add ubi_sync() at the end, to make sure the underlying MTD device has
> synchronized its buffers.
>
> Clean up comments and formatting.
>
> Signed-off-by: Kevin Cernekee <[email protected]>
I've pushed this patch to ubi-2.6.git/master. I've tested it
even. And I amended comments a little. Please, check it:
http://git.infradead.org/ubi-2.6.git?a=commit;h=79c293b2629882a1db3a850ed6b84bd1d8a464a7
If you are fine with this, I'll put it to ubi-2.6.git/linux-next
and merge it this merge window.
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)
On Wed, 2009-06-10 at 12:35 +0300, Artem Bityutskiy wrote:
> On Tue, 2009-06-09 at 10:59 -0700, Kevin Cernekee wrote:
> > Terminate the UBI background thread prior to restarting the system.
> >
> > Changes from v1:
> >
> > Add ubi_sync() at the end, to make sure the underlying MTD device has
> > synchronized its buffers.
> >
> > Clean up comments and formatting.
> >
> > Signed-off-by: Kevin Cernekee <[email protected]>
>
> I've pushed this patch to ubi-2.6.git/master. I've tested it
> even. And I amended comments a little. Please, check it:
>
> http://git.infradead.org/ubi-2.6.git?a=commit;h=79c293b2629882a1db3a850ed6b84bd1d8a464a7
>
> If you are fine with this, I'll put it to ubi-2.6.git/linux-next
> and merge it this merge window.
Ok, I've actually pushed this to ubi-2.6.git/linux-next, I just
assume you should not have issues with my tweaks.
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)
On Wed, Jun 10, 2009 at 2:35 AM, Artem Bityutskiy<[email protected]> wrote:
> I've pushed this patch to ubi-2.6.git/master. I've tested it
> even. And I amended comments a little. Please, check it:
>
> http://git.infradead.org/ubi-2.6.git?a=commit;h=79c293b2629882a1db3a850ed6b84bd1d8a464a7
>
> If you are fine with this, I'll put it to ubi-2.6.git/linux-next
> and merge it this merge window.
Looks good to me. Thanks.
FYI, I do not like this patch, because it is not solving the problem, it
just makes the problem less likely. I.e., you are still screwed if you
reboot while some other task is doing erasure. Indeed, the background
thread is not the only task which may do erases.
I'm not going to revert this patch, but wanted to let you know that I
think it was my mistake to push it.
On Tue, 2009-06-09 at 10:59 -0700, Kevin Cernekee wrote:
> Terminate the UBI background thread prior to restarting the system.
>
> Changes from v1:
>
> Add ubi_sync() at the end, to make sure the underlying MTD device has
> synchronized its buffers.
>
> Clean up comments and formatting.
>
> Signed-off-by: Kevin Cernekee <[email protected]>
> ---
> drivers/mtd/ubi/build.c | 30 ++++++++++++++++++++++++++++++
> drivers/mtd/ubi/ubi.h | 3 +++
> 2 files changed, 33 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index 4048db8..18b42c9 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -41,6 +41,7 @@
> #include <linux/miscdevice.h>
> #include <linux/log2.h>
> #include <linux/kthread.h>
> +#include <linux/reboot.h>
> #include "ubi.h"
>
> /* Maximum length of the 'mtd=' parameter */
> @@ -726,6 +727,29 @@ static int autoresize(struct ubi_device *ubi, int vol_id)
> }
>
> /**
> + * ubi_reboot_notifier - halt UBI transactions immediately prior to a reboot.
> + * @n: reboot notifier object
> + * @state: SYS_RESTART, SYS_HALT, or SYS_POWER_OFF
> + * @cmd: pointer to command string for RESTART2
> + *
> + * This function stops the UBI background thread so that the flash device
> + * remains quiescent when Linux restarts the system. Any queued work will
> + * be discarded, but this function will block until do_work() finishes if
> + * an operation is already in progress.
> + */
> +static int ubi_reboot_notifier(struct notifier_block *n, unsigned long state,
> + void *cmd)
> +{
> + struct ubi_device *ubi;
> +
> + ubi = container_of(n, struct ubi_device, reboot_notifier);
> + if (ubi->bgt_thread)
> + kthread_stop(ubi->bgt_thread);
> + ubi_sync(ubi->ubi_num);
> + return NOTIFY_DONE;
> +}
> +
> +/**
> * ubi_attach_mtd_dev - attach an MTD device.
> * @mtd: MTD device description object
> * @ubi_num: number to assign to the new UBI device
> @@ -876,6 +900,11 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, int vid_hdr_offset)
> ubi->thread_enabled = 1;
> wake_up_process(ubi->bgt_thread);
>
> + /* Flash device priority is 0 - UBI needs to shut down first */
> + ubi->reboot_notifier.priority = 1;
> + ubi->reboot_notifier.notifier_call = ubi_reboot_notifier;
> + register_reboot_notifier(&ubi->reboot_notifier);
> +
> ubi_devices[ubi_num] = ubi;
> return ubi_num;
>
> @@ -945,6 +974,7 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
> * Before freeing anything, we have to stop the background thread to
> * prevent it from doing anything on this device while we are freeing.
> */
> + unregister_reboot_notifier(&ubi->reboot_notifier);
> if (ubi->bgt_thread)
> kthread_stop(ubi->bgt_thread);
>
> diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
> index c055511..c8fab4a 100644
> --- a/drivers/mtd/ubi/ubi.h
> +++ b/drivers/mtd/ubi/ubi.h
> @@ -36,6 +36,7 @@
> #include <linux/device.h>
> #include <linux/string.h>
> #include <linux/vmalloc.h>
> +#include <linux/notifier.h>
> #include <linux/mtd/mtd.h>
> #include <linux/mtd/ubi.h>
>
> @@ -339,6 +340,7 @@ struct ubi_wl_entry;
> * @bgt_thread: background thread description object
> * @thread_enabled: if the background thread is enabled
> * @bgt_name: background thread name
> + * @reboot_notifier: notifier to terminate background thread before rebooting
> *
> * @flash_size: underlying MTD device size (in bytes)
> * @peb_count: count of physical eraseblocks on the MTD device
> @@ -420,6 +422,7 @@ struct ubi_device {
> struct task_struct *bgt_thread;
> int thread_enabled;
> char bgt_name[sizeof(UBI_BGT_NAME_PATTERN)+2];
> + struct notifier_block reboot_notifier;
>
> /* I/O sub-system's stuff */
> long long flash_size;
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
Artem Bityutskiy wrote:
> FYI, I do not like this patch, because it is not solving the problem, it
> just makes the problem less likely. I.e., you are still screwed if you
> reboot while some other task is doing erasure. Indeed, the background
> thread is not the only task which may do erases.
>
> I'm not going to revert this patch, but wanted to let you know that I
> think it was my mistake to push it.
>
so, how to solve this properly ?
probably the only way is the reboot notifier of the MTD chip driver
(for NOR flash only cmdset_0001 has one).
On Fri, 12 Feb 2010, Norbert van Bolhuis wrote:
> so, how to solve this properly ?
In hardware.
Anything we do in software will be unreliable, and by masking the problem
we do a disservice to anyone who actually tests their prototype hardware,
because we might hide the problem from them until it's too late for
them to fix it.
--
dwmw2
On Fri, 2010-02-12 at 14:45 +0100, Norbert van Bolhuis wrote:
> Artem Bityutskiy wrote:
> > FYI, I do not like this patch, because it is not solving the problem, it
> > just makes the problem less likely. I.e., you are still screwed if you
> > reboot while some other task is doing erasure. Indeed, the background
> > thread is not the only task which may do erases.
> >
> > I'm not going to revert this patch, but wanted to let you know that I
> > think it was my mistake to push it.
> >
>
> so, how to solve this properly ?
>
> probably the only way is the reboot notifier of the MTD chip driver
> (for NOR flash only cmdset_0001 has one).
Well. UBI itself does not have problems with half-erased eraseblocks. As
the comment to the patch says, this is for a strange platform where the
boot loader panics when it encounters a half-erased EB.
So the most proper fix is to fix the boot-loader. Remember, a power cut
can introduce half-erased EBs, and you cannot address this in SW.
For clean reboots, you would need to have a reboot notifier in the MTD
lever, not in UBI. This notifier should:
1. Finish the current erase operation.
2. Prevent further erase operations.
The UBI patch is not a good solution.
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)