On Wed, Feb 10, 2021 at 8:33 AM David Howells <dhowells@xxxxxxxxxx> wrote:
>
> Then I could follow it up with this patch here, moving towards dropping the
> PG_fscache alias for the new API.
So I don't mind the alias per se, but I did mind the odd mixing of
names for the same thing.
So I think your change to make it be named "wait_on_page_private_2()"
fixed that mixing, but I also think that it's probably then a good
idea to have aliases in place for filesystems that actually include
the fscache.h header.
Put another way: I think that it would be even better to simply just
have a function like
static inline void wait_on_page_fscache(struct page *page)
{
if (PagePrivate2(page))
wait_on_page_bit(page, PG_private_2);
}
and make that be *not* in <linux/pagemap.h>, but simply be in
<linux/fscache.h> under that big comment about how PG_private_2 is
used for the fscache bit. You already have that comment, putting the
above kind of helper function right there would very much explain why
a "wait for fscache bit" function then uses the PagePrivate2 function
to test the bit. Agreed?
Alternatively, since that header file already has
#define PageFsCache(page) PagePrivate2((page))
you could also just write the above as
static inline void wait_on_page_fscache(struct page *page)
{
if (PageFsCache(page))
wait_on_page_bit(page, PG_fscache);
}
and now it is even more obvious. And there's no odd mixing of
"fscache" and "private_2", it's all consistent.
IOW, I'm not against "wait_on_page_fscache()" as a function, but I
*am* against the odd _mixing_ of things without a big explanation,
where the code itself looks very odd and questionable.
And I think the "fscache" waiting functions should not be visible to
any core VM or filesystem code - it should be limited explicitly to
those filesystems that use fscache, and include that header file.
Wouldn't that make sense?
Also, honestly, I really *REALLY* want your commit messages to talk
about who has been cc'd, who has been part of development, and point
to the PUBLIC MAILING LISTS WHERE THAT DISCUSSION WAS TAKING PLACE, so
that I can actually see that "yes, other people were involved"
No, I don't require this in general, but exactly because of the
history we have, I really really want to see that. I want to see a
Link: https://lore.kernel.org/r/....
and the Cc's - or better yet, the Reviewed-by's etc - so that when I
get a pull request, it really is very obvious to me when I look at it
that others really have been involved.
So if I continue to see just
Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
at the end of the commit messages, I will not pull.
Yes, in this thread a couple of people have piped up and said that
they were part of the discussion and that they are interested, but if
I have to start asking around just to see that, then it's too little,
too late.
No more of this "it looks like David Howells did things in private". I
want links I can follow to see the discussion, and I really want to
see that others really have been involved.
Ok?
Linus
