https://bugzilla.kernel.org/show_bug.cgi?id=25832
--- Comment #93 from Alan Stern <[email protected]> 2011-09-05 17:44:30 ---
It causes the ext4 driver to used for ext2 and ext3 filesystems, instead of
using the ext2 and ext3 drivers.
--
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
Bugzilla.kernel.org is down, so apologies to people who have
subscribed to this bug but which I didn't cc explicitly...
Rocko, Alan, could you try this patch and see what happens. It may be
that we'll crash somewhere else; the problem is that Linux that the
low-level generic hd routines doesn't have a formal way of informing
the VFS and layers below that the disk has disappeared. It just yanks
it out from under the file system, and we've been manually patching
around kernel crashes....
- Ted
commit 6e478d46e58181ec4814f25a2fd91c6323e16ad4
Author: Theodore Ts'o <[email protected]>
Date: Fri Sep 9 15:02:54 2011 -0400
ext4: add ext4-specific kludge to avoid an oops after the disk disappears
The del_gendisk() function uninitializes the disk-specific data
structures, including the bdi structure, without telling anyone
else. Once this happens, any attempt to call mark_buffer_dirty()
(for example, by ext4_commit_super), will cause a kernel OOPS.
Fix this for now until we can fix things in an architecturally correct
way.
Signed-off-by: "Theodore Ts'o" <[email protected]>
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index ee2f74a..48cb615 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -414,6 +414,22 @@ static void save_error_info(struct super_block *sb, const char *func,
ext4_commit_super(sb, 1);
}
+/*
+ * The del_gendisk() function deactivates the inode and deactivates
+ * the bdi without telling the file system. Once this happens, any
+ * attempt to call mark_buffer_dirty() (for example, by
+ * ext4_commit_super), will cause a kernel OOPS. This is a kludge to
+ * prevent these oops until we can put in a proper hook in
+ * del_gendisk() to inform the VFS and file system layers.
+ */
+static int block_device_ejected(struct super_block *sb)
+{
+ struct inode *bd_inode = sb->s_bdev->bd_inode;
+ struct backing_dev_info *bdi = bd_inode->i_mapping->backing_dev_info;
+
+ return bdi->dev == NULL;
+}
+
/* Deal with the reporting of failure conditions on a filesystem such as
* inconsistencies detected or read IO failures.
@@ -4072,7 +4088,7 @@ static int ext4_commit_super(struct super_block *sb, int sync)
struct buffer_head *sbh = EXT4_SB(sb)->s_sbh;
int error = 0;
- if (!sbh)
+ if (!sbh || block_device_ejected(sb))
return error;
if (buffer_write_io_error(sbh)) {
/*
On Fri, 9 Sep 2011, Ted Ts'o wrote:
> Rocko, Alan, could you try this patch and see what happens. It may be
> that we'll crash somewhere else; the problem is that Linux that the
> low-level generic hd routines doesn't have a formal way of informing
> the VFS and layers below that the disk has disappeared. It just yanks
> it out from under the file system, and we've been manually patching
> around kernel crashes....
I confirm that this patch fixes the issue on with my test script. The
unmounts occurred with no apparent problems.
However you probably should make the same change to the ext3 driver,
because it has exactly the same issue and some people may still be
using it.
Alan Stern
On Sat, Sep 10, 2011 at 01:49:44AM +0000, Rocko Requin wrote:
>
> The patch does stop my console-only test script from crashing the
> kernel (thanks for figuring this patch out, Ted!), but if I try it
> from a desktop, the desktop still freezes after two or three
> bind/unbind iterations. So I guess there must be another way to try
> and access the missing file system that also need patching.
Can you get stack traces or register information? Via sysrq-t /
sysrq-p? This might require setting up serial console on your desktop
if you don't have kernel mode switching set up so you can switch away
from the X server.
- Ted
On Fri, 9 Sep 2011, Ted Ts'o wrote:
> commit 6e478d46e58181ec4814f25a2fd91c6323e16ad4
> Author: Theodore Ts'o <[email protected]>
> Date: Fri Sep 9 15:02:54 2011 -0400
>
> ext4: add ext4-specific kludge to avoid an oops after the disk disappears
>
> The del_gendisk() function uninitializes the disk-specific data
> structures, including the bdi structure, without telling anyone
> else. Once this happens, any attempt to call mark_buffer_dirty()
> (for example, by ext4_commit_super), will cause a kernel OOPS.
>
> Fix this for now until we can fix things in an architecturally correct
> way.
>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
Further testing revealed the following problem. I changed the test
script so that after the USB device is unbound, the script tries to
write a file before unmounting the ext4 filesystem.
There was no drastic failure; the unregistered bdi structure wasn't
accessed. But lockdep complained. This is what I got:
[ 166.932194] end_request: I/O error, dev uba, sector 136
[ 166.940903] EXT4-fs error (device uba): ext4_find_entry:934: inode #2: comm sh: reading directory lblock 0
[ 166.949284] end_request: I/O error, dev uba, sector 164
[ 166.952084] EXT4-fs error (device uba): ext4_read_inode_bitmap:161: comm sh: Cannot read inode bitmap - block_group = 0, inode_bitmap = 82
[ 166.952906] EXT4-fs error (device uba) in ext4_new_inode:1073: IO failure
[ 166.953357]
[ 166.953381] =============================================
[ 166.953624] [ INFO: possible recursive locking detected ]
[ 166.953958] 3.1.0-rc4 #34
[ 166.954099] ---------------------------------------------
[ 166.954295] sh/819 is trying to acquire lock:
[ 166.954613] (&sb->s_type->i_mutex_key#9){+.+.+.}, at: [<c1101290>] ext4_evict_inode+0x17/0x288
[ 166.955947]
[ 166.955969] but task is already holding lock:
[ 166.956281] (&sb->s_type->i_mutex_key#9){+.+.+.}, at: [<c10aeb45>] do_last+0x165/0x4ff
[ 166.956586]
[ 166.956586] other info that might help us debug this:
[ 166.956586] Possible unsafe locking scenario:
[ 166.956586]
[ 166.956586] CPU0
[ 166.956586] ----
[ 166.956586] lock(&sb->s_type->i_mutex_key);
[ 166.956586] lock(&sb->s_type->i_mutex_key);
[ 166.956586]
[ 166.956586] *** DEADLOCK ***
[ 166.956586]
[ 166.956586] May be due to missing lock nesting notation
[ 166.956586]
[ 166.956586] 2 locks held by sh/819:
[ 166.956586] #0: (&sb->s_type->i_mutex_key#9){+.+.+.}, at: [<c10aeb45>] do_last+0x165/0x4ff
[ 166.956586] #1: (jbd2_handle){+.+...}, at: [<c112469f>] start_this_handle+0x3c2/0x41e
[ 166.956586]
[ 166.956586] stack backtrace:
[ 166.956586] Pid: 819, comm: sh Not tainted 3.1.0-rc4 #34
[ 166.956586] Call Trace:
[ 166.956586] [<c135f26e>] ? printk+0xf/0x11
[ 166.956586] [<c105223c>] __lock_acquire+0x875/0xbe7
[ 166.956586] [<c1361600>] ? _raw_spin_unlock_irq+0x2d/0x30
[ 166.956586] [<c105183a>] ? mark_lock+0x26/0x1b3
[ 166.956586] [<c105183a>] ? mark_lock+0x26/0x1b3
[ 166.956586] [<c1052944>] lock_acquire+0x59/0x70
[ 166.956586] [<c1101290>] ? ext4_evict_inode+0x17/0x288
[ 166.956586] [<c13601f9>] __mutex_lock_common+0x38/0x2d4
[ 166.956586] [<c1101290>] ? ext4_evict_inode+0x17/0x288
[ 166.956586] [<c1360573>] mutex_lock_nested+0x32/0x3b
[ 166.956586] [<c1101290>] ? ext4_evict_inode+0x17/0x288
[ 166.956586] [<c1101290>] ext4_evict_inode+0x17/0x288
[ 166.956586] [<c10b5f63>] evict+0x7b/0x11c
[ 166.956586] [<c10b6136>] iput+0x132/0x137
[ 166.956586] [<c10fc467>] ext4_new_inode+0xa53/0xa92
[ 166.956586] [<c1108942>] ? ext4_journal_start_sb+0xdd/0xec
[ 166.956586] [<c10b4afb>] ? d_splice_alias+0xa9/0xb1
[ 166.956586] [<c11045ec>] ext4_create+0xa6/0x10b
[ 166.956586] [<c10ae2d7>] vfs_create+0x61/0x7b
[ 166.956586] [<c10aebd7>] do_last+0x1f7/0x4ff
[ 166.956586] [<c10aefa1>] path_openat+0x9d/0x2b7
[ 166.956586] [<c1052636>] ? lock_release_non_nested+0x88/0x1f7
[ 166.956586] [<c10af1f3>] do_filp_open+0x21/0x5d
[ 166.956586] [<c1361666>] ? _raw_spin_unlock+0x1d/0x2a
[ 166.956586] [<c10b78b1>] ? alloc_fd+0xc0/0xcb
[ 166.956586] [<c10a4207>] do_sys_open+0x54/0xcd
[ 166.956586] [<c10a429e>] sys_open+0x1e/0x26
[ 166.956586] [<c1361820>] syscall_call+0x7/0xb
[ 167.175766] end_request: I/O error, dev uba, sector 16534
[ 167.177204] Aborting journal on device uba-8.
[ 167.179255] end_request: I/O error, dev uba, sector 16516
[ 167.179768] Buffer I/O error on device uba, logical block 8258
[ 167.179983] lost page write due to I/O error on uba
[ 167.180866] JBD2: I/O error detected when updating journal superblock for uba-8.
[ 167.181956] journal commit I/O error
[ 167.195334] EXT4-fs error (device uba): ext4_put_super:817: Couldn't clean up the journal
[ 167.195777] EXT4-fs (uba): Remounting filesystem read-only
It appears to be an unrelated error, but worth looking at.
Alan Stern
Ted:
You also ought to look at this bug report and the follow-up thread.
The symptoms are similar, although not exactly the same.
http://marc.info/?l=linux-kernel&m=131504588401397&w=2
([BUG] D state process after unplug and umount usb disk)
Alan Stern
On Thu, 15 Sep 2011, Rocko Requin wrote:
> Unfortunately the lockup is complete - I can't switch away from the X
> server and sysrq-t/p doesn't work if I'm in a tty console when it
> happens. The stack traces are like the ones I posted earlier in the
> bug, and they didn't contain any useful information.
Try applying the patch below. It will print out some extra debugging
information during normal operation and especially when the USB drive
is mounted and unmounted. Oh yes -- and be certain to run the test
from a tty console so that the messages don't get lost. Maybe you can
capture the log messages using a network console.
This may not give any useful information in the end, because it
concentrates on the BDI interface which Ted's patch should have fixed.
If something else is causing your crashes, you might not see anything.
But it's worth a try.
Alan Stern
Index: usb-3.1/kernel/timer.c
===================================================================
--- usb-3.1.orig/kernel/timer.c
+++ usb-3.1/kernel/timer.c
@@ -111,6 +111,143 @@ timer_set_base(struct timer_list *timer,
tbase_get_deferrable(timer->base));
}
+static void check_timer_list(struct list_head *start, char *name)
+{
+ struct timer_list *t, *tnext, *tprev, *nt;
+ struct list_head *h = start;
+
+ nt = NULL;
+ do {
+ if (!h->next || !h->prev) {
+ nt = list_entry(h, struct timer_list, entry);
+ break;
+ }
+ h = h->next;
+ } while (h != start);
+ if (!nt)
+ return;
+ pr_err("%s: Found bad timer at %p\n", name, nt);
+
+ tnext = tprev = list_entry(start, struct timer_list, entry);
+ list_for_each_entry(t, start, entry) {
+ if (!t)
+ break;
+ pr_info(" Entry %p cb %pS list %p\n", t, t->function,
+ t->entry.prev);
+ if (t == nt)
+ break;
+ tprev = t;
+ }
+ pr_info(" -----\n");
+
+ tnext = list_entry(start, struct timer_list, entry);
+ list_for_each_entry_reverse(t, start, entry) {
+ if (!t) {
+ pr_info(" Broken link\n");
+ break;
+ }
+ if (t == nt)
+ break;
+ pr_info(" Entry %p cb %pS list %p\n", t, t->function,
+ t->entry.next);
+ tnext = t;
+ }
+ pr_info(" ----- Fixing\n");
+ nt->entry.prev = &tprev->entry;
+ tprev->entry.next = &nt->entry;
+ nt->entry.next = &tnext->entry;
+ tnext->entry.prev = &nt->entry;
+}
+
+struct timer_list *alantimer;
+int alanok;
+
+#include <linux/perf_event.h>
+#include <linux/hw_breakpoint.h>
+
+struct perf_event * __percpu *alanhbp;
+unsigned long alanunused;
+int alanhbp_enabled;
+struct list_head **alanptr;
+
+extern void *last_bdi_unreg;
+
+static void check_alan(char *type)
+{
+ if (!alanok)
+ return;
+ if (!alantimer->entry.next || !alantimer->entry.prev) {
+ pr_err("ERROR %s: Bad alantimer %p\n", type, alantimer);
+ alanok = 0;
+ }
+}
+
+static void alanhbp_handler(struct perf_event *bp,
+ struct perf_sample_data *data,
+ struct pt_regs *regs)
+{
+ pr_info("*alanptr written: %p\n", *alanptr);
+ if (!alanok || !alanhbp_enabled)
+ return;
+ if (alantimer->entry.next)
+ return;
+ dump_stack();
+}
+
+static void set_alan(struct timer_list *timer)
+{
+ if (alantimer)
+ return;
+ alantimer = timer;
+ alanok = 1;
+
+ if (alanhbp)
+ alanhbp_enabled = (alanptr == &alantimer->entry.next);
+}
+
+static void clear_alan(struct timer_list *timer)
+{
+ if (alantimer != timer)
+ return;
+ alanok = 0;
+ alantimer = NULL;
+ alanhbp_enabled = 0;
+}
+
+void init_alan(unsigned long addr)
+{
+ struct perf_event_attr attr;
+
+ if (alanhbp) {
+ unregister_wide_hw_breakpoint(alanhbp);
+ alanhbp = NULL;
+ alanhbp_enabled = 0;
+ }
+
+ if (addr) {
+ hw_breakpoint_init(&attr);
+ attr.bp_addr = addr;
+ attr.bp_len = HW_BREAKPOINT_LEN_4;
+ attr.bp_type = HW_BREAKPOINT_W;
+ alanhbp = register_wide_hw_breakpoint(&attr, alanhbp_handler,
+ NULL);
+ if (IS_ERR((void __force *) alanhbp)) {
+ pr_info("Breakpoint reg failed %ld\n",
+ PTR_ERR((void __force *) alanhbp));
+ alanhbp = NULL;
+ } else if (!alanhbp) {
+ pr_info("alanhbp was not created\n");
+ } else {
+ pr_info("alanhbp created\n");
+ }
+
+ alanptr = (struct list_head **) addr;
+ alanhbp_enabled = (alanok && alanptr == &alantimer->entry.next);
+ pr_info("alanhbp set for %p\n", alanptr);
+ }
+}
+EXPORT_SYMBOL(init_alan);
+
static unsigned long round_jiffies_common(unsigned long j, int cpu,
bool force_up)
{
@@ -330,6 +467,9 @@ void set_timer_slack(struct timer_list *
}
EXPORT_SYMBOL_GPL(set_timer_slack);
+extern void wakeup_timer_fn(unsigned long data);
+#include <linux/backing-dev.h>
+
static void internal_add_timer(struct tvec_base *base, struct timer_list *timer)
{
unsigned long expires = timer->expires;
@@ -369,7 +509,17 @@ static void internal_add_timer(struct tv
/*
* Timers are FIFO:
*/
+check_timer_list(vec, "internal_add_1");
+if (timer->function == wakeup_timer_fn) {
+ struct backing_dev_info *bdi = (struct backing_dev_info *) timer->data;
+
+ pr_info("Adding wakeup %p: bdi %p name %s\n", timer, bdi, bdi->name);
+}
list_add_tail(&timer->entry, vec);
+if (timer->function == wakeup_timer_fn)
+ set_alan(timer);
+check_timer_list(vec, "internal_add_2");
+check_alan("add");
}
#ifdef CONFIG_TIMER_STATS
@@ -608,17 +758,24 @@ void init_timer_deferrable_key(struct ti
}
EXPORT_SYMBOL(init_timer_deferrable_key);
-static inline void detach_timer(struct timer_list *timer,
+static void detach_timer(struct timer_list *timer,
int clear_pending)
{
struct list_head *entry = &timer->entry;
+check_alan("detach 1");
+if (timer->function == wakeup_timer_fn) {
+ pr_info("Detaching wakeup %p\n", timer);
+ clear_alan(timer);
+}
+
debug_deactivate(timer);
__list_del(entry->prev, entry->next);
if (clear_pending)
entry->next = NULL;
entry->prev = LIST_POISON2;
+check_alan("detach 2");
}
/*
@@ -1026,6 +1183,7 @@ static int cascade(struct tvec_base *bas
struct list_head tv_list;
list_replace_init(tv->vec + index, &tv_list);
+check_alan("cascade 1");
/*
* We are removing _all_ timers from the list, so we
@@ -1033,7 +1191,10 @@ static int cascade(struct tvec_base *bas
*/
list_for_each_entry_safe(timer, tmp, &tv_list, entry) {
BUG_ON(tbase_get_base(timer->base) != base);
+if (timer->function == wakeup_timer_fn)
+ pr_info("Cascading wakeup_timer %p\n", timer);
internal_add_timer(base, timer);
+check_alan("cascade 2");
}
return index;
@@ -1109,6 +1270,7 @@ static inline void __run_timers(struct t
cascade(base, &base->tv5, INDEX(3));
++base->timer_jiffies;
list_replace_init(base->tv1.vec + index, &work_list);
+check_alan("run 1");
while (!list_empty(head)) {
void (*fn)(unsigned long);
unsigned long data;
@@ -1148,6 +1310,7 @@ static unsigned long __next_timer_interr
/* Look for timer events in tv1. */
index = slot = timer_jiffies & TVR_MASK;
do {
+check_timer_list(base->tv1.vec + slot, "next_timer_1");
list_for_each_entry(nte, base->tv1.vec + slot, entry) {
if (tbase_get_deferrable(nte->base))
continue;
@@ -1179,6 +1342,7 @@ cascade:
index = slot = timer_jiffies & TVN_MASK;
do {
+check_timer_list(varp->vec + slot, "next_timer_2");
list_for_each_entry(nte, varp->vec + slot, entry) {
if (tbase_get_deferrable(nte->base))
continue;
Index: usb-3.1/mm/backing-dev.c
===================================================================
--- usb-3.1.orig/mm/backing-dev.c
+++ usb-3.1/mm/backing-dev.c
@@ -308,7 +308,7 @@ static void sync_supers_timer_fn(unsigne
bdi_arm_supers_timer();
}
-static void wakeup_timer_fn(unsigned long data)
+void wakeup_timer_fn(unsigned long data)
{
struct backing_dev_info *bdi = (struct backing_dev_info *)data;
@@ -328,6 +328,8 @@ static void wakeup_timer_fn(unsigned lon
spin_unlock_bh(&bdi->wb_lock);
}
+void *last_bdi_unreg;
+
/*
* This function is used when the first inode for this bdi is marked dirty. It
* wakes-up the corresponding bdi thread which should then take care of the
@@ -345,6 +347,8 @@ void bdi_wakeup_thread_delayed(struct ba
timeout = msecs_to_jiffies(dirty_writeback_interval * 10);
mod_timer(&bdi->wb.wakeup_timer, jiffies + timeout);
+if (bdi == last_bdi_unreg)
+ dump_stack();
}
/*
@@ -547,6 +551,7 @@ int bdi_register(struct backing_dev_info
return PTR_ERR(wb->task);
}
+pr_info("bdi register %s %p\n", dev_name(dev), bdi);
bdi_debug_register(bdi, dev_name(dev));
set_bit(BDI_registered, &bdi->state);
@@ -617,6 +622,8 @@ void bdi_unregister(struct backing_dev_i
bdi_set_min_ratio(bdi, 0);
trace_writeback_bdi_unregister(bdi);
bdi_prune_sb(bdi);
+pr_info("bdi_unreg: wb %p bdi %p\n", &bdi->wb, bdi);
+last_bdi_unreg = bdi;
del_timer_sync(&bdi->wb.wakeup_timer);
if (!bdi_cap_flush_forker(bdi))
@@ -632,6 +639,8 @@ static void bdi_wb_init(struct bdi_write
{
memset(wb, 0, sizeof(*wb));
+pr_info("bdi_wb_init: wb %p bdi %p\n", wb, bdi);
+last_bdi_unreg = NULL;
wb->bdi = bdi;
wb->last_old_flush = jiffies;
INIT_LIST_HEAD(&wb->b_dirty);
Index: usb-3.1/drivers/usb/core/usb.c
===================================================================
--- usb-3.1.orig/drivers/usb/core/usb.c
+++ usb-3.1/drivers/usb/core/usb.c
@@ -974,6 +974,29 @@ struct dentry *usb_debug_root;
EXPORT_SYMBOL_GPL(usb_debug_root);
static struct dentry *usb_debug_devices;
+static struct dentry *alan_dentry;
+
+static ssize_t alan_write(struct file *fd, const char __user *buf,
+ size_t len, loff_t *ptr)
+{
+ unsigned long addr;
+ char buf2[16];
+ void init_alan(unsigned long);
+
+ if (len >= 16)
+ return -EINVAL;
+ buf2[len] = 0;
+ if (copy_from_user(buf2, buf, len))
+ return -EFAULT;
+
+ addr = simple_strtoul(buf2, NULL, 16);
+ init_alan(addr);
+ return len;
+}
+
+static const struct file_operations alan_fops = {
+ .write = alan_write,
+};
static int usb_debugfs_init(void)
{
@@ -990,11 +1013,17 @@ static int usb_debugfs_init(void)
return -ENOENT;
}
+ alan_dentry = debugfs_create_file("alan", 0200,
+ usb_debug_root, NULL, &alan_fops);
+ if (!alan_dentry)
+ pr_err("Unable to register alan\n");
+
return 0;
}
static void usb_debugfs_cleanup(void)
{
+ debugfs_remove(alan_dentry);
debugfs_remove(usb_debug_devices);
debugfs_remove(usb_debug_root);
}
Please don't include an entire 400-line message in your reply if you're
only going to add a few new lines of text. Use some judicious trimming.
On Sat, 17 Sep 2011, Rocko Requin wrote:
> Here's a log of the latest kernel from git crashing with that patch
> applied (as well as Ted's patch), does it help any?
It does. It shows a new problem that's completely unrelated to the
earlier one.
> The gnome-terminal console cursor
Why were you using gnome-terminal? You should be running the tests at
a console VT, not under X at all. Ctrl-Alt-F2 or the equivalent...
> stopped flashing after the last
> 'detaching wakeup' message for a while (it *seemed* to have locked up
> at this point) but then it came back after what looks like 17 seconds
> or so from the log (apport reported something else crashing at this
> point) and then the oops happened and it crashed for good.
Here's another patch to address the new problem. You can apply it on
top of all the other patches.
Alan Stern
Index: usb-3.1/block/blk-core.c
===================================================================
--- usb-3.1.orig/block/blk-core.c
+++ usb-3.1/block/blk-core.c
@@ -367,8 +367,10 @@ void blk_cleanup_queue(struct request_qu
queue_flag_set_unlocked(QUEUE_FLAG_DEAD, q);
mutex_unlock(&q->sysfs_lock);
- if (q->elevator)
+ if (q->elevator) {
elevator_exit(q->elevator);
+ q->elevator = NULL;
+ }
blk_throtl_exit(q);
Index: usb-3.1/block/elevator.c
===================================================================
--- usb-3.1.orig/block/elevator.c
+++ usb-3.1/block/elevator.c
@@ -812,7 +812,7 @@ void elv_completed_request(struct reques
*/
if (blk_account_rq(rq)) {
q->in_flight[rq_is_sync(rq)]--;
- if ((rq->cmd_flags & REQ_SORTED) &&
+ if ((rq->cmd_flags & REQ_SORTED) && e &&
e->ops->elevator_completed_req_fn)
e->ops->elevator_completed_req_fn(q, rq);
}