Re: [PATCH 04/20] btrfs: handle root deletion lookup error in btrfs_del_root()

[David Sterba <dsterba@suse.cz>]

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.