Re: [PATCH v2] btrfs: repair super block num_devices automatically

[David Sterba <dsterba@suse.cz>]

On Mon, Feb 28, 2022 at 08:51:30PM +0800, Anand Jain wrote:
> >>> I didn't get your point.
> >>> What do you want to get from this patch?
> >>>
> >>> Isn't this already the behavior of this patch?
> >>
> >>   Let me clarify - we don't need this patch if we fix the actual bug as
> >> above. IMO.
> > 
> > Big NO NO.
> > 
> > The damage is already done, we must be responsible for whatever damage
> > we caused, especially the damage has already reached disk.
> > 
> > Just fixing the cause and call it a day is definitely not a responsible 
> > way.
> > 
> > Especially when the damage is done, you have no way to mount it, just
> > like the reporter.
> > 
> > You dare to say the same thing to the end user?
> 
> You have a btrfs-progs patch to recover from that situation. Right?
> Plus, I suppose you are sending a kernel patch for the actual bug
> which is causing this corruption. No?

Normally we'd have just the repair code in check, but this is sometimes
difficult to run eg. on a root filesystem, or if the boot fails because
some mount fails and ends up in the rescue environment with limited
options.

> This patch is the reporter side fix. I don't encourage fixing the
> reporter because a similar corruption might happen for reasons unknown
> yet. For example, raid1 split-brain? Which is not yet completely
> analyzed and test-cased yet.

So that's a valid question if the auto-repair should be done or not in
kernel. But an offline repair would do the same thing, read number of
device items and set the num_devices. The decision on kernel side at
mount or by user when running btrfs check has the same amount of
information, so forcing user to do offline repair is a bit less
convenient.

The num_devices is basically a cached value of the number of device
items, used in many places as a shortcut so we don't have to count them
each time. Once we make sure that the numbers are in sync, it's the
correct stat. We know about one scenario where it would get out of sync,
now fixed, so the simple auto repair is IMHO safe.

For the raid1 split brain, I can't tell and if you say it's not analyzed
properly we'd have to rely on other mechanisms to catch the
inconsistency. Eg. a missing physical device would be recognized and
would require degraded mount, but it's unrelated to the value of
num_devices.