| Message ID | a3879c9484eb245085f08fc90f94dbf027dbe22a.1706130791.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Error handling fixes | expand |
On 2024/1/25 07:48, David Sterba wrote: > We're deleting a root and looking it up by key does not succeed, this > is an inconsistent state and we can't do anything. All callers handle > errors and abort a transaction. > > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/root-tree.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c > index 603ad1459368..ba7e2181ff4e 100644 > --- a/fs/btrfs/root-tree.c > +++ b/fs/btrfs/root-tree.c > @@ -323,8 +323,11 @@ int btrfs_del_root(struct btrfs_trans_handle *trans, > ret = btrfs_search_slot(trans, root, key, path, -1, 1); > if (ret < 0) > goto out; > - > - BUG_ON(ret != 0); > + if (ret != 0) { > + /* The root must exist but we did not find it by the key. */ > + ret = -EUCLEAN; IIRC every EUCLEAN needs a message (at least that's the rule inside tree-checker). And the only two callers are also aborting the transaction, thus I believe we can just abort here. Thanks, Qu > + goto out; > + } > > ret = btrfs_del_item(trans, root, path); > out:
On Thu, Jan 25, 2024 at 02:31:01PM +1030, Qu Wenruo wrote: > > > On 2024/1/25 07:48, David Sterba wrote: > > We're deleting a root and looking it up by key does not succeed, this > > is an inconsistent state and we can't do anything. All callers handle > > errors and abort a transaction. > > > > Signed-off-by: David Sterba <dsterba@suse.com> > > --- > > fs/btrfs/root-tree.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c > > index 603ad1459368..ba7e2181ff4e 100644 > > --- a/fs/btrfs/root-tree.c > > +++ b/fs/btrfs/root-tree.c > > @@ -323,8 +323,11 @@ int btrfs_del_root(struct btrfs_trans_handle *trans, > > ret = btrfs_search_slot(trans, root, key, path, -1, 1); > > if (ret < 0) > > goto out; > > - > > - BUG_ON(ret != 0); > > + if (ret != 0) { > > + /* The root must exist but we did not find it by the key. */ > > + ret = -EUCLEAN; > > IIRC every EUCLEAN needs a message (at least that's the rule inside > tree-checker). In tree-checker yes, there are many reasons why reading/writing a block may fail. There the data are read for the first time and we could expect failures, so it's the first line of defence. Many other EUCLEAN errors are returned without any messages from deep call chains and are practically never to be seen, this is the last resort defence. > And the only two callers are also aborting the transaction, thus I > believe we can just abort here. As said in the other reply, it's up to the caller.
diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c index 603ad1459368..ba7e2181ff4e 100644 --- a/fs/btrfs/root-tree.c +++ b/fs/btrfs/root-tree.c @@ -323,8 +323,11 @@ int btrfs_del_root(struct btrfs_trans_handle *trans, ret = btrfs_search_slot(trans, root, key, path, -1, 1); if (ret < 0) goto out; - - BUG_ON(ret != 0); + if (ret != 0) { + /* The root must exist but we did not find it by the key. */ + ret = -EUCLEAN; + goto out; + } ret = btrfs_del_item(trans, root, path); out:
We're deleting a root and looking it up by key does not succeed, this is an inconsistent state and we can't do anything. All callers handle errors and abort a transaction. Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/root-tree.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)