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.
