diff mbox series

[19/24] shmem, swap: refactor error check on OOM or race

Message ID 20231119194740.94101-20-ryncsn@gmail.com
State New
Headers
Series Swapin path refactor for optimization and bugfix |

Commit Message

Kairui Song Nov. 19, 2023, 7:47 p.m. UTC 
  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(-)

Comments

Chris Li Nov. 20, 2023, 7:04 a.m. UTC | #1 
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
Kairui Song Nov. 20, 2023, 11:17 a.m. UTC | #2 
Chris Li <chrisl@kernel.org> 于2023年11月20日周一 15:12写道：
>
> 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.

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
diff mbox series

Patch

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;
+	if (IS_ERR_OR_NULL(page)) {
+		if (!page)
+			error = -ENOMEM;
 		else
-			return -EINVAL;
-	} else if (!page) {
-		error = -ENOMEM;
+			error = -EINVAL;
 		goto failed;
 	} else {
 		folio = page_folio(page);