Strange condition in invalidate_mapping_pages()

Strange condition in invalidate_mapping_pages()

[Jan Kara]

Hi Kirill,

in commit fc127da085c26 "truncate: handle file thp" you've added the
following to invalidate_mapping_pages():

          /* Middle of THP: skip */
          if (PageTransTail(page)) {
                  unlock_page(page);
                  continue;
          } else if (PageTransHuge(page)) {
                  index += HPAGE_PMD_NR - 1;
                  i += HPAGE_PMD_NR - 1;
                  /* 'end' is in the middle of THP */
                  if (index ==  round_down(end, HPAGE_PMD_NR))
                          continue;
          }

Now how can ever condition "if (index ==  round_down(end,
HPAGE_PMD_NR))" be true? We have just added HPAGE_PMD_NR - 1 to 'index'
so it will not be a multiple of HPAGE_PMD_NR. Presumably you wanted to
check whether the current THP is the one containing 'end' here which would
be something like 'round_down(index, HPAGE_PMD_NR) == round_down(end,
HPAGE_PMD_NR)' but then I still miss why you'd like to avoid invalidating
the partial THP at the end of file... Can you please enlighten me? Thanks!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Strange condition in invalidate_mapping_pages()

[Kirill A. Shutemov]

On Thu, May 18, 2017 at 03:28:18PM +0200, Jan Kara wrote:
> Hi Kirill,
> 
> in commit fc127da085c26 "truncate: handle file thp" you've added the
> following to invalidate_mapping_pages():
> 
>           /* Middle of THP: skip */
>           if (PageTransTail(page)) {
>                   unlock_page(page);
>                   continue;
>           } else if (PageTransHuge(page)) {
>                   index += HPAGE_PMD_NR - 1;
>                   i += HPAGE_PMD_NR - 1;
>                   /* 'end' is in the middle of THP */
>                   if (index ==  round_down(end, HPAGE_PMD_NR))
>                           continue;
>           }
> 
> Now how can ever condition "if (index ==  round_down(end,
> HPAGE_PMD_NR))" be true? We have just added HPAGE_PMD_NR - 1 to 'index'
> so it will not be a multiple of HPAGE_PMD_NR. Presumably you wanted to
> check whether the current THP is the one containing 'end' here which would
> be something like 'round_down(index, HPAGE_PMD_NR) == round_down(end,
> HPAGE_PMD_NR)'.

You're right, it's a bug. 'page->index' instead of 'index' should do the
trick.

Would you like to prepare the patch? (I'm deep in 5-level paging at the
moment.)

> but then I still miss why you'd like to avoid invalidating the partial
> THP at the end of file... Can you please enlighten me? Thanks!

My logic was that the data in the non-invalidated part of the page can be
still useful and it's better to leave it in page cache.

I don't have performance numbers to validate my intuition.

-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>