Bug 1236649 - os-prober does not detect os on btrfs partition without any subvolume
Summary: os-prober does not detect os on btrfs partition without any subvolume
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: os-prober
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Hedayat Vatankhah
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-06-29 16:19 UTC by Helmut Horvath
Modified: 2016-03-02 08:23 UTC (History)
3 users (show)

Fixed In Version: os-prober-1.70-1.fc23 os-prober-1.70-1.fc22
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-11-17 11:58:09 UTC
Type: Bug


Attachments (Terms of Use)
patch (3.20 KB, patch)
2015-06-29 16:19 UTC, Helmut Horvath
no flags Details | Diff
new patch (3.37 KB, patch)
2015-06-29 20:18 UTC, Helmut Horvath
no flags Details | Diff
Working patch vs 1.68-1.fc23 (1.36 KB, patch)
2015-11-12 11:45 UTC, John
no flags Details | Diff
Better patch vs os-prober 1.68-1 (1.59 KB, patch)
2015-11-13 11:53 UTC, John
no flags Details | Diff

Description Helmut Horvath 2015-06-29 16:19:37 UTC
Created attachment 1044415 [details]
patch

Description of problem:

This bug is against a downstream patch of fedora: http://pkgs.fedoraproject.org/cgit/os-prober.git/tree/os-prober-btrfsfix.patch

As mentioned in the upstream bug report (https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=688336#35), the btrfs patch does miss on case for btrfs partition handling: when there is no subvolume.
However, the patch provided there, seems a bit chaotic to me (bad coding style) and does no make that much sense to me.
Furthermore it is buggy, as os-prober prints an os found on a btrfs partition without subvolume twice, when that partition is mounted somewhere.
So I have experimented and came up with a new patch, that works perfectly here and has none of the problems mentioned.
The diff, requires the os-prober-btrfsfix.patch patch to be applied before.


Additional info:

Also see: https://bugzilla.redhat.com/show_bug.cgi?id=1236358

Comment 1 Helmut Horvath 2015-06-29 18:53:18 UTC
Okay, thinking of it, I could imagine a case where this breaks (root in top level btrfs, but with subvolume in root filesystem), will test another day.

Comment 2 Helmut Horvath 2015-06-29 20:18:15 UTC
Created attachment 1044462 [details]
new patch

New version of the patch, sorry for the inconvenience.

Comment 3 Helmut Horvath 2015-06-29 20:22:16 UTC
See https://bugzilla.redhat.com/attachment.cgi?id=1044461&action=diff for easier review.

Comment 4 Jan Kurik 2015-07-15 13:53:38 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 23 development cycle.
Changing version to '23'.

(As we did not run this process for some time, it could affect also pre-Fedora 23 development
cycle bugs. We are very sorry. It will help us with cleanup during Fedora 23 End Of Life. Thank you.)

More information and reason for this action is here:
https://fedoraproject.org/wiki/BugZappers/HouseKeeping/Fedora23

Comment 5 John 2015-11-10 11:01:27 UTC
os-prober 1.68-1.fc22 fails to detect my debian and debian-testing installs using btrfs filesystem with no btrfs subvolume. 

OS-prober DOES detect my kubuntu install on sda3, which does use btrfs subvolumes.

I can mount the debian & deb-testing partitions easily, with default mount options:

/dev/sda6 on /mnt/debian type btrfs (rw,relatime,ssd,space_cache,subvolid=5,subvol=/)
/dev/sda7 on /mnt/deb-testing type btrfs (rw,relatime,ssd,space_cache,subvolid=5,subvol=/)

But all os-prober gives me is this:

os-prober
/dev/sda2:Linux Mint 17.2 Rafaela (17.2):LinuxMint:linux
/dev/sda3:Debian GNU/Linux (jessie/sid):Debian:linux:btrfs:UUID=7fdfa51b-2b8e-4ddf-bd5d-a1dca1a1b915:subvol=@
/dev/sdb1:Windows 7 (loader):Windows:chain
/dev/sdb2:openSUSE 13.1 (x86_64):SuSE:linux

Note: os-prober misidentifies the Kubuntu on /dev/sda3 as Debian (grub correctly identifies it as Kubuntu in the Adbanced options menu):

Note: os-prober gives no listing for debian or debian testing on sda6 & sda7


This needs to be fixed.

Comment 6 John 2015-11-11 13:23:34 UTC
You guys do realise the impact of this bug is that any poor bastard who installs fedora after installing Kubuntu or somseuch, has just lost access to their Kubuntu install (unless they have the skills to hack their grub config, whcih is an unreasonable expectation)?

Do you not realise how serious that is? 

How happy would you be, if Ubuntu didn't give a damn, and missed Fedora and RHEL when it installed grub?

How is it possible that this bug still exists on fedora 23? 

The os-prober used by Kubuntu obviously detects their subvolumes. How does it not work in the os-prober version used by F23?

Comment 7 John 2015-11-11 13:34:01 UTC
Sorry, some confusion in my last sarcastic comment. The Kubuntu does use btrfs subvolumes by default, it's the debian which doesn't, and which gets nuked by Fedora's grub/os-prober.

I just have to say that I am well sick of these issues, because I've just upgraded from fedora 21 to 22, to 23, and 23 is an abortion with the nvidia/xorg incompatibilities, and inability to probe my debian OSes.

And the irony is that in F21 which i was running up until a few days ago, i had the opposite issue - i'm moderately sure the os-prober/grub in F21 was finding my debian and debian-testing installs, and i could mount them. But i don't think i could mount the kubuntu OS with btrfs subvolumes in f21...

Now in f23 I can mount all my other os-volumes, and os-prober has magically re-discovered my kubuntu, but I get kicked in the nuts by this os-prober which has suddenly become unable to probe my debian installs.

So i'm going to have to boot into kubuntu to reinstall grub, in order to regain access to debian, and then i'm going to go back to using debian.


I'll give you threee guesses whether I'm going to bother using fedora much in the near future...

Comment 8 Helmut Horvath 2015-11-11 15:41:51 UTC
John, could you try my patch?
This would really help, if someone else than me would confirm it ... so the fedora devs could include the patch ...

It's not fedora's fault ... os-prober has bad btrfs support in any distribution ... Fedora's btrfs support is superior to the other distro's (acutally a few other distros also ship a patched os-prober like Opensuse ...), but the patch fedora is using for os-prober has one bug: it does not detect os installed on a simple btrfs partition without root being on a subvolume ...

Comment 9 Hedayat Vatankhah 2015-11-11 18:50:09 UTC
@Helmut: Thank you for your support despite the fact that I (unfortunately) was unable to consider & integrate your fix. I hope to be able to do it soon.

@John: You can easily try upstream os-prober (which you should already have in Debian) and see how it behaves before blaming me. We've patched os-prober heavily and sent many of the fixes to upstream, but except a few simple ones, other are still waiting for review in Debian Bugzilla. BTRFS support in Fedora is contributed from a Fedora contributor. What is used in OpenSUSE (and probably others) is based on Fedora patch. If you are unhappy with os-prober status (as I am), you can complain upstream for being too conservative. If you find any distribution to have a better os-prober, it is certainly because of their own patches.

Comment 10 John 2015-11-12 00:39:29 UTC
Sorry Hedayat, i didn't mean to imply any blame on you.

The way i see it, filesystem developers eg btrfs devs shouldn't implement features like these subvolumes, unless they also going to help fix the tools their new feature breaks, or depends on.

And if the filesystem devs haven't ensured the requisite tools are available, then the filesystem/features shouldn't be used.

Obviously installing several of my oses to to btrfs was a mistake, i expected too much.

Stable-type distros should be refusing to offer btrfs as an option for installing root filesystem - until btrfs devs have helped to properly fix these issues in os-prober.

Just discovered i don't have enough space to upgrade my kubuntu to latest release but i do have enough space to upgrade my debian and debian testing. 

So when i'm up-to-date I'll report back on how the latest os-prober goes in debian.


Helmut! I just realised your patch is to a script which makes things easy. I will patch it into my F23 os-prober later today and give it a test.

Comment 11 John 2015-11-12 11:43:01 UTC
Hi Helmut, Hedayat

Helmut's patch fails to apply against the current os-prober-1.68-1.fc23.x86_64

It looks like much of the original patch has already gone in at some stage, so i was able to juggle things a bit and find a simple patch that addresses the last issue - when btrfs has no subvolumes.

With the attached 50mounted-tests.vs.1.68-1.fc23.x86_64.patch applied, i can get my btrfs OSes with no subvols (ie debian) into grub at last.

I hope this can be applied upstream ASAP.

Comment 12 John 2015-11-12 11:45:20 UTC
Created attachment 1093187 [details]
Working patch vs 1.68-1.fc23

Comment 13 John 2015-11-12 12:59:28 UTC
Ah, i've just had a look at the link Helmut suggested in comment 3

"See https://bugzilla.redhat.com/attachment.cgi?id=1044461&action=diff for easier review."

That does make it easier to see what the patch was doing, and i can see my patch also misses the case where you have a btrfs with a subvolume, but the root is in the top of the btrfs (not in a subvolume). If that's possible.

Tomorrow night I'll have another go at sticking closer to Helmut's original patch and catching this case too.

Comment 14 John 2015-11-13 11:51:37 UTC
Here we go, I'm attaching a new patch to replace my first attempt. This one tests the top-level of the btrfs partition, and then continues to test the subvolumes if there are any. It also fixes a disembodied continue statement that shouldn't have been in earlier patches.

Note: the new patch of one file is all i need in order to get my btrfs debian volumes (with or without subvols) probed and into grub now.

It's been tested and works for:
 1) kubuntu style btrfs partitions (no os in the default volume, root in "@" subvolume, home in "@home" subvolume), 
 2) debian style (root in main volume, no subvolumes)
 3) "mongrel" style (root in main volume, plus extra subvolumes)

It might also work for
 4) "mongrel-bastard" style (crud in main volume, root in subvolume)
But that would depend more on whether the other scripts can cope with that, and I'm not even sure it's a bootable/feasible configuration.

Comment 15 John 2015-11-13 11:53:49 UTC
Created attachment 1093594 [details]
Better patch vs os-prober 1.68-1

Comment 16 Hedayat Vatankhah 2015-11-13 15:16:16 UTC
Thanks John. I'll try to review you and Helmut patches ASAP. Helmut: it'd be nice if you could also try John's patch.

Comment 17 Fedora Update System 2015-11-13 20:24:26 UTC
os-prober-1.70-1.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2015-77188a4636

Comment 18 Fedora Update System 2015-11-13 20:24:27 UTC
os-prober-1.70-1.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-067aae5bb6

Comment 19 Fedora Update System 2015-11-14 03:21:33 UTC
os-prober-1.70-1.fc22 has been pushed to the Fedora 22 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update os-prober'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-77188a4636

Comment 20 Fedora Update System 2015-11-14 04:52:39 UTC
os-prober-1.70-1.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update os-prober'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-067aae5bb6

Comment 21 Helmut Horvath 2015-11-15 07:56:17 UTC
John, from what I can see, your patch is identical to mine, or did you change something?
I do not have time to test exactly and diff your patch and mine but from quickly looking over my patch and yours, the only difference I notice, is that you do not indent the code in the new "else" statement, hence your patch looks shorter ...
I have not tested why my patch does not apply anymore, I do not have much time currently ... but when I do, I'll report back ...
Meanwhile it would be great if you could tell me what you changed, compared to my patch.

Comment 22 Hedayat Vatankhah 2015-11-15 15:47:15 UTC
Yes, I also found it quite the same. The most notable difference was removing a 'continue' in your patch. Otherwise I see your patches to be quite the same.

Actually, I would do things a little differently (e.g. refactoring the repeated code into a separate function or remove the duplication in another way), but for now I applied the patch almost unmodified. I'll do that later.

BTW, thank you both of you for reporting the bug & a fix. And sorry for the late review.

Comment 23 Helmut Horvath 2015-11-15 19:46:57 UTC
Yes, the continue statement it not needed outside the for loop, happened, because of copy and paste.
Regarding functions: Yes, would be cool.
Are you interested in changing the btrfsfix.patch a little bit?

Imagine the following situation: Your root is inside a subvol @. Now you take a snapshot (not readonly) of @ called @-backup.
os-prober as of now will list @-backup additionally, despite the fact, that booting that entry would simply boot @, because the fstab of @-backup, talks of booting @.

If you would be interested in resolving that issue, then I'll see if I can find the time to open a new bug and propose my patch (which I have tested locally for quite some time) there.

Comment 24 Hedayat Vatankhah 2015-11-16 11:00:02 UTC
Well, I'm not sure if os-prober should go that far, but it could be a nice addition. However, what if the backup boots the same / partition but different /home (or any other difference).

Comment 25 John 2015-11-16 12:32:25 UTC
(In reply to Hedayat Vatankhah from comment #22)
> Yes, I also found it quite the same. The most notable difference was
> removing a 'continue' in your patch. Otherwise I see your patches to be
> quite the same.

Yep that's pretty much all i did for the 50-mounted-tests, fix the continue in Helmut's patch - though I could've sworn that when i first looked at Helmuts patch file i thought it had some other additions that had already gone into the version i was comparing it too, 1.68. But I was pretty fatigued at the time, I might've got my wires crossed with some of Helmut's patches for the other os-prober scripts...

I'm just happy this last bit has gone in to 50-mounted-tests and I'll keep my boot entries for Debian now, when i update my fedora kernels :-)

Thanks guys.

Comment 26 Fedora Update System 2015-11-17 11:58:02 UTC
os-prober-1.70-1.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.

Comment 27 Fedora Update System 2016-03-02 08:23:27 UTC
os-prober-1.70-1.fc22 has been pushed to the Fedora 22 stable repository. If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.