Page MenuHomeFreeBSD

Add VOP_READ_PGCACHE(9)
ClosedPublic

Authored by kib on Feb 28 2021, 1:11 AM.

Details

Summary

PR: 253894

Diff Detail

Repository
R10 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 requested review of this revision.Feb 28 2021, 1:11 AM
kib created this revision.

Install. Cross-ref from vnode(9).

rwatson added inline comments.
share/man/man9/VOP_READ_PGCACHE.9
51

I would probably spell this IO.

58

Is it worth specifically stating how this differs from VOP_READ(9) -- i.e., the caller will not acquire the vnode lock, and will retry once holding the lock using VOP_READ(9) if this doesn't work? (Assuming I understand correctly.)

Should the implementation of VO_READ_PGCACHE(9) promise *not* to do expensive things, such as disk IO, or is this simply an opportunity for it to provide a read operation without requiring the caller to lock?

67

Perhaps instead: "request; it also might choose..."

83

Should we refer the reader to VOP_READ(9) for the possible flag values ..?

84

Should be "cred" not "cnp"?

88

Can drop the definite article, "The," here.

92

Should we drop the argument? Or is the plan that we might support it at some point?

(Can we assert this in a VOP pre-hook to avoid confusion arising..?)

96

Should we say something about the vnode lock not being required for callers .. and about whether the callee should or shouldn't acquire it? Should the callee fail if it needs to acquire the vnode lock, and let a followup call to VOP_READ(9) handle it?

104

In this case, the access is performed, and the uio is still updated to reflect data read ..? Or is this an error and VOP_READ should be tried?

I guess it makes me slightly uncomfortable to have a non-zero success mode, as it's so easy for callers to get that horribly wrong (vis OpenSSL), but I suppose there's also only one caller of this. Regardless, I think the expected semantics should be documented more thoroughly here (and possibly above as well, rather than in the error code section).

105

Possibly we should point at possible error returns from VOP_READ(9)?

This revision now requires changes to proceed.Feb 28 2021, 10:20 AM
kib marked 10 inline comments as done.Feb 28 2021, 5:18 PM
kib added inline comments.
share/man/man9/VOP_READ_PGCACHE.9
58

It is up to the filesystem to do whatever it intents to. Typically fs would not want to do vn_lock(); VOP_READ(); VOP_UNLOCK(); if only because the vnode can be reclaimed before or during lock, in which case fs should not do read on it at all.

WRT expensive ops, it is up to fs.

The main difference in the call environment: lockless and the permit to delegate to later VOP_READ() by special error return, are stated.

92

It is up to filesystem, doing it in pre is not better than ensure that callers do it right.

I do not want to drop the flag arg, we might want to pass some info, and I want to have
KBI in stable to do it if needed.

96

I added the note about vnode not being locked.

I do not want to give even a hint that the implementation of the VOP could lock the vnode.

104

I added a note that uio is updated as far as IO is done. I thought that this is obvious (how would the caller know it, otherwise?)

There should be a way to distinguish partial read vs. short read. EJUSTRETURN is as good as other ways to notify about partial read, e.g. explicit EOF flag. EJUSTRETURN not returned to userspace is used by other parts of VFS, e.g. for successful namei(9) call that does not return resulting vnode (e.g. CREATE).

Of course it is important to handle EJUSTRETURN, but it is VFS problem and not fs issue. Somebody calling the VOP must know much more than somebody implementing it. Man pages are to provide the first look into the interface.

kib marked 4 inline comments as done.

Handle rwatson' notes.

gbe added a subscriber: gbe.

LGTM from manpages

share/man/man9/VOP_READ_PGCACHE.9
1

This year could be updated to 2021.

kib marked an inline comment as done.Feb 28 2021, 5:35 PM
rwatson added inline comments.
share/man/man9/VOP_READ_PGCACHE.9
123

I wonder if it's worth

This revision is now accepted and ready to land.Feb 28 2021, 6:13 PM
share/man/man9/VOP_READ_PGCACHE.9
123

Can you explain this note, please?

share/man/man9/VOP_READ_PGCACHE.9
123

I started making a comment and then decided not to make it, and experienced a user error. You can disregard this note.

(I was going to suggest giving an example of such a situation, but ... I think it's OK without it.)

This revision was automatically updated to reflect the committed changes.