[PATCH] f2fs: avoid readahead race condition

[PATCH] f2fs: avoid readahead race condition

[Jaegeuk Kim]

If two readahead threads having same offset enter in readpages, every read
IOs are split and issued to the disk which giving lower bandwidth.

This patch tries to avoid redundant readahead calls.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/data.c  | 15 +++++++++++++++
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/super.c |  2 ++
 3 files changed, 18 insertions(+)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index dfd3225153570..1886d83bc5f15 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2292,6 +2292,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	unsigned nr_pages = rac ? readahead_count(rac) : 1;
 	unsigned max_nr_pages = nr_pages;
 	int ret = 0;
+	bool drop_ra = false;
 
 	map.m_pblk = 0;
 	map.m_lblk = 0;
@@ -2302,6 +2303,17 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	map.m_seg_type = NO_CHECK_TYPE;
 	map.m_may_create = false;
 
+	/*
+	 * Two readahead threads for same address range can cause race condition
+	 * which fragments sequential read IOs. So let's avoid each other.
+	 */
+	if (rac && readahead_count(rac)) {
+		if (F2FS_I(inode)->ra_offset == readahead_index(rac))
+			drop_ra = true;
+		else
+			F2FS_I(inode)->ra_offset = readahead_index(rac);
+	}
+
 	for (; nr_pages; nr_pages--) {
 		if (rac) {
 			page = readahead_page(rac);
@@ -2368,6 +2380,9 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	}
 	if (bio)
 		__submit_bio(F2FS_I_SB(inode), bio, DATA);
+
+	if (rac && readahead_count(rac) && !drop_ra)
+		F2FS_I(inode)->ra_offset = -1;
 	return ret;
 }
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 7fb2a1a334388..753782426feac 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -809,6 +809,7 @@ struct f2fs_inode_info {
 	struct list_head inmem_pages;	/* inmemory pages managed by f2fs */
 	struct task_struct *inmem_task;	/* store inmemory task */
 	struct mutex inmem_lock;	/* lock for inmemory pages */
+	pgoff_t ra_offset;		/* ongoing readahead offset */
 	struct extent_tree *extent_tree;	/* cached extent_tree entry */
 
 	/* avoid racing between foreground op and gc */
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 7326522057378..80cb7cd358f84 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1015,6 +1015,8 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
 	/* Will be used by directory only */
 	fi->i_dir_level = F2FS_SB(sb)->dir_level;
 
+	fi->ra_offset = -1;
+
 	return &fi->vfs_inode;
 }
 
-- 
2.27.0.111.gc72c7da667-goog

[f2fs-dev] [PATCH] f2fs: avoid readahead race condition

[Jaegeuk Kim]

If two readahead threads having same offset enter in readpages, every read
IOs are split and issued to the disk which giving lower bandwidth.

This patch tries to avoid redundant readahead calls.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/data.c  | 15 +++++++++++++++
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/super.c |  2 ++
 3 files changed, 18 insertions(+)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index dfd3225153570..1886d83bc5f15 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2292,6 +2292,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	unsigned nr_pages = rac ? readahead_count(rac) : 1;
 	unsigned max_nr_pages = nr_pages;
 	int ret = 0;
+	bool drop_ra = false;
 
 	map.m_pblk = 0;
 	map.m_lblk = 0;
@@ -2302,6 +2303,17 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	map.m_seg_type = NO_CHECK_TYPE;
 	map.m_may_create = false;
 
+	/*
+	 * Two readahead threads for same address range can cause race condition
+	 * which fragments sequential read IOs. So let's avoid each other.
+	 */
+	if (rac && readahead_count(rac)) {
+		if (F2FS_I(inode)->ra_offset == readahead_index(rac))
+			drop_ra = true;
+		else
+			F2FS_I(inode)->ra_offset = readahead_index(rac);
+	}
+
 	for (; nr_pages; nr_pages--) {
 		if (rac) {
 			page = readahead_page(rac);
@@ -2368,6 +2380,9 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	}
 	if (bio)
 		__submit_bio(F2FS_I_SB(inode), bio, DATA);
+
+	if (rac && readahead_count(rac) && !drop_ra)
+		F2FS_I(inode)->ra_offset = -1;
 	return ret;
 }
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 7fb2a1a334388..753782426feac 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -809,6 +809,7 @@ struct f2fs_inode_info {
 	struct list_head inmem_pages;	/* inmemory pages managed by f2fs */
 	struct task_struct *inmem_task;	/* store inmemory task */
 	struct mutex inmem_lock;	/* lock for inmemory pages */
+	pgoff_t ra_offset;		/* ongoing readahead offset */
 	struct extent_tree *extent_tree;	/* cached extent_tree entry */
 
 	/* avoid racing between foreground op and gc */
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 7326522057378..80cb7cd358f84 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1015,6 +1015,8 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
 	/* Will be used by directory only */
 	fi->i_dir_level = F2FS_SB(sb)->dir_level;
 
+	fi->ra_offset = -1;
+
 	return &fi->vfs_inode;
 }
 
-- 
2.27.0.111.gc72c7da667-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: avoid readahead race condition

[Chao Yu]

On 2020/6/24 9:21, Jaegeuk Kim wrote:
> If two readahead threads having same offset enter in readpages, every read
> IOs are split and issued to the disk which giving lower bandwidth.
> 
> This patch tries to avoid redundant readahead calls.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/data.c  | 15 +++++++++++++++
>  fs/f2fs/f2fs.h  |  1 +
>  fs/f2fs/super.c |  2 ++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index dfd3225153570..1886d83bc5f15 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2292,6 +2292,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
>  	unsigned nr_pages = rac ? readahead_count(rac) : 1;
>  	unsigned max_nr_pages = nr_pages;
>  	int ret = 0;
> +	bool drop_ra = false;
>  
>  	map.m_pblk = 0;
>  	map.m_lblk = 0;
> @@ -2302,6 +2303,17 @@ static int f2fs_mpage_readpages(struct inode *inode,
>  	map.m_seg_type = NO_CHECK_TYPE;
>  	map.m_may_create = false;
>  
> +	/*
> +	 * Two readahead threads for same address range can cause race condition
> +	 * which fragments sequential read IOs. So let's avoid each other.
> +	 */
> +	if (rac && readahead_count(rac)) {
> +		if (F2FS_I(inode)->ra_offset == readahead_index(rac))
> +			drop_ra = true;

I guess you missed to return at somewhere when drop_ra is true?

thanks,

> +		else
> +			F2FS_I(inode)->ra_offset = readahead_index(rac);
> +	}
> +
>  	for (; nr_pages; nr_pages--) {
>  		if (rac) {
>  			page = readahead_page(rac);
> @@ -2368,6 +2380,9 @@ static int f2fs_mpage_readpages(struct inode *inode,
>  	}
>  	if (bio)
>  		__submit_bio(F2FS_I_SB(inode), bio, DATA);
> +
> +	if (rac && readahead_count(rac) && !drop_ra)
> +		F2FS_I(inode)->ra_offset = -1;
>  	return ret;
>  }
>  
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 7fb2a1a334388..753782426feac 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -809,6 +809,7 @@ struct f2fs_inode_info {
>  	struct list_head inmem_pages;	/* inmemory pages managed by f2fs */
>  	struct task_struct *inmem_task;	/* store inmemory task */
>  	struct mutex inmem_lock;	/* lock for inmemory pages */
> +	pgoff_t ra_offset;		/* ongoing readahead offset */
>  	struct extent_tree *extent_tree;	/* cached extent_tree entry */
>  
>  	/* avoid racing between foreground op and gc */
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 7326522057378..80cb7cd358f84 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1015,6 +1015,8 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
>  	/* Will be used by directory only */
>  	fi->i_dir_level = F2FS_SB(sb)->dir_level;
>  
> +	fi->ra_offset = -1;
> +
>  	return &fi->vfs_inode;
>  }
>  
> 

Re: [f2fs-dev] [PATCH] f2fs: avoid readahead race condition

[Chao Yu]

On 2020/6/24 9:21, Jaegeuk Kim wrote:
> If two readahead threads having same offset enter in readpages, every read
> IOs are split and issued to the disk which giving lower bandwidth.
> 
> This patch tries to avoid redundant readahead calls.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/data.c  | 15 +++++++++++++++
>  fs/f2fs/f2fs.h  |  1 +
>  fs/f2fs/super.c |  2 ++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index dfd3225153570..1886d83bc5f15 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2292,6 +2292,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
>  	unsigned nr_pages = rac ? readahead_count(rac) : 1;
>  	unsigned max_nr_pages = nr_pages;
>  	int ret = 0;
> +	bool drop_ra = false;
>  
>  	map.m_pblk = 0;
>  	map.m_lblk = 0;
> @@ -2302,6 +2303,17 @@ static int f2fs_mpage_readpages(struct inode *inode,
>  	map.m_seg_type = NO_CHECK_TYPE;
>  	map.m_may_create = false;
>  
> +	/*
> +	 * Two readahead threads for same address range can cause race condition
> +	 * which fragments sequential read IOs. So let's avoid each other.
> +	 */
> +	if (rac && readahead_count(rac)) {
> +		if (F2FS_I(inode)->ra_offset == readahead_index(rac))
> +			drop_ra = true;

I guess you missed to return at somewhere when drop_ra is true?

thanks,

> +		else
> +			F2FS_I(inode)->ra_offset = readahead_index(rac);
> +	}
> +
>  	for (; nr_pages; nr_pages--) {
>  		if (rac) {
>  			page = readahead_page(rac);
> @@ -2368,6 +2380,9 @@ static int f2fs_mpage_readpages(struct inode *inode,
>  	}
>  	if (bio)
>  		__submit_bio(F2FS_I_SB(inode), bio, DATA);
> +
> +	if (rac && readahead_count(rac) && !drop_ra)
> +		F2FS_I(inode)->ra_offset = -1;
>  	return ret;
>  }
>  
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 7fb2a1a334388..753782426feac 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -809,6 +809,7 @@ struct f2fs_inode_info {
>  	struct list_head inmem_pages;	/* inmemory pages managed by f2fs */
>  	struct task_struct *inmem_task;	/* store inmemory task */
>  	struct mutex inmem_lock;	/* lock for inmemory pages */
> +	pgoff_t ra_offset;		/* ongoing readahead offset */
>  	struct extent_tree *extent_tree;	/* cached extent_tree entry */
>  
>  	/* avoid racing between foreground op and gc */
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 7326522057378..80cb7cd358f84 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1015,6 +1015,8 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
>  	/* Will be used by directory only */
>  	fi->i_dir_level = F2FS_SB(sb)->dir_level;
>  
> +	fi->ra_offset = -1;
> +
>  	return &fi->vfs_inode;
>  }
>  
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v2] f2fs: avoid readahead race condition

[Jaegeuk Kim]

If two readahead threads having same offset enter in readpages, every read
IOs are split and issued to the disk which giving lower bandwidth.

This patch tries to avoid redundant readahead calls.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
v2:
 - add missing code to bypass read

 fs/f2fs/data.c  | 18 +++++++++++++++++-
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/super.c |  2 ++
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index d6094b9f3916..9b69a159cc6c 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2403,6 +2403,7 @@ int f2fs_mpage_readpages(struct address_space *mapping,
 #endif
 	unsigned max_nr_pages = nr_pages;
 	int ret = 0;
+	bool drop_ra = false;
 
 	map.m_pblk = 0;
 	map.m_lblk = 0;
@@ -2413,13 +2414,25 @@ int f2fs_mpage_readpages(struct address_space *mapping,
 	map.m_seg_type = NO_CHECK_TYPE;
 	map.m_may_create = false;
 
+	/*
+	 * Two readahead threads for same address range can cause race condition
+	 * which fragments sequential read IOs. So let's avoid each other.
+	 */
+	if (pages && is_readahead) {
+		page = list_last_entry(pages, struct page, lru);
+		if (F2FS_I(inode)->ra_offset == page_index(page))
+			drop_ra = true;
+		else
+			F2FS_I(inode)->ra_offset = page_index(page);
+	}
+
 	for (; nr_pages; nr_pages--) {
 		if (pages) {
 			page = list_last_entry(pages, struct page, lru);
 
 			prefetchw(&page->flags);
 			list_del(&page->lru);
-			if (add_to_page_cache_lru(page, mapping,
+			if (drop_ra || add_to_page_cache_lru(page, mapping,
 						  page_index(page),
 						  readahead_gfp_mask(mapping)))
 				goto next_page;
@@ -2484,6 +2497,9 @@ int f2fs_mpage_readpages(struct address_space *mapping,
 	BUG_ON(pages && !list_empty(pages));
 	if (bio)
 		__f2fs_submit_read_bio(F2FS_I_SB(inode), bio, DATA);
+
+	if (pages && is_readahead && !drop_ra)
+		F2FS_I(inode)->ra_offset = -1;
 	return pages ? 0 : ret;
 }
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 35afa13124b8..a95f84d72a55 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -806,6 +806,7 @@ struct f2fs_inode_info {
 	struct list_head inmem_pages;	/* inmemory pages managed by f2fs */
 	struct task_struct *inmem_task;	/* store inmemory task */
 	struct mutex inmem_lock;	/* lock for inmemory pages */
+	pgoff_t ra_offset;		/* ongoing readahead offset */
 	struct extent_tree *extent_tree;	/* cached extent_tree entry */
 
 	/* avoid racing between foreground op and gc */
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 0e860186a9c5..6fd2ad43d9e4 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1011,6 +1011,8 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
 	/* Will be used by directory only */
 	fi->i_dir_level = F2FS_SB(sb)->dir_level;
 
+	fi->ra_offset = -1;
+
 	return &fi->vfs_inode;
 }
 
-- 
2.27.0.111.gc72c7da667-goog

Re: [f2fs-dev] [PATCH v2] f2fs: avoid readahead race condition

[Jaegeuk Kim]

If two readahead threads having same offset enter in readpages, every read
IOs are split and issued to the disk which giving lower bandwidth.

This patch tries to avoid redundant readahead calls.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
v2:
 - add missing code to bypass read

 fs/f2fs/data.c  | 18 +++++++++++++++++-
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/super.c |  2 ++
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index d6094b9f3916..9b69a159cc6c 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2403,6 +2403,7 @@ int f2fs_mpage_readpages(struct address_space *mapping,
 #endif
 	unsigned max_nr_pages = nr_pages;
 	int ret = 0;
+	bool drop_ra = false;
 
 	map.m_pblk = 0;
 	map.m_lblk = 0;
@@ -2413,13 +2414,25 @@ int f2fs_mpage_readpages(struct address_space *mapping,
 	map.m_seg_type = NO_CHECK_TYPE;
 	map.m_may_create = false;
 
+	/*
+	 * Two readahead threads for same address range can cause race condition
+	 * which fragments sequential read IOs. So let's avoid each other.
+	 */
+	if (pages && is_readahead) {
+		page = list_last_entry(pages, struct page, lru);
+		if (F2FS_I(inode)->ra_offset == page_index(page))
+			drop_ra = true;
+		else
+			F2FS_I(inode)->ra_offset = page_index(page);
+	}
+
 	for (; nr_pages; nr_pages--) {
 		if (pages) {
 			page = list_last_entry(pages, struct page, lru);
 
 			prefetchw(&page->flags);
 			list_del(&page->lru);
-			if (add_to_page_cache_lru(page, mapping,
+			if (drop_ra || add_to_page_cache_lru(page, mapping,
 						  page_index(page),
 						  readahead_gfp_mask(mapping)))
 				goto next_page;
@@ -2484,6 +2497,9 @@ int f2fs_mpage_readpages(struct address_space *mapping,
 	BUG_ON(pages && !list_empty(pages));
 	if (bio)
 		__f2fs_submit_read_bio(F2FS_I_SB(inode), bio, DATA);
+
+	if (pages && is_readahead && !drop_ra)
+		F2FS_I(inode)->ra_offset = -1;
 	return pages ? 0 : ret;
 }
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 35afa13124b8..a95f84d72a55 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -806,6 +806,7 @@ struct f2fs_inode_info {
 	struct list_head inmem_pages;	/* inmemory pages managed by f2fs */
 	struct task_struct *inmem_task;	/* store inmemory task */
 	struct mutex inmem_lock;	/* lock for inmemory pages */
+	pgoff_t ra_offset;		/* ongoing readahead offset */
 	struct extent_tree *extent_tree;	/* cached extent_tree entry */
 
 	/* avoid racing between foreground op and gc */
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 0e860186a9c5..6fd2ad43d9e4 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1011,6 +1011,8 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
 	/* Will be used by directory only */
 	fi->i_dir_level = F2FS_SB(sb)->dir_level;
 
+	fi->ra_offset = -1;
+
 	return &fi->vfs_inode;
 }
 
-- 
2.27.0.111.gc72c7da667-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v2] f2fs: avoid readahead race condition

[Eric Biggers]

On Mon, Jun 29, 2020 at 08:03:23AM -0700, Jaegeuk Kim wrote:
> If two readahead threads having same offset enter in readpages, every read
> IOs are split and issued to the disk which giving lower bandwidth.
> 
> This patch tries to avoid redundant readahead calls.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> v2:
>  - add missing code to bypass read
> 
>  fs/f2fs/data.c  | 18 +++++++++++++++++-
>  fs/f2fs/f2fs.h  |  1 +
>  fs/f2fs/super.c |  2 ++
>  3 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index d6094b9f3916..9b69a159cc6c 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2403,6 +2403,7 @@ int f2fs_mpage_readpages(struct address_space *mapping,
>  #endif
>  	unsigned max_nr_pages = nr_pages;
>  	int ret = 0;
> +	bool drop_ra = false;
>  
>  	map.m_pblk = 0;
>  	map.m_lblk = 0;
> @@ -2413,13 +2414,25 @@ int f2fs_mpage_readpages(struct address_space *mapping,
>  	map.m_seg_type = NO_CHECK_TYPE;
>  	map.m_may_create = false;
>  
> +	/*
> +	 * Two readahead threads for same address range can cause race condition
> +	 * which fragments sequential read IOs. So let's avoid each other.
> +	 */
> +	if (pages && is_readahead) {
> +		page = list_last_entry(pages, struct page, lru);
> +		if (F2FS_I(inode)->ra_offset == page_index(page))
> +			drop_ra = true;
> +		else
> +			F2FS_I(inode)->ra_offset = page_index(page);
> +	}

This is a data race because ra_offset can be read/written by different threads
concurrently.

It either needs locking, or READ_ONCE() and WRITE_ONCE() if races are okay.

- Eric

Re: [f2fs-dev] [PATCH v2] f2fs: avoid readahead race condition

[Eric Biggers]

On Mon, Jun 29, 2020 at 08:03:23AM -0700, Jaegeuk Kim wrote:
> If two readahead threads having same offset enter in readpages, every read
> IOs are split and issued to the disk which giving lower bandwidth.
> 
> This patch tries to avoid redundant readahead calls.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> v2:
>  - add missing code to bypass read
> 
>  fs/f2fs/data.c  | 18 +++++++++++++++++-
>  fs/f2fs/f2fs.h  |  1 +
>  fs/f2fs/super.c |  2 ++
>  3 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index d6094b9f3916..9b69a159cc6c 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2403,6 +2403,7 @@ int f2fs_mpage_readpages(struct address_space *mapping,
>  #endif
>  	unsigned max_nr_pages = nr_pages;
>  	int ret = 0;
> +	bool drop_ra = false;
>  
>  	map.m_pblk = 0;
>  	map.m_lblk = 0;
> @@ -2413,13 +2414,25 @@ int f2fs_mpage_readpages(struct address_space *mapping,
>  	map.m_seg_type = NO_CHECK_TYPE;
>  	map.m_may_create = false;
>  
> +	/*
> +	 * Two readahead threads for same address range can cause race condition
> +	 * which fragments sequential read IOs. So let's avoid each other.
> +	 */
> +	if (pages && is_readahead) {
> +		page = list_last_entry(pages, struct page, lru);
> +		if (F2FS_I(inode)->ra_offset == page_index(page))
> +			drop_ra = true;
> +		else
> +			F2FS_I(inode)->ra_offset = page_index(page);
> +	}

This is a data race because ra_offset can be read/written by different threads
concurrently.

It either needs locking, or READ_ONCE() and WRITE_ONCE() if races are okay.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v2] f2fs: avoid readahead race condition

[Jaegeuk Kim]

On 06/29, Eric Biggers wrote:
> On Mon, Jun 29, 2020 at 08:03:23AM -0700, Jaegeuk Kim wrote:
> > If two readahead threads having same offset enter in readpages, every read
> > IOs are split and issued to the disk which giving lower bandwidth.
> > 
> > This patch tries to avoid redundant readahead calls.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> > v2:
> >  - add missing code to bypass read
> > 
> >  fs/f2fs/data.c  | 18 +++++++++++++++++-
> >  fs/f2fs/f2fs.h  |  1 +
> >  fs/f2fs/super.c |  2 ++
> >  3 files changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index d6094b9f3916..9b69a159cc6c 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -2403,6 +2403,7 @@ int f2fs_mpage_readpages(struct address_space *mapping,
> >  #endif
> >  	unsigned max_nr_pages = nr_pages;
> >  	int ret = 0;
> > +	bool drop_ra = false;
> >  
> >  	map.m_pblk = 0;
> >  	map.m_lblk = 0;
> > @@ -2413,13 +2414,25 @@ int f2fs_mpage_readpages(struct address_space *mapping,
> >  	map.m_seg_type = NO_CHECK_TYPE;
> >  	map.m_may_create = false;
> >  
> > +	/*
> > +	 * Two readahead threads for same address range can cause race condition
> > +	 * which fragments sequential read IOs. So let's avoid each other.
> > +	 */
> > +	if (pages && is_readahead) {
> > +		page = list_last_entry(pages, struct page, lru);
> > +		if (F2FS_I(inode)->ra_offset == page_index(page))
> > +			drop_ra = true;
> > +		else
> > +			F2FS_I(inode)->ra_offset = page_index(page);
> > +	}
> 
> This is a data race because ra_offset can be read/written by different threads
> concurrently.
> 
> It either needs locking, or READ_ONCE() and WRITE_ONCE() if races are okay.

I just wanted to keep zero overhead, since it doesn't matter either cases of
skipping readahead or not.

> 
> - Eric

Re: [f2fs-dev] [PATCH v2] f2fs: avoid readahead race condition

[Jaegeuk Kim]

On 06/29, Eric Biggers wrote:
> On Mon, Jun 29, 2020 at 08:03:23AM -0700, Jaegeuk Kim wrote:
> > If two readahead threads having same offset enter in readpages, every read
> > IOs are split and issued to the disk which giving lower bandwidth.
> > 
> > This patch tries to avoid redundant readahead calls.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> > v2:
> >  - add missing code to bypass read
> > 
> >  fs/f2fs/data.c  | 18 +++++++++++++++++-
> >  fs/f2fs/f2fs.h  |  1 +
> >  fs/f2fs/super.c |  2 ++
> >  3 files changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index d6094b9f3916..9b69a159cc6c 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -2403,6 +2403,7 @@ int f2fs_mpage_readpages(struct address_space *mapping,
> >  #endif
> >  	unsigned max_nr_pages = nr_pages;
> >  	int ret = 0;
> > +	bool drop_ra = false;
> >  
> >  	map.m_pblk = 0;
> >  	map.m_lblk = 0;
> > @@ -2413,13 +2414,25 @@ int f2fs_mpage_readpages(struct address_space *mapping,
> >  	map.m_seg_type = NO_CHECK_TYPE;
> >  	map.m_may_create = false;
> >  
> > +	/*
> > +	 * Two readahead threads for same address range can cause race condition
> > +	 * which fragments sequential read IOs. So let's avoid each other.
> > +	 */
> > +	if (pages && is_readahead) {
> > +		page = list_last_entry(pages, struct page, lru);
> > +		if (F2FS_I(inode)->ra_offset == page_index(page))
> > +			drop_ra = true;
> > +		else
> > +			F2FS_I(inode)->ra_offset = page_index(page);
> > +	}
> 
> This is a data race because ra_offset can be read/written by different threads
> concurrently.
> 
> It either needs locking, or READ_ONCE() and WRITE_ONCE() if races are okay.

I just wanted to keep zero overhead, since it doesn't matter either cases of
skipping readahead or not.

> 
> - Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v2] f2fs: avoid readahead race condition

[Eric Biggers]

On Mon, Jun 29, 2020 at 11:24:14AM -0700, Jaegeuk Kim wrote:
> On 06/29, Eric Biggers wrote:
> > On Mon, Jun 29, 2020 at 08:03:23AM -0700, Jaegeuk Kim wrote:
> > > If two readahead threads having same offset enter in readpages, every read
> > > IOs are split and issued to the disk which giving lower bandwidth.
> > > 
> > > This patch tries to avoid redundant readahead calls.
> > > 
> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > ---
> > > v2:
> > >  - add missing code to bypass read
> > > 
> > >  fs/f2fs/data.c  | 18 +++++++++++++++++-
> > >  fs/f2fs/f2fs.h  |  1 +
> > >  fs/f2fs/super.c |  2 ++
> > >  3 files changed, 20 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > index d6094b9f3916..9b69a159cc6c 100644
> > > --- a/fs/f2fs/data.c
> > > +++ b/fs/f2fs/data.c
> > > @@ -2403,6 +2403,7 @@ int f2fs_mpage_readpages(struct address_space *mapping,
> > >  #endif
> > >  	unsigned max_nr_pages = nr_pages;
> > >  	int ret = 0;
> > > +	bool drop_ra = false;
> > >  
> > >  	map.m_pblk = 0;
> > >  	map.m_lblk = 0;
> > > @@ -2413,13 +2414,25 @@ int f2fs_mpage_readpages(struct address_space *mapping,
> > >  	map.m_seg_type = NO_CHECK_TYPE;
> > >  	map.m_may_create = false;
> > >  
> > > +	/*
> > > +	 * Two readahead threads for same address range can cause race condition
> > > +	 * which fragments sequential read IOs. So let's avoid each other.
> > > +	 */
> > > +	if (pages && is_readahead) {
> > > +		page = list_last_entry(pages, struct page, lru);
> > > +		if (F2FS_I(inode)->ra_offset == page_index(page))
> > > +			drop_ra = true;
> > > +		else
> > > +			F2FS_I(inode)->ra_offset = page_index(page);
> > > +	}
> > 
> > This is a data race because ra_offset can be read/written by different threads
> > concurrently.
> > 
> > It either needs locking, or READ_ONCE() and WRITE_ONCE() if races are okay.
> 
> I just wanted to keep zero overhead, since it doesn't matter either cases of
> skipping readahead or not.
> 

Okay, then it should use READ_ONCE() and WRITE_ONCE().

- Eric

Re: [f2fs-dev] [PATCH v2] f2fs: avoid readahead race condition

[Eric Biggers]

On Mon, Jun 29, 2020 at 11:24:14AM -0700, Jaegeuk Kim wrote:
> On 06/29, Eric Biggers wrote:
> > On Mon, Jun 29, 2020 at 08:03:23AM -0700, Jaegeuk Kim wrote:
> > > If two readahead threads having same offset enter in readpages, every read
> > > IOs are split and issued to the disk which giving lower bandwidth.
> > > 
> > > This patch tries to avoid redundant readahead calls.
> > > 
> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > ---
> > > v2:
> > >  - add missing code to bypass read
> > > 
> > >  fs/f2fs/data.c  | 18 +++++++++++++++++-
> > >  fs/f2fs/f2fs.h  |  1 +
> > >  fs/f2fs/super.c |  2 ++
> > >  3 files changed, 20 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > index d6094b9f3916..9b69a159cc6c 100644
> > > --- a/fs/f2fs/data.c
> > > +++ b/fs/f2fs/data.c
> > > @@ -2403,6 +2403,7 @@ int f2fs_mpage_readpages(struct address_space *mapping,
> > >  #endif
> > >  	unsigned max_nr_pages = nr_pages;
> > >  	int ret = 0;
> > > +	bool drop_ra = false;
> > >  
> > >  	map.m_pblk = 0;
> > >  	map.m_lblk = 0;
> > > @@ -2413,13 +2414,25 @@ int f2fs_mpage_readpages(struct address_space *mapping,
> > >  	map.m_seg_type = NO_CHECK_TYPE;
> > >  	map.m_may_create = false;
> > >  
> > > +	/*
> > > +	 * Two readahead threads for same address range can cause race condition
> > > +	 * which fragments sequential read IOs. So let's avoid each other.
> > > +	 */
> > > +	if (pages && is_readahead) {
> > > +		page = list_last_entry(pages, struct page, lru);
> > > +		if (F2FS_I(inode)->ra_offset == page_index(page))
> > > +			drop_ra = true;
> > > +		else
> > > +			F2FS_I(inode)->ra_offset = page_index(page);
> > > +	}
> > 
> > This is a data race because ra_offset can be read/written by different threads
> > concurrently.
> > 
> > It either needs locking, or READ_ONCE() and WRITE_ONCE() if races are okay.
> 
> I just wanted to keep zero overhead, since it doesn't matter either cases of
> skipping readahead or not.
> 

Okay, then it should use READ_ONCE() and WRITE_ONCE().

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v3] f2fs: avoid readahead race condition

[Jaegeuk Kim]

If two readahead threads having same offset enter in readpages, every read
IOs are split and issued to the disk which giving lower bandwidth.

This patch tries to avoid redundant readahead calls.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
v3:
 - use READ|WRITE_ONCE
v2:
  - add missing code to bypass read
 
 fs/f2fs/data.c  | 18 ++++++++++++++++++
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/super.c |  2 ++
 3 files changed, 21 insertions(+)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 995cf78b23c5e..360b4c9080d97 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2296,6 +2296,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	unsigned nr_pages = rac ? readahead_count(rac) : 1;
 	unsigned max_nr_pages = nr_pages;
 	int ret = 0;
+	bool drop_ra = false;
 
 	map.m_pblk = 0;
 	map.m_lblk = 0;
@@ -2306,10 +2307,24 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	map.m_seg_type = NO_CHECK_TYPE;
 	map.m_may_create = false;
 
+	/*
+	 * Two readahead threads for same address range can cause race condition
+	 * which fragments sequential read IOs. So let's avoid each other.
+	 */
+	if (rac && readahead_count(rac)) {
+		if (READ_ONCE(F2FS_I(inode)->ra_offset) == readahead_index(rac))
+			drop_ra = true;
+		else
+			WRITE_ONCE(F2FS_I(inode)->ra_offset,
+						readahead_index(rac));
+	}
+
 	for (; nr_pages; nr_pages--) {
 		if (rac) {
 			page = readahead_page(rac);
 			prefetchw(&page->flags);
+			if (drop_ra)
+				goto next_page;
 		}
 
 #ifdef CONFIG_F2FS_FS_COMPRESSION
@@ -2372,6 +2387,9 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	}
 	if (bio)
 		__submit_bio(F2FS_I_SB(inode), bio, DATA);
+
+	if (rac && readahead_count(rac) && !drop_ra)
+		WRITE_ONCE(F2FS_I(inode)->ra_offset, -1);
 	return ret;
 }
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 6a655edeb522f..e6e47618a3576 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -809,6 +809,7 @@ struct f2fs_inode_info {
 	struct list_head inmem_pages;	/* inmemory pages managed by f2fs */
 	struct task_struct *inmem_task;	/* store inmemory task */
 	struct mutex inmem_lock;	/* lock for inmemory pages */
+	pgoff_t ra_offset;		/* ongoing readahead offset */
 	struct extent_tree *extent_tree;	/* cached extent_tree entry */
 
 	/* avoid racing between foreground op and gc */
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 7326522057378..80cb7cd358f84 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1015,6 +1015,8 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
 	/* Will be used by directory only */
 	fi->i_dir_level = F2FS_SB(sb)->dir_level;
 
+	fi->ra_offset = -1;
+
 	return &fi->vfs_inode;
 }
 
-- 
2.27.0.212.ge8ba1cc988-goog

Re: [f2fs-dev] [PATCH v3] f2fs: avoid readahead race condition

[Jaegeuk Kim]

If two readahead threads having same offset enter in readpages, every read
IOs are split and issued to the disk which giving lower bandwidth.

This patch tries to avoid redundant readahead calls.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
v3:
 - use READ|WRITE_ONCE
v2:
  - add missing code to bypass read
 
 fs/f2fs/data.c  | 18 ++++++++++++++++++
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/super.c |  2 ++
 3 files changed, 21 insertions(+)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 995cf78b23c5e..360b4c9080d97 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2296,6 +2296,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	unsigned nr_pages = rac ? readahead_count(rac) : 1;
 	unsigned max_nr_pages = nr_pages;
 	int ret = 0;
+	bool drop_ra = false;
 
 	map.m_pblk = 0;
 	map.m_lblk = 0;
@@ -2306,10 +2307,24 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	map.m_seg_type = NO_CHECK_TYPE;
 	map.m_may_create = false;
 
+	/*
+	 * Two readahead threads for same address range can cause race condition
+	 * which fragments sequential read IOs. So let's avoid each other.
+	 */
+	if (rac && readahead_count(rac)) {
+		if (READ_ONCE(F2FS_I(inode)->ra_offset) == readahead_index(rac))
+			drop_ra = true;
+		else
+			WRITE_ONCE(F2FS_I(inode)->ra_offset,
+						readahead_index(rac));
+	}
+
 	for (; nr_pages; nr_pages--) {
 		if (rac) {
 			page = readahead_page(rac);
 			prefetchw(&page->flags);
+			if (drop_ra)
+				goto next_page;
 		}
 
 #ifdef CONFIG_F2FS_FS_COMPRESSION
@@ -2372,6 +2387,9 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	}
 	if (bio)
 		__submit_bio(F2FS_I_SB(inode), bio, DATA);
+
+	if (rac && readahead_count(rac) && !drop_ra)
+		WRITE_ONCE(F2FS_I(inode)->ra_offset, -1);
 	return ret;
 }
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 6a655edeb522f..e6e47618a3576 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -809,6 +809,7 @@ struct f2fs_inode_info {
 	struct list_head inmem_pages;	/* inmemory pages managed by f2fs */
 	struct task_struct *inmem_task;	/* store inmemory task */
 	struct mutex inmem_lock;	/* lock for inmemory pages */
+	pgoff_t ra_offset;		/* ongoing readahead offset */
 	struct extent_tree *extent_tree;	/* cached extent_tree entry */
 
 	/* avoid racing between foreground op and gc */
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 7326522057378..80cb7cd358f84 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1015,6 +1015,8 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
 	/* Will be used by directory only */
 	fi->i_dir_level = F2FS_SB(sb)->dir_level;
 
+	fi->ra_offset = -1;
+
 	return &fi->vfs_inode;
 }
 
-- 
2.27.0.212.ge8ba1cc988-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v3] f2fs: avoid readahead race condition

[Nathan Chancellor]

On Mon, Jun 29, 2020 at 01:27:20PM -0700, Jaegeuk Kim wrote:
> If two readahead threads having same offset enter in readpages, every read
> IOs are split and issued to the disk which giving lower bandwidth.
> 
> This patch tries to avoid redundant readahead calls.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> v3:
>  - use READ|WRITE_ONCE
> v2:
>   - add missing code to bypass read
>  
>  fs/f2fs/data.c  | 18 ++++++++++++++++++
>  fs/f2fs/f2fs.h  |  1 +
>  fs/f2fs/super.c |  2 ++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 995cf78b23c5e..360b4c9080d97 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2296,6 +2296,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
>  	unsigned nr_pages = rac ? readahead_count(rac) : 1;
>  	unsigned max_nr_pages = nr_pages;
>  	int ret = 0;
> +	bool drop_ra = false;
>  
>  	map.m_pblk = 0;
>  	map.m_lblk = 0;
> @@ -2306,10 +2307,24 @@ static int f2fs_mpage_readpages(struct inode *inode,
>  	map.m_seg_type = NO_CHECK_TYPE;
>  	map.m_may_create = false;
>  
> +	/*
> +	 * Two readahead threads for same address range can cause race condition
> +	 * which fragments sequential read IOs. So let's avoid each other.
> +	 */
> +	if (rac && readahead_count(rac)) {
> +		if (READ_ONCE(F2FS_I(inode)->ra_offset) == readahead_index(rac))
> +			drop_ra = true;
> +		else
> +			WRITE_ONCE(F2FS_I(inode)->ra_offset,
> +						readahead_index(rac));
> +	}
> +
>  	for (; nr_pages; nr_pages--) {
>  		if (rac) {
>  			page = readahead_page(rac);
>  			prefetchw(&page->flags);
> +			if (drop_ra)
> +				goto next_page;

When CONFIG_F2FS_FS_COMPRESSION is not set (i.e. x86_64 defconfig +
CONFIG_F2FS_FS=y):

$ make -skj"$(nproc)" O=out distclean defconfig fs/f2fs/data.o
../fs/f2fs/data.c: In function ‘f2fs_mpage_readpages’:
../fs/f2fs/data.c:2327:5: error: label ‘next_page’ used but not defined
 2327 |     goto next_page;
      |     ^~~~
...

Cheers,
Nathan

Re: [f2fs-dev] [PATCH v3] f2fs: avoid readahead race condition

[Nathan Chancellor]

On Mon, Jun 29, 2020 at 01:27:20PM -0700, Jaegeuk Kim wrote:
> If two readahead threads having same offset enter in readpages, every read
> IOs are split and issued to the disk which giving lower bandwidth.
> 
> This patch tries to avoid redundant readahead calls.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> v3:
>  - use READ|WRITE_ONCE
> v2:
>   - add missing code to bypass read
>  
>  fs/f2fs/data.c  | 18 ++++++++++++++++++
>  fs/f2fs/f2fs.h  |  1 +
>  fs/f2fs/super.c |  2 ++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 995cf78b23c5e..360b4c9080d97 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2296,6 +2296,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
>  	unsigned nr_pages = rac ? readahead_count(rac) : 1;
>  	unsigned max_nr_pages = nr_pages;
>  	int ret = 0;
> +	bool drop_ra = false;
>  
>  	map.m_pblk = 0;
>  	map.m_lblk = 0;
> @@ -2306,10 +2307,24 @@ static int f2fs_mpage_readpages(struct inode *inode,
>  	map.m_seg_type = NO_CHECK_TYPE;
>  	map.m_may_create = false;
>  
> +	/*
> +	 * Two readahead threads for same address range can cause race condition
> +	 * which fragments sequential read IOs. So let's avoid each other.
> +	 */
> +	if (rac && readahead_count(rac)) {
> +		if (READ_ONCE(F2FS_I(inode)->ra_offset) == readahead_index(rac))
> +			drop_ra = true;
> +		else
> +			WRITE_ONCE(F2FS_I(inode)->ra_offset,
> +						readahead_index(rac));
> +	}
> +
>  	for (; nr_pages; nr_pages--) {
>  		if (rac) {
>  			page = readahead_page(rac);
>  			prefetchw(&page->flags);
> +			if (drop_ra)
> +				goto next_page;

When CONFIG_F2FS_FS_COMPRESSION is not set (i.e. x86_64 defconfig +
CONFIG_F2FS_FS=y):

$ make -skj"$(nproc)" O=out distclean defconfig fs/f2fs/data.o
../fs/f2fs/data.c: In function ‘f2fs_mpage_readpages’:
../fs/f2fs/data.c:2327:5: error: label ‘next_page’ used but not defined
 2327 |     goto next_page;
      |     ^~~~
...

Cheers,
Nathan


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v3] f2fs: avoid readahead race condition

[Jaegeuk Kim]

On 06/30, Nathan Chancellor wrote:
> On Mon, Jun 29, 2020 at 01:27:20PM -0700, Jaegeuk Kim wrote:
> > If two readahead threads having same offset enter in readpages, every read
> > IOs are split and issued to the disk which giving lower bandwidth.
> > 
> > This patch tries to avoid redundant readahead calls.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> > v3:
> >  - use READ|WRITE_ONCE
> > v2:
> >   - add missing code to bypass read
> >  
> >  fs/f2fs/data.c  | 18 ++++++++++++++++++
> >  fs/f2fs/f2fs.h  |  1 +
> >  fs/f2fs/super.c |  2 ++
> >  3 files changed, 21 insertions(+)
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 995cf78b23c5e..360b4c9080d97 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -2296,6 +2296,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
> >  	unsigned nr_pages = rac ? readahead_count(rac) : 1;
> >  	unsigned max_nr_pages = nr_pages;
> >  	int ret = 0;
> > +	bool drop_ra = false;
> >  
> >  	map.m_pblk = 0;
> >  	map.m_lblk = 0;
> > @@ -2306,10 +2307,24 @@ static int f2fs_mpage_readpages(struct inode *inode,
> >  	map.m_seg_type = NO_CHECK_TYPE;
> >  	map.m_may_create = false;
> >  
> > +	/*
> > +	 * Two readahead threads for same address range can cause race condition
> > +	 * which fragments sequential read IOs. So let's avoid each other.
> > +	 */
> > +	if (rac && readahead_count(rac)) {
> > +		if (READ_ONCE(F2FS_I(inode)->ra_offset) == readahead_index(rac))
> > +			drop_ra = true;
> > +		else
> > +			WRITE_ONCE(F2FS_I(inode)->ra_offset,
> > +						readahead_index(rac));
> > +	}
> > +
> >  	for (; nr_pages; nr_pages--) {
> >  		if (rac) {
> >  			page = readahead_page(rac);
> >  			prefetchw(&page->flags);
> > +			if (drop_ra)
> > +				goto next_page;
> 
> When CONFIG_F2FS_FS_COMPRESSION is not set (i.e. x86_64 defconfig +
> CONFIG_F2FS_FS=y):
> 
> $ make -skj"$(nproc)" O=out distclean defconfig fs/f2fs/data.o
> ../fs/f2fs/data.c: In function ‘f2fs_mpage_readpages’:
> ../fs/f2fs/data.c:2327:5: error: label ‘next_page’ used but not defined
>  2327 |     goto next_page;
>       |     ^~~~
> ...

Thanks. I pushed the fix for -next.
https://lore.kernel.org/linux-f2fs-devel/1be18397-7fc6-703e-121b-e210e101357f@infradead.org/T/#t

Thanks,

> 
> Cheers,
> Nathan
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v3] f2fs: avoid readahead race condition

[Jaegeuk Kim]

On 06/30, Nathan Chancellor wrote:
> On Mon, Jun 29, 2020 at 01:27:20PM -0700, Jaegeuk Kim wrote:
> > If two readahead threads having same offset enter in readpages, every read
> > IOs are split and issued to the disk which giving lower bandwidth.
> > 
> > This patch tries to avoid redundant readahead calls.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> > v3:
> >  - use READ|WRITE_ONCE
> > v2:
> >   - add missing code to bypass read
> >  
> >  fs/f2fs/data.c  | 18 ++++++++++++++++++
> >  fs/f2fs/f2fs.h  |  1 +
> >  fs/f2fs/super.c |  2 ++
> >  3 files changed, 21 insertions(+)
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 995cf78b23c5e..360b4c9080d97 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -2296,6 +2296,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
> >  	unsigned nr_pages = rac ? readahead_count(rac) : 1;
> >  	unsigned max_nr_pages = nr_pages;
> >  	int ret = 0;
> > +	bool drop_ra = false;
> >  
> >  	map.m_pblk = 0;
> >  	map.m_lblk = 0;
> > @@ -2306,10 +2307,24 @@ static int f2fs_mpage_readpages(struct inode *inode,
> >  	map.m_seg_type = NO_CHECK_TYPE;
> >  	map.m_may_create = false;
> >  
> > +	/*
> > +	 * Two readahead threads for same address range can cause race condition
> > +	 * which fragments sequential read IOs. So let's avoid each other.
> > +	 */
> > +	if (rac && readahead_count(rac)) {
> > +		if (READ_ONCE(F2FS_I(inode)->ra_offset) == readahead_index(rac))
> > +			drop_ra = true;
> > +		else
> > +			WRITE_ONCE(F2FS_I(inode)->ra_offset,
> > +						readahead_index(rac));
> > +	}
> > +
> >  	for (; nr_pages; nr_pages--) {
> >  		if (rac) {
> >  			page = readahead_page(rac);
> >  			prefetchw(&page->flags);
> > +			if (drop_ra)
> > +				goto next_page;
> 
> When CONFIG_F2FS_FS_COMPRESSION is not set (i.e. x86_64 defconfig +
> CONFIG_F2FS_FS=y):
> 
> $ make -skj"$(nproc)" O=out distclean defconfig fs/f2fs/data.o
> ../fs/f2fs/data.c: In function ‘f2fs_mpage_readpages’:
> ../fs/f2fs/data.c:2327:5: error: label ‘next_page’ used but not defined
>  2327 |     goto next_page;
>       |     ^~~~
> ...

Thanks. I pushed the fix for -next.
https://lore.kernel.org/linux-f2fs-devel/1be18397-7fc6-703e-121b-e210e101357f@infradead.org/T/#t

Thanks,

> 
> Cheers,
> Nathan
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v3] f2fs: avoid readahead race condition

[Chao Yu]

On 2020/7/1 4:56, Jaegeuk Kim wrote:
> On 06/30, Nathan Chancellor wrote:
>> On Mon, Jun 29, 2020 at 01:27:20PM -0700, Jaegeuk Kim wrote:
>>> If two readahead threads having same offset enter in readpages, every read
>>> IOs are split and issued to the disk which giving lower bandwidth.
>>>
>>> This patch tries to avoid redundant readahead calls.
>>>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>> v3:
>>>  - use READ|WRITE_ONCE
>>> v2:
>>>   - add missing code to bypass read
>>>  
>>>  fs/f2fs/data.c  | 18 ++++++++++++++++++
>>>  fs/f2fs/f2fs.h  |  1 +
>>>  fs/f2fs/super.c |  2 ++
>>>  3 files changed, 21 insertions(+)
>>>
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index 995cf78b23c5e..360b4c9080d97 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -2296,6 +2296,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
>>>  	unsigned nr_pages = rac ? readahead_count(rac) : 1;
>>>  	unsigned max_nr_pages = nr_pages;
>>>  	int ret = 0;
>>> +	bool drop_ra = false;
>>>  
>>>  	map.m_pblk = 0;
>>>  	map.m_lblk = 0;
>>> @@ -2306,10 +2307,24 @@ static int f2fs_mpage_readpages(struct inode *inode,
>>>  	map.m_seg_type = NO_CHECK_TYPE;
>>>  	map.m_may_create = false;
>>>  
>>> +	/*
>>> +	 * Two readahead threads for same address range can cause race condition
>>> +	 * which fragments sequential read IOs. So let's avoid each other.
>>> +	 */
>>> +	if (rac && readahead_count(rac)) {
>>> +		if (READ_ONCE(F2FS_I(inode)->ra_offset) == readahead_index(rac))
>>> +			drop_ra = true;
>>> +		else
>>> +			WRITE_ONCE(F2FS_I(inode)->ra_offset,
>>> +						readahead_index(rac));
>>> +	}
>>> +
>>>  	for (; nr_pages; nr_pages--) {
>>>  		if (rac) {
>>>  			page = readahead_page(rac);
>>>  			prefetchw(&page->flags);
>>> +			if (drop_ra)
>>> +				goto next_page;
>>
>> When CONFIG_F2FS_FS_COMPRESSION is not set (i.e. x86_64 defconfig +
>> CONFIG_F2FS_FS=y):
>>
>> $ make -skj"$(nproc)" O=out distclean defconfig fs/f2fs/data.o
>> ../fs/f2fs/data.c: In function ‘f2fs_mpage_readpages’:
>> ../fs/f2fs/data.c:2327:5: error: label ‘next_page’ used but not defined
>>  2327 |     goto next_page;
>>       |     ^~~~
>> ...
> 
> Thanks. I pushed the fix for -next.
> https://lore.kernel.org/linux-f2fs-devel/1be18397-7fc6-703e-121b-e210e101357f@infradead.org/T/#t

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

> 
> Thanks,
> 
>>
>> Cheers,
>> Nathan
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 

Re: [f2fs-dev] [PATCH v3] f2fs: avoid readahead race condition

[Chao Yu]

On 2020/7/1 4:56, Jaegeuk Kim wrote:
> On 06/30, Nathan Chancellor wrote:
>> On Mon, Jun 29, 2020 at 01:27:20PM -0700, Jaegeuk Kim wrote:
>>> If two readahead threads having same offset enter in readpages, every read
>>> IOs are split and issued to the disk which giving lower bandwidth.
>>>
>>> This patch tries to avoid redundant readahead calls.
>>>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>> v3:
>>>  - use READ|WRITE_ONCE
>>> v2:
>>>   - add missing code to bypass read
>>>  
>>>  fs/f2fs/data.c  | 18 ++++++++++++++++++
>>>  fs/f2fs/f2fs.h  |  1 +
>>>  fs/f2fs/super.c |  2 ++
>>>  3 files changed, 21 insertions(+)
>>>
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index 995cf78b23c5e..360b4c9080d97 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -2296,6 +2296,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
>>>  	unsigned nr_pages = rac ? readahead_count(rac) : 1;
>>>  	unsigned max_nr_pages = nr_pages;
>>>  	int ret = 0;
>>> +	bool drop_ra = false;
>>>  
>>>  	map.m_pblk = 0;
>>>  	map.m_lblk = 0;
>>> @@ -2306,10 +2307,24 @@ static int f2fs_mpage_readpages(struct inode *inode,
>>>  	map.m_seg_type = NO_CHECK_TYPE;
>>>  	map.m_may_create = false;
>>>  
>>> +	/*
>>> +	 * Two readahead threads for same address range can cause race condition
>>> +	 * which fragments sequential read IOs. So let's avoid each other.
>>> +	 */
>>> +	if (rac && readahead_count(rac)) {
>>> +		if (READ_ONCE(F2FS_I(inode)->ra_offset) == readahead_index(rac))
>>> +			drop_ra = true;
>>> +		else
>>> +			WRITE_ONCE(F2FS_I(inode)->ra_offset,
>>> +						readahead_index(rac));
>>> +	}
>>> +
>>>  	for (; nr_pages; nr_pages--) {
>>>  		if (rac) {
>>>  			page = readahead_page(rac);
>>>  			prefetchw(&page->flags);
>>> +			if (drop_ra)
>>> +				goto next_page;
>>
>> When CONFIG_F2FS_FS_COMPRESSION is not set (i.e. x86_64 defconfig +
>> CONFIG_F2FS_FS=y):
>>
>> $ make -skj"$(nproc)" O=out distclean defconfig fs/f2fs/data.o
>> ../fs/f2fs/data.c: In function ‘f2fs_mpage_readpages’:
>> ../fs/f2fs/data.c:2327:5: error: label ‘next_page’ used but not defined
>>  2327 |     goto next_page;
>>       |     ^~~~
>> ...
> 
> Thanks. I pushed the fix for -next.
> https://lore.kernel.org/linux-f2fs-devel/1be18397-7fc6-703e-121b-e210e101357f@infradead.org/T/#t

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

> 
> Thanks,
> 
>>
>> Cheers,
>> Nathan
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v3] f2fs: avoid readahead race condition

[Chao Yu]

On 2020/7/1 9:59, Chao Yu wrote:
> On 2020/7/1 4:56, Jaegeuk Kim wrote:
>> On 06/30, Nathan Chancellor wrote:
>>> On Mon, Jun 29, 2020 at 01:27:20PM -0700, Jaegeuk Kim wrote:
>>>> If two readahead threads having same offset enter in readpages, every read
>>>> IOs are split and issued to the disk which giving lower bandwidth.
>>>>
>>>> This patch tries to avoid redundant readahead calls.
>>>>
>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>> ---
>>>> v3:
>>>>  - use READ|WRITE_ONCE
>>>> v2:
>>>>   - add missing code to bypass read
>>>>  
>>>>  fs/f2fs/data.c  | 18 ++++++++++++++++++
>>>>  fs/f2fs/f2fs.h  |  1 +
>>>>  fs/f2fs/super.c |  2 ++
>>>>  3 files changed, 21 insertions(+)
>>>>
>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>> index 995cf78b23c5e..360b4c9080d97 100644
>>>> --- a/fs/f2fs/data.c
>>>> +++ b/fs/f2fs/data.c
>>>> @@ -2296,6 +2296,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
>>>>  	unsigned nr_pages = rac ? readahead_count(rac) : 1;
>>>>  	unsigned max_nr_pages = nr_pages;
>>>>  	int ret = 0;
>>>> +	bool drop_ra = false;
>>>>  
>>>>  	map.m_pblk = 0;
>>>>  	map.m_lblk = 0;
>>>> @@ -2306,10 +2307,24 @@ static int f2fs_mpage_readpages(struct inode *inode,
>>>>  	map.m_seg_type = NO_CHECK_TYPE;
>>>>  	map.m_may_create = false;
>>>>  
>>>> +	/*
>>>> +	 * Two readahead threads for same address range can cause race condition
>>>> +	 * which fragments sequential read IOs. So let's avoid each other.
>>>> +	 */
>>>> +	if (rac && readahead_count(rac)) {
>>>> +		if (READ_ONCE(F2FS_I(inode)->ra_offset) == readahead_index(rac))
>>>> +			drop_ra = true;
>>>> +		else
>>>> +			WRITE_ONCE(F2FS_I(inode)->ra_offset,
>>>> +						readahead_index(rac));
>>>> +	}
>>>> +
>>>>  	for (; nr_pages; nr_pages--) {
>>>>  		if (rac) {
>>>>  			page = readahead_page(rac);
>>>>  			prefetchw(&page->flags);
>>>> +			if (drop_ra)
>>>> +				goto next_page;
>>>
>>> When CONFIG_F2FS_FS_COMPRESSION is not set (i.e. x86_64 defconfig +
>>> CONFIG_F2FS_FS=y):
>>>
>>> $ make -skj"$(nproc)" O=out distclean defconfig fs/f2fs/data.o
>>> ../fs/f2fs/data.c: In function ‘f2fs_mpage_readpages’:
>>> ../fs/f2fs/data.c:2327:5: error: label ‘next_page’ used but not defined
>>>  2327 |     goto next_page;
>>>       |     ^~~~
>>> ...
>>
>> Thanks. I pushed the fix for -next.
>> https://lore.kernel.org/linux-f2fs-devel/1be18397-7fc6-703e-121b-e210e101357f@infradead.org/T/#t

It will hang the kernel because we missed to unlock those cached pages,
I changed to 'goto set_error_page', the issue was gone.

Thanks,

> 
> Reviewed-by: Chao Yu <yuchao0@huawei.com>
> 
> Thanks,
> 
>>
>> Thanks,
>>
>>>
>>> Cheers,
>>> Nathan
>>>
>>>
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> Linux-f2fs-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 

Re: [f2fs-dev] [PATCH v3] f2fs: avoid readahead race condition

[Chao Yu]

On 2020/7/1 9:59, Chao Yu wrote:
> On 2020/7/1 4:56, Jaegeuk Kim wrote:
>> On 06/30, Nathan Chancellor wrote:
>>> On Mon, Jun 29, 2020 at 01:27:20PM -0700, Jaegeuk Kim wrote:
>>>> If two readahead threads having same offset enter in readpages, every read
>>>> IOs are split and issued to the disk which giving lower bandwidth.
>>>>
>>>> This patch tries to avoid redundant readahead calls.
>>>>
>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>> ---
>>>> v3:
>>>>  - use READ|WRITE_ONCE
>>>> v2:
>>>>   - add missing code to bypass read
>>>>  
>>>>  fs/f2fs/data.c  | 18 ++++++++++++++++++
>>>>  fs/f2fs/f2fs.h  |  1 +
>>>>  fs/f2fs/super.c |  2 ++
>>>>  3 files changed, 21 insertions(+)
>>>>
>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>> index 995cf78b23c5e..360b4c9080d97 100644
>>>> --- a/fs/f2fs/data.c
>>>> +++ b/fs/f2fs/data.c
>>>> @@ -2296,6 +2296,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
>>>>  	unsigned nr_pages = rac ? readahead_count(rac) : 1;
>>>>  	unsigned max_nr_pages = nr_pages;
>>>>  	int ret = 0;
>>>> +	bool drop_ra = false;
>>>>  
>>>>  	map.m_pblk = 0;
>>>>  	map.m_lblk = 0;
>>>> @@ -2306,10 +2307,24 @@ static int f2fs_mpage_readpages(struct inode *inode,
>>>>  	map.m_seg_type = NO_CHECK_TYPE;
>>>>  	map.m_may_create = false;
>>>>  
>>>> +	/*
>>>> +	 * Two readahead threads for same address range can cause race condition
>>>> +	 * which fragments sequential read IOs. So let's avoid each other.
>>>> +	 */
>>>> +	if (rac && readahead_count(rac)) {
>>>> +		if (READ_ONCE(F2FS_I(inode)->ra_offset) == readahead_index(rac))
>>>> +			drop_ra = true;
>>>> +		else
>>>> +			WRITE_ONCE(F2FS_I(inode)->ra_offset,
>>>> +						readahead_index(rac));
>>>> +	}
>>>> +
>>>>  	for (; nr_pages; nr_pages--) {
>>>>  		if (rac) {
>>>>  			page = readahead_page(rac);
>>>>  			prefetchw(&page->flags);
>>>> +			if (drop_ra)
>>>> +				goto next_page;
>>>
>>> When CONFIG_F2FS_FS_COMPRESSION is not set (i.e. x86_64 defconfig +
>>> CONFIG_F2FS_FS=y):
>>>
>>> $ make -skj"$(nproc)" O=out distclean defconfig fs/f2fs/data.o
>>> ../fs/f2fs/data.c: In function ‘f2fs_mpage_readpages’:
>>> ../fs/f2fs/data.c:2327:5: error: label ‘next_page’ used but not defined
>>>  2327 |     goto next_page;
>>>       |     ^~~~
>>> ...
>>
>> Thanks. I pushed the fix for -next.
>> https://lore.kernel.org/linux-f2fs-devel/1be18397-7fc6-703e-121b-e210e101357f@infradead.org/T/#t

It will hang the kernel because we missed to unlock those cached pages,
I changed to 'goto set_error_page', the issue was gone.

Thanks,

> 
> Reviewed-by: Chao Yu <yuchao0@huawei.com>
> 
> Thanks,
> 
>>
>> Thanks,
>>
>>>
>>> Cheers,
>>> Nathan
>>>
>>>
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> Linux-f2fs-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v3] f2fs: avoid readahead race condition

[Jaegeuk Kim]

On 07/01, Chao Yu wrote:
> On 2020/7/1 9:59, Chao Yu wrote:
> > On 2020/7/1 4:56, Jaegeuk Kim wrote:
> >> On 06/30, Nathan Chancellor wrote:
> >>> On Mon, Jun 29, 2020 at 01:27:20PM -0700, Jaegeuk Kim wrote:
> >>>> If two readahead threads having same offset enter in readpages, every read
> >>>> IOs are split and issued to the disk which giving lower bandwidth.
> >>>>
> >>>> This patch tries to avoid redundant readahead calls.
> >>>>
> >>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> >>>> ---
> >>>> v3:
> >>>>  - use READ|WRITE_ONCE
> >>>> v2:
> >>>>   - add missing code to bypass read
> >>>>  
> >>>>  fs/f2fs/data.c  | 18 ++++++++++++++++++
> >>>>  fs/f2fs/f2fs.h  |  1 +
> >>>>  fs/f2fs/super.c |  2 ++
> >>>>  3 files changed, 21 insertions(+)
> >>>>
> >>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >>>> index 995cf78b23c5e..360b4c9080d97 100644
> >>>> --- a/fs/f2fs/data.c
> >>>> +++ b/fs/f2fs/data.c
> >>>> @@ -2296,6 +2296,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
> >>>>  	unsigned nr_pages = rac ? readahead_count(rac) : 1;
> >>>>  	unsigned max_nr_pages = nr_pages;
> >>>>  	int ret = 0;
> >>>> +	bool drop_ra = false;
> >>>>  
> >>>>  	map.m_pblk = 0;
> >>>>  	map.m_lblk = 0;
> >>>> @@ -2306,10 +2307,24 @@ static int f2fs_mpage_readpages(struct inode *inode,
> >>>>  	map.m_seg_type = NO_CHECK_TYPE;
> >>>>  	map.m_may_create = false;
> >>>>  
> >>>> +	/*
> >>>> +	 * Two readahead threads for same address range can cause race condition
> >>>> +	 * which fragments sequential read IOs. So let's avoid each other.
> >>>> +	 */
> >>>> +	if (rac && readahead_count(rac)) {
> >>>> +		if (READ_ONCE(F2FS_I(inode)->ra_offset) == readahead_index(rac))
> >>>> +			drop_ra = true;
> >>>> +		else
> >>>> +			WRITE_ONCE(F2FS_I(inode)->ra_offset,
> >>>> +						readahead_index(rac));
> >>>> +	}
> >>>> +
> >>>>  	for (; nr_pages; nr_pages--) {
> >>>>  		if (rac) {
> >>>>  			page = readahead_page(rac);
> >>>>  			prefetchw(&page->flags);
> >>>> +			if (drop_ra)
> >>>> +				goto next_page;
> >>>
> >>> When CONFIG_F2FS_FS_COMPRESSION is not set (i.e. x86_64 defconfig +
> >>> CONFIG_F2FS_FS=y):
> >>>
> >>> $ make -skj"$(nproc)" O=out distclean defconfig fs/f2fs/data.o
> >>> ../fs/f2fs/data.c: In function ‘f2fs_mpage_readpages’:
> >>> ../fs/f2fs/data.c:2327:5: error: label ‘next_page’ used but not defined
> >>>  2327 |     goto next_page;
> >>>       |     ^~~~
> >>> ...
> >>
> >> Thanks. I pushed the fix for -next.
> >> https://lore.kernel.org/linux-f2fs-devel/1be18397-7fc6-703e-121b-e210e101357f@infradead.org/T/#t
> 
> It will hang the kernel because we missed to unlock those cached pages,
> I changed to 'goto set_error_page', the issue was gone.

How about v4?

> 
> Thanks,
> 
> > 
> > Reviewed-by: Chao Yu <yuchao0@huawei.com>
> > 
> > Thanks,
> > 
> >>
> >> Thanks,
> >>
> >>>
> >>> Cheers,
> >>> Nathan
> >>>
> >>>
> >>> _______________________________________________
> >>> Linux-f2fs-devel mailing list
> >>> Linux-f2fs-devel@lists.sourceforge.net
> >>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >>
> >>
> >> _______________________________________________
> >> Linux-f2fs-devel mailing list
> >> Linux-f2fs-devel@lists.sourceforge.net
> >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >>
> > 
> > 
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > 

Re: [f2fs-dev] [PATCH v3] f2fs: avoid readahead race condition

[Jaegeuk Kim]

On 07/01, Chao Yu wrote:
> On 2020/7/1 9:59, Chao Yu wrote:
> > On 2020/7/1 4:56, Jaegeuk Kim wrote:
> >> On 06/30, Nathan Chancellor wrote:
> >>> On Mon, Jun 29, 2020 at 01:27:20PM -0700, Jaegeuk Kim wrote:
> >>>> If two readahead threads having same offset enter in readpages, every read
> >>>> IOs are split and issued to the disk which giving lower bandwidth.
> >>>>
> >>>> This patch tries to avoid redundant readahead calls.
> >>>>
> >>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> >>>> ---
> >>>> v3:
> >>>>  - use READ|WRITE_ONCE
> >>>> v2:
> >>>>   - add missing code to bypass read
> >>>>  
> >>>>  fs/f2fs/data.c  | 18 ++++++++++++++++++
> >>>>  fs/f2fs/f2fs.h  |  1 +
> >>>>  fs/f2fs/super.c |  2 ++
> >>>>  3 files changed, 21 insertions(+)
> >>>>
> >>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >>>> index 995cf78b23c5e..360b4c9080d97 100644
> >>>> --- a/fs/f2fs/data.c
> >>>> +++ b/fs/f2fs/data.c
> >>>> @@ -2296,6 +2296,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
> >>>>  	unsigned nr_pages = rac ? readahead_count(rac) : 1;
> >>>>  	unsigned max_nr_pages = nr_pages;
> >>>>  	int ret = 0;
> >>>> +	bool drop_ra = false;
> >>>>  
> >>>>  	map.m_pblk = 0;
> >>>>  	map.m_lblk = 0;
> >>>> @@ -2306,10 +2307,24 @@ static int f2fs_mpage_readpages(struct inode *inode,
> >>>>  	map.m_seg_type = NO_CHECK_TYPE;
> >>>>  	map.m_may_create = false;
> >>>>  
> >>>> +	/*
> >>>> +	 * Two readahead threads for same address range can cause race condition
> >>>> +	 * which fragments sequential read IOs. So let's avoid each other.
> >>>> +	 */
> >>>> +	if (rac && readahead_count(rac)) {
> >>>> +		if (READ_ONCE(F2FS_I(inode)->ra_offset) == readahead_index(rac))
> >>>> +			drop_ra = true;
> >>>> +		else
> >>>> +			WRITE_ONCE(F2FS_I(inode)->ra_offset,
> >>>> +						readahead_index(rac));
> >>>> +	}
> >>>> +
> >>>>  	for (; nr_pages; nr_pages--) {
> >>>>  		if (rac) {
> >>>>  			page = readahead_page(rac);
> >>>>  			prefetchw(&page->flags);
> >>>> +			if (drop_ra)
> >>>> +				goto next_page;
> >>>
> >>> When CONFIG_F2FS_FS_COMPRESSION is not set (i.e. x86_64 defconfig +
> >>> CONFIG_F2FS_FS=y):
> >>>
> >>> $ make -skj"$(nproc)" O=out distclean defconfig fs/f2fs/data.o
> >>> ../fs/f2fs/data.c: In function ‘f2fs_mpage_readpages’:
> >>> ../fs/f2fs/data.c:2327:5: error: label ‘next_page’ used but not defined
> >>>  2327 |     goto next_page;
> >>>       |     ^~~~
> >>> ...
> >>
> >> Thanks. I pushed the fix for -next.
> >> https://lore.kernel.org/linux-f2fs-devel/1be18397-7fc6-703e-121b-e210e101357f@infradead.org/T/#t
> 
> It will hang the kernel because we missed to unlock those cached pages,
> I changed to 'goto set_error_page', the issue was gone.

How about v4?

> 
> Thanks,
> 
> > 
> > Reviewed-by: Chao Yu <yuchao0@huawei.com>
> > 
> > Thanks,
> > 
> >>
> >> Thanks,
> >>
> >>>
> >>> Cheers,
> >>> Nathan
> >>>
> >>>
> >>> _______________________________________________
> >>> Linux-f2fs-devel mailing list
> >>> Linux-f2fs-devel@lists.sourceforge.net
> >>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >>
> >>
> >> _______________________________________________
> >> Linux-f2fs-devel mailing list
> >> Linux-f2fs-devel@lists.sourceforge.net
> >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >>
> > 
> > 
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v3] f2fs: avoid readahead race condition

[Chao Yu]

On 2020/7/2 0:14, Jaegeuk Kim wrote:
> How about v4?

It looks good to me.

Thanks,

Re: [f2fs-dev] [PATCH v3] f2fs: avoid readahead race condition

[Chao Yu]

On 2020/7/2 0:14, Jaegeuk Kim wrote:
> How about v4?

It looks good to me.

Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v4] f2fs: avoid readahead race condition

[Jaegeuk Kim]

From 3634864095bd1aafbb60ff49dac7d13ce157b658 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk@kernel.org>
Date: Mon, 22 Jun 2020 23:01:05 -0700
Subject: [PATCH] f2fs: avoid readahead race condition
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If two readahead threads having same offset enter in readpages, every read
IOs are split and issued to the disk which giving lower bandwidth.

This patch tries to avoid redundant readahead calls.

Fixes one build error reported by Randy.
Fix build error when F2FS_FS_COMPRESSION is not set/enabled.
This label is needed in either case.

../fs/f2fs/data.c: In function ‘f2fs_mpage_readpages’:
../fs/f2fs/data.c:2327:5: error: label ‘next_page’ used but not defined
     goto next_page;

Reviewed-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 v4:
  - fix missing unlock_page
 v3:
  - use READ|WRITE_ONCE
 v2:
   - add missing code to bypass read

 fs/f2fs/data.c  | 20 ++++++++++++++++++++
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/super.c |  2 ++
 3 files changed, 23 insertions(+)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 995cf78b23c5e..066d29938c03a 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2296,6 +2296,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	unsigned nr_pages = rac ? readahead_count(rac) : 1;
 	unsigned max_nr_pages = nr_pages;
 	int ret = 0;
+	bool drop_ra = false;
 
 	map.m_pblk = 0;
 	map.m_lblk = 0;
@@ -2306,10 +2307,26 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	map.m_seg_type = NO_CHECK_TYPE;
 	map.m_may_create = false;
 
+	/*
+	 * Two readahead threads for same address range can cause race condition
+	 * which fragments sequential read IOs. So let's avoid each other.
+	 */
+	if (rac && readahead_count(rac)) {
+		if (READ_ONCE(F2FS_I(inode)->ra_offset) == readahead_index(rac))
+			drop_ra = true;
+		else
+			WRITE_ONCE(F2FS_I(inode)->ra_offset,
+						readahead_index(rac));
+	}
+
 	for (; nr_pages; nr_pages--) {
 		if (rac) {
 			page = readahead_page(rac);
 			prefetchw(&page->flags);
+			if (drop_ra) {
+				f2fs_put_page(page, 1);
+				continue;
+			}
 		}
 
 #ifdef CONFIG_F2FS_FS_COMPRESSION
@@ -2372,6 +2389,9 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	}
 	if (bio)
 		__submit_bio(F2FS_I_SB(inode), bio, DATA);
+
+	if (rac && readahead_count(rac) && !drop_ra)
+		WRITE_ONCE(F2FS_I(inode)->ra_offset, -1);
 	return ret;
 }
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 6a655edeb522f..e6e47618a3576 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -809,6 +809,7 @@ struct f2fs_inode_info {
 	struct list_head inmem_pages;	/* inmemory pages managed by f2fs */
 	struct task_struct *inmem_task;	/* store inmemory task */
 	struct mutex inmem_lock;	/* lock for inmemory pages */
+	pgoff_t ra_offset;		/* ongoing readahead offset */
 	struct extent_tree *extent_tree;	/* cached extent_tree entry */
 
 	/* avoid racing between foreground op and gc */
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 7326522057378..80cb7cd358f84 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1015,6 +1015,8 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
 	/* Will be used by directory only */
 	fi->i_dir_level = F2FS_SB(sb)->dir_level;
 
+	fi->ra_offset = -1;
+
 	return &fi->vfs_inode;
 }
 
-- 
2.27.0.212.ge8ba1cc988-goog

Re: [f2fs-dev] [PATCH v4] f2fs: avoid readahead race condition

[Jaegeuk Kim]

From 3634864095bd1aafbb60ff49dac7d13ce157b658 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk@kernel.org>
Date: Mon, 22 Jun 2020 23:01:05 -0700
Subject: [PATCH] f2fs: avoid readahead race condition
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If two readahead threads having same offset enter in readpages, every read
IOs are split and issued to the disk which giving lower bandwidth.

This patch tries to avoid redundant readahead calls.

Fixes one build error reported by Randy.
Fix build error when F2FS_FS_COMPRESSION is not set/enabled.
This label is needed in either case.

../fs/f2fs/data.c: In function ‘f2fs_mpage_readpages’:
../fs/f2fs/data.c:2327:5: error: label ‘next_page’ used but not defined
     goto next_page;

Reviewed-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 v4:
  - fix missing unlock_page
 v3:
  - use READ|WRITE_ONCE
 v2:
   - add missing code to bypass read

 fs/f2fs/data.c  | 20 ++++++++++++++++++++
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/super.c |  2 ++
 3 files changed, 23 insertions(+)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 995cf78b23c5e..066d29938c03a 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2296,6 +2296,7 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	unsigned nr_pages = rac ? readahead_count(rac) : 1;
 	unsigned max_nr_pages = nr_pages;
 	int ret = 0;
+	bool drop_ra = false;
 
 	map.m_pblk = 0;
 	map.m_lblk = 0;
@@ -2306,10 +2307,26 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	map.m_seg_type = NO_CHECK_TYPE;
 	map.m_may_create = false;
 
+	/*
+	 * Two readahead threads for same address range can cause race condition
+	 * which fragments sequential read IOs. So let's avoid each other.
+	 */
+	if (rac && readahead_count(rac)) {
+		if (READ_ONCE(F2FS_I(inode)->ra_offset) == readahead_index(rac))
+			drop_ra = true;
+		else
+			WRITE_ONCE(F2FS_I(inode)->ra_offset,
+						readahead_index(rac));
+	}
+
 	for (; nr_pages; nr_pages--) {
 		if (rac) {
 			page = readahead_page(rac);
 			prefetchw(&page->flags);
+			if (drop_ra) {
+				f2fs_put_page(page, 1);
+				continue;
+			}
 		}
 
 #ifdef CONFIG_F2FS_FS_COMPRESSION
@@ -2372,6 +2389,9 @@ static int f2fs_mpage_readpages(struct inode *inode,
 	}
 	if (bio)
 		__submit_bio(F2FS_I_SB(inode), bio, DATA);
+
+	if (rac && readahead_count(rac) && !drop_ra)
+		WRITE_ONCE(F2FS_I(inode)->ra_offset, -1);
 	return ret;
 }
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 6a655edeb522f..e6e47618a3576 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -809,6 +809,7 @@ struct f2fs_inode_info {
 	struct list_head inmem_pages;	/* inmemory pages managed by f2fs */
 	struct task_struct *inmem_task;	/* store inmemory task */
 	struct mutex inmem_lock;	/* lock for inmemory pages */
+	pgoff_t ra_offset;		/* ongoing readahead offset */
 	struct extent_tree *extent_tree;	/* cached extent_tree entry */
 
 	/* avoid racing between foreground op and gc */
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 7326522057378..80cb7cd358f84 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1015,6 +1015,8 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
 	/* Will be used by directory only */
 	fi->i_dir_level = F2FS_SB(sb)->dir_level;
 
+	fi->ra_offset = -1;
+
 	return &fi->vfs_inode;
 }
 
-- 
2.27.0.212.ge8ba1cc988-goog


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel