On Wed, Feb 07, 2024 at 10:44:22AM +0530, Anand Jain wrote:
>
>
> On 2/5/24 23:13, David Sterba wrote:
> > There are reports that since version 6.7 update-grub fails to find the
> > device of the root on systems without initrd and on a single device.
> >
> > This looks like the device name changed in the output of
> > /proc/self/mountinfo:
> >
> > 6.5-rc5 working
> >
> > 18 1 0:16 / / rw,noatime - btrfs /dev/sda8 ...
> >
> > 6.7 not working:
> >
> > 17 1 0:15 / / rw,noatime - btrfs /dev/root ...
> >
> > and "update-grub" shows this error:
> >
> > /usr/sbin/grub-probe: error: cannot find a device for / (is /dev mounted?)
> >
> > This looks like it's related to the device name, but grub-probe
> > recognizes the "/dev/root" path and tries to find the underlying device.
> > However there's a special case for some filesystems, for btrfs in
> > particular.
> >
> > The generic root device detection heuristic is not done and it all
> > relies on reading the device infos by a btrfs specific ioctl. This ioctl
> > returns the device name as it was saved at the time of device scan (in
> > this case it's /dev/root).
> >
> > The change in 6.7 for temp_fsid to allow several single device
> > filesystem to exist with the same fsid (and transparently generate a new
> > UUID at mount time) was to skip caching/registering such devices.
> >
>
> > This also skipped mounted device.
>
> It's reasonable. However is there any logs from the debug message to
> confirm?
>
>
> > One step of scanning is to check if
> > the device name hasn't changed, and if yes then update the cached value.
>
> >
> > This broke the grub-probe as it always read the device /dev/root and
> > couldn't find it in the system. A temporary workaround is to create a
> > symlink but this does not survive reboot.
> >
>
> > The right fix is to allow updating the device path of a mounted
> > filesystem even if this is a single device one.
>
> And at the same time, if the device is not mounted, it still does
> not register the single device. As in the original design.
>
> > This does not affect the
> > temp_fsid feature, the UUID of the mounted filesystem remains the same
> > and the matching is based on device major:minor which is unique per
> > mounted filesystem.
> >
>
> It looks like the logic in find_fsid_by_device() does not verify if the
> %fsid_fs_devices and %devt_fs_devices are the same. I am concerned that
> if they aren't, and %devt_fs_devices is matched by devt only, it implies
> that the rest of the populated values in fs_devices may be inconsistent
> (from the super).
And this actually a problem, a lot of fstests fail with
messages like:
btrfs/004 [22:53:33][ 346.883630] run fstests btrfs/004 at 2024-02-06 22:53:34
[ 347.513164] BTRFS: device fsid e161b391-e959-419b-b6a6-9c1a371a6c15 devid 1 transid 11 /dev/vda scanned by mount (30014)
[ 347.515665] BTRFS info (device vda): first mount of filesystem e161b391-e959-419b-b6a6-9c1a371a6c15
[ 347.516597] BTRFS info (device vda): using crc32c (crc32c-generic) checksum algorithm
[ 347.517716] BTRFS info (device vda): using free-space-tree
[ 347.996843] BTRFS: device fsid d9f25a27-92c4-4ff3-a2da-634546711e68 devid 1 transid 6 /dev/vdb skip registration scanned by mkfs.btrfs (30078)
[ 348.009943] BTRFS: device fsid d9f25a27-92c4-4ff3-a2da-634546711e68 devid 1 transid 6 /dev/vdb scanned by mount (30082)
[ 348.013764] BTRFS info (device vdb): first mount of filesystem d9f25a27-92c4-4ff3-a2da-634546711e68
[ 348.014943] BTRFS info (device vdb): using crc32c (crc32c-generic) checksum algorithm
[ 348.015949] BTRFS error (device vdb): superblock fsid doesn't match fsid of fs_devices: d9f25a27-92c4-4ff3-a2da-634546711e68 != 3fb9009e-b1d5-46ac-ba94-8120384610e4
[ 348.017653] BTRFS error (device vdb): superblock metadata_uuid doesn't match metadata uuid of fs_devices: d9f25a27-92c4-4ff3-a2da-634546711e68 != 3fb9009e-b1d5-46ac-ba94-8120384610e4
this
[ 348.019590] BTRFS error (device vdb): dev_item UUID does not match metadata fsid: 3fb9009e-b1d5-46ac-ba94-8120384610e4 != d9f25a27-92c4-4ff3-a2da-634546711e68
[ 348.021313] BTRFS error (device vdb): superblock contains fatal errors
[ 348.022263] BTRFS error (device vdb): open_ctree failed
[failed, exit status 1][ 348.058344] BTRFS info (device vda): last unmount of filesystem e161b391-e959-419b-b6a6-9c1a371a6c15
and there are messages about using temp-fsid feature but that's not what
I'd expect, apparently the uuids get totally mixed up:
btrfs/002 [22:52:24][ 276.908253] run fstests btrfs/002 at 2024-02-06 22:52:24
[ 277.531910] BTRFS: device fsid e161b391-e959-419b-b6a6-9c1a371a6c15 devid 1 transid 9 /dev/vda scanned by mount (15065)
[ 277.534162] BTRFS info (device vda): first mount of filesystem e161b391-e959-419b-b6a6-9c1a371a6c15
[ 277.535108] BTRFS info (device vda): using crc32c (crc32c-generic) checksum algorithm
[ 277.535913] BTRFS info (device vda): using free-space-tree
[ 277.782925] BTRFS: device fsid c69cb0c6-6ca1-4e07-9e4e-c5b5cc09b2c9 devid 1 transid 6 /dev/vdb skip registration scanned by mkfs.btrfs (15109)
[ 277.824885] BTRFS: device /dev/vdb using temp-fsid d1299d0e-92d4-47a5-b6e7-5f998f86c5f0
[ 277.827699] BTRFS: device fsid c69cb0c6-6ca1-4e07-9e4e-c5b5cc09b2c9 devid 1 transid 6 /dev/vdb scanned by mount (15117)
[ 277.830983] BTRFS info (device vdb): first mount of filesystem c69cb0c6-6ca1-4e07-9e4e-c5b5cc09b2c9
[ 277.832252] BTRFS info (device vdb): using crc32c (crc32c-generic) checksum algorithm
[ 277.833325] BTRFS info (device vdb): using free-space-tree
[ 277.847045] BTRFS info (device vdb): checking UUID tree
[ 294.727566] grep (19725) used greatest stack depth: 24816 bytes left
[ 294.788244] dd (19697) used greatest stack depth: 24544 bytes left
[ 295.631873] dd (19752) used greatest stack depth: 24328 bytes left
[ 317.755256] BTRFS info (device vdb): last unmount of filesystem d1299d0e-92d4-47a5-b6e7-5f998f86c5f0
[ 317.820227] BTRFS info (device vda): last unmount of filesystem e161b391-e959-419b-b6a6-9c1a371a6c15
> I had a bunch of test cases for temp-fsid, I need to (find them) and
> send it out.
>
> > As the main part of device scanning and list update is done in
> > device_list_add() that handles all corner cases and locking, it is
> > extended to take a parameter that tells it to do everything as before,
> > except adding a new device entry.
>
> Hm. %new_device_added was for btrfs_scan_one_device() so that it can
> call btrfs_free_stale_devices(), removing any stale devices if present.
> However, theoretically, there shouldn't be any stale devices. Let's keep
> it for now, as the find_fsid_by_device() logic might return incorrect
> fs_devices (I need to confirm this again).
Ok, that's a reasonable idea.
> > This covers the path when the device (that exists for all mounted
> > devices) name changes, updating /dev/root to /dev/sdx. Any other single
> > device with filesystem is skipped.
> >
> > Note that if a system is booted and initial mount is done on the
> > /dev/root device, this will be the cached name of the device. Only after
> > the command "btrfs device rescan" it will change as it triggers the
>
> "btrfs device scan"
>
> > rename.
> >
> > The fix was verified by users whose systems were affected.
> >
> > CC: stable@xxxxxxxxxxxxxxx # 6.7+
> > Fixes: bc27d6f0aa0e ("btrfs: scan but don't register device on single device filesystem")
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=218353
> > Link: https://lore.kernel.org/lkml/CAKLYgeJ1tUuqLcsquwuFqjDXPSJpEiokrWK2gisPKDZLs8Y2TQ@xxxxxxxxxxxxxx/
> > Signed-off-by: David Sterba <dsterba@xxxxxxxx>
> > ---
> > fs/btrfs/volumes.c | 30 ++++++++++++++----------------
> > 1 file changed, 14 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index 474ab7ed65ea..f2c2f7ca5c3d 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -738,6 +738,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
> > bool same_fsid_diff_dev = false;
> > bool has_metadata_uuid = (btrfs_super_incompat_flags(disk_super) &
> > BTRFS_FEATURE_INCOMPAT_METADATA_UUID);
>
>
> > + bool can_create_new = *new_device_added;
>
>
> >
> > if (btrfs_super_flags(disk_super) & BTRFS_SUPER_FLAG_CHANGING_FSID_V2) {
> > btrfs_err(NULL,
> > @@ -753,6 +754,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
> > return ERR_PTR(error);
> > }
> >
>
>
> > + *new_device_added = false;
>
>
>
> > fs_devices = find_fsid_by_device(disk_super, path_devt, &same_fsid_diff_dev);
> >
> > if (!fs_devices) {
> > @@ -804,6 +806,15 @@ static noinline struct btrfs_device *device_list_add(const char *path,
> > return ERR_PTR(-EBUSY);
> > }
> >
> > + if (!can_create_new) {
>
>
>
> > + pr_info(
> > + "BTRFS: device fsid %pU devid %llu transid %llu %s skip registration scanned by %s (%d)\n",
> > + disk_super->fsid, devid, found_transid, path,
> > + current->comm, task_pid_nr(current));
> > + mutex_unlock(&fs_devices->device_list_mutex);
> > + return NULL;
> > + }
> > +
>
>
> I just ran the temp-fsid test cases attached here. Comparing the output
> with and without this patch seems to be breaking something in the
> temp-fsid feature. I need to further verify. I will convert this test
> to fstests. However, in the meantime, it can be used to verify.
Yeah, at this state the patch is broken.
