On Fri, Jun 10, 2022 at 12:56:48PM -0700, Linus Torvalds wrote: > On Fri, Jun 10, 2022 at 12:22 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > > > - Don't release a folio while it's still locked > > Ugh. > > I *hate* this patch. It's just incredibly broken. > > Yes, I've pulled this, but I have looked at that readahead_folio() > function before, and I have despised it before, but this patch really > drove home how incredibly broken that function is. > > Honestly, readahead_folio() returns a folio *AFTER* it has dropped the > ref to that folio. OK, you caught me. I realised (a little too late) that the rules around refcounts in ->readpage and ->readahead are different, and that creates pain for people writing filesystems. For ->readahead, I stuck with the refcount model that was in ->readpages (there is an extra refcount on the folio and the filesystem must put it before it returns). But I don't want to change the refcounting rules on a method without changing something else about the method, because trying to find a missing refcount change is misery. Anyway, my cunning thought was that if I bundle the change to the refcount rule with the change from readahead_page() to readahead_folio(), once all filesystems are converted to readahead_folio(), I can pull the refcount game out of readahead_folio() and do it in the caller where it belongs, all transparent to the filesystems. I think it's worth doing, because it's two fewer atomic ops per folio that we read from a file. But I didn't think through the transition process clearly enough, and right now it's a mess. How would you like me to proceed? (I don't think the erofs code has a bug because it doesn't remove the folio from the pagecache while holding the lock -- the folio lock prevents anyone _else_ from removing the folio from the pagecache, so there must be a reference on the folio up until erofs calls folio_unlock()).
