Date:   Sun, 14 Nov 2021 15:28:08 +0000
From:   Matthew Wilcox <willy@infradead.org>
To:     Yang Shi <shy828301@gmail.com>
Cc:     torvalds@linux-foundation.org, arnd@arndb.de, hughd@google.com,
        kirill.shutemov@linux.intel.com, naoya.horiguchi@nec.com,
        osalvador@suse.de, peterx@redhat.com, ajaygargnsit@gmail.com,
        songmuchun@bytedance.com, akpm@linux-foundation.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [UPDATE PATCH] mm: shmem: don't truncate page if memory failure
 happens
Message-ID: <YZErCLzSixIwTr6C@casper.infradead.org>
References: <20211114053221.315753-1-shy828301@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211114053221.315753-1-shy828301@gmail.com>
Precedence: bulk

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?