Re: [PATCH v4 4/5] mm/shmem: fix infinite loop when swap in shmem error at swapoff time

[Miaohe Lin <linmiaohe@huawei.com>]

On 2022/5/23 11:01, Miaohe Lin wrote:
> On 2022/5/23 7:53, HORIGUCHI NAOYA(堀口 直也) wrote:
>> On Fri, May 20, 2022 at 04:17:45PM +0800, Miaohe Lin wrote:
>>> On 2022/5/20 14:34, HORIGUCHI NAOYA(堀口 直也) wrote:
>>>> On Thu, May 19, 2022 at 08:50:29PM +0800, Miaohe Lin wrote:
>>>>> When swap in shmem error at swapoff time, there would be a infinite loop
>>>>> in the while loop in shmem_unuse_inode(). It's because swapin error is
>>>>> deliberately ignored now and thus info->swapped will never reach 0. So
>>>>> we can't escape the loop in shmem_unuse().
>>>>>
>>>>> In order to fix the issue, swapin_error entry is stored in the mapping
>>>>> when swapin error occurs. So the swapcache page can be freed and the
>>>>> user won't end up with a permanently mounted swap because a sector is
>>>>> bad. If the page is accessed later, the user process will be killed
>>>>> so that corrupted data is never consumed. On the other hand, if the
>>>>> page is never accessed, the user won't even notice it.
>>>>>
>>>>> Reported-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
>>>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>>>
>>>> Hi Miaohe,
>>>>
>>>> Thank you for the update.  I might miss something, but I still see the same
>>>> problem (I checked it on mm-everything-2022-05-19-00-03 + this patchset).
>>>
>>> I was testing this patch on my 5.10 kernel. I reproduced the problem in my env and
>>> fixed it. It seems there might be some critical difference though I checked that by
>>> reviewing the code... Sorry. :(
>>>
>>>>
>>>> This patch has the effect to change the return value of shmem_swapin_folio(),
>>>> -EIO (without this patch) to -EEXIST (with this patch).
>>>
>>> In fact, I didn't change the return value from -EIO to -EEXIST:
>>>
>>> @@ -1762,6 +1799,8 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
>>>  failed:
>>>  	if (!shmem_confirm_swap(mapping, index, swap))
>>>  		error = -EEXIST;
>>> +	if (error == -EIO)
>>> +		shmem_set_folio_swapin_error(inode, index, folio, swap)
>>>
>>>> But shmem_unuse_swap_entries() checks neither, so no change from caller's view point.
>>>> Maybe breaking in errors (rather than ENOMEM) in for loop in shmem_unuse_swap_entries()
>>>> solves the issue?  I briefly checked with the below change, then swapoff can return
>>>> with failure.
>>>>
>>>> @@ -1222,7 +1222,7 @@ static int shmem_unuse_swap_entries(struct inode *inode,
>>>>                         folio_put(folio);
>>>>                         ret++;
>>>>                 }
>>>> -               if (error == -ENOMEM)
>>>> +               if (error < 0)
>>>>                         break;
>>>>                 error = 0;
>>>>         }
>>>
>>> Yes, this is the simplest and straightforward way to fix the issue. But it has the side effect
>>> that user will end up with a permanently mounted swap just because a sector is bad. That might
>>> be somewhat unacceptable?
>>
>> Ah, you're right, swapoff should return with success instead of with
>> failure.  I tried the fix in your another email, and that makes swapoff
>> return with success, so your fix looks better than mine.
> 
> I reproduced the deadloop issues when swapin error occurs at swapoff time in my linux-next-next-20220520 env,
> and I found this patch could solve the issue now with the fix in my another email.
> 
> BTW: When I use dm-dust to inject the swapin IO error, I don't see non-uptodate folio when shmem_swapin_folio
> and swapoff succeeds. There might be some issues around that module (so I resort to the another way to inject
> the swapin error), but the patch itself works anyway. ;)

Sorry, the reason I don't see non-uptodate folio when shmem_swapin_folio is that all the shmem pages are still
in the swapcache. They're not read from disk so there is no really IO error. :) When they're indeed freed, the
deadloop issue occurs.

I am thinking about extending the function of MADV_PAGEOUT to free the swapcache page. The page resides in the
swapcache does not save the system memory anyway. And this could help test the swapin behavior. But I'm not
sure whether it's needed.

Thanks! ;)

> 
>>
>> Thanks,
> 
> Thanks a lot!
> 
>> Naoya Horiguchi
>>
>