Re: [PATCH v1] mm, hwpoison: enable error handling on shmem thp

[Matthew Wilcox]

On Thu, Mar 11, 2021 at 02:00:40PM -0800, Hugh Dickins wrote:
> On Thu, 11 Mar 2021, Jue Wang wrote:
> > In our experiment with SHMEM THPs, later accesses resulted in a zero
> > page allocated instead of a SIGBUS with BUS_MCEERR_AR reported by the
> > page fault handler. That part might be an opportunity to prevent some
> > silent data corruption just in case.
> 
> Thanks for filling in more detail, Jue: I understand better now.
> 
> Maybe mm/shmem.c is wrong to be using generic_error_remove_page(),
> the function which punches a hole on memory-failure.
> 
> That works well for filesystems backed by storage (at least when the
> page had not been modified), because it does not (I think) actually
> punch a hole in the stored object; and the next touch at that offset of
> the file will allocate a new cache page to be filled from good storage.
> 
> But in the case of shmem (if we ignore the less likely swap cache case)
> there is no storage to read back good data from, so the next touch just
> fills a new cache page with zeroes (as you report above).
> 
> I don't know enough of the philosophy of memory-failure to say, but
> I can see there's an argument for leaving the bad page in cache, to
> give SIGBUS or EFAULT or EIO (whether by observation of PageHWPoison,
> or by another MCE) to whoever accesses it - until the file or that
> part of it is deleted (then that page never returned to use again).

I think you have it right here.  If the page hasn't been modified since
being read in from swap, it's perfectly fine to discard it (because the
data can be read back from swap if accessed again).  But if it has been
modified, the user has lost data and must be made aware of this.

It seems to me that other filesystems also get this wrong.  If the page
is dirty (eg for ext2/ext4/xfs), we'll silently throw away the user's
data and reload it from storage.  That's spelled "silent data corruption".

So I think this is part of the solution here:

+++ b/mm/truncate.c
@@ -236,6 +236,8 @@ int generic_error_remove_page(struct address_space *mapping, struct page *page)
         */
        if (!S_ISREG(mapping->host->i_mode))
                return -EIO;
+       if (PageDirty(page))
+               return -EBUSY;
        return truncate_inode_page(mapping, page);
 }
 EXPORT_SYMBOL(generic_error_remove_page);

Things I don't know ...

 - Does shmem mark pages dirty in the right circumstances for this to work?
 - How does memory-failure cope with an -EBUSY return from
   ->error_remove_page() today?  It seems to treat all errors the same
   and return MF_FAILED, but I don't know whether that's going to do
   the right thing.
 - Do we correctly decline to evict a HWPoison page from the page cache
   until the page is truncated (hole punched, whatever).