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

Nick Piggin
2009-05-28T12:24:10Z

On Thu, May 28, 2009 at 05:59:34PM +0800, Wu Fengguang wrote:
> Hi Nick,
>
> > > + /*
> > > + * remove_from_page_cache assumes (mapping && !mapped)
> > > + */
> > > + if (page_mapping(p) && !page_mapped(p)) {
> > > + remove_from_page_cache(p);
> > > + page_cache_release(p);
> > > + }
> >
> > remove_mapping would probably be a better idea. Otherwise you can
> > probably introduce pagecache removal vs page fault races which
> > will make the kernel bug.
>
> We use remove_mapping() at first, then discovered that it made strong
> assumption on page_count=2.
>
> I guess it is safe from races since we are locking the page?

Yes it probably should (although you will lose get_user_pages data, but
I guess that's the aim anyway).

But I just don't like this one file having all that required knowledge
(and few comments) of all the files in mm/. If you want to get rid
of the page and don't care what it's count or dirtyness is, then
truncate_inode_pages_range is the correct API to use.

(or you could extract out some of it so you can call it directly on
individual locked pages, if that helps).

> > > + }
> > > +
> > > + me_pagecache_clean(p);
> > > +
> > > + /*
> > > + * Did the earlier release work?
> > > + */
> > > + if (page_has_private(p) && !try_to_release_page(p, GFP_NOIO))
> > > + return FAILED;
> > > +
> > > + return RECOVERED;
> > > +}
> > > +
> > > +/*
> > > + * Clean and dirty swap cache.
> > > + */
> > > +static int me_swapcache_dirty(struct page *p)
> > > +{
> > > + ClearPageDirty(p);
> > > +
> > > + if (!isolate_lru_page(p))
> > > + page_cache_release(p);
> > > +
> > > + return DELAYED;
> > > +}
> > > +
> > > +static int me_swapcache_clean(struct page *p)
> > > +{
> > > + ClearPageUptodate(p);
> > > +
> > > + if (!isolate_lru_page(p))
> > > + page_cache_release(p);
> > > +
> > > + delete_from_swap_cache(p);
> > > +
> > > + return RECOVERED;
> > > +}
> >
> > All these handlers are quite interesting in that they need to
> > know about most of the mm. What are you trying to do in each
> > of them would be a good idea to say, and probably they should
> > rather go into their appropriate files instead of all here
> > (eg. swapcache stuff should go in mm/swap_state for example).
>
> Yup, they at least need more careful comments.
>
> Dirty swap cache page is tricky to handle. The page could live both in page
> cache and swap cache(ie. page is freshly swapped in). So it could be referenced
> concurrently by 2 types of PTEs: one normal PTE and another swap PTE. We try to
> handle them consistently by calling try_to_unmap(TTU_IGNORE_HWPOISON) to convert
> the normal PTEs to swap PTEs, and then
> - clear dirty bit to prevent IO
> - remove from LRU
> - but keep in the swap cache, so that when we return to it on
> a later page fault, we know the application is accessing
> corrupted data and shall be killed (we installed simple
> interception code in do_swap_page to catch it).

OK this is the point I was missing.

Should all be commented and put into mm/swap_state.c (or somewhere that
Hugh prefers).

> Clean swap cache pages can be directly isolated. A later page fault will bring
> in the known good data from disk.

OK, but why do you ClearPageUptodate if it is just to be deleted from
swapcache anyway?

> > You haven't waited on writeback here AFAIKS, and have you
> > *really* verified it is safe to call delete_from_swap_cache?
>
> Good catch. I'll soon submit patches for handling the under
> read/write IO pages. In this patchset they are simply ignored.

Well that's quite important ;) I would suggest you just wait_on_page_writeback.
It is simple and should work. _Unless_ you can show it is a big problem that
needs equivalently big mes to fix ;)