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.
