Messages in this thread | | | From | Chris Li <> | Date | Sun, 19 Nov 2023 23:04:02 -0800 | Subject | Re: [PATCH 19/24] shmem, swap: refactor error check on OOM or race |
| |
Hi Kairui,
On Sun, Nov 19, 2023 at 11:49 AM Kairui Song <ryncsn@gmail.com> wrote: > > From: Kairui Song <kasong@tencent.com> > > 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@tencent.com> > --- > 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
| |