I am sorry for forggeting to write the reproducing steps.
I injects the ftrace's logging code and the fault to the Linux kerenl
v4.10-rc7.
The diff is too long for pasting here.
So, I put the repository of the kernel here.
https://github.com/tk1012/linux-for-reproduce-btrfs-failure.git
And the steps to reproduce are:
$ git clone
https://github.com/tk1012/linux-for-reproduce-btrfs-failure.git
$ make menuconfig # enable these configs
- CONFIG_FAILSLAB
- CONFIG_FAULT_INJECTION_DEBUG_FS
- CONFIG_FTRACE
$ make && make install
Then, I run the below reproducer.
And, you can see the ftrace log like:
$ cat /sys/kernel/debug/tracing/trace > ~/ftrace_log
FYI, you can use the kernel config here.
https://github.com/tk1012/linux-for-reproduce-btrfs-failure/blob/master/minconfig
----------
#!/bin/bash
set -xe
echo 128000 > /sys/kernel/debug/tracing/buffer_size_kb
echo N > /sys/kernel/debug/failk9/task-filter
echo N > /sys/kernel/debug/failk9/ignore-gfp-wait
echo 100 > /sys/kernel/debug/failk9/probability
echo 1 > /sys/kernel/debug/failk9/interval
directory=/mnt/btrfs
mkdir -p ${directory}
truncate -s 100G btrfs.img
mkfs.btrfs btrfs.img
mount btrfs.img ${directory}
for i in `seq 1 1 100`; do
echo 1 > /sys/kernel/debug/failk9/times
bash -c "echo 1 > /proc/self/make-it-fail && dd if=/dev/zero
of=${directory}/text_txt bs=1M count=10 2> /dev/null"
sync # never return
done
umount ${directory}
----------
Sincerely,
-takafumi
On 2017/02/06 12:35, Liu Bo wrote:
On Sat, Feb 04, 2017 at 09:42:17PM +0900, takafumi-sslab wrote:
(But it could be changed after subpagesize block patchset, and there is
more work rather than just adding a end_page_writeback, e.g. writepage
endio also needs to be updated).
Ok... the discussion become complicated.
So, let me make this clear.
you think
a) this is a bug;
we need to clear the writeback bit in the error handling if the bit remains.
b) however, the way of fixing this bug has some concerns. ( and now we
discuss the best solution )
Is my understanding correct?
Sorry for making you confused even more, to clarify it, I don't think
the bug could exist in the current btrfs because blocksize is equal to
PAGE_SIZE so that @nr in __extent_writepage could only be 0 or 1.
a) __extent_writepage has handled the case when nr == 0.
b) when nr == 1, the page is marked with writeback bit and added into a
bio, thus we have bio_end to deal with page bits.
So I don't think the patch is necessary for now.
But as I said, the fact (nr == 0 or 1) would be changed if the
subpagesize blocksize is supported.
Thanks,
-liubo
Sincerely,
-takafumi
Thanks,
-liubo
Sincerely,
On 2017/01/31 5:09, Liu Bo wrote:
On Fri, Jan 13, 2017 at 03:12:31PM +0900, takafumi-sslab wrote:
Thanks for your replying.
I understand this bug is more complicated than I expected.
I classify error cases under submit_extent_page() below
A: ENOMEM error at btrfs_bio_alloc() in submit_extent_page()
I first assumed this case and sent the mail.
When bio_ret is NULL, submit_extent_page() calls btrfs_bio_alloc().
Then, btrfs_bio_alloc() may fail and submit_extent_page() returns -ENOMEM.
In this case, bio_endio() is not called and the page's writeback bit
remains.
So, there is a need to call end_page_writeback() in the error handling.
B: errors under submit_one_bio() of submit_extent_page()
Errors that occur under submit_one_bio() handles at bio_endio(), and
bio_endio() would call end_page_writeback().
Therefore, as you mentioned in the last mail, simply adding
end_page_writeback() like my last email and commit 55e3bd2e0c2e1 can
conflict in the case of B.
To avoid such conflict, one easy solution is adding PageWriteback() check
too.
How do you think of this solution?
(sorry for the late reply.)
I think its caller, "__extent_writepage", has covered the above case
by setting page writeback again.
Thanks,
-liubo
Sincerely,
On 2016/12/22 15:20, Liu Bo wrote:
On Fri, Dec 16, 2016 at 03:41:50PM +0900, Takafumi Kubota wrote:
This is actually inspired by Filipe's patch(55e3bd2e0c2e1).
When submit_extent_page() in __extent_writepage_io() fails,
Btrfs misses clearing a writeback bit of the failed page.
This causes the false under-writeback page.
Then, another sync task hangs in filemap_fdatawait_range(),
because it waits the false under-writeback page.
CPU0 CPU1
__extent_writepage_io()
ret = submit_extent_page() // fail
if (ret)
SetPageError(page)
// miss clearing the writeback bit
sync()
...
filemap_fdatawait_range()
wait_on_page_writeback(page);
// wait the false under-writeback page
Signed-off-by: Takafumi Kubota <takafumi.kubota1...@sslab.ics.keio.ac.jp>
---
fs/btrfs/extent_io.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 1e67723..ef9793b 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3443,8 +3443,10 @@ static noinline_for_stack int
__extent_writepage_io(struct inode *inode,
bdev->bio, max_nr,
end_bio_extent_writepage,
0, 0, 0, false);
- if (ret)
+ if (ret) {
SetPageError(page);
+ end_page_writeback(page);
+ }
OK...this could be complex as we don't know which part in
submit_extent_page gets the error, if the page has been added into bio
and bio_end would call end_page_writepage(page) as well, so whichever
comes later, the BUG() in end_page_writeback() would complain.
Looks like commit 55e3bd2e0c2e1 also has the same problem although I
gave it my reviewed-by.
Thanks,
-liubo
cur = cur + iosize;
pg_offset += iosize;
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Keio University
System Software Laboratory
Takafumi Kubota
takafumi.kubota1...@sslab.ics.keio.jp
--
Keio University
System Software Laboratory
Takafumi Kubota
takafumi.kubota1...@sslab.ics.keio.jp
--
Keio University
System Software Laboratory
Takafumi Kubota
takafumi.kubota1...@sslab.ics.keio.jp
--
Keio University
System Software Laboratory
Takafumi Kubota
takafumi.kubota1...@sslab.ics.keio.jp
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
