Re: [PATCH] btrfs: remove pointless code when creating and deleting a subvolume

[Boris Burkov <boris@bur.io>]

On Thu, Jun 06, 2024 at 09:30:07AM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> When creating and deleting a subvolume, after starting a transaction we
> are explicitly calling btrfs_record_root_in_trans() for the root which we
> passed to btrfs_start_transaction(). This is pointless because at
> transaction.c:start_transaction() we end up doing that call, regardless
> of whether we actually start a new transaction or join an existing one,
> and if we were not it would mean the root item of that root would not
> be updated in the root tree when committing the transaction, leading to
> problems easy to spot with fstests for example.
> 
> Remove these redundant calls. They were introduced with commit
> 74e97958121a ("btrfs: qgroup: fix qgroup prealloc rsv leak in subvolume
> operations").

Re-reading it now, I agree that start_transaction will ensure what we need,
and if it fails we go to paths that result in freeing the reserved space
here in the subvolume code.

With that said, I spent like two weeks on this battling generic/269 so
there might be something subtle that I'm forgetting and didn't explain
well enough in the patch. Reading it now, I do think it's most likely
that the fixes to the release path were sufficient to fix the bug.

Just to be safe, can you run generic/269 with squotas turned on via mkfs
~10 times? It usually reproduced for me in 5-10 runs, so if it's not too
much bother, maybe 20 to be really safe.

Thanks,
Boris

> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/inode.c | 5 -----
>  fs/btrfs/ioctl.c | 3 ---
>  2 files changed, 8 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 0610b9fa6fae..ddf96c4cc858 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4552,11 +4552,6 @@ int btrfs_delete_subvolume(struct btrfs_inode *dir, struct dentry *dentry)
>  		ret = PTR_ERR(trans);
>  		goto out_release;
>  	}
> -	ret = btrfs_record_root_in_trans(trans, root);
> -	if (ret) {
> -		btrfs_abort_transaction(trans, ret);
> -		goto out_end_trans;
> -	}
>  	btrfs_qgroup_convert_reserved_meta(root, qgroup_reserved);
>  	qgroup_reserved = 0;
>  	trans->block_rsv = &block_rsv;
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 5e3cb0210869..d00d49338ecb 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -658,9 +658,6 @@ static noinline int create_subvol(struct mnt_idmap *idmap,
>  		ret = PTR_ERR(trans);
>  		goto out_release_rsv;
>  	}
> -	ret = btrfs_record_root_in_trans(trans, BTRFS_I(dir)->root);
> -	if (ret)
> -		goto out;
>  	btrfs_qgroup_convert_reserved_meta(root, qgroup_reserved);
>  	qgroup_reserved = 0;
>  	trans->block_rsv = &block_rsv;
> -- 
> 2.43.0
>