Re: [PATCH] MM: minor improvements to readahead documentation

[Matthew Wilcox <willy@infradead.org>]

On Mon, Apr 04, 2022 at 02:10:51PM +1000, NeilBrown wrote:
> >   * Readahead is triggered when an application read request (whether a
> > - * systemcall or a page fault) finds that the requested page is not in
> > + * system call or a page fault) finds that the requested folio is not in
> >   * the page cache, or that it is in the page cache and has the
> > - * %PG_readahead flag set.  This flag indicates that the page was loaded
> > - * as part of a previous read-ahead request and now that it has been
> > - * accessed, it is time for the next read-ahead.
> > + * readahead flag set.  This flag indicates that the folio was read
> 
> Ugh.  Why don't you like %PG_readahead?   I absolutely loath the
> practice of hiding flags inside accessor functions, and hiding the truth
> in documentation is just as bad.  It all makes grepping that much
> harder.
> I would MUCH prefer that the %PG_ were restored.  Please.

I absolutely loathe it that there are references to PG_* anywhere
outside page-flags.h.  We have the abstraction layer, we want people
to use it, and we shouldn't needlessly multiply entities by referring
to the implementation of the abstraction.  I remove references to PG_
flags wherever I find them.  I agree that grepping for page/folio flags
doesn't work, and it's something I spend a lot of time thinking about.
In particular, I want to produce decent kernel-doc for them.

> > - * In the last two cases, the page should be unlocked to indicate that
> > - * the read attempt has failed.  In the first case the page will be
> > - * unlocked by the caller.
> > + * In the last two cases, the folio should be unlocked by the filesystem
> > + * to indicate that the read attempt has failed.  In the first case the
> > + * folio will be unlocked by the VFS.
> 
> VFS??  The code is in mm/readahead.c, not in fs/*.c
> Why didn't you like "caller" ??

I view mm/readahead.c, mm/filemap.c and mm/page-writeback.c as part
of the VFS more than as part of the VM.  But that's something that
reasonable people can disagree on.  I think from the point of view of
the filesystem author, it's all VFS.