Hi Ryusuke, On Mon, 2013-05-06 at 23:07 +0900, Ryusuke Konishi wrote: [snip] > I agree to use count of descriptor blocks of ifile to > calculate the approximate value of the total file nodes. > > Here are my comments on the patch: > > (1) Consider using nilfs_bmap_last_key() instead of repeating read > of descriptor blocks to decide new desc_blocks count. > The current logic may incur many device read accesses > even if it is updated in a stepwise fashion with > NILFS_IFILE_I(ifile)->mi.mi_desc_blocks_count. > Maybe I misunderstand something but I think that nilfs_bmap_last_key() can be less efficient solution. As I understand, descriptor block with 4 KB in size can describe 1024 groups. Every group can describe 32768 inodes. As a result, we can describe 32768 * 1024 = 33554432 inodes by means of one descriptor block. So, the most frequent use-case will be calculation with 1 descriptor block, from my point of view. Yes, I agree that it is possible not read mi_desc_blocks_count in the beginning of nilfs_count_free_inodes() but to use simply 1 as constant value. But, anyway, even if we need to define a real count of descriptor blocks then it makes only once (if we exhaust descriptor block capacity or after mount in the case of presence of several descriptor blocks). This counted value is saved in mi_desc_blocks_count and is used in further calculations by means of reading mi_desc_blocks_count during exhausting of next 33554432 inodes count. Do you mean that nilfs_bmap_last_key() can initiate lesser read requests as my way of using nilfs_palloc_get_desc_block() call? > (2) Do not use __statfs_word type for local variables or arguments of > functions. I think an integer type such as u64 should be used > instead. > Ok. I agree. > (3) Consider moving the main part of nilfs_count_free_inodes() > function to alloc.c, for example, as a function like: > > int nilfs_palloc_count_max_entries(struct inode *inode, u64 nentries, > u64 *nmaxentries); > > Then, nilfs_palloc_groups_per_desc_block() and > nilfs_palloc_groups_count() do not need to be moved, > and nilfs_root argument becomes eliminable from the routine > added to ifile.c. > Ok. I'll do it. > (4) Please consider the name convention of functions in ifile.c. > The ifile.c includes routines related to ifile inode, > and all its functions have the same name convention (i.e. > nilfs_ifile_xxxx() ). > Ok. I agree. > (5) nilfs_count_free_inodes() may return -ERANGE. > (It's OK if inode_count is given as an argument of the function.) > But, nilfs_statfs() should not return -ERANGE as-is. > > In that case, I think nilfs_statfs() should output a warning > message and ignore the error code. > Currently, it is processed returned error of nilfs_count_free_blocks() in nilfs_statfs(). Yes, nilfs_count_free_blocks() doesn't return any error really. But, potentially, nilfs_statfs() can return error from this method. So, as I understand, you suggest simply to warn in nilfs_statfs() about errors in called methods and doesn't return these errors from it. Am I correct? With the best regards, Vyacheslav Dubeyko. > Regards, > Ryusuke Konishi > -- To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
