On Mon, Jan 01, 2024 at 12:02:56AM +0000, Matthew Wilcox wrote: > On Sun, Dec 31, 2023 at 12:13:49PM -0800, Darrick J. Wong wrote: > > + error = xfile_stat(xf, &sb); > > + if (error) > > + return error; > > + > > + printk(KERN_ALERT "xfile ino 0x%lx isize 0x%llx dump:", inode->i_ino, > > + sb.size); > > + pflags = memalloc_nofs_save(); > > Hm, why? What makes it a bad idea to call back into the filesysteam at > this point? I don't want xfile_dump to invoke direct reclaim which will then call back into xfs because scrub (or any xfile caller) might already be holding the an ILOCK. Granted it's /probably/ redundant since the scrub transaction will have already memalloc_nofs_save'd. But as this is a debug function, I figured it was better to burn an unsigned long to prevent making problems worse... > > + page = shmem_read_mapping_page_gfp(mapping, > > + datapos >> PAGE_SHIFT, __GFP_NOWARN); > > This GFP flag looks wrong. Why can't we use GFP_KERNEL here? I think that's an omission. > I'm also not thrilled about the use of page APIs instead of folio APIs, > but given how long this patchset has been in development, I understand why > you didn't start out with folio APIs. It's not a blocker by any means. Heh. Yeah, I really wish xfiles had been merged before you started the folioization instead of adding to legacy code creeping. > I can come through and convert it later when I decide that it's finally > time to get rid of shmem_read_mapping_page_gfp(), which is going to take > a big gulp because it now means touching GPU drivers ... Hehhehee yep. --D
