linux-kernel - Re: [GIT PULL] Rename page_offset() to page

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wgCAGVwAVTuaoJu4bF99JEG66iN7_vzih=Z33GMmOTC_Q@mail.gmail.com>
Date:   Sat, 11 Apr 2020 13:57:56 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] Rename page_offset() to page_pos()

On Sat, Apr 11, 2020 at 1:32 PM Matthew Wilcox <willy@...radead.org> 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