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
