On Sat, Apr 11, 2020 at 1:32 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> We've had some trouble recently with page_offset() being confusingly
> named.
This makes little sense to me.
I don't find "page_pos()" to be in the least more intuitive than
"page_offset()". Yes, you have some numbers of "offset" vs "pos"
being used for the position in the file, but they aren't _that_
different, and honestly, if you look at things like the man-page for
"lseek()", the byte offset you seek to is called an "offset".
The fact that somebody was confused by the current name is a red
herring - there's nothing to say that they wouldn't have been confused
by "page_pos()", except for the fact that that wasn't the name.
So honestly, i the confusion is that we have "pgoff_t", which is the
offset of the page counted in _pages_, then my reaction is that
(a) I think the truly confusing name is "pgoff_t" (and any
"page_offset" variable of that type). Calling that "pgindex_t" and
"page_index" would be a real clarification.
(b) if we really do want to rename page_offset() because of confusion
with the page index "offset", then the logical thing would be to
clarify that it's a byte offset, not the page index.
So "page_pos()" to me sounds not at all more descriptive, and having
two names (for stable kernels, for people with memories, for
historical patches, whatever) only sounds like a source of even more
confusion in the future.
If we'd want a _descriptive_ name, then "byte_offset_of_page()" would
probably be that. That's hard to mis-understand.
Yes that's also more of a mouthful, and it still has the "two
different names for the same thing" issue wrt
stable/old/rebased/whatever patches.
But if there are enough people who find "page_offset()" to be a source
of confusion, then I'd at least prefer to _truly_ remove any
possibility of confusion with that longer name.
I'd like to have a few more people step up and say "I find that name
confusing enough that I think it's worth the confusion of renaming
it".
We've had the "page_offset()" name _forever_, this is the first time I
hear it being a problem (it goes back to 2005, and before that it was
used inside the NFS code).
Of course, we've also had "pgoff_t" forever - that name goes back to 2002.
But unlike "page_offset()", I do think that "pgoff_t" is actually a
truly bad name.
Which is why I'd much rather change "pgoff_t" to "pgindex_t" and
related "page_offset" variables to "page_index" variables.
Linus
