[RFC] iomap: fix race between readahead and direct write

[RFC] iomap: fix race between readahead and direct write

[yu kuai]

I noticed that generic/418 test may fail with small probability. And with
futher investiation, it can be reproduced with:

./src/dio-invalidate-cache -wp -b 4096 -n 8 -i 1 -f filename
./src/dio-invalidate-cache -wt -b 4096-n 8 -i 1 -f filename

The failure is because direct write wrote none-zero but buffer read got
zero.

In the process of buffer read, if the page do not exist, readahead will
be triggered.  __do_page_cache_readahead() will allocate page first. Next,
if the file block is unwritten(or hole), iomap_begin() will set iomap->type
to IOMAP_UNWRITTEN(or IOMAP_HOLE). Then, iomap_readpages_actor() will add
page to page cache. Finally, iomap_readpage_actor() will zero the page.

However, there is no lock or serialization between initializing iomap and
adding page to page cache against direct write. If direct write happen to
fininsh between them, the type of iomap should be IOMAP_MAPPED instead of
IOMAP_UNWRITTEN or IOMAP_HOLE. And the page will end up zeroed out in page
cache, while on-disk page hold the data of direct write.

| thread 1                    | thread 2                   |
| --------------------------  | -------------------------- |
| generic_file_buffered_read  |                            |
|  ondemand_readahead         |                            |
|   read_pages                |                            |
|    iomap_readpages          |                            |
|     iomap_apply             |                            |
|      xfs_read_iomap_begin   |                            |
|                             | xfs_file_dio_aio_write     |
|                             |  iomap_dio_rw              |
|                             |   ioamp_apply              |
|     ioamp_readpages_actor   |                            |
|      iomap_next_page        |                            |
|       add_to_page_cache_lru |                            |
|      iomap_readpage_actor   |                            |
|       zero_user             |                            |
|    iomap_set_range_uptodate |                            |
|                             | generic_file_buffered_read |
|                             |  copy_page_to_iter        |

For consequences, the content in the page is zero while the content in the
disk is not.

I tried to fix the problem by moving "add to page cache" before
iomap_begin(). However, performance might be worse since iomap_begin()
will be called for each page. I tested the performance for sequential
read with fio:

kernel version: v5.5-rc6
platform: arm64, 96 cpu
fio version: fio-3.15-2
test cmd:
fio -filename=/mnt/testfile -rw=read -bs=4k -size=20g -direct=0 -fsync=0
-numjobs=1 / 32 -ioengine=libaio -name=test -ramp_time=10 -runtime=120
test result:
|                  | without patch MiB/s | with patch MiB/s |
| ---------------- | ------------------- | ---------------- |
| ssd, numjobs=1   | 512                 | 512              |
| ssd, numjobs=32  | 3615                | 3714             |
| nvme, numjobs=1  | 1167                | 1118             |
| nvme, numjobs=32 | 3679                | 3606             |

Test result shows that the impact on performance is minimal.

Signed-off-by: yu kuai <yukuai3@huawei.com>
---
 fs/iomap/buffered-io.c | 104 ++++++++++++++++++++---------------------
 1 file changed, 52 insertions(+), 52 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 828444e14d09..ccfa1a52d966 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -329,26 +329,44 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
 	return pos - orig_pos + plen;
 }
 
-int
-iomap_readpage(struct page *page, const struct iomap_ops *ops)
+static int
+do_iomap_readpage_apply(
+	loff_t				offset,
+	int				flag,
+	const struct iomap_ops		*ops,
+	struct iomap_readpage_ctx	*ctx,
+	iomap_actor_t			actor,
+	bool				fatal)
 {
-	struct iomap_readpage_ctx ctx = { .cur_page = page };
-	struct inode *inode = page->mapping->host;
-	unsigned poff;
-	loff_t ret;
-
-	trace_iomap_readpage(page->mapping->host, 1);
+	unsigned int			poff;
+	loff_t				ret;
+	struct page			*page = ctx->cur_page;
+	struct inode			*inode = page->mapping->host;
 
 	for (poff = 0; poff < PAGE_SIZE; poff += ret) {
-		ret = iomap_apply(inode, page_offset(page) + poff,
-				PAGE_SIZE - poff, 0, ops, &ctx,
-				iomap_readpage_actor);
+		ret = iomap_apply(inode, offset + poff, PAGE_SIZE - poff,
+				  flag, ops, ctx, actor);
 		if (ret <= 0) {
 			WARN_ON_ONCE(ret == 0);
+			if (fatal)
+				return ret;
 			SetPageError(page);
-			break;
+			return 0;
 		}
 	}
+	return ret;
+}
+
+
+int
+iomap_readpage(struct page *page, const struct iomap_ops *ops)
+{
+	struct iomap_readpage_ctx ctx = { .cur_page = page };
+
+	trace_iomap_readpage(page->mapping->host, 1);
+
+	do_iomap_readpage_apply(page_offset(page), 0, ops, &ctx,
+				iomap_readpage_actor, false);
 
 	if (ctx.bio) {
 		submit_bio(ctx.bio);
@@ -395,34 +413,6 @@ iomap_next_page(struct inode *inode, struct list_head *pages, loff_t pos,
 	return NULL;
 }
 
-static loff_t
-iomap_readpages_actor(struct inode *inode, loff_t pos, loff_t length,
-		void *data, struct iomap *iomap, struct iomap *srcmap)
-{
-	struct iomap_readpage_ctx *ctx = data;
-	loff_t done, ret;
-
-	for (done = 0; done < length; done += ret) {
-		if (ctx->cur_page && offset_in_page(pos + done) == 0) {
-			if (!ctx->cur_page_in_bio)
-				unlock_page(ctx->cur_page);
-			put_page(ctx->cur_page);
-			ctx->cur_page = NULL;
-		}
-		if (!ctx->cur_page) {
-			ctx->cur_page = iomap_next_page(inode, ctx->pages,
-					pos, length, &done);
-			if (!ctx->cur_page)
-				break;
-			ctx->cur_page_in_bio = false;
-		}
-		ret = iomap_readpage_actor(inode, pos + done, length - done,
-				ctx, iomap, srcmap);
-	}
-
-	return done;
-}
-
 int
 iomap_readpages(struct address_space *mapping, struct list_head *pages,
 		unsigned nr_pages, const struct iomap_ops *ops)
@@ -433,22 +423,32 @@ iomap_readpages(struct address_space *mapping, struct list_head *pages,
 	};
 	loff_t pos = page_offset(list_entry(pages->prev, struct page, lru));
 	loff_t last = page_offset(list_entry(pages->next, struct page, lru));
-	loff_t length = last - pos + PAGE_SIZE, ret = 0;
+	loff_t length = last - pos + PAGE_SIZE, ret = 0, done;
 
 	trace_iomap_readpages(mapping->host, nr_pages);
 
-	while (length > 0) {
-		ret = iomap_apply(mapping->host, pos, length, 0, ops,
-				&ctx, iomap_readpages_actor);
+	for (done = 0; done < length; done += PAGE_SIZE) {
+		if (ctx.cur_page) {
+			if (!ctx.cur_page_in_bio)
+				unlock_page(ctx.cur_page);
+			put_page(ctx.cur_page);
+			ctx.cur_page = NULL;
+		}
+		ctx.cur_page = iomap_next_page(mapping->host, ctx.pages,
+					       pos, length, &done);
+		if (!ctx.cur_page)
+			break;
+		ctx.cur_page_in_bio = false;
+
+		ret = do_iomap_readpage_apply(pos+done, 0, ops, &ctx,
+					      iomap_readpage_actor, true);
 		if (ret <= 0) {
-			WARN_ON_ONCE(ret == 0);
-			goto done;
+			done = ret;
+			break;
 		}
-		pos += ret;
-		length -= ret;
+
 	}
-	ret = 0;
-done:
+
 	if (ctx.bio)
 		submit_bio(ctx.bio);
 	if (ctx.cur_page) {
@@ -461,8 +461,8 @@ iomap_readpages(struct address_space *mapping, struct list_head *pages,
 	 * Check that we didn't lose a page due to the arcance calling
 	 * conventions..
 	 */
-	WARN_ON_ONCE(!ret && !list_empty(ctx.pages));
-	return ret;
+	WARN_ON_ONCE((done == length) && !list_empty(ctx.pages));
+	return done;
 }
 EXPORT_SYMBOL_GPL(iomap_readpages);
 
-- 
2.17.2

Re: [RFC] iomap: fix race between readahead and direct write

[Jan Kara]

On Thu 16-01-20 14:36:01, yu kuai wrote:
> I noticed that generic/418 test may fail with small probability. And with
> futher investiation, it can be reproduced with:
> 
> ./src/dio-invalidate-cache -wp -b 4096 -n 8 -i 1 -f filename
> ./src/dio-invalidate-cache -wt -b 4096-n 8 -i 1 -f filename
> 
> The failure is because direct write wrote none-zero but buffer read got
> zero.
> 
> In the process of buffer read, if the page do not exist, readahead will
> be triggered.  __do_page_cache_readahead() will allocate page first. Next,
> if the file block is unwritten(or hole), iomap_begin() will set iomap->type
> to IOMAP_UNWRITTEN(or IOMAP_HOLE). Then, iomap_readpages_actor() will add
> page to page cache. Finally, iomap_readpage_actor() will zero the page.
> 
> However, there is no lock or serialization between initializing iomap and
> adding page to page cache against direct write. If direct write happen to
> fininsh between them, the type of iomap should be IOMAP_MAPPED instead of
> IOMAP_UNWRITTEN or IOMAP_HOLE. And the page will end up zeroed out in page
> cache, while on-disk page hold the data of direct write.
> 
> | thread 1                    | thread 2                   |
> | --------------------------  | -------------------------- |
> | generic_file_buffered_read  |                            |
> |  ondemand_readahead         |                            |
> |   read_pages                |                            |
> |    iomap_readpages          |                            |
> |     iomap_apply             |                            |
> |      xfs_read_iomap_begin   |                            |
> |                             | xfs_file_dio_aio_write     |
> |                             |  iomap_dio_rw              |
> |                             |   ioamp_apply              |
> |     ioamp_readpages_actor   |                            |
> |      iomap_next_page        |                            |
> |       add_to_page_cache_lru |                            |
> |      iomap_readpage_actor   |                            |
> |       zero_user             |                            |
> |    iomap_set_range_uptodate |                            |
> |                             | generic_file_buffered_read |
> |                             |  copy_page_to_iter        |

Thanks for the report and the patch. But the data integrity when mixing
buffered and direct IO like this is best effort only. We definitely do not
want to sacrifice performance of common cases or code complexity to make
cases like this work reliably.

								Honza

> For consequences, the content in the page is zero while the content in the
> disk is not.
> 
> I tried to fix the problem by moving "add to page cache" before
> iomap_begin(). However, performance might be worse since iomap_begin()
> will be called for each page. I tested the performance for sequential
> read with fio:
> 
> kernel version: v5.5-rc6
> platform: arm64, 96 cpu
> fio version: fio-3.15-2
> test cmd:
> fio -filename=/mnt/testfile -rw=read -bs=4k -size=20g -direct=0 -fsync=0
> -numjobs=1 / 32 -ioengine=libaio -name=test -ramp_time=10 -runtime=120
> test result:
> |                  | without patch MiB/s | with patch MiB/s |
> | ---------------- | ------------------- | ---------------- |
> | ssd, numjobs=1   | 512                 | 512              |
> | ssd, numjobs=32  | 3615                | 3714             |
> | nvme, numjobs=1  | 1167                | 1118             |
> | nvme, numjobs=32 | 3679                | 3606             |
> 
> Test result shows that the impact on performance is minimal.
> 
> Signed-off-by: yu kuai <yukuai3@huawei.com>
> ---
>  fs/iomap/buffered-io.c | 104 ++++++++++++++++++++---------------------
>  1 file changed, 52 insertions(+), 52 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 828444e14d09..ccfa1a52d966 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -329,26 +329,44 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
>  	return pos - orig_pos + plen;
>  }
>  
> -int
> -iomap_readpage(struct page *page, const struct iomap_ops *ops)
> +static int
> +do_iomap_readpage_apply(
> +	loff_t				offset,
> +	int				flag,
> +	const struct iomap_ops		*ops,
> +	struct iomap_readpage_ctx	*ctx,
> +	iomap_actor_t			actor,
> +	bool				fatal)
>  {
> -	struct iomap_readpage_ctx ctx = { .cur_page = page };
> -	struct inode *inode = page->mapping->host;
> -	unsigned poff;
> -	loff_t ret;
> -
> -	trace_iomap_readpage(page->mapping->host, 1);
> +	unsigned int			poff;
> +	loff_t				ret;
> +	struct page			*page = ctx->cur_page;
> +	struct inode			*inode = page->mapping->host;
>  
>  	for (poff = 0; poff < PAGE_SIZE; poff += ret) {
> -		ret = iomap_apply(inode, page_offset(page) + poff,
> -				PAGE_SIZE - poff, 0, ops, &ctx,
> -				iomap_readpage_actor);
> +		ret = iomap_apply(inode, offset + poff, PAGE_SIZE - poff,
> +				  flag, ops, ctx, actor);
>  		if (ret <= 0) {
>  			WARN_ON_ONCE(ret == 0);
> +			if (fatal)
> +				return ret;
>  			SetPageError(page);
> -			break;
> +			return 0;
>  		}
>  	}
> +	return ret;
> +}
> +
> +
> +int
> +iomap_readpage(struct page *page, const struct iomap_ops *ops)
> +{
> +	struct iomap_readpage_ctx ctx = { .cur_page = page };
> +
> +	trace_iomap_readpage(page->mapping->host, 1);
> +
> +	do_iomap_readpage_apply(page_offset(page), 0, ops, &ctx,
> +				iomap_readpage_actor, false);
>  
>  	if (ctx.bio) {
>  		submit_bio(ctx.bio);
> @@ -395,34 +413,6 @@ iomap_next_page(struct inode *inode, struct list_head *pages, loff_t pos,
>  	return NULL;
>  }
>  
> -static loff_t
> -iomap_readpages_actor(struct inode *inode, loff_t pos, loff_t length,
> -		void *data, struct iomap *iomap, struct iomap *srcmap)
> -{
> -	struct iomap_readpage_ctx *ctx = data;
> -	loff_t done, ret;
> -
> -	for (done = 0; done < length; done += ret) {
> -		if (ctx->cur_page && offset_in_page(pos + done) == 0) {
> -			if (!ctx->cur_page_in_bio)
> -				unlock_page(ctx->cur_page);
> -			put_page(ctx->cur_page);
> -			ctx->cur_page = NULL;
> -		}
> -		if (!ctx->cur_page) {
> -			ctx->cur_page = iomap_next_page(inode, ctx->pages,
> -					pos, length, &done);
> -			if (!ctx->cur_page)
> -				break;
> -			ctx->cur_page_in_bio = false;
> -		}
> -		ret = iomap_readpage_actor(inode, pos + done, length - done,
> -				ctx, iomap, srcmap);
> -	}
> -
> -	return done;
> -}
> -
>  int
>  iomap_readpages(struct address_space *mapping, struct list_head *pages,
>  		unsigned nr_pages, const struct iomap_ops *ops)
> @@ -433,22 +423,32 @@ iomap_readpages(struct address_space *mapping, struct list_head *pages,
>  	};
>  	loff_t pos = page_offset(list_entry(pages->prev, struct page, lru));
>  	loff_t last = page_offset(list_entry(pages->next, struct page, lru));
> -	loff_t length = last - pos + PAGE_SIZE, ret = 0;
> +	loff_t length = last - pos + PAGE_SIZE, ret = 0, done;
>  
>  	trace_iomap_readpages(mapping->host, nr_pages);
>  
> -	while (length > 0) {
> -		ret = iomap_apply(mapping->host, pos, length, 0, ops,
> -				&ctx, iomap_readpages_actor);
> +	for (done = 0; done < length; done += PAGE_SIZE) {
> +		if (ctx.cur_page) {
> +			if (!ctx.cur_page_in_bio)
> +				unlock_page(ctx.cur_page);
> +			put_page(ctx.cur_page);
> +			ctx.cur_page = NULL;
> +		}
> +		ctx.cur_page = iomap_next_page(mapping->host, ctx.pages,
> +					       pos, length, &done);
> +		if (!ctx.cur_page)
> +			break;
> +		ctx.cur_page_in_bio = false;
> +
> +		ret = do_iomap_readpage_apply(pos+done, 0, ops, &ctx,
> +					      iomap_readpage_actor, true);
>  		if (ret <= 0) {
> -			WARN_ON_ONCE(ret == 0);
> -			goto done;
> +			done = ret;
> +			break;
>  		}
> -		pos += ret;
> -		length -= ret;
> +
>  	}
> -	ret = 0;
> -done:
> +
>  	if (ctx.bio)
>  		submit_bio(ctx.bio);
>  	if (ctx.cur_page) {
> @@ -461,8 +461,8 @@ iomap_readpages(struct address_space *mapping, struct list_head *pages,
>  	 * Check that we didn't lose a page due to the arcance calling
>  	 * conventions..
>  	 */
> -	WARN_ON_ONCE(!ret && !list_empty(ctx.pages));
> -	return ret;
> +	WARN_ON_ONCE((done == length) && !list_empty(ctx.pages));
> +	return done;
>  }
>  EXPORT_SYMBOL_GPL(iomap_readpages);
>  
> -- 
> 2.17.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC] iomap: fix race between readahead and direct write

[yukuai (C)]

On 2020/1/16 23:32, Jan Kara wrote:
> Thanks for the report and the patch. But the data integrity when mixing
> buffered and direct IO like this is best effort only. We definitely do not
> want to sacrifice performance of common cases or code complexity to make
> cases like this work reliably.

In the patch, the only thing that is diffrent is that iomap_begin() will
be called for each page. However, it seems the performance in sequential
read didn't get worse. Is there a specific case that the performance
will get worse?

Thanks!
Yu Kuai!

Re: [RFC] iomap: fix race between readahead and direct write

[Jan Kara]

On Fri 17-01-20 17:39:03, yukuai (C) wrote:
> On 2020/1/16 23:32, Jan Kara wrote:
> > Thanks for the report and the patch. But the data integrity when mixing
> > buffered and direct IO like this is best effort only. We definitely do not
> > want to sacrifice performance of common cases or code complexity to make
> > cases like this work reliably.
> 
> In the patch, the only thing that is diffrent is that iomap_begin() will
> be called for each page. However, it seems the performance in sequential
> read didn't get worse. Is there a specific case that the performance
> will get worse?

Well, one of the big points of iomap infrastructure is that you call
filesystem once to give you large extent instead of calling it to provide
allocation for each page separately. The additional CPU overhead will be
visible if you push the machine hard enough. So IMHO the overhead just is
not worth it for a corner-case like you presented. But that's just my
opinion, Darrick and Christoph are definitive arbiters here...

								Honza

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

Re: [RFC] iomap: fix race between readahead and direct write

[Darrick J. Wong]

On Fri, Jan 17, 2020 at 12:05:36PM +0100, Jan Kara wrote:
> On Fri 17-01-20 17:39:03, yukuai (C) wrote:
> > On 2020/1/16 23:32, Jan Kara wrote:
> > > Thanks for the report and the patch. But the data integrity when mixing
> > > buffered and direct IO like this is best effort only. We definitely do not
> > > want to sacrifice performance of common cases or code complexity to make
> > > cases like this work reliably.
> > 
> > In the patch, the only thing that is diffrent is that iomap_begin() will
> > be called for each page. However, it seems the performance in sequential
> > read didn't get worse. Is there a specific case that the performance
> > will get worse?
> 
> Well, one of the big points of iomap infrastructure is that you call
> filesystem once to give you large extent instead of calling it to provide
> allocation for each page separately. The additional CPU overhead will be
> visible if you push the machine hard enough. So IMHO the overhead just is
> not worth it for a corner-case like you presented. But that's just my
> opinion, Darrick and Christoph are definitive arbiters here...

Does the problem go away if you apply[1]?  If I understand the race
correctly, marking the extents unwritten and leaving them that way until
after we've written the disk should eliminate the exposure vector...? :)

[1] https://lore.kernel.org/linux-xfs/157915535059.2406747.264640456606868955.stgit@magnolia/

--D

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

Re: [RFC] iomap: fix race between readahead and direct write

[yukuai (C)]

On 2020/1/18 0:24, Darrick J. Wong wrote:
> Does the problem go away if you apply[1]?  If I understand the race
> correctly, marking the extents unwritten and leaving them that way until
> after we've written the disk should eliminate the exposure vector...?:)

Thank you for your response.

The patch [1] fixed the stale data exposure problem which was tested by
generic/042. I'm afarid it's completely a diffrent case for generic/418.

In generic/418, direct write wrote some data to disk successfully, while
buffer read left the correspond page uptodate with 0 content in
pagecache.

Thanks!
Yu Kuai

Re: [RFC] iomap: fix race between readahead and direct write

[yukuai (C)]

On 2020/1/17 19:05, Jan Kara wrote:
> provide
> allocation for each page separately

Thank you for your response!

I do understand there will be additional CPU overhead. But page is 
allocated in __do_page_cache_readahead(), which is called before
iomap_begin(). And I did not change that.

Thanks!
Yu Kuai

Re: [RFC] iomap: fix race between readahead and direct write

[Jan Kara]

On Sun 19-01-20 09:17:00, yukuai (C) wrote:
> 
> 
> On 2020/1/17 19:05, Jan Kara wrote:
> > provide
> > allocation for each page separately
> 
> Thank you for your response!
> 
> I do understand there will be additional CPU overhead. But page is allocated
> in __do_page_cache_readahead(), which is called before
> iomap_begin(). And I did not change that.

Sorry, I didn't express myself clearly. In "...one of the big points of iomap
infrastructure is that you call filesystem once to give you large extent
instead of calling it to provide allocation for each page separately." I
meant that with your patch, you call into filesystem to provide "block
allocation information" for each page separately. And it seems we both
agree this is going to cause additional CPU usage in the common case to
improve mostly unsupported corner case.

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

Re: [RFC] iomap: fix race between readahead and direct write

[Matthew Wilcox]

On Thu, Jan 16, 2020 at 02:36:01PM +0800, yu kuai wrote:
> I noticed that generic/418 test may fail with small probability. And with
> futher investiation, it can be reproduced with:
> 
> ./src/dio-invalidate-cache -wp -b 4096 -n 8 -i 1 -f filename
> ./src/dio-invalidate-cache -wt -b 4096-n 8 -i 1 -f filename
> 
> The failure is because direct write wrote none-zero but buffer read got
> zero.
> 
> In the process of buffer read, if the page do not exist, readahead will
> be triggered.  __do_page_cache_readahead() will allocate page first. Next,
> if the file block is unwritten(or hole), iomap_begin() will set iomap->type
> to IOMAP_UNWRITTEN(or IOMAP_HOLE). Then, iomap_readpages_actor() will add
> page to page cache. Finally, iomap_readpage_actor() will zero the page.
> 
> However, there is no lock or serialization between initializing iomap and
> adding page to page cache against direct write. If direct write happen to
> fininsh between them, the type of iomap should be IOMAP_MAPPED instead of
> IOMAP_UNWRITTEN or IOMAP_HOLE. And the page will end up zeroed out in page
> cache, while on-disk page hold the data of direct write.

It's worth noting that my patch series from earlier this week to
redesign the readahead API will fix this problem.  Direct write will block
on the locked pages in the page cache.

Re: [RFC] iomap: fix race between readahead and direct write

[yukuai (C)]

On 2020/1/19 7:08, Matthew Wilcox wrote:
> It's worth noting that my patch series from earlier this week to
> redesign the readahead API will fix this problem.  Direct write will block
> on the locked pages in the page cache.

Thank you for your response!

In this case, direct write finish while page do not exist in the page
cache. This is the fundamental condition of the race, because readahead
won't allocate page if page exist in page cache.

By the way, in the current logic, if page exist in page cache, direct
write need to hold lock for page in invalidate_inode_pages2_range().

Thanks!
Yu Kuai

Re: [RFC] iomap: fix race between readahead and direct write

[Matthew Wilcox]

On Sun, Jan 19, 2020 at 09:34:32AM +0800, yukuai (C) wrote:
> 
> 
> On 2020/1/19 7:08, Matthew Wilcox wrote:
> > It's worth noting that my patch series from earlier this week to
> > redesign the readahead API will fix this problem.  Direct write will block
> > on the locked pages in the page cache.
> 
> Thank you for your response!
> 
> In this case, direct write finish while page do not exist in the page
> cache. This is the fundamental condition of the race, because readahead
> won't allocate page if page exist in page cache.
> 
> By the way, in the current logic, if page exist in page cache, direct
> write need to hold lock for page in invalidate_inode_pages2_range().

Did you read my patch series?  The current code allocates pages,
but does not put them in the page cache until after iomap is called.
My patch series changes that to put the pages in the page cache as soon
as they're allocated, and before iomap is called.

Re: [RFC] iomap: fix race between readahead and direct write

[yukuai (C)]

On 2020/1/19 9:42, Matthew Wilcox wrote:
> Did you read my patch series?  The current code allocates pages,
> but does not put them in the page cache until after iomap is called.
> My patch series changes that to put the pages in the page cache as soon
> as they're allocated, and before iomap is called.

The current code to determin if page need to be allocated:

     page = xa_load(&mapping->i_pages, page_offset);
     if (page && !xa_is_value(page)) {
       /*
       ┊* Page already present?  Kick off the current batch of
       ┊* contiguous pages before continuing with the next
       ┊* batch.
       ┊*/
       if (nr_pages)
         read_pages(mapping, filp, &page_pool, nr_pages,
             gfp_mask);
       nr_pages = 0;
       continue;
     }

     page = __page_cache_alloc(gfp_mask);
     if (!page)
       break;

Page will be allocated if the page do not exist in page cache. And I
don't see your patch series change that. Or am I missing something?

Thanks！
Yu Kuai

Re: [RFC] iomap: fix race between readahead and direct write

[yukuai (C)]

On 2020/1/19 9:42, Matthew Wilcox wrote:
> Did you read my patch series?  The current code allocates pages,
> but does not put them in the page cache until after iomap is called.
> My patch series changes that to put the pages in the page cache as soon
> as they're allocated, and before iomap is called.

I just read you patch series again.

At first, if you try to add all pages to pagecache and lock them before
iomap_begin. I thought aboult it before, but I throw away the idea
becacuse all other operation that will lock the page will need to wait
for readahead to finish. And it might cause problem for performance
overhead. And if you try to add each page to page cache and call iomap
before adding the next page. Then, we are facing the same CPU overhead
issure.

Then, there might be a problem in your implementation.
if 'use_list' is set to true here:
+	bool use_list = mapping->a_ops->readpages;

Your code do not call add_to_page_cache_lru for the page.
+		if (use_list) {
+			page->index = page_offset;
+			list_add(&page->lru, &page_pool);
+		} else if (!add_to_page_cache_lru(page, mapping, page_offset,
+					gfp_mask)) {
+			if (nr_pages)
+				read_pages(mapping, filp, &page_pool,
+						page_offset - nr_pages,
+						nr_pages);
+			nr_pages = 0;
+			continue;
+		}

And later, you replace 'iomap_next_page' with 'readahead_page'
+static inline
+struct page *readahead_page(struct address_space *mapping, loff_t pos)
+{
+	struct page *page = xa_load(&mapping->i_pages, pos / PAGE_SIZE);
+	VM_BUG_ON_PAGE(!PageLocked(page), page);
+
+	return page;
+}
+

It seems that the page will never add to page cache.

Thanks!
Yu Kuai

Re: [RFC] iomap: fix race between readahead and direct write

[Gao Xiang]

On Sun, Jan 19, 2020 at 10:51:37AM +0800, yukuai (C) wrote:
> 
> 
> Then, there might be a problem in your implementation.
> if 'use_list' is set to true here:
> +	bool use_list = mapping->a_ops->readpages;
> 
> Your code do not call add_to_page_cache_lru for the page.

IMO, if use_list == true, it will call read_pages -> .readpages
and go just like the current implementation.

->.readahead is just alloc_page and then add_to_page_cache_lru
right after in time and in principle it saves some extra page
allocation.

Thanks,
Gao Xiang

> +		if (use_list) {
> +			page->index = page_offset;
> +			list_add(&page->lru, &page_pool);
> +		} else if (!add_to_page_cache_lru(page, mapping, page_offset,
> +					gfp_mask)) {
> +			if (nr_pages)
> +				read_pages(mapping, filp, &page_pool,
> +						page_offset - nr_pages,
> +						nr_pages);
> +			nr_pages = 0;
> +			continue;
> +		}
> 

Re: [RFC] iomap: fix race between readahead and direct write

[yukuai (C)]

On 2020/1/19 11:01, Gao Xiang wrote:
> IMO, if use_list == true, it will call read_pages -> .readpages
> and go just like the current implementation.

The problem is that in the current implementatin, iomap_next_page()
add page to page cache and it is replaced with readahead_page()

Thanks!
Yu Kuai

Re: [RFC] iomap: fix race between readahead and direct write

[Matthew Wilcox]

On Sun, Jan 19, 2020 at 10:51:37AM +0800, yukuai (C) wrote:
> At first, if you try to add all pages to pagecache and lock them before
> iomap_begin. I thought aboult it before, but I throw away the idea
> becacuse all other operation that will lock the page will need to wait
> for readahead to finish. And it might cause problem for performance
> overhead. And if you try to add each page to page cache and call iomap
> before adding the next page. Then, we are facing the same CPU overhead
> issure.

I don't understand your reasoning here.  If another process wants to
access a page of the file which isn't currently in cache, it would have
to first read the page in from storage.  If it's under readahead, it
has to wait for the read to finish.  Why is the second case worse than
the second?  It seems better to me.

The implementation doesn't call iomap for each page.  It allocates all
the pages and then calls iomap for the range.

> Then, there might be a problem in your implementation.
> if 'use_list' is set to true here:
> +	bool use_list = mapping->a_ops->readpages;
> 
> Your code do not call add_to_page_cache_lru for the page.

It can't.  The readpages implementation has to call add_to_page_cache_lru.
But for filesystems which use readpage or readahead, we can put the pages
in the page cache before calling readahead.

> And later, you replace 'iomap_next_page' with 'readahead_page'
> +static inline
> +struct page *readahead_page(struct address_space *mapping, loff_t pos)
> +{
> +	struct page *page = xa_load(&mapping->i_pages, pos / PAGE_SIZE);
> +	VM_BUG_ON_PAGE(!PageLocked(page), page);
> +
> +	return page;
> +}
> +
> 
> It seems that the page will never add to page cache.

At the same time, the iomap code is switched from ->readpages to
->readahead, so yes, the pages are added to the page cache.

Re: [RFC] iomap: fix race between readahead and direct write

[yukuai (C)]

On 2020/1/19 14:14, Matthew Wilcox wrote:
> I don't understand your reasoning here.  If another process wants to
> access a page of the file which isn't currently in cache, it would have
> to first read the page in from storage.  If it's under readahead, it
> has to wait for the read to finish.  Why is the second case worse than
> the second?  It seems better to me.

Thanks for your response! My worries is that, for example:

We read page 0, and trigger readahead to read n pages(0 - n-1). While in 
another thread, we read page n-1.

In the current implementation, if readahead is in the process of reading
page 0 - n-2,  later operation doesn't need to wait the former one to 
finish. However, later operation will have to wait if we add all pages
to page cache first. And that is why I said it might cause problem for
performance overhead.

> At the same time, the iomap code is switched from ->readpages to
> ->readahead, so yes, the pages are added to the page cache.

Yes, it's not a problem in your implementation.

Thanks!
Yu Kuai

Re: [RFC] iomap: fix race between readahead and direct write

[Matthew Wilcox]

On Sun, Jan 19, 2020 at 02:55:14PM +0800, yukuai (C) wrote:
> On 2020/1/19 14:14, Matthew Wilcox wrote:
> > I don't understand your reasoning here.  If another process wants to
> > access a page of the file which isn't currently in cache, it would have
> > to first read the page in from storage.  If it's under readahead, it
> > has to wait for the read to finish.  Why is the second case worse than
> > the second?  It seems better to me.
> 
> Thanks for your response! My worries is that, for example:
> 
> We read page 0, and trigger readahead to read n pages(0 - n-1). While in
> another thread, we read page n-1.
> 
> In the current implementation, if readahead is in the process of reading
> page 0 - n-2,  later operation doesn't need to wait the former one to
> finish. However, later operation will have to wait if we add all pages
> to page cache first. And that is why I said it might cause problem for
> performance overhead.

OK, but let's put some numbers on that.  Imagine that we're using high
performance spinning rust so we have an access latency of 5ms (200
IOPS), we're accessing 20 consecutive pages which happen to have their
data contiguous on disk.  Our CPU is running at 2GHz and takes about
100,000 cycles to submit an I/O, plus 1,000 cycles to add an extra page
to the I/O.

Current implementation: Allocate 20 pages, place 19 of them in the cache,
fail to place the last one in the cache.  The later thread actually gets
to jump the queue and submit its bio first.  Its latency will be 100,000
cycles (20us) plus the 5ms access time.  But it only has 20,000 cycles
(4us) to hit this race, or it will end up behaving the same way as below.

New implementation: Allocate 20 pages, place them all in the cache,
then takes 120,000 cycles to build & submit the I/O, and wait 5ms for
the I/O to complete.

But look how much more likely it is that it'll hit during the window
where we're waiting for the I/O to complete -- 5ms is 1250 times longer
than 4us.

If it _does_ get the latency benefit of jumping the queue, the readahead
will create one or two I/Os.  If it hit page 18 instead of page 19, we'd
end up doing three I/Os; the first for page 18, then one for pages 0-17,
and one for page 19.  And that means the disk is going to be busy for
15ms, delaying the next I/O for up to 10ms.  It's actually beneficial in
the long term for the second thread to wait for the readahead to finish.

Oh, and the current ->readpages code has a race where if the page tagged
with PageReadahead ends up not being inserted, we'll lose that bit,
which means the readahead will just stop and have to restart (because
it will look to the readahead code like it's not being effective).
That's a far worse performance problem.

Re: [RFC] iomap: fix race between readahead and direct write

[yukuai (C)]

On 2020/1/19 15:58, Matthew Wilcox wrote:
> On Sun, Jan 19, 2020 at 02:55:14PM +0800, yukuai (C) wrote:
>> On 2020/1/19 14:14, Matthew Wilcox wrote:
>>> I don't understand your reasoning here.  If another process wants to
>>> access a page of the file which isn't currently in cache, it would have
>>> to first read the page in from storage.  If it's under readahead, it
>>> has to wait for the read to finish.  Why is the second case worse than
>>> the second?  It seems better to me.
>>
>> Thanks for your response! My worries is that, for example:
>>
>> We read page 0, and trigger readahead to read n pages(0 - n-1). While in
>> another thread, we read page n-1.
>>
>> In the current implementation, if readahead is in the process of reading
>> page 0 - n-2,  later operation doesn't need to wait the former one to
>> finish. However, later operation will have to wait if we add all pages
>> to page cache first. And that is why I said it might cause problem for
>> performance overhead.
> 
> OK, but let's put some numbers on that.  Imagine that we're using high
> performance spinning rust so we have an access latency of 5ms (200
> IOPS), we're accessing 20 consecutive pages which happen to have their
> data contiguous on disk.  Our CPU is running at 2GHz and takes about
> 100,000 cycles to submit an I/O, plus 1,000 cycles to add an extra page
> to the I/O.
> 
> Current implementation: Allocate 20 pages, place 19 of them in the cache,
> fail to place the last one in the cache.  The later thread actually gets
> to jump the queue and submit its bio first.  Its latency will be 100,000
> cycles (20us) plus the 5ms access time.  But it only has 20,000 cycles
> (4us) to hit this race, or it will end up behaving the same way as below.
> 
> New implementation: Allocate 20 pages, place them all in the cache,
> then takes 120,000 cycles to build & submit the I/O, and wait 5ms for
> the I/O to complete.
> 
> But look how much more likely it is that it'll hit during the window
> where we're waiting for the I/O to complete -- 5ms is 1250 times longer
> than 4us.
> 
> If it _does_ get the latency benefit of jumping the queue, the readahead
> will create one or two I/Os.  If it hit page 18 instead of page 19, we'd
> end up doing three I/Os; the first for page 18, then one for pages 0-17,
> and one for page 19.  And that means the disk is going to be busy for
> 15ms, delaying the next I/O for up to 10ms.  It's actually beneficial in
> the long term for the second thread to wait for the readahead to finish.
> 

Thank you very much for your detailed explanation, I was too blind for
my sided view. And I do agree that your patch series is a better
solution for the problem.

Yu Kuai