On 2020/5/14 下午9:20, Filipe Manana wrote:
> On Thu, May 14, 2020 at 8:35 AM Qu Wenruo <wqu@xxxxxxxx> wrote:
>>
>> There are a lot of root owner check in btrfs_truncate_inode_items()
>> like:
>>
>> if (test_bit(BTRFS_ROOT_SHAREABLE, &root->state) ||
>> root == fs_info->tree_root)
>>
>> But considering that, there are only those trees can have INODE_ITEMs:
>> - tree root (For v1 space cache)
>> - subvolume trees
>> - tree reloc trees
>> - data reloc tree
>> - log trees
>>
>> And since subvolume/tree reloc/data reloc trees all have SHAREABLE bit,
>> and we're checking tree root manually, so above check is just excluding
>> log trees.
>>
>> This patch will replace two of such checks to a much simpler one:
>>
>> if (root->root_key.objectid != BTRFS_TREE_LOG_OBJECTID)
>>
>> This would merge btrfs_drop_extent_cache() and lock_extent_bits() call
>> into the same if branch.
>>
>> Also since we're here, add one comment explaining why we don't want to
>> call lock_extent_bits()/drop_extent_cache() on log trees.
>>
>> Finally replace ALIGN()/ALIGN_DOWN() to round_up()/round_down(), as I'm
>> always bad at determing the alignement direction.
>>
>> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
>> ---
>> fs/btrfs/inode.c | 27 ++++++++++++++-------------
>> 1 file changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index a6c26c10ffc5..771af55038bf 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -4101,7 +4101,7 @@ int btrfs_truncate_inode_items(struct btrfs_trans_handle *trans,
>> u64 bytes_deleted = 0;
>> bool be_nice = false;
>> bool should_throttle = false;
>> - const u64 lock_start = ALIGN_DOWN(new_size, fs_info->sectorsize);
>> + const u64 lock_start = round_down(new_size, fs_info->sectorsize);
>
> Hum, seriously? Why does ALIGN_DOWN confuses you? ALIGN, means to
> align, and the DOWN part is very explicit about the direction.
ALIGN_DOWN() doesn't, but the later ALIGN() needs to check if it's
ceiling or floor.
So I unify them to round_up/round_down().
>
>> struct extent_state *cached_state = NULL;
>>
>> BUG_ON(new_size > 0 && min_type != BTRFS_EXTENT_DATA_KEY);
>> @@ -4121,20 +4121,22 @@ int btrfs_truncate_inode_items(struct btrfs_trans_handle *trans,
>> return -ENOMEM;
>> path->reada = READA_BACK;
>>
>> - if (root->root_key.objectid != BTRFS_TREE_LOG_OBJECTID)
>> - lock_extent_bits(&BTRFS_I(inode)->io_tree, lock_start, (u64)-1,
>> - &cached_state);
>> -
>> /*
>> - * We want to drop from the next block forward in case this new size is
>> - * not block aligned since we will be keeping the last block of the
>> - * extent just the way it is.
>> + * There will be a lot of exceptions for log trees, as log inodes are
>> + * not real inodes, but an anchor for logged inodes.
>
> This is a very confusing sentence, you're saying logged inodes are an
> "anchor" (whatever that means) to themselves.
> Either leave nothing as it was, or just say log tree operations aren't
> supposed to change anything on the inodes.
OK, I'll not add such confusing comment then.
Thanks,
Qu
>
> Thanks.
>
>> */
>> - if (test_bit(BTRFS_ROOT_SHAREABLE, &root->state) ||
>> - root == fs_info->tree_root)
>> - btrfs_drop_extent_cache(BTRFS_I(inode), ALIGN(new_size,
>> + if (root->root_key.objectid != BTRFS_TREE_LOG_OBJECTID) {
>> + /*
>> + * We want to drop from the next block forward in case this
>> + * new size is not block aligned since we will be keeping the
>> + * last block of the extent just the way it is.
>> + */
>> + lock_extent_bits(&BTRFS_I(inode)->io_tree, lock_start, (u64)-1,
>> + &cached_state);
>> + btrfs_drop_extent_cache(BTRFS_I(inode), round_up(new_size,
>> fs_info->sectorsize),
>> (u64)-1, 0);
>> + }
>>
>> /*
>> * This function is also used to drop the items in the log tree before
>> @@ -4335,8 +4337,7 @@ int btrfs_truncate_inode_items(struct btrfs_trans_handle *trans,
>> should_throttle = false;
>>
>> if (found_extent &&
>> - (test_bit(BTRFS_ROOT_SHAREABLE, &root->state) ||
>> - root == fs_info->tree_root)) {
>> + root->root_key.objectid != BTRFS_TREE_LOG_OBJECTID) {
>> struct btrfs_ref ref = { 0 };
>>
>> bytes_deleted += extent_num_bytes;
>> --
>> 2.26.2
>>
>
>
Attachment:
signature.asc
Description: OpenPGP digital signature
