Re: [RFC v4 PATCH 0/6] Solve silent data loss caused by poisoned page cache (shmem/tmpfs)

[Yang Shi]

On Mon, Oct 18, 2021 at 10:53 PM Naoya Horiguchi
<naoya.horiguchi@xxxxxxxxx> wrote:
>
> On Thu, Oct 14, 2021 at 12:16:09PM -0700, Yang Shi wrote:
> >
> > When discussing the patch that splits page cache THP in order to offline the
> > poisoned page, Noaya mentioned there is a bigger problem [1] that prevents this
> > from working since the page cache page will be truncated if uncorrectable
> > errors happen.  By looking this deeper it turns out this approach (truncating
> > poisoned page) may incur silent data loss for all non-readonly filesystems if
> > the page is dirty.  It may be worse for in-memory filesystem, e.g. shmem/tmpfs
> > since the data blocks are actually gone.
> >
> > To solve this problem we could keep the poisoned dirty page in page cache then
> > notify the users on any later access, e.g. page fault, read/write, etc.  The
> > clean page could be truncated as is since they can be reread from disk later on.
> >
> > The consequence is the filesystems may find poisoned page and manipulate it as
> > healthy page since all the filesystems actually don't check if the page is
> > poisoned or not in all the relevant paths except page fault.  In general, we
> > need make the filesystems be aware of poisoned page before we could keep the
> > poisoned page in page cache in order to solve the data loss problem.
> >
> > To make filesystems be aware of poisoned page we should consider:
> > - The page should be not written back: clearing dirty flag could prevent from
> >   writeback.
> > - The page should not be dropped (it shows as a clean page) by drop caches or
> >   other callers: the refcount pin from hwpoison could prevent from invalidating
> >   (called by cache drop, inode cache shrinking, etc), but it doesn't avoid
> >   invalidation in DIO path.
> > - The page should be able to get truncated/hole punched/unlinked: it works as it
> >   is.
> > - Notify users when the page is accessed, e.g. read/write, page fault and other
> >   paths (compression, encryption, etc).
> >
> > The scope of the last one is huge since almost all filesystems need do it once
> > a page is returned from page cache lookup.  There are a couple of options to
> > do it:
> >
> > 1. Check hwpoison flag for every path, the most straightforward way.
> > 2. Return NULL for poisoned page from page cache lookup, the most callsites
> >    check if NULL is returned, this should have least work I think.  But the
> >    error handling in filesystems just return -ENOMEM, the error code will incur
> >    confusion to the users obviously.
> > 3. To improve #2, we could return error pointer, e.g. ERR_PTR(-EIO), but this
> >    will involve significant amount of code change as well since all the paths
> >    need check if the pointer is ERR or not just like option #1.
> >
> > I did prototype for both #1 and #3, but it seems #3 may require more changes
> > than #1.  For #3 ERR_PTR will be returned so all the callers need to check the
> > return value otherwise invalid pointer may be dereferenced, but not all callers
> > really care about the content of the page, for example, partial truncate which
> > just sets the truncated range in one page to 0.  So for such paths it needs
> > additional modification if ERR_PTR is returned.  And if the callers have their
> > own way to handle the problematic pages we need to add a new FGP flag to tell
> > FGP functions to return the pointer to the page.
> >
> > It may happen very rarely, but once it happens the consequence (data corruption)
> > could be very bad and it is very hard to debug.  It seems this problem had been
> > slightly discussed before, but seems no action was taken at that time. [2]
> >
> > As the aforementioned investigation, it needs huge amount of work to solve
> > the potential data loss for all filesystems.  But it is much easier for
> > in-memory filesystems and such filesystems actually suffer more than others
> > since even the data blocks are gone due to truncating.  So this patchset starts
> > from shmem/tmpfs by taking option #1.
>
> Thank you for the work. I have a few comment on todo...
>
> >
> > TODO:
> > * The unpoison has been broken since commit 0ed950d1f281 ("mm,hwpoison: make
> >   get_hwpoison_page() call get_any_page()"), and this patch series make
> >   refcount check for unpoisoning shmem page fail.
>
> It's OK to leave unpoison unsolved now. I'm working on this now (revising
> v1 patch [1]), but I'm facing some race issue cauisng kernel panic with kernel
> mode page fault, so I need to solve it.
>
> [1] https://lore.kernel.org/linux-mm/20210614021212.223326-1-nao.horiguchi@xxxxxxxxx/

Thanks.

>
> > * Expand to other filesystems.  But I haven't heard feedback from filesystem
> >   developers yet.
>
> I think that hugetlbfs can be a good next target because it's similar to
> shmem in that it's in-memory filesystem.

Yeah, I agree. Will look into it later. Thanks for the suggestion.

>
> Thanks,
> Naoya Horiguchi