Any qualms about reverting 3d0518f4, ext4: New rec_len encoding for very large blocksizes ?
I think we should possibly revert this commit, due to a perf regression:
commit 3d0518f4758eca4339e75e5b9dbb7e06a5ce08b4
Author: Wei Yongjun <[email protected]> Date: Sat Feb 14 23:01:36 2009 -0500
ext4: New rec_len encoding for very large blocksizes
The rec_len field in the directory entry is 16 bits, so to encode blocksizes larger than 64k becomes problematic. This patch allows us to supprot block sizes up to 256k, by using the low 2 bits to extend the range of rec_len to 2**18-1 (since valid rec_len sizes must be a multiple of 4). We use the convention that a rec_len of 0 or 65535 means the filesystem block size, for compatibility with older kernels.
It's unlikely we'll see VM pages of up to 256k, but at some point we might find that the Linux VM has been enhanced to support filesystem block sizes > than the VM page size, at which point it might be useful for some applications to allow very large filesystem block sizes.
It's a novel solution to the problem, but I'm not sure it's a problem that needs to be solved today since we cannot even make filesystems with
Quoted 1 line
64k blocks:
$ mkfs.ext4 -b 131072 fsfile mkfs.ext4: invalid block size - 131072
Further, the patch un-inlines ext4_rec_len_from_disk() and adds complexity to it, and this is a pretty hot function (called from ext4_check_dir_entry) on a file-creation workload.
We could maybe try to fix it up or make it conditional on blocksize, but it seems to me that there's no reason to have this complexity in the codepaths today, when it's addressing an impossible situation.
Just for reference, the testing I did on RHEL6 was:
# bonnie++ -u root -s 0 -f -x 200 -d /mnt/test -n 32
(this does 200 iterations) and got this for the file creations:
ext4 stock: Average = 21206.8 files/s ext4 patched: Average = 22822.1 files/s
This is a 7.6% improvement...
Thoughts?
Thanks,
-Eric
-Eric
Quoted 13 lines · Eric Sandeen
commit
3d0518f4758eca4339e75e5b9dbb7e06a5ce08b4Author: Wei Yongjun <[email protected]> Date: Sat Feb 14 23:01:36 2009 -0500ext4: New
rec_lenencoding for very large blocksizesThe
rec_lenfield in the directory entry is 16 bits, so to encode blocksizes larger than 64k becomes problematic. This patch allows us to supprot block sizes up to 256k, by using the low 2 bits to extend the range ofrec_lento 2**18-1 (since validrec_lensizes must be a multiple of 4). We use the convention that arec_lenof 0 or 65535 means the filesystem block size, for compatibility with older kernels.
Quoted 4 lines
It's a novel solution to the problem, but I'm not sure it's a problem that needs to be solved today since we cannot even make filesystems with
Quoted 2 lines
64k blocks:
I don't object to reverting the > 64kB support, but we should strive to keep 64kB directory blocks working, if that is possibly going to break with reverting this patch. I don't think any of the other extN code is expecting to handle block sizes larger than 64kB, so little value to do it just in the directory handling code.
Quoted 10 lines
Just for reference, the testing I did on RHEL6 was:
# bonnie++ -u root -s 0 -f -x 200 -d
/mnt/test-n 32(this does 200 iterations) and got this for the file creations:
ext4 stock: Average = 21206.8 files/s ext4 patched: Average = 22822.1 files/sThis is a 7.6% improvement...
Nothing to sneeze at.
Cheers, Andreas
Quoted 18 lines · Andreas Dilger
Quoted 17 lines · Eric Sandeen
commit
3d0518f4758eca4339e75e5b9dbb7e06a5ce08b4Author: Wei Yongjun
<[email protected]> Date: Sat Feb 14 23:01:36 2009 -0500ext4: New
rec_lenencoding for very large blocksizesThe
rec_lenfield in the directory entry is 16 bits, so to encode blocksizes larger than 64k becomes problematic. This patch allows us to supprot block sizes up to 256k, by using the low 2 bits to extend the range ofrec_lento 2**18-1 (since validrec_lensizes must be a multiple of 4). We use the convention that arec_lenof 0 or 65535 means the filesystem block size, for compatibility with older kernels.It's a novel solution to the problem, but I'm not sure it's a problem that needs to be solved today since we cannot even make filesystems with
Quoted 1 line
Quoted 7 lines
Quoted 1 line
Quoted 1 line
64k blocks:
I don't object to reverting the > 64kB support, but we should strive to keep 64kB directory blocks working, if that is possibly going to break with reverting this patch. I don't think any of the other extN code is expecting to handle block sizes larger than 64kB, so little value to do it just in the directory handling code.
As far as I know, reverting it won't break 64kb dir blocks...?
A simpler solution to the perf problem might be to just re-inline the function, but I'm not sure what the point of all this is, as you say.
Thanks,
-Eric
Quoted 14 lines
Quoted 10 lines
Just for reference, the testing I did on RHEL6 was:
# bonnie++ -u root -s 0 -f -x 200 -d
/mnt/test-n 32(this does 200 iterations) and got this for the file creations:
ext4 stock: Average = 21206.8 files/s ext4 patched: Average = 22822.1 files/sThis is a 7.6% improvement...
Nothing to sneeze at.
Cheers, Andreas
Quoted 2 lines · Eric Sandeen
As far as I know, reverting it won't break 64kb dir blocks...?
I seem to recall there was some confusion about what was the correct way of recording a rec_len of 64k --- 0 or 65535. So after reverting the patch, we need to make sure we didn't end up breaking compatibility with (a) existing file systems and (b) what older versions of mke2fs may have generated.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
(this does 200 iterations) and got this for the file creations:
ext4 stock: Average = 21206.8 files/s ext4 patched: Average = 22822.1 files/sThis is a 7.6% improvement...
Wow. I assume that was because actually ending up burning enough CPU time that it slowed bonnie's performance? I'm not sure how how realistic is a benchmark that is simply creating vast numbers of small files in a tight loop, but certainly on non-Itanium systems where the page size is nowhere near 64k, it's arguably pointless. (Can you even configure an Itanic to have to have a page size > 64k?)
So one way of dealing wih this is making it an inline, and then #ifdef'ing out the more complex code if the page size is < 64k....
- Ted
Quoted 8 lines · Ted Ts'o
Quoted 1 line · Eric Sandeen
As far as I know, reverting it won't break 64kb dir blocks...?
I seem to recall there was some confusion about what was the correct way of recording a
rec_lenof 64k --- 0 or 65535. So after reverting the patch, we need to make sure we didn't end up breaking compatibility with (a) existing file systems and (b) what older versions of mke2fs may have generated.
Right - the newer code accepts both EXT4_MAX_REC_LEN and 0 to mean "blocksize". The old code took EXT4_MAX_REC_LEN to mean "1 << 16". For filesystems with blocksize < 64k there is no difference, but I think it makes sense to continue to accept both EXT4_MAX_REC_LEN and "0".
Quoted 9 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
(this does 200 iterations) and got this for the file creations:
ext4 stock: Average = 21206.8 files/s ext4 patched: Average = 22822.1 files/sThis is a 7.6% improvement...
So one way of dealing wih this is making it an inline, and then #ifdef'ing out the more complex code if the page size is < 64k....
Should it also be put under "unlikely()", or for that matter, the whole thing could be #ifdef'd out for PAGE_SIZE < 65536 since we won't magically grow blocksize > PAGE_SIZE support without knowing it.
Cheers, Andreas
Quoted 9 lines · Andreas Dilger
Quoted 7 lines · Ted Ts'o
Quoted 1 line · Eric Sandeen
As far as I know, reverting it won't break 64kb dir blocks...?
I seem to recall there was some confusion about what was the correct way of recording a
rec_lenof 64k --- 0 or 65535. So after reverting the patch, we need to make sure we didn't end up breaking
compatibility with (a) existing file systems and (b) what older
versions of mke2fs may have generated.
Quoted 15 lines
Right - the newer code accepts both
EXT4_MAX_REC_LENand 0 to mean "blocksize". The old code tookEXT4_MAX_REC_LENto mean "1 << 16". For filesystems with blocksize < 64k there is no difference, but I think it makes sense to continue to accept bothEXT4_MAX_REC_LENand "0".Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
(this does 200 iterations) and got this for the file creations:
ext4 stock: Average = 21206.8 files/s ext4 patched: Average
= 22822.1 files/sThis is a 7.6% improvement...
So one way of dealing wih this is making it an inline, and then #ifdef'ing out the more complex code if the page size is < 64k....
Ok, I'd thought about that route too (#ifdef, or whatnot) - I'll see if I can work it out that way.
However, #ifdef on PAGE_CACHE_SIZE didn't seem to quite work, as the original motivation was for the "whenever the VM allows block size > page size" case, I think...
-Eric
Quoted 6 lines
Should it also be put under "
unlikely()", or for that matter, the whole thing could be #ifdef'd out forPAGE_SIZE< 65536 since we won't magically grow blocksize >PAGE_SIZEsupport without knowing it.Cheers, Andreas