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

["NeilBrown" <neilb@suse.de>]

On Mon, 04 Apr 2022, Matthew Wilcox wrote:
> 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.

Yes, we have an abstraction layer - but WHY do you have an abstraction
layer?  I can't see that it adds anything other than obfuscation.

Do you WANT to keep the learning curve nice and steep?

> 
> > > - * 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.
> 
You didn't answer the second question.

NeilBrown