Page MenuHomeFreeBSD

Make nfs pageout coherent with the buffer cache.
ClosedPublic

Authored by kib on Apr 3 2017, 9:23 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mar 18 2024, 11:51 PM
Unknown Object (File)
Mar 10 2024, 5:03 AM
Unknown Object (File)
Jan 18 2024, 9:34 AM
Unknown Object (File)
Jan 17 2024, 12:54 AM
Unknown Object (File)
Dec 24 2023, 6:57 AM
Unknown Object (File)
Dec 23 2023, 7:46 PM
Unknown Object (File)
Dec 23 2023, 3:58 AM
Unknown Object (File)
Dec 9 2023, 5:42 PM
Subscribers

Details

Summary

Write out the dirty pages using VOP_WRITE() instead of directly calling ncl_writerpc(). Most important, the state of the buffers now reflects the write, fixing some hard to diagnose consistency and write order issues. Minor goods are removal of the now unneeded additional remapping of paged out pages into kernel space and related allocation of the phys buffer, some initial support of IO_ASYNC write mode.

Patch also more or less consistently adds checks for the reclaimed vnode after call to ncl_vinvalbuf(), which might need to upgrade the vnode lock and allows for vgone() to occur meantime.

Since vnode' object page removal or cleanup recursively called from ncl_vinvalbuf() during pageout would cause deadlock and also does not make sense, a new flag V_VMIO is added for bufobj_invalbuf(), which requests to avoid VM calls.

The calculation of IO_XXX flags from VM_PAGER_PUT_XXX flags was extracted from vnode_pager_generic_putpages(). Related style cleanup was performed in the block for local declarations.

Patch will be naturally split per change for actual commit.

Test Plan

Patch was tested by pho.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

sys/fs/nfsclient/nfs_clbio.c
610 ↗(On Diff #26943)

I think (v_iflag & VI_DOOMED) != 0 implies error == 0, but elsewhere there is an explicit check for error == 0.

sys/fs/nfsclient/nfs_clvnops.c
724 ↗(On Diff #26943)

A VI_DOOMED check seems to be needed here.

1677 ↗(On Diff #26943)

Isn't it incorrect to call nfs_removerpc() in the VI_DOOMED case?

sys/vm/vnode_pager.h
47 ↗(On Diff #26943)

"sync" should probably change to "flags."

kib marked 4 inline comments as done.

Handle markj feedback: more accurate handling of reclaim in several places, rename sync to flags.

Ran tests on Diff 27010 for five hours just to be safe. No problems seen.

markj added inline comments.
sys/fs/nfsclient/nfs_clvnops.c
725 ↗(On Diff #27010)

Needs to be v_iflag.

726 ↗(On Diff #27010)

How can this happen? ncl_reclaim() sets vp->v_data to NULL.

This revision is now accepted and ready to land.Apr 4 2017, 4:43 PM
kib marked an inline comment as done.Apr 4 2017, 5:16 PM
kib added inline comments.
sys/fs/nfsclient/nfs_clvnops.c
726 ↗(On Diff #27010)

VI_DOOMED is set by vgonel(), not by VOP_RECLAIM(). In the vgonel() code between setting VI_DOOMED and the call to VOP_RECLAIM(), there are several places, e.g. vfs_notify_upper(RECLAIM) or VOP_CLOSE() or VOP_INACTIVE() calls which might drop the vnode lock, allowing other threads to see doomed but not fully relcaimed vnodes.

Hmm, I mis-remembered notify_upper() as the main offender there, but apparently I fixed nullfs_reclaim_lowervp() by using NULLV_NOUNLOCK. E.g. nfs_close() calls ncl_vinvalbuf() but still it should be safe because VOP_CLOSE() from vgonel() is called with exclusive vnode lock owned.

So I removed this chunk for now, but still I do not think that we really ensure that half-vgoned node is never relocked.

1677 ↗(On Diff #26943)

I suspect that the most correct action would be to recheck the sillyrename condition once more if the vnode got VI_DOOMED flag but we still see non-NULL v_data. But this can only occur of the vgonel() dropped vnode lock, our thread raced with the reclaim thread and we won. I do not think that it is worth the trouble, in the worst case such race would leave the .nfs.XXXX file on server. So I only added the 'else' before the old 'if' statement there.

kib edited edge metadata.

Fix flags member for VI_DOOMED, remove np != NULL chunk.

This revision now requires review to proceed.Apr 4 2017, 5:17 PM
sys/fs/nfsclient/nfs_clvnops.c
726 ↗(On Diff #27010)

I was looking through vgonel() and its callees for places where the lock was dropped and did not find any, hence my question. In general it seems you're right that there's no strong guarantee that the lock is not dropped between VI_DOOMED being set and the VOP_RECLAIM() call, so I don't feel strongly either way about keeping the check.

This revision is now accepted and ready to land.Apr 4 2017, 5:40 PM

I am not familiar with the VM side, but it looks good to me, rick.

This revision was automatically updated to reflect the committed changes.