linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] f2fs: remove unnecessary IS_SWAPFILE check
@ 2021-02-27 12:02 Huang Jianan
  2021-02-27 12:02 ` [PATCH 2/3] f2fs: fix last_lblock check in check_swap_activate_fast Huang Jianan
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Huang Jianan @ 2021-02-27 12:02 UTC (permalink / raw)
  To: linux-f2fs-devel; +Cc: huangjianan, guoweichao, zhangshiming, linux-kernel

Now swapfile in f2fs directly submit IO to blockdev according to
swapfile extents reported by f2fs when swapon, therefore there is
no need to check IS_SWAPFILE when exec filesystem operation.

Signed-off-by: Huang Jianan <huangjianan@oppo.com>
Signed-off-by: Guo Weichao <guoweichao@oppo.com>
---
 fs/f2fs/data.c | 2 +-
 fs/f2fs/f2fs.h | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index b9721c8f116c..a1498a1a345c 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1723,7 +1723,7 @@ static int get_data_block_dio_write(struct inode *inode, sector_t iblock,
 	return __get_data_block(inode, iblock, bh_result, create,
 				F2FS_GET_BLOCK_DIO, NULL,
 				f2fs_rw_hint_to_seg_type(inode->i_write_hint),
-				IS_SWAPFILE(inode) ? false : true);
+				true);
 }
 
 static int get_data_block_dio(struct inode *inode, sector_t iblock,
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index cccdfb1a40ab..3f65cfe11a0f 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -4176,8 +4176,7 @@ static inline bool f2fs_force_buffered_io(struct inode *inode,
 		if (F2FS_IO_ALIGNED(sbi))
 			return true;
 	}
-	if (is_sbi_flag_set(F2FS_I_SB(inode), SBI_CP_DISABLED) &&
-					!IS_SWAPFILE(inode))
+	if (is_sbi_flag_set(F2FS_I_SB(inode), SBI_CP_DISABLED))
 		return true;
 
 	return false;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/3] f2fs: fix last_lblock check in check_swap_activate_fast
  2021-02-27 12:02 [PATCH 1/3] f2fs: remove unnecessary IS_SWAPFILE check Huang Jianan
@ 2021-02-27 12:02 ` Huang Jianan
  2021-03-01  2:01   ` [f2fs-dev] " Chao Yu
  2021-02-27 12:02 ` [PATCH 3/3] f2fs: check if swapfile is section-alligned Huang Jianan
  2021-03-01  1:58 ` [f2fs-dev] [PATCH 1/3] f2fs: remove unnecessary IS_SWAPFILE check Chao Yu
  2 siblings, 1 reply; 8+ messages in thread
From: Huang Jianan @ 2021-02-27 12:02 UTC (permalink / raw)
  To: linux-f2fs-devel; +Cc: huangjianan, guoweichao, zhangshiming, linux-kernel

Because page_no < sis->max guarantees that the while loop break out 
normally, the wrong check contidion here doesn't cause a problem.

Signed-off-by: Huang Jianan <huangjianan@oppo.com>
Signed-off-by: Guo Weichao <guoweichao@oppo.com>
---
 fs/f2fs/data.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index a1498a1a345c..4dbc1cafc55d 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -3804,7 +3804,7 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
 	last_lblock = bytes_to_blks(inode, i_size_read(inode));
 	len = i_size_read(inode);
 
-	while (cur_lblock <= last_lblock && cur_lblock < sis->max) {
+	while (cur_lblock + 1 <= last_lblock && cur_lblock < sis->max) {
 		struct f2fs_map_blocks map;
 		pgoff_t next_pgofs;
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/3] f2fs: check if swapfile is section-alligned
  2021-02-27 12:02 [PATCH 1/3] f2fs: remove unnecessary IS_SWAPFILE check Huang Jianan
  2021-02-27 12:02 ` [PATCH 2/3] f2fs: fix last_lblock check in check_swap_activate_fast Huang Jianan
@ 2021-02-27 12:02 ` Huang Jianan
  2021-03-01  2:41   ` [f2fs-dev] " Chao Yu
  2021-03-01  1:58 ` [f2fs-dev] [PATCH 1/3] f2fs: remove unnecessary IS_SWAPFILE check Chao Yu
  2 siblings, 1 reply; 8+ messages in thread
From: Huang Jianan @ 2021-02-27 12:02 UTC (permalink / raw)
  To: linux-f2fs-devel; +Cc: huangjianan, guoweichao, zhangshiming, linux-kernel

If the swapfile isn't created by pin and fallocate, it cann't be
guaranteed section-aligned, so it may be selected by f2fs gc. When
gc_pin_file_threshold is reached, the address of swapfile may change,
but won't be synchroniz to swap_extent, so swap will write to wrong
address, which will cause data corruption.

Signed-off-by: Huang Jianan <huangjianan@oppo.com>
Signed-off-by: Guo Weichao <guoweichao@oppo.com>
---
 fs/f2fs/data.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 4dbc1cafc55d..3e523d6e4643 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -3781,11 +3781,63 @@ int f2fs_migrate_page(struct address_space *mapping,
 #endif
 
 #ifdef CONFIG_SWAP
+static int f2fs_check_file_aligned(struct inode *inode)
+{
+	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
+	block_t main_blkaddr = SM_I(sbi)->main_blkaddr;
+	block_t cur_lblock;
+	block_t last_lblock;
+	block_t pblock;
+	unsigned long len;
+	unsigned long nr_pblocks;
+	unsigned int blocks_per_sec = sbi->blocks_per_seg * sbi->segs_per_sec;
+	int ret;
+
+	cur_lblock = 0;
+	last_lblock = bytes_to_blks(inode, i_size_read(inode));
+	len = i_size_read(inode);
+
+	while (cur_lblock < last_lblock) {
+		struct f2fs_map_blocks map;
+		pgoff_t next_pgofs;
+
+		memset(&map, 0, sizeof(map));
+		map.m_lblk = cur_lblock;
+		map.m_len = bytes_to_blks(inode, len) - cur_lblock;
+		map.m_next_pgofs = &next_pgofs;
+		map.m_seg_type = NO_CHECK_TYPE;
+
+		ret = f2fs_map_blocks(inode, &map, 0, F2FS_GET_BLOCK_FIEMAP);
+
+		if (ret)
+			goto err_out;
+
+		/* hole */
+		if (!(map.m_flags & F2FS_MAP_FLAGS))
+			goto err_out;
+
+		pblock = map.m_pblk;
+		nr_pblocks = map.m_len;
+
+		if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
+			nr_pblocks & (blocks_per_sec - 1))
+			goto err_out;
+
+		cur_lblock += nr_pblocks;
+	}
+
+	return 0;
+err_out:
+	pr_err("swapon: swapfile isn't section-aligned\n");
+	return -EINVAL;
+}
+
 static int check_swap_activate_fast(struct swap_info_struct *sis,
 				struct file *swap_file, sector_t *span)
 {
 	struct address_space *mapping = swap_file->f_mapping;
 	struct inode *inode = mapping->host;
+	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	sector_t cur_lblock;
 	sector_t last_lblock;
 	sector_t pblock;
@@ -3793,6 +3845,7 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
 	sector_t highest_pblock = 0;
 	int nr_extents = 0;
 	unsigned long nr_pblocks;
+	unsigned int blocks_per_sec = sbi->blocks_per_seg * sbi->segs_per_sec;
 	u64 len;
 	int ret;
 
@@ -3827,6 +3880,13 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
 		pblock = map.m_pblk;
 		nr_pblocks = map.m_len;
 
+		if ((pblock - SM_I(sbi)->main_blkaddr) & (blocks_per_sec - 1) ||
+			nr_pblocks & (blocks_per_sec - 1)) {
+			pr_err("swapon: swapfile isn't section-aligned\n");
+			ret = -EINVAL;
+			goto out;
+		}
+
 		if (cur_lblock + nr_pblocks >= sis->max)
 			nr_pblocks = sis->max - cur_lblock;
 
@@ -3878,6 +3938,9 @@ static int check_swap_activate(struct swap_info_struct *sis,
 	if (PAGE_SIZE == F2FS_BLKSIZE)
 		return check_swap_activate_fast(sis, swap_file, span);
 
+	if (f2fs_check_file_aligned(inode))
+		return -EINVAL;
+
 	blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
 
 	/*
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [f2fs-dev] [PATCH 1/3] f2fs: remove unnecessary IS_SWAPFILE check
  2021-02-27 12:02 [PATCH 1/3] f2fs: remove unnecessary IS_SWAPFILE check Huang Jianan
  2021-02-27 12:02 ` [PATCH 2/3] f2fs: fix last_lblock check in check_swap_activate_fast Huang Jianan
  2021-02-27 12:02 ` [PATCH 3/3] f2fs: check if swapfile is section-alligned Huang Jianan
@ 2021-03-01  1:58 ` Chao Yu
  2 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2021-03-01  1:58 UTC (permalink / raw)
  To: Huang Jianan, linux-f2fs-devel; +Cc: linux-kernel, zhangshiming

On 2021/2/27 20:02, Huang Jianan via Linux-f2fs-devel wrote:
> Now swapfile in f2fs directly submit IO to blockdev according to
> swapfile extents reported by f2fs when swapon, therefore there is
> no need to check IS_SWAPFILE when exec filesystem operation.
> 
> Signed-off-by: Huang Jianan <huangjianan@oppo.com>
> Signed-off-by: Guo Weichao <guoweichao@oppo.com>

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

Thanks,

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [f2fs-dev] [PATCH 2/3] f2fs: fix last_lblock check in check_swap_activate_fast
  2021-02-27 12:02 ` [PATCH 2/3] f2fs: fix last_lblock check in check_swap_activate_fast Huang Jianan
@ 2021-03-01  2:01   ` Chao Yu
  0 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2021-03-01  2:01 UTC (permalink / raw)
  To: Huang Jianan, linux-f2fs-devel; +Cc: linux-kernel, zhangshiming

On 2021/2/27 20:02, Huang Jianan via Linux-f2fs-devel wrote:
> Because page_no < sis->max guarantees that the while loop break out
> normally, the wrong check contidion here doesn't cause a problem.
> 
> Signed-off-by: Huang Jianan <huangjianan@oppo.com>
> Signed-off-by: Guo Weichao <guoweichao@oppo.com>
> ---
>   fs/f2fs/data.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index a1498a1a345c..4dbc1cafc55d 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -3804,7 +3804,7 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>   	last_lblock = bytes_to_blks(inode, i_size_read(inode));
>   	len = i_size_read(inode);
>   
> -	while (cur_lblock <= last_lblock && cur_lblock < sis->max) {
> +	while (cur_lblock + 1 <= last_lblock && cur_lblock < sis->max) {

while (cur_lblock < last_lblock && cur_lblock < sis->max) {

It's nitpick, if necessary, I guess Jaegeuk could help to change this
while merging.

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

Thanks,

>   		struct f2fs_map_blocks map;
>   		pgoff_t next_pgofs;
>   
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [f2fs-dev] [PATCH 3/3] f2fs: check if swapfile is section-alligned
  2021-02-27 12:02 ` [PATCH 3/3] f2fs: check if swapfile is section-alligned Huang Jianan
@ 2021-03-01  2:41   ` Chao Yu
  2021-03-02  4:20     ` Jaegeuk Kim
  0 siblings, 1 reply; 8+ messages in thread
From: Chao Yu @ 2021-03-01  2:41 UTC (permalink / raw)
  To: Huang Jianan, linux-f2fs-devel; +Cc: linux-kernel, zhangshiming

Hi Jianan,

On 2021/2/27 20:02, Huang Jianan via Linux-f2fs-devel wrote:
> If the swapfile isn't created by pin and fallocate, it cann't be

Typo:

can't

> guaranteed section-aligned, so it may be selected by f2fs gc. When
> gc_pin_file_threshold is reached, the address of swapfile may change,
> but won't be synchroniz to swap_extent, so swap will write to wrong

synchronized

> address, which will cause data corruption.
> 
> Signed-off-by: Huang Jianan <huangjianan@oppo.com>
> Signed-off-by: Guo Weichao <guoweichao@oppo.com>
> ---
>   fs/f2fs/data.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 63 insertions(+)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 4dbc1cafc55d..3e523d6e4643 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -3781,11 +3781,63 @@ int f2fs_migrate_page(struct address_space *mapping,
>   #endif
>   
>   #ifdef CONFIG_SWAP
> +static int f2fs_check_file_aligned(struct inode *inode)

f2fs_check_file_alignment() or f2fs_is_file_aligned()?

> +{
> +	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> +	block_t main_blkaddr = SM_I(sbi)->main_blkaddr;
> +	block_t cur_lblock;
> +	block_t last_lblock;
> +	block_t pblock;
> +	unsigned long len;
> +	unsigned long nr_pblocks;
> +	unsigned int blocks_per_sec = sbi->blocks_per_seg * sbi->segs_per_sec;

unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);

> +	int ret;
> +
> +	cur_lblock = 0;
> +	last_lblock = bytes_to_blks(inode, i_size_read(inode));
> +	len = i_size_read(inode);
> +
> +	while (cur_lblock < last_lblock) {
> +		struct f2fs_map_blocks map;
> +		pgoff_t next_pgofs;
> +
> +		memset(&map, 0, sizeof(map));
> +		map.m_lblk = cur_lblock;
> +		map.m_len = bytes_to_blks(inode, len) - cur_lblock;

map.m_len = last_lblock - cur_lblock;

> +		map.m_next_pgofs = &next_pgofs;

map.m_next_pgofs = NULL;
map.m_next_extent = NULL;

> +		map.m_seg_type = NO_CHECK_TYPE;

map.m_may_create = false;

> +
> +		ret = f2fs_map_blocks(inode, &map, 0, F2FS_GET_BLOCK_FIEMAP);
> +

Unneeded blank line.

> +		if (ret)
> +			goto err_out;
> +
> +		/* hole */
> +		if (!(map.m_flags & F2FS_MAP_FLAGS))

ret = -ENOENT;

> +			goto err_out;
> +
> +		pblock = map.m_pblk;
> +		nr_pblocks = map.m_len;
> +
> +		if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
> +			nr_pblocks & (blocks_per_sec - 1))

ret = -EINVAL;

> +			goto err_out;
> +
> +		cur_lblock += nr_pblocks;
> +	}
> +
> +	return 0;
> +err_out:
> +	pr_err("swapon: swapfile isn't section-aligned\n");

We should show above message only after we fail in check condition:

	if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
		nr_pblocks & (blocks_per_sec - 1)) {
		f2fs_err(sbi, "Swapfile does not align to section");
		goto err_out;
	}

And please use f2fs_{err,warn,info..} macro rather than pr_{err,warn,info..}.

Could you please fix above related issues in check_swap_activate_fast() as well.

> +	return -EINVAL;

return ret;

> +}
> +
>   static int check_swap_activate_fast(struct swap_info_struct *sis,
>   				struct file *swap_file, sector_t *span)
>   {
>   	struct address_space *mapping = swap_file->f_mapping;
>   	struct inode *inode = mapping->host;
> +	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>   	sector_t cur_lblock;
>   	sector_t last_lblock;
>   	sector_t pblock;
> @@ -3793,6 +3845,7 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>   	sector_t highest_pblock = 0;
>   	int nr_extents = 0;
>   	unsigned long nr_pblocks;
> +	unsigned int blocks_per_sec = sbi->blocks_per_seg * sbi->segs_per_sec;

Ditto,

>   	u64 len;
>   	int ret;
>   
> @@ -3827,6 +3880,13 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>   		pblock = map.m_pblk;
>   		nr_pblocks = map.m_len;
>   
> +		if ((pblock - SM_I(sbi)->main_blkaddr) & (blocks_per_sec - 1) ||
> +			nr_pblocks & (blocks_per_sec - 1)) {
> +			pr_err("swapon: swapfile isn't section-aligned\n");

Ditto,

> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
>   		if (cur_lblock + nr_pblocks >= sis->max)
>   			nr_pblocks = sis->max - cur_lblock;
>   
> @@ -3878,6 +3938,9 @@ static int check_swap_activate(struct swap_info_struct *sis,
>   	if (PAGE_SIZE == F2FS_BLKSIZE)
>   		return check_swap_activate_fast(sis, swap_file, span);
>   
> +	if (f2fs_check_file_aligned(inode))

ret = f2fs_check_file_aligned();
if (ret)
	return ret;

Thanks,

> +		return -EINVAL;
> +
>   	blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
>   
>   	/*
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [f2fs-dev] [PATCH 3/3] f2fs: check if swapfile is section-alligned
  2021-03-01  2:41   ` [f2fs-dev] " Chao Yu
@ 2021-03-02  4:20     ` Jaegeuk Kim
  2021-03-02  4:25       ` Jaegeuk Kim
  0 siblings, 1 reply; 8+ messages in thread
From: Jaegeuk Kim @ 2021-03-02  4:20 UTC (permalink / raw)
  To: Chao Yu; +Cc: Huang Jianan, linux-f2fs-devel, zhangshiming, linux-kernel

On 03/01, Chao Yu wrote:
> Hi Jianan,

Merged 1/3 and 2/3, so please post v2 on 3/3.

Thanks,

> 
> On 2021/2/27 20:02, Huang Jianan via Linux-f2fs-devel wrote:
> > If the swapfile isn't created by pin and fallocate, it cann't be
> 
> Typo:
> 
> can't
> 
> > guaranteed section-aligned, so it may be selected by f2fs gc. When
> > gc_pin_file_threshold is reached, the address of swapfile may change,
> > but won't be synchroniz to swap_extent, so swap will write to wrong
> 
> synchronized
> 
> > address, which will cause data corruption.
> > 
> > Signed-off-by: Huang Jianan <huangjianan@oppo.com>
> > Signed-off-by: Guo Weichao <guoweichao@oppo.com>
> > ---
> >   fs/f2fs/data.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 63 insertions(+)
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 4dbc1cafc55d..3e523d6e4643 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -3781,11 +3781,63 @@ int f2fs_migrate_page(struct address_space *mapping,
> >   #endif
> >   #ifdef CONFIG_SWAP
> > +static int f2fs_check_file_aligned(struct inode *inode)
> 
> f2fs_check_file_alignment() or f2fs_is_file_aligned()?
> 
> > +{
> > +	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > +	block_t main_blkaddr = SM_I(sbi)->main_blkaddr;
> > +	block_t cur_lblock;
> > +	block_t last_lblock;
> > +	block_t pblock;
> > +	unsigned long len;
> > +	unsigned long nr_pblocks;
> > +	unsigned int blocks_per_sec = sbi->blocks_per_seg * sbi->segs_per_sec;
> 
> unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
> 
> > +	int ret;
> > +
> > +	cur_lblock = 0;
> > +	last_lblock = bytes_to_blks(inode, i_size_read(inode));
> > +	len = i_size_read(inode);
> > +
> > +	while (cur_lblock < last_lblock) {
> > +		struct f2fs_map_blocks map;
> > +		pgoff_t next_pgofs;
> > +
> > +		memset(&map, 0, sizeof(map));
> > +		map.m_lblk = cur_lblock;
> > +		map.m_len = bytes_to_blks(inode, len) - cur_lblock;
> 
> map.m_len = last_lblock - cur_lblock;
> 
> > +		map.m_next_pgofs = &next_pgofs;
> 
> map.m_next_pgofs = NULL;
> map.m_next_extent = NULL;
> 
> > +		map.m_seg_type = NO_CHECK_TYPE;
> 
> map.m_may_create = false;
> 
> > +
> > +		ret = f2fs_map_blocks(inode, &map, 0, F2FS_GET_BLOCK_FIEMAP);
> > +
> 
> Unneeded blank line.
> 
> > +		if (ret)
> > +			goto err_out;
> > +
> > +		/* hole */
> > +		if (!(map.m_flags & F2FS_MAP_FLAGS))
> 
> ret = -ENOENT;
> 
> > +			goto err_out;
> > +
> > +		pblock = map.m_pblk;
> > +		nr_pblocks = map.m_len;
> > +
> > +		if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
> > +			nr_pblocks & (blocks_per_sec - 1))
> 
> ret = -EINVAL;
> 
> > +			goto err_out;
> > +
> > +		cur_lblock += nr_pblocks;
> > +	}
> > +
> > +	return 0;
> > +err_out:
> > +	pr_err("swapon: swapfile isn't section-aligned\n");
> 
> We should show above message only after we fail in check condition:
> 
> 	if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
> 		nr_pblocks & (blocks_per_sec - 1)) {
> 		f2fs_err(sbi, "Swapfile does not align to section");
> 		goto err_out;
> 	}
> 
> And please use f2fs_{err,warn,info..} macro rather than pr_{err,warn,info..}.
> 
> Could you please fix above related issues in check_swap_activate_fast() as well.
> 
> > +	return -EINVAL;
> 
> return ret;
> 
> > +}
> > +
> >   static int check_swap_activate_fast(struct swap_info_struct *sis,
> >   				struct file *swap_file, sector_t *span)
> >   {
> >   	struct address_space *mapping = swap_file->f_mapping;
> >   	struct inode *inode = mapping->host;
> > +	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> >   	sector_t cur_lblock;
> >   	sector_t last_lblock;
> >   	sector_t pblock;
> > @@ -3793,6 +3845,7 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
> >   	sector_t highest_pblock = 0;
> >   	int nr_extents = 0;
> >   	unsigned long nr_pblocks;
> > +	unsigned int blocks_per_sec = sbi->blocks_per_seg * sbi->segs_per_sec;
> 
> Ditto,
> 
> >   	u64 len;
> >   	int ret;
> > @@ -3827,6 +3880,13 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
> >   		pblock = map.m_pblk;
> >   		nr_pblocks = map.m_len;
> > +		if ((pblock - SM_I(sbi)->main_blkaddr) & (blocks_per_sec - 1) ||
> > +			nr_pblocks & (blocks_per_sec - 1)) {
> > +			pr_err("swapon: swapfile isn't section-aligned\n");
> 
> Ditto,
> 
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +
> >   		if (cur_lblock + nr_pblocks >= sis->max)
> >   			nr_pblocks = sis->max - cur_lblock;
> > @@ -3878,6 +3938,9 @@ static int check_swap_activate(struct swap_info_struct *sis,
> >   	if (PAGE_SIZE == F2FS_BLKSIZE)
> >   		return check_swap_activate_fast(sis, swap_file, span);
> > +	if (f2fs_check_file_aligned(inode))
> 
> ret = f2fs_check_file_aligned();
> if (ret)
> 	return ret;
> 
> Thanks,
> 
> > +		return -EINVAL;
> > +
> >   	blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
> >   	/*
> > 
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [f2fs-dev] [PATCH 3/3] f2fs: check if swapfile is section-alligned
  2021-03-02  4:20     ` Jaegeuk Kim
@ 2021-03-02  4:25       ` Jaegeuk Kim
  0 siblings, 0 replies; 8+ messages in thread
From: Jaegeuk Kim @ 2021-03-02  4:25 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, zhangshiming

On 03/01, Jaegeuk Kim wrote:
> On 03/01, Chao Yu wrote:
> > Hi Jianan,
> 
> Merged 1/3 and 2/3, so please post v2 on 3/3.

NVM. Found v2.

> 
> Thanks,
> 
> > 
> > On 2021/2/27 20:02, Huang Jianan via Linux-f2fs-devel wrote:
> > > If the swapfile isn't created by pin and fallocate, it cann't be
> > 
> > Typo:
> > 
> > can't
> > 
> > > guaranteed section-aligned, so it may be selected by f2fs gc. When
> > > gc_pin_file_threshold is reached, the address of swapfile may change,
> > > but won't be synchroniz to swap_extent, so swap will write to wrong
> > 
> > synchronized
> > 
> > > address, which will cause data corruption.
> > > 
> > > Signed-off-by: Huang Jianan <huangjianan@oppo.com>
> > > Signed-off-by: Guo Weichao <guoweichao@oppo.com>
> > > ---
> > >   fs/f2fs/data.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 63 insertions(+)
> > > 
> > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > index 4dbc1cafc55d..3e523d6e4643 100644
> > > --- a/fs/f2fs/data.c
> > > +++ b/fs/f2fs/data.c
> > > @@ -3781,11 +3781,63 @@ int f2fs_migrate_page(struct address_space *mapping,
> > >   #endif
> > >   #ifdef CONFIG_SWAP
> > > +static int f2fs_check_file_aligned(struct inode *inode)
> > 
> > f2fs_check_file_alignment() or f2fs_is_file_aligned()?
> > 
> > > +{
> > > +	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > > +	block_t main_blkaddr = SM_I(sbi)->main_blkaddr;
> > > +	block_t cur_lblock;
> > > +	block_t last_lblock;
> > > +	block_t pblock;
> > > +	unsigned long len;
> > > +	unsigned long nr_pblocks;
> > > +	unsigned int blocks_per_sec = sbi->blocks_per_seg * sbi->segs_per_sec;
> > 
> > unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
> > 
> > > +	int ret;
> > > +
> > > +	cur_lblock = 0;
> > > +	last_lblock = bytes_to_blks(inode, i_size_read(inode));
> > > +	len = i_size_read(inode);
> > > +
> > > +	while (cur_lblock < last_lblock) {
> > > +		struct f2fs_map_blocks map;
> > > +		pgoff_t next_pgofs;
> > > +
> > > +		memset(&map, 0, sizeof(map));
> > > +		map.m_lblk = cur_lblock;
> > > +		map.m_len = bytes_to_blks(inode, len) - cur_lblock;
> > 
> > map.m_len = last_lblock - cur_lblock;
> > 
> > > +		map.m_next_pgofs = &next_pgofs;
> > 
> > map.m_next_pgofs = NULL;
> > map.m_next_extent = NULL;
> > 
> > > +		map.m_seg_type = NO_CHECK_TYPE;
> > 
> > map.m_may_create = false;
> > 
> > > +
> > > +		ret = f2fs_map_blocks(inode, &map, 0, F2FS_GET_BLOCK_FIEMAP);
> > > +
> > 
> > Unneeded blank line.
> > 
> > > +		if (ret)
> > > +			goto err_out;
> > > +
> > > +		/* hole */
> > > +		if (!(map.m_flags & F2FS_MAP_FLAGS))
> > 
> > ret = -ENOENT;
> > 
> > > +			goto err_out;
> > > +
> > > +		pblock = map.m_pblk;
> > > +		nr_pblocks = map.m_len;
> > > +
> > > +		if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
> > > +			nr_pblocks & (blocks_per_sec - 1))
> > 
> > ret = -EINVAL;
> > 
> > > +			goto err_out;
> > > +
> > > +		cur_lblock += nr_pblocks;
> > > +	}
> > > +
> > > +	return 0;
> > > +err_out:
> > > +	pr_err("swapon: swapfile isn't section-aligned\n");
> > 
> > We should show above message only after we fail in check condition:
> > 
> > 	if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
> > 		nr_pblocks & (blocks_per_sec - 1)) {
> > 		f2fs_err(sbi, "Swapfile does not align to section");
> > 		goto err_out;
> > 	}
> > 
> > And please use f2fs_{err,warn,info..} macro rather than pr_{err,warn,info..}.
> > 
> > Could you please fix above related issues in check_swap_activate_fast() as well.
> > 
> > > +	return -EINVAL;
> > 
> > return ret;
> > 
> > > +}
> > > +
> > >   static int check_swap_activate_fast(struct swap_info_struct *sis,
> > >   				struct file *swap_file, sector_t *span)
> > >   {
> > >   	struct address_space *mapping = swap_file->f_mapping;
> > >   	struct inode *inode = mapping->host;
> > > +	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > >   	sector_t cur_lblock;
> > >   	sector_t last_lblock;
> > >   	sector_t pblock;
> > > @@ -3793,6 +3845,7 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
> > >   	sector_t highest_pblock = 0;
> > >   	int nr_extents = 0;
> > >   	unsigned long nr_pblocks;
> > > +	unsigned int blocks_per_sec = sbi->blocks_per_seg * sbi->segs_per_sec;
> > 
> > Ditto,
> > 
> > >   	u64 len;
> > >   	int ret;
> > > @@ -3827,6 +3880,13 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
> > >   		pblock = map.m_pblk;
> > >   		nr_pblocks = map.m_len;
> > > +		if ((pblock - SM_I(sbi)->main_blkaddr) & (blocks_per_sec - 1) ||
> > > +			nr_pblocks & (blocks_per_sec - 1)) {
> > > +			pr_err("swapon: swapfile isn't section-aligned\n");
> > 
> > Ditto,
> > 
> > > +			ret = -EINVAL;
> > > +			goto out;
> > > +		}
> > > +
> > >   		if (cur_lblock + nr_pblocks >= sis->max)
> > >   			nr_pblocks = sis->max - cur_lblock;
> > > @@ -3878,6 +3938,9 @@ static int check_swap_activate(struct swap_info_struct *sis,
> > >   	if (PAGE_SIZE == F2FS_BLKSIZE)
> > >   		return check_swap_activate_fast(sis, swap_file, span);
> > > +	if (f2fs_check_file_aligned(inode))
> > 
> > ret = f2fs_check_file_aligned();
> > if (ret)
> > 	return ret;
> > 
> > Thanks,
> > 
> > > +		return -EINVAL;
> > > +
> > >   	blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
> > >   	/*
> > > 
> > 
> > 
> > _______________________________________________
> > 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-03-02  8:12 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-27 12:02 [PATCH 1/3] f2fs: remove unnecessary IS_SWAPFILE check Huang Jianan
2021-02-27 12:02 ` [PATCH 2/3] f2fs: fix last_lblock check in check_swap_activate_fast Huang Jianan
2021-03-01  2:01   ` [f2fs-dev] " Chao Yu
2021-02-27 12:02 ` [PATCH 3/3] f2fs: check if swapfile is section-alligned Huang Jianan
2021-03-01  2:41   ` [f2fs-dev] " Chao Yu
2021-03-02  4:20     ` Jaegeuk Kim
2021-03-02  4:25       ` Jaegeuk Kim
2021-03-01  1:58 ` [f2fs-dev] [PATCH 1/3] f2fs: remove unnecessary IS_SWAPFILE check Chao Yu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).