Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 12 Oct 2021 09:57:02 +0800
From:   Peter Xu <peterx@redhat.com>
To:     Yang Shi <shy828301@gmail.com>
Cc:     naoya.horiguchi@nec.com, hughd@google.com,
        kirill.shutemov@linux.intel.com, willy@infradead.org,
        osalvador@suse.de, akpm@linux-foundation.org, linux-mm@kvack.org,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [v3 PATCH 4/5] mm: shmem: don't truncate page if memory failure
 happens
Message-ID: <YWTrbgf0kpwayWHL@t490s>
References: <20210930215311.240774-1-shy828301@gmail.com>
 <20210930215311.240774-5-shy828301@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20210930215311.240774-5-shy828301@gmail.com>
Precedence: bulk

On Thu, Sep 30, 2021 at 02:53:10PM -0700, Yang Shi wrote:
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 88742953532c..75c36b6a405a 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2456,6 +2456,7 @@ shmem_write_begin(struct file *file, struct address_space *mapping,
>  	struct inode *inode = mapping->host;
>  	struct shmem_inode_info *info = SHMEM_I(inode);
>  	pgoff_t index = pos >> PAGE_SHIFT;
> +	int ret = 0;
>  
>  	/* i_rwsem is held by caller */
>  	if (unlikely(info->seals & (F_SEAL_GROW |
> @@ -2466,7 +2467,17 @@ shmem_write_begin(struct file *file, struct address_space *mapping,
>  			return -EPERM;
>  	}
>  
> -	return shmem_getpage(inode, index, pagep, SGP_WRITE);
> +	ret = shmem_getpage(inode, index, pagep, SGP_WRITE);
> +
> +	if (*pagep) {
> +		if (PageHWPoison(*pagep)) {
> +			unlock_page(*pagep);
> +			put_page(*pagep);
> +			ret = -EIO;
> +		}
> +	}
> +
> +	return ret;
>  }
>  
>  static int
> @@ -2555,6 +2566,11 @@ static ssize_t shmem_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  			unlock_page(page);
>  		}
>  
> +		if (page && PageHWPoison(page)) {
> +			error = -EIO;
> +			break;
> +		}
> +
>  		/*
>  		 * We must evaluate after, since reads (unlike writes)
>  		 * are called without i_rwsem protection against truncate

[...]

> @@ -4193,6 +4216,10 @@ struct page *shmem_read_mapping_page_gfp(struct address_space *mapping,
>  		page = ERR_PTR(error);
>  	else
>  		unlock_page(page);
> +
> +	if (PageHWPoison(page))
> +		page = ERR_PTR(-EIO);
> +
>  	return page;
>  #else
>  	/*
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index 7a9008415534..b688d5327177 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -233,6 +233,11 @@ static int mcontinue_atomic_pte(struct mm_struct *dst_mm,
>  		goto out;
>  	}
>  
> +	if (PageHWPoison(page)) {
> +		ret = -EIO;
> +		goto out_release;
> +	}
> +
>  	ret = mfill_atomic_install_pte(dst_mm, dst_pmd, dst_vma, dst_addr,
>  				       page, false, wp_copy);
>  	if (ret)
> -- 
> 2.26.2
> 

These are shmem_getpage_gfp() call sites:

  shmem_getpage[151]             return shmem_getpage_gfp(inode, index, pagep, sgp,
  shmem_fault[2112]              err = shmem_getpage_gfp(inode, vmf->pgoff, &vmf->page, SGP_CACHE,
  shmem_read_mapping_page_gfp[4188] error = shmem_getpage_gfp(inode, index, &page, SGP_CACHE,

These are further shmem_getpage() call sites:

  collapse_file[1735]            if (shmem_getpage(mapping->host, index, &page,
  shmem_undo_range[965]          shmem_getpage(inode, start - 1, &page, SGP_READ);
  shmem_undo_range[980]          shmem_getpage(inode, end, &page, SGP_READ);
  shmem_write_begin[2467]        return shmem_getpage(inode, index, pagep, SGP_WRITE);
  shmem_file_read_iter[2544]     error = shmem_getpage(inode, index, &page, sgp);
  shmem_fallocate[2733]          error = shmem_getpage(inode, index, &page, SGP_FALLOC);
  shmem_symlink[3079]            error = shmem_getpage(inode, 0, &page, SGP_WRITE);
  shmem_get_link[3120]           error = shmem_getpage(inode, 0, &page, SGP_READ);
  mcontinue_atomic_pte[235]      ret = shmem_getpage(inode, pgoff, &page, SGP_READ);

Wondering whether this patch covered all of them.

This also reminded me that whether we should simply fail shmem_getpage_gfp()
directly, then all above callers will get a proper failure, rather than we do
PageHWPoison() check everywhere?

-- 
Peter Xu