[01/16] huge tmpfs: fix fallocate(vanilla) advance over huge pages

Commit Message

Hugh Dickins July 30, 2021, 7:25 a.m. UTC

shmem_fallocate() goes to a lot of trouble to leave its newly allocated
pages !Uptodate, partly to identify and undo them on failure, partly to
leave the overhead of clearing them until later.  But the huge page case
did not skip to the end of the extent, walked through the tail pages one
by one, and appeared to work just fine: but in doing so, cleared and
Uptodated the huge page, so there was no way to undo it on failure.

Now advance immediately to the end of the huge extent, with a comment on
why this is more than just an optimization.  But although this speeds up
huge tmpfs fallocation, it does leave the clearing until first use, and
some users may have come to appreciate slow fallocate but fast first use:
if they complain, then we can consider adding a pass to clear at the end.

Fixes: 800d8c63b2e9 ("shmem: add huge pages support")
Signed-off-by: Hugh Dickins <[e-mail]>
---
 mm/shmem.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Comments

Yang Shi July 30, 2021, 9:36 p.m. UTC | #1

On Fri, Jul 30, 2021 at 12:25 AM Hugh Dickins <[e-mail]> wrote:
>
> shmem_fallocate() goes to a lot of trouble to leave its newly allocated
> pages !Uptodate, partly to identify and undo them on failure, partly to
> leave the overhead of clearing them until later.  But the huge page case
> did not skip to the end of the extent, walked through the tail pages one
> by one, and appeared to work just fine: but in doing so, cleared and
> Uptodated the huge page, so there was no way to undo it on failure.
>
> Now advance immediately to the end of the huge extent, with a comment on
> why this is more than just an optimization.  But although this speeds up
> huge tmpfs fallocation, it does leave the clearing until first use, and
> some users may have come to appreciate slow fallocate but fast first use:
> if they complain, then we can consider adding a pass to clear at the end.
>
> Fixes: 800d8c63b2e9 ("shmem: add huge pages support")
> Signed-off-by: Hugh Dickins <[e-mail]>

Reviewed-by: Yang Shi <[e-mail]>

A nit below:

> ---
>  mm/shmem.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 70d9ce294bb4..0cd5c9156457 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2736,7 +2736,7 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
>         inode->i_private = &shmem_falloc;
>         spin_unlock(&inode->i_lock);
>
> -       for (index = start; index < end; index++) {
> +       for (index = start; index < end; ) {
>                 struct page *page;
>
>                 /*
> @@ -2759,13 +2759,26 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
>                         goto undone;
>                 }
>
> +               index++;
> +               /*
> +                * Here is a more important optimization than it appears:
> +                * a second SGP_FALLOC on the same huge page will clear it,
> +                * making it PageUptodate and un-undoable if we fail later.
> +                */
> +               if (PageTransCompound(page)) {
> +                       index = round_up(index, HPAGE_PMD_NR);
> +                       /* Beware 32-bit wraparound */
> +                       if (!index)
> +                               index--;
> +               }
> +
>                 /*
>                  * Inform shmem_writepage() how far we have reached.
>                  * No need for lock or barrier: we have the page lock.
>                  */
> -               shmem_falloc.next++;
>                 if (!PageUptodate(page))
> -                       shmem_falloc.nr_falloced++;
> +                       shmem_falloc.nr_falloced += index - shmem_falloc.next;
> +               shmem_falloc.next = index;

This also fixed the wrong accounting of nr_falloced, so it should be
able to avoid returning -ENOMEM prematurely IIUC. Is it worth
mentioning in the commit log?

>
>                 /*
>                  * If !PageUptodate, leave it that way so that freeable pages
> --
> 2.26.2
>

Hugh Dickins Aug. 1, 2021, 3:38 a.m. UTC | #2

On Fri, 30 Jul 2021, Yang Shi wrote:
> On Fri, Jul 30, 2021 at 12:25 AM Hugh Dickins <[e-mail]> wrote:
> >
> > shmem_fallocate() goes to a lot of trouble to leave its newly allocated
> > pages !Uptodate, partly to identify and undo them on failure, partly to
> > leave the overhead of clearing them until later.  But the huge page case
> > did not skip to the end of the extent, walked through the tail pages one
> > by one, and appeared to work just fine: but in doing so, cleared and
> > Uptodated the huge page, so there was no way to undo it on failure.
> >
> > Now advance immediately to the end of the huge extent, with a comment on
> > why this is more than just an optimization.  But although this speeds up
> > huge tmpfs fallocation, it does leave the clearing until first use, and
> > some users may have come to appreciate slow fallocate but fast first use:
> > if they complain, then we can consider adding a pass to clear at the end.
> >
> > Fixes: 800d8c63b2e9 ("shmem: add huge pages support")
> > Signed-off-by: Hugh Dickins <[e-mail]>
> 
> Reviewed-by: Yang Shi <[e-mail]>

Many thanks for reviewing so many of these.

> 
> A nit below:
> 
> > ---
> >  mm/shmem.c | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 70d9ce294bb4..0cd5c9156457 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -2736,7 +2736,7 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
> >         inode->i_private = &shmem_falloc;
> >         spin_unlock(&inode->i_lock);
> >
> > -       for (index = start; index < end; index++) {
> > +       for (index = start; index < end; ) {
> >                 struct page *page;
> >
> >                 /*
> > @@ -2759,13 +2759,26 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
> >                         goto undone;
> >                 }
> >
> > +               index++;
> > +               /*
> > +                * Here is a more important optimization than it appears:
> > +                * a second SGP_FALLOC on the same huge page will clear it,
> > +                * making it PageUptodate and un-undoable if we fail later.
> > +                */
> > +               if (PageTransCompound(page)) {
> > +                       index = round_up(index, HPAGE_PMD_NR);
> > +                       /* Beware 32-bit wraparound */
> > +                       if (!index)
> > +                               index--;
> > +               }
> > +
> >                 /*
> >                  * Inform shmem_writepage() how far we have reached.
> >                  * No need for lock or barrier: we have the page lock.
> >                  */
> > -               shmem_falloc.next++;
> >                 if (!PageUptodate(page))
> > -                       shmem_falloc.nr_falloced++;
> > +                       shmem_falloc.nr_falloced += index - shmem_falloc.next;
> > +               shmem_falloc.next = index;
> 
> This also fixed the wrong accounting of nr_falloced, so it should be
> able to avoid returning -ENOMEM prematurely IIUC. Is it worth
> mentioning in the commit log?

It took me a long time to see your point there: ah yes, because it made
the whole huge page Uptodate when it reached the first tail, there would
have been only one nr_falloced++ for the whole of the huge page: well
spotted, thanks, I hadn't realized that.

Though I'm not so sure about your premature -ENOMEM: because once it has
made the huge page Uptodate, the other end (shmem_writepage()) will not
be incrementing nr_unswapped at all: so -ENOMEM would have been deferred
rather than premature, wouldn't it?

Add a comment on this in the commit log: yes, I guess so, but I haven't
worked out what to write yet.

Hugh

> 
> >
> >                 /*
> >                  * If !PageUptodate, leave it that way so that freeable pages
> > --
> > 2.26.2

Yang Shi Aug. 2, 2021, 8:36 p.m. UTC | #3

On Sat, Jul 31, 2021 at 8:38 PM Hugh Dickins <[e-mail]> wrote:
>
> On Fri, 30 Jul 2021, Yang Shi wrote:
> > On Fri, Jul 30, 2021 at 12:25 AM Hugh Dickins <[e-mail]> wrote:
> > >
> > > shmem_fallocate() goes to a lot of trouble to leave its newly allocated
> > > pages !Uptodate, partly to identify and undo them on failure, partly to
> > > leave the overhead of clearing them until later.  But the huge page case
> > > did not skip to the end of the extent, walked through the tail pages one
> > > by one, and appeared to work just fine: but in doing so, cleared and
> > > Uptodated the huge page, so there was no way to undo it on failure.
> > >
> > > Now advance immediately to the end of the huge extent, with a comment on
> > > why this is more than just an optimization.  But although this speeds up
> > > huge tmpfs fallocation, it does leave the clearing until first use, and
> > > some users may have come to appreciate slow fallocate but fast first use:
> > > if they complain, then we can consider adding a pass to clear at the end.
> > >
> > > Fixes: 800d8c63b2e9 ("shmem: add huge pages support")
> > > Signed-off-by: Hugh Dickins <[e-mail]>
> >
> > Reviewed-by: Yang Shi <[e-mail]>
>
> Many thanks for reviewing so many of these.
>
> >
> > A nit below:
> >
> > > ---
> > >  mm/shmem.c | 19 ++++++++++++++++---
> > >  1 file changed, 16 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/mm/shmem.c b/mm/shmem.c
> > > index 70d9ce294bb4..0cd5c9156457 100644
> > > --- a/mm/shmem.c
> > > +++ b/mm/shmem.c
> > > @@ -2736,7 +2736,7 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
> > >         inode->i_private = &shmem_falloc;
> > >         spin_unlock(&inode->i_lock);
> > >
> > > -       for (index = start; index < end; index++) {
> > > +       for (index = start; index < end; ) {
> > >                 struct page *page;
> > >
> > >                 /*
> > > @@ -2759,13 +2759,26 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
> > >                         goto undone;
> > >                 }
> > >
> > > +               index++;
> > > +               /*
> > > +                * Here is a more important optimization than it appears:
> > > +                * a second SGP_FALLOC on the same huge page will clear it,
> > > +                * making it PageUptodate and un-undoable if we fail later.
> > > +                */
> > > +               if (PageTransCompound(page)) {
> > > +                       index = round_up(index, HPAGE_PMD_NR);
> > > +                       /* Beware 32-bit wraparound */
> > > +                       if (!index)
> > > +                               index--;
> > > +               }
> > > +
> > >                 /*
> > >                  * Inform shmem_writepage() how far we have reached.
> > >                  * No need for lock or barrier: we have the page lock.
> > >                  */
> > > -               shmem_falloc.next++;
> > >                 if (!PageUptodate(page))
> > > -                       shmem_falloc.nr_falloced++;
> > > +                       shmem_falloc.nr_falloced += index - shmem_falloc.next;
> > > +               shmem_falloc.next = index;
> >
> > This also fixed the wrong accounting of nr_falloced, so it should be
> > able to avoid returning -ENOMEM prematurely IIUC. Is it worth
> > mentioning in the commit log?
>
> It took me a long time to see your point there: ah yes, because it made
> the whole huge page Uptodate when it reached the first tail, there would
> have been only one nr_falloced++ for the whole of the huge page: well
> spotted, thanks, I hadn't realized that.
>
> Though I'm not so sure about your premature -ENOMEM: because once it has
> made the huge page Uptodate, the other end (shmem_writepage()) will not
> be incrementing nr_unswapped at all: so -ENOMEM would have been deferred
> rather than premature, wouldn't it?

Ah, ok, I didn't pay too much attention to how nr_unswapped is
incremented. Just thought nr_falloced will be incremented by 512
rather than 1, so it is more unlikely to return -ENOMEM.

>
> Add a comment on this in the commit log: yes, I guess so, but I haven't
> worked out what to write yet.
>
> Hugh
>
> >
> > >
> > >                 /*
> > >                  * If !PageUptodate, leave it that way so that freeable pages
> > > --
> > > 2.26.2

Patch

diff --git a/mm/shmem.c b/mm/shmem.c
index 70d9ce294bb4..0cd5c9156457 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2736,7 +2736,7 @@  static long shmem_fallocate(struct file *file, int mode, loff_t offset,
 	inode->i_private = &shmem_falloc;
 	spin_unlock(&inode->i_lock);
 
-	for (index = start; index < end; index++) {
+	for (index = start; index < end; ) {
 		struct page *page;
 
 		/*
@@ -2759,13 +2759,26 @@  static long shmem_fallocate(struct file *file, int mode, loff_t offset,
 			goto undone;
 		}
 
+		index++;
+		/*
+		 * Here is a more important optimization than it appears:
+		 * a second SGP_FALLOC on the same huge page will clear it,
+		 * making it PageUptodate and un-undoable if we fail later.
+		 */
+		if (PageTransCompound(page)) {
+			index = round_up(index, HPAGE_PMD_NR);
+			/* Beware 32-bit wraparound */
+			if (!index)
+				index--;
+		}
+
 		/*
 		 * Inform shmem_writepage() how far we have reached.
 		 * No need for lock or barrier: we have the page lock.
 		 */
-		shmem_falloc.next++;
 		if (!PageUptodate(page))
-			shmem_falloc.nr_falloced++;
+			shmem_falloc.nr_falloced += index - shmem_falloc.next;
+		shmem_falloc.next = index;
 
 		/*
 		 * If !PageUptodate, leave it that way so that freeable pages