Page MenuHomeFreeBSD

ufs_need_inactive.
ClosedPublic

Authored by kib on Sun, Dec 1, 12:22 AM.

Details

Summary

The checks literally repeat conditions that make ufs_inactive() to take some actions.
The doubt point is that most of the conditions are only stable under the vnode lock.

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.

Event Timeline

kib created this revision.Sun, Dec 1, 12:22 AM
jeff added a reviewer: jeff.Sun, Dec 1, 12:29 AM
jeff added a comment.Tue, Dec 3, 7:00 AM

This misses the explicit vrecycle(). I don't know how often that matters.

I wonder whether we would be better served by a 'NEEDS INACTIVE' flag that is conditionally set by compliant filesystems and always set for non-compliant ones. It would save us the VOP call on every vput(). It is easy to identify where these conditions change in ufs. It is only inode updates and truncate and unlink. The inode updates are already managed in a common place.

kib added a comment.Tue, Dec 3, 11:51 AM
In D22616#495270, @jeff wrote:

This misses the explicit vrecycle(). I don't know how often that matters.

Sorry, I do not understand this note. Isn't VOP_NEED_INACTIVE() should only indicate whether inactivation has anything to do ? We do not own the vnode lock in NEED_INACTIVE, which is the point of the NEED_INACTIVE(): to avoid taking the lock if inactivation is not useful.

I wonder whether we would be better served by a 'NEEDS INACTIVE' flag that is conditionally set by compliant filesystems and always set for non-compliant ones. It would save us the VOP call on every vput(). It is easy to identify where these conditions change in ufs. It is only inode updates and truncate and unlink. The inode updates are already managed in a common place.

You mean, set OWEINACT when fs changes the vnode in a way that would require subsequent inactivation ? This is good idea, but a lot of very delicate work, and easily causing data corruption.

mjg added a comment.EditedTue, Dec 3, 2:39 PM
In D22616#495270, @jeff wrote:

This misses the explicit vrecycle(). I don't know how often that matters.
I wonder whether we would be better served by a 'NEEDS INACTIVE' flag that is conditionally set by compliant filesystems and always set for non-compliant ones. It would save us the VOP call on every vput(). It is easy to identify where these conditions change in ufs. It is only inode updates and truncate and unlink. The inode updates are already managed in a common place.

I think we would be best served with VOP_VPUTX instead. Filesystems could do whatever they see fit and the current code could be chopped up into helper functions. Default routine would do what vputx is doing now. In my original patch for the entire thing I had filesystems check what they want and resort to calling vop_stdvputx if necessary which could provide an intermediate step. Or in general, VFS going forward needs to do less hand holding for filesystems as the current approach is prone to doing additional work which dirties cache lines and is often unnecessary.

I consider the current approach to not be flexible enough. For instance I'm looking at (temporarily) caching vnodes on the active list to reduce constant movement and requeing. As it is filesystems don't have a way to indicate whether the vnode is up for being cached (e.g., it was not unlinked). In the current scheme I would have to use need_inactive to refrain from caching (and in fact un-cache) even if it the vnode was not unlinked and only needed some processing.

That said, I think for the time being we should just sort out the need_inactive routine for ufs and interface changes (if any) are a separate and a lengthy conversation. Doing things the simple way now makes them eligible for getting merged to stable/12.

jeff accepted this revision.Wed, Dec 4, 1:52 AM

I understand my one objection now.

This revision is now accepted and ready to land.Wed, Dec 4, 1:52 AM
This revision was automatically updated to reflect the committed changes.