Re: [PATCH 10/18] btrfs: track owning root in btrfs_ref

[Boris Burkov <boris@bur.io>]

On Thu, Jul 13, 2023 at 12:58:25PM -0400, Josef Bacik wrote:
> On Wed, Jul 05, 2023 at 04:20:47PM -0700, Boris Burkov wrote:
> > While data extents require us to store additional inline refs to track
> > the original owner on free, this information is available implicitly for
> > metadata. It is found in the owner field of the header of the tree
> > block. Even if other trees refer to this block and the original ref goes
> > away, we will not rewrite that header field, so it will reliably give the
> > original owner.
> > 
> > In addition, there is a relocation case where a new data extent needs to
> > have an owning root separate from the referring root wired through
> > delayed refs.
> > 
> > To use it for recording simple quota deltas, we need to wire this root
> > id through from when we create the delayed ref until we fully process
> > it. Store it in the generic btrfs_ref struct of the delayed ref.
> > 
> > Signed-off-by: Boris Burkov <boris@bur.io>
> > ---
> >  fs/btrfs/delayed-ref.c |  7 ++++---
> >  fs/btrfs/delayed-ref.h | 13 +++++++++++--
> >  fs/btrfs/extent-tree.c | 19 ++++++++++++-------
> >  fs/btrfs/file.c        | 10 +++++-----
> >  fs/btrfs/inode-item.c  |  2 +-
> >  fs/btrfs/relocation.c  | 16 +++++++++-------
> >  fs/btrfs/tree-log.c    |  3 ++-
> >  7 files changed, 44 insertions(+), 26 deletions(-)
> > 
> > diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
> > index f0bae1e1c455..49c320f2334b 100644
> > --- a/fs/btrfs/delayed-ref.c
> > +++ b/fs/btrfs/delayed-ref.c
> > @@ -840,7 +840,7 @@ add_delayed_ref_head(struct btrfs_trans_handle *trans,
> >  static void init_delayed_ref_common(struct btrfs_fs_info *fs_info,
> >  				    struct btrfs_delayed_ref_node *ref,
> >  				    u64 bytenr, u64 num_bytes, u64 ref_root,
> > -				    int action, u8 ref_type)
> > +				    int action, u8 ref_type, u64 owning_root)
> >  {
> >  	u64 seq = 0;
> >  
> > @@ -857,6 +857,7 @@ static void init_delayed_ref_common(struct btrfs_fs_info *fs_info,
> >  	ref->action = action;
> >  	ref->seq = seq;
> >  	ref->type = ref_type;
> > +	ref->owning_root = owning_root;
> >  	RB_CLEAR_NODE(&ref->ref_node);
> >  	INIT_LIST_HEAD(&ref->add_list);
> >  }
> > @@ -915,7 +916,7 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans,
> >  
> >  	init_delayed_ref_common(fs_info, &ref->node, bytenr, num_bytes,
> >  				generic_ref->tree_ref.ref_root, action,
> > -				ref_type);
> > +				ref_type, generic_ref->owning_root);
> >  	ref->root = generic_ref->tree_ref.ref_root;
> >  	ref->parent = parent;
> >  	ref->level = level;
> > @@ -989,7 +990,7 @@ int btrfs_add_delayed_data_ref(struct btrfs_trans_handle *trans,
> >  	else
> >  	        ref_type = BTRFS_EXTENT_DATA_REF_KEY;
> >  	init_delayed_ref_common(fs_info, &ref->node, bytenr, num_bytes,
> > -				ref_root, action, ref_type);
> > +				ref_root, action, ref_type, ref_root);
> >  	ref->root = ref_root;
> >  	ref->parent = parent;
> >  	ref->objectid = owner;
> > diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
> > index a71eff78469c..336c33c28191 100644
> > --- a/fs/btrfs/delayed-ref.h
> > +++ b/fs/btrfs/delayed-ref.h
> > @@ -32,6 +32,12 @@ struct btrfs_delayed_ref_node {
> >  	/* seq number to keep track of insertion order */
> >  	u64 seq;
> >  
> > +	/*
> > +	 * root which originally allocated this extent and owns it for
> > +	 * simple quota accounting purposes.
> > +	 */
> > +	u64 owning_root;
> > +
> >  	/* ref count on this data structure */
> >  	refcount_t refs;
> >  
> > @@ -239,6 +245,7 @@ struct btrfs_ref {
> >  #endif
> >  	u64 bytenr;
> >  	u64 len;
> > +	u64 owning_root;
> >  
> >  	/* Bytenr of the parent tree block */
> >  	u64 parent;
> > @@ -278,16 +285,18 @@ static inline u64 btrfs_calc_delayed_ref_bytes(const struct btrfs_fs_info *fs_in
> >  }
> >  
> >  static inline void btrfs_init_generic_ref(struct btrfs_ref *generic_ref,
> > -				int action, u64 bytenr, u64 len, u64 parent)
> > +				int action, u64 bytenr, u64 len, u64 parent, u64 owning_root)
> >  {
> >  	generic_ref->action = action;
> >  	generic_ref->bytenr = bytenr;
> >  	generic_ref->len = len;
> >  	generic_ref->parent = parent;
> > +	generic_ref->owning_root = owning_root;
> >  }
> >  
> >  static inline void btrfs_init_tree_ref(struct btrfs_ref *generic_ref,
> > -				int level, u64 root, u64 mod_root, bool skip_qgroup)
> > +				int level, u64 root, u64 mod_root,
> > +				bool skip_qgroup)
> >  {
> >  #ifdef CONFIG_BTRFS_FS_REF_VERIFY
> >  	/* If @real_root not set, use @root as fallback */
> > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > index 041f2eb153d7..fa53f7cbd84a 100644
> > --- a/fs/btrfs/extent-tree.c
> > +++ b/fs/btrfs/extent-tree.c
> > @@ -2410,7 +2410,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
> >  			num_bytes = btrfs_file_extent_disk_num_bytes(buf, fi);
> >  			key.offset -= btrfs_file_extent_offset(buf, fi);
> >  			btrfs_init_generic_ref(&generic_ref, action, bytenr,
> > -					       num_bytes, parent);
> > +					       num_bytes, parent, ref_root);
> >  			btrfs_init_data_ref(&generic_ref, ref_root, key.objectid,
> >  					    key.offset, root->root_key.objectid,
> >  					    for_reloc);
> > @@ -2424,7 +2424,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
> >  			bytenr = btrfs_node_blockptr(buf, i);
> >  			num_bytes = fs_info->nodesize;
> >  			btrfs_init_generic_ref(&generic_ref, action, bytenr,
> > -					       num_bytes, parent);
> > +					       num_bytes, parent, ref_root);
> >  			btrfs_init_tree_ref(&generic_ref, level - 1, ref_root,
> >  					    root->root_key.objectid, for_reloc);
> >  			if (inc)
> > @@ -3242,7 +3242,7 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
> >  	int ret;
> >  
> >  	btrfs_init_generic_ref(&generic_ref, BTRFS_DROP_DELAYED_REF,
> > -			       buf->start, buf->len, parent);
> > +			       buf->start, buf->len, parent, btrfs_header_owner(buf));
> >  	btrfs_init_tree_ref(&generic_ref, btrfs_header_level(buf),
> >  			    root_id, 0, false);
> >  
> > @@ -4699,12 +4699,16 @@ int btrfs_alloc_reserved_file_extent(struct btrfs_trans_handle *trans,
> >  				     struct btrfs_key *ins)
> >  {
> >  	struct btrfs_ref generic_ref = { 0 };
> > +	u64 root_objectid = root->root_key.objectid;
> > +	u64 owning_root = root_objectid;
> > +
> > +	BUG_ON(root_objectid == BTRFS_TREE_LOG_OBJECTID);
> >  
> 
> This is a duplicate check of what's checked below.  Also we don't want to add
> new BUG_ON()'s, we want to add ASSERT()'s.  Thanks,

Oops. I just meant to use root_objectid instead of
root->root_key.objectid and somehow left in the old line, my bad.

I assume we don't want to change these existing BUG_ONs to ASSERTs
without specific justification, though.

> 
> Josef