[PATCH 0/2] btrfs-progs: forget removed devices

[PATCH 0/2] btrfs-progs: forget removed devices

[Boris Burkov]

To fix bugs in multi-dev filesystems not handling a devt changing under
them when a device gets destroyed and re-created with a different devt,
we need to forget devices as they get removed.

Modify scan -u to take advantage of the kernel taking unvalidated block
dev names and modify udev to invoke this scan -u on device remove.

Boris Burkov (2):
  btrfs-progs: allow btrfs device scan -u on dead dev
  btrfs-progs: add udev rule to forget removed device

 64-btrfs-rm.rules | 7 +++++++
 Makefile          | 2 +-
 cmds/device.c     | 2 +-
 3 files changed, 9 insertions(+), 2 deletions(-)
 create mode 100644 64-btrfs-rm.rules

-- 
2.43.0

[PATCH 1/2] btrfs-progs: allow btrfs device scan -u on dead dev

[Boris Burkov]

If a device has been removed from the system, it lingers on in the btrfs
stale device cache. It is useful to be able to forget it, so allow that
by not checking for the file being a proper device file for the forget
path. Kernels that don't handle dead devices by name will do a devt
lookup which will fail, if there is no device, so this isn't a strictly
necessary check, so this isn't a strictly necessary check, so this isn't
a strictly necessary check.

Signed-off-by: Boris Burkov <boris@bur.io>
---
 cmds/device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmds/device.c b/cmds/device.c
index 4b34300b5..d52bde2e3 100644
--- a/cmds/device.c
+++ b/cmds/device.c
@@ -487,7 +487,7 @@ static int cmd_device_scan(const struct cmd_struct *cmd, int argc, char **argv)
 	for( i = devstart ; i < argc ; i++ ){
 		char *path;
 
-		if (path_is_block_device(argv[i]) != 1) {
+		if (!forget && path_is_block_device(argv[i]) != 1) {
 			error("not a block device: %s", argv[i]);
 			ret = 1;
 			goto out;
-- 
2.43.0

[PATCH 2/2] btrfs-progs: add udev rule to forget removed device

[Boris Burkov]

Now that btrfs supports forgetting devices that don't exist, we can add
a udev rule to take advantage of that. This avoids bad edge cases
with cached devices in multi-device filesystems without having to rescan
all the devices on every change.

Signed-of-by: Boris Burkov <boris@bur.io>
---
 64-btrfs-rm.rules | 7 +++++++
 Makefile          | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)
 create mode 100644 64-btrfs-rm.rules

diff --git a/64-btrfs-rm.rules b/64-btrfs-rm.rules
new file mode 100644
index 000000000..852155d28
--- /dev/null
+++ b/64-btrfs-rm.rules
@@ -0,0 +1,7 @@
+SUBSYSTEM!="block", GOTO="btrfs_rm_end"
+ACTION!="remove", GOTO="btrfs_rm_end"
+ENV{ID_FS_TYPE}!="btrfs", GOTO="btrfs_rm_end"
+
+RUN+="/usr/local/bin/btrfs device scan -u $devnode"
+
+LABEL="btrfs_rm_end"
diff --git a/Makefile b/Makefile
index 374f59b99..eaeed9baf 100644
--- a/Makefile
+++ b/Makefile
@@ -271,7 +271,7 @@ tune_objects = tune/main.o tune/seeding.o tune/change-uuid.o tune/change-metadat
 all_objects = $(objects) $(cmds_objects) $(libbtrfs_objects) $(convert_objects) \
 	      $(mkfs_objects) $(image_objects) $(tune_objects) $(libbtrfsutil_objects)
 
-udev_rules = 64-btrfs-dm.rules 64-btrfs-zoned.rules
+udev_rules = 64-btrfs-dm.rules 64-btrfs-zoned.rules 64-btrfs-rm.rules
 
 ifeq ("$(origin V)", "command line")
   BUILD_VERBOSE = $(V)
-- 
2.43.0

Re: [PATCH 2/2] btrfs-progs: add udev rule to forget removed device

[David Sterba]

On Thu, Feb 29, 2024 at 10:36:55AM -0800, Boris Burkov wrote:
> Now that btrfs supports forgetting devices that don't exist, we can add
> a udev rule to take advantage of that. This avoids bad edge cases
> with cached devices in multi-device filesystems without having to rescan
> all the devices on every change.
> 
> Signed-of-by: Boris Burkov <boris@bur.io>
> ---
>  64-btrfs-rm.rules | 7 +++++++
>  Makefile          | 2 +-
>  2 files changed, 8 insertions(+), 1 deletion(-)
>  create mode 100644 64-btrfs-rm.rules
> 
> diff --git a/64-btrfs-rm.rules b/64-btrfs-rm.rules
> new file mode 100644
> index 000000000..852155d28
> --- /dev/null
> +++ b/64-btrfs-rm.rules
> @@ -0,0 +1,7 @@

Please add a comment that explains when and why this udev rule should be
used.

> +SUBSYSTEM!="block", GOTO="btrfs_rm_end"
> +ACTION!="remove", GOTO="btrfs_rm_end"
> +ENV{ID_FS_TYPE}!="btrfs", GOTO="btrfs_rm_end"
> +
> +RUN+="/usr/local/bin/btrfs device scan -u $devnode"

Is the full path mandatory or is 'btrfs' sufficient? I think systemd
uses own tool of the same name.

Please use long option name so it's more obvious what it does.

Re: [PATCH 2/2] btrfs-progs: add udev rule to forget removed device

[Boris Burkov]

On Thu, Feb 29, 2024 at 08:53:39PM +0100, David Sterba wrote:
> On Thu, Feb 29, 2024 at 10:36:55AM -0800, Boris Burkov wrote:
> > Now that btrfs supports forgetting devices that don't exist, we can add
> > a udev rule to take advantage of that. This avoids bad edge cases
> > with cached devices in multi-device filesystems without having to rescan
> > all the devices on every change.
> > 
> > Signed-of-by: Boris Burkov <boris@bur.io>
> > ---
> >  64-btrfs-rm.rules | 7 +++++++
> >  Makefile          | 2 +-
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> >  create mode 100644 64-btrfs-rm.rules
> > 
> > diff --git a/64-btrfs-rm.rules b/64-btrfs-rm.rules
> > new file mode 100644
> > index 000000000..852155d28
> > --- /dev/null
> > +++ b/64-btrfs-rm.rules
> > @@ -0,0 +1,7 @@
> 
> Please add a comment that explains when and why this udev rule should be
> used.
> 

Definitely happy to add a comment.

This is certainly the discussion I was hoping to have, as well, but I
thiiink we just always want this? Basically if we don't have it,
multi-device users are in danger of accidentally making a stale device
cache between mounts. It's probably not that big of a risk in general,
but we did hit an easier to hit variant in v5.19 at Meta.

OTOH, there is also the problem that this is a no-op unless the kernel
has the patch I sent at the same time:
btrfs: support device name lookup in forget

I don't think there is any downside to running this command which will
simply fail on an older kernel.

If this becomes ubiquitous, then we can also remove the special case for
single device cache clearing from the btrfs unmount code.

> > +SUBSYSTEM!="block", GOTO="btrfs_rm_end"
> > +ACTION!="remove", GOTO="btrfs_rm_end"
> > +ENV{ID_FS_TYPE}!="btrfs", GOTO="btrfs_rm_end"
> > +
> > +RUN+="/usr/local/bin/btrfs device scan -u $devnode"
> 
> Is the full path mandatory or is 'btrfs' sufficient? I think systemd
> uses own tool of the same name.

Unfortunately, it did not work for me. I saw logs saying
/usr/lib/udev/rules.d/btrfs file not found or something like that in
dmesg.

I also considered the btrfs udev "builtin" but from experimenting and
checking out the code, it looks like that only does device ready, not
all device commands.

> 
> Please use long option name so it's more obvious what it does.

Re: [PATCH 0/2] btrfs-progs: forget removed devices

[Anand Jain]

On 3/1/24 00:06, Boris Burkov wrote:
> To fix bugs in multi-dev filesystems not handling a devt changing under
> them when a device gets destroyed and re-created with a different devt,
> we need to forget devices as they get removed.
> 
> Modify scan -u to take advantage of the kernel taking unvalidated block
> dev names and modify udev to invoke this scan -u on device remove.
> 

Unless we have a specific bug still present after the patch
"[PATCH] btrfs: validate device maj:min during open," can we
hold off on using the external udev trigger to clean up stale
devices in the btrfs kernel?

IMO, these loopholes have to be fixed in the kernel regardless.

Thanks, Anand

> Boris Burkov (2):
>    btrfs-progs: allow btrfs device scan -u on dead dev
>    btrfs-progs: add udev rule to forget removed device
> 
>   64-btrfs-rm.rules | 7 +++++++
>   Makefile          | 2 +-
>   cmds/device.c     | 2 +-
>   3 files changed, 9 insertions(+), 2 deletions(-)
>   create mode 100644 64-btrfs-rm.rules
> 

Re: [PATCH 0/2] btrfs-progs: forget removed devices

[David Sterba]

On Fri, Mar 01, 2024 at 08:01:44AM +0530, Anand Jain wrote:
> On 3/1/24 00:06, Boris Burkov wrote:
> > To fix bugs in multi-dev filesystems not handling a devt changing under
> > them when a device gets destroyed and re-created with a different devt,
> > we need to forget devices as they get removed.
> > 
> > Modify scan -u to take advantage of the kernel taking unvalidated block
> > dev names and modify udev to invoke this scan -u on device remove.
> > 
> 
> Unless we have a specific bug still present after the patch
> "[PATCH] btrfs: validate device maj:min during open," can we
> hold off on using the external udev trigger to clean up stale
> devices in the btrfs kernel?
> 
> IMO, these loopholes have to be fixed in the kernel regardless.

Agreed, spreading the solution to user space and depending on async
events would only make things harder to debug in the end.

Re: [PATCH 0/2] btrfs-progs: forget removed devices

[Boris Burkov]

On Fri, Mar 01, 2024 at 12:54:42PM +0100, David Sterba wrote:
> On Fri, Mar 01, 2024 at 08:01:44AM +0530, Anand Jain wrote:
> > On 3/1/24 00:06, Boris Burkov wrote:
> > > To fix bugs in multi-dev filesystems not handling a devt changing under
> > > them when a device gets destroyed and re-created with a different devt,
> > > we need to forget devices as they get removed.
> > > 
> > > Modify scan -u to take advantage of the kernel taking unvalidated block
> > > dev names and modify udev to invoke this scan -u on device remove.
> > > 
> > 
> > Unless we have a specific bug still present after the patch
> > "[PATCH] btrfs: validate device maj:min during open," can we
> > hold off on using the external udev trigger to clean up stale
> > devices in the btrfs kernel?
> > 
> > IMO, these loopholes have to be fixed in the kernel regardless.
> 
> Agreed, spreading the solution to user space and depending on async
> events would only make things harder to debug in the end.

In my opinion, leaving the incorrect cache around after a device is
destroyed is a footgun and a recipe for future error. Patching it up
with Anand's kernel-only fix will fix the problem for now, but I would
not be at all surprised if there isn't a different more serious problem
waiting for us down the road. We're just looking at exactly devt and
temp_fsid, and I'm quite focused on breaking single device filesystems.
I spent a week or two hacking on that reproducer. Are we *confident* that
I couldn't do it again if I tried with all kinds of raid arrays, error
injection, races, etc? What about after we add three more features to
volume management and it's been two years?

Getting a notification on device destruction and removing our state for
that device is the correct fix. If things like this need to be
in-kernel, then why is udev the only mechanism for this kind of
communication? Can we get a callback from the block layer?

Boris

Re: [PATCH 0/2] btrfs-progs: forget removed devices

[David Sterba]

On Fri, Mar 01, 2024 at 07:44:44AM -0800, Boris Burkov wrote:
> On Fri, Mar 01, 2024 at 12:54:42PM +0100, David Sterba wrote:
> > On Fri, Mar 01, 2024 at 08:01:44AM +0530, Anand Jain wrote:
> > > On 3/1/24 00:06, Boris Burkov wrote:
> > > > To fix bugs in multi-dev filesystems not handling a devt changing under
> > > > them when a device gets destroyed and re-created with a different devt,
> > > > we need to forget devices as they get removed.
> > > > 
> > > > Modify scan -u to take advantage of the kernel taking unvalidated block
> > > > dev names and modify udev to invoke this scan -u on device remove.
> > > > 
> > > 
> > > Unless we have a specific bug still present after the patch
> > > "[PATCH] btrfs: validate device maj:min during open," can we
> > > hold off on using the external udev trigger to clean up stale
> > > devices in the btrfs kernel?
> > > 
> > > IMO, these loopholes have to be fixed in the kernel regardless.
> > 
> > Agreed, spreading the solution to user space and depending on async
> > events would only make things harder to debug in the end.
> 
> In my opinion, leaving the incorrect cache around after a device is
> destroyed is a footgun and a recipe for future error.

I agree here partially, leaving stale cached entries can lead to bad
outcome, but including userspace as part of the solution is again making
it fragile and less reliable. We're not targeting the default and common
use case, that would work with or without the fixes or udev rules.

The udev event is inherently racy so if something tries to mount before
the umount udev rule starts, no change other than we now have yet
another factor to take into account when debugging.

> Patching it up
> with Anand's kernel-only fix will fix the problem for now,

This is a general stance kernel vs user space, we can't rely on state
correctnes on userspace, so kernel can get hints (like device scanning)
but refuse to mount if the devices are not present.

> but I would
> not be at all surprised if there isn't a different more serious problem
> waiting for us down the road. We're just looking at exactly devt and
> temp_fsid, and I'm quite focused on breaking single device filesystems.
> I spent a week or two hacking on that reproducer. Are we *confident* that
> I couldn't do it again if I tried with all kinds of raid arrays, error
> injection, races, etc? What about after we add three more features to
> volume management and it's been two years?

We can only add more safety checks or enforcing some feature using flags
or options. Adding features to a layer means we need to revisit how it
would go with the current state, something that we did with the
temp-fsid, now we're chasing out the bugs. We did not have a good test
coverage initially so it's more than what could be expected for a
post-release validation.

> Getting a notification on device destruction and removing our state for
> that device is the correct fix. If things like this need to be
> in-kernel, then why is udev the only mechanism for this kind of
> communication? Can we get a callback from the block layer?

I don't know, it sounds simple but such things can get tricky to get
right once it's spanning more layers and state changes. I've heared too
many horror stories from CPU or memory hotplug, the unexpected changes
in block devices sound too similar.

The approach we have is to store some state and validate it when needed,
but we apparently lack enough information sources to make the validation
100% reliable. I think we gained a lot of knowledge just from debugging
that and prototyping the counter examples but I'm observing we may not
have means for fixes.

Re: [PATCH 0/2] btrfs-progs: forget removed devices

[Boris Burkov]

On Mon, Mar 04, 2024 at 07:07:36PM +0100, David Sterba wrote:
> On Fri, Mar 01, 2024 at 07:44:44AM -0800, Boris Burkov wrote:
> > On Fri, Mar 01, 2024 at 12:54:42PM +0100, David Sterba wrote:
> > > On Fri, Mar 01, 2024 at 08:01:44AM +0530, Anand Jain wrote:
> > > > On 3/1/24 00:06, Boris Burkov wrote:
> > > > > To fix bugs in multi-dev filesystems not handling a devt changing under
> > > > > them when a device gets destroyed and re-created with a different devt,
> > > > > we need to forget devices as they get removed.
> > > > > 
> > > > > Modify scan -u to take advantage of the kernel taking unvalidated block
> > > > > dev names and modify udev to invoke this scan -u on device remove.
> > > > > 
> > > > 
> > > > Unless we have a specific bug still present after the patch
> > > > "[PATCH] btrfs: validate device maj:min during open," can we
> > > > hold off on using the external udev trigger to clean up stale
> > > > devices in the btrfs kernel?
> > > > 
> > > > IMO, these loopholes have to be fixed in the kernel regardless.
> > > 
> > > Agreed, spreading the solution to user space and depending on async
> > > events would only make things harder to debug in the end.
> > 
> > In my opinion, leaving the incorrect cache around after a device is
> > destroyed is a footgun and a recipe for future error.
> 
> I agree here partially, leaving stale cached entries can lead to bad
> outcome, but including userspace as part of the solution is again making
> it fragile and less reliable. We're not targeting the default and common
> use case, that would work with or without the fixes or udev rules.
> 
> The udev event is inherently racy so if something tries to mount before
> the umount udev rule starts, no change other than we now have yet
> another factor to take into account when debugging.

That's a good point. I do agree that this is a race we need a solution
for, and is a good argument for an in-kernel fix.

> 
> > Patching it up
> > with Anand's kernel-only fix will fix the problem for now,
> 
> This is a general stance kernel vs user space, we can't rely on state
> correctnes on userspace, so kernel can get hints (like device scanning)
> but refuse to mount if the devices are not present.
> 
> > but I would
> > not be at all surprised if there isn't a different more serious problem
> > waiting for us down the road. We're just looking at exactly devt and
> > temp_fsid, and I'm quite focused on breaking single device filesystems.
> > I spent a week or two hacking on that reproducer. Are we *confident* that
> > I couldn't do it again if I tried with all kinds of raid arrays, error
> > injection, races, etc? What about after we add three more features to
> > volume management and it's been two years?
> 
> We can only add more safety checks or enforcing some feature using flags
> or options. Adding features to a layer means we need to revisit how it
> would go with the current state, something that we did with the
> temp-fsid, now we're chasing out the bugs. We did not have a good test
> coverage initially so it's more than what could be expected for a
> post-release validation.

I think it's fine for new features to hit unexpected bugs, that's pretty
much normal. All I'm saying is that  putting bandaids on bandaids is what
leads to a much higher likelihood of bugs any time something changes.

> 
> > Getting a notification on device destruction and removing our state for
> > that device is the correct fix. If things like this need to be
> > in-kernel, then why is udev the only mechanism for this kind of
> > communication? Can we get a callback from the block layer?
> 
> I don't know, it sounds simple but such things can get tricky to get
> right once it's spanning more layers and state changes. I've heared too
> many horror stories from CPU or memory hotplug, the unexpected changes
> in block devices sound too similar.
> 
> The approach we have is to store some state and validate it when needed,
> but we apparently lack enough information sources to make the validation
> 100% reliable. I think we gained a lot of knowledge just from debugging
> that and prototyping the counter examples but I'm observing we may not
> have means for fixes.

Agreed with everything you've said. I naively feel like a notification
model can work, but it does have a kind of spooky air to it for the
reasons you gave.

An idea that popped out of our group at Meta discussing this bug was to
use udev to manage state used by a btrfs specific version of mount. That
btrfs-mount would handle all the "scan before mount" considerations we
currently have. 

Ideally, it means we can get rid of the in-kernel cache entirely and so
we aren't afraid of races with userspace. Food for thought, at least.

Since a solution like that would be a pretty serious undertaking, I
think we probably do need a more pragmatic short term fix like Anand's.
What about refactoring it a bit to just trust the cache a lot less. It
would essentially be the same as Anand's patch, just without special
cases for mismatches. Basically, something like "create a new btrfs_device
no matter what"