Re: [PATCH 0/2] nilfs2: improve inode allocation algorithm
Hi, On Sun, 12 Oct 2014 12:38:21 +0200, Andreas Rohner wrote: Hi, The algorithm simply makes sure, that after a directory inode there are a certain number of free slots available and the search for file inodes is started at their parent directory. I havn't had the time yet to do a full scale performance test of it, but my simple preliminary tests have shown, that the allocation of inodes takes a little bit longer and the lookup is a little bit faster. My simple test just creates 1500 directories and after that creates 10 files in each directory. So more testing is definetly necessary, but I wanted to get some feedback about the design first. Is my code a step in the right direction? Best regards, Andreas Rohner Andreas Rohner (2): nilfs2: support the allocation of whole blocks of meta data entries nilfs2: improve inode allocation algorithm fs/nilfs2/alloc.c | 161 fs/nilfs2/alloc.h | 18 +- fs/nilfs2/ifile.c | 63 ++-- fs/nilfs2/ifile.h | 6 +- fs/nilfs2/inode.c | 6 +- fs/nilfs2/segment.c | 5 +- 6 files changed, 235 insertions(+), 24 deletions(-) I don't know whether this patchset is going in the right direction. .. we should first measure how the original naive allocator is bad in comparison with an elaborately designed allocator like this. But, I will add some comments anyway: 1) You must not use sizeof(struct nilfs_inode) to get inode size. The size of on-disk inodes is variable and you have to use NILFS_MDT(ifile)-mi_entry_size to ensure compatibility. To get ipb (= number of inodes per block), you should use NILFS_MDT(ifile)-mi_entries_per_block. Please remove nilfs_ifile_inodes_per_block(). It's redundant. 2) __nilfs_palloc_prepare_alloc_entry() The argument block_size is so confusing. Usually we use it for the size of disk block. Please use a proper word alignment_size or so. 3) nilfs_palloc_find_available_slot_align32() This function seems to be violating endian compatibility. The order of two 32-bit words in a 64-bit word in little endian architectures differs from that of big endian architectures. Having three different implementations looks too overkill to me at this time. It should be removed unless it will make a significant difference. 4) nilfs_cpu_to_leul() Adding this macro is not preferable. It depends on endian. Did you look for a generic macro which does the same thing ? Regards, Ryusuke Konishi -- To unsubscribe from this list: send the line unsubscribe linux-nilfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] nilfs2: improve inode allocation algorithm
On Mon, 13 Oct 2014 23:52:59 +0900 (JST), Ryusuke Konishi wrote: Hi, On Sun, 12 Oct 2014 12:38:21 +0200, Andreas Rohner wrote: Hi, The algorithm simply makes sure, that after a directory inode there are a certain number of free slots available and the search for file inodes is started at their parent directory. I havn't had the time yet to do a full scale performance test of it, but my simple preliminary tests have shown, that the allocation of inodes takes a little bit longer and the lookup is a little bit faster. My simple test just creates 1500 directories and after that creates 10 files in each directory. So more testing is definetly necessary, but I wanted to get some feedback about the design first. Is my code a step in the right direction? Best regards, Andreas Rohner Andreas Rohner (2): nilfs2: support the allocation of whole blocks of meta data entries nilfs2: improve inode allocation algorithm fs/nilfs2/alloc.c | 161 fs/nilfs2/alloc.h | 18 +- fs/nilfs2/ifile.c | 63 ++-- fs/nilfs2/ifile.h | 6 +- fs/nilfs2/inode.c | 6 +- fs/nilfs2/segment.c | 5 +- 6 files changed, 235 insertions(+), 24 deletions(-) I don't know whether this patchset is going in the right direction. .. we should first measure how the original naive allocator is bad in comparison with an elaborately designed allocator like this. But, I will add some comments anyway: 1) You must not use sizeof(struct nilfs_inode) to get inode size. The size of on-disk inodes is variable and you have to use NILFS_MDT(ifile)-mi_entry_size to ensure compatibility. To get ipb (= number of inodes per block), you should use NILFS_MDT(ifile)-mi_entries_per_block. Please remove nilfs_ifile_inodes_per_block(). It's redundant. 2) __nilfs_palloc_prepare_alloc_entry() The argument block_size is so confusing. Usually we use it for the size of disk block. Please use a proper word alignment_size or so. 3) nilfs_palloc_find_available_slot_align32() This function seems to be violating endian compatibility. The order of two 32-bit words in a 64-bit word in little endian architectures differs from that of big endian architectures. Sorry, the implementation looks correct (I misread earlier). Ignore this one. Regards, Ryusuke Konishi Having three different implementations looks too overkill to me at this time. It should be removed unless it will make a significant difference. 4) nilfs_cpu_to_leul() Adding this macro is not preferable. It depends on endian. Did you look for a generic macro which does the same thing ? Regards, Ryusuke Konishi -- To unsubscribe from this list: send the line unsubscribe linux-nilfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] nilfs2: improve inode allocation algorithm
On 2014-10-13 16:52, Ryusuke Konishi wrote: Hi, On Sun, 12 Oct 2014 12:38:21 +0200, Andreas Rohner wrote: Hi, The algorithm simply makes sure, that after a directory inode there are a certain number of free slots available and the search for file inodes is started at their parent directory. I havn't had the time yet to do a full scale performance test of it, but my simple preliminary tests have shown, that the allocation of inodes takes a little bit longer and the lookup is a little bit faster. My simple test just creates 1500 directories and after that creates 10 files in each directory. So more testing is definetly necessary, but I wanted to get some feedback about the design first. Is my code a step in the right direction? Best regards, Andreas Rohner Andreas Rohner (2): nilfs2: support the allocation of whole blocks of meta data entries nilfs2: improve inode allocation algorithm fs/nilfs2/alloc.c | 161 fs/nilfs2/alloc.h | 18 +- fs/nilfs2/ifile.c | 63 ++-- fs/nilfs2/ifile.h | 6 +- fs/nilfs2/inode.c | 6 +- fs/nilfs2/segment.c | 5 +- 6 files changed, 235 insertions(+), 24 deletions(-) I don't know whether this patchset is going in the right direction. .. we should first measure how the original naive allocator is bad in comparison with an elaborately designed allocator like this. But, I will add some comments anyway: I think the alignment creates a lot of overhead, because every directory uses up a whole block in the ifile. I could also create a simpler patch that only stores the last allocated inode number in struct nilfs_ifile_info and starts the search from there for the next allocation. Then I can test the three versions against each other in a large scale test. 1) You must not use sizeof(struct nilfs_inode) to get inode size. The size of on-disk inodes is variable and you have to use NILFS_MDT(ifile)-mi_entry_size to ensure compatibility. To get ipb (= number of inodes per block), you should use NILFS_MDT(ifile)-mi_entries_per_block. Please remove nilfs_ifile_inodes_per_block(). It's redundant. Agreed. 2) __nilfs_palloc_prepare_alloc_entry() The argument block_size is so confusing. Usually we use it for the size of disk block. Please use a proper word alignment_size or so. Yes that's true alignment_size sounds better. 3) nilfs_palloc_find_available_slot_align32() This function seems to be violating endian compatibility. The order of two 32-bit words in a 64-bit word in little endian architectures differs from that of big endian architectures. Having three different implementations looks too overkill to me at this time. It should be removed unless it will make a significant difference. 32 is the most common case (4096 block size and 128 inode size), so I thought it makes sense to optimize for it. But it is not necessary and it shouldn't make a big difference. 4) nilfs_cpu_to_leul() Adding this macro is not preferable. It depends on endian. Did you look for a generic macro which does the same thing ? There are only macros for specific bit lengths, as far as I know. But unsigned long varies for 32bit and 64bit systems. You could also implement it like this: #if BITS_PER_LONG == 64 #define nilfs_cpu_to_leul cpu_to_le64 #elif BITS_PER_LONG == 32 #define nilfs_cpu_to_leul cpu_to_le32 #else #error BITS_PER_LONG not defined #endif Best regards, Andreas Rohner -- To unsubscribe from this list: send the line unsubscribe linux-nilfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] nilfs2: improve inode allocation algorithm
Hi, The algorithm simply makes sure, that after a directory inode there are a certain number of free slots available and the search for file inodes is started at their parent directory. I havn't had the time yet to do a full scale performance test of it, but my simple preliminary tests have shown, that the allocation of inodes takes a little bit longer and the lookup is a little bit faster. My simple test just creates 1500 directories and after that creates 10 files in each directory. So more testing is definetly necessary, but I wanted to get some feedback about the design first. Is my code a step in the right direction? Best regards, Andreas Rohner Andreas Rohner (2): nilfs2: support the allocation of whole blocks of meta data entries nilfs2: improve inode allocation algorithm fs/nilfs2/alloc.c | 161 fs/nilfs2/alloc.h | 18 +- fs/nilfs2/ifile.c | 63 ++-- fs/nilfs2/ifile.h | 6 +- fs/nilfs2/inode.c | 6 +- fs/nilfs2/segment.c | 5 +- 6 files changed, 235 insertions(+), 24 deletions(-) -- 2.1.2 -- To unsubscribe from this list: send the line unsubscribe linux-nilfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
