In D39246#894657, @mckusick wrote:

You have a very valid point. I addressed this by having just a single message come out when running with -p (preen mode). When encountering a filesystem with no depths it prints `UPDATING FILESYSTEM TO TRACK DIRECTORY DEPTH'' then updates all the directory depths. When running in interactive mode and -y mode it asks once `UPDATE FILESYSTEM TO TRACK DIRECTORY DEPTH?'' then just does it. No further reporting happens in either case.

The concern I have with this change is about user interface. With this patch, if someone renames a directory that is the root of a subtree containing other directories and then unmounts the file system cleanly, fsck will report that the file system is now corrupted because all of the dirdepth values for directories in that subtree are wrong. In fact, every existing file system will immediately be reported as corrupted the first time that the new fsck is run. There are millions of people in the world who know what the fsck output for a clean and uncorrupted UFS file system looks like, and that clean fsck output is very easy to recognize because it's very short and always the same (modulo the line with the last-mounted-on path and the line at the end with the block/inode usage and fragmentation summary). After this patch, fsck can produce millions of lines of output for file systems that aren't really corrupted but rather are merely not optimal, and it will no longer be so simple to tell if a file system is corrupted or not by looking at the fsck output.

I was waiting for the UFS fixes to land.

Updated the diff based on review comments.
The suggestion was to only enable vn_read_from_obj() if the fs fragment size is larger than the page size, but it's actually fine to use vn_read_from_obj() as long as the fs block size is greater than or equal to the page size, so I made the check that way.

This makes sense to me.

No, in this example of 16k pages and 4k blocks, the first page would contain the first 4 blocks, the second page would contain the next 4 blocks, etc, so mmap'ing the file works as you would expect.

made requested changes.

It would be good to go ahead and resolve the XXX comments that are currently being added by the patch.

I would characterize this change as: redefine "read_bias==0" from the current meaning of "prefer writes" to instead mean "no read-vs-write bias".
This patch looks correct in implementing that new meaning, though it would be nice if it didn't involve essentially duplicating the code of cam_iosched_get_write() in cam_iosched_next_bio().

I agree with kib that there is no need to serialize read requests (that need to choose whether to read from lower or upper) with write requests (that change the target of future reads from lower to upper)... allowing a pending read from the lower layer to complete after a write changes the target of future reads of that sector to be the upper layer is fine. The application must be prepared for the read to return either the old data or the new data in this case because the order of processing of these parallel async operations is not guaranteed.

Yea, I couldn't see any reason for the linker to leave the gap in libcrypto.so's memory layout either, but it doesn't seem like an invalid thing to do either, and it's easy enough to handle it.

update for review comments.

there's one more unsafe use of g_io_request() in UFS, in softdep_synchronize(), but that can be fixed separately from this one.

I'm told that these day using "bool" and true/false is preferred over using "int" and 1/0, but otherwise this is good.

overall I agree with mjg and kirk that it would be simpler and better in the long term for the bufobj to be part of the fs-specific "inode" structure for file systems that need it, rather than having the fs-independent code need to know about multiple flavors of vnodes.

what is the "issue reported by jhb" that you mention?

The same issue exists in g_io_speedup() (though that function appears to be unused at the moment).

It might be that brelse/bqrelse should also assert that the flag is cleared, but there I am not that sure.

Would it make sense to put this clearing of B_BARRIER someplace central like in bufdone() rather than in these specific error paths?
If we don't clear this flag in bufdone() then it would be good to assert in bufdone() that the flag is not set, to catch such mistakes earlier.

ahh, right. the test I added for this fails the I/O in such a way (using gnop) that the iodone is actually called in the start path, so it's the same thread that xbusied the pages in that case. you said this problem was reported by other automated testing, how does that test fail I/O such that the iodone is called in a separate thread?

In D25431#570827, @ngie wrote:

Curious... is there a reason why this was added in addition to lib/libc/tests/sys/sendfile_test.c?

update diff for kib's latest comments.

typo

rename the new function and limit this diff to only fixing vnode_pager_generic_getpages_done().

In D25430#566098, @markj wrote:

In D25430#565910, @kib wrote:

In D25430#565902, @markj wrote:

In D25430#565838, @kib wrote:

The proposal means that it is impossible to wire the page in non-sleepable context, unless we own xbusy on the page in advance. I believe this is no-go.

It is unusual for code to wire managed pages without using the busy lock to provide some consistency. vm_page_grab(), etc., provide mechanisms to avoid blocking, so callers can block and retry at a point of their choosing. In fact I cannot see any places in the tree today where a managed wiring is created without the busy lock.

I would agree that wiring could obey to this rule, but I believe we must provide sleep-less mechanism to prevent page from reuse. Some time ago it was hold, since then hold count was merged with wire count, thus wire count should provide such functionality.

Let's keep it for now then. In the meantime, we might generalize vm_page_readahead_free() to handle non-readahead pages. For example, fault_page_free() could also remove the page from its object. This function could also be used in tmpfs_reg_resize() and shm_dotruncate_locked().