mountpoint-util: Deal with kernel API breakage in "norecovery" mount option #32892
Conversation
github-actions
bot
added
util-lib
tests
please-review
|
Note We had successfully released a new major release. We are no longer in a development freeze phase. |
…option "norecovery" was deprecated for btrfs in torvalds/linux@74ef001 and removed in torvalds/linux@a1912f7. Let's drop our assumption that btrfs supports "norecovery" and first query for the new name of the option followed by querying for the old name.
bluca
added
btrfs
good-to-merge/waiting-for-ci 👍
yuwata
added
good-to-merge/with-minor-suggestions
and removed
good-to-merge/waiting-for-ci 👍
|
You can call it API breakage as you want but note that it's been deprecated for 3 years (in 5.11). Nobody asked for keeping the option so it got removed. |
Sorry, but expecting every one to closely follow the LKML at all times to monitor userspace API breakages is really not realistic. These days Github's code search works great, and can easily show whether an API is actually used or not, so it would be best if proponents of backward-incompatible changes actively tried and look for affected userspace components, rather than expecting everyone to notice by themselves - realistically, that only happens too late, once the breakage is done, as we have seen now. |
|
@kdave How were we supposed to know the option was even deprecated or removed? A warning in dmesg is not sufficient to make people aware this is happening. A very simple search on github from your end before removing the option would also have avoided this issue. You can't assume userspace developers are searching dmesg for warnings about deprecations. There is not even a kernel cmdline option we could have enabled to notice this deprecation before the option was actually removed. |
yuwata
added
good-to-merge/waiting-for-ci 👍
|
Are you sure this even works? Iirc btrfs only very recently started to support the new mount api, and hence the nount option detection logic that builds on that doesnt work at all, it will always say supported. I am pretty sure this change causes some serious borkage on old kernels? |
|
I figure one has to check for some mount option first that definitely doesnt exist. If that test succeeds nonetheless we are on a kernel where btrfs wasnt taught the new mount api yet, and hence assume norecover is the way to got. What a clusterfuck. I wish kernel folks had any sense of "we dont break userspace" actually means. |
|
Ah our helper already does exactly that. Ignore me then. |
keszybz
removed
the
good-to-merge/waiting-for-ci 👍
|
So actually no, this isnt right quite yet. I am pretty sure that if support for the new mount api in btrfs is not available we should assume "norecovery" exists. With this PR we assume it doeasnt however. Hence, please make sure that if we cannot determine if btrfs supports norecovery or not (EAGAIN) we assume it does. |
|
I posted this on linux-btrfs ML btw: https://lore.kernel.org/linux-btrfs/ZkxZT0J-z0GYvfy8@gardel-login/T/#u I do hope they will revert this change upstream. But let's see. |
…rted by btrfs Fixup for e3828d7, as requested in systemd#32892 (comment).
→ #32948 |
Although 'norecovery' mount option is marked deprecated for a long time and a warning message is introduced during the deprecation window, it's still actively utilized by several projects that need a safely way to mount a btrfs without any writes. Furthermore this 'norecovery' mount option is supported by most major filesystems, which makes it harder to validate our motivation. This patch would re-introduce the 'norecovery' mount option, and output a message to recommend 'rescue=nologreplay' option. Link: https://lore.kernel.org/linux-btrfs/ZkxZT0J-z0GYvfy8@gardel-login/#t Link: systemd/systemd#32892 Link: https://bugzilla.suse.com/show_bug.cgi?id=1222429 Reported-by: Lennart Poettering <lennart@poettering.net> Reported-by: Jiri Slaby <jslaby@suse.com> Fixes: a1912f7 ("btrfs: remove code for inode_cache and recovery mount options") Cc: stable@vger.kernel.org # 6.8+ Signed-off-by: Qu Wenruo <wqu@suse.com>
|
Seems we'll get "norecovery" back: https://lore.kernel.org/linux-btrfs/57c63f57-fde4-4bd6-a556-c476e122a547@wdc.com/T/#t |
|
That's great, thanks for reporting that - I assume we want to keep this machinery anyway, given it is in at least 2 kernel releases? And there will be a warning anyway if the old option is used |
…rted by btrfs Fixup for e3828d7, as requested in #32892 (comment).
Although 'norecovery' mount option was marked as deprecated for a long time and a warning message was printed during the deprecation window, it's still actively utilized by several projects that need a safer way to mount a btrfs without any writes. Furthermore this 'norecovery' mount option is supported by other major filesystems, which makes it less clear what's our motivation to remove it. Re-introduce the 'norecovery' mount option, and output a message to recommend 'rescue=nologreplay' option. Link: https://lore.kernel.org/linux-btrfs/ZkxZT0J-z0GYvfy8@gardel-login/#t Link: systemd/systemd#32892 Link: https://bugzilla.suse.com/show_bug.cgi?id=1222429 Reported-by: Lennart Poettering <lennart@poettering.net> Reported-by: Jiri Slaby <jslaby@suse.com> Fixes: a1912f7 ("btrfs: remove code for inode_cache and recovery mount options") CC: stable@vger.kernel.org # 6.8+ Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
Although 'norecovery' mount option was marked as deprecated for a long time and a warning message was printed during the deprecation window, it's still actively utilized by several projects that need a safer way to mount a btrfs without any writes. Furthermore this 'norecovery' mount option is supported by other major filesystems, which makes it less clear what's our motivation to remove it. Re-introduce the 'norecovery' mount option, and output a message to recommend 'rescue=nologreplay' option. Link: https://lore.kernel.org/linux-btrfs/ZkxZT0J-z0GYvfy8@gardel-login/#t Link: systemd/systemd#32892 Link: https://bugzilla.suse.com/show_bug.cgi?id=1222429 Reported-by: Lennart Poettering <lennart@poettering.net> Reported-by: Jiri Slaby <jslaby@suse.com> Fixes: a1912f7 ("btrfs: remove code for inode_cache and recovery mount options") CC: stable@vger.kernel.org # 6.8+ Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
|
yes, new kernels will print an info level message apparently, so i guess we should keep this patch. |
Although 'norecovery' mount option was marked as deprecated for a long time and a warning message was printed during the deprecation window, it's still actively utilized by several projects that need a safer way to mount a btrfs without any writes. Furthermore this 'norecovery' mount option is supported by other major filesystems, which makes it less clear what's our motivation to remove it. Re-introduce the 'norecovery' mount option, and output a message to recommend 'rescue=nologreplay' option. Link: https://lore.kernel.org/linux-btrfs/ZkxZT0J-z0GYvfy8@gardel-login/#t Link: systemd/systemd#32892 Link: https://bugzilla.suse.com/show_bug.cgi?id=1222429 Reported-by: Lennart Poettering <lennart@poettering.net> Reported-by: Jiri Slaby <jslaby@suse.com> Fixes: a1912f7 ("btrfs: remove code for inode_cache and recovery mount options") CC: stable@vger.kernel.org # 6.8+ Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
Although 'norecovery' mount option is marked deprecated for a long time and a warning message is introduced during the deprecation window, it's still actively utilized by several projects that need a safely way to mount a btrfs without any writes. Furthermore this 'norecovery' mount option is supported by most major filesystems, which makes it harder to validate our motivation. This patch would re-introduce the 'norecovery' mount option, and output a message to recommend 'rescue=nologreplay' option. Link: https://lore.kernel.org/linux-btrfs/ZkxZT0J-z0GYvfy8@gardel-login/#t Link: systemd/systemd#32892 Link: https://bugzilla.suse.com/show_bug.cgi?id=1222429 Reported-by: Lennart Poettering <lennart@poettering.net> Reported-by: Jiri Slaby <jslaby@suse.com> Fixes: a1912f7 ("btrfs: remove code for inode_cache and recovery mount options") Cc: stable@vger.kernel.org # 6.8+ Change-Id: I10f2d85b9139fa6a8719d7ef2841b1b4ac03d5aa Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Jiri Slaby <jslaby@suse.cz> Signed-off-by: Ionut Nechita <ionut_n2001@yahoo.com>
…rted by btrfs Fixup for e3828d7, as requested in systemd/systemd#32892 (comment). (cherry picked from commit 055b465)
…rted by btrfs Fixup for e3828d7, as requested in systemd/systemd#32892 (comment). (cherry picked from commit 055b465)
…rted by btrfs Fixup for e3828d7, as requested in systemd/systemd#32892 (comment). (cherry picked from commit 055b465)
…rted by btrfs Fixup for e3828d7, as requested in systemd/systemd#32892 (comment). (cherry picked from commit 055b465) (cherry picked from commit 78e023a)
…rted by btrfs Fixup for e3828d7, as requested in systemd/systemd#32892 (comment). (cherry picked from commit 055b465) (cherry picked from commit 78e023a)
…rted by btrfs Fixup for e3828d7, as requested in systemd/systemd#32892 (comment). (cherry picked from commit 055b465) (cherry picked from commit 78e023a)
…rted by btrfs Fixup for e3828d7, as requested in systemd/systemd#32892 (comment). (cherry picked from commit 055b465) (cherry picked from commit 78e023a)
BugLink: https://bugs.launchpad.net/bugs/2068591 Although 'norecovery' mount option was marked as deprecated for a long time and a warning message was printed during the deprecation window, it's still actively utilized by several projects that need a safer way to mount a btrfs without any writes. Furthermore this 'norecovery' mount option is supported by other major filesystems, which makes it less clear what's our motivation to remove it. Re-introduce the 'norecovery' mount option, and output a message to recommend 'rescue=nologreplay' option. Link: https://lore.kernel.org/linux-btrfs/ZkxZT0J-z0GYvfy8@gardel-login/#t Link: systemd/systemd#32892 Link: https://bugzilla.suse.com/show_bug.cgi?id=1222429 Reported-by: Lennart Poettering <lennart@poettering.net> Reported-by: Jiri Slaby <jslaby@suse.com> Fixes: a1912f7 ("btrfs: remove code for inode_cache and recovery mount options") CC: stable@vger.kernel.org # 6.8+ Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> Signed-off-by: David Sterba <dsterba@suse.com> (cherry picked from commit 440861b1a03c72cc7be4a307e178dcaa6894479b) Signed-off-by: Roxana Nicolescu <roxana.nicolescu@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> Acked-by: Paolo Pisati <paolo.pisati@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
"norecovery" was deprecated for btrfs in
torvalds/linux@74ef001 and removed in
torvalds/linux@a1912f7.
Let's drop our assumption that btrfs supports "norecovery" and first query for the new name of the option followed by querying for the old name.