Re: [PATCH v5 10/18] btrfs: track original extent owner in head_ref

David Sterba
2023-09-07T16:56:17Z

On Thu, Jul 27, 2023 at 03:12:57PM -0700, Boris Burkov wrote:
> Simple quotas requires tracking the original creating root of any given
> extent. This gets complicated when multiple subvolumes create
> overlapping/contradictory refs in the same transaction. For example,
> due to modifying or deleting an extent while also snapshotting it.
>
> To resolve this in a general way, take advantage of the fact that we are
> essentially already tracking this for handling releasing reservations.
> The head ref coalesces the various refs and uses must_insert_reserved to
> check if it needs to create an extent/free reservation. Store the ref
> that set must_insert_reserved as the owning ref on the head ref.
>
> Note that this can result in writing an extent for the very first time
> with an owner different from its only ref, but it will look the same as
> if you first created it with the original owning ref, then added the
> other ref, then removed the owning ref.
>
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
> fs/btrfs/delayed-ref.c | 20 ++++++++++++++++----
> fs/btrfs/delayed-ref.h | 7 +++++++
> 2 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
> index f0bae1e1c455..28ba7a9eb3c3 100644
> --- a/fs/btrfs/delayed-ref.c
> +++ b/fs/btrfs/delayed-ref.c
> @@ -623,6 +623,16 @@ static noinline void update_existing_head_ref(struct btrfs_trans_handle *trans,
> BUG_ON(existing->is_data != update->is_data);
>
> spin_lock(&existing->lock);
> +
> + /*
> + * When freeing an extent, we may not know the owning root
> + * when we first create the head_ref. However, some deref before the
> + * last deref will know it, so we just need to update the head_ref
> + * accordingly

We now write comments as full sentences so please use a "." at the end.

> + */
> + if (!existing->owning_root)
> + existing->owning_root = update->owning_root;
> +
> if (update->must_insert_reserved) {
> /* if the extent was freed and then
> * reallocated before the delayed ref
> @@ -632,6 +642,7 @@ static noinline void update_existing_head_ref(struct btrfs_trans_handle *trans,
> * Set it again here
> */
> existing->must_insert_reserved = update->must_insert_reserved;
> + existing->owning_root = update->owning_root;
>
> /*
> * update the num_bytes so we make sure the accounting
> @@ -694,7 +705,7 @@ static void init_delayed_ref_head(struct btrfs_delayed_ref_head *head_ref,
> struct btrfs_qgroup_extent_record *qrecord,
> u64 bytenr, u64 num_bytes, u64 ref_root,
> u64 reserved, int action, bool is_data,
> - bool is_system)
> + bool is_system, u64 owning_root)
> {
> int count_mod = 1;
> bool must_insert_reserved = false;
> @@ -735,6 +746,7 @@ static void init_delayed_ref_head(struct btrfs_delayed_ref_head *head_ref,
> head_ref->num_bytes = num_bytes;
> head_ref->ref_mod = count_mod;
> head_ref->must_insert_reserved = must_insert_reserved;
> + head_ref->owning_root = owning_root;
> head_ref->is_data = is_data;
> head_ref->is_system = is_system;
> head_ref->ref_tree = RB_ROOT_CACHED;
> @@ -922,7 +934,7 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans,
>
> init_delayed_ref_head(head_ref, record, bytenr, num_bytes,
> generic_ref->tree_ref.ref_root, 0, action,
> - false, is_system);
> + false, is_system, generic_ref->owning_root);
> head_ref->extent_op = extent_op;
>
> delayed_refs = &trans->transaction->delayed_refs;
> @@ -1014,7 +1026,7 @@ int btrfs_add_delayed_data_ref(struct btrfs_trans_handle *trans,
> }
>
> init_delayed_ref_head(head_ref, record, bytenr, num_bytes, ref_root,
> - reserved, action, true, false);
> + reserved, action, true, false, generic_ref->owning_root);
> head_ref->extent_op = NULL;
>
> delayed_refs = &trans->transaction->delayed_refs;
> @@ -1060,7 +1072,7 @@ int btrfs_add_delayed_extent_op(struct btrfs_trans_handle *trans,
> return -ENOMEM;
>
> init_delayed_ref_head(head_ref, NULL, bytenr, num_bytes, 0, 0,
> - BTRFS_UPDATE_DELAYED_HEAD, false, false);
> + BTRFS_UPDATE_DELAYED_HEAD, false, false, 0);
> head_ref->extent_op = extent_op;
>
> delayed_refs = &trans->transaction->delayed_refs;
> diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
> index 0729850a9193..71f0a6e5d583 100644
> --- a/fs/btrfs/delayed-ref.h
> +++ b/fs/btrfs/delayed-ref.h
> @@ -117,6 +117,13 @@ struct btrfs_delayed_ref_head {
> * the free has happened.
> */
> bool must_insert_reserved;
> +
> + /*
> + * The root which triggered the allocation when
> + * must_insert_reserved is true
> + */
> + u64 owning_root;

Please reorder this so it's not among bools, this would create a gap of
7 bytes before that.

> +
> bool is_data;
> bool is_system;
> bool processing;
> --
> 2.41.0