Chris Li <chrisl@xxxxxxxxxx> 于2023年11月20日周一 15:12写道：
>
> 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.
Hi, Chris, thanks for the review.
This was also called under failed: label so I think there is no need
to keep it here.
>
> > + 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.
Yes, thanks for the suggestion.
>
> 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?
Your code looks cleaner :)
This patch is trying to clean up the error path after the helper
refactor, and your suggestions are very helpful, thanks!
>
> Chris
