Re: [PATCH] [13/16] HWPOISON: The high level memory error handler in the VM v3

[Nick Piggin]

On Tue, Jun 02, 2009 at 02:47:57PM +0200, Andi Kleen wrote:
> On Tue, Jun 02, 2009 at 02:00:42PM +0200, Nick Piggin wrote:
> > not a big deal and just avoids duplicating code. I attached an
> > (untested) patch.
> 
> Thanks. But the function in the patch is not doing the same what
> the me_pagecache_clean/dirty are doing. For once there is no error
> checking, as in the second try_to_release_page()
> 
> Then it doesn't do all the IO error and missing mapping handling.

Obviously I don't mean just use that single call for the entire
handler. You can set the EIO bit or whatever you like. The
"error handling" you have there also seems strange. You could
retain it, but the page is assured to be removed from pagecache.

 
> The page_mapped() check is useless because the pages are not 
> mapped here etc.

That's OK, it is a core part of the protocol to prevent
truncated pages from being mapped, so I like it to be in
that function.

(you are also doing extraneous page_mapped tests in your handler,
so surely your concern isn't from the perspective of this
error handler code)


> We could probably call truncate_complete_page(), but then
> we would also need to duplicate most of the checking outside
> the function anyways and there wouldn't be any possibility
> to share the clean/dirty variants. If you insist I can
> do it, but I think it would be significantly worse code
> than before and I'm reluctant to do that.

I can write you the patch for that too if you like.

 
> I don't also really see what the big deal is of just
> calling these few functions directly. After all we're not
> truncating here and they're all already called from other files.
>
> > > > No, it seems rather insane to do something like this here that no other
> > > > code in the mm ever does.
> > > 
> > > Just because the rest of the VM doesn't do it doesn't mean it might make sense.
> > 
> > It is going to be possible to do it somehow surely, but it is insane
> > to try to add such constraints to the VM to close a few small windows
> 
> We don't know currently if they are small. If they are small I would
> agree with you, but that needs numbers. That said fancy writeback handling
> is currently not on my agenda.

Yes, writeback pages are very limited, a tiny number at any time and
the faction gets relatively smaller as total RAM size gets larger.


> > if you already have other large ones.
> 
> That's unclear too.

You can't do much about most kernel pages, and dirty metadata pages
are both going to cause big problems. User pagetable pages. Lots of
stuff.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/