From: Dave Chinner <david@fromorbit.com>
To: Jan Kara <jack@suse.cz>
Cc: linux-fsdevel@vger.kernel.org,
Christoph Hellwig <hch@infradead.org>,
Matthew Wilcox <willy@infradead.org>
Subject: Re: [PATCH 1/2] mm: Protect operations adding pages to page cache with i_mapping_lock
Date: Tue, 9 Feb 2021 12:12:58 +1100 [thread overview]
Message-ID: <20210209011258.GQ4626@dread.disaster.area> (raw)
In-Reply-To: <20210208163918.7871-2-jack@suse.cz>
On Mon, Feb 08, 2021 at 05:39:17PM +0100, Jan Kara wrote:
> Currently, serializing operations such as page fault, read, or readahead
> against hole punching is rather difficult. The basic race scheme is
> like:
>
> fallocate(FALLOC_FL_PUNCH_HOLE) read / fault / ..
> truncate_inode_pages_range()
> <create pages in page
> cache here>
> <update fs block mapping and free blocks>
>
> Now the problem is in this way read / page fault / readahead can
> instantiate pages in page cache with potentially stale data (if blocks
> get quickly reused). Avoiding this race is not simple - page locks do
> not work because we want to make sure there are *no* pages in given
> range. inode->i_rwsem does not work because page fault happens under
> mmap_sem which ranks below inode->i_rwsem. Also using it for reads makes
> the performance for mixed read-write workloads suffer.
>
> So create a new rw_semaphore in the inode - i_mapping_sem - that
> protects adding of pages to page cache for page faults / reads /
> readahead.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> fs/inode.c | 3 +++
> include/linux/fs.h | 2 ++
> mm/filemap.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
> mm/readahead.c | 2 ++
> 4 files changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 6442d97d9a4a..8df49d98e1cd 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -174,6 +174,9 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
>
> init_rwsem(&inode->i_rwsem);
> lockdep_set_class(&inode->i_rwsem, &sb->s_type->i_mutex_key);
> + init_rwsem(&inode->i_mapping_sem);
> + lockdep_set_class(&inode->i_mapping_sem,
> + &sb->s_type->i_mapping_sem_key);
>
> atomic_set(&inode->i_dio_count, 0);
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index b20ddd8a6e62..248609bc61a2 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -658,6 +658,7 @@ struct inode {
> /* Misc */
> unsigned long i_state;
> struct rw_semaphore i_rwsem;
> + struct rw_semaphore i_mapping_sem;
>
> unsigned long dirtied_when; /* jiffies of first dirtying */
> unsigned long dirtied_time_when;
> @@ -2249,6 +2250,7 @@ struct file_system_type {
>
> struct lock_class_key i_lock_key;
> struct lock_class_key i_mutex_key;
> + struct lock_class_key i_mapping_sem_key;
> struct lock_class_key i_mutex_dir_key;
> };
>
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 16a3bf693d4a..02f778ff02e0 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2257,16 +2257,28 @@ static int filemap_update_page(struct kiocb *iocb,
> {
> int error;
>
> + if (iocb->ki_flags & IOCB_NOWAIT) {
> + if (!down_read_trylock(&mapping->host->i_mapping_sem))
> + return -EAGAIN;
> + } else {
> + down_read(&mapping->host->i_mapping_sem);
> + }
> +
> if (!trylock_page(page)) {
> - if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_NOIO))
> + if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_NOIO)) {
> + up_read(&mapping->host->i_mapping_sem);
> return -EAGAIN;
> + }
> if (!(iocb->ki_flags & IOCB_WAITQ)) {
> + up_read(&mapping->host->i_mapping_sem);
> put_and_wait_on_page_locked(page, TASK_KILLABLE);
> return AOP_TRUNCATED_PAGE;
> }
> error = __lock_page_async(page, iocb->ki_waitq);
> - if (error)
> + if (error) {
> + up_read(&mapping->host->i_mapping_sem);
> return error;
> + }
> }
What tree is this against? I don't see filemap_update_page() in a
5.11-rc7 tree...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
next prev parent reply other threads:[~2021-02-09 1:14 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-08 16:39 [PATCH 0/2 RFC v2] fs: Hole punch vs page cache filling races Jan Kara
2021-02-08 16:39 ` [PATCH 1/2] mm: Protect operations adding pages to page cache with i_mapping_lock Jan Kara
2021-02-09 1:12 ` Dave Chinner [this message]
2021-02-09 1:29 ` Matthew Wilcox
2021-02-08 16:39 ` [PATCH 2/2] ext4: Convert to use inode->i_mapping_sem Jan Kara
2021-02-09 1:43 ` [PATCH 0/2 RFC v2] fs: Hole punch vs page cache filling races Dave Chinner
2021-02-12 16:01 ` 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=20210209011258.GQ4626@dread.disaster.area \
--to=david@fromorbit.com \
--cc=hch@infradead.org \
--cc=jack@suse.cz \
--cc=linux-fsdevel@vger.kernel.org \
--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).