Re: [PATCH 1/9] xfs: dump xfiles for debugging purposes

Darrick J. Wong
2024-01-03T01:52:21Z

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