Re: EXT4 regression caused 4eec7
On Sun 12-05-13 13:01:11, Dmitry Monakhov wrote: > On Sat, 11 May 2013 19:05:59 -0400, Theodore Ts'o wrote: > > On Sat, May 11, 2013 at 03:00:53PM +0400, Dmitry Monakhov wrote: > > > I've bisected ext4 related issue. It is appeared that it is pure ext4 > > > specific. Regression caused by following commit > > > commit 4eec708d263f0ee10861d69251708a225b64cac7 > > > Author: Jan Kara > > > Date: Thu Apr 11 23:56:53 2013 -0400 > > > ext4: use io_end for multiple bios > > > > Hmm... the next question is why did I see this in my testing. Did you > > find this on ia64, or x86? > No simple x86_64. > > Also what about the slab corruption which > > you saw when running XFS; was that unrelated? > My theory about mysterious corruption in mm layer which broke everything > was wrong. We have to absolutely independent regressions in different > filesystems: > * Slub corruption on XFS > - testcase: xfstests/generic/007 > - bad commit: 666d64 > - LINK: https://lkml.org/lkml/2013/5/11/154 > > * Slub corruption on EXT4 > - testcase: xfstests/generic/299 > - bad commit: 4eec70 > - link: https://lkml.org/lkml/2013/5/11/37 > - fix: https://lkml.org/lkml/2013/5/11/142 > > In fact '4eec70' are vexing because I have reviewed and tested this patch > before it was marked as Review-by, but missed the bug. This is because > xfstests was executed manually logs was full of warnings but tainted flag > was not checked at the end. To prevent this thing in future I'll always > use my local autotest(http://autotest.github.io/) farm next time. OK, so I finally nailed this down. DIO code has that peculiarity that it doesn't call ->end_io callback when no IO was actually submitted. As a bonus the generic code does a cleanup of generic stuff that is otherwise left to ->end_io callback. Since I need to do io_end cleanup in ->end_io callback I have to compensate for that in ext4_ext_direct_IO(). Sadly I've got the condition wrong and also forgot that generic code already decremented i_dio_count in the error failure case so cleanup sometimes happened twice. I'll add fixed version of the patch back in my series (since the patch got reverted upstream). Honza -- Jan Kara SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: EXT4 regression caused 4eec7
On 5/14/13 2:11 AM, Dmitry Monakhov wrote: > On Mon, 13 May 2013 12:09:22 -0500, Eric Sandeen wrote: >> On 5/13/13 12:01 PM, Jan Kara wrote: >>> On Mon 13-05-13 11:34:12, Eric Sandeen wrote: On 5/12/13 4:01 AM, Dmitry Monakhov wrote: > In fact '4eec70' are vexing because I have reviewed and tested this patch > before > it was marked as Review-by, but missed the bug. This is because xfstests > was executed manually logs was full of warnings but tainted flag was not > checked at the end. Can you elaborate on this? What was logged, and is it something we could try to pick up post-test in xfstests? >>> Generally I think it might be useful if xfstests would fail / warn if >>> kernel became tainted during the test (e.g. due to WARN_ON or oops, or >>> something like that). It should be even relatively easy to implement >>> (just compare /proc/sys/kernel/tainted before and after each test). >>> >>> Honza >>> >> >> Ah, right. That should be easy, I'll see if I can cook that up. > Also we can use abrt's kernel-oops handler to collect messages. I sent a pretty simple patch to just check the sysctl to the xfs list yesterday. -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: EXT4 regression caused 4eec7
On Mon, 13 May 2013 12:09:22 -0500, Eric Sandeen wrote: > On 5/13/13 12:01 PM, Jan Kara wrote: > > On Mon 13-05-13 11:34:12, Eric Sandeen wrote: > >> On 5/12/13 4:01 AM, Dmitry Monakhov wrote: > >>> In fact '4eec70' are vexing because I have reviewed and tested this patch > >>> before > >>> it was marked as Review-by, but missed the bug. This is because xfstests > >>> was executed manually logs was full of warnings but tainted flag was not > >>> checked at the end. > >> > >> Can you elaborate on this? What was logged, and is it something we could > >> try to pick up post-test in xfstests? > > Generally I think it might be useful if xfstests would fail / warn if > > kernel became tainted during the test (e.g. due to WARN_ON or oops, or > > something like that). It should be even relatively easy to implement > > (just compare /proc/sys/kernel/tainted before and after each test). > > > > Honza > > > > Ah, right. That should be easy, I'll see if I can cook that up. Also we can use abrt's kernel-oops handler to collect messages. > > Thanks, > -Eric > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: EXT4 regression caused 4eec7
On Mon, May 13, 2013 at 05:59:25PM +0400, Dmitry Monakhov wrote: > > This is how it's failing for me > Because you ask questions, but do not read answers :) > http://marc.info/?l=linux-ext4&m=136580060822252&w=2 > http://marc.info/?l=linux-ext4&m=136610044500931&w=2 Sorry, I thought I was running with the updated fio, but apparently I was using an old xfstests.tar.gz tarball on my KVM image. Mea culpa - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: EXT4 regression caused 4eec7
On 5/13/13 12:01 PM, Jan Kara wrote: > On Mon 13-05-13 11:34:12, Eric Sandeen wrote: >> On 5/12/13 4:01 AM, Dmitry Monakhov wrote: >>> In fact '4eec70' are vexing because I have reviewed and tested this patch >>> before >>> it was marked as Review-by, but missed the bug. This is because xfstests >>> was executed manually logs was full of warnings but tainted flag was not >>> checked at the end. >> >> Can you elaborate on this? What was logged, and is it something we could >> try to pick up post-test in xfstests? > Generally I think it might be useful if xfstests would fail / warn if > kernel became tainted during the test (e.g. due to WARN_ON or oops, or > something like that). It should be even relatively easy to implement > (just compare /proc/sys/kernel/tainted before and after each test). > > Honza > Ah, right. That should be easy, I'll see if I can cook that up. Thanks, -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: EXT4 regression caused 4eec7
On Mon 13-05-13 11:34:12, Eric Sandeen wrote: > On 5/12/13 4:01 AM, Dmitry Monakhov wrote: > > In fact '4eec70' are vexing because I have reviewed and tested this patch > > before > > it was marked as Review-by, but missed the bug. This is because xfstests > > was executed manually logs was full of warnings but tainted flag was not > > checked at the end. > > Can you elaborate on this? What was logged, and is it something we could > try to pick up post-test in xfstests? Generally I think it might be useful if xfstests would fail / warn if kernel became tainted during the test (e.g. due to WARN_ON or oops, or something like that). It should be even relatively easy to implement (just compare /proc/sys/kernel/tainted before and after each test). Honza -- Jan Kara SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: EXT4 regression caused 4eec7
On 5/12/13 4:01 AM, Dmitry Monakhov wrote: > In fact '4eec70' are vexing because I have reviewed and tested this patch > before > it was marked as Review-by, but missed the bug. This is because xfstests > was executed manually logs was full of warnings but tainted flag was not > checked at the end. Can you elaborate on this? What was logged, and is it something we could try to pick up post-test in xfstests? Thanks, -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Re: Re: EXT4 regression caused 4eec7
On Mon, May 13, 2013 at 05:17:27PM +0200, Jan Kara wrote: > On Mon 13-05-13 21:56:43, Zheng Liu wrote: > > On Mon, May 13, 2013 at 03:18:09PM +0200, Jan Kara wrote: > > > On Sun 12-05-13 13:04:59, EUNBONG SONG wrote: > > > > > > > > > > > > >> Since at this point it's safer to rollback the change and we can > > > > >> investigate more deeply how to fix it correctly for the next > > > > >> development cycle, this is the patch which I'm testing. > > > > > > > > >> - Ted > > > > > > > > > Hello, I've tested with your patch. But the same problem was > > > > > reproduced. > > > > > Currently, I'm trying to git bisect. If i done git bisect, i will let > > > > > you know. > > > > > > > > Hi, I've done git bisect. and panic at jbd2_journal_put_journal_head() > > > > is caused by > > > > ae4647fb7654676fc44a97e86eb35f9f06b99f66: "jbd2: reduce journal_head > > > > size." > > > > I write just code patch which revert > > > > ae4647fb7654676fc44a97e86eb35f9f06b99f66 because > > > > I don't know the root cause. > > > This is really strange. I've verified the code and all the places > > > modifying b_jlist or b_modified are holding bh_state lock so we should be > > > safe... > > > > Hi Jan, > > > > Could you please take a look at this mail [1]. I don't think we hold > > bh_state lock there. > > > > 1. http://www.spinics.net/lists/linux-ext4/msg38205.html > I'll also reply to that thread but: Yes, we don't hold bh_state lock when > reading b_jlist in that one case (that's why I wrote 'modify' and not just > 'access' in my previous email). But that's really harmless since we don't > do any complex operations with b_jlist (only get & set) so we either see an > old value or a new one. And that particular use is going away anyway later > in my series. I see. Thanks for your explanation. - Zheng -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Re: Re: EXT4 regression caused 4eec7
On Mon 13-05-13 21:56:43, Zheng Liu wrote: > On Mon, May 13, 2013 at 03:18:09PM +0200, Jan Kara wrote: > > On Sun 12-05-13 13:04:59, EUNBONG SONG wrote: > > > > > > > > > >> Since at this point it's safer to rollback the change and we can > > > >> investigate more deeply how to fix it correctly for the next > > > >> development cycle, this is the patch which I'm testing. > > > > > > >> - Ted > > > > > > > Hello, I've tested with your patch. But the same problem was reproduced. > > > > Currently, I'm trying to git bisect. If i done git bisect, i will let > > > > you know. > > > > > > Hi, I've done git bisect. and panic at jbd2_journal_put_journal_head() is > > > caused by > > > ae4647fb7654676fc44a97e86eb35f9f06b99f66: "jbd2: reduce journal_head > > > size." > > > I write just code patch which revert > > > ae4647fb7654676fc44a97e86eb35f9f06b99f66 because > > > I don't know the root cause. > > This is really strange. I've verified the code and all the places > > modifying b_jlist or b_modified are holding bh_state lock so we should be > > safe... > > Hi Jan, > > Could you please take a look at this mail [1]. I don't think we hold > bh_state lock there. > > 1. http://www.spinics.net/lists/linux-ext4/msg38205.html I'll also reply to that thread but: Yes, we don't hold bh_state lock when reading b_jlist in that one case (that's why I wrote 'modify' and not just 'access' in my previous email). But that's really harmless since we don't do any complex operations with b_jlist (only get & set) so we either see an old value or a new one. And that particular use is going away anyway later in my series. Honza -- Jan Kara SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: EXT4 regression caused 4eec7
On Mon, 13 May 2013 09:52:21 -0400, Theodore Ts'o wrote: > On Mon, May 13, 2013 at 05:47:05PM +0400, Dmitry Monakhov wrote: > > In fact generic/299 always succeed for me, but it produce warning > > WARNING: at fs/ext4/inode.c:3218 ext4_ext_direct_IO > > and complains from slab debug. But it was missed because i've missed > > this error in the logs and forget to check /proc/sys/kernel/tained. > > This is how it's failing for me Because you ask questions, but do not read answers :) http://marc.info/?l=linux-ext4&m=136580060822252&w=2 http://marc.info/?l=linux-ext4&m=136610044500931&w=2 > > generic/299 [23:06:27][ 109.243220] fio (3376) used greatest stack > depth: 5240 bytes left > [ 109.252757] fio (3380) used greatest stack depth: 5140 bytes left > [ 109.307227] fio (3374) used greatest stack depth: 4944 bytes left > [23:08:10] [failed, exit status 1] - output mismatch (see > /root/xfstests/results/generic/299.out.bad) > --- tests/generic/299.out 2013-04-05 21:41:17.0 -0400 > +++ /root/xfstests/results/generic/299.out.bad2013-05-11 > 23:08:10.835356876 -0400 > @@ -3,3 +3,6 @@ > Run fio with random aio-dio pattern > > Start fallocate/truncate loop > +./common/rc: line 2055: 3335 Segmentation fault "$@" >> > $seqres.full 2>&1 > +failed: '/root/xfstests/bin/fio /tmp/3135-299.fio' > +(see /root/xfstests/results/generic/299.full for details) > ... > (Run 'diff -u tests/generic/299.out > /root/xfstests/results/generic/299.out.bad' to see the entire diff) > > I haven't had a chance to investigate the core dump yet > > - Ted > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: EXT4 regression caused 4eec7
On Mon, May 13, 2013 at 05:47:05PM +0400, Dmitry Monakhov wrote: > In fact generic/299 always succeed for me, but it produce warning > WARNING: at fs/ext4/inode.c:3218 ext4_ext_direct_IO > and complains from slab debug. But it was missed because i've missed > this error in the logs and forget to check /proc/sys/kernel/tained. This is how it's failing for me generic/299 [23:06:27][ 109.243220] fio (3376) used greatest stack depth: 5240 bytes left [ 109.252757] fio (3380) used greatest stack depth: 5140 bytes left [ 109.307227] fio (3374) used greatest stack depth: 4944 bytes left [23:08:10] [failed, exit status 1] - output mismatch (see /root/xfstests/results/generic/299.out.bad) --- tests/generic/299.out 2013-04-05 21:41:17.0 -0400 +++ /root/xfstests/results/generic/299.out.bad 2013-05-11 23:08:10.835356876 -0400 @@ -3,3 +3,6 @@ Run fio with random aio-dio pattern Start fallocate/truncate loop +./common/rc: line 2055: 3335 Segmentation fault "$@" >> $seqres.full 2>&1 +failed: '/root/xfstests/bin/fio /tmp/3135-299.fio' +(see /root/xfstests/results/generic/299.full for details) ... (Run 'diff -u tests/generic/299.out /root/xfstests/results/generic/299.out.bad' to see the entire diff) I haven't had a chance to investigate the core dump yet - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: EXT4 regression caused 4eec7
On Mon, 13 May 2013 09:30:36 -0400, Theodore Ts'o wrote: > On Mon, May 13, 2013 at 03:18:09PM +0200, Jan Kara wrote: > > Grumble. In this case I think bitfields are not worth the trouble with gcc. > > It's a pitty we have to spend additional 8 bytes for every journal_head but > > we'll survive... I'll send Ted a partial revert and add a comment so that > > we won't repeat this mistake in future. > > Or just switch things to use explicit 32-bit boolean operations. > Sounds the safest way to go is to simply not trust bitfields to be > something gcc is competent to compile correctly, and just open code it > in standard C. (Large portions of ext4 and e2fsprogs do this > manually, for historical reasons, and it sounds like we have a good > reason to do it going forward.) > > Jan, Dmitry --- I still have in my tree a revert for commit 4eec708d2: > ext4: use io_end for multiple bios, since I belive Dmitry still > bisected a regression for xfstests 299. Dmitry, can you confirm that > you are definitely seeing a regression here? Yes, this patch provoke use-after-free which detected by slab sanity checks. > Jan, do you mind if we > try to figure out how to fix this during the next development cycle, > since it was part of your much longer, extensive patch series anyway? > > I've determined that the reason why I didn't see a problem was because > xfstests 299 was failing earlier on the baseline, and crashing my > regression tests. So I simply commented it out just so I could > complete the testing. It seems that xfstests 299 is problematic for > me, and I need to focus on how to make it pass successfully. (Dmitry, > when I revert the commit which you identified, xfstests 299 is *still* > failing for me) In fact generic/299 always succeed for me, but it produce warning WARNING: at fs/ext4/inode.c:3218 ext4_ext_direct_IO and complains from slab debug. But it was missed because i've missed this error in the logs and forget to check /proc/sys/kernel/tained. > > - Ted > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Re: Re: EXT4 regression caused 4eec7
On Mon, May 13, 2013 at 03:18:09PM +0200, Jan Kara wrote: > On Sun 12-05-13 13:04:59, EUNBONG SONG wrote: > > > > > > >> Since at this point it's safer to rollback the change and we can > > >> investigate more deeply how to fix it correctly for the next > > >> development cycle, this is the patch which I'm testing. > > > > >> - Ted > > > > > Hello, I've tested with your patch. But the same problem was reproduced. > > > Currently, I'm trying to git bisect. If i done git bisect, i will let you > > > know. > > > > Hi, I've done git bisect. and panic at jbd2_journal_put_journal_head() is > > caused by > > ae4647fb7654676fc44a97e86eb35f9f06b99f66: "jbd2: reduce journal_head size." > > I write just code patch which revert > > ae4647fb7654676fc44a97e86eb35f9f06b99f66 because > > I don't know the root cause. > This is really strange. I've verified the code and all the places > modifying b_jlist or b_modified are holding bh_state lock so we should be > safe... Hi Jan, Could you please take a look at this mail [1]. I don't think we hold bh_state lock there. 1. http://www.spinics.net/lists/linux-ext4/msg38205.html Thanks, - Zheng -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: EXT4 regression caused 4eec7
On Mon 13-05-13 09:30:36, Ted Tso wrote: > On Mon, May 13, 2013 at 03:18:09PM +0200, Jan Kara wrote: > > Grumble. In this case I think bitfields are not worth the trouble with gcc. > > It's a pitty we have to spend additional 8 bytes for every journal_head but > > we'll survive... I'll send Ted a partial revert and add a comment so that > > we won't repeat this mistake in future. > > Or just switch things to use explicit 32-bit boolean operations. > Sounds the safest way to go is to simply not trust bitfields to be > something gcc is competent to compile correctly, and just open code it > in standard C. (Large portions of ext4 and e2fsprogs do this > manually, for historical reasons, and it sounds like we have a good > reason to do it going forward.) Yeah, but in this case b_jlist testing / setting would require accessor functions which is slightly ugly. For now I just submitted a revert of the bitfield part and if someone feels like saving 8 bytes in struct journal_head is worth the hassle, then we can later go that route. > Jan, Dmitry --- I still have in my tree a revert for commit 4eec708d2: > ext4: use io_end for multiple bios, since I belive Dmitry still > bisected a regression for xfstests 299. Dmitry, can you confirm that > you are definitely seeing a regression here? Jan, do you mind if we > try to figure out how to fix this during the next development cycle, > since it was part of your much longer, extensive patch series anyway? Yeah, for now just send a revert to Linus. I'll look into that failure now but since I didn't hit the problem in my testing it may take a while. Honza -- Jan Kara SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: EXT4 regression caused 4eec7
On Mon, May 13, 2013 at 03:18:09PM +0200, Jan Kara wrote: > Grumble. In this case I think bitfields are not worth the trouble with gcc. > It's a pitty we have to spend additional 8 bytes for every journal_head but > we'll survive... I'll send Ted a partial revert and add a comment so that > we won't repeat this mistake in future. Or just switch things to use explicit 32-bit boolean operations. Sounds the safest way to go is to simply not trust bitfields to be something gcc is competent to compile correctly, and just open code it in standard C. (Large portions of ext4 and e2fsprogs do this manually, for historical reasons, and it sounds like we have a good reason to do it going forward.) Jan, Dmitry --- I still have in my tree a revert for commit 4eec708d2: ext4: use io_end for multiple bios, since I belive Dmitry still bisected a regression for xfstests 299. Dmitry, can you confirm that you are definitely seeing a regression here? Jan, do you mind if we try to figure out how to fix this during the next development cycle, since it was part of your much longer, extensive patch series anyway? I've determined that the reason why I didn't see a problem was because xfstests 299 was failing earlier on the baseline, and crashing my regression tests. So I simply commented it out just so I could complete the testing. It seems that xfstests 299 is problematic for me, and I need to focus on how to make it pass successfully. (Dmitry, when I revert the commit which you identified, xfstests 299 is *still* failing for me) - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Re: Re: EXT4 regression caused 4eec7
On Sun 12-05-13 13:04:59, EUNBONG SONG wrote:
>
>
> >> Since at this point it's safer to rollback the change and we can
> >> investigate more deeply how to fix it correctly for the next
> >> development cycle, this is the patch which I'm testing.
>
> >> - Ted
>
> > Hello, I've tested with your patch. But the same problem was reproduced.
> > Currently, I'm trying to git bisect. If i done git bisect, i will let you
> > know.
>
> Hi, I've done git bisect. and panic at jbd2_journal_put_journal_head() is
> caused by
> ae4647fb7654676fc44a97e86eb35f9f06b99f66: "jbd2: reduce journal_head size."
> I write just code patch which revert ae4647fb7654676fc44a97e86eb35f9f06b99f66
> because
> I don't know the root cause.
This is really strange. I've verified the code and all the places
modifying b_jlist or b_modified are holding bh_state lock so we should be
safe...
Hum, but I remember I was debugging similar problems with bit fields in
btrfs on ia64 as well (see http://lwn.net/Articles/478657/). So I think what
has happened is that your compiler compiled bitfield access as 64-bit and
updates to b_jcount and b_jlist / b_modified crashed into one another. I
didn't hit it because my compiler was not so "clever".
Grumble. In this case I think bitfields are not worth the trouble with gcc.
It's a pitty we have to spend additional 8 bytes for every journal_head but
we'll survive... I'll send Ted a partial revert and add a comment so that
we won't repeat this mistake in future.
Honza
> Signed-off-by: Eunbong Song
> ---
> include/linux/journal-head.h | 11 +--
> 1 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/journal-head.h b/include/linux/journal-head.h
> index 13a3da2..c18b46f 100644
> --- a/include/linux/journal-head.h
> +++ b/include/linux/journal-head.h
> @@ -31,14 +31,21 @@ struct journal_head {
> /*
>* Journalling list for this buffer [jbd_lock_bh_state()]
>*/
> - unsigned b_jlist:4;
> + unsigned b_jlist;
>
> /*
>* This flag signals the buffer has been modified by
>* the currently running transaction
>* [jbd_lock_bh_state()]
>*/
> - unsigned b_modified:1;
> + unsigned b_modified;
> +
> + /*
> + * This feild tracks the last transaction id in which this buffer
> + * has been cowed
> + * [jbd_lock_bh_state()]
> + */
> + tid_t b_cow_tid;
>
> /*
>* Copy of the buffer data frozen for writing to the log.
> --
> 1.7.0.4
>
>
> Thanks.
--
Jan Kara
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Re: Re: EXT4 regression caused 4eec7
On Sun, 12 May 2013 13:05:00 + (GMT), EUNBONG SONG
wrote:
>
>
> >> Since at this point it's safer to rollback the change and we can
> >> investigate more deeply how to fix it correctly for the next
> >> development cycle, this is the patch which I'm testing.
>
> >> - Ted
>
> > Hello, I've tested with your patch. But the same problem was reproduced.
> > Currently, I'm trying to git bisect. If i done git bisect, i will let you
> > know.
>
> Hi, I've done git bisect. and panic at jbd2_journal_put_journal_head() is
> caused by
> ae4647fb7654676fc44a97e86eb35f9f06b99f66: "jbd2: reduce journal_head size."
OK that explains both regressions ext4/jbd2 and
ext3/jbd(https://lkml.org/lkml/2013/5/10/434)
Probably some race because b_jlist accessed w/o lock some places.
BUT i still can not reproduce original issue :(
I've tried all types of disks near me (ssd,hdd,usb-dongle),
What disk type you use? Is it reproducible on x86?
Please post following info:
#fdisk -l $HDD
#hdparm -t $HDD
#df -h
#cat /proc/mounts
> I write just code patch which revert ae4647fb7654676fc44a97e86eb35f9f06b99f66
> because
> I don't know the root cause.
>
>
> Signed-off-by: Eunbong Song
> ---
> include/linux/journal-head.h | 11 +--
> 1 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/journal-head.h b/include/linux/journal-head.h
> index 13a3da2..c18b46f 100644
> --- a/include/linux/journal-head.h
> +++ b/include/linux/journal-head.h
> @@ -31,14 +31,21 @@ struct journal_head {
> /*
>* Journalling list for this buffer [jbd_lock_bh_state()]
>*/
> - unsigned b_jlist:4;
> + unsigned b_jlist;
>
> /*
>* This flag signals the buffer has been modified by
>* the currently running transaction
>* [jbd_lock_bh_state()]
>*/
> - unsigned b_modified:1;
> + unsigned b_modified;
> +
> + /*
> + * This feild tracks the last transaction id in which this buffer
> + * has been cowed
> + * [jbd_lock_bh_state()]
> + */
> + tid_t b_cow_tid;
>
> /*
>* Copy of the buffer data frozen for writing to the log.
> --
> 1.7.0.4
>
>
> Thanks.
> N㎠咽ry鉉bXФ푤v^)頻{.n+램{喩zX㎍●}꼿z&j:+v㉭喩zZ++zf＂h~i鎬zwⅱ?②&)刪f^j푹ym
>
@Aa뛴 0띠hi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Re: Re: EXT4 regression caused 4eec7
>> Since at this point it's safer to rollback the change and we can
>> investigate more deeply how to fix it correctly for the next
>> development cycle, this is the patch which I'm testing.
>> - Ted
> Hello, I've tested with your patch. But the same problem was reproduced.
> Currently, I'm trying to git bisect. If i done git bisect, i will let you
> know.
Hi, I've done git bisect. and panic at jbd2_journal_put_journal_head() is
caused by
ae4647fb7654676fc44a97e86eb35f9f06b99f66: "jbd2: reduce journal_head size."
I write just code patch which revert ae4647fb7654676fc44a97e86eb35f9f06b99f66
because
I don't know the root cause.
Signed-off-by: Eunbong Song
---
include/linux/journal-head.h | 11 +--
1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/include/linux/journal-head.h b/include/linux/journal-head.h
index 13a3da2..c18b46f 100644
--- a/include/linux/journal-head.h
+++ b/include/linux/journal-head.h
@@ -31,14 +31,21 @@ struct journal_head {
/*
* Journalling list for this buffer [jbd_lock_bh_state()]
*/
- unsigned b_jlist:4;
+ unsigned b_jlist;
/*
* This flag signals the buffer has been modified by
* the currently running transaction
* [jbd_lock_bh_state()]
*/
- unsigned b_modified:1;
+ unsigned b_modified;
+
+ /*
+* This feild tracks the last transaction id in which this buffer
+* has been cowed
+* [jbd_lock_bh_state()]
+*/
+ tid_t b_cow_tid;
/*
* Copy of the buffer data frozen for writing to the log.
--
1.7.0.4
Thanks.
N떑꿩�r툤y鉉싕b쾊Ф푤v�^�)頻{.n�+돴쪐{콗喩zX㎍썳變}찠꼿쟺�&j:+v돣�쳭喩zZ+�+zf＂톒쉱�~넮녬i鎬z� 췿ⅱ�?솳鈺�&�)刪f뷌^j푹y쬶끷@A첺뛴
0띠h��뭝
Re: Re: EXT4 regression caused 4eec7
> Since at this point it's safer to rollback the change and we can > investigate more deeply how to fix it correctly for the next > development cycle, this is the patch which I'm testing. > - Ted Hello, I've tested with your patch. But the same problem was reproduced. Currently, I'm trying to git bisect. If i done git bisect, i will let you know. Thanks.
Re: EXT4 regression caused 4eec7
On Sat, 11 May 2013 19:05:59 -0400, Theodore Ts'o wrote:
> On Sat, May 11, 2013 at 03:00:53PM +0400, Dmitry Monakhov wrote:
> > I've bisected ext4 related issue. It is appeared that it is pure ext4
> > specific. Regression caused by following commit
> > commit 4eec708d263f0ee10861d69251708a225b64cac7
> > Author: Jan Kara
> > Date: Thu Apr 11 23:56:53 2013 -0400
> > ext4: use io_end for multiple bios
>
> Hmm... the next question is why did I see this in my testing. Did you
> find this on ia64, or x86?
No simple x86_64.
> Also what about the slab corruption which
> you saw when running XFS; was that unrelated?
My theory about mysterious corruption in mm layer which broke everything
was wrong. We have to absolutely independent regressions in different
filesystems:
* Slub corruption on XFS
- testcase: xfstests/generic/007
- bad commit: 666d64
- LINK: https://lkml.org/lkml/2013/5/11/154
* Slub corruption on EXT4
- testcase: xfstests/generic/299
- bad commit: 4eec70
- link: https://lkml.org/lkml/2013/5/11/37
- fix: https://lkml.org/lkml/2013/5/11/142
In fact '4eec70' are vexing because I have reviewed and tested this patch before
it was marked as Review-by, but missed the bug. This is because xfstests
was executed manually logs was full of warnings but tainted flag was not
checked at the end. To prevent this thing in future I'll always use my
local autotest(http://autotest.github.io/) farm next time.
BTW here is my config with various debug options enabled.
config.xz
Description: application/xz
>
> Since at this point it's safer to rollback the change and we can
> investigate more deeply how to fix it correctly for the next
> development cycle, this is the patch which I'm testing.
>
>- Ted
>
> From dfd5057908a66b414d5d5fa2fc7ff7945e5a277c Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o
> Date: Sat, 11 May 2013 18:59:39 -0400
> Subject: [PATCH] Revert "ext4: use io_end for multiple bios"
>
> This reverts commit 4eec708d263f0ee10861d69251708a225b64cac7.
>
> Multiple users have reported crashes which is apparently caused by
> this commit. Thanks to Dmitry Monakhov for bisecting it.
>
> Signed-off-by: "Theodore Ts'o"
> Cc: Dmitry Monakhov
> Cc: Jan Kara
> ---
> fs/ext4/ext4.h| 8 +---
> fs/ext4/inode.c | 85 +-
> fs/ext4/page-io.c | 121
> --
> 3 files changed, 85 insertions(+), 129 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 0aabb34..5aae3d1 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -209,7 +209,6 @@ typedef struct ext4_io_end {
> ssize_t size; /* size of the extent */
> struct kiocb*iocb; /* iocb struct for AIO */
> int result; /* error value for AIO */
> - atomic_tcount; /* reference counter */
> } ext4_io_end_t;
>
> struct ext4_io_submit {
> @@ -2651,14 +2650,11 @@ extern int ext4_move_extents(struct file *o_filp,
> struct file *d_filp,
>
> /* page-io.c */
> extern int __init ext4_init_pageio(void);
> +extern void ext4_add_complete_io(ext4_io_end_t *io_end);
> extern void ext4_exit_pageio(void);
> extern void ext4_ioend_shutdown(struct inode *);
> +extern void ext4_free_io_end(ext4_io_end_t *io);
> extern ext4_io_end_t *ext4_init_io_end(struct inode *inode, gfp_t flags);
> -extern ext4_io_end_t *ext4_get_io_end(ext4_io_end_t *io_end);
> -extern int ext4_put_io_end(ext4_io_end_t *io_end);
> -extern void ext4_put_io_end_defer(ext4_io_end_t *io_end);
> -extern void ext4_io_submit_init(struct ext4_io_submit *io,
> - struct writeback_control *wbc);
> extern void ext4_end_io_work(struct work_struct *work);
> extern void ext4_io_submit(struct ext4_io_submit *io);
> extern int ext4_bio_write_page(struct ext4_io_submit *io,
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 793d44b..d666569 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1487,10 +1487,7 @@ static int mpage_da_submit_io(struct mpage_da_data
> *mpd,
> struct ext4_io_submit io_submit;
>
> BUG_ON(mpd->next_page <= mpd->first_page);
> - ext4_io_submit_init(&io_submit, mpd->wbc);
> - io_submit.io_end = ext4_init_io_end(inode, GFP_NOFS);
> - if (!io_submit.io_end)
> - return -ENOMEM;
> + memset(&io_submit, 0, sizeof(io_submit));
> /*
>* We need to start from the first_page to the next_page - 1
>* to make sure we also write the mapped dirty buffer_heads.
> @@ -1578,8 +1575,6 @@ static int mpage_da_submit_io(struct mpage_da_data *mpd,
> pagevec_release(&pvec);
> }
> ext4_io_submit(&io_submit);
> - /* Drop io_end reference we got from init */
> - ext4_put_io_end_defer(io_submit.io_end);
> return ret;
> }
>
> @@ -2238,16 +2233,9 @@ static int ext4_writepage(stru
Re: EXT4 regression caused 4eec7
On Sat, May 11, 2013 at 03:00:53PM +0400, Dmitry Monakhov wrote:
> I've bisected ext4 related issue. It is appeared that it is pure ext4
> specific. Regression caused by following commit
> commit 4eec708d263f0ee10861d69251708a225b64cac7
> Author: Jan Kara
> Date: Thu Apr 11 23:56:53 2013 -0400
> ext4: use io_end for multiple bios
Hmm... the next question is why did I see this in my testing. Did you
find this on ia64, or x86? Also what about the slab corruption which
you saw when running XFS; was that unrelated?
Since at this point it's safer to rollback the change and we can
investigate more deeply how to fix it correctly for the next
development cycle, this is the patch which I'm testing.
- Ted
>From dfd5057908a66b414d5d5fa2fc7ff7945e5a277c Mon Sep 17 00:00:00 2001
From: Theodore Ts'o
Date: Sat, 11 May 2013 18:59:39 -0400
Subject: [PATCH] Revert "ext4: use io_end for multiple bios"
This reverts commit 4eec708d263f0ee10861d69251708a225b64cac7.
Multiple users have reported crashes which is apparently caused by
this commit. Thanks to Dmitry Monakhov for bisecting it.
Signed-off-by: "Theodore Ts'o"
Cc: Dmitry Monakhov
Cc: Jan Kara
---
fs/ext4/ext4.h| 8 +---
fs/ext4/inode.c | 85 +-
fs/ext4/page-io.c | 121 --
3 files changed, 85 insertions(+), 129 deletions(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 0aabb34..5aae3d1 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -209,7 +209,6 @@ typedef struct ext4_io_end {
ssize_t size; /* size of the extent */
struct kiocb*iocb; /* iocb struct for AIO */
int result; /* error value for AIO */
- atomic_tcount; /* reference counter */
} ext4_io_end_t;
struct ext4_io_submit {
@@ -2651,14 +2650,11 @@ extern int ext4_move_extents(struct file *o_filp,
struct file *d_filp,
/* page-io.c */
extern int __init ext4_init_pageio(void);
+extern void ext4_add_complete_io(ext4_io_end_t *io_end);
extern void ext4_exit_pageio(void);
extern void ext4_ioend_shutdown(struct inode *);
+extern void ext4_free_io_end(ext4_io_end_t *io);
extern ext4_io_end_t *ext4_init_io_end(struct inode *inode, gfp_t flags);
-extern ext4_io_end_t *ext4_get_io_end(ext4_io_end_t *io_end);
-extern int ext4_put_io_end(ext4_io_end_t *io_end);
-extern void ext4_put_io_end_defer(ext4_io_end_t *io_end);
-extern void ext4_io_submit_init(struct ext4_io_submit *io,
- struct writeback_control *wbc);
extern void ext4_end_io_work(struct work_struct *work);
extern void ext4_io_submit(struct ext4_io_submit *io);
extern int ext4_bio_write_page(struct ext4_io_submit *io,
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 793d44b..d666569 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1487,10 +1487,7 @@ static int mpage_da_submit_io(struct mpage_da_data *mpd,
struct ext4_io_submit io_submit;
BUG_ON(mpd->next_page <= mpd->first_page);
- ext4_io_submit_init(&io_submit, mpd->wbc);
- io_submit.io_end = ext4_init_io_end(inode, GFP_NOFS);
- if (!io_submit.io_end)
- return -ENOMEM;
+ memset(&io_submit, 0, sizeof(io_submit));
/*
* We need to start from the first_page to the next_page - 1
* to make sure we also write the mapped dirty buffer_heads.
@@ -1578,8 +1575,6 @@ static int mpage_da_submit_io(struct mpage_da_data *mpd,
pagevec_release(&pvec);
}
ext4_io_submit(&io_submit);
- /* Drop io_end reference we got from init */
- ext4_put_io_end_defer(io_submit.io_end);
return ret;
}
@@ -2238,16 +2233,9 @@ static int ext4_writepage(struct page *page,
*/
return __ext4_journalled_writepage(page, len);
- ext4_io_submit_init(&io_submit, wbc);
- io_submit.io_end = ext4_init_io_end(inode, GFP_NOFS);
- if (!io_submit.io_end) {
- redirty_page_for_writepage(wbc, page);
- return -ENOMEM;
- }
+ memset(&io_submit, 0, sizeof(io_submit));
ret = ext4_bio_write_page(&io_submit, page, len, wbc);
ext4_io_submit(&io_submit);
- /* Drop io_end reference we got from init */
- ext4_put_io_end_defer(io_submit.io_end);
return ret;
}
@@ -3078,13 +3066,9 @@ static void ext4_end_io_dio(struct kiocb *iocb, loff_t
offset,
struct inode *inode = file_inode(iocb->ki_filp);
ext4_io_end_t *io_end = iocb->private;
- /* if not async direct IO just return */
- if (!io_end) {
- inode_dio_done(inode);
- if (is_async)
- aio_complete(iocb, ret, 0);
- return;
- }
+ /* if not async direct IO or dio with 0 bytes write, just return */
+ if (!io_end || !size)
+
