vfs_donmount: in certain cases try r/o mount if r/w mount fails
ClosedPublic

Authored by avg on Dec 4 2017, 1:53 PM.

Details

Summary

If the operation is not an update, if neither r/w nor r/o mode is
explicitly requested and if the error code hints at the possibility
of the media being read-only, then we can try to automatically
downgrade to the readonly mode.

This is especially useful for auto-mounting of removable media
that sometimes can happen to be write-protected.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
avg created this revision.Dec 4 2017, 1:53 PM
kib requested changes to this revision.Dec 4 2017, 3:36 PM

Such level of artificial intelligence was never applied to the Unix behavior. IMO we should not out-guess the user, and if the requested mode of operation failed, to not retry with randomly tweaked parameters.

For instance, we do not guess the filesystem type in the kernel. Why rw/ro should be different ?

This revision now requires changes to proceed.Dec 4 2017, 3:36 PM
avg added a comment.Dec 4 2017, 3:53 PM
In D13361#278943, @kib wrote:

For instance, we do not guess the filesystem type in the kernel. Why rw/ro should be different ?

Because it's convenient and it is really not hard or messy to implement.

Another, perhaps weak, argument is that mount(8) does not mandate behavior for the case where neither ro or rw are requested (the default is not documented) and that leaves room for the proposed change.

imp added a comment.Dec 4 2017, 4:25 PM
In D13361#278954, @avg wrote:
In D13361#278943, @kib wrote:

For instance, we do not guess the filesystem type in the kernel. Why rw/ro should be different ?

Because it's convenient and it is really not hard or messy to implement.

Another, perhaps weak, argument is that mount(8) does not mandate behavior for the case where neither ro or rw are requested (the default is not documented) and that leaves room for the proposed change.

Historic unix behavior is against you. People rely on the current behavior failing when the disk isn't writable to keep them from proceeding in, for example, their scripts. They don't specify rw because that's the default and has been the default behavior since Bell Labs First Edition Unix when the mount command was introduced...

avg added a comment.Dec 4 2017, 4:37 PM

I'd rather focus on the convenience of a modern user than on the historic Unix behavior.

Not sure if any of you use a FreeBSD-based desktop environment, I do, KDE.
It's supposed to be one with the most knobs.
Unfortunately, it does not offer a read-only mount button, just mount.
So, if I happen to insert a write-protected SD card, then I cannot mount it through the desktop interface.
Of course, I know how to use the command line. And, of course, it's a deficiency of KDE that it does not offer a readonly mount option and does not automatically detect readonly media, etc.
But if I can make things a little bit better (for myself, first and foremost), I try to do it.

P.S.
And it's a shame that we have VFS / buffer cache bugs like this one https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=224037#c0 and they do not get fixed for many months (if not years).

imp added a comment.Dec 4 2017, 5:14 PM
In D13361#278967, @avg wrote:

I'd rather focus on the convenience of a modern user than on the historic Unix behavior.

I'd rather not break scripts that reply on this behavior. I know of several that exist at prior employers.

It's not a no, but it's push back that this isn't POLA based on what people expect.

And it's a shame that we have VFS / buffer cache bugs like this one https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=224037#c0 and they do not get fixed for many months (if not years).

We all know there's issues in the buffer cache for a long time. This is but one of many problems there, and it isn't so easy to fix. Every time I've tweaked code in that area, I've discovered I introduced another bug that's not discovered or traced down for months... It's also orthogonal to the current issue at hand... Then again, I've been looking to fund a position that would do nothing but fix the strange panics from the error path of the buffer cache/ufs w/o luck. :(

avg added a comment.Dec 4 2017, 5:29 PM

It's not a no, but it's push back that this isn't POLA based on what people expect.

Well, it depends on definition of "people".
I know that one wide-spread OS does this and so some people are used to it.
"mount: block device /dev/sdb1 is write-protected, mounting read-only"
But I am not sure if it does that in kernel or in userland.

kib added a comment.Dec 4 2017, 5:46 PM
In D13361#278984, @avg wrote:

It's not a no, but it's push back that this isn't POLA based on what people expect.

Well, it depends on definition of "people".
I know that one wide-spread OS does this and so some people are used to it.
"mount: block device /dev/sdb1 is write-protected, mounting read-only"
But I am not sure if it does that in kernel or in userland.

I might accept this behavior if enabled by a specific mount option. But generally this is quite anti-pattern for Unix, where command does exactly what asked for or fails.

avg updated this revision to Diff 40320.EditedMar 15 2018, 4:25 PM

just rebase on a newer head

avg updated this revision to Diff 40322.Mar 15 2018, 5:09 PM
  • do not allow automatic r/o fallback by default.
  • add a new mount option, autoro, to request the fallback.
  • provide a new sysctl, vfs.default_autoro, to enable the automatic fallback by default; the fallback is attempted only if none of ro, rw, noro, norw, rdonly is specified.

Maybe autoro and default_autoro are not the best names.
Any suggestions are appreciated.

cem accepted this revision.Mar 15 2018, 11:13 PM
cem added inline comments.
sys/kern/vfs_mount.c
84 ↗(On Diff #40322)

Prefer bool for new boolean sysctls, I think.

cem added inline comments.Mar 15 2018, 11:15 PM
sys/kern/vfs_mount.c
692–697 ↗(On Diff #40322)

It does seem like these are not needed or make the fsflags MNT_RDONLY check redundant.

avg updated this revision to Diff 40341.Mar 16 2018, 7:21 AM

document autoro in mount.8

avg updated this revision to Diff 40346.Mar 16 2018, 2:41 PM

use SYSCTL_BOOL for a boolean knob (suggested by @cem)

avg marked an inline comment as done.Mar 16 2018, 2:50 PM
avg added inline comments.
sys/kern/vfs_mount.c
84 ↗(On Diff #40322)

I didn't realize that we had SYSCTL_BOOL.
Thanks!

692–697 ↗(On Diff #40322)

These are indeed redundant, but I want to have them, so that it is very clear than any explicit option overrides the fallback logic.

cem added inline comments.Mar 16 2018, 3:24 PM
sys/kern/vfs_mount.c
692–697 ↗(On Diff #40322)

Sure, it's harmless.

kib accepted this revision.Mar 22 2018, 2:39 PM
This revision is now accepted and ready to land.Mar 22 2018, 2:39 PM
avg updated this revision to Diff 40792.Mar 27 2018, 2:22 PM
avg marked an inline comment as done.

rebase to r331615

This revision now requires review to proceed.Mar 27 2018, 2:22 PM
This revision was not accepted when it landed; it landed in state Needs Review.Mar 27 2018, 2:32 PM
This revision was automatically updated to reflect the committed changes.