Hi,
cgroup based writeback sometimes appears to manipulate inode->i_io_list
while holding the lock on the wrong bdi_writeback object.
Following is a crash I was able to produce by adding extra delays in
between list
pointer updates (repro patch attached).
[ 116.595958] ------------[ cut here ]------------
[ 116.596508] kernel BUG at lib/list_debug.c:74!
[ 116.597000] invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC
[ 116.597654] CPU: 3 PID: 940 Comm: kworker/u8:6 Not tainted 4.5.0-rc6+ #39
[ 116.598397] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS Bochs 01/01/2011
[ 116.599290] Workqueue: writeback wb_workfn (flush-8:16)
[ 116.599895] task: ffff88007c213700 ti: ffff88007c714000 task.ti:
ffff88007c714000
[ 116.600006] RIP: 0010:[<ffffffff805a3393>] [<ffffffff805a3393>]
__list_del_entry+0x53/0x60
[ 116.600006] RSP: 0018:ffff88007c717c50 EFLAGS: 00010206
[ 116.600006] RAX: dead000000000200 RBX: ffff88007c284818 RCX: ffff88007c717db0
[ 116.600006] RDX: ffff8800765bbdd8 RSI: ffff88007c717c90 RDI: ffff8800768595d8
[ 116.600006] RBP: ffff88007c717c60 R08: ffff8800768591d8 R09: 0000000000000000
[ 116.600006] R10: 0000000000000004 R11: 0000000000000001 R12: ffff88007c284818
[ 116.600006] R13: ffff88007c717c90 R14: ffff88007c284818 R15: ffff88007c717d28
[ 116.600006] FS: 0000000000000000(0000) GS:ffff88007f980000(0000)
knlGS:0000000000000000
[ 116.600006] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 116.600006] CR2: 00007fff5822ff40 CR3: 0000000000e0a000 CR4: 00000000000006e0
[ 116.600006] Stack:
[ 116.600006] ffff8800768595d8 ffff88007c46f000 ffff88007c717cc8
ffffffff8035e37f
[ 116.600006] ffff88007c284828 0000000000000082 000000000000026a
ffff88007c717cc0
[ 116.600006] ffff8800768111d8 ffff880076a309d8 ffff88007c284800
ffff88007c284828
[ 116.600006] Call Trace:
[ 116.600006] [<ffffffff8035e37f>] move_expired_inodes+0x4f/0x180
[ 116.600006] [<ffffffff8035f0a1>] queue_io+0x61/0xb0
[ 116.600006] [<ffffffff80360813>] wb_writeback+0x1a3/0x1e0
[ 116.600006] [<ffffffff803609db>] wb_workfn+0x18b/0x280
[ 116.600006] [<ffffffff802795a8>] process_one_work+0x128/0x300
[ 116.600006] [<ffffffff802798a0>] worker_thread+0x120/0x480
[ 116.600006] [<ffffffff80279780>] ? process_one_work+0x300/0x300
[ 116.600006] [<ffffffff8027ea64>] kthread+0xc4/0xe0
[ 116.600006] [<ffffffff8032d1e8>] ? kfree+0xc8/0x100
[ 116.600006] [<ffffffff8027e9a0>] ? __kthread_parkme+0x70/0x70
[ 116.600006] [<ffffffff80970bdf>] ret_from_fork+0x3f/0x70
[ 116.600006] [<ffffffff8027e9a0>] ? __kthread_parkme+0x70/0x70
[ 116.600006] Code: 39 c3 74 29 48 3b 3b 75 22 49 3b 7c 24 08 75 19
49 89 5c 24 08 e8 fe fd ff ff 4c 89 23 e8 f6 fd ff ff 5b 41 5c 5d c3
0f 0b 0f 0b <0f> 0b 0f 0b 66 0f 1f 84 00 00 00 00 00 55 48 89 e5 53 48
89 fb
[ 116.600006] RIP [<ffffffff805a3393>] __list_del_entry+0x53/0x60
[ 116.600006] RSP <ffff88007c717c50>
[ 116.623099] ---[ end trace ff07596b6e4f4928 ]---
I see a few places where a wrong bdi_writeback object may be locked while
manipulating inode->i_io_list links.
The first one is in writeback_single_inode():
1353 spin_lock(&wb->list_lock);
1354 spin_lock(&inode->i_lock);
1355 /*
1356 * If inode is clean, remove it from writeback lists.
Otherwise don't
1357 * touch it. See comment above for explanation.
1358 */
1359 if (!(inode->i_state & I_DIRTY_ALL))
1360 inode_io_list_del_locked(inode, wb);
1361 spin_unlock(&wb->list_lock);
The locked wb is passed in as a parameter and equals to inode_to_bdi(inode)->wb.
But this may not actually match inode->i_wb.
The second one is in writeback_sb_inodes():
1499 __writeback_single_inode(inode, &wbc);
1500
1501 wbc_detach_inode(&wbc);
1502 work->nr_pages -= write_chunk - wbc.nr_to_write;
1503 wrote += write_chunk - wbc.nr_to_write;
1504
1505 if (need_resched()) {
1506 /*
1507 * We're trying to balance between
building up a nice
1508 * long list of IOs to improve our merge rate, and
1509 * getting those IOs out quickly for
anyone throttling
1510 * in balance_dirty_pages(). cond_resched() doesn't
1511 * unplug, so get our IOs out the door before we
1512 * give up the CPU.
1513 */
1514 blk_flush_plug(current);
1515 cond_resched();
1516 }
1517
1518
1519 spin_lock(&wb->list_lock);
1520 spin_lock(&inode->i_lock);
1521 if (!(inode->i_state & I_DIRTY_ALL))
1522 wrote++;
1523 requeue_inode(inode, wb, &wbc);
After wbc_detach_inode() is called, inode's i_wb could have changed. So locking
the original wb seems wrong. The same issue exists in writeback_single_inode().
Repro patch:
---
fs/fs-writeback.c | 4 ++++
include/linux/list.h | 6 ++++++
lib/list_debug.c | 29 ++++++++++++++++-------------
repro.sh | 31 +++++++++++++++++++++++++++++++
4 files changed, 57 insertions(+), 13 deletions(-)
create mode 100755 repro.sh
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 1f76d89..bd1bd75 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1301,6 +1301,8 @@ __writeback_single_inode(struct inode *inode,
struct writeback_control *wbc)
return ret;
}
+atomic_t slow_down = ATOMIC_INIT(0);
+
/*
* Write out an inode's dirty pages. Either the caller has an active reference
* on the inode or the inode has I_WILL_FREE set.
@@ -1315,6 +1317,7 @@ writeback_single_inode(struct inode *inode,
struct bdi_writeback *wb,
{
int ret = 0;
+ atomic_inc(&slow_down);
spin_lock(&inode->i_lock);
if (!atomic_read(&inode->i_count))
WARN_ON(!(inode->i_state & (I_WILL_FREE|I_FREEING)));
@@ -1362,6 +1365,7 @@ writeback_single_inode(struct inode *inode,
struct bdi_writeback *wb,
inode_sync_complete(inode);
out:
spin_unlock(&inode->i_lock);
+ atomic_dec(&slow_down);
return ret;
}
diff --git a/include/linux/list.h b/include/linux/list.h
index 30cf420..e3b032b 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -6,6 +6,8 @@
#include <linux/poison.h>
#include <linux/const.h>
#include <linux/kernel.h>
+#include <asm/atomic.h>
+#include <asm/processor.h>
/*
* Simple doubly linked list implementation.
@@ -77,6 +79,8 @@ static inline void list_add_tail(struct list_head
*new, struct list_head *head)
__list_add(new, head->prev, head);
}
+void do_delay(void);
+
/*
* Delete a list entry by making the prev/next entries
* point to each other.
@@ -87,7 +91,9 @@ static inline void list_add_tail(struct list_head
*new, struct list_head *head)
static inline void __list_del(struct list_head * prev, struct list_head * next)
{
next->prev = prev;
+ do_delay();
WRITE_ONCE(prev->next, next);
+ do_delay();
}
/**
diff --git a/lib/list_debug.c b/lib/list_debug.c
index 3345a08..7195c4f 100644
--- a/lib/list_debug.c
+++ b/lib/list_debug.c
@@ -19,6 +19,14 @@ void list_force_poison(struct list_head *entry)
entry->prev = &force_poison;
}
+extern atomic_t slow_down;
+
+void do_delay() {
+ int i;
+ for (i=0; atomic_read(&slow_down) && i<100000; i++)
+ cpu_relax();
+}
+
/*
* Insert a new entry between two known consecutive entries.
*
@@ -44,9 +52,13 @@ void __list_add(struct list_head *new,
"list_add double add: new=%p, prev=%p, next=%p.\n",
new, prev, next);
next->prev = new;
+ do_delay();
new->next = next;
+ do_delay();
new->prev = prev;
+ do_delay();
WRITE_ONCE(prev->next, new);
+ do_delay();
}
EXPORT_SYMBOL(__list_add);
@@ -57,19 +69,10 @@ void __list_del_entry(struct list_head *entry)
prev = entry->prev;
next = entry->next;
- if (WARN(next == LIST_POISON1,
- "list_del corruption, %p->next is LIST_POISON1 (%p)\n",
- entry, LIST_POISON1) ||
- WARN(prev == LIST_POISON2,
- "list_del corruption, %p->prev is LIST_POISON2 (%p)\n",
- entry, LIST_POISON2) ||
- WARN(prev->next != entry,
- "list_del corruption. prev->next should be %p, "
- "but was %p\n", entry, prev->next) ||
- WARN(next->prev != entry,
- "list_del corruption. next->prev should be %p, "
- "but was %p\n", entry, next->prev))
- return;
+ BUG_ON(next == LIST_POISON1);
+ BUG_ON(prev == LIST_POISON2);
+ BUG_ON(prev->next != entry);
+ BUG_ON(next->prev != entry);
__list_del(prev, next);
}
diff --git a/repro.sh b/repro.sh
new file mode 100755
index 0000000..05de4c7
--- /dev/null
+++ b/repro.sh
@@ -0,0 +1,31 @@
+#!/bin/bash
+
+CGROUP_ROOT=/mnt-cgroup2
+
+mkdir -p $CGROUP_ROOT
+
+if ! mount | grep -qw cgroup2; then
+ mount -t cgroup2 none $CGROUP_ROOT
+fi
+
+mkdir -p $CGROUP_ROOT/mem1
+
+echo '+memory' > $CGROUP_ROOT/cgroup.subtree_control
+
+echo $$ > $CGROUP_ROOT/mem1/cgroup.procs
+
+if dumpe2fs -h /dev/sdb |grep -q 'Journal size'; then
+ echo 'This repro requires ext4 without journaling'
+ exit 1
+fi
+
+if ! mount | grep -qw /dev/sdb; then
+ mount /dev/sdb /mnt/sdb
+fi
+
+(for i in {1..10000}; do dd if=/dev/urandom of=/mnt/sdb/fsync1
bs=4096 count=1 conv=notrunc,fsync &> /dev/null; done)&
+
+(for i in {1..10000};do dd if=/dev/urandom of=/mnt/sdb/mark_dirty$i
bs=4096 count=1 conv=notrunc &> /dev/null;done)&
+
+wait
+
--
2.7.0.rc3.207.g0ac5344
--
To unsubscribe from this list: send the line "unsubscribe cgroups" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
