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

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

[fdmanana]

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").

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

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

[Boris Burkov]

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
> 

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

[Filipe Manana]

On Thu, Jun 6, 2024 at 8:06 PM Boris Burkov <boris@bur.io> wrote:
>
> 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.

I think it's obvious these calls are redundant.
Things would be seriously broken if start_transaction() didn't call
btrfs_record_root_in_trans().

>
> 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.

I tried that, yes, but the test sometimes fails qgroup cleanups with
-ENOSPC, both before and after this patch.
The failure is reported as a warning message in dmesg, it doesn't make
the test case fail:

[99872.487855] run fstests generic/269 at 2024-06-06 20:53:33
[99872.771632] BTRFS: device fsid 7a9e9120-5656-486a-8ea2-e0f7e12648b7
devid 1 transid 10 /dev/sdc (8:32) scanned by mount (2754727)
[99872.772502] BTRFS info (device sdc): first mount of filesystem
7a9e9120-5656-486a-8ea2-e0f7e12648b7
[99872.772515] BTRFS info (device sdc): using crc32c (crc32c-intel)
checksum algorithm
[99872.772519] BTRFS info (device sdc): using free-space-tree
[99872.774615] BTRFS info (device sdc): checking UUID tree
[99877.835179] btrfs_drop_snapshot: 10 callbacks suppressed
[99877.835191] BTRFS warning (device sdc): failed to cleanup qgroup 0/342: -28
[99901.986789] BTRFS info (device sdc): last unmount of filesystem
7a9e9120-5656-486a-8ea2-e0f7e12648b7
[99902.004561] BTRFS info (device sda): last unmount of filesystem
4dea6050-7fd2-4823-a0c6-23a321b9f183

If that's what the commit tried to fix, then it wasn't not enough or
some change after it introduced a regression.

Thanks.

>
> 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
> >

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

[Boris Burkov]

On Thu, Jun 06, 2024 at 08:56:31PM +0100, Filipe Manana wrote:
> On Thu, Jun 6, 2024 at 8:06 PM Boris Burkov <boris@bur.io> wrote:
> >
> > 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.
> 
> I think it's obvious these calls are redundant.
> Things would be seriously broken if start_transaction() didn't call
> btrfs_record_root_in_trans().
> 

+1

> >
> > 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.
> 
> I tried that, yes, but the test sometimes fails qgroup cleanups with
> -ENOSPC, both before and after this patch.
> The failure is reported as a warning message in dmesg, it doesn't make
> the test case fail:
> 
> [99872.487855] run fstests generic/269 at 2024-06-06 20:53:33
> [99872.771632] BTRFS: device fsid 7a9e9120-5656-486a-8ea2-e0f7e12648b7
> devid 1 transid 10 /dev/sdc (8:32) scanned by mount (2754727)
> [99872.772502] BTRFS info (device sdc): first mount of filesystem
> 7a9e9120-5656-486a-8ea2-e0f7e12648b7
> [99872.772515] BTRFS info (device sdc): using crc32c (crc32c-intel)
> checksum algorithm
> [99872.772519] BTRFS info (device sdc): using free-space-tree
> [99872.774615] BTRFS info (device sdc): checking UUID tree
> [99877.835179] btrfs_drop_snapshot: 10 callbacks suppressed
> [99877.835191] BTRFS warning (device sdc): failed to cleanup qgroup 0/342: -28
> [99901.986789] BTRFS info (device sdc): last unmount of filesystem
> 7a9e9120-5656-486a-8ea2-e0f7e12648b7
> [99902.004561] BTRFS info (device sda): last unmount of filesystem
> 4dea6050-7fd2-4823-a0c6-23a321b9f183

No the patch fixed space reservation leaks that showed up as test
failures from the unmount reservation checking. I'll keep an eye out for
the ENOSPC errors, haven't seen that before.

> 
> If that's what the commit tried to fix, then it wasn't not enough or
> some change after it introduced a regression.
> 
> Thanks.
> 
> >
> > Thanks,
> > Boris
> >
> > >
> > > Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Boris Burkov <boris@bur.io>

> > > ---
> > >  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
> > >