!PageLocked BUG_ON hit in clear_page_dirty_for_io

Not sure if I've already reported this one, but I've been seeing this a lot this last couple days.

kernel BUG at mm/page-writeback.c:2654!

Not sure if I've already reported this one, but I've been seeing this a lot this last couple days.

kernel BUG at mm/page-writeback.c:2654!
invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN

CPU: 1 PID: 2566 Comm: trinity-c1 Tainted: G        W       4.4.0-rc4-think+ #14
task: ffff880462811b80 ti: ffff8800cd808000 task.ti: ffff8800cd808000
RIP: 0010:[<ffffffff8826ff40>]  [<ffffffff8826ff40>] clear_page_dirty_for_io+0x180/0x1d0

Quoted 3 lines · Dave Jones

Not sure if I've already reported this one, but I've been seeing this a lot this last couple days.

Quoted 5 lines

kernel BUG at mm/page-writeback.c:2654!
invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN

CPU: 1 PID: 2566 Comm: trinity-c1 Tainted: G        W       4.4.0-rc4-think+ #14
task: ffff880462811b80 ti: ffff8800cd808000 task.ti: ffff8800cd808000
RIP: 0010:[<ffffffff8826ff40>]  [<ffffffff8826ff40>] clear_page_dirty_for_io+0x180/0x1d0

Huh, are you able to reproduce at will? From this code path it should mean somebody else is unlocking a page they don't own.

Huh, are you able to reproduce at will? From this code path it should mean somebody else is unlocking a page they don't own.

Quoted 10 lines · Chris Mason

Quoted 8 lines · Dave Jones

Not sure if I've already reported this one, but I've been seeing this a lot this last couple days.

kernel BUG at mm/page-writeback.c:2654!
invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN

CPU: 1 PID: 2566 Comm: trinity-c1 Tainted: G        W       4.4.0-rc4-think+ #14
task: ffff880462811b80 ti: ffff8800cd808000 task.ti: ffff8800cd808000
RIP: 0010:[<ffffffff8826ff40>]  [<ffffffff8826ff40>] clear_page_dirty_for_io+0x180/0x1d0

Quoted 2 lines

Huh, are you able to reproduce at will? From this code path it should mean somebody else is unlocking a page they don't own.

pretty easily yeah. I hit it maybe a couple dozen times yesterday. So if you've got some idea of printk's to spray anywhere I can give that a shot.

Quoted 14 lines · Dave Jones

Quoted 12 lines · Chris Mason

Quoted 8 lines · Dave Jones

Not sure if I've already reported this one, but I've been seeing this a lot this last couple days.

kernel BUG at mm/page-writeback.c:2654!
invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN

CPU: 1 PID: 2566 Comm: trinity-c1 Tainted: G        W       4.4.0-rc4-think+ #14
task: ffff880462811b80 ti: ffff8800cd808000 task.ti: ffff8800cd808000
RIP: 0010:[<ffffffff8826ff40>]  [<ffffffff8826ff40>] clear_page_dirty_for_io+0x180/0x1d0

Huh, are you able to reproduce at will? From this code path it should mean somebody else is unlocking a page they don't own.

Quoted 3 lines

pretty easily yeah. I hit it maybe a couple dozen times yesterday. So if you've got some idea of printk's to spray anywhere I can give that a shot.

I'd rather try to trigger it here. Going to have to add some way to record which stack trace last unlocked and/or freed the page.

-chris
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 15 lines · Chris Mason

Quoted 14 lines · Dave Jones

Quoted 12 lines · Chris Mason

Quoted 8 lines · Dave Jones

Not sure if I've already reported this one, but I've been seeing this a lot this last couple days.

kernel BUG at mm/page-writeback.c:2654!
invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN

CPU: 1 PID: 2566 Comm: trinity-c1 Tainted: G        W       4.4.0-rc4-think+ #14
task: ffff880462811b80 ti: ffff8800cd808000 task.ti: ffff8800cd808000
RIP: 0010:[<ffffffff8826ff40>]  [<ffffffff8826ff40>] clear_page_dirty_for_io+0x180/0x1d0

Huh, are you able to reproduce at will? From this code path it should mean somebody else is unlocking a page they don't own.

Quoted 6 lines

Quoted 3 lines

pretty easily yeah. I hit it maybe a couple dozen times yesterday. So if you've got some idea of printk's to spray anywhere I can give that a shot.

I'd rather try to trigger it here. Going to have to add some way to record which stack trace last unlocked and/or freed the page.

Looks like a bisect with 4.3 might target the commit.

Thanks,

-liubo

Quoted 6 lines

-chris
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 14 lines · Dave Jones

Quoted 12 lines · Chris Mason

Quoted 8 lines · Dave Jones

Not sure if I've already reported this one, but I've been seeing this a lot this last couple days.

kernel BUG at mm/page-writeback.c:2654!
invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN

CPU: 1 PID: 2566 Comm: trinity-c1 Tainted: G        W       4.4.0-rc4-think+ #14
task: ffff880462811b80 ti: ffff8800cd808000 task.ti: ffff8800cd808000
RIP: 0010:[<ffffffff8826ff40>]  [<ffffffff8826ff40>] clear_page_dirty_for_io+0x180/0x1d0

Huh, are you able to reproduce at will? From this code path it should mean somebody else is unlocking a page they don't own.

Quoted 3 lines

pretty easily yeah. I hit it maybe a couple dozen times yesterday. So if you've got some idea of printk's to spray anywhere I can give that a shot.

I'd rather try to trigger it here. Going to have to add some way to record which stack trace last unlocked and/or freed the page.

Quoted 14 lines · Dave Jones

Quoted 12 lines · Chris Mason

Quoted 8 lines · Dave Jones

Not sure if I've already reported this one, but I've been seeing this a lot this last couple days.

kernel BUG at mm/page-writeback.c:2654!
invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN

CPU: 1 PID: 2566 Comm: trinity-c1 Tainted: G        W       4.4.0-rc4-think+ #14
task: ffff880462811b80 ti: ffff8800cd808000 task.ti: ffff8800cd808000
RIP: 0010:[<ffffffff8826ff40>]  [<ffffffff8826ff40>] clear_page_dirty_for_io+0x180/0x1d0

Huh, are you able to reproduce at will? From this code path it should mean somebody else is unlocking a page they don't own.

Quoted 3 lines

pretty easily yeah. I hit it maybe a couple dozen times yesterday. So if you've got some idea of printk's to spray anywhere I can give that a shot.

I'd rather try to trigger it here. Going to have to add some way to record which stack trace last unlocked and/or freed the page.

Quoted 15 lines · Chris Mason

Quoted 14 lines · Dave Jones

Quoted 12 lines · Chris Mason

Quoted 8 lines · Dave Jones

Not sure if I've already reported this one, but I've been seeing this a lot this last couple days.

kernel BUG at mm/page-writeback.c:2654!
invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN

CPU: 1 PID: 2566 Comm: trinity-c1 Tainted: G        W       4.4.0-rc4-think+ #14
task: ffff880462811b80 ti: ffff8800cd808000 task.ti: ffff8800cd808000
RIP: 0010:[<ffffffff8826ff40>]  [<ffffffff8826ff40>] clear_page_dirty_for_io+0x180/0x1d0

Huh, are you able to reproduce at will? From this code path it should mean somebody else is unlocking a page they don't own.

Quoted 6 lines

Quoted 3 lines

pretty easily yeah. I hit it maybe a couple dozen times yesterday. So if you've got some idea of printk's to spray anywhere I can give that a shot.

I'd rather try to trigger it here. Going to have to add some way to record which stack trace last unlocked and/or freed the page.

I'm using..

trinity -q -l off -C8 -a64 -x fsync -x fdatasync -x syncfs -x sync --enable-fds=testfile,pseudo

interestingly, if I just use 'testfile' by itself, I can't reproduce it. (That means "create a bunch a few files in current dir and use their fds") the "pseudo" bit means "also use fds from /proc, /sys and /dev".

Not sure if I've already reported this one, but I've been seeing this a lot this last couple days.

kernel BUG at mm/page-writeback.c:2654!
invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN

From f0be89af049857bcc537a53fe2a2fae080e7a5bd Mon Sep 17 00:00:00 2001

Quoted 5 lines · Dave Jones

Not sure if I've already reported this one, but I've been seeing this a lot this last couple days.

kernel BUG at mm/page-writeback.c:2654!
invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN

We ended up discussing this in more detail on lkml, but I'll summarize here.

There were two problems. First lock_page() might not actually lock the page in v4.4-rc4, it can bail out if a signal is pending. This got fixed just before v4.4-rc5, so if you were on rc4, upgrade asap.

Second, prepare_pages had a bug for single page writes:

From f0be89af049857bcc537a53fe2a2fae080e7a5bd Mon Sep 17 00:00:00 2001
From: Chris Mason <[email protected]>
Date: Mon, 14 Dec 2015 15:40:44 -0800
Subject: [PATCH] Btrfs: check prepare_uptodate_page() error code earlier

prepare_pages() may end up calling prepare_uptodate_page() twice if our write only spans a single page. But if the first call returns an error, our page will be unlocked and its not safe to call it again.

This bug goes all the way back to 2011, and it's not something commonly hit.

While we're here, add a more explicit check for the page being truncated away. The bare lock_page() alone is protected only by good thoughts and i_mutex, which we're sure to regret eventually.

Reported-by Dave Jones <[email protected]>

Signed-off-by Chris Mason <[email protected]>

---
 fs/btrfs/file.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

fs/btrfs/file.c +14 -4

index 72e7346..0f09526 100644
@@ -1291,7 +1291,8 @@ out:
  * on error we return an unlocked page and the error value
  * on success we return a locked page and 0
  */
-static int prepare_uptodate_page(struct page *page, u64 pos,
+static int prepare_uptodate_page(struct inode *inode,
+				 struct page *page, u64 pos,
 				 bool force_uptodate)
 {
 	int ret = 0;

@@ -1306,6 +1307,10 @@ static int prepare_uptodate_page(struct page *page, u64 pos,
 			unlock_page(page);
 			return -EIO;
 		}
+		if (page->mapping != inode->i_mapping) {
+			unlock_page(page);
+			return -EAGAIN;
+		}
 	}
 	return 0;
 }

@@ -1324,6 +1329,7 @@ static noinline int prepare_pages(struct inode *inode, struct page **pages,
 	int faili;
 
 	for (i = 0; i < num_pages; i++) {
+again:
 		pages[i] = find_or_create_page(inode->i_mapping, index + i,
 					       mask | __GFP_WRITE);
 		if (!pages[i]) {

@@ -1333,13 +1339,17 @@ static noinline int prepare_pages(struct inode *inode, struct page **pages,
 		}
 
 		if (i == 0)
-			err = prepare_uptodate_page(pages[i], pos,
+			err = prepare_uptodate_page(inode, pages[i], pos,
 						    force_uptodate);
-		if (i == num_pages - 1)
-			err = prepare_uptodate_page(pages[i],
+		if (!err && i == num_pages - 1)
+			err = prepare_uptodate_page(inode, pages[i],
 						    pos + write_bytes, false);
 		if (err) {
 			page_cache_release(pages[i]);
+			if (err == -EAGAIN) {
+				err = 0;
+				goto again;
+			}
 			faili = i - 1;
 			goto fail;
 		}

Signature · 6 lines

2.4.6

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · Dave Jones

Not sure if I've already reported this one, but I've been seeing this a lot this last couple days.

kernel BUG at mm/page-writeback.c:2654!
invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN

We ended up discussing this in more detail on lkml, but I'll summarize here.

There were two problems. First lock_page() might not actually lock the page in v4.4-rc4, it can bail out if a signal is pending. This got fixed just before v4.4-rc5, so if you were on rc4, upgrade asap.

Second, prepare_pages had a bug for single page writes:

From f0be89af049857bcc537a53fe2a2fae080e7a5bd Mon Sep 17 00:00:00 2001
From: Chris Mason <[email protected]>
Date: Mon, 14 Dec 2015 15:40:44 -0800
Subject: [PATCH] Btrfs: check prepare_uptodate_page() error code earlier

prepare_pages() may end up calling prepare_uptodate_page() twice if our write only spans a single page. But if the first call returns an error, our page will be unlocked and its not safe to call it again.

This bug goes all the way back to 2011, and it's not something commonly hit.

While we're here, add a more explicit check for the page being truncated away. The bare lock_page() alone is protected only by good thoughts and i_mutex, which we're sure to regret eventually.

Reported-by Dave Jones <[email protected]>

Signed-off-by Chris Mason <[email protected]>

---
 fs/btrfs/file.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

fs/btrfs/file.c +14 -5

index 72e7346..0f09526 100644
@@ -1291,7 +1291,8 @@ out:
  * on error we return an unlocked page and the error value
  * on success we return a locked page and 0
  */
-static int prepare_uptodate_page(struct page *page, u64 pos,
+static int prepare_uptodate_page(struct inode *inode,
+                                struct page *page, u64 pos,
                                 bool force_uptodate)
 {
        int ret = 0;

@@ -1306,6 +1307,10 @@ static int prepare_uptodate_page(struct page *page, u64 pos,
                        unlock_page(page);
                        return -EIO;
                }
+               if (page->mapping != inode->i_mapping) {
+                       unlock_page(page);
+                       return -EAGAIN;
+               }
        }
        return 0;
 }

@@ -1324,6 +1329,7 @@ static noinline int prepare_pages(struct inode *inode, struct page **pages,
        int faili;
 
        for (i = 0; i < num_pages; i++) {
+again:
                pages[i] = find_or_create_page(inode->i_mapping, index + i,
                                               mask | __GFP_WRITE);
                if (!pages[i]) {

@@ -1333,13 +1339,17 @@ static noinline int prepare_pages(struct inode *inode, struct page **pages,
                }
 
                if (i == 0)
-                       err = prepare_uptodate_page(pages[i], pos,
+                       err = prepare_uptodate_page(inode, pages[i], pos,
                                                    force_uptodate);
-               if (i == num_pages - 1)
-                       err = prepare_uptodate_page(pages[i],
+               if (!err && i == num_pages - 1)
+                       err = prepare_uptodate_page(inode, pages[i],
                                                    pos + write_bytes, false);
                if (err) {
                        page_cache_release(pages[i]);
+                       if (err == -EAGAIN) {
+                               err = 0;
+                               goto again;
+                       }
                        faili = i - 1;
                        goto fail;
                }
--

2.4.6