| Message ID | alpine.LFD.2.00.1303121502240.7128@dhcp-1-104.brq.redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Mar 12, 2013 at 03:11:23PM +0100, Lukáš Czerner wrote: > So there is indeed a problem with the mentioned commit > > 67a5da564f97f31c4054d358e00b34d7ee570da5 > > Due to the bug in that code is has exactly the opposite result - > with this commit we will _never_ zero out blocks instead of creating > uninitialized extents. In other words, we will always create > uninitialized extent. Whoops. I even remember how this bug happened. Originally max_zeroout was in file system blocks, and it was suggested that we change this to use units of kilobytes instead. Unfortunately, this change wasn't done completely. :-( > This can be easily fixed by the following patch (which makes the > warning go away), but it brings up a question whether this "optimization" > was worth it in the first place since noone noticed that it had exactly > the opposite effect than it should have had :) Well, I had noticed that random AIO workloads resulted in the extent tree getting far more fragmented than I had expected. (See previous discusisons about how we really need to improve our ability to merge empty leaf and index nodes in the extent tree.) It will be worthwhile to fix this bug and then see how much remains of extent tree fragmentation problem once this is fixed.... Thanks for the catch! When you have a chance, could you resend with a commit description? > However it still does not resolve the issue completely, because even > without the zeroout we should have had reserved enough metadata > blocks to cover the extent split. I still need to investigate > a little bit further. Yep, agreed. Sounds like there is more than one bug hiding here. - Ted -- 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 Tue, Mar 12, 2013 at 12:19:13PM -0400, Theodore Ts'o wrote: > On Tue, Mar 12, 2013 at 03:11:23PM +0100, Lukáš Czerner wrote: > > So there is indeed a problem with the mentioned commit > > > > 67a5da564f97f31c4054d358e00b34d7ee570da5 > > > > Due to the bug in that code is has exactly the opposite result - > > with this commit we will _never_ zero out blocks instead of creating > > uninitialized extents. In other words, we will always create > > uninitialized extent. > > Whoops. I even remember how this bug happened. Originally > max_zeroout was in file system blocks, and it was suggested that we > change this to use units of kilobytes instead. Unfortunately, this > change wasn't done completely. :-( > > > This can be easily fixed by the following patch (which makes the > > warning go away), but it brings up a question whether this "optimization" > > was worth it in the first place since noone noticed that it had exactly > > the opposite effect than it should have had :) > > Well, I had noticed that random AIO workloads resulted in the extent > tree getting far more fragmented than I had expected. (See previous > discusisons about how we really need to improve our ability to merge > empty leaf and index nodes in the extent tree.) Agree, we could do better in merging extent tree. I will take a look at it, but, sorry, my plate is too full recently. So it has a low priority. Regards, - Zheng -- 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 cac80120..aeb80bc 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -3227,7 +3227,7 @@ static int ext4_ext_convert_to_initialized(handle_t *handle, if (EXT4_EXT_MAY_ZEROOUT & split_flag) max_zeroout = sbi->s_extent_max_zeroout_kb >> - inode->i_sb->s_blocksize_bits; + (inode->i_sb->s_blocksize_bits - 10); /* If extent is less than s_max_zeroout_kb, zeroout directly */ if (max_zeroout && (ee_len <= max_zeroout)) {