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 <w...@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
>
> > + 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?
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.
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.
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.
