| Message ID | fb12d07525d725188649f8dae90c0cfc8d31a0db.1735767974.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] btrfs: prefer to use "/dev/mapper/name" soft link instead of "/dev/dm-*" | expand |
On Wed, Jan 1, 2025 at 4:47 PM Qu Wenruo <wqu@suse.com> wrote: > > [BUG] > There is a gentoo bug report that upstream commit a5d74fa24752 > ("btrfs: avoid unnecessary device path update for the same device") breaks > 6.6 LTS kernel behavior where previously lsblk can properly show multiple > mount points of the same btrfs: > > With kernel 6.6.62: > NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINTS > sdb 8:16 0 9,1T 0 disk > `-sdb1 8:17 0 9,1T 0 part > `-hdd2 254:6 0 9,1T 0 crypt /mnt/hdd2 > /var/cache/distfiles > /var/cache/binpkgs > > But not with that upstream commit backported: > > With kernel 6.6.67: > sdb 8:16 0 9,1T 0 disk > `-sdb1 8:17 0 9,1T 0 part > `-hdd2 254:6 0 9,1T 0 crypt /mnt/hdd2 > > [CAUSE] > With that upstream patch backported to 6.6 LTS, the main change is in > the mount info result: > > Before: > /dev/mapper/hdd2 /mnt/hdd2 btrfs rw,relatime,space_cache=v2,subvolid=382,subvol=/hdd2 0 0 > /dev/mapper/hdd2 /mnt/dump btrfs rw,relatime,space_cache=v2,subvolid=393,subvol=/dump 0 0 > > After: > /dev/dm-1 /mnt/hdd2 btrfs rw,relatime,space_cache=v2,subvolid=382,subvol=/hdd2 0 0 > /dev/dm-1 /mnt/dump btrfs rw,relatime,space_cache=v2,subvolid=393,subvol=/dump 0 0 > > I believe that change of mount device is causing problems for lsblk. > > And before that patch, even if udev registered "/dev/dm-1" to btrfs, > later mount triggered a rescan and change the name back to > "/dev/mapper/hdd2" thus older kernels will work as expected. > > But after that patch, if udev registered "/dev/dm-1", then mount > triggered a rescan, but since btrfs detects both "/dev/dm-1" and > "/dev/mapper/hdd2" are pointing to the same block device, btrfs refuses > to do the rename, causing the less human readable "/dev/dm-1" to be > there forever, and trigger the bug for lsblk. > > Fortunately for newer kernels, I guess due to the migration to fsconfig > mount API, even if the end user explicitly mount the fs using > "/dev/dm-1", the mount will resolve the path to "/dev/mapper/hdd2" link > anyway. > > So for newer kernels it won't be a big deal but one extra safety net. > > [FIX] > Add an extra exception for is_same_device(), that if both paths are > pointing to the same device, but the newer path begins with "/dev/mapper" > and the older path is not, then still treat them as different devices, > so that we can rename to use the more human readable device path. > > Reported-by: David Duchesne <aether@disroot.org> > Link: https://bugs.gentoo.org/947126 > Fixes: a5d74fa24752 ("btrfs: avoid unnecessary device path update for the same device") > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > Changelog: > v2: > - Fix unnecessary rename where the filename is the same > The problem is in the exception handling that if both old and new path > matches, we will still do the rename. > Fix it by revert the exception check condition and setting is_same to true when > path_equal() is true. > > - Fix special chars in the commit message > --- > fs/btrfs/volumes.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index c8b079ad1dfa..5a0327e11807 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -832,8 +832,21 @@ static bool is_same_device(struct btrfs_device *device, const char *new_path) > ret = kern_path(new_path, LOOKUP_FOLLOW, &new); > if (ret) > goto out; > - if (path_equal(&old, &new)) > + if (path_equal(&old, &new)) { > is_same = true; > + /* > + * Special case for device mapper with its soft links. > + * > + * We can have the old path as "/dev/dm-3", but udev triggered > + * rescan on the soft link "/dev/mapper/test". > + * In that case we want to rename to that mapper link, to avoid > + * a bug in libblkid where it can not handle multiple mount > + * points if the device is "/dev/dm-3". > + */ > + if (strncmp(old_path, "/dev/mapper/", strlen("/dev/mapper")) && > + !strncmp(new_path, "/dev/mapper/", strlen("/dev/mapper"))) > + is_same = false; > + } > out: > kfree(old_path); > path_put(&old); > -- > 2.47.1 > > LGTM. Reviewed-by: Neal Gompa <neal@gompa.dev>
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index c8b079ad1dfa..5a0327e11807 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -832,8 +832,21 @@ static bool is_same_device(struct btrfs_device *device, const char *new_path) ret = kern_path(new_path, LOOKUP_FOLLOW, &new); if (ret) goto out; - if (path_equal(&old, &new)) + if (path_equal(&old, &new)) { is_same = true; + /* + * Special case for device mapper with its soft links. + * + * We can have the old path as "/dev/dm-3", but udev triggered + * rescan on the soft link "/dev/mapper/test". + * In that case we want to rename to that mapper link, to avoid + * a bug in libblkid where it can not handle multiple mount + * points if the device is "/dev/dm-3". + */ + if (strncmp(old_path, "/dev/mapper/", strlen("/dev/mapper")) && + !strncmp(new_path, "/dev/mapper/", strlen("/dev/mapper"))) + is_same = false; + } out: kfree(old_path); path_put(&old);
[BUG] There is a gentoo bug report that upstream commit a5d74fa24752 ("btrfs: avoid unnecessary device path update for the same device") breaks 6.6 LTS kernel behavior where previously lsblk can properly show multiple mount points of the same btrfs: With kernel 6.6.62: NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINTS sdb 8:16 0 9,1T 0 disk `-sdb1 8:17 0 9,1T 0 part `-hdd2 254:6 0 9,1T 0 crypt /mnt/hdd2 /var/cache/distfiles /var/cache/binpkgs But not with that upstream commit backported: With kernel 6.6.67: sdb 8:16 0 9,1T 0 disk `-sdb1 8:17 0 9,1T 0 part `-hdd2 254:6 0 9,1T 0 crypt /mnt/hdd2 [CAUSE] With that upstream patch backported to 6.6 LTS, the main change is in the mount info result: Before: /dev/mapper/hdd2 /mnt/hdd2 btrfs rw,relatime,space_cache=v2,subvolid=382,subvol=/hdd2 0 0 /dev/mapper/hdd2 /mnt/dump btrfs rw,relatime,space_cache=v2,subvolid=393,subvol=/dump 0 0 After: /dev/dm-1 /mnt/hdd2 btrfs rw,relatime,space_cache=v2,subvolid=382,subvol=/hdd2 0 0 /dev/dm-1 /mnt/dump btrfs rw,relatime,space_cache=v2,subvolid=393,subvol=/dump 0 0 I believe that change of mount device is causing problems for lsblk. And before that patch, even if udev registered "/dev/dm-1" to btrfs, later mount triggered a rescan and change the name back to "/dev/mapper/hdd2" thus older kernels will work as expected. But after that patch, if udev registered "/dev/dm-1", then mount triggered a rescan, but since btrfs detects both "/dev/dm-1" and "/dev/mapper/hdd2" are pointing to the same block device, btrfs refuses to do the rename, causing the less human readable "/dev/dm-1" to be there forever, and trigger the bug for lsblk. Fortunately for newer kernels, I guess due to the migration to fsconfig mount API, even if the end user explicitly mount the fs using "/dev/dm-1", the mount will resolve the path to "/dev/mapper/hdd2" link anyway. So for newer kernels it won't be a big deal but one extra safety net. [FIX] Add an extra exception for is_same_device(), that if both paths are pointing to the same device, but the newer path begins with "/dev/mapper" and the older path is not, then still treat them as different devices, so that we can rename to use the more human readable device path. Reported-by: David Duchesne <aether@disroot.org> Link: https://bugs.gentoo.org/947126 Fixes: a5d74fa24752 ("btrfs: avoid unnecessary device path update for the same device") Signed-off-by: Qu Wenruo <wqu@suse.com> --- Changelog: v2: - Fix unnecessary rename where the filename is the same The problem is in the exception handling that if both old and new path matches, we will still do the rename. Fix it by revert the exception check condition and setting is_same to true when path_equal() is true. - Fix special chars in the commit message --- fs/btrfs/volumes.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)