Re: [PATCH] mm: Fix __dump_page when mapping->host is not set

[Michal Hocko]

[my mbox didn't get synced completely so our emails "crossed"]

On Fri 15-03-19 10:21:18, Hugh Dickins wrote:
> On Fri, 15 Mar 2019, Oscar Salvador wrote:
> > On Fri, Mar 15, 2019 at 01:47:33PM +0100, Michal Hocko wrote:
> > > diff --git a/mm/debug.c b/mm/debug.c
> > > index 1611cf00a137..499c26d5ebe5 100644
> > > --- a/mm/debug.c
> > > +++ b/mm/debug.c
> > > @@ -78,6 +78,9 @@ void __dump_page(struct page *page, const char *reason)
> > >  	else if (PageKsm(page))
> > >  		pr_warn("ksm ");
> > >  	else if (mapping) {
> > > +		if (PageSwapCache(page))
> > > +			mapping = page_swap_info(page)->swap_file->f_mapping;
> > > +
> > >  		pr_warn("%ps ", mapping->a_ops);
> > >  		if (mapping->host->i_dentry.first) {
> > >  			struct dentry *dentry;
> > 
> > This looks like a much nicer fix, indeed.
> > I gave it a spin and it works.
> > 
> > Since the mapping is set during the swapon, I would assume that this should
> > always work for swap.
> > Although I am not sure if once you start playing with e.g zswap the picture can
> > change.
> > 
> > Let us wait for Hugh and Jan.
> > 
> > Thanks Michal
> 
> Sorry, I don't agree that Michal's more sophisticated patch is nicer:
> the appropriate patch was your original, just checking for NULL.
> 
> Though, would I be too snarky to suggest that your patch description
> would be better at 2 lines than 90?  Swap mapping->host is NULL,
> so of course __dump_page() needs to be careful about that.
> 
> I was a little disturbed to see __dump_page() now getting into dentries,
> but admit that it can sometimes be very helpful to see the name of the
> file involved; so if that is not in danger of breaking anything, okay.
> 
> It is very often useful to see if a page is PageSwapCache (typically
> because that should account for 1 of its refcount); I cannot think of
> a time when it's been useful to know the name of the underlying swap
> device (if that's indeed what f_mapping leads to: it's new to me).
> And if you need swp_type and swp_offset, they're in the raw output.
> 
> The cleverer __dump_page() tries to get, the more likely that it will
> itself crash just when you need it most. Please just keep it simple.

OK, fair enough. If we ever have anybody suggesting to follow the swap
lead then we can add it. I do not have a good use case for that right
now. Let's go with Oscar's original patch. Thanks!

Acked-by: Michal Hocko <mhocko@xxxxxxxx>
-- 
Michal Hocko
SUSE Labs