Hi Kairui,
On Sun, Nov 19, 2023 at 11:49 AM Kairui Song <ryncsn@xxxxxxxxx> wrote:
>
> From: Kairui Song <kasong@xxxxxxxxxxx>
>
> It should always check if a swap entry is already swaped in on error,
> fix potential false error issue.
>
> Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> ---
> mm/shmem.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 81d129aa66d1..6154b5b68b8f 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1857,13 +1857,11 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
> page = swapin_page_non_fault(swap, gfp, mpol, ilx, fault_mm, &result);
> mpol_cond_put(mpol);
>
> - if (PTR_ERR(page) == -EBUSY) {
> - if (!shmem_confirm_swap(mapping, index, swap))
> - return -EEXIST;
Do you intentionally remove checking shmem_confirm_swap()?
I am not sure I am following.
> + if (IS_ERR_OR_NULL(page)) {
> + if (!page)
> + error = -ENOMEM;
> else
> - return -EINVAL;
> - } else if (!page) {
> - error = -ENOMEM;
> + error = -EINVAL;
The resulting code is a bit hard to read in diff. In plan source it is like:
if (IS_ERR_OR_NULL(page)) {
if (!page)
error = -ENOMEM;
else
error = -EINVAL;
goto failed;
} else {
folio = page_folio(page);
if (fault_type && result != SWAP_CACHE_HIT) {
*fault_type |= VM_FAULT_MAJOR;
count_vm_event(PGMAJFAULT);
count_memcg_event_mm(fault_mm, PGMAJFAULT);
}
}
First of all, if the first always "goto failed", the second else is not needed.
The whole else block can be flatten one indentation.
if (IS_ERR_OR_NULL(page)) {
if (!page)
error = -ENOMEM;
else
error = -EINVAL;
goto failed;
} else {
Can be rewrite as following with less indentation:
if (!page) {
error = -ENOMEM;
goto failed;
}
if (IS_ERR(page)) {
error = -EINVAL;
goto failed;
}
/* else block */
Am I missing something and misreading your code?
Chris
