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().

update diff with markj's suggestions.

updated diff for markj's latest comments.

In D25430#563262, @markj wrote:

In D25430#562442, @chs wrote:

And yea, this patch does leave these invalid pages in an odd state, where the pagedaemon can't reclaim them (though if the vnode is reclaimed then all of these invalid pages will be freed at that point). I wasn't going to worry about that right now since the sendfile code already can leave pages in this state after an I/O error, and kib thought that was ok in a review of a previous fix for sendfile I/O error handling. But if you'd like this fix to avoid creating this situation in vnode_pager_generic_getpages_done(), we can do that too.

While I was looking into vm_page_readahead_finish(), I noticed that swp_pager_async_iodone() also leaves pages in this state.

I see. That looks like a bug to me.

redo diff as markj suggested.

Yea, I didn't know what the problem having invalid pages on the paging queues would be either.

The intention is that ffs_subr.c should only contain functions that are shared by the ufs kernel module and the libufs userland library. At some point the geom_label kernel module started using ffs_subr.c too even though that is cross-module direct reference, which is problematic when not all of the modules are statically included in the base kernel. Now that I moved the functions which are only needed by the ufs kernel module elsewhere, geom_label's instance of ffs_subr.c no longer references M_UFSMNT, so this change is not necessary.

Gleb asked me to commit this so that git blame will still blame me. :-)
The diff looks fine to me, adding kirk as a reviewer also.

really remove putpages wrapper this time.

update to address review comments.

update diff to 360823

Are there any more comments on this diff?

generally, having the function that submits an async request return a pointer a request structure that is freed when the request completes is a bad idea, since the caller will have handle the request structure having already been freed, and thus having a pointer to it isn't useful. as you guys discussed, using the normal timeout path for polled commands (and fixing the normal timeout path to not be racy) would be good. a fun corner case will be when the use of the abort command in the timeout path itself times out.

update with changes suggested by kib and imp.

There's no central place in the ufs code to add an assertion about not creating buffers on odevvp. What I could do is define a new BO_NOBUFS flag for bufobj bo_flag, set that flag on odevvp in ffs_mountfs() and clear the flag in ffs_unmount(), and then in buf_vlist_add() assert that (bo->bo_flag & BO_NOBUFS) == 0. Is that what you had in mind?

minor nit: the returned "resid" value is unused. otherwise looks good.

looks good.

looks good.

I'll try to address all the points that everyone has made so far here.