Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move assignment of i_blkbits field #4906

Closed
wants to merge 1 commit into from

Conversation

lorddoskias
Copy link
Contributor

So this continues my quest in eliminating zfs_inode_update implementation. This change is trivial enough, however when researching it I observed something peculiar. In the linux kernel most filessystem don't set this field directly as its already being set from the generic layer in inode_init_always to sb->s_blocksize_bits. In zfs this field is being set to whatever the current value of the recordsize property is set to. I cannot help it but wonder shouldn't the inode shift bits be set at allocation time to whatever the current s_blocksize_bits is set to, since this is the size of the record that the inode is going to hold?

@behlendorf @dweeezil can you please explain why the inode's shift bits are always seto the minimum allowed shift and not to ilog2(sb->s_blocksize_bits) ?

Currently i_blkbits is always set to SPA_MINBLOCKSHIFT everytime
zfs_inode_update_impl is called. Since this value never changes
move its assignment at inode creation time.
@behlendorf
Copy link
Contributor

@lorddoskias always setting the block size to SPA_MINBLOCKSHIFT arguably isn't correct but neither is setting it to sb->s_blocksize_bits. The block size for the inode will change up until the maximum record size is reached and the inode contains multiple blocks.

Really the best thing we could do here is update the inode's i_blkbits field when the block size changes along with i_blocks. Then we shouldn't need the sa_object_size() call in zfs_getattr_fast() and could rely on generic_fillattr(). That might even help speed up a getattr although sa_object_size() is already fairly optimized.

@lorddoskias
Copy link
Contributor Author

@behlendorf Ok, got you. But the current change doesn't really change the semantics, so I think it should be good on its own. I really like to do away with the whole zfs_inode_update conundrum

@behlendorf
Copy link
Contributor

Absolutely. The current patch LGTM.

@behlendorf behlendorf added this to the 0.7.0 milestone Jul 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants