Re: [PATCH 7/8] btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref()

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2018/12/11 上午1:28, David Sterba wrote:
> On Mon, Dec 10, 2018 at 11:52:25AM +0200, Nikolay Borisov wrote:
>> On 6.12.18 г. 8:59 ч., Qu Wenruo wrote:
>>> Now we don't need to play the dirty game of reusing @owner for tree block
>>> level.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  fs/btrfs/ctree.h       |  6 ++---
>>>  fs/btrfs/extent-tree.c | 58 ++++++++++++++++++++++--------------------
>>>  fs/btrfs/file.c        | 20 ++++++++++-----
>>>  fs/btrfs/inode.c       | 10 +++++---
>>>  fs/btrfs/ioctl.c       | 17 ++++++++-----
>>>  fs/btrfs/relocation.c  | 44 ++++++++++++++++++++------------
>>>  fs/btrfs/tree-log.c    | 12 ++++++---
>>>  7 files changed, 100 insertions(+), 67 deletions(-)
>>>
>>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>>> index 6f4b1e605736..db3df5ce6087 100644
>>> --- a/fs/btrfs/ctree.h
>>> +++ b/fs/btrfs/ctree.h
>>> @@ -40,6 +40,7 @@ extern struct kmem_cache *btrfs_bit_radix_cachep;
>>>  extern struct kmem_cache *btrfs_path_cachep;
>>>  extern struct kmem_cache *btrfs_free_space_cachep;
>>>  struct btrfs_ordered_sum;
>>> +struct btrfs_ref;
>>>  
>>>  #define BTRFS_MAGIC 0x4D5F53665248425FULL /* ascii _BHRfS_M, no null */
>>>  
>>> @@ -2682,10 +2683,7 @@ int btrfs_free_and_pin_reserved_extent(struct btrfs_fs_info *fs_info,
>>>  void btrfs_prepare_extent_commit(struct btrfs_fs_info *fs_info);
>>>  int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans);
>>>  int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,
>>> -			 struct btrfs_root *root,
>>> -			 u64 bytenr, u64 num_bytes, u64 parent,
>>> -			 u64 root_objectid, u64 owner, u64 offset,
>>> -			 bool for_reloc);
>>> +			 struct btrfs_ref *generic_ref);
>>>  
>>>  int btrfs_start_dirty_block_groups(struct btrfs_trans_handle *trans);
>>>  int btrfs_write_dirty_block_groups(struct btrfs_trans_handle *trans,
>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>> index 70c05ca30d9a..ff60091aef6b 100644
>>> --- a/fs/btrfs/extent-tree.c
>>> +++ b/fs/btrfs/extent-tree.c
>>> @@ -2026,36 +2026,28 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
>>>  
>>>  /* Can return -ENOMEM */
>>>  int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,
>>> -			 struct btrfs_root *root,
>>> -			 u64 bytenr, u64 num_bytes, u64 parent,
>>> -			 u64 root_objectid, u64 owner, u64 offset,
>>> -			 bool for_reloc)
>>> +			 struct btrfs_ref *generic_ref)
>>>  {
>>> -	struct btrfs_fs_info *fs_info = root->fs_info;
>>> -	struct btrfs_ref generic_ref = { 0 };
>>> +	struct btrfs_fs_info *fs_info = trans->fs_info;
>>>  	int old_ref_mod, new_ref_mod;
>>>  	int ret;
>>>  
>>> -	BUG_ON(owner < BTRFS_FIRST_FREE_OBJECTID &&
>>> -	       root_objectid == BTRFS_TREE_LOG_OBJECTID);
>>> +	BUG_ON(generic_ref->type == BTRFS_REF_NOT_SET ||
>>> +	       !generic_ref->action);
>>
>> Ouch, we should be removing BUG_ONs and not introducing new ones. Make
>> it an assert

Oh I forgot this one, it should be ASSERT().

>>
>>> +	BUG_ON(generic_ref->type == BTRFS_REF_METADATA &&
>>> +	       generic_ref->tree_ref.root == BTRFS_TREE_LOG_OBJECTID);
>>
>> Ideally this should also be converted to an assert. I guess it could
>> only happen if the filesystem is corrupted so perhaps is redundant now?

This is from the original code, just changed to btrfs_ref interface.

Unlike previous easy check, this one could happen by careless caller,
just one wrong @ref_root could easily lead to this BUG_ON().

> 
> I think we need more types of assert, not all BUG_ONs can be replaced by
> ASSERT as behaviour would change depending ond CONFIG_BTRFS_ASSERTS. IMO
> the asserts are best suited for development-time checks and cannot
> replace runtime-checks where the BUG_ON is a big hammer to stop
> execution otherwise it would cause worse things later.

Yep, so I kept the original BUG_ON().
(Although forgot to use ASSERT() for the new check)

> 
> This was mentioned in the past a few times, what we really don't want
> are new BUG_ONs instead of proper error handling. The rest would be nice
> to annotate by comments why it's there and that there's really no other
> way around that.

Then I'd go back to my old practice, return EINVAL/EUCLEAN with proper
error message.
By that way, we won't continue under all case, and with proper error
message to info developer to simplify user debug.

The only reason for me not to do this practice this time is, I didn't
see much adoption except me.

So if there is some positive feedback, I'd like to use such practice more.

Thanks,
Qu

> 
> I don't recall all the ideas so am not saying either way for the asserts
> or bugons. Both would be ok here and full audit of BUG_ONs is one of my
> long-term projects so it would get sorted eventually.
>