| Message ID | 0d0347a460b26e36966f95604ca8c69b956f1c62.1709814676.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: remove pointless BUG_ON() when creating snapshot | expand |
在 2024/3/8 02:31, fdmanana@kernel.org 写道: > From: Filipe Manana <fdmanana@suse.com> > > When creating a snapshot we first check with btrfs_lookup_dir_item() if > there is a name collision in the parent directory and then return an error > if there's a collision. Then later on when trying to insert a dir item for > the snapshot we BUG_ON() if the return value is -EEXIST or -EOVERFLOW: > > static noinline int create_pending_snapshot(...) > { > (...) > > /* check if there is a file/dir which has the same name. */ > dir_item = btrfs_lookup_dir_item(...); > (...) > > ret = btrfs_insert_dir_item(...); > /* We have check then name at the beginning, so it is impossible. */ > BUG_ON(ret == -EEXIST || ret == -EOVERFLOW); > if (ret) { > btrfs_abort_transaction(trans, ret); > goto fail; > } > > (...) > } > > It's impossible to get the -EEXIST because we previously checked for a > potential collision with btrfs_lookup_dir_item() and we know that after > that no one could have added a colliding name because at this point the > transaction is in its critical section, state TRANS_STATE_COMMIT_DOING, > so no one can join this transaction to add a colliding name and neither > can anyone start a new transaction to do that. > > As for the -EOVERFLOW, that can't happen as long as we have the extended > references feature enabled, which is a mkfs default for many years now. > > In either case, the BUG_ON() is excessive as we can properly deal with > any error and can abort the transaction and jump to the 'fail' label, > in which case we'll also get the useful stack trace (just like a BUG_ON()) > from the abort if the error is either -EEXIST or -EOVERFLOW. > > So remove the BUG_ON(). > > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/transaction.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index 46e8426adf4f..5b6536c1f6d1 100644 > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -1864,8 +1864,6 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, > ret = btrfs_insert_dir_item(trans, &fname.disk_name, > BTRFS_I(parent_inode), &key, BTRFS_FT_DIR, > index); > - /* We have check then name at the beginning, so it is impossible. */ > - BUG_ON(ret == -EEXIST || ret == -EOVERFLOW); > if (ret) { > btrfs_abort_transaction(trans, ret); > goto fail;
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index 46e8426adf4f..5b6536c1f6d1 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -1864,8 +1864,6 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, ret = btrfs_insert_dir_item(trans, &fname.disk_name, BTRFS_I(parent_inode), &key, BTRFS_FT_DIR, index); - /* We have check then name at the beginning, so it is impossible. */ - BUG_ON(ret == -EEXIST || ret == -EOVERFLOW); if (ret) { btrfs_abort_transaction(trans, ret); goto fail;