Still need to track down problem cited by Peter Holm.

Changes suggested by kib and refinements suggested by Robert Morris

I concur that this note should be added.

Calculations and comment are accurate.

This is an accurate calculation the the extra needed space.

This seems like a useful change.

I like the approach suggested by kib that ufs_open() not set VIRF_PGREAD when the page size is larger than the fragment size.

I concur with the analysis. Waiting for trims in the current location is clearly too late. Waiting in the new location will ensure that all the dependencies get flushed. And once flushed it should not be possible to add any new ones thus no new trims should be able to appear.

Looks good. Short and to the point.

The main clients of making snapshots are dump(8), fsck_ffs(8), and mksnap_ffs(8) and none of them want to change any other attributes. Generally speaking, administrators do not use the mount(8) command directly to take snapshots. The one other change I would recommend is to document in the mount.8 manual page that when the "snapshot" option is specified, any other options will be ignored.

I picked those names when ufs was the only filesystem in BSD. I should have renamed them when I added the VFS interface. Better late than never!

This change looks reasonable to me.

minor format question. Change looks correct.

This update reflects some minor cleanups that were reported by Peter Holm during his testing.

These changes address comments from all three reviewers.

This patch eliminates the LOR in my test case.

I agree with this change.

Eivind Eklund was the person that first added KASSERT which was the start of what probably should have been called kassert.h. Many others added to it over time probably putting more effort into than he did. But I am inclined to give him credit for starting the ball rolling. He was an active committer from February 1997 until June 2005 (461 base, 69 ports, 26 doc). I note that he still has a freefall account.

In D34089#770782, @kib wrote:

I copied license from systm.h, but clearly UCB has no relations to this relatively modern stuff at all. Not sure what to do there.

Useful cleanup.

Once passing Peter Holm's testing, this looks good to go.

Based on our previous discussions in D33921 these changes improve readability and consistency.

In D33921#770058, @kib wrote:

So more I am trying to fix consequences of the snap relock following Peter reports, more I doubt the original issue.

Lets consider ffs_read() report: we got a locked UFS vnode, which reads some buffer, which is locked to be read. There are two cases: vnode is a snapshot, or it is not. In the second case, the order is right from the start.

In the first case, I claim that all snapshot vnode' buffers are after snaplk: in both ffs_copyonwrite() and ffs_snapblkfree(), we lock snaplk, and then iterate over snapshots taking corresponding buffer locks as needed, and this is IMO the natural order. So in the first case, read from the snapshot vnode, the order is right.

On the other hand, all data blocks are before snaplk. In other words, the order of the locks is (UFS vnode)->(data bufwait)->(snaplk)->(snapshot block bufwait). We could get LoR only in case when data buffer is becoming snapshot data during some of the snapshot low ops (copyonwrite or blkfree). But we never do that rename, we always allocate new block for snapshot and copy data as needed.

Let me summarize: I now think that the original report about LoR in ffs_read() is a false positive from witness, since it cannot distinguish between snap/normal data buffers.

P.S. Even if I am wrong above, I do not think that the approach with relocking of the snaplk lock is workable. For instance, in ffs_balloc_ufs2(), unlocking snaplk allows other thread to create softupdate tracker for allocation of the same logical block which is not yet allocated.

I think that this change is an improvement.

This looks good to me.

Changes to avoid needing to free newb look good. Really glad to not have to figure out how to do that. Other changes look right. Awaiting Peter's tests to see if you have found all the needed changes.

Generally looks good. Definitely want to see if Peter turns up anything. If Peter clears it, freeing newb will be necessary. Unfortunately freeing newb is going to require writing a bunch of code to unwind a lot of soft update stuff. It will be a lot easier if we reorder the code to get the buffer first and then only allocate the block once we know we will have a buffer for it. Since the buffer is hashed on lbn rather than bn we do not need to have a block allocated before we get its buffer.

This patch eliminates my bufwait <-> snaplk LOR warning when doing background fsck.

Latest updates all look good.

Spelling nit.

Looks good to me. If inspired a comment over msdosfs_remount_ro() would be nice, though the name pretty much says what it does.

Overall looks good. Glad to see it done.

Your comment wrap rework looks good. As you note, it is consistent with the way di_shortlink is commented.

I have made the commit to head and have scheduled MFC in two weeks.
I will certainly MFC it to 13. I have not set up an environment for MFC'ing to 12, but will do so if time permits.
Please send me your email address to mckusick@mckusick.com.

Since the change is such a minor edit, I will make it without starting a new Phabricator review.
Thanks for pointing out the need for clarification.

This looks like a useful change.

See suggested clarification.

This looks good.

The idea and implementation look good. I would like my noted style comments handled.

Comment added. The function has always scanned the entire string and used the last instance of the option. Changing it now may break existing and working installations so I left it as is. Note that if it gets changed here it should be changed in the other six places to be consistent.

Add comment describing getfsopt(). Historical footnote: this code originated in 4.4BSD.

Incorporate feedback from kib.

I changed the name from hasopt() to getfsopt() in keeping with the other names in getfsent(3) so that it can be added more easily in the future if desired. At the moment there are variations of getfsopt() used in seven places in the base system lib/libutil/quotafile.c, libexec/rpc.rquotad/rquotad.c, sbin/fsck/fsck.c, sbin/mount/mount.c, sbin/swapon/swapon.c, usr.sbin/bsdinstall/partedit/gpart_ops.c, and usr.sbin/bsdinstall/partedit/partedit.c). Updated diff to follow shortly.

Sorry for my long delay.

I have added comments / questions to your three latest (helpful) comments. I would also like your feedback on my question to your earlier comment at line 340 of g_union.c.