LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Yang Shi <shy828301@gmail.com>
To: Hugh Dickins <hughd@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Shakeel Butt <shakeelb@google.com>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	Miaohe Lin <linmiaohe@huawei.com>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Michal Hocko <mhocko@suse.com>, Rik van Riel <riel@surriel.com>,
	Matthew Wilcox <willy@infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Linux MM <linux-mm@kvack.org>
Subject: Re: [PATCH 6/9] huge tmpfs: SGP_NOALLOC to stop collapse_file() on race
Date: Tue, 17 Aug 2021 13:14:16 -0700	[thread overview]
Message-ID: <CAHbLzkp9yUUtn3kKz3re1fNwmtNmP8wUo+-sEiqWXj4ryJd8gg@mail.gmail.com> (raw)
In-Reply-To: <1355343b-acf-4653-ef79-6aee40214ac5@google.com>

On Tue, Aug 17, 2021 at 1:17 AM Hugh Dickins <hughd@google.com> wrote:
>
> khugepaged's collapse_file() currently uses SGP_NOHUGE to tell
> shmem_getpage() not to try allocating a huge page, in the very unlikely
> event that a racing hole-punch removes the swapped or fallocated page as
> soon as i_pages lock is dropped.
>
> We want to consolidate shmem's huge decisions, removing SGP_HUGE and
> SGP_NOHUGE; but cannot quite persuade ourselves that it's okay to regress
> the protection in this case - Yang Shi points out that the huge page
> would remain indefinitely, charged to root instead of the intended memcg.
>
> collapse_file() should not even allocate a small page in this case: why
> proceed if someone is punching a hole?  SGP_READ is almost the right flag
> here, except that it optimizes away from a fallocated page, with NULL to
> tell caller to fill with zeroes (like a hole); whereas collapse_file()'s
> sequence relies on using a cache page.  Add SGP_NOALLOC just for this.
>
> There are too many consecutive "if (page"s there in shmem_getpage_gfp():
> group it better; and fix the outdated "bring it back from swap" comment.
>
> Signed-off-by: Hugh Dickins <hughd@google.com>

Reviewed-by: Yang Shi <shy828301@gmail.com>

> ---
>  include/linux/shmem_fs.h |  1 +
>  mm/khugepaged.c          |  2 +-
>  mm/shmem.c               | 29 +++++++++++++++++------------
>  3 files changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
> index 9b7f7ac52351..7d97b15a2f7a 100644
> --- a/include/linux/shmem_fs.h
> +++ b/include/linux/shmem_fs.h
> @@ -94,6 +94,7 @@ extern unsigned long shmem_partial_swap_usage(struct address_space *mapping,
>  /* Flag allocation requirements to shmem_getpage */
>  enum sgp_type {
>         SGP_READ,       /* don't exceed i_size, don't allocate page */
> +       SGP_NOALLOC,    /* similar, but fail on hole or use fallocated page */
>         SGP_CACHE,      /* don't exceed i_size, may allocate page */
>         SGP_NOHUGE,     /* like SGP_CACHE, but no huge pages */
>         SGP_HUGE,       /* like SGP_CACHE, huge pages preferred */
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index b0412be08fa2..045cc579f724 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1721,7 +1721,7 @@ static void collapse_file(struct mm_struct *mm,
>                                 xas_unlock_irq(&xas);
>                                 /* swap in or instantiate fallocated page */
>                                 if (shmem_getpage(mapping->host, index, &page,
> -                                                 SGP_NOHUGE)) {
> +                                                 SGP_NOALLOC)) {
>                                         result = SCAN_FAIL;
>                                         goto xa_unlocked;
>                                 }
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 740d48ef1eb5..226ac3a911e9 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1871,26 +1871,31 @@ static int shmem_getpage_gfp(struct inode *inode, pgoff_t index,
>                 return error;
>         }
>
> -       if (page)
> +       if (page) {
>                 hindex = page->index;
> -       if (page && sgp == SGP_WRITE)
> -               mark_page_accessed(page);
> -
> -       /* fallocated page? */
> -       if (page && !PageUptodate(page)) {
> +               if (sgp == SGP_WRITE)
> +                       mark_page_accessed(page);
> +               if (PageUptodate(page))
> +                       goto out;
> +               /* fallocated page */
>                 if (sgp != SGP_READ)
>                         goto clear;
>                 unlock_page(page);
>                 put_page(page);
> -               page = NULL;
> -               hindex = index;
>         }
> -       if (page || sgp == SGP_READ)
> -               goto out;
>
>         /*
> -        * Fast cache lookup did not find it:
> -        * bring it back from swap or allocate.
> +        * SGP_READ: succeed on hole, with NULL page, letting caller zero.
> +        * SGP_NOALLOC: fail on hole, with NULL page, letting caller fail.
> +        */
> +       *pagep = NULL;
> +       if (sgp == SGP_READ)
> +               return 0;
> +       if (sgp == SGP_NOALLOC)
> +               return -ENOENT;
> +
> +       /*
> +        * Fast cache lookup and swap lookup did not find it: allocate.
>          */
>
>         if (vma && userfaultfd_missing(vma)) {
> --
> 2.26.2
>

  reply	other threads:[~2021-08-17 20:18 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-17  8:03 [PATCH 0/9] huge tmpfs: shmem_is_huge() fixes and cleanups Hugh Dickins
2021-08-17  8:06 ` [PATCH 1/9] huge tmpfs: fix fallocate(vanilla) advance over huge pages Hugh Dickins
2021-08-17  8:08 ` [PATCH 2/9] huge tmpfs: fix split_huge_page() after FALLOC_FL_KEEP_SIZE Hugh Dickins
2021-08-17  8:10 ` [PATCH 3/9] huge tmpfs: remove shrinklist addition from shmem_setattr() Hugh Dickins
2021-08-17  8:12 ` [PATCH 4/9] huge tmpfs: revert shmem's use of transhuge_vma_enabled() Hugh Dickins
2021-08-17  8:14 ` [PATCH 5/9] huge tmpfs: move shmem_huge_enabled() upwards Hugh Dickins
2021-08-17  8:17 ` [PATCH 6/9] huge tmpfs: SGP_NOALLOC to stop collapse_file() on race Hugh Dickins
2021-08-17 20:14   ` Yang Shi [this message]
2021-08-17  8:19 ` [PATCH 7/9] huge tmpfs: shmem_is_huge(vma, inode, index) Hugh Dickins
2021-08-17 20:16   ` Yang Shi
2021-08-17  8:22 ` [PATCH 8/9] huge tmpfs: decide stat.st_blksize by shmem_is_huge() Hugh Dickins
2021-08-17  8:28 ` [PATCH 9/9] shmem: shmem_writepage() split unlikely i915 THP Hugh Dickins

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAHbLzkp9yUUtn3kKz3re1fNwmtNmP8wUo+-sEiqWXj4ryJd8gg@mail.gmail.com \
    --to=shy828301@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=hughd@google.com \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=linmiaohe@huawei.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@suse.com \
    --cc=mike.kravetz@oracle.com \
    --cc=riel@surriel.com \
    --cc=shakeelb@google.com \
    --cc=willy@infradead.org \
    --subject='Re: [PATCH 6/9] huge tmpfs: SGP_NOALLOC to stop collapse_file() on race' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).