| Message ID | 20140402164258.GH6901@thunk.org |
|---|---|
| State | Accepted, archived |
| Headers | show |
Hi Ted. 2014-04-03 1:42 GMT+09:00 Theodore Ts'o <tytso@mit.edu>: > Some final testing luckily caught a bug in your patch. I'll fix it > up, but for future reference, it's important to check to see if the > handle is valid (i.e., we are not in no-journal mode) before using any > of the journalling functions. Otherwise, you will crash if you try > using the code path when the file system does not have a journal. Thanks for finding bug. I missed no journal case.. > > BTW, I would strongly recommend using the automated regression test > system found at: > > git://git.kernel.org/pub/scm/fs/ext2/xfstests-bld.git > > It finds these sorts of problems very quickly. After you build a > kernel, running "kvm-xfstests -g quick" would have found the problem > very quickly. The fastest reproducer for the bug that I found is: > "kvm-xfstests -c nojournal shared/001". Okay, I will do the way you guided next time. Thanks! > > Cheers, > > - Ted > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index 7015917..bbba1ef 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -4902,6 +4902,9 @@ ext4_access_path(handle_t *handle, struct inode *inode, > { > int credits, err; > > + if (!ext4_handle_valid(handle)) > + return 0; > + > /* > * Check if need to extend journal credits > * 3 for leaf, sb, and inode plus 2 (bmap and group > > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 02, 2014 at 12:42:58PM -0400, Theodore Ts'o wrote: > Some final testing luckily caught a bug in your patch. I'll fix it > up, but for future reference, it's important to check to see if the > handle is valid (i.e., we are not in no-journal mode) before using any > of the journalling functions. Otherwise, you will crash if you try > using the code path when the file system does not have a journal. > > BTW, I would strongly recommend using the automated regression test > system found at: > > git://git.kernel.org/pub/scm/fs/ext2/xfstests-bld.git > > It finds these sorts of problems very quickly. After you build a > kernel, running "kvm-xfstests -g quick" would have found the problem > very quickly. The fastest reproducer for the bug that I found is: > "kvm-xfstests -c nojournal shared/001". Hi Ted, If you check the xfstests commit history, I think you'll find that Namjae wrote the xfstests that triggered the failure you are seeing. You're just testing more esoteric configurations than most people do - same as I do for XFS. We can't expect every developer to test their code on every possible configuration before they submit it.... ;) Cheers, Dave.
On Tue, Apr 08, 2014 at 08:55:04PM +1000, Dave Chinner wrote: > > If you check the xfstests commit history, I think you'll find that > Namjae wrote the xfstests that triggered the failure you are > seeing. > > You're just testing more esoteric configurations than most people do > - same as I do for XFS. We can't expect every developer to test > their code on every possible configuration before they submit > it.... ;) "kvm-xfstests -g smoke" takes about 4-5 hours to run[1], and allows all of the ext4 developers to run a much wider set of configurations. The more work they can do, the more I scale. This is one of the reasons why I'm pushing the testing automation so heavily. :-) - Ted [1] It's even faster if you run kvm-xfstests on a sacrificial ssd, or if you have a big enough machine to run using a series of 5g ramdisks. "kvm-xfstests -g auto" takes about 14 hours to run with spinning platters, so it's much more of a commitment. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index 7015917..bbba1ef 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -4902,6 +4902,9 @@ ext4_access_path(handle_t *handle, struct inode *inode, { int credits, err; + if (!ext4_handle_valid(handle)) + return 0; + /* * Check if need to extend journal credits * 3 for leaf, sb, and inode plus 2 (bmap and group