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/
> * 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.
Thanks,
Naoya Horiguchi
