All of lore.kernel.org
 help / color / mirror / Atom feed
From: William Kucharski <william.kucharski@oracle.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: "Kirill A. Shutemov" <kirill@shutemov.name>,
	Linux-MM <linux-mm@kvack.org>,
	linux-fsdevel@vger.kernel.org,
	open list <linux-kernel@vger.kernel.org>,
	Hugh Dickins <hughd@google.com>, Jan Kara <jack@suse.cz>,
	Song Liu <liu.song.a23@gmail.com>
Subject: Re: [PATCH v3] page cache: Store only head pages in i_pages
Date: Wed, 6 Mar 2019 02:36:35 -0700	[thread overview]
Message-ID: <863F9255-E992-402F-827D-DA5F4661B9AB@oracle.com> (raw)
In-Reply-To: <CAPhsuW5a8=QJe2acWXQGWic1a=CJigwPR6BxSu2O2vg4W1mhzA@mail.gmail.com>


Other than the bug Song found in memfd_tag_pins(), I'd like to suggest two quick
but pedantic changes to mm/filemap.c:

Though not modified in this patch, in line 284, the parenthesis should be moved
to after the period:

 * modified.) The function expects only THP head pages to be present in the

> +		 * Move to the next page in the vector if this is a small page
> +		 * or the index is of the last page in this compound page).

A few lines later, there is an extraneous parenthesis, and the comment could be a bit
clearer.

Might I suggest:

                 * Move to the next page in the vector if this is a PAGESIZE
                 * page or if the index is of the last PAGESIZE page within
                 * this compound page.

You can say "base" instead of "PAGESIZE," but "small" seems open to interpretation.

I haven't run across any problems and have been hammering the code for over five days
without issue; all my testing was with transparent_hugepage/enabled set to
"always."

Tested-by: William Kucharski <william.kucharski@oracle.com>
Reviewed-by: William Kucharski <william.kucharski@oracle.com>

  reply	other threads:[~2019-03-06  9:37 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-15 22:25 [PATCH v3] page cache: Store only head pages in i_pages Matthew Wilcox
2019-03-01 19:12 ` Song Liu
2019-03-01 19:12   ` Song Liu
2019-03-01 22:20   ` Song Liu
2019-03-01 22:20     ` Song Liu
2019-03-06  9:36     ` William Kucharski [this message]
2019-03-07 15:06       ` Matthew Wilcox

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=863F9255-E992-402F-827D-DA5F4661B9AB@oracle.com \
    --to=william.kucharski@oracle.com \
    --cc=hughd@google.com \
    --cc=jack@suse.cz \
    --cc=kirill@shutemov.name \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=liu.song.a23@gmail.com \
    --cc=willy@infradead.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.