UFS SU: handle races on remounts rw<->ro

[kib]

Summary

rw<->ro remounts are not atomic, filesystem is accessible by other threads during the process. As result, its internal state is inconsistent. Just blocking writers with suspend is not enough.

Instead, set fs_ronly = 1 and clear MNT_SOFTDEP flags very early in the process of unmount, while allowing SU code that needs to modify filesystem for final sync to process by changing the test from MNT_SOFTDEP flag to um_softdep != NULL.
Also ensure that non-NULL um_softdep is fully initialized when observed, which is important for ro->rw remounts.

Nullfs changes are not strictly related but were discovered during tests. They are separate commits with its own explanations.

The split into individual commits is available at https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=shortlog;h=refs/heads/ufs

[mckusick]

In your summary you say ``rw<->ro remounts are not atomic, filesystem is accessible by other threads during the process. As result, its internal state is inconsistent. Just blocking writers with suspend is not enough.'' Can you elaborate on how having other processes reading the filesystem causes trouble?

[kib]

In D29178#653253, @mckusick wrote:

In your summary you say ``rw<->ro remounts are not atomic, filesystem is accessible by other threads during the process. As result, its internal state is inconsistent. Just blocking writers with suspend is not enough.'' Can you elaborate on how having other processes reading the filesystem causes trouble?

Suppose that we remount rw->ro and in parallel some reader tries to instantiate a vnode, e.g. during lookup. Suppose that softdep_unmount() already started, but we did not cleared the MNT_SOFTDEP flag yet. Then ffs_vgetf() calls into softdep_load_inodeblock() which accessed destroyed hashes and freed memory. This was the biggest offender.

The solution I propose is to set fs_ronly early, and clear MNT_SOFTDEP early. Then I need to arrange that existing dependency flush code paths still work.

[mckusick]

Nearly all of the changes in ffs_softdep.c are code cleanups and not related to this bug fix. I would prefer to see the code cleanups in a separate commit so that it is easier to see the changes that are needed to fix this problem. That said, this update appears to solve the problem that you describe.

sys/ufs/ffs/ffs_extern.h

131 ↗

(On Diff #85457)

It would be helpful to document/comment these flags.

[mckusick]

It would help if I had looked at your commit logs before my previous comment. You have in fact separated everything out appropriately.

[kib]

Document ffs_vgetf() flags

[kib]

In D29178#653926, @mckusick wrote:

Nearly all of the changes in ffs_softdep.c are code cleanups and not related to this bug fix. I would prefer to see the code cleanups in a separate commit so that it is easier to see the changes that are needed to fix this problem. That said, this update appears to solve the problem that you describe.

In fact only a simplification for jourmal_mount() (removal of error == 0 check) and some style fixes can be accounted as unrelated. All other non-nullfs changes do fix bugs occuring at rw/ro remounts, or prepare for other commits that are bug fixes.

[mckusick]

Flag definitions look good.
Breakdown of commits is excellent.
Changes should resolve the problem.