Re: [PATCH 02/11] readahead: push invalidate_lock out of page_cache_ra_unbounded

[Eric Biggers <ebiggers@kernel.org>]

On Mon, Feb 02, 2026 at 04:17:55PM +0100, Christoph Hellwig wrote:
> On Mon, Feb 02, 2026 at 03:11:45PM +0000, Matthew Wilcox wrote:
> > On Mon, Feb 02, 2026 at 07:06:31AM +0100, Christoph Hellwig wrote:
> > > +++ b/fs/f2fs/file.c
> > > @@ -4418,7 +4418,9 @@ static int redirty_blocks(struct inode *inode, pgoff_t page_idx, int len)
> > >  	pgoff_t redirty_idx = page_idx;
> > >  	int page_len = 0, ret = 0;
> > >  
> > > +	filemap_invalidate_lock_shared(mapping);
> > >  	page_cache_ra_unbounded(&ractl, len, 0);
> > > +	filemap_invalidate_unlock_shared(mapping);
> > 
> > Why is f2fs calling page_cache_ra_unbounded() here?
> 
> From tracing the callers is seems to be able to be called from the
> garbage collector, which might have to move fsverity files.  Not sure if
> that was the reason or is incidental.
> 
> (using the pagecache for GC is generally a very bad idea, and there is
> at least one academic paper showing it is a huge performance problem in
> f2fs, and my initial attempts at using the pagecache for GC in zoned XFS
> also showed horrible results)
> 
> > >  	unsigned int nofs = memalloc_nofs_save();
> > >  
> > > +	lockdep_assert_held_read(&mapping->invalidate_lock);
> > 
> > Hm, why are we asserting that it's not write-locked?  For the
> > purposes of this function, I'd think we want to just
> > lockdep_assert_held()?
> 
> Fine with me.
> 
> > In the tree I'm looking at, there are also calls to
> > page_cache_ra_unbounded() in fs/ext4/verity.c and fs/f2fs/verity.c
> > which probably need the lock taken too?
> 
> I consolidated those into the single call in fs/verity/pagecache.c
> in the previous iteration of this series, and Eric merged the
> first few patches including that one into the fsverity tree.

I changed both instances of lockdep_assert_held_read() to
lockdep_assert_held() when applying.

- Eric