Re: BUG in nfs_read_folio() when tracing is enabled

[Christoph Hellwig <hch@infradead.org>]

On Tue, Jul 09, 2024 at 11:45:11AM -0400, David Wysochanski wrote:
> > > So Dave, I haven't tested the patch you posted a couple
> > > days ago, because there hasn't been a clear answer about
> > > whether nfs_read_folio() needs to protect itself against
> > > the ->mapping changing, in which case, that's probably
> > > a better fix.
> >
> > ->read_folio is called with the folio locked and only unlocks it
> > on I/O completion, so it doesn't really need any protection.  So the
> > patch to simply move the trace point to before unlocking the folio
> > should fix the issue.
> >
> I didn't see this so maybe you sent it privately or I missed a message.
> 
> > Alternatively we could just use the mapping from the inode variable
> > and pass it in.
> >
> I'm not sure I follow - are you suggesting fixing the tracepoint?

I'm suggesting to fix the tracepoint folio.  I though I had something
out, but I guess it never made it.

> Regardless of possible tracepoint movement or other fixes,
> nfs_folio_length() needs to be patched because it should
> handle folio->mapping == NULL.  Ditto for nfs_page_length().

nfs_page_length is completely unused and I've already sent a patch
to remove it.  nfs_folio_length is only sanely usable with the
folio either locked or under writeback.  So adding this random
check there is probably not a good idea, but an audit of the callers
would be very useful.