| Message ID | 292b2e90e9a64cd3156b0545f6e62b253ea17b9e.1717662443.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: remove pointless code when creating and deleting a subvolume | expand |
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 >
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 > >
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 > > >
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;