From: Hugh Dickins <hughd@google.com>
To: Dave Chinner <david@fromorbit.com>
Cc: Hugh Dickins <hughd@google.com>, Jan Kara <jack@suse.cz>,
Amir Goldstein <amir73il@gmail.com>,
Andreas Gruenbacher <agruenba@redhat.com>,
Theodore Tso <tytso@mit.edu>,
Martin Brandenburg <martin@omnibond.com>,
Mike Marshall <hubcap@omnibond.com>,
Damien Le Moal <damien.lemoal@wdc.com>,
Jaegeuk Kim <jaegeuk@kernel.org>,
Qiuyang Sun <sunqiuyang@huawei.com>,
linux-xfs <linux-xfs@vger.kernel.org>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
Linux MM <linux-mm@kvack.org>,
linux-kernel <linux-kernel@vger.kernel.org>,
Matthew Wilcox <willy@infradead.org>,
Linus Torvalds <torvalds@linux-foundation.org>,
"Kirill A. Shutemov" <kirill@shutemov.name>,
Andrew Morton <akpm@linux-foundation.org>,
Al Viro <viro@zeniv.linux.org.uk>,
nborisov@suse.de
Subject: Re: More filesystem need this fix (xfs: use MMAPLOCK around filemap_map_pages())
Date: Thu, 17 Sep 2020 00:47:10 -0700 (PDT) [thread overview]
Message-ID: <alpine.LSU.2.11.2009170017590.8077@eggly.anvils> (raw)
In-Reply-To: <20200917064532.GI12131@dread.disaster.area>
On Thu, 17 Sep 2020, Dave Chinner wrote:
> On Wed, Sep 16, 2020 at 07:04:46PM -0700, Hugh Dickins wrote:
> > On Thu, 17 Sep 2020, Dave Chinner wrote:
> > > <pte now points to a freed page>
> >
> > No. filemap_map_pages() checks page->mapping after trylock_page(),
> > before setting up the pte; and truncate_cleanup_page() does a one-page
> > unmap_mapping_range() if page_mapped(), while holding page lock.
>
> Ok, fair, I missed that.
>
> So why does truncate_pagecache() talk about fault races and require
> a second unmap range after the invalidation "for correctness" if
> this sort of race cannot happen?
I thought the comment
* unmap_mapping_range is called twice, first simply for
* efficiency so that truncate_inode_pages does fewer
* single-page unmaps. However after this first call, and
* before truncate_inode_pages finishes, it is possible for
* private pages to be COWed, which remain after
* truncate_inode_pages finishes, hence the second
* unmap_mapping_range call must be made for correctness.
explains it fairly well. It's because POSIX demanded that when a file
is truncated, the user will get SIGBUS on trying to access even the
COWed pages beyond EOF in a MAP_PRIVATE mapping. Page lock on the
cache page does not serialize the pages COWed from it very well.
But there's no such SIGBUS requirement in the case of hole-punching,
and trying to unmap those pages racily instantiated just after the
punching cursor passed, would probably do more harm than good.
>
> Why is that different to truncate_pagecache_range() which -doesn't-i
> do that second removal? It's called for more than just hole_punch -
> from the filesystem's persepective holepunch should do exactly the
> same as truncate to the page cache, and for things like
> COLLAPSE_RANGE it is absolutely essential because the data in that
> range is -not zero- and will be stale if the mappings are not
> invalidated completely....
I can't speak to COLLAPSE_RANGE.
>
> Also, if page->mapping == NULL is sufficient to detect an invalidated
> page in all cases, then why does page_cache_delete() explicitly
> leave page->index intact:
>
> page->mapping = NULL;
> /* Leave page->index set: truncation lookup relies upon it */
Because there was, and I think still is (but might it now be xarrayed
away?), code (mainly in mm/truncate.c) which finds it convenient to
check page->index for end of range, without necessitating the overhead
of getting page lock. I've no doubt it's an (minor) optimization that
could be discarded if there were ever a need to invalidate page->index
when deleting; but nobody has required that yet.
Hugh
next prev parent reply other threads:[~2020-09-17 7:47 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20200623052059.1893966-1-david@fromorbit.com>
2020-09-12 6:19 ` More filesystem need this fix (xfs: use MMAPLOCK around filemap_map_pages()) Amir Goldstein
2020-09-14 11:35 ` Jan Kara
2020-09-14 12:29 ` Andreas Gruenbacher
2020-09-16 15:58 ` Jan Kara
2020-09-17 1:44 ` Dave Chinner
2020-09-17 2:04 ` Hugh Dickins
2020-09-17 6:45 ` Dave Chinner
2020-09-17 7:47 ` Hugh Dickins [this message]
2020-09-21 8:26 ` Dave Chinner
2020-09-21 9:11 ` Jan Kara
2020-09-21 16:20 ` Linus Torvalds
2020-09-21 17:59 ` Matthew Wilcox
2020-09-22 7:54 ` Jan Kara
2020-09-17 3:01 ` Matthew Wilcox
2020-09-17 5:37 ` Nikolay Borisov
2020-09-17 7:40 ` Jan Kara
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=alpine.LSU.2.11.2009170017590.8077@eggly.anvils \
--to=hughd@google.com \
--cc=agruenba@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=amir73il@gmail.com \
--cc=damien.lemoal@wdc.com \
--cc=david@fromorbit.com \
--cc=hubcap@omnibond.com \
--cc=jack@suse.cz \
--cc=jaegeuk@kernel.org \
--cc=kirill@shutemov.name \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-xfs@vger.kernel.org \
--cc=martin@omnibond.com \
--cc=nborisov@suse.de \
--cc=sunqiuyang@huawei.com \
--cc=torvalds@linux-foundation.org \
--cc=tytso@mit.edu \
--cc=viro@zeniv.linux.org.uk \
--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 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).