Re: [UPDATE PATCH] mm: shmem: don't truncate page if memory failure happens

[Matthew Wilcox <willy@xxxxxxxxxxxxx>]

On Sat, Nov 13, 2021 at 09:32:21PM -0800, Yang Shi wrote:
> @@ -2466,7 +2467,18 @@ 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 (ret)
> +		return ret;
> +
> +	if (*pagep && PageHWPoison(*pagep)) {
> +		unlock_page(*pagep);
> +		put_page(*pagep);
> +		ret = -EIO;

You definitely need to add:

		*pagep = NULL;

I'm not entirely convinced that you need the conditional on '*pagep'.
If we returned 0, there had better be a page at pagep!

I also think this would be clearer if written as:

	if (PageHWPoison(*pagep)) {
		unlock_page(*pagep);
		put_page(*pagep);
		*pagep = NULL;
		return -EIO;
	}

	return 0;

instead of re-using ret.  Sometimes that can make the code flow clearer,
but here, I don't think it does.

> @@ -4168,9 +4201,12 @@ struct page *shmem_read_mapping_page_gfp(struct address_space *mapping,
>  	error = shmem_getpage_gfp(inode, index, &page, SGP_CACHE,
>  				  gfp, NULL, NULL, NULL);
>  	if (error)
> -		page = ERR_PTR(error);
> -	else
> -		unlock_page(page);
> +		return ERR_PTR(error);
> +
> +	unlock_page(page);
> +	if (PageHWPoison(page))
> +		return ERR_PTR(-EIO);

Do we need to put_page() the page in this error case?