All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] f2fs: swap: remove dead codes
@ 2021-05-26  6:29 ` Chao Yu
  0 siblings, 0 replies; 22+ messages in thread
From: Chao Yu @ 2021-05-26  6:29 UTC (permalink / raw)
  To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, chao, Chao Yu

After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
we will never run into original logic of check_swap_activate() before
f2fs supports non 4k-sized page, so let's delete those dead codes.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/data.c | 171 +------------------------------------------------
 1 file changed, 3 insertions(+), 168 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 3058c7e28b11..9c23fde93b76 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
 #endif
 
 #ifdef CONFIG_SWAP
-static int f2fs_is_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 nr_pblocks;
-	unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
-	unsigned int not_aligned = 0;
-	int ret = 0;
-
-	cur_lblock = 0;
-	last_lblock = bytes_to_blks(inode, i_size_read(inode));
-
-	while (cur_lblock < last_lblock) {
-		struct f2fs_map_blocks map;
-
-		memset(&map, 0, sizeof(map));
-		map.m_lblk = cur_lblock;
-		map.m_len = last_lblock - cur_lblock;
-		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);
-		if (ret)
-			goto out;
-
-		/* hole */
-		if (!(map.m_flags & F2FS_MAP_FLAGS)) {
-			f2fs_err(sbi, "Swapfile has holes\n");
-			ret = -ENOENT;
-			goto out;
-		}
-
-		pblock = map.m_pblk;
-		nr_pblocks = map.m_len;
-
-		if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
-			nr_pblocks & (blocks_per_sec - 1)) {
-			if (f2fs_is_pinned_file(inode)) {
-				f2fs_err(sbi, "Swapfile does not align to section");
-				ret = -EINVAL;
-				goto out;
-			}
-			not_aligned++;
-		}
-
-		cur_lblock += nr_pblocks;
-	}
-	if (not_aligned)
-		f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
-			"\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
-			not_aligned);
-out:
-	return ret;
-}
-
-static int check_swap_activate_fast(struct swap_info_struct *sis,
+static int check_swap_activate(struct swap_info_struct *sis,
 				struct file *swap_file, sector_t *span)
 {
 	struct address_space *mapping = swap_file->f_mapping;
@@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
 	unsigned int not_aligned = 0;
 	int ret = 0;
 
+	f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
+
 	/*
 	 * Map all the blocks into the extent list.  This code doesn't try
 	 * to be very smart.
@@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
 	return ret;
 }
 
-/* Copied from generic_swapfile_activate() to check any holes */
-static int check_swap_activate(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);
-	unsigned blocks_per_page;
-	unsigned long page_no;
-	sector_t probe_block;
-	sector_t last_block;
-	sector_t lowest_block = -1;
-	sector_t highest_block = 0;
-	int nr_extents = 0;
-	int ret = 0;
-
-	if (PAGE_SIZE == F2FS_BLKSIZE)
-		return check_swap_activate_fast(sis, swap_file, span);
-
-	ret = f2fs_is_file_aligned(inode);
-	if (ret)
-		goto out;
-
-	blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
-
-	/*
-	 * Map all the blocks into the extent list.  This code doesn't try
-	 * to be very smart.
-	 */
-	probe_block = 0;
-	page_no = 0;
-	last_block = bytes_to_blks(inode, i_size_read(inode));
-	while ((probe_block + blocks_per_page) <= last_block &&
-			page_no < sis->max) {
-		unsigned block_in_page;
-		sector_t first_block;
-		sector_t block = 0;
-
-		cond_resched();
-
-		block = probe_block;
-		ret = bmap(inode, &block);
-		if (ret)
-			goto out;
-		if (!block)
-			goto bad_bmap;
-		first_block = block;
-
-		/*
-		 * It must be PAGE_SIZE aligned on-disk
-		 */
-		if (first_block & (blocks_per_page - 1)) {
-			probe_block++;
-			goto reprobe;
-		}
-
-		for (block_in_page = 1; block_in_page < blocks_per_page;
-					block_in_page++) {
-
-			block = probe_block + block_in_page;
-			ret = bmap(inode, &block);
-			if (ret)
-				goto out;
-			if (!block)
-				goto bad_bmap;
-
-			if (block != first_block + block_in_page) {
-				/* Discontiguity */
-				probe_block++;
-				goto reprobe;
-			}
-		}
-
-		first_block >>= (PAGE_SHIFT - inode->i_blkbits);
-		if (page_no) {	/* exclude the header page */
-			if (first_block < lowest_block)
-				lowest_block = first_block;
-			if (first_block > highest_block)
-				highest_block = first_block;
-		}
-
-		/*
-		 * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
-		 */
-		ret = add_swap_extent(sis, page_no, 1, first_block);
-		if (ret < 0)
-			goto out;
-		nr_extents += ret;
-		page_no++;
-		probe_block += blocks_per_page;
-reprobe:
-		continue;
-	}
-	ret = nr_extents;
-	*span = 1 + highest_block - lowest_block;
-	if (page_no == 0)
-		page_no = 1;	/* force Empty message */
-	sis->max = page_no;
-	sis->pages = page_no - 1;
-	sis->highest_bit = page_no - 1;
-out:
-	return ret;
-bad_bmap:
-	f2fs_err(sbi, "Swapfile has holes\n");
-	return -EINVAL;
-}
-
 static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
 				sector_t *span)
 {
-- 
2.29.2


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

* [f2fs-dev] [PATCH 1/2] f2fs: swap: remove dead codes
@ 2021-05-26  6:29 ` Chao Yu
  0 siblings, 0 replies; 22+ messages in thread
From: Chao Yu @ 2021-05-26  6:29 UTC (permalink / raw)
  To: jaegeuk; +Cc: linux-kernel, linux-f2fs-devel

After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
we will never run into original logic of check_swap_activate() before
f2fs supports non 4k-sized page, so let's delete those dead codes.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/data.c | 171 +------------------------------------------------
 1 file changed, 3 insertions(+), 168 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 3058c7e28b11..9c23fde93b76 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
 #endif
 
 #ifdef CONFIG_SWAP
-static int f2fs_is_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 nr_pblocks;
-	unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
-	unsigned int not_aligned = 0;
-	int ret = 0;
-
-	cur_lblock = 0;
-	last_lblock = bytes_to_blks(inode, i_size_read(inode));
-
-	while (cur_lblock < last_lblock) {
-		struct f2fs_map_blocks map;
-
-		memset(&map, 0, sizeof(map));
-		map.m_lblk = cur_lblock;
-		map.m_len = last_lblock - cur_lblock;
-		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);
-		if (ret)
-			goto out;
-
-		/* hole */
-		if (!(map.m_flags & F2FS_MAP_FLAGS)) {
-			f2fs_err(sbi, "Swapfile has holes\n");
-			ret = -ENOENT;
-			goto out;
-		}
-
-		pblock = map.m_pblk;
-		nr_pblocks = map.m_len;
-
-		if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
-			nr_pblocks & (blocks_per_sec - 1)) {
-			if (f2fs_is_pinned_file(inode)) {
-				f2fs_err(sbi, "Swapfile does not align to section");
-				ret = -EINVAL;
-				goto out;
-			}
-			not_aligned++;
-		}
-
-		cur_lblock += nr_pblocks;
-	}
-	if (not_aligned)
-		f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
-			"\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
-			not_aligned);
-out:
-	return ret;
-}
-
-static int check_swap_activate_fast(struct swap_info_struct *sis,
+static int check_swap_activate(struct swap_info_struct *sis,
 				struct file *swap_file, sector_t *span)
 {
 	struct address_space *mapping = swap_file->f_mapping;
@@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
 	unsigned int not_aligned = 0;
 	int ret = 0;
 
+	f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
+
 	/*
 	 * Map all the blocks into the extent list.  This code doesn't try
 	 * to be very smart.
@@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
 	return ret;
 }
 
-/* Copied from generic_swapfile_activate() to check any holes */
-static int check_swap_activate(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);
-	unsigned blocks_per_page;
-	unsigned long page_no;
-	sector_t probe_block;
-	sector_t last_block;
-	sector_t lowest_block = -1;
-	sector_t highest_block = 0;
-	int nr_extents = 0;
-	int ret = 0;
-
-	if (PAGE_SIZE == F2FS_BLKSIZE)
-		return check_swap_activate_fast(sis, swap_file, span);
-
-	ret = f2fs_is_file_aligned(inode);
-	if (ret)
-		goto out;
-
-	blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
-
-	/*
-	 * Map all the blocks into the extent list.  This code doesn't try
-	 * to be very smart.
-	 */
-	probe_block = 0;
-	page_no = 0;
-	last_block = bytes_to_blks(inode, i_size_read(inode));
-	while ((probe_block + blocks_per_page) <= last_block &&
-			page_no < sis->max) {
-		unsigned block_in_page;
-		sector_t first_block;
-		sector_t block = 0;
-
-		cond_resched();
-
-		block = probe_block;
-		ret = bmap(inode, &block);
-		if (ret)
-			goto out;
-		if (!block)
-			goto bad_bmap;
-		first_block = block;
-
-		/*
-		 * It must be PAGE_SIZE aligned on-disk
-		 */
-		if (first_block & (blocks_per_page - 1)) {
-			probe_block++;
-			goto reprobe;
-		}
-
-		for (block_in_page = 1; block_in_page < blocks_per_page;
-					block_in_page++) {
-
-			block = probe_block + block_in_page;
-			ret = bmap(inode, &block);
-			if (ret)
-				goto out;
-			if (!block)
-				goto bad_bmap;
-
-			if (block != first_block + block_in_page) {
-				/* Discontiguity */
-				probe_block++;
-				goto reprobe;
-			}
-		}
-
-		first_block >>= (PAGE_SHIFT - inode->i_blkbits);
-		if (page_no) {	/* exclude the header page */
-			if (first_block < lowest_block)
-				lowest_block = first_block;
-			if (first_block > highest_block)
-				highest_block = first_block;
-		}
-
-		/*
-		 * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
-		 */
-		ret = add_swap_extent(sis, page_no, 1, first_block);
-		if (ret < 0)
-			goto out;
-		nr_extents += ret;
-		page_no++;
-		probe_block += blocks_per_page;
-reprobe:
-		continue;
-	}
-	ret = nr_extents;
-	*span = 1 + highest_block - lowest_block;
-	if (page_no == 0)
-		page_no = 1;	/* force Empty message */
-	sis->max = page_no;
-	sis->pages = page_no - 1;
-	sis->highest_bit = page_no - 1;
-out:
-	return ret;
-bad_bmap:
-	f2fs_err(sbi, "Swapfile has holes\n");
-	return -EINVAL;
-}
-
 static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
 				sector_t *span)
 {
-- 
2.29.2



_______________________________________________
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] 22+ messages in thread

* [PATCH 2/2] f2fs: swap: support migrating swapfile in aligned write mode
  2021-05-26  6:29 ` [f2fs-dev] " Chao Yu
@ 2021-05-26  6:29   ` Chao Yu
  -1 siblings, 0 replies; 22+ messages in thread
From: Chao Yu @ 2021-05-26  6:29 UTC (permalink / raw)
  To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, chao, Chao Yu

This patch supports to migrate swapfile in aligned write mode during
swapon in order to keep swapfile being aligned to section as much as
possible, then pinned swapfile will locates fully filled section which
may not affected by GC.

However, for the case that swapfile's size is not aligned to section
size, it will still leave last extent in file's tail as unaligned due
to its size is smaller than section size, like case #2.

case #1
xfs_io -f /mnt/f2fs/file -c "pwrite 0 4M" -c "fsync"

Before swapon:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..3047]:       1123352..1126399  3048 0x1000
   1: [3048..7143]:    237568..241663    4096 0x1000
   2: [7144..8191]:    245760..246807    1048 0x1001
After swapon:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..8191]:       249856..258047    8192 0x1001
Kmsg:
F2FS-fs (zram0): Swapfile (2) is not align to section:
1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate(2097152 * n)

case #2
xfs_io -f /mnt/f2fs/file -c "pwrite 0 3M" -c "fsync"

Before swapon:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..3047]:       246808..249855    3048 0x1000
   1: [3048..6143]:    237568..240663    3096 0x1001
After swapon:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..4095]:       258048..262143    4096 0x1000
   1: [4096..6143]:    238616..240663    2048 0x1001
Kmsg:
F2FS-fs (zram0): Swapfile: last extent is not aligned to section
F2FS-fs (zram0): Swapfile (2) is not align to section:
1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate(2097152 * n)

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/data.c    | 107 ++++++++++++++++++++++++++++++++++++++++------
 fs/f2fs/f2fs.h    |   1 +
 fs/f2fs/node.h    |   3 ++
 fs/f2fs/segment.c |   3 ++
 4 files changed, 101 insertions(+), 13 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 9c23fde93b76..348d88c4a8df 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2472,6 +2472,10 @@ static inline bool check_inplace_update_policy(struct inode *inode,
 
 bool f2fs_should_update_inplace(struct inode *inode, struct f2fs_io_info *fio)
 {
+	/* swap file is migrating in aligned write mode */
+	if (is_inode_flag_set(inode, FI_ALIGNED_WRITE))
+		return false;
+
 	if (f2fs_is_pinned_file(inode))
 		return true;
 
@@ -2494,6 +2498,11 @@ bool f2fs_should_update_outplace(struct inode *inode, struct f2fs_io_info *fio)
 		return true;
 	if (f2fs_is_atomic_file(inode))
 		return true;
+
+	/* swap file is migrating in aligned write mode */
+	if (is_inode_flag_set(inode, FI_ALIGNED_WRITE))
+		return true;
+
 	if (fio) {
 		if (page_private_gcing(fio->page))
 			return true;
@@ -3830,6 +3839,65 @@ int f2fs_migrate_page(struct address_space *mapping,
 #endif
 
 #ifdef CONFIG_SWAP
+static int f2fs_migrate_blocks(struct inode *inode, block_t start_blk,
+							unsigned int blkcnt)
+{
+	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
+	unsigned int blkofs;
+	unsigned int blk_per_sec = BLKS_PER_SEC(sbi);
+	unsigned int secidx = start_blk / blk_per_sec;
+	unsigned int end_sec = secidx + blkcnt / blk_per_sec;
+	int ret = 0;
+
+	down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
+	down_write(&F2FS_I(inode)->i_mmap_sem);
+
+	set_inode_flag(inode, FI_ALIGNED_WRITE);
+
+	for (; secidx < end_sec; secidx++) {
+		down_write(&sbi->pin_sem);
+
+		f2fs_lock_op(sbi);
+		f2fs_allocate_new_section(sbi, CURSEG_COLD_DATA_PINNED, false);
+		f2fs_unlock_op(sbi);
+
+		set_inode_flag(inode, FI_DO_DEFRAG);
+
+		for (blkofs = 0; blkofs < blk_per_sec; blkofs++) {
+			struct page *page;
+			unsigned int blkidx = secidx * blk_per_sec + blkofs;
+
+			page = f2fs_get_lock_data_page(inode, blkidx, true);
+			if (IS_ERR(page)) {
+				up_write(&sbi->pin_sem);
+				ret = PTR_ERR(page);
+				goto done;
+			}
+
+			set_page_dirty(page);
+			f2fs_put_page(page, 1);
+		}
+
+		clear_inode_flag(inode, FI_DO_DEFRAG);
+
+		ret = filemap_fdatawrite(inode->i_mapping);
+
+		up_write(&sbi->pin_sem);
+
+		if (ret)
+			break;
+	}
+
+done:
+	clear_inode_flag(inode, FI_DO_DEFRAG);
+	clear_inode_flag(inode, FI_ALIGNED_WRITE);
+
+	up_write(&F2FS_I(inode)->i_mmap_sem);
+	up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
+
+	return ret;
+}
+
 static int check_swap_activate(struct swap_info_struct *sis,
 				struct file *swap_file, sector_t *span)
 {
@@ -3843,7 +3911,8 @@ static int check_swap_activate(struct swap_info_struct *sis,
 	sector_t highest_pblock = 0;
 	int nr_extents = 0;
 	unsigned long nr_pblocks;
-	unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
+	unsigned int blks_per_sec = BLKS_PER_SEC(sbi);
+	unsigned int sec_blks_mask = BLKS_PER_SEC(sbi) - 1;
 	unsigned int not_aligned = 0;
 	int ret = 0;
 
@@ -3858,7 +3927,7 @@ static int check_swap_activate(struct swap_info_struct *sis,
 
 	while (cur_lblock < last_lblock && cur_lblock < sis->max) {
 		struct f2fs_map_blocks map;
-
+retry:
 		cond_resched();
 
 		memset(&map, 0, sizeof(map));
@@ -3883,16 +3952,28 @@ static int check_swap_activate(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)) {
-			if (f2fs_is_pinned_file(inode)) {
-				f2fs_err(sbi, "Swapfile does not align to section");
-				ret = -EINVAL;
-				goto out;
-			}
+		if ((pblock - SM_I(sbi)->main_blkaddr) & sec_blks_mask ||
+				nr_pblocks & sec_blks_mask) {
 			not_aligned++;
-		}
 
+			nr_pblocks = roundup(nr_pblocks, blks_per_sec);
+			if (cur_lblock + nr_pblocks > sis->max)
+				nr_pblocks -= blks_per_sec;
+
+			if (!nr_pblocks) {
+				/* this extent is last one */
+				nr_pblocks = map.m_len;
+				f2fs_warn(sbi, "Swapfile: last extent is not aligned to section");
+				goto next;
+			}
+
+			ret = f2fs_migrate_blocks(inode, cur_lblock,
+							nr_pblocks);
+			if (ret)
+				goto out;
+			goto retry;
+		}
+next:
 		if (cur_lblock + nr_pblocks >= sis->max)
 			nr_pblocks = sis->max - cur_lblock;
 
@@ -3920,11 +4001,11 @@ static int check_swap_activate(struct swap_info_struct *sis,
 	sis->pages = cur_lblock - 1;
 	sis->highest_bit = cur_lblock - 1;
 
+out:
 	if (not_aligned)
 		f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
-			"\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
-			not_aligned);
-out:
+			"\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate(%u * n)",
+			not_aligned, blks_per_sec * F2FS_BLKSIZE);
 	return ret;
 }
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 7798fb842847..de672c8239bf 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -708,6 +708,7 @@ enum {
 	FI_COMPRESS_CORRUPT,	/* indicate compressed cluster is corrupted */
 	FI_MMAP_FILE,		/* indicate file was mmapped */
 	FI_ENABLE_COMPRESS,	/* enable compression in "user" compression mode */
+	FI_ALIGNED_WRITE,	/* enable aligned write */
 	FI_MAX,			/* max flag, never be used */
 };
 
diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
index d85e8659cfda..e5235dfe4670 100644
--- a/fs/f2fs/node.h
+++ b/fs/f2fs/node.h
@@ -38,6 +38,9 @@
 /* return value for read_node_page */
 #define LOCKED_PAGE	1
 
+/* check pinned file's alignment status of physical blocks */
+#define FILE_NOT_ALIGNED	1
+
 /* For flag in struct node_info */
 enum {
 	IS_CHECKPOINTED,	/* is it checkpointed before? */
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 19b86bea0dbc..1cc7a555763b 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -3291,6 +3291,9 @@ static int __get_segment_type_6(struct f2fs_io_info *fio)
 	if (fio->type == DATA) {
 		struct inode *inode = fio->page->mapping->host;
 
+		if (is_inode_flag_set(inode, FI_ALIGNED_WRITE))
+			return CURSEG_COLD_DATA_PINNED;
+
 		if (page_private_gcing(fio->page)) {
 			if (fio->sbi->am.atgc_enabled &&
 				(fio->io_type == FS_DATA_IO) &&
-- 
2.29.2


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

* [f2fs-dev] [PATCH 2/2] f2fs: swap: support migrating swapfile in aligned write mode
@ 2021-05-26  6:29   ` Chao Yu
  0 siblings, 0 replies; 22+ messages in thread
From: Chao Yu @ 2021-05-26  6:29 UTC (permalink / raw)
  To: jaegeuk; +Cc: linux-kernel, linux-f2fs-devel

This patch supports to migrate swapfile in aligned write mode during
swapon in order to keep swapfile being aligned to section as much as
possible, then pinned swapfile will locates fully filled section which
may not affected by GC.

However, for the case that swapfile's size is not aligned to section
size, it will still leave last extent in file's tail as unaligned due
to its size is smaller than section size, like case #2.

case #1
xfs_io -f /mnt/f2fs/file -c "pwrite 0 4M" -c "fsync"

Before swapon:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..3047]:       1123352..1126399  3048 0x1000
   1: [3048..7143]:    237568..241663    4096 0x1000
   2: [7144..8191]:    245760..246807    1048 0x1001
After swapon:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..8191]:       249856..258047    8192 0x1001
Kmsg:
F2FS-fs (zram0): Swapfile (2) is not align to section:
1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate(2097152 * n)

case #2
xfs_io -f /mnt/f2fs/file -c "pwrite 0 3M" -c "fsync"

Before swapon:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..3047]:       246808..249855    3048 0x1000
   1: [3048..6143]:    237568..240663    3096 0x1001
After swapon:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..4095]:       258048..262143    4096 0x1000
   1: [4096..6143]:    238616..240663    2048 0x1001
Kmsg:
F2FS-fs (zram0): Swapfile: last extent is not aligned to section
F2FS-fs (zram0): Swapfile (2) is not align to section:
1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate(2097152 * n)

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/data.c    | 107 ++++++++++++++++++++++++++++++++++++++++------
 fs/f2fs/f2fs.h    |   1 +
 fs/f2fs/node.h    |   3 ++
 fs/f2fs/segment.c |   3 ++
 4 files changed, 101 insertions(+), 13 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 9c23fde93b76..348d88c4a8df 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2472,6 +2472,10 @@ static inline bool check_inplace_update_policy(struct inode *inode,
 
 bool f2fs_should_update_inplace(struct inode *inode, struct f2fs_io_info *fio)
 {
+	/* swap file is migrating in aligned write mode */
+	if (is_inode_flag_set(inode, FI_ALIGNED_WRITE))
+		return false;
+
 	if (f2fs_is_pinned_file(inode))
 		return true;
 
@@ -2494,6 +2498,11 @@ bool f2fs_should_update_outplace(struct inode *inode, struct f2fs_io_info *fio)
 		return true;
 	if (f2fs_is_atomic_file(inode))
 		return true;
+
+	/* swap file is migrating in aligned write mode */
+	if (is_inode_flag_set(inode, FI_ALIGNED_WRITE))
+		return true;
+
 	if (fio) {
 		if (page_private_gcing(fio->page))
 			return true;
@@ -3830,6 +3839,65 @@ int f2fs_migrate_page(struct address_space *mapping,
 #endif
 
 #ifdef CONFIG_SWAP
+static int f2fs_migrate_blocks(struct inode *inode, block_t start_blk,
+							unsigned int blkcnt)
+{
+	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
+	unsigned int blkofs;
+	unsigned int blk_per_sec = BLKS_PER_SEC(sbi);
+	unsigned int secidx = start_blk / blk_per_sec;
+	unsigned int end_sec = secidx + blkcnt / blk_per_sec;
+	int ret = 0;
+
+	down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
+	down_write(&F2FS_I(inode)->i_mmap_sem);
+
+	set_inode_flag(inode, FI_ALIGNED_WRITE);
+
+	for (; secidx < end_sec; secidx++) {
+		down_write(&sbi->pin_sem);
+
+		f2fs_lock_op(sbi);
+		f2fs_allocate_new_section(sbi, CURSEG_COLD_DATA_PINNED, false);
+		f2fs_unlock_op(sbi);
+
+		set_inode_flag(inode, FI_DO_DEFRAG);
+
+		for (blkofs = 0; blkofs < blk_per_sec; blkofs++) {
+			struct page *page;
+			unsigned int blkidx = secidx * blk_per_sec + blkofs;
+
+			page = f2fs_get_lock_data_page(inode, blkidx, true);
+			if (IS_ERR(page)) {
+				up_write(&sbi->pin_sem);
+				ret = PTR_ERR(page);
+				goto done;
+			}
+
+			set_page_dirty(page);
+			f2fs_put_page(page, 1);
+		}
+
+		clear_inode_flag(inode, FI_DO_DEFRAG);
+
+		ret = filemap_fdatawrite(inode->i_mapping);
+
+		up_write(&sbi->pin_sem);
+
+		if (ret)
+			break;
+	}
+
+done:
+	clear_inode_flag(inode, FI_DO_DEFRAG);
+	clear_inode_flag(inode, FI_ALIGNED_WRITE);
+
+	up_write(&F2FS_I(inode)->i_mmap_sem);
+	up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
+
+	return ret;
+}
+
 static int check_swap_activate(struct swap_info_struct *sis,
 				struct file *swap_file, sector_t *span)
 {
@@ -3843,7 +3911,8 @@ static int check_swap_activate(struct swap_info_struct *sis,
 	sector_t highest_pblock = 0;
 	int nr_extents = 0;
 	unsigned long nr_pblocks;
-	unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
+	unsigned int blks_per_sec = BLKS_PER_SEC(sbi);
+	unsigned int sec_blks_mask = BLKS_PER_SEC(sbi) - 1;
 	unsigned int not_aligned = 0;
 	int ret = 0;
 
@@ -3858,7 +3927,7 @@ static int check_swap_activate(struct swap_info_struct *sis,
 
 	while (cur_lblock < last_lblock && cur_lblock < sis->max) {
 		struct f2fs_map_blocks map;
-
+retry:
 		cond_resched();
 
 		memset(&map, 0, sizeof(map));
@@ -3883,16 +3952,28 @@ static int check_swap_activate(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)) {
-			if (f2fs_is_pinned_file(inode)) {
-				f2fs_err(sbi, "Swapfile does not align to section");
-				ret = -EINVAL;
-				goto out;
-			}
+		if ((pblock - SM_I(sbi)->main_blkaddr) & sec_blks_mask ||
+				nr_pblocks & sec_blks_mask) {
 			not_aligned++;
-		}
 
+			nr_pblocks = roundup(nr_pblocks, blks_per_sec);
+			if (cur_lblock + nr_pblocks > sis->max)
+				nr_pblocks -= blks_per_sec;
+
+			if (!nr_pblocks) {
+				/* this extent is last one */
+				nr_pblocks = map.m_len;
+				f2fs_warn(sbi, "Swapfile: last extent is not aligned to section");
+				goto next;
+			}
+
+			ret = f2fs_migrate_blocks(inode, cur_lblock,
+							nr_pblocks);
+			if (ret)
+				goto out;
+			goto retry;
+		}
+next:
 		if (cur_lblock + nr_pblocks >= sis->max)
 			nr_pblocks = sis->max - cur_lblock;
 
@@ -3920,11 +4001,11 @@ static int check_swap_activate(struct swap_info_struct *sis,
 	sis->pages = cur_lblock - 1;
 	sis->highest_bit = cur_lblock - 1;
 
+out:
 	if (not_aligned)
 		f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
-			"\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
-			not_aligned);
-out:
+			"\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate(%u * n)",
+			not_aligned, blks_per_sec * F2FS_BLKSIZE);
 	return ret;
 }
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 7798fb842847..de672c8239bf 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -708,6 +708,7 @@ enum {
 	FI_COMPRESS_CORRUPT,	/* indicate compressed cluster is corrupted */
 	FI_MMAP_FILE,		/* indicate file was mmapped */
 	FI_ENABLE_COMPRESS,	/* enable compression in "user" compression mode */
+	FI_ALIGNED_WRITE,	/* enable aligned write */
 	FI_MAX,			/* max flag, never be used */
 };
 
diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
index d85e8659cfda..e5235dfe4670 100644
--- a/fs/f2fs/node.h
+++ b/fs/f2fs/node.h
@@ -38,6 +38,9 @@
 /* return value for read_node_page */
 #define LOCKED_PAGE	1
 
+/* check pinned file's alignment status of physical blocks */
+#define FILE_NOT_ALIGNED	1
+
 /* For flag in struct node_info */
 enum {
 	IS_CHECKPOINTED,	/* is it checkpointed before? */
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 19b86bea0dbc..1cc7a555763b 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -3291,6 +3291,9 @@ static int __get_segment_type_6(struct f2fs_io_info *fio)
 	if (fio->type == DATA) {
 		struct inode *inode = fio->page->mapping->host;
 
+		if (is_inode_flag_set(inode, FI_ALIGNED_WRITE))
+			return CURSEG_COLD_DATA_PINNED;
+
 		if (page_private_gcing(fio->page)) {
 			if (fio->sbi->am.atgc_enabled &&
 				(fio->io_type == FS_DATA_IO) &&
-- 
2.29.2



_______________________________________________
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] 22+ messages in thread

* Re: [PATCH 1/2] f2fs: swap: remove dead codes
  2021-05-26  6:29 ` [f2fs-dev] " Chao Yu
@ 2021-05-26 13:30   ` Jaegeuk Kim
  -1 siblings, 0 replies; 22+ messages in thread
From: Jaegeuk Kim @ 2021-05-26 13:30 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, chao

On 05/26, Chao Yu wrote:
> After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
> we will never run into original logic of check_swap_activate() before
> f2fs supports non 4k-sized page, so let's delete those dead codes.

Why not keeping this for large page support in future maybe?

> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/data.c | 171 +------------------------------------------------
>  1 file changed, 3 insertions(+), 168 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 3058c7e28b11..9c23fde93b76 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
>  #endif
>  
>  #ifdef CONFIG_SWAP
> -static int f2fs_is_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 nr_pblocks;
> -	unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
> -	unsigned int not_aligned = 0;
> -	int ret = 0;
> -
> -	cur_lblock = 0;
> -	last_lblock = bytes_to_blks(inode, i_size_read(inode));
> -
> -	while (cur_lblock < last_lblock) {
> -		struct f2fs_map_blocks map;
> -
> -		memset(&map, 0, sizeof(map));
> -		map.m_lblk = cur_lblock;
> -		map.m_len = last_lblock - cur_lblock;
> -		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);
> -		if (ret)
> -			goto out;
> -
> -		/* hole */
> -		if (!(map.m_flags & F2FS_MAP_FLAGS)) {
> -			f2fs_err(sbi, "Swapfile has holes\n");
> -			ret = -ENOENT;
> -			goto out;
> -		}
> -
> -		pblock = map.m_pblk;
> -		nr_pblocks = map.m_len;
> -
> -		if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
> -			nr_pblocks & (blocks_per_sec - 1)) {
> -			if (f2fs_is_pinned_file(inode)) {
> -				f2fs_err(sbi, "Swapfile does not align to section");
> -				ret = -EINVAL;
> -				goto out;
> -			}
> -			not_aligned++;
> -		}
> -
> -		cur_lblock += nr_pblocks;
> -	}
> -	if (not_aligned)
> -		f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
> -			"\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
> -			not_aligned);
> -out:
> -	return ret;
> -}
> -
> -static int check_swap_activate_fast(struct swap_info_struct *sis,
> +static int check_swap_activate(struct swap_info_struct *sis,
>  				struct file *swap_file, sector_t *span)
>  {
>  	struct address_space *mapping = swap_file->f_mapping;
> @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>  	unsigned int not_aligned = 0;
>  	int ret = 0;
>  
> +	f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
> +
>  	/*
>  	 * Map all the blocks into the extent list.  This code doesn't try
>  	 * to be very smart.
> @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>  	return ret;
>  }
>  
> -/* Copied from generic_swapfile_activate() to check any holes */
> -static int check_swap_activate(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);
> -	unsigned blocks_per_page;
> -	unsigned long page_no;
> -	sector_t probe_block;
> -	sector_t last_block;
> -	sector_t lowest_block = -1;
> -	sector_t highest_block = 0;
> -	int nr_extents = 0;
> -	int ret = 0;
> -
> -	if (PAGE_SIZE == F2FS_BLKSIZE)
> -		return check_swap_activate_fast(sis, swap_file, span);
> -
> -	ret = f2fs_is_file_aligned(inode);
> -	if (ret)
> -		goto out;
> -
> -	blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
> -
> -	/*
> -	 * Map all the blocks into the extent list.  This code doesn't try
> -	 * to be very smart.
> -	 */
> -	probe_block = 0;
> -	page_no = 0;
> -	last_block = bytes_to_blks(inode, i_size_read(inode));
> -	while ((probe_block + blocks_per_page) <= last_block &&
> -			page_no < sis->max) {
> -		unsigned block_in_page;
> -		sector_t first_block;
> -		sector_t block = 0;
> -
> -		cond_resched();
> -
> -		block = probe_block;
> -		ret = bmap(inode, &block);
> -		if (ret)
> -			goto out;
> -		if (!block)
> -			goto bad_bmap;
> -		first_block = block;
> -
> -		/*
> -		 * It must be PAGE_SIZE aligned on-disk
> -		 */
> -		if (first_block & (blocks_per_page - 1)) {
> -			probe_block++;
> -			goto reprobe;
> -		}
> -
> -		for (block_in_page = 1; block_in_page < blocks_per_page;
> -					block_in_page++) {
> -
> -			block = probe_block + block_in_page;
> -			ret = bmap(inode, &block);
> -			if (ret)
> -				goto out;
> -			if (!block)
> -				goto bad_bmap;
> -
> -			if (block != first_block + block_in_page) {
> -				/* Discontiguity */
> -				probe_block++;
> -				goto reprobe;
> -			}
> -		}
> -
> -		first_block >>= (PAGE_SHIFT - inode->i_blkbits);
> -		if (page_no) {	/* exclude the header page */
> -			if (first_block < lowest_block)
> -				lowest_block = first_block;
> -			if (first_block > highest_block)
> -				highest_block = first_block;
> -		}
> -
> -		/*
> -		 * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
> -		 */
> -		ret = add_swap_extent(sis, page_no, 1, first_block);
> -		if (ret < 0)
> -			goto out;
> -		nr_extents += ret;
> -		page_no++;
> -		probe_block += blocks_per_page;
> -reprobe:
> -		continue;
> -	}
> -	ret = nr_extents;
> -	*span = 1 + highest_block - lowest_block;
> -	if (page_no == 0)
> -		page_no = 1;	/* force Empty message */
> -	sis->max = page_no;
> -	sis->pages = page_no - 1;
> -	sis->highest_bit = page_no - 1;
> -out:
> -	return ret;
> -bad_bmap:
> -	f2fs_err(sbi, "Swapfile has holes\n");
> -	return -EINVAL;
> -}
> -
>  static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
>  				sector_t *span)
>  {
> -- 
> 2.29.2

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

* Re: [f2fs-dev] [PATCH 1/2] f2fs: swap: remove dead codes
@ 2021-05-26 13:30   ` Jaegeuk Kim
  0 siblings, 0 replies; 22+ messages in thread
From: Jaegeuk Kim @ 2021-05-26 13:30 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel

On 05/26, Chao Yu wrote:
> After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
> we will never run into original logic of check_swap_activate() before
> f2fs supports non 4k-sized page, so let's delete those dead codes.

Why not keeping this for large page support in future maybe?

> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/data.c | 171 +------------------------------------------------
>  1 file changed, 3 insertions(+), 168 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 3058c7e28b11..9c23fde93b76 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
>  #endif
>  
>  #ifdef CONFIG_SWAP
> -static int f2fs_is_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 nr_pblocks;
> -	unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
> -	unsigned int not_aligned = 0;
> -	int ret = 0;
> -
> -	cur_lblock = 0;
> -	last_lblock = bytes_to_blks(inode, i_size_read(inode));
> -
> -	while (cur_lblock < last_lblock) {
> -		struct f2fs_map_blocks map;
> -
> -		memset(&map, 0, sizeof(map));
> -		map.m_lblk = cur_lblock;
> -		map.m_len = last_lblock - cur_lblock;
> -		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);
> -		if (ret)
> -			goto out;
> -
> -		/* hole */
> -		if (!(map.m_flags & F2FS_MAP_FLAGS)) {
> -			f2fs_err(sbi, "Swapfile has holes\n");
> -			ret = -ENOENT;
> -			goto out;
> -		}
> -
> -		pblock = map.m_pblk;
> -		nr_pblocks = map.m_len;
> -
> -		if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
> -			nr_pblocks & (blocks_per_sec - 1)) {
> -			if (f2fs_is_pinned_file(inode)) {
> -				f2fs_err(sbi, "Swapfile does not align to section");
> -				ret = -EINVAL;
> -				goto out;
> -			}
> -			not_aligned++;
> -		}
> -
> -		cur_lblock += nr_pblocks;
> -	}
> -	if (not_aligned)
> -		f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
> -			"\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
> -			not_aligned);
> -out:
> -	return ret;
> -}
> -
> -static int check_swap_activate_fast(struct swap_info_struct *sis,
> +static int check_swap_activate(struct swap_info_struct *sis,
>  				struct file *swap_file, sector_t *span)
>  {
>  	struct address_space *mapping = swap_file->f_mapping;
> @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>  	unsigned int not_aligned = 0;
>  	int ret = 0;
>  
> +	f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
> +
>  	/*
>  	 * Map all the blocks into the extent list.  This code doesn't try
>  	 * to be very smart.
> @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>  	return ret;
>  }
>  
> -/* Copied from generic_swapfile_activate() to check any holes */
> -static int check_swap_activate(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);
> -	unsigned blocks_per_page;
> -	unsigned long page_no;
> -	sector_t probe_block;
> -	sector_t last_block;
> -	sector_t lowest_block = -1;
> -	sector_t highest_block = 0;
> -	int nr_extents = 0;
> -	int ret = 0;
> -
> -	if (PAGE_SIZE == F2FS_BLKSIZE)
> -		return check_swap_activate_fast(sis, swap_file, span);
> -
> -	ret = f2fs_is_file_aligned(inode);
> -	if (ret)
> -		goto out;
> -
> -	blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
> -
> -	/*
> -	 * Map all the blocks into the extent list.  This code doesn't try
> -	 * to be very smart.
> -	 */
> -	probe_block = 0;
> -	page_no = 0;
> -	last_block = bytes_to_blks(inode, i_size_read(inode));
> -	while ((probe_block + blocks_per_page) <= last_block &&
> -			page_no < sis->max) {
> -		unsigned block_in_page;
> -		sector_t first_block;
> -		sector_t block = 0;
> -
> -		cond_resched();
> -
> -		block = probe_block;
> -		ret = bmap(inode, &block);
> -		if (ret)
> -			goto out;
> -		if (!block)
> -			goto bad_bmap;
> -		first_block = block;
> -
> -		/*
> -		 * It must be PAGE_SIZE aligned on-disk
> -		 */
> -		if (first_block & (blocks_per_page - 1)) {
> -			probe_block++;
> -			goto reprobe;
> -		}
> -
> -		for (block_in_page = 1; block_in_page < blocks_per_page;
> -					block_in_page++) {
> -
> -			block = probe_block + block_in_page;
> -			ret = bmap(inode, &block);
> -			if (ret)
> -				goto out;
> -			if (!block)
> -				goto bad_bmap;
> -
> -			if (block != first_block + block_in_page) {
> -				/* Discontiguity */
> -				probe_block++;
> -				goto reprobe;
> -			}
> -		}
> -
> -		first_block >>= (PAGE_SHIFT - inode->i_blkbits);
> -		if (page_no) {	/* exclude the header page */
> -			if (first_block < lowest_block)
> -				lowest_block = first_block;
> -			if (first_block > highest_block)
> -				highest_block = first_block;
> -		}
> -
> -		/*
> -		 * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
> -		 */
> -		ret = add_swap_extent(sis, page_no, 1, first_block);
> -		if (ret < 0)
> -			goto out;
> -		nr_extents += ret;
> -		page_no++;
> -		probe_block += blocks_per_page;
> -reprobe:
> -		continue;
> -	}
> -	ret = nr_extents;
> -	*span = 1 + highest_block - lowest_block;
> -	if (page_no == 0)
> -		page_no = 1;	/* force Empty message */
> -	sis->max = page_no;
> -	sis->pages = page_no - 1;
> -	sis->highest_bit = page_no - 1;
> -out:
> -	return ret;
> -bad_bmap:
> -	f2fs_err(sbi, "Swapfile has holes\n");
> -	return -EINVAL;
> -}
> -
>  static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
>  				sector_t *span)
>  {
> -- 
> 2.29.2


_______________________________________________
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] 22+ messages in thread

* Re: [PATCH 1/2] f2fs: swap: remove dead codes
  2021-05-26 13:30   ` [f2fs-dev] " Jaegeuk Kim
@ 2021-05-26 15:02     ` Chao Yu
  -1 siblings, 0 replies; 22+ messages in thread
From: Chao Yu @ 2021-05-26 15:02 UTC (permalink / raw)
  To: Jaegeuk Kim, Chao Yu; +Cc: linux-f2fs-devel, linux-kernel

On 2021/5/26 21:30, Jaegeuk Kim wrote:
> On 05/26, Chao Yu wrote:
>> After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
>> we will never run into original logic of check_swap_activate() before
>> f2fs supports non 4k-sized page, so let's delete those dead codes.
> 
> Why not keeping this for large page support in future maybe?

Well, if so, at that time, it would be better to refactor
check_swap_activate_fast() implementation based on f2fs_map_block() rather
than refactoring check_swap_activate() implementation based on low efficient
bmap()?

Thanks,

> 
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>>   fs/f2fs/data.c | 171 +------------------------------------------------
>>   1 file changed, 3 insertions(+), 168 deletions(-)
>>
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index 3058c7e28b11..9c23fde93b76 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
>>   #endif
>>   
>>   #ifdef CONFIG_SWAP
>> -static int f2fs_is_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 nr_pblocks;
>> -	unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
>> -	unsigned int not_aligned = 0;
>> -	int ret = 0;
>> -
>> -	cur_lblock = 0;
>> -	last_lblock = bytes_to_blks(inode, i_size_read(inode));
>> -
>> -	while (cur_lblock < last_lblock) {
>> -		struct f2fs_map_blocks map;
>> -
>> -		memset(&map, 0, sizeof(map));
>> -		map.m_lblk = cur_lblock;
>> -		map.m_len = last_lblock - cur_lblock;
>> -		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);
>> -		if (ret)
>> -			goto out;
>> -
>> -		/* hole */
>> -		if (!(map.m_flags & F2FS_MAP_FLAGS)) {
>> -			f2fs_err(sbi, "Swapfile has holes\n");
>> -			ret = -ENOENT;
>> -			goto out;
>> -		}
>> -
>> -		pblock = map.m_pblk;
>> -		nr_pblocks = map.m_len;
>> -
>> -		if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
>> -			nr_pblocks & (blocks_per_sec - 1)) {
>> -			if (f2fs_is_pinned_file(inode)) {
>> -				f2fs_err(sbi, "Swapfile does not align to section");
>> -				ret = -EINVAL;
>> -				goto out;
>> -			}
>> -			not_aligned++;
>> -		}
>> -
>> -		cur_lblock += nr_pblocks;
>> -	}
>> -	if (not_aligned)
>> -		f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
>> -			"\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
>> -			not_aligned);
>> -out:
>> -	return ret;
>> -}
>> -
>> -static int check_swap_activate_fast(struct swap_info_struct *sis,
>> +static int check_swap_activate(struct swap_info_struct *sis,
>>   				struct file *swap_file, sector_t *span)
>>   {
>>   	struct address_space *mapping = swap_file->f_mapping;
>> @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>   	unsigned int not_aligned = 0;
>>   	int ret = 0;
>>   
>> +	f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
>> +
>>   	/*
>>   	 * Map all the blocks into the extent list.  This code doesn't try
>>   	 * to be very smart.
>> @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>   	return ret;
>>   }
>>   
>> -/* Copied from generic_swapfile_activate() to check any holes */
>> -static int check_swap_activate(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);
>> -	unsigned blocks_per_page;
>> -	unsigned long page_no;
>> -	sector_t probe_block;
>> -	sector_t last_block;
>> -	sector_t lowest_block = -1;
>> -	sector_t highest_block = 0;
>> -	int nr_extents = 0;
>> -	int ret = 0;
>> -
>> -	if (PAGE_SIZE == F2FS_BLKSIZE)
>> -		return check_swap_activate_fast(sis, swap_file, span);
>> -
>> -	ret = f2fs_is_file_aligned(inode);
>> -	if (ret)
>> -		goto out;
>> -
>> -	blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
>> -
>> -	/*
>> -	 * Map all the blocks into the extent list.  This code doesn't try
>> -	 * to be very smart.
>> -	 */
>> -	probe_block = 0;
>> -	page_no = 0;
>> -	last_block = bytes_to_blks(inode, i_size_read(inode));
>> -	while ((probe_block + blocks_per_page) <= last_block &&
>> -			page_no < sis->max) {
>> -		unsigned block_in_page;
>> -		sector_t first_block;
>> -		sector_t block = 0;
>> -
>> -		cond_resched();
>> -
>> -		block = probe_block;
>> -		ret = bmap(inode, &block);
>> -		if (ret)
>> -			goto out;
>> -		if (!block)
>> -			goto bad_bmap;
>> -		first_block = block;
>> -
>> -		/*
>> -		 * It must be PAGE_SIZE aligned on-disk
>> -		 */
>> -		if (first_block & (blocks_per_page - 1)) {
>> -			probe_block++;
>> -			goto reprobe;
>> -		}
>> -
>> -		for (block_in_page = 1; block_in_page < blocks_per_page;
>> -					block_in_page++) {
>> -
>> -			block = probe_block + block_in_page;
>> -			ret = bmap(inode, &block);
>> -			if (ret)
>> -				goto out;
>> -			if (!block)
>> -				goto bad_bmap;
>> -
>> -			if (block != first_block + block_in_page) {
>> -				/* Discontiguity */
>> -				probe_block++;
>> -				goto reprobe;
>> -			}
>> -		}
>> -
>> -		first_block >>= (PAGE_SHIFT - inode->i_blkbits);
>> -		if (page_no) {	/* exclude the header page */
>> -			if (first_block < lowest_block)
>> -				lowest_block = first_block;
>> -			if (first_block > highest_block)
>> -				highest_block = first_block;
>> -		}
>> -
>> -		/*
>> -		 * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
>> -		 */
>> -		ret = add_swap_extent(sis, page_no, 1, first_block);
>> -		if (ret < 0)
>> -			goto out;
>> -		nr_extents += ret;
>> -		page_no++;
>> -		probe_block += blocks_per_page;
>> -reprobe:
>> -		continue;
>> -	}
>> -	ret = nr_extents;
>> -	*span = 1 + highest_block - lowest_block;
>> -	if (page_no == 0)
>> -		page_no = 1;	/* force Empty message */
>> -	sis->max = page_no;
>> -	sis->pages = page_no - 1;
>> -	sis->highest_bit = page_no - 1;
>> -out:
>> -	return ret;
>> -bad_bmap:
>> -	f2fs_err(sbi, "Swapfile has holes\n");
>> -	return -EINVAL;
>> -}
>> -
>>   static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
>>   				sector_t *span)
>>   {
>> -- 
>> 2.29.2

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

* Re: [f2fs-dev] [PATCH 1/2] f2fs: swap: remove dead codes
@ 2021-05-26 15:02     ` Chao Yu
  0 siblings, 0 replies; 22+ messages in thread
From: Chao Yu @ 2021-05-26 15:02 UTC (permalink / raw)
  To: Jaegeuk Kim, Chao Yu; +Cc: linux-kernel, linux-f2fs-devel

On 2021/5/26 21:30, Jaegeuk Kim wrote:
> On 05/26, Chao Yu wrote:
>> After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
>> we will never run into original logic of check_swap_activate() before
>> f2fs supports non 4k-sized page, so let's delete those dead codes.
> 
> Why not keeping this for large page support in future maybe?

Well, if so, at that time, it would be better to refactor
check_swap_activate_fast() implementation based on f2fs_map_block() rather
than refactoring check_swap_activate() implementation based on low efficient
bmap()?

Thanks,

> 
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>>   fs/f2fs/data.c | 171 +------------------------------------------------
>>   1 file changed, 3 insertions(+), 168 deletions(-)
>>
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index 3058c7e28b11..9c23fde93b76 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
>>   #endif
>>   
>>   #ifdef CONFIG_SWAP
>> -static int f2fs_is_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 nr_pblocks;
>> -	unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
>> -	unsigned int not_aligned = 0;
>> -	int ret = 0;
>> -
>> -	cur_lblock = 0;
>> -	last_lblock = bytes_to_blks(inode, i_size_read(inode));
>> -
>> -	while (cur_lblock < last_lblock) {
>> -		struct f2fs_map_blocks map;
>> -
>> -		memset(&map, 0, sizeof(map));
>> -		map.m_lblk = cur_lblock;
>> -		map.m_len = last_lblock - cur_lblock;
>> -		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);
>> -		if (ret)
>> -			goto out;
>> -
>> -		/* hole */
>> -		if (!(map.m_flags & F2FS_MAP_FLAGS)) {
>> -			f2fs_err(sbi, "Swapfile has holes\n");
>> -			ret = -ENOENT;
>> -			goto out;
>> -		}
>> -
>> -		pblock = map.m_pblk;
>> -		nr_pblocks = map.m_len;
>> -
>> -		if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
>> -			nr_pblocks & (blocks_per_sec - 1)) {
>> -			if (f2fs_is_pinned_file(inode)) {
>> -				f2fs_err(sbi, "Swapfile does not align to section");
>> -				ret = -EINVAL;
>> -				goto out;
>> -			}
>> -			not_aligned++;
>> -		}
>> -
>> -		cur_lblock += nr_pblocks;
>> -	}
>> -	if (not_aligned)
>> -		f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
>> -			"\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
>> -			not_aligned);
>> -out:
>> -	return ret;
>> -}
>> -
>> -static int check_swap_activate_fast(struct swap_info_struct *sis,
>> +static int check_swap_activate(struct swap_info_struct *sis,
>>   				struct file *swap_file, sector_t *span)
>>   {
>>   	struct address_space *mapping = swap_file->f_mapping;
>> @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>   	unsigned int not_aligned = 0;
>>   	int ret = 0;
>>   
>> +	f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
>> +
>>   	/*
>>   	 * Map all the blocks into the extent list.  This code doesn't try
>>   	 * to be very smart.
>> @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>   	return ret;
>>   }
>>   
>> -/* Copied from generic_swapfile_activate() to check any holes */
>> -static int check_swap_activate(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);
>> -	unsigned blocks_per_page;
>> -	unsigned long page_no;
>> -	sector_t probe_block;
>> -	sector_t last_block;
>> -	sector_t lowest_block = -1;
>> -	sector_t highest_block = 0;
>> -	int nr_extents = 0;
>> -	int ret = 0;
>> -
>> -	if (PAGE_SIZE == F2FS_BLKSIZE)
>> -		return check_swap_activate_fast(sis, swap_file, span);
>> -
>> -	ret = f2fs_is_file_aligned(inode);
>> -	if (ret)
>> -		goto out;
>> -
>> -	blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
>> -
>> -	/*
>> -	 * Map all the blocks into the extent list.  This code doesn't try
>> -	 * to be very smart.
>> -	 */
>> -	probe_block = 0;
>> -	page_no = 0;
>> -	last_block = bytes_to_blks(inode, i_size_read(inode));
>> -	while ((probe_block + blocks_per_page) <= last_block &&
>> -			page_no < sis->max) {
>> -		unsigned block_in_page;
>> -		sector_t first_block;
>> -		sector_t block = 0;
>> -
>> -		cond_resched();
>> -
>> -		block = probe_block;
>> -		ret = bmap(inode, &block);
>> -		if (ret)
>> -			goto out;
>> -		if (!block)
>> -			goto bad_bmap;
>> -		first_block = block;
>> -
>> -		/*
>> -		 * It must be PAGE_SIZE aligned on-disk
>> -		 */
>> -		if (first_block & (blocks_per_page - 1)) {
>> -			probe_block++;
>> -			goto reprobe;
>> -		}
>> -
>> -		for (block_in_page = 1; block_in_page < blocks_per_page;
>> -					block_in_page++) {
>> -
>> -			block = probe_block + block_in_page;
>> -			ret = bmap(inode, &block);
>> -			if (ret)
>> -				goto out;
>> -			if (!block)
>> -				goto bad_bmap;
>> -
>> -			if (block != first_block + block_in_page) {
>> -				/* Discontiguity */
>> -				probe_block++;
>> -				goto reprobe;
>> -			}
>> -		}
>> -
>> -		first_block >>= (PAGE_SHIFT - inode->i_blkbits);
>> -		if (page_no) {	/* exclude the header page */
>> -			if (first_block < lowest_block)
>> -				lowest_block = first_block;
>> -			if (first_block > highest_block)
>> -				highest_block = first_block;
>> -		}
>> -
>> -		/*
>> -		 * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
>> -		 */
>> -		ret = add_swap_extent(sis, page_no, 1, first_block);
>> -		if (ret < 0)
>> -			goto out;
>> -		nr_extents += ret;
>> -		page_no++;
>> -		probe_block += blocks_per_page;
>> -reprobe:
>> -		continue;
>> -	}
>> -	ret = nr_extents;
>> -	*span = 1 + highest_block - lowest_block;
>> -	if (page_no == 0)
>> -		page_no = 1;	/* force Empty message */
>> -	sis->max = page_no;
>> -	sis->pages = page_no - 1;
>> -	sis->highest_bit = page_no - 1;
>> -out:
>> -	return ret;
>> -bad_bmap:
>> -	f2fs_err(sbi, "Swapfile has holes\n");
>> -	return -EINVAL;
>> -}
>> -
>>   static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
>>   				sector_t *span)
>>   {
>> -- 
>> 2.29.2


_______________________________________________
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] 22+ messages in thread

* Re: [PATCH 1/2] f2fs: swap: remove dead codes
  2021-05-26 15:02     ` [f2fs-dev] " Chao Yu
@ 2021-05-27  1:40       ` Jaegeuk Kim
  -1 siblings, 0 replies; 22+ messages in thread
From: Jaegeuk Kim @ 2021-05-27  1:40 UTC (permalink / raw)
  To: Chao Yu; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel

On 05/26, Chao Yu wrote:
> On 2021/5/26 21:30, Jaegeuk Kim wrote:
> > On 05/26, Chao Yu wrote:
> > > After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
> > > we will never run into original logic of check_swap_activate() before
> > > f2fs supports non 4k-sized page, so let's delete those dead codes.
> > 
> > Why not keeping this for large page support in future maybe?
> 
> Well, if so, at that time, it would be better to refactor
> check_swap_activate_fast() implementation based on f2fs_map_block() rather
> than refactoring check_swap_activate() implementation based on low efficient
> bmap()?

Let's first check whether we support large page. Have you quickly tested it?
If we support it now and current flow is just inefficient, I'd say keeping but
refactoring it later.

> 
> Thanks,
> 
> > 
> > > 
> > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > ---
> > >   fs/f2fs/data.c | 171 +------------------------------------------------
> > >   1 file changed, 3 insertions(+), 168 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > index 3058c7e28b11..9c23fde93b76 100644
> > > --- a/fs/f2fs/data.c
> > > +++ b/fs/f2fs/data.c
> > > @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
> > >   #endif
> > >   #ifdef CONFIG_SWAP
> > > -static int f2fs_is_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 nr_pblocks;
> > > -	unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
> > > -	unsigned int not_aligned = 0;
> > > -	int ret = 0;
> > > -
> > > -	cur_lblock = 0;
> > > -	last_lblock = bytes_to_blks(inode, i_size_read(inode));
> > > -
> > > -	while (cur_lblock < last_lblock) {
> > > -		struct f2fs_map_blocks map;
> > > -
> > > -		memset(&map, 0, sizeof(map));
> > > -		map.m_lblk = cur_lblock;
> > > -		map.m_len = last_lblock - cur_lblock;
> > > -		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);
> > > -		if (ret)
> > > -			goto out;
> > > -
> > > -		/* hole */
> > > -		if (!(map.m_flags & F2FS_MAP_FLAGS)) {
> > > -			f2fs_err(sbi, "Swapfile has holes\n");
> > > -			ret = -ENOENT;
> > > -			goto out;
> > > -		}
> > > -
> > > -		pblock = map.m_pblk;
> > > -		nr_pblocks = map.m_len;
> > > -
> > > -		if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
> > > -			nr_pblocks & (blocks_per_sec - 1)) {
> > > -			if (f2fs_is_pinned_file(inode)) {
> > > -				f2fs_err(sbi, "Swapfile does not align to section");
> > > -				ret = -EINVAL;
> > > -				goto out;
> > > -			}
> > > -			not_aligned++;
> > > -		}
> > > -
> > > -		cur_lblock += nr_pblocks;
> > > -	}
> > > -	if (not_aligned)
> > > -		f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
> > > -			"\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
> > > -			not_aligned);
> > > -out:
> > > -	return ret;
> > > -}
> > > -
> > > -static int check_swap_activate_fast(struct swap_info_struct *sis,
> > > +static int check_swap_activate(struct swap_info_struct *sis,
> > >   				struct file *swap_file, sector_t *span)
> > >   {
> > >   	struct address_space *mapping = swap_file->f_mapping;
> > > @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
> > >   	unsigned int not_aligned = 0;
> > >   	int ret = 0;
> > > +	f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
> > > +
> > >   	/*
> > >   	 * Map all the blocks into the extent list.  This code doesn't try
> > >   	 * to be very smart.
> > > @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
> > >   	return ret;
> > >   }
> > > -/* Copied from generic_swapfile_activate() to check any holes */
> > > -static int check_swap_activate(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);
> > > -	unsigned blocks_per_page;
> > > -	unsigned long page_no;
> > > -	sector_t probe_block;
> > > -	sector_t last_block;
> > > -	sector_t lowest_block = -1;
> > > -	sector_t highest_block = 0;
> > > -	int nr_extents = 0;
> > > -	int ret = 0;
> > > -
> > > -	if (PAGE_SIZE == F2FS_BLKSIZE)
> > > -		return check_swap_activate_fast(sis, swap_file, span);
> > > -
> > > -	ret = f2fs_is_file_aligned(inode);
> > > -	if (ret)
> > > -		goto out;
> > > -
> > > -	blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
> > > -
> > > -	/*
> > > -	 * Map all the blocks into the extent list.  This code doesn't try
> > > -	 * to be very smart.
> > > -	 */
> > > -	probe_block = 0;
> > > -	page_no = 0;
> > > -	last_block = bytes_to_blks(inode, i_size_read(inode));
> > > -	while ((probe_block + blocks_per_page) <= last_block &&
> > > -			page_no < sis->max) {
> > > -		unsigned block_in_page;
> > > -		sector_t first_block;
> > > -		sector_t block = 0;
> > > -
> > > -		cond_resched();
> > > -
> > > -		block = probe_block;
> > > -		ret = bmap(inode, &block);
> > > -		if (ret)
> > > -			goto out;
> > > -		if (!block)
> > > -			goto bad_bmap;
> > > -		first_block = block;
> > > -
> > > -		/*
> > > -		 * It must be PAGE_SIZE aligned on-disk
> > > -		 */
> > > -		if (first_block & (blocks_per_page - 1)) {
> > > -			probe_block++;
> > > -			goto reprobe;
> > > -		}
> > > -
> > > -		for (block_in_page = 1; block_in_page < blocks_per_page;
> > > -					block_in_page++) {
> > > -
> > > -			block = probe_block + block_in_page;
> > > -			ret = bmap(inode, &block);
> > > -			if (ret)
> > > -				goto out;
> > > -			if (!block)
> > > -				goto bad_bmap;
> > > -
> > > -			if (block != first_block + block_in_page) {
> > > -				/* Discontiguity */
> > > -				probe_block++;
> > > -				goto reprobe;
> > > -			}
> > > -		}
> > > -
> > > -		first_block >>= (PAGE_SHIFT - inode->i_blkbits);
> > > -		if (page_no) {	/* exclude the header page */
> > > -			if (first_block < lowest_block)
> > > -				lowest_block = first_block;
> > > -			if (first_block > highest_block)
> > > -				highest_block = first_block;
> > > -		}
> > > -
> > > -		/*
> > > -		 * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
> > > -		 */
> > > -		ret = add_swap_extent(sis, page_no, 1, first_block);
> > > -		if (ret < 0)
> > > -			goto out;
> > > -		nr_extents += ret;
> > > -		page_no++;
> > > -		probe_block += blocks_per_page;
> > > -reprobe:
> > > -		continue;
> > > -	}
> > > -	ret = nr_extents;
> > > -	*span = 1 + highest_block - lowest_block;
> > > -	if (page_no == 0)
> > > -		page_no = 1;	/* force Empty message */
> > > -	sis->max = page_no;
> > > -	sis->pages = page_no - 1;
> > > -	sis->highest_bit = page_no - 1;
> > > -out:
> > > -	return ret;
> > > -bad_bmap:
> > > -	f2fs_err(sbi, "Swapfile has holes\n");
> > > -	return -EINVAL;
> > > -}
> > > -
> > >   static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
> > >   				sector_t *span)
> > >   {
> > > -- 
> > > 2.29.2

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

* Re: [f2fs-dev] [PATCH 1/2] f2fs: swap: remove dead codes
@ 2021-05-27  1:40       ` Jaegeuk Kim
  0 siblings, 0 replies; 22+ messages in thread
From: Jaegeuk Kim @ 2021-05-27  1:40 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel

On 05/26, Chao Yu wrote:
> On 2021/5/26 21:30, Jaegeuk Kim wrote:
> > On 05/26, Chao Yu wrote:
> > > After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
> > > we will never run into original logic of check_swap_activate() before
> > > f2fs supports non 4k-sized page, so let's delete those dead codes.
> > 
> > Why not keeping this for large page support in future maybe?
> 
> Well, if so, at that time, it would be better to refactor
> check_swap_activate_fast() implementation based on f2fs_map_block() rather
> than refactoring check_swap_activate() implementation based on low efficient
> bmap()?

Let's first check whether we support large page. Have you quickly tested it?
If we support it now and current flow is just inefficient, I'd say keeping but
refactoring it later.

> 
> Thanks,
> 
> > 
> > > 
> > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > ---
> > >   fs/f2fs/data.c | 171 +------------------------------------------------
> > >   1 file changed, 3 insertions(+), 168 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > index 3058c7e28b11..9c23fde93b76 100644
> > > --- a/fs/f2fs/data.c
> > > +++ b/fs/f2fs/data.c
> > > @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
> > >   #endif
> > >   #ifdef CONFIG_SWAP
> > > -static int f2fs_is_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 nr_pblocks;
> > > -	unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
> > > -	unsigned int not_aligned = 0;
> > > -	int ret = 0;
> > > -
> > > -	cur_lblock = 0;
> > > -	last_lblock = bytes_to_blks(inode, i_size_read(inode));
> > > -
> > > -	while (cur_lblock < last_lblock) {
> > > -		struct f2fs_map_blocks map;
> > > -
> > > -		memset(&map, 0, sizeof(map));
> > > -		map.m_lblk = cur_lblock;
> > > -		map.m_len = last_lblock - cur_lblock;
> > > -		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);
> > > -		if (ret)
> > > -			goto out;
> > > -
> > > -		/* hole */
> > > -		if (!(map.m_flags & F2FS_MAP_FLAGS)) {
> > > -			f2fs_err(sbi, "Swapfile has holes\n");
> > > -			ret = -ENOENT;
> > > -			goto out;
> > > -		}
> > > -
> > > -		pblock = map.m_pblk;
> > > -		nr_pblocks = map.m_len;
> > > -
> > > -		if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
> > > -			nr_pblocks & (blocks_per_sec - 1)) {
> > > -			if (f2fs_is_pinned_file(inode)) {
> > > -				f2fs_err(sbi, "Swapfile does not align to section");
> > > -				ret = -EINVAL;
> > > -				goto out;
> > > -			}
> > > -			not_aligned++;
> > > -		}
> > > -
> > > -		cur_lblock += nr_pblocks;
> > > -	}
> > > -	if (not_aligned)
> > > -		f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
> > > -			"\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
> > > -			not_aligned);
> > > -out:
> > > -	return ret;
> > > -}
> > > -
> > > -static int check_swap_activate_fast(struct swap_info_struct *sis,
> > > +static int check_swap_activate(struct swap_info_struct *sis,
> > >   				struct file *swap_file, sector_t *span)
> > >   {
> > >   	struct address_space *mapping = swap_file->f_mapping;
> > > @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
> > >   	unsigned int not_aligned = 0;
> > >   	int ret = 0;
> > > +	f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
> > > +
> > >   	/*
> > >   	 * Map all the blocks into the extent list.  This code doesn't try
> > >   	 * to be very smart.
> > > @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
> > >   	return ret;
> > >   }
> > > -/* Copied from generic_swapfile_activate() to check any holes */
> > > -static int check_swap_activate(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);
> > > -	unsigned blocks_per_page;
> > > -	unsigned long page_no;
> > > -	sector_t probe_block;
> > > -	sector_t last_block;
> > > -	sector_t lowest_block = -1;
> > > -	sector_t highest_block = 0;
> > > -	int nr_extents = 0;
> > > -	int ret = 0;
> > > -
> > > -	if (PAGE_SIZE == F2FS_BLKSIZE)
> > > -		return check_swap_activate_fast(sis, swap_file, span);
> > > -
> > > -	ret = f2fs_is_file_aligned(inode);
> > > -	if (ret)
> > > -		goto out;
> > > -
> > > -	blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
> > > -
> > > -	/*
> > > -	 * Map all the blocks into the extent list.  This code doesn't try
> > > -	 * to be very smart.
> > > -	 */
> > > -	probe_block = 0;
> > > -	page_no = 0;
> > > -	last_block = bytes_to_blks(inode, i_size_read(inode));
> > > -	while ((probe_block + blocks_per_page) <= last_block &&
> > > -			page_no < sis->max) {
> > > -		unsigned block_in_page;
> > > -		sector_t first_block;
> > > -		sector_t block = 0;
> > > -
> > > -		cond_resched();
> > > -
> > > -		block = probe_block;
> > > -		ret = bmap(inode, &block);
> > > -		if (ret)
> > > -			goto out;
> > > -		if (!block)
> > > -			goto bad_bmap;
> > > -		first_block = block;
> > > -
> > > -		/*
> > > -		 * It must be PAGE_SIZE aligned on-disk
> > > -		 */
> > > -		if (first_block & (blocks_per_page - 1)) {
> > > -			probe_block++;
> > > -			goto reprobe;
> > > -		}
> > > -
> > > -		for (block_in_page = 1; block_in_page < blocks_per_page;
> > > -					block_in_page++) {
> > > -
> > > -			block = probe_block + block_in_page;
> > > -			ret = bmap(inode, &block);
> > > -			if (ret)
> > > -				goto out;
> > > -			if (!block)
> > > -				goto bad_bmap;
> > > -
> > > -			if (block != first_block + block_in_page) {
> > > -				/* Discontiguity */
> > > -				probe_block++;
> > > -				goto reprobe;
> > > -			}
> > > -		}
> > > -
> > > -		first_block >>= (PAGE_SHIFT - inode->i_blkbits);
> > > -		if (page_no) {	/* exclude the header page */
> > > -			if (first_block < lowest_block)
> > > -				lowest_block = first_block;
> > > -			if (first_block > highest_block)
> > > -				highest_block = first_block;
> > > -		}
> > > -
> > > -		/*
> > > -		 * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
> > > -		 */
> > > -		ret = add_swap_extent(sis, page_no, 1, first_block);
> > > -		if (ret < 0)
> > > -			goto out;
> > > -		nr_extents += ret;
> > > -		page_no++;
> > > -		probe_block += blocks_per_page;
> > > -reprobe:
> > > -		continue;
> > > -	}
> > > -	ret = nr_extents;
> > > -	*span = 1 + highest_block - lowest_block;
> > > -	if (page_no == 0)
> > > -		page_no = 1;	/* force Empty message */
> > > -	sis->max = page_no;
> > > -	sis->pages = page_no - 1;
> > > -	sis->highest_bit = page_no - 1;
> > > -out:
> > > -	return ret;
> > > -bad_bmap:
> > > -	f2fs_err(sbi, "Swapfile has holes\n");
> > > -	return -EINVAL;
> > > -}
> > > -
> > >   static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
> > >   				sector_t *span)
> > >   {
> > > -- 
> > > 2.29.2


_______________________________________________
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] 22+ messages in thread

* Re: [PATCH 1/2] f2fs: swap: remove dead codes
  2021-05-27  1:40       ` [f2fs-dev] " Jaegeuk Kim
@ 2021-05-27  2:12         ` Chao Yu
  -1 siblings, 0 replies; 22+ messages in thread
From: Chao Yu @ 2021-05-27  2:12 UTC (permalink / raw)
  To: Jaegeuk Kim, Chao Yu; +Cc: linux-f2fs-devel, linux-kernel

On 2021/5/27 9:40, Jaegeuk Kim wrote:
> On 05/26, Chao Yu wrote:
>> On 2021/5/26 21:30, Jaegeuk Kim wrote:
>>> On 05/26, Chao Yu wrote:
>>>> After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
>>>> we will never run into original logic of check_swap_activate() before
>>>> f2fs supports non 4k-sized page, so let's delete those dead codes.
>>>
>>> Why not keeping this for large page support in future maybe?
>>
>> Well, if so, at that time, it would be better to refactor
>> check_swap_activate_fast() implementation based on f2fs_map_block() rather
>> than refactoring check_swap_activate() implementation based on low efficient
>> bmap()?
> 
> Let's first check whether we support large page. Have you quickly tested it?
> If we support it now and current flow is just inefficient, I'd say keeping but
> refactoring it later.

Let me check this, but I guess it missed to handle large page in lots of flows,
not sure where we may panic at... :(

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>>>
>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>> ---
>>>>    fs/f2fs/data.c | 171 +------------------------------------------------
>>>>    1 file changed, 3 insertions(+), 168 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>> index 3058c7e28b11..9c23fde93b76 100644
>>>> --- a/fs/f2fs/data.c
>>>> +++ b/fs/f2fs/data.c
>>>> @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
>>>>    #endif
>>>>    #ifdef CONFIG_SWAP
>>>> -static int f2fs_is_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 nr_pblocks;
>>>> -	unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
>>>> -	unsigned int not_aligned = 0;
>>>> -	int ret = 0;
>>>> -
>>>> -	cur_lblock = 0;
>>>> -	last_lblock = bytes_to_blks(inode, i_size_read(inode));
>>>> -
>>>> -	while (cur_lblock < last_lblock) {
>>>> -		struct f2fs_map_blocks map;
>>>> -
>>>> -		memset(&map, 0, sizeof(map));
>>>> -		map.m_lblk = cur_lblock;
>>>> -		map.m_len = last_lblock - cur_lblock;
>>>> -		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);
>>>> -		if (ret)
>>>> -			goto out;
>>>> -
>>>> -		/* hole */
>>>> -		if (!(map.m_flags & F2FS_MAP_FLAGS)) {
>>>> -			f2fs_err(sbi, "Swapfile has holes\n");
>>>> -			ret = -ENOENT;
>>>> -			goto out;
>>>> -		}
>>>> -
>>>> -		pblock = map.m_pblk;
>>>> -		nr_pblocks = map.m_len;
>>>> -
>>>> -		if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
>>>> -			nr_pblocks & (blocks_per_sec - 1)) {
>>>> -			if (f2fs_is_pinned_file(inode)) {
>>>> -				f2fs_err(sbi, "Swapfile does not align to section");
>>>> -				ret = -EINVAL;
>>>> -				goto out;
>>>> -			}
>>>> -			not_aligned++;
>>>> -		}
>>>> -
>>>> -		cur_lblock += nr_pblocks;
>>>> -	}
>>>> -	if (not_aligned)
>>>> -		f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
>>>> -			"\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
>>>> -			not_aligned);
>>>> -out:
>>>> -	return ret;
>>>> -}
>>>> -
>>>> -static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>> +static int check_swap_activate(struct swap_info_struct *sis,
>>>>    				struct file *swap_file, sector_t *span)
>>>>    {
>>>>    	struct address_space *mapping = swap_file->f_mapping;
>>>> @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>    	unsigned int not_aligned = 0;
>>>>    	int ret = 0;
>>>> +	f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
>>>> +
>>>>    	/*
>>>>    	 * Map all the blocks into the extent list.  This code doesn't try
>>>>    	 * to be very smart.
>>>> @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>    	return ret;
>>>>    }
>>>> -/* Copied from generic_swapfile_activate() to check any holes */
>>>> -static int check_swap_activate(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);
>>>> -	unsigned blocks_per_page;
>>>> -	unsigned long page_no;
>>>> -	sector_t probe_block;
>>>> -	sector_t last_block;
>>>> -	sector_t lowest_block = -1;
>>>> -	sector_t highest_block = 0;
>>>> -	int nr_extents = 0;
>>>> -	int ret = 0;
>>>> -
>>>> -	if (PAGE_SIZE == F2FS_BLKSIZE)
>>>> -		return check_swap_activate_fast(sis, swap_file, span);
>>>> -
>>>> -	ret = f2fs_is_file_aligned(inode);
>>>> -	if (ret)
>>>> -		goto out;
>>>> -
>>>> -	blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
>>>> -
>>>> -	/*
>>>> -	 * Map all the blocks into the extent list.  This code doesn't try
>>>> -	 * to be very smart.
>>>> -	 */
>>>> -	probe_block = 0;
>>>> -	page_no = 0;
>>>> -	last_block = bytes_to_blks(inode, i_size_read(inode));
>>>> -	while ((probe_block + blocks_per_page) <= last_block &&
>>>> -			page_no < sis->max) {
>>>> -		unsigned block_in_page;
>>>> -		sector_t first_block;
>>>> -		sector_t block = 0;
>>>> -
>>>> -		cond_resched();
>>>> -
>>>> -		block = probe_block;
>>>> -		ret = bmap(inode, &block);
>>>> -		if (ret)
>>>> -			goto out;
>>>> -		if (!block)
>>>> -			goto bad_bmap;
>>>> -		first_block = block;
>>>> -
>>>> -		/*
>>>> -		 * It must be PAGE_SIZE aligned on-disk
>>>> -		 */
>>>> -		if (first_block & (blocks_per_page - 1)) {
>>>> -			probe_block++;
>>>> -			goto reprobe;
>>>> -		}
>>>> -
>>>> -		for (block_in_page = 1; block_in_page < blocks_per_page;
>>>> -					block_in_page++) {
>>>> -
>>>> -			block = probe_block + block_in_page;
>>>> -			ret = bmap(inode, &block);
>>>> -			if (ret)
>>>> -				goto out;
>>>> -			if (!block)
>>>> -				goto bad_bmap;
>>>> -
>>>> -			if (block != first_block + block_in_page) {
>>>> -				/* Discontiguity */
>>>> -				probe_block++;
>>>> -				goto reprobe;
>>>> -			}
>>>> -		}
>>>> -
>>>> -		first_block >>= (PAGE_SHIFT - inode->i_blkbits);
>>>> -		if (page_no) {	/* exclude the header page */
>>>> -			if (first_block < lowest_block)
>>>> -				lowest_block = first_block;
>>>> -			if (first_block > highest_block)
>>>> -				highest_block = first_block;
>>>> -		}
>>>> -
>>>> -		/*
>>>> -		 * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
>>>> -		 */
>>>> -		ret = add_swap_extent(sis, page_no, 1, first_block);
>>>> -		if (ret < 0)
>>>> -			goto out;
>>>> -		nr_extents += ret;
>>>> -		page_no++;
>>>> -		probe_block += blocks_per_page;
>>>> -reprobe:
>>>> -		continue;
>>>> -	}
>>>> -	ret = nr_extents;
>>>> -	*span = 1 + highest_block - lowest_block;
>>>> -	if (page_no == 0)
>>>> -		page_no = 1;	/* force Empty message */
>>>> -	sis->max = page_no;
>>>> -	sis->pages = page_no - 1;
>>>> -	sis->highest_bit = page_no - 1;
>>>> -out:
>>>> -	return ret;
>>>> -bad_bmap:
>>>> -	f2fs_err(sbi, "Swapfile has holes\n");
>>>> -	return -EINVAL;
>>>> -}
>>>> -
>>>>    static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
>>>>    				sector_t *span)
>>>>    {
>>>> -- 
>>>> 2.29.2
> .
> 

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

* Re: [f2fs-dev] [PATCH 1/2] f2fs: swap: remove dead codes
@ 2021-05-27  2:12         ` Chao Yu
  0 siblings, 0 replies; 22+ messages in thread
From: Chao Yu @ 2021-05-27  2:12 UTC (permalink / raw)
  To: Jaegeuk Kim, Chao Yu; +Cc: linux-kernel, linux-f2fs-devel

On 2021/5/27 9:40, Jaegeuk Kim wrote:
> On 05/26, Chao Yu wrote:
>> On 2021/5/26 21:30, Jaegeuk Kim wrote:
>>> On 05/26, Chao Yu wrote:
>>>> After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
>>>> we will never run into original logic of check_swap_activate() before
>>>> f2fs supports non 4k-sized page, so let's delete those dead codes.
>>>
>>> Why not keeping this for large page support in future maybe?
>>
>> Well, if so, at that time, it would be better to refactor
>> check_swap_activate_fast() implementation based on f2fs_map_block() rather
>> than refactoring check_swap_activate() implementation based on low efficient
>> bmap()?
> 
> Let's first check whether we support large page. Have you quickly tested it?
> If we support it now and current flow is just inefficient, I'd say keeping but
> refactoring it later.

Let me check this, but I guess it missed to handle large page in lots of flows,
not sure where we may panic at... :(

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>>>
>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>> ---
>>>>    fs/f2fs/data.c | 171 +------------------------------------------------
>>>>    1 file changed, 3 insertions(+), 168 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>> index 3058c7e28b11..9c23fde93b76 100644
>>>> --- a/fs/f2fs/data.c
>>>> +++ b/fs/f2fs/data.c
>>>> @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
>>>>    #endif
>>>>    #ifdef CONFIG_SWAP
>>>> -static int f2fs_is_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 nr_pblocks;
>>>> -	unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
>>>> -	unsigned int not_aligned = 0;
>>>> -	int ret = 0;
>>>> -
>>>> -	cur_lblock = 0;
>>>> -	last_lblock = bytes_to_blks(inode, i_size_read(inode));
>>>> -
>>>> -	while (cur_lblock < last_lblock) {
>>>> -		struct f2fs_map_blocks map;
>>>> -
>>>> -		memset(&map, 0, sizeof(map));
>>>> -		map.m_lblk = cur_lblock;
>>>> -		map.m_len = last_lblock - cur_lblock;
>>>> -		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);
>>>> -		if (ret)
>>>> -			goto out;
>>>> -
>>>> -		/* hole */
>>>> -		if (!(map.m_flags & F2FS_MAP_FLAGS)) {
>>>> -			f2fs_err(sbi, "Swapfile has holes\n");
>>>> -			ret = -ENOENT;
>>>> -			goto out;
>>>> -		}
>>>> -
>>>> -		pblock = map.m_pblk;
>>>> -		nr_pblocks = map.m_len;
>>>> -
>>>> -		if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
>>>> -			nr_pblocks & (blocks_per_sec - 1)) {
>>>> -			if (f2fs_is_pinned_file(inode)) {
>>>> -				f2fs_err(sbi, "Swapfile does not align to section");
>>>> -				ret = -EINVAL;
>>>> -				goto out;
>>>> -			}
>>>> -			not_aligned++;
>>>> -		}
>>>> -
>>>> -		cur_lblock += nr_pblocks;
>>>> -	}
>>>> -	if (not_aligned)
>>>> -		f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
>>>> -			"\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
>>>> -			not_aligned);
>>>> -out:
>>>> -	return ret;
>>>> -}
>>>> -
>>>> -static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>> +static int check_swap_activate(struct swap_info_struct *sis,
>>>>    				struct file *swap_file, sector_t *span)
>>>>    {
>>>>    	struct address_space *mapping = swap_file->f_mapping;
>>>> @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>    	unsigned int not_aligned = 0;
>>>>    	int ret = 0;
>>>> +	f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
>>>> +
>>>>    	/*
>>>>    	 * Map all the blocks into the extent list.  This code doesn't try
>>>>    	 * to be very smart.
>>>> @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>    	return ret;
>>>>    }
>>>> -/* Copied from generic_swapfile_activate() to check any holes */
>>>> -static int check_swap_activate(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);
>>>> -	unsigned blocks_per_page;
>>>> -	unsigned long page_no;
>>>> -	sector_t probe_block;
>>>> -	sector_t last_block;
>>>> -	sector_t lowest_block = -1;
>>>> -	sector_t highest_block = 0;
>>>> -	int nr_extents = 0;
>>>> -	int ret = 0;
>>>> -
>>>> -	if (PAGE_SIZE == F2FS_BLKSIZE)
>>>> -		return check_swap_activate_fast(sis, swap_file, span);
>>>> -
>>>> -	ret = f2fs_is_file_aligned(inode);
>>>> -	if (ret)
>>>> -		goto out;
>>>> -
>>>> -	blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
>>>> -
>>>> -	/*
>>>> -	 * Map all the blocks into the extent list.  This code doesn't try
>>>> -	 * to be very smart.
>>>> -	 */
>>>> -	probe_block = 0;
>>>> -	page_no = 0;
>>>> -	last_block = bytes_to_blks(inode, i_size_read(inode));
>>>> -	while ((probe_block + blocks_per_page) <= last_block &&
>>>> -			page_no < sis->max) {
>>>> -		unsigned block_in_page;
>>>> -		sector_t first_block;
>>>> -		sector_t block = 0;
>>>> -
>>>> -		cond_resched();
>>>> -
>>>> -		block = probe_block;
>>>> -		ret = bmap(inode, &block);
>>>> -		if (ret)
>>>> -			goto out;
>>>> -		if (!block)
>>>> -			goto bad_bmap;
>>>> -		first_block = block;
>>>> -
>>>> -		/*
>>>> -		 * It must be PAGE_SIZE aligned on-disk
>>>> -		 */
>>>> -		if (first_block & (blocks_per_page - 1)) {
>>>> -			probe_block++;
>>>> -			goto reprobe;
>>>> -		}
>>>> -
>>>> -		for (block_in_page = 1; block_in_page < blocks_per_page;
>>>> -					block_in_page++) {
>>>> -
>>>> -			block = probe_block + block_in_page;
>>>> -			ret = bmap(inode, &block);
>>>> -			if (ret)
>>>> -				goto out;
>>>> -			if (!block)
>>>> -				goto bad_bmap;
>>>> -
>>>> -			if (block != first_block + block_in_page) {
>>>> -				/* Discontiguity */
>>>> -				probe_block++;
>>>> -				goto reprobe;
>>>> -			}
>>>> -		}
>>>> -
>>>> -		first_block >>= (PAGE_SHIFT - inode->i_blkbits);
>>>> -		if (page_no) {	/* exclude the header page */
>>>> -			if (first_block < lowest_block)
>>>> -				lowest_block = first_block;
>>>> -			if (first_block > highest_block)
>>>> -				highest_block = first_block;
>>>> -		}
>>>> -
>>>> -		/*
>>>> -		 * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
>>>> -		 */
>>>> -		ret = add_swap_extent(sis, page_no, 1, first_block);
>>>> -		if (ret < 0)
>>>> -			goto out;
>>>> -		nr_extents += ret;
>>>> -		page_no++;
>>>> -		probe_block += blocks_per_page;
>>>> -reprobe:
>>>> -		continue;
>>>> -	}
>>>> -	ret = nr_extents;
>>>> -	*span = 1 + highest_block - lowest_block;
>>>> -	if (page_no == 0)
>>>> -		page_no = 1;	/* force Empty message */
>>>> -	sis->max = page_no;
>>>> -	sis->pages = page_no - 1;
>>>> -	sis->highest_bit = page_no - 1;
>>>> -out:
>>>> -	return ret;
>>>> -bad_bmap:
>>>> -	f2fs_err(sbi, "Swapfile has holes\n");
>>>> -	return -EINVAL;
>>>> -}
>>>> -
>>>>    static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
>>>>    				sector_t *span)
>>>>    {
>>>> -- 
>>>> 2.29.2
> .
> 


_______________________________________________
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] 22+ messages in thread

* Re: [PATCH 1/2] f2fs: swap: remove dead codes
  2021-05-27  2:12         ` [f2fs-dev] " Chao Yu
@ 2021-06-04 11:12           ` Chao Yu
  -1 siblings, 0 replies; 22+ messages in thread
From: Chao Yu @ 2021-06-04 11:12 UTC (permalink / raw)
  To: Chao Yu, Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel

On 2021/5/27 10:12, Chao Yu wrote:
> On 2021/5/27 9:40, Jaegeuk Kim wrote:
>> On 05/26, Chao Yu wrote:
>>> On 2021/5/26 21:30, Jaegeuk Kim wrote:
>>>> On 05/26, Chao Yu wrote:
>>>>> After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
>>>>> we will never run into original logic of check_swap_activate() before
>>>>> f2fs supports non 4k-sized page, so let's delete those dead codes.
>>>>
>>>> Why not keeping this for large page support in future maybe?
>>>
>>> Well, if so, at that time, it would be better to refactor
>>> check_swap_activate_fast() implementation based on f2fs_map_block() rather
>>> than refactoring check_swap_activate() implementation based on low efficient
>>> bmap()?
>>
>> Let's first check whether we support large page. Have you quickly tested it?
>> If we support it now and current flow is just inefficient, I'd say keeping but
>> refactoring it later.
> 
> Let me check this, but I guess it missed to handle large page in lots of flows,
> not sure where we may panic at... :(

I've checked f2fs behavior on 64kb page size in arm64 vm,
- 'ls -ls' will cause f2fs printing "invalid namelen(0), ino:%u, run fsck
to fix." each time.
- after touch a file and umount, fsck reports that image is corrupted.

F2FS uses PAGE_SIZE and F2FS_BLKSIZE directly in a lot of places, I doubt
most of places may not consider large page size condition.

So, thoughts? I guess we have to admit we do not support 64KB page right
now... :P

Thanks,

> 
> Thanks,
> 
>>
>>>
>>> Thanks,
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>> ---
>>>>>    fs/f2fs/data.c | 171 +------------------------------------------------
>>>>>    1 file changed, 3 insertions(+), 168 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>>> index 3058c7e28b11..9c23fde93b76 100644
>>>>> --- a/fs/f2fs/data.c
>>>>> +++ b/fs/f2fs/data.c
>>>>> @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
>>>>>    #endif
>>>>>    #ifdef CONFIG_SWAP
>>>>> -static int f2fs_is_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 nr_pblocks;
>>>>> -    unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
>>>>> -    unsigned int not_aligned = 0;
>>>>> -    int ret = 0;
>>>>> -
>>>>> -    cur_lblock = 0;
>>>>> -    last_lblock = bytes_to_blks(inode, i_size_read(inode));
>>>>> -
>>>>> -    while (cur_lblock < last_lblock) {
>>>>> -        struct f2fs_map_blocks map;
>>>>> -
>>>>> -        memset(&map, 0, sizeof(map));
>>>>> -        map.m_lblk = cur_lblock;
>>>>> -        map.m_len = last_lblock - cur_lblock;
>>>>> -        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);
>>>>> -        if (ret)
>>>>> -            goto out;
>>>>> -
>>>>> -        /* hole */
>>>>> -        if (!(map.m_flags & F2FS_MAP_FLAGS)) {
>>>>> -            f2fs_err(sbi, "Swapfile has holes\n");
>>>>> -            ret = -ENOENT;
>>>>> -            goto out;
>>>>> -        }
>>>>> -
>>>>> -        pblock = map.m_pblk;
>>>>> -        nr_pblocks = map.m_len;
>>>>> -
>>>>> -        if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
>>>>> -            nr_pblocks & (blocks_per_sec - 1)) {
>>>>> -            if (f2fs_is_pinned_file(inode)) {
>>>>> -                f2fs_err(sbi, "Swapfile does not align to section");
>>>>> -                ret = -EINVAL;
>>>>> -                goto out;
>>>>> -            }
>>>>> -            not_aligned++;
>>>>> -        }
>>>>> -
>>>>> -        cur_lblock += nr_pblocks;
>>>>> -    }
>>>>> -    if (not_aligned)
>>>>> -        f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
>>>>> -            "\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
>>>>> -            not_aligned);
>>>>> -out:
>>>>> -    return ret;
>>>>> -}
>>>>> -
>>>>> -static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>> +static int check_swap_activate(struct swap_info_struct *sis,
>>>>>                    struct file *swap_file, sector_t *span)
>>>>>    {
>>>>>        struct address_space *mapping = swap_file->f_mapping;
>>>>> @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>>        unsigned int not_aligned = 0;
>>>>>        int ret = 0;
>>>>> +    f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
>>>>> +
>>>>>        /*
>>>>>         * Map all the blocks into the extent list.  This code doesn't try
>>>>>         * to be very smart.
>>>>> @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>>        return ret;
>>>>>    }
>>>>> -/* Copied from generic_swapfile_activate() to check any holes */
>>>>> -static int check_swap_activate(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);
>>>>> -    unsigned blocks_per_page;
>>>>> -    unsigned long page_no;
>>>>> -    sector_t probe_block;
>>>>> -    sector_t last_block;
>>>>> -    sector_t lowest_block = -1;
>>>>> -    sector_t highest_block = 0;
>>>>> -    int nr_extents = 0;
>>>>> -    int ret = 0;
>>>>> -
>>>>> -    if (PAGE_SIZE == F2FS_BLKSIZE)
>>>>> -        return check_swap_activate_fast(sis, swap_file, span);
>>>>> -
>>>>> -    ret = f2fs_is_file_aligned(inode);
>>>>> -    if (ret)
>>>>> -        goto out;
>>>>> -
>>>>> -    blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
>>>>> -
>>>>> -    /*
>>>>> -     * Map all the blocks into the extent list.  This code doesn't try
>>>>> -     * to be very smart.
>>>>> -     */
>>>>> -    probe_block = 0;
>>>>> -    page_no = 0;
>>>>> -    last_block = bytes_to_blks(inode, i_size_read(inode));
>>>>> -    while ((probe_block + blocks_per_page) <= last_block &&
>>>>> -            page_no < sis->max) {
>>>>> -        unsigned block_in_page;
>>>>> -        sector_t first_block;
>>>>> -        sector_t block = 0;
>>>>> -
>>>>> -        cond_resched();
>>>>> -
>>>>> -        block = probe_block;
>>>>> -        ret = bmap(inode, &block);
>>>>> -        if (ret)
>>>>> -            goto out;
>>>>> -        if (!block)
>>>>> -            goto bad_bmap;
>>>>> -        first_block = block;
>>>>> -
>>>>> -        /*
>>>>> -         * It must be PAGE_SIZE aligned on-disk
>>>>> -         */
>>>>> -        if (first_block & (blocks_per_page - 1)) {
>>>>> -            probe_block++;
>>>>> -            goto reprobe;
>>>>> -        }
>>>>> -
>>>>> -        for (block_in_page = 1; block_in_page < blocks_per_page;
>>>>> -                    block_in_page++) {
>>>>> -
>>>>> -            block = probe_block + block_in_page;
>>>>> -            ret = bmap(inode, &block);
>>>>> -            if (ret)
>>>>> -                goto out;
>>>>> -            if (!block)
>>>>> -                goto bad_bmap;
>>>>> -
>>>>> -            if (block != first_block + block_in_page) {
>>>>> -                /* Discontiguity */
>>>>> -                probe_block++;
>>>>> -                goto reprobe;
>>>>> -            }
>>>>> -        }
>>>>> -
>>>>> -        first_block >>= (PAGE_SHIFT - inode->i_blkbits);
>>>>> -        if (page_no) {    /* exclude the header page */
>>>>> -            if (first_block < lowest_block)
>>>>> -                lowest_block = first_block;
>>>>> -            if (first_block > highest_block)
>>>>> -                highest_block = first_block;
>>>>> -        }
>>>>> -
>>>>> -        /*
>>>>> -         * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
>>>>> -         */
>>>>> -        ret = add_swap_extent(sis, page_no, 1, first_block);
>>>>> -        if (ret < 0)
>>>>> -            goto out;
>>>>> -        nr_extents += ret;
>>>>> -        page_no++;
>>>>> -        probe_block += blocks_per_page;
>>>>> -reprobe:
>>>>> -        continue;
>>>>> -    }
>>>>> -    ret = nr_extents;
>>>>> -    *span = 1 + highest_block - lowest_block;
>>>>> -    if (page_no == 0)
>>>>> -        page_no = 1;    /* force Empty message */
>>>>> -    sis->max = page_no;
>>>>> -    sis->pages = page_no - 1;
>>>>> -    sis->highest_bit = page_no - 1;
>>>>> -out:
>>>>> -    return ret;
>>>>> -bad_bmap:
>>>>> -    f2fs_err(sbi, "Swapfile has holes\n");
>>>>> -    return -EINVAL;
>>>>> -}
>>>>> -
>>>>>    static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
>>>>>                    sector_t *span)
>>>>>    {
>>>>> -- 
>>>>> 2.29.2
>> .
>>

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

* Re: [f2fs-dev] [PATCH 1/2] f2fs: swap: remove dead codes
@ 2021-06-04 11:12           ` Chao Yu
  0 siblings, 0 replies; 22+ messages in thread
From: Chao Yu @ 2021-06-04 11:12 UTC (permalink / raw)
  To: Chao Yu, Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel

On 2021/5/27 10:12, Chao Yu wrote:
> On 2021/5/27 9:40, Jaegeuk Kim wrote:
>> On 05/26, Chao Yu wrote:
>>> On 2021/5/26 21:30, Jaegeuk Kim wrote:
>>>> On 05/26, Chao Yu wrote:
>>>>> After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
>>>>> we will never run into original logic of check_swap_activate() before
>>>>> f2fs supports non 4k-sized page, so let's delete those dead codes.
>>>>
>>>> Why not keeping this for large page support in future maybe?
>>>
>>> Well, if so, at that time, it would be better to refactor
>>> check_swap_activate_fast() implementation based on f2fs_map_block() rather
>>> than refactoring check_swap_activate() implementation based on low efficient
>>> bmap()?
>>
>> Let's first check whether we support large page. Have you quickly tested it?
>> If we support it now and current flow is just inefficient, I'd say keeping but
>> refactoring it later.
> 
> Let me check this, but I guess it missed to handle large page in lots of flows,
> not sure where we may panic at... :(

I've checked f2fs behavior on 64kb page size in arm64 vm,
- 'ls -ls' will cause f2fs printing "invalid namelen(0), ino:%u, run fsck
to fix." each time.
- after touch a file and umount, fsck reports that image is corrupted.

F2FS uses PAGE_SIZE and F2FS_BLKSIZE directly in a lot of places, I doubt
most of places may not consider large page size condition.

So, thoughts? I guess we have to admit we do not support 64KB page right
now... :P

Thanks,

> 
> Thanks,
> 
>>
>>>
>>> Thanks,
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>> ---
>>>>>    fs/f2fs/data.c | 171 +------------------------------------------------
>>>>>    1 file changed, 3 insertions(+), 168 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>>> index 3058c7e28b11..9c23fde93b76 100644
>>>>> --- a/fs/f2fs/data.c
>>>>> +++ b/fs/f2fs/data.c
>>>>> @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
>>>>>    #endif
>>>>>    #ifdef CONFIG_SWAP
>>>>> -static int f2fs_is_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 nr_pblocks;
>>>>> -    unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
>>>>> -    unsigned int not_aligned = 0;
>>>>> -    int ret = 0;
>>>>> -
>>>>> -    cur_lblock = 0;
>>>>> -    last_lblock = bytes_to_blks(inode, i_size_read(inode));
>>>>> -
>>>>> -    while (cur_lblock < last_lblock) {
>>>>> -        struct f2fs_map_blocks map;
>>>>> -
>>>>> -        memset(&map, 0, sizeof(map));
>>>>> -        map.m_lblk = cur_lblock;
>>>>> -        map.m_len = last_lblock - cur_lblock;
>>>>> -        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);
>>>>> -        if (ret)
>>>>> -            goto out;
>>>>> -
>>>>> -        /* hole */
>>>>> -        if (!(map.m_flags & F2FS_MAP_FLAGS)) {
>>>>> -            f2fs_err(sbi, "Swapfile has holes\n");
>>>>> -            ret = -ENOENT;
>>>>> -            goto out;
>>>>> -        }
>>>>> -
>>>>> -        pblock = map.m_pblk;
>>>>> -        nr_pblocks = map.m_len;
>>>>> -
>>>>> -        if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
>>>>> -            nr_pblocks & (blocks_per_sec - 1)) {
>>>>> -            if (f2fs_is_pinned_file(inode)) {
>>>>> -                f2fs_err(sbi, "Swapfile does not align to section");
>>>>> -                ret = -EINVAL;
>>>>> -                goto out;
>>>>> -            }
>>>>> -            not_aligned++;
>>>>> -        }
>>>>> -
>>>>> -        cur_lblock += nr_pblocks;
>>>>> -    }
>>>>> -    if (not_aligned)
>>>>> -        f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
>>>>> -            "\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
>>>>> -            not_aligned);
>>>>> -out:
>>>>> -    return ret;
>>>>> -}
>>>>> -
>>>>> -static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>> +static int check_swap_activate(struct swap_info_struct *sis,
>>>>>                    struct file *swap_file, sector_t *span)
>>>>>    {
>>>>>        struct address_space *mapping = swap_file->f_mapping;
>>>>> @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>>        unsigned int not_aligned = 0;
>>>>>        int ret = 0;
>>>>> +    f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
>>>>> +
>>>>>        /*
>>>>>         * Map all the blocks into the extent list.  This code doesn't try
>>>>>         * to be very smart.
>>>>> @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>>        return ret;
>>>>>    }
>>>>> -/* Copied from generic_swapfile_activate() to check any holes */
>>>>> -static int check_swap_activate(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);
>>>>> -    unsigned blocks_per_page;
>>>>> -    unsigned long page_no;
>>>>> -    sector_t probe_block;
>>>>> -    sector_t last_block;
>>>>> -    sector_t lowest_block = -1;
>>>>> -    sector_t highest_block = 0;
>>>>> -    int nr_extents = 0;
>>>>> -    int ret = 0;
>>>>> -
>>>>> -    if (PAGE_SIZE == F2FS_BLKSIZE)
>>>>> -        return check_swap_activate_fast(sis, swap_file, span);
>>>>> -
>>>>> -    ret = f2fs_is_file_aligned(inode);
>>>>> -    if (ret)
>>>>> -        goto out;
>>>>> -
>>>>> -    blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
>>>>> -
>>>>> -    /*
>>>>> -     * Map all the blocks into the extent list.  This code doesn't try
>>>>> -     * to be very smart.
>>>>> -     */
>>>>> -    probe_block = 0;
>>>>> -    page_no = 0;
>>>>> -    last_block = bytes_to_blks(inode, i_size_read(inode));
>>>>> -    while ((probe_block + blocks_per_page) <= last_block &&
>>>>> -            page_no < sis->max) {
>>>>> -        unsigned block_in_page;
>>>>> -        sector_t first_block;
>>>>> -        sector_t block = 0;
>>>>> -
>>>>> -        cond_resched();
>>>>> -
>>>>> -        block = probe_block;
>>>>> -        ret = bmap(inode, &block);
>>>>> -        if (ret)
>>>>> -            goto out;
>>>>> -        if (!block)
>>>>> -            goto bad_bmap;
>>>>> -        first_block = block;
>>>>> -
>>>>> -        /*
>>>>> -         * It must be PAGE_SIZE aligned on-disk
>>>>> -         */
>>>>> -        if (first_block & (blocks_per_page - 1)) {
>>>>> -            probe_block++;
>>>>> -            goto reprobe;
>>>>> -        }
>>>>> -
>>>>> -        for (block_in_page = 1; block_in_page < blocks_per_page;
>>>>> -                    block_in_page++) {
>>>>> -
>>>>> -            block = probe_block + block_in_page;
>>>>> -            ret = bmap(inode, &block);
>>>>> -            if (ret)
>>>>> -                goto out;
>>>>> -            if (!block)
>>>>> -                goto bad_bmap;
>>>>> -
>>>>> -            if (block != first_block + block_in_page) {
>>>>> -                /* Discontiguity */
>>>>> -                probe_block++;
>>>>> -                goto reprobe;
>>>>> -            }
>>>>> -        }
>>>>> -
>>>>> -        first_block >>= (PAGE_SHIFT - inode->i_blkbits);
>>>>> -        if (page_no) {    /* exclude the header page */
>>>>> -            if (first_block < lowest_block)
>>>>> -                lowest_block = first_block;
>>>>> -            if (first_block > highest_block)
>>>>> -                highest_block = first_block;
>>>>> -        }
>>>>> -
>>>>> -        /*
>>>>> -         * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
>>>>> -         */
>>>>> -        ret = add_swap_extent(sis, page_no, 1, first_block);
>>>>> -        if (ret < 0)
>>>>> -            goto out;
>>>>> -        nr_extents += ret;
>>>>> -        page_no++;
>>>>> -        probe_block += blocks_per_page;
>>>>> -reprobe:
>>>>> -        continue;
>>>>> -    }
>>>>> -    ret = nr_extents;
>>>>> -    *span = 1 + highest_block - lowest_block;
>>>>> -    if (page_no == 0)
>>>>> -        page_no = 1;    /* force Empty message */
>>>>> -    sis->max = page_no;
>>>>> -    sis->pages = page_no - 1;
>>>>> -    sis->highest_bit = page_no - 1;
>>>>> -out:
>>>>> -    return ret;
>>>>> -bad_bmap:
>>>>> -    f2fs_err(sbi, "Swapfile has holes\n");
>>>>> -    return -EINVAL;
>>>>> -}
>>>>> -
>>>>>    static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
>>>>>                    sector_t *span)
>>>>>    {
>>>>> -- 
>>>>> 2.29.2
>> .
>>


_______________________________________________
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] 22+ messages in thread

* Re: [PATCH 1/2] f2fs: swap: remove dead codes
  2021-06-04 11:12           ` [f2fs-dev] " Chao Yu
@ 2021-06-04 23:51             ` Jaegeuk Kim
  -1 siblings, 0 replies; 22+ messages in thread
From: Jaegeuk Kim @ 2021-06-04 23:51 UTC (permalink / raw)
  To: Chao Yu; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel

On 06/04, Chao Yu wrote:
> On 2021/5/27 10:12, Chao Yu wrote:
> > On 2021/5/27 9:40, Jaegeuk Kim wrote:
> > > On 05/26, Chao Yu wrote:
> > > > On 2021/5/26 21:30, Jaegeuk Kim wrote:
> > > > > On 05/26, Chao Yu wrote:
> > > > > > After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
> > > > > > we will never run into original logic of check_swap_activate() before
> > > > > > f2fs supports non 4k-sized page, so let's delete those dead codes.
> > > > > 
> > > > > Why not keeping this for large page support in future maybe?
> > > > 
> > > > Well, if so, at that time, it would be better to refactor
> > > > check_swap_activate_fast() implementation based on f2fs_map_block() rather
> > > > than refactoring check_swap_activate() implementation based on low efficient
> > > > bmap()?
> > > 
> > > Let's first check whether we support large page. Have you quickly tested it?
> > > If we support it now and current flow is just inefficient, I'd say keeping but
> > > refactoring it later.
> > 
> > Let me check this, but I guess it missed to handle large page in lots of flows,
> > not sure where we may panic at... :(
> 
> I've checked f2fs behavior on 64kb page size in arm64 vm,
> - 'ls -ls' will cause f2fs printing "invalid namelen(0), ino:%u, run fsck
> to fix." each time.
> - after touch a file and umount, fsck reports that image is corrupted.
> 
> F2FS uses PAGE_SIZE and F2FS_BLKSIZE directly in a lot of places, I doubt
> most of places may not consider large page size condition.
> 
> So, thoughts? I guess we have to admit we do not support 64KB page right
> now... :P

Yeah, we need to block the large page support unfortunately. :(

> 
> Thanks,
> 
> > 
> > Thanks,
> > 
> > > 
> > > > 
> > > > Thanks,
> > > > 
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > > > > ---
> > > > > >    fs/f2fs/data.c | 171 +------------------------------------------------
> > > > > >    1 file changed, 3 insertions(+), 168 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > > > > index 3058c7e28b11..9c23fde93b76 100644
> > > > > > --- a/fs/f2fs/data.c
> > > > > > +++ b/fs/f2fs/data.c
> > > > > > @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
> > > > > >    #endif
> > > > > >    #ifdef CONFIG_SWAP
> > > > > > -static int f2fs_is_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 nr_pblocks;
> > > > > > -    unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
> > > > > > -    unsigned int not_aligned = 0;
> > > > > > -    int ret = 0;
> > > > > > -
> > > > > > -    cur_lblock = 0;
> > > > > > -    last_lblock = bytes_to_blks(inode, i_size_read(inode));
> > > > > > -
> > > > > > -    while (cur_lblock < last_lblock) {
> > > > > > -        struct f2fs_map_blocks map;
> > > > > > -
> > > > > > -        memset(&map, 0, sizeof(map));
> > > > > > -        map.m_lblk = cur_lblock;
> > > > > > -        map.m_len = last_lblock - cur_lblock;
> > > > > > -        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);
> > > > > > -        if (ret)
> > > > > > -            goto out;
> > > > > > -
> > > > > > -        /* hole */
> > > > > > -        if (!(map.m_flags & F2FS_MAP_FLAGS)) {
> > > > > > -            f2fs_err(sbi, "Swapfile has holes\n");
> > > > > > -            ret = -ENOENT;
> > > > > > -            goto out;
> > > > > > -        }
> > > > > > -
> > > > > > -        pblock = map.m_pblk;
> > > > > > -        nr_pblocks = map.m_len;
> > > > > > -
> > > > > > -        if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
> > > > > > -            nr_pblocks & (blocks_per_sec - 1)) {
> > > > > > -            if (f2fs_is_pinned_file(inode)) {
> > > > > > -                f2fs_err(sbi, "Swapfile does not align to section");
> > > > > > -                ret = -EINVAL;
> > > > > > -                goto out;
> > > > > > -            }
> > > > > > -            not_aligned++;
> > > > > > -        }
> > > > > > -
> > > > > > -        cur_lblock += nr_pblocks;
> > > > > > -    }
> > > > > > -    if (not_aligned)
> > > > > > -        f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
> > > > > > -            "\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
> > > > > > -            not_aligned);
> > > > > > -out:
> > > > > > -    return ret;
> > > > > > -}
> > > > > > -
> > > > > > -static int check_swap_activate_fast(struct swap_info_struct *sis,
> > > > > > +static int check_swap_activate(struct swap_info_struct *sis,
> > > > > >                    struct file *swap_file, sector_t *span)
> > > > > >    {
> > > > > >        struct address_space *mapping = swap_file->f_mapping;
> > > > > > @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
> > > > > >        unsigned int not_aligned = 0;
> > > > > >        int ret = 0;
> > > > > > +    f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
> > > > > > +
> > > > > >        /*
> > > > > >         * Map all the blocks into the extent list.  This code doesn't try
> > > > > >         * to be very smart.
> > > > > > @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
> > > > > >        return ret;
> > > > > >    }
> > > > > > -/* Copied from generic_swapfile_activate() to check any holes */
> > > > > > -static int check_swap_activate(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);
> > > > > > -    unsigned blocks_per_page;
> > > > > > -    unsigned long page_no;
> > > > > > -    sector_t probe_block;
> > > > > > -    sector_t last_block;
> > > > > > -    sector_t lowest_block = -1;
> > > > > > -    sector_t highest_block = 0;
> > > > > > -    int nr_extents = 0;
> > > > > > -    int ret = 0;
> > > > > > -
> > > > > > -    if (PAGE_SIZE == F2FS_BLKSIZE)
> > > > > > -        return check_swap_activate_fast(sis, swap_file, span);
> > > > > > -
> > > > > > -    ret = f2fs_is_file_aligned(inode);
> > > > > > -    if (ret)
> > > > > > -        goto out;
> > > > > > -
> > > > > > -    blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
> > > > > > -
> > > > > > -    /*
> > > > > > -     * Map all the blocks into the extent list.  This code doesn't try
> > > > > > -     * to be very smart.
> > > > > > -     */
> > > > > > -    probe_block = 0;
> > > > > > -    page_no = 0;
> > > > > > -    last_block = bytes_to_blks(inode, i_size_read(inode));
> > > > > > -    while ((probe_block + blocks_per_page) <= last_block &&
> > > > > > -            page_no < sis->max) {
> > > > > > -        unsigned block_in_page;
> > > > > > -        sector_t first_block;
> > > > > > -        sector_t block = 0;
> > > > > > -
> > > > > > -        cond_resched();
> > > > > > -
> > > > > > -        block = probe_block;
> > > > > > -        ret = bmap(inode, &block);
> > > > > > -        if (ret)
> > > > > > -            goto out;
> > > > > > -        if (!block)
> > > > > > -            goto bad_bmap;
> > > > > > -        first_block = block;
> > > > > > -
> > > > > > -        /*
> > > > > > -         * It must be PAGE_SIZE aligned on-disk
> > > > > > -         */
> > > > > > -        if (first_block & (blocks_per_page - 1)) {
> > > > > > -            probe_block++;
> > > > > > -            goto reprobe;
> > > > > > -        }
> > > > > > -
> > > > > > -        for (block_in_page = 1; block_in_page < blocks_per_page;
> > > > > > -                    block_in_page++) {
> > > > > > -
> > > > > > -            block = probe_block + block_in_page;
> > > > > > -            ret = bmap(inode, &block);
> > > > > > -            if (ret)
> > > > > > -                goto out;
> > > > > > -            if (!block)
> > > > > > -                goto bad_bmap;
> > > > > > -
> > > > > > -            if (block != first_block + block_in_page) {
> > > > > > -                /* Discontiguity */
> > > > > > -                probe_block++;
> > > > > > -                goto reprobe;
> > > > > > -            }
> > > > > > -        }
> > > > > > -
> > > > > > -        first_block >>= (PAGE_SHIFT - inode->i_blkbits);
> > > > > > -        if (page_no) {    /* exclude the header page */
> > > > > > -            if (first_block < lowest_block)
> > > > > > -                lowest_block = first_block;
> > > > > > -            if (first_block > highest_block)
> > > > > > -                highest_block = first_block;
> > > > > > -        }
> > > > > > -
> > > > > > -        /*
> > > > > > -         * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
> > > > > > -         */
> > > > > > -        ret = add_swap_extent(sis, page_no, 1, first_block);
> > > > > > -        if (ret < 0)
> > > > > > -            goto out;
> > > > > > -        nr_extents += ret;
> > > > > > -        page_no++;
> > > > > > -        probe_block += blocks_per_page;
> > > > > > -reprobe:
> > > > > > -        continue;
> > > > > > -    }
> > > > > > -    ret = nr_extents;
> > > > > > -    *span = 1 + highest_block - lowest_block;
> > > > > > -    if (page_no == 0)
> > > > > > -        page_no = 1;    /* force Empty message */
> > > > > > -    sis->max = page_no;
> > > > > > -    sis->pages = page_no - 1;
> > > > > > -    sis->highest_bit = page_no - 1;
> > > > > > -out:
> > > > > > -    return ret;
> > > > > > -bad_bmap:
> > > > > > -    f2fs_err(sbi, "Swapfile has holes\n");
> > > > > > -    return -EINVAL;
> > > > > > -}
> > > > > > -
> > > > > >    static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
> > > > > >                    sector_t *span)
> > > > > >    {
> > > > > > -- 
> > > > > > 2.29.2
> > > .
> > > 

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

* Re: [f2fs-dev] [PATCH 1/2] f2fs: swap: remove dead codes
@ 2021-06-04 23:51             ` Jaegeuk Kim
  0 siblings, 0 replies; 22+ messages in thread
From: Jaegeuk Kim @ 2021-06-04 23:51 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel

On 06/04, Chao Yu wrote:
> On 2021/5/27 10:12, Chao Yu wrote:
> > On 2021/5/27 9:40, Jaegeuk Kim wrote:
> > > On 05/26, Chao Yu wrote:
> > > > On 2021/5/26 21:30, Jaegeuk Kim wrote:
> > > > > On 05/26, Chao Yu wrote:
> > > > > > After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
> > > > > > we will never run into original logic of check_swap_activate() before
> > > > > > f2fs supports non 4k-sized page, so let's delete those dead codes.
> > > > > 
> > > > > Why not keeping this for large page support in future maybe?
> > > > 
> > > > Well, if so, at that time, it would be better to refactor
> > > > check_swap_activate_fast() implementation based on f2fs_map_block() rather
> > > > than refactoring check_swap_activate() implementation based on low efficient
> > > > bmap()?
> > > 
> > > Let's first check whether we support large page. Have you quickly tested it?
> > > If we support it now and current flow is just inefficient, I'd say keeping but
> > > refactoring it later.
> > 
> > Let me check this, but I guess it missed to handle large page in lots of flows,
> > not sure where we may panic at... :(
> 
> I've checked f2fs behavior on 64kb page size in arm64 vm,
> - 'ls -ls' will cause f2fs printing "invalid namelen(0), ino:%u, run fsck
> to fix." each time.
> - after touch a file and umount, fsck reports that image is corrupted.
> 
> F2FS uses PAGE_SIZE and F2FS_BLKSIZE directly in a lot of places, I doubt
> most of places may not consider large page size condition.
> 
> So, thoughts? I guess we have to admit we do not support 64KB page right
> now... :P

Yeah, we need to block the large page support unfortunately. :(

> 
> Thanks,
> 
> > 
> > Thanks,
> > 
> > > 
> > > > 
> > > > Thanks,
> > > > 
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > > > > ---
> > > > > >    fs/f2fs/data.c | 171 +------------------------------------------------
> > > > > >    1 file changed, 3 insertions(+), 168 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > > > > index 3058c7e28b11..9c23fde93b76 100644
> > > > > > --- a/fs/f2fs/data.c
> > > > > > +++ b/fs/f2fs/data.c
> > > > > > @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
> > > > > >    #endif
> > > > > >    #ifdef CONFIG_SWAP
> > > > > > -static int f2fs_is_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 nr_pblocks;
> > > > > > -    unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
> > > > > > -    unsigned int not_aligned = 0;
> > > > > > -    int ret = 0;
> > > > > > -
> > > > > > -    cur_lblock = 0;
> > > > > > -    last_lblock = bytes_to_blks(inode, i_size_read(inode));
> > > > > > -
> > > > > > -    while (cur_lblock < last_lblock) {
> > > > > > -        struct f2fs_map_blocks map;
> > > > > > -
> > > > > > -        memset(&map, 0, sizeof(map));
> > > > > > -        map.m_lblk = cur_lblock;
> > > > > > -        map.m_len = last_lblock - cur_lblock;
> > > > > > -        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);
> > > > > > -        if (ret)
> > > > > > -            goto out;
> > > > > > -
> > > > > > -        /* hole */
> > > > > > -        if (!(map.m_flags & F2FS_MAP_FLAGS)) {
> > > > > > -            f2fs_err(sbi, "Swapfile has holes\n");
> > > > > > -            ret = -ENOENT;
> > > > > > -            goto out;
> > > > > > -        }
> > > > > > -
> > > > > > -        pblock = map.m_pblk;
> > > > > > -        nr_pblocks = map.m_len;
> > > > > > -
> > > > > > -        if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
> > > > > > -            nr_pblocks & (blocks_per_sec - 1)) {
> > > > > > -            if (f2fs_is_pinned_file(inode)) {
> > > > > > -                f2fs_err(sbi, "Swapfile does not align to section");
> > > > > > -                ret = -EINVAL;
> > > > > > -                goto out;
> > > > > > -            }
> > > > > > -            not_aligned++;
> > > > > > -        }
> > > > > > -
> > > > > > -        cur_lblock += nr_pblocks;
> > > > > > -    }
> > > > > > -    if (not_aligned)
> > > > > > -        f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
> > > > > > -            "\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
> > > > > > -            not_aligned);
> > > > > > -out:
> > > > > > -    return ret;
> > > > > > -}
> > > > > > -
> > > > > > -static int check_swap_activate_fast(struct swap_info_struct *sis,
> > > > > > +static int check_swap_activate(struct swap_info_struct *sis,
> > > > > >                    struct file *swap_file, sector_t *span)
> > > > > >    {
> > > > > >        struct address_space *mapping = swap_file->f_mapping;
> > > > > > @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
> > > > > >        unsigned int not_aligned = 0;
> > > > > >        int ret = 0;
> > > > > > +    f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
> > > > > > +
> > > > > >        /*
> > > > > >         * Map all the blocks into the extent list.  This code doesn't try
> > > > > >         * to be very smart.
> > > > > > @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
> > > > > >        return ret;
> > > > > >    }
> > > > > > -/* Copied from generic_swapfile_activate() to check any holes */
> > > > > > -static int check_swap_activate(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);
> > > > > > -    unsigned blocks_per_page;
> > > > > > -    unsigned long page_no;
> > > > > > -    sector_t probe_block;
> > > > > > -    sector_t last_block;
> > > > > > -    sector_t lowest_block = -1;
> > > > > > -    sector_t highest_block = 0;
> > > > > > -    int nr_extents = 0;
> > > > > > -    int ret = 0;
> > > > > > -
> > > > > > -    if (PAGE_SIZE == F2FS_BLKSIZE)
> > > > > > -        return check_swap_activate_fast(sis, swap_file, span);
> > > > > > -
> > > > > > -    ret = f2fs_is_file_aligned(inode);
> > > > > > -    if (ret)
> > > > > > -        goto out;
> > > > > > -
> > > > > > -    blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
> > > > > > -
> > > > > > -    /*
> > > > > > -     * Map all the blocks into the extent list.  This code doesn't try
> > > > > > -     * to be very smart.
> > > > > > -     */
> > > > > > -    probe_block = 0;
> > > > > > -    page_no = 0;
> > > > > > -    last_block = bytes_to_blks(inode, i_size_read(inode));
> > > > > > -    while ((probe_block + blocks_per_page) <= last_block &&
> > > > > > -            page_no < sis->max) {
> > > > > > -        unsigned block_in_page;
> > > > > > -        sector_t first_block;
> > > > > > -        sector_t block = 0;
> > > > > > -
> > > > > > -        cond_resched();
> > > > > > -
> > > > > > -        block = probe_block;
> > > > > > -        ret = bmap(inode, &block);
> > > > > > -        if (ret)
> > > > > > -            goto out;
> > > > > > -        if (!block)
> > > > > > -            goto bad_bmap;
> > > > > > -        first_block = block;
> > > > > > -
> > > > > > -        /*
> > > > > > -         * It must be PAGE_SIZE aligned on-disk
> > > > > > -         */
> > > > > > -        if (first_block & (blocks_per_page - 1)) {
> > > > > > -            probe_block++;
> > > > > > -            goto reprobe;
> > > > > > -        }
> > > > > > -
> > > > > > -        for (block_in_page = 1; block_in_page < blocks_per_page;
> > > > > > -                    block_in_page++) {
> > > > > > -
> > > > > > -            block = probe_block + block_in_page;
> > > > > > -            ret = bmap(inode, &block);
> > > > > > -            if (ret)
> > > > > > -                goto out;
> > > > > > -            if (!block)
> > > > > > -                goto bad_bmap;
> > > > > > -
> > > > > > -            if (block != first_block + block_in_page) {
> > > > > > -                /* Discontiguity */
> > > > > > -                probe_block++;
> > > > > > -                goto reprobe;
> > > > > > -            }
> > > > > > -        }
> > > > > > -
> > > > > > -        first_block >>= (PAGE_SHIFT - inode->i_blkbits);
> > > > > > -        if (page_no) {    /* exclude the header page */
> > > > > > -            if (first_block < lowest_block)
> > > > > > -                lowest_block = first_block;
> > > > > > -            if (first_block > highest_block)
> > > > > > -                highest_block = first_block;
> > > > > > -        }
> > > > > > -
> > > > > > -        /*
> > > > > > -         * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
> > > > > > -         */
> > > > > > -        ret = add_swap_extent(sis, page_no, 1, first_block);
> > > > > > -        if (ret < 0)
> > > > > > -            goto out;
> > > > > > -        nr_extents += ret;
> > > > > > -        page_no++;
> > > > > > -        probe_block += blocks_per_page;
> > > > > > -reprobe:
> > > > > > -        continue;
> > > > > > -    }
> > > > > > -    ret = nr_extents;
> > > > > > -    *span = 1 + highest_block - lowest_block;
> > > > > > -    if (page_no == 0)
> > > > > > -        page_no = 1;    /* force Empty message */
> > > > > > -    sis->max = page_no;
> > > > > > -    sis->pages = page_no - 1;
> > > > > > -    sis->highest_bit = page_no - 1;
> > > > > > -out:
> > > > > > -    return ret;
> > > > > > -bad_bmap:
> > > > > > -    f2fs_err(sbi, "Swapfile has holes\n");
> > > > > > -    return -EINVAL;
> > > > > > -}
> > > > > > -
> > > > > >    static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
> > > > > >                    sector_t *span)
> > > > > >    {
> > > > > > -- 
> > > > > > 2.29.2
> > > .
> > > 


_______________________________________________
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] 22+ messages in thread

* Re: [PATCH 1/2] f2fs: swap: remove dead codes
  2021-06-04 23:51             ` [f2fs-dev] " Jaegeuk Kim
@ 2021-06-07 15:14               ` Chao Yu
  -1 siblings, 0 replies; 22+ messages in thread
From: Chao Yu @ 2021-06-07 15:14 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel

On 2021/6/5 7:51, Jaegeuk Kim wrote:
> On 06/04, Chao Yu wrote:
>> On 2021/5/27 10:12, Chao Yu wrote:
>>> On 2021/5/27 9:40, Jaegeuk Kim wrote:
>>>> On 05/26, Chao Yu wrote:
>>>>> On 2021/5/26 21:30, Jaegeuk Kim wrote:
>>>>>> On 05/26, Chao Yu wrote:
>>>>>>> After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
>>>>>>> we will never run into original logic of check_swap_activate() before
>>>>>>> f2fs supports non 4k-sized page, so let's delete those dead codes.
>>>>>>
>>>>>> Why not keeping this for large page support in future maybe?
>>>>>
>>>>> Well, if so, at that time, it would be better to refactor
>>>>> check_swap_activate_fast() implementation based on f2fs_map_block() rather
>>>>> than refactoring check_swap_activate() implementation based on low efficient
>>>>> bmap()?
>>>>
>>>> Let's first check whether we support large page. Have you quickly tested it?
>>>> If we support it now and current flow is just inefficient, I'd say keeping but
>>>> refactoring it later.
>>>
>>> Let me check this, but I guess it missed to handle large page in lots of flows,
>>> not sure where we may panic at... :(
>>
>> I've checked f2fs behavior on 64kb page size in arm64 vm,
>> - 'ls -ls' will cause f2fs printing "invalid namelen(0), ino:%u, run fsck
>> to fix." each time.
>> - after touch a file and umount, fsck reports that image is corrupted.
>>
>> F2FS uses PAGE_SIZE and F2FS_BLKSIZE directly in a lot of places, I doubt
>> most of places may not consider large page size condition.
>>
>> So, thoughts? I guess we have to admit we do not support 64KB page right
>> now... :P
> 
> Yeah, we need to block the large page support unfortunately. :(

Could you please queue this series in dev-test?

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>> Thanks,
>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>>>> ---
>>>>>>>     fs/f2fs/data.c | 171 +------------------------------------------------
>>>>>>>     1 file changed, 3 insertions(+), 168 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>>>>> index 3058c7e28b11..9c23fde93b76 100644
>>>>>>> --- a/fs/f2fs/data.c
>>>>>>> +++ b/fs/f2fs/data.c
>>>>>>> @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
>>>>>>>     #endif
>>>>>>>     #ifdef CONFIG_SWAP
>>>>>>> -static int f2fs_is_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 nr_pblocks;
>>>>>>> -    unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
>>>>>>> -    unsigned int not_aligned = 0;
>>>>>>> -    int ret = 0;
>>>>>>> -
>>>>>>> -    cur_lblock = 0;
>>>>>>> -    last_lblock = bytes_to_blks(inode, i_size_read(inode));
>>>>>>> -
>>>>>>> -    while (cur_lblock < last_lblock) {
>>>>>>> -        struct f2fs_map_blocks map;
>>>>>>> -
>>>>>>> -        memset(&map, 0, sizeof(map));
>>>>>>> -        map.m_lblk = cur_lblock;
>>>>>>> -        map.m_len = last_lblock - cur_lblock;
>>>>>>> -        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);
>>>>>>> -        if (ret)
>>>>>>> -            goto out;
>>>>>>> -
>>>>>>> -        /* hole */
>>>>>>> -        if (!(map.m_flags & F2FS_MAP_FLAGS)) {
>>>>>>> -            f2fs_err(sbi, "Swapfile has holes\n");
>>>>>>> -            ret = -ENOENT;
>>>>>>> -            goto out;
>>>>>>> -        }
>>>>>>> -
>>>>>>> -        pblock = map.m_pblk;
>>>>>>> -        nr_pblocks = map.m_len;
>>>>>>> -
>>>>>>> -        if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
>>>>>>> -            nr_pblocks & (blocks_per_sec - 1)) {
>>>>>>> -            if (f2fs_is_pinned_file(inode)) {
>>>>>>> -                f2fs_err(sbi, "Swapfile does not align to section");
>>>>>>> -                ret = -EINVAL;
>>>>>>> -                goto out;
>>>>>>> -            }
>>>>>>> -            not_aligned++;
>>>>>>> -        }
>>>>>>> -
>>>>>>> -        cur_lblock += nr_pblocks;
>>>>>>> -    }
>>>>>>> -    if (not_aligned)
>>>>>>> -        f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
>>>>>>> -            "\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
>>>>>>> -            not_aligned);
>>>>>>> -out:
>>>>>>> -    return ret;
>>>>>>> -}
>>>>>>> -
>>>>>>> -static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>>>> +static int check_swap_activate(struct swap_info_struct *sis,
>>>>>>>                     struct file *swap_file, sector_t *span)
>>>>>>>     {
>>>>>>>         struct address_space *mapping = swap_file->f_mapping;
>>>>>>> @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>>>>         unsigned int not_aligned = 0;
>>>>>>>         int ret = 0;
>>>>>>> +    f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
>>>>>>> +
>>>>>>>         /*
>>>>>>>          * Map all the blocks into the extent list.  This code doesn't try
>>>>>>>          * to be very smart.
>>>>>>> @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>>>>         return ret;
>>>>>>>     }
>>>>>>> -/* Copied from generic_swapfile_activate() to check any holes */
>>>>>>> -static int check_swap_activate(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);
>>>>>>> -    unsigned blocks_per_page;
>>>>>>> -    unsigned long page_no;
>>>>>>> -    sector_t probe_block;
>>>>>>> -    sector_t last_block;
>>>>>>> -    sector_t lowest_block = -1;
>>>>>>> -    sector_t highest_block = 0;
>>>>>>> -    int nr_extents = 0;
>>>>>>> -    int ret = 0;
>>>>>>> -
>>>>>>> -    if (PAGE_SIZE == F2FS_BLKSIZE)
>>>>>>> -        return check_swap_activate_fast(sis, swap_file, span);
>>>>>>> -
>>>>>>> -    ret = f2fs_is_file_aligned(inode);
>>>>>>> -    if (ret)
>>>>>>> -        goto out;
>>>>>>> -
>>>>>>> -    blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
>>>>>>> -
>>>>>>> -    /*
>>>>>>> -     * Map all the blocks into the extent list.  This code doesn't try
>>>>>>> -     * to be very smart.
>>>>>>> -     */
>>>>>>> -    probe_block = 0;
>>>>>>> -    page_no = 0;
>>>>>>> -    last_block = bytes_to_blks(inode, i_size_read(inode));
>>>>>>> -    while ((probe_block + blocks_per_page) <= last_block &&
>>>>>>> -            page_no < sis->max) {
>>>>>>> -        unsigned block_in_page;
>>>>>>> -        sector_t first_block;
>>>>>>> -        sector_t block = 0;
>>>>>>> -
>>>>>>> -        cond_resched();
>>>>>>> -
>>>>>>> -        block = probe_block;
>>>>>>> -        ret = bmap(inode, &block);
>>>>>>> -        if (ret)
>>>>>>> -            goto out;
>>>>>>> -        if (!block)
>>>>>>> -            goto bad_bmap;
>>>>>>> -        first_block = block;
>>>>>>> -
>>>>>>> -        /*
>>>>>>> -         * It must be PAGE_SIZE aligned on-disk
>>>>>>> -         */
>>>>>>> -        if (first_block & (blocks_per_page - 1)) {
>>>>>>> -            probe_block++;
>>>>>>> -            goto reprobe;
>>>>>>> -        }
>>>>>>> -
>>>>>>> -        for (block_in_page = 1; block_in_page < blocks_per_page;
>>>>>>> -                    block_in_page++) {
>>>>>>> -
>>>>>>> -            block = probe_block + block_in_page;
>>>>>>> -            ret = bmap(inode, &block);
>>>>>>> -            if (ret)
>>>>>>> -                goto out;
>>>>>>> -            if (!block)
>>>>>>> -                goto bad_bmap;
>>>>>>> -
>>>>>>> -            if (block != first_block + block_in_page) {
>>>>>>> -                /* Discontiguity */
>>>>>>> -                probe_block++;
>>>>>>> -                goto reprobe;
>>>>>>> -            }
>>>>>>> -        }
>>>>>>> -
>>>>>>> -        first_block >>= (PAGE_SHIFT - inode->i_blkbits);
>>>>>>> -        if (page_no) {    /* exclude the header page */
>>>>>>> -            if (first_block < lowest_block)
>>>>>>> -                lowest_block = first_block;
>>>>>>> -            if (first_block > highest_block)
>>>>>>> -                highest_block = first_block;
>>>>>>> -        }
>>>>>>> -
>>>>>>> -        /*
>>>>>>> -         * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
>>>>>>> -         */
>>>>>>> -        ret = add_swap_extent(sis, page_no, 1, first_block);
>>>>>>> -        if (ret < 0)
>>>>>>> -            goto out;
>>>>>>> -        nr_extents += ret;
>>>>>>> -        page_no++;
>>>>>>> -        probe_block += blocks_per_page;
>>>>>>> -reprobe:
>>>>>>> -        continue;
>>>>>>> -    }
>>>>>>> -    ret = nr_extents;
>>>>>>> -    *span = 1 + highest_block - lowest_block;
>>>>>>> -    if (page_no == 0)
>>>>>>> -        page_no = 1;    /* force Empty message */
>>>>>>> -    sis->max = page_no;
>>>>>>> -    sis->pages = page_no - 1;
>>>>>>> -    sis->highest_bit = page_no - 1;
>>>>>>> -out:
>>>>>>> -    return ret;
>>>>>>> -bad_bmap:
>>>>>>> -    f2fs_err(sbi, "Swapfile has holes\n");
>>>>>>> -    return -EINVAL;
>>>>>>> -}
>>>>>>> -
>>>>>>>     static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
>>>>>>>                     sector_t *span)
>>>>>>>     {
>>>>>>> -- 
>>>>>>> 2.29.2
>>>> .
>>>>

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

* Re: [f2fs-dev] [PATCH 1/2] f2fs: swap: remove dead codes
@ 2021-06-07 15:14               ` Chao Yu
  0 siblings, 0 replies; 22+ messages in thread
From: Chao Yu @ 2021-06-07 15:14 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel

On 2021/6/5 7:51, Jaegeuk Kim wrote:
> On 06/04, Chao Yu wrote:
>> On 2021/5/27 10:12, Chao Yu wrote:
>>> On 2021/5/27 9:40, Jaegeuk Kim wrote:
>>>> On 05/26, Chao Yu wrote:
>>>>> On 2021/5/26 21:30, Jaegeuk Kim wrote:
>>>>>> On 05/26, Chao Yu wrote:
>>>>>>> After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
>>>>>>> we will never run into original logic of check_swap_activate() before
>>>>>>> f2fs supports non 4k-sized page, so let's delete those dead codes.
>>>>>>
>>>>>> Why not keeping this for large page support in future maybe?
>>>>>
>>>>> Well, if so, at that time, it would be better to refactor
>>>>> check_swap_activate_fast() implementation based on f2fs_map_block() rather
>>>>> than refactoring check_swap_activate() implementation based on low efficient
>>>>> bmap()?
>>>>
>>>> Let's first check whether we support large page. Have you quickly tested it?
>>>> If we support it now and current flow is just inefficient, I'd say keeping but
>>>> refactoring it later.
>>>
>>> Let me check this, but I guess it missed to handle large page in lots of flows,
>>> not sure where we may panic at... :(
>>
>> I've checked f2fs behavior on 64kb page size in arm64 vm,
>> - 'ls -ls' will cause f2fs printing "invalid namelen(0), ino:%u, run fsck
>> to fix." each time.
>> - after touch a file and umount, fsck reports that image is corrupted.
>>
>> F2FS uses PAGE_SIZE and F2FS_BLKSIZE directly in a lot of places, I doubt
>> most of places may not consider large page size condition.
>>
>> So, thoughts? I guess we have to admit we do not support 64KB page right
>> now... :P
> 
> Yeah, we need to block the large page support unfortunately. :(

Could you please queue this series in dev-test?

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>> Thanks,
>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>>>> ---
>>>>>>>     fs/f2fs/data.c | 171 +------------------------------------------------
>>>>>>>     1 file changed, 3 insertions(+), 168 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>>>>> index 3058c7e28b11..9c23fde93b76 100644
>>>>>>> --- a/fs/f2fs/data.c
>>>>>>> +++ b/fs/f2fs/data.c
>>>>>>> @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
>>>>>>>     #endif
>>>>>>>     #ifdef CONFIG_SWAP
>>>>>>> -static int f2fs_is_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 nr_pblocks;
>>>>>>> -    unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
>>>>>>> -    unsigned int not_aligned = 0;
>>>>>>> -    int ret = 0;
>>>>>>> -
>>>>>>> -    cur_lblock = 0;
>>>>>>> -    last_lblock = bytes_to_blks(inode, i_size_read(inode));
>>>>>>> -
>>>>>>> -    while (cur_lblock < last_lblock) {
>>>>>>> -        struct f2fs_map_blocks map;
>>>>>>> -
>>>>>>> -        memset(&map, 0, sizeof(map));
>>>>>>> -        map.m_lblk = cur_lblock;
>>>>>>> -        map.m_len = last_lblock - cur_lblock;
>>>>>>> -        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);
>>>>>>> -        if (ret)
>>>>>>> -            goto out;
>>>>>>> -
>>>>>>> -        /* hole */
>>>>>>> -        if (!(map.m_flags & F2FS_MAP_FLAGS)) {
>>>>>>> -            f2fs_err(sbi, "Swapfile has holes\n");
>>>>>>> -            ret = -ENOENT;
>>>>>>> -            goto out;
>>>>>>> -        }
>>>>>>> -
>>>>>>> -        pblock = map.m_pblk;
>>>>>>> -        nr_pblocks = map.m_len;
>>>>>>> -
>>>>>>> -        if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
>>>>>>> -            nr_pblocks & (blocks_per_sec - 1)) {
>>>>>>> -            if (f2fs_is_pinned_file(inode)) {
>>>>>>> -                f2fs_err(sbi, "Swapfile does not align to section");
>>>>>>> -                ret = -EINVAL;
>>>>>>> -                goto out;
>>>>>>> -            }
>>>>>>> -            not_aligned++;
>>>>>>> -        }
>>>>>>> -
>>>>>>> -        cur_lblock += nr_pblocks;
>>>>>>> -    }
>>>>>>> -    if (not_aligned)
>>>>>>> -        f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
>>>>>>> -            "\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
>>>>>>> -            not_aligned);
>>>>>>> -out:
>>>>>>> -    return ret;
>>>>>>> -}
>>>>>>> -
>>>>>>> -static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>>>> +static int check_swap_activate(struct swap_info_struct *sis,
>>>>>>>                     struct file *swap_file, sector_t *span)
>>>>>>>     {
>>>>>>>         struct address_space *mapping = swap_file->f_mapping;
>>>>>>> @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>>>>         unsigned int not_aligned = 0;
>>>>>>>         int ret = 0;
>>>>>>> +    f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
>>>>>>> +
>>>>>>>         /*
>>>>>>>          * Map all the blocks into the extent list.  This code doesn't try
>>>>>>>          * to be very smart.
>>>>>>> @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>>>>         return ret;
>>>>>>>     }
>>>>>>> -/* Copied from generic_swapfile_activate() to check any holes */
>>>>>>> -static int check_swap_activate(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);
>>>>>>> -    unsigned blocks_per_page;
>>>>>>> -    unsigned long page_no;
>>>>>>> -    sector_t probe_block;
>>>>>>> -    sector_t last_block;
>>>>>>> -    sector_t lowest_block = -1;
>>>>>>> -    sector_t highest_block = 0;
>>>>>>> -    int nr_extents = 0;
>>>>>>> -    int ret = 0;
>>>>>>> -
>>>>>>> -    if (PAGE_SIZE == F2FS_BLKSIZE)
>>>>>>> -        return check_swap_activate_fast(sis, swap_file, span);
>>>>>>> -
>>>>>>> -    ret = f2fs_is_file_aligned(inode);
>>>>>>> -    if (ret)
>>>>>>> -        goto out;
>>>>>>> -
>>>>>>> -    blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
>>>>>>> -
>>>>>>> -    /*
>>>>>>> -     * Map all the blocks into the extent list.  This code doesn't try
>>>>>>> -     * to be very smart.
>>>>>>> -     */
>>>>>>> -    probe_block = 0;
>>>>>>> -    page_no = 0;
>>>>>>> -    last_block = bytes_to_blks(inode, i_size_read(inode));
>>>>>>> -    while ((probe_block + blocks_per_page) <= last_block &&
>>>>>>> -            page_no < sis->max) {
>>>>>>> -        unsigned block_in_page;
>>>>>>> -        sector_t first_block;
>>>>>>> -        sector_t block = 0;
>>>>>>> -
>>>>>>> -        cond_resched();
>>>>>>> -
>>>>>>> -        block = probe_block;
>>>>>>> -        ret = bmap(inode, &block);
>>>>>>> -        if (ret)
>>>>>>> -            goto out;
>>>>>>> -        if (!block)
>>>>>>> -            goto bad_bmap;
>>>>>>> -        first_block = block;
>>>>>>> -
>>>>>>> -        /*
>>>>>>> -         * It must be PAGE_SIZE aligned on-disk
>>>>>>> -         */
>>>>>>> -        if (first_block & (blocks_per_page - 1)) {
>>>>>>> -            probe_block++;
>>>>>>> -            goto reprobe;
>>>>>>> -        }
>>>>>>> -
>>>>>>> -        for (block_in_page = 1; block_in_page < blocks_per_page;
>>>>>>> -                    block_in_page++) {
>>>>>>> -
>>>>>>> -            block = probe_block + block_in_page;
>>>>>>> -            ret = bmap(inode, &block);
>>>>>>> -            if (ret)
>>>>>>> -                goto out;
>>>>>>> -            if (!block)
>>>>>>> -                goto bad_bmap;
>>>>>>> -
>>>>>>> -            if (block != first_block + block_in_page) {
>>>>>>> -                /* Discontiguity */
>>>>>>> -                probe_block++;
>>>>>>> -                goto reprobe;
>>>>>>> -            }
>>>>>>> -        }
>>>>>>> -
>>>>>>> -        first_block >>= (PAGE_SHIFT - inode->i_blkbits);
>>>>>>> -        if (page_no) {    /* exclude the header page */
>>>>>>> -            if (first_block < lowest_block)
>>>>>>> -                lowest_block = first_block;
>>>>>>> -            if (first_block > highest_block)
>>>>>>> -                highest_block = first_block;
>>>>>>> -        }
>>>>>>> -
>>>>>>> -        /*
>>>>>>> -         * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
>>>>>>> -         */
>>>>>>> -        ret = add_swap_extent(sis, page_no, 1, first_block);
>>>>>>> -        if (ret < 0)
>>>>>>> -            goto out;
>>>>>>> -        nr_extents += ret;
>>>>>>> -        page_no++;
>>>>>>> -        probe_block += blocks_per_page;
>>>>>>> -reprobe:
>>>>>>> -        continue;
>>>>>>> -    }
>>>>>>> -    ret = nr_extents;
>>>>>>> -    *span = 1 + highest_block - lowest_block;
>>>>>>> -    if (page_no == 0)
>>>>>>> -        page_no = 1;    /* force Empty message */
>>>>>>> -    sis->max = page_no;
>>>>>>> -    sis->pages = page_no - 1;
>>>>>>> -    sis->highest_bit = page_no - 1;
>>>>>>> -out:
>>>>>>> -    return ret;
>>>>>>> -bad_bmap:
>>>>>>> -    f2fs_err(sbi, "Swapfile has holes\n");
>>>>>>> -    return -EINVAL;
>>>>>>> -}
>>>>>>> -
>>>>>>>     static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
>>>>>>>                     sector_t *span)
>>>>>>>     {
>>>>>>> -- 
>>>>>>> 2.29.2
>>>> .
>>>>


_______________________________________________
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] 22+ messages in thread

* Re: [PATCH 1/2] f2fs: swap: remove dead codes
  2021-06-07 15:14               ` [f2fs-dev] " Chao Yu
@ 2021-06-07 16:47                 ` Jaegeuk Kim
  -1 siblings, 0 replies; 22+ messages in thread
From: Jaegeuk Kim @ 2021-06-07 16:47 UTC (permalink / raw)
  To: Chao Yu; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel

On 06/07, Chao Yu wrote:
> On 2021/6/5 7:51, Jaegeuk Kim wrote:
> > On 06/04, Chao Yu wrote:
> > > On 2021/5/27 10:12, Chao Yu wrote:
> > > > On 2021/5/27 9:40, Jaegeuk Kim wrote:
> > > > > On 05/26, Chao Yu wrote:
> > > > > > On 2021/5/26 21:30, Jaegeuk Kim wrote:
> > > > > > > On 05/26, Chao Yu wrote:
> > > > > > > > After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
> > > > > > > > we will never run into original logic of check_swap_activate() before
> > > > > > > > f2fs supports non 4k-sized page, so let's delete those dead codes.
> > > > > > > 
> > > > > > > Why not keeping this for large page support in future maybe?
> > > > > > 
> > > > > > Well, if so, at that time, it would be better to refactor
> > > > > > check_swap_activate_fast() implementation based on f2fs_map_block() rather
> > > > > > than refactoring check_swap_activate() implementation based on low efficient
> > > > > > bmap()?
> > > > > 
> > > > > Let's first check whether we support large page. Have you quickly tested it?
> > > > > If we support it now and current flow is just inefficient, I'd say keeping but
> > > > > refactoring it later.
> > > > 
> > > > Let me check this, but I guess it missed to handle large page in lots of flows,
> > > > not sure where we may panic at... :(
> > > 
> > > I've checked f2fs behavior on 64kb page size in arm64 vm,
> > > - 'ls -ls' will cause f2fs printing "invalid namelen(0), ino:%u, run fsck
> > > to fix." each time.
> > > - after touch a file and umount, fsck reports that image is corrupted.
> > > 
> > > F2FS uses PAGE_SIZE and F2FS_BLKSIZE directly in a lot of places, I doubt
> > > most of places may not consider large page size condition.
> > > 
> > > So, thoughts? I guess we have to admit we do not support 64KB page right
> > > now... :P
> > 
> > Yeah, we need to block the large page support unfortunately. :(
> 
> Could you please queue this series in dev-test?
> 
> Thanks,
> 
> > 
> > > 
> > > Thanks,
> > > 
> > > > 
> > > > Thanks,
> > > > 
> > > > > 
> > > > > > 
> > > > > > Thanks,
> > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > > > > > > ---
> > > > > > > >     fs/f2fs/data.c | 171 +------------------------------------------------
> > > > > > > >     1 file changed, 3 insertions(+), 168 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > > > > > > index 3058c7e28b11..9c23fde93b76 100644
> > > > > > > > --- a/fs/f2fs/data.c
> > > > > > > > +++ b/fs/f2fs/data.c
> > > > > > > > @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
> > > > > > > >     #endif
> > > > > > > >     #ifdef CONFIG_SWAP
> > > > > > > > -static int f2fs_is_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 nr_pblocks;
> > > > > > > > -    unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
> > > > > > > > -    unsigned int not_aligned = 0;
> > > > > > > > -    int ret = 0;
> > > > > > > > -
> > > > > > > > -    cur_lblock = 0;
> > > > > > > > -    last_lblock = bytes_to_blks(inode, i_size_read(inode));
> > > > > > > > -
> > > > > > > > -    while (cur_lblock < last_lblock) {
> > > > > > > > -        struct f2fs_map_blocks map;
> > > > > > > > -
> > > > > > > > -        memset(&map, 0, sizeof(map));
> > > > > > > > -        map.m_lblk = cur_lblock;
> > > > > > > > -        map.m_len = last_lblock - cur_lblock;
> > > > > > > > -        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);
> > > > > > > > -        if (ret)
> > > > > > > > -            goto out;
> > > > > > > > -
> > > > > > > > -        /* hole */
> > > > > > > > -        if (!(map.m_flags & F2FS_MAP_FLAGS)) {
> > > > > > > > -            f2fs_err(sbi, "Swapfile has holes\n");
> > > > > > > > -            ret = -ENOENT;
> > > > > > > > -            goto out;
> > > > > > > > -        }
> > > > > > > > -
> > > > > > > > -        pblock = map.m_pblk;
> > > > > > > > -        nr_pblocks = map.m_len;
> > > > > > > > -
> > > > > > > > -        if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
> > > > > > > > -            nr_pblocks & (blocks_per_sec - 1)) {
> > > > > > > > -            if (f2fs_is_pinned_file(inode)) {
> > > > > > > > -                f2fs_err(sbi, "Swapfile does not align to section");
> > > > > > > > -                ret = -EINVAL;
> > > > > > > > -                goto out;
> > > > > > > > -            }
> > > > > > > > -            not_aligned++;
> > > > > > > > -        }
> > > > > > > > -
> > > > > > > > -        cur_lblock += nr_pblocks;
> > > > > > > > -    }
> > > > > > > > -    if (not_aligned)
> > > > > > > > -        f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
> > > > > > > > -            "\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
> > > > > > > > -            not_aligned);
> > > > > > > > -out:
> > > > > > > > -    return ret;
> > > > > > > > -}
> > > > > > > > -
> > > > > > > > -static int check_swap_activate_fast(struct swap_info_struct *sis,
> > > > > > > > +static int check_swap_activate(struct swap_info_struct *sis,
> > > > > > > >                     struct file *swap_file, sector_t *span)
> > > > > > > >     {
> > > > > > > >         struct address_space *mapping = swap_file->f_mapping;
> > > > > > > > @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
> > > > > > > >         unsigned int not_aligned = 0;
> > > > > > > >         int ret = 0;
> > > > > > > > +    f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);

Let me remove this, since we have this actually.

in init_f2fs_fs(),

	if (PAGE_SIZE != F2FS_BLKSIZE) {
		printk("F2FS not supported on PAGE_SIZE(%lu) != %d\n",
				PAGE_SIZE, F2FS_BLKSIZE);
		return -EINVAL;
	}



> > > > > > > > +
> > > > > > > >         /*
> > > > > > > >          * Map all the blocks into the extent list.  This code doesn't try
> > > > > > > >          * to be very smart.
> > > > > > > > @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
> > > > > > > >         return ret;
> > > > > > > >     }
> > > > > > > > -/* Copied from generic_swapfile_activate() to check any holes */
> > > > > > > > -static int check_swap_activate(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);
> > > > > > > > -    unsigned blocks_per_page;
> > > > > > > > -    unsigned long page_no;
> > > > > > > > -    sector_t probe_block;
> > > > > > > > -    sector_t last_block;
> > > > > > > > -    sector_t lowest_block = -1;
> > > > > > > > -    sector_t highest_block = 0;
> > > > > > > > -    int nr_extents = 0;
> > > > > > > > -    int ret = 0;
> > > > > > > > -
> > > > > > > > -    if (PAGE_SIZE == F2FS_BLKSIZE)
> > > > > > > > -        return check_swap_activate_fast(sis, swap_file, span);
> > > > > > > > -
> > > > > > > > -    ret = f2fs_is_file_aligned(inode);
> > > > > > > > -    if (ret)
> > > > > > > > -        goto out;
> > > > > > > > -
> > > > > > > > -    blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
> > > > > > > > -
> > > > > > > > -    /*
> > > > > > > > -     * Map all the blocks into the extent list.  This code doesn't try
> > > > > > > > -     * to be very smart.
> > > > > > > > -     */
> > > > > > > > -    probe_block = 0;
> > > > > > > > -    page_no = 0;
> > > > > > > > -    last_block = bytes_to_blks(inode, i_size_read(inode));
> > > > > > > > -    while ((probe_block + blocks_per_page) <= last_block &&
> > > > > > > > -            page_no < sis->max) {
> > > > > > > > -        unsigned block_in_page;
> > > > > > > > -        sector_t first_block;
> > > > > > > > -        sector_t block = 0;
> > > > > > > > -
> > > > > > > > -        cond_resched();
> > > > > > > > -
> > > > > > > > -        block = probe_block;
> > > > > > > > -        ret = bmap(inode, &block);
> > > > > > > > -        if (ret)
> > > > > > > > -            goto out;
> > > > > > > > -        if (!block)
> > > > > > > > -            goto bad_bmap;
> > > > > > > > -        first_block = block;
> > > > > > > > -
> > > > > > > > -        /*
> > > > > > > > -         * It must be PAGE_SIZE aligned on-disk
> > > > > > > > -         */
> > > > > > > > -        if (first_block & (blocks_per_page - 1)) {
> > > > > > > > -            probe_block++;
> > > > > > > > -            goto reprobe;
> > > > > > > > -        }
> > > > > > > > -
> > > > > > > > -        for (block_in_page = 1; block_in_page < blocks_per_page;
> > > > > > > > -                    block_in_page++) {
> > > > > > > > -
> > > > > > > > -            block = probe_block + block_in_page;
> > > > > > > > -            ret = bmap(inode, &block);
> > > > > > > > -            if (ret)
> > > > > > > > -                goto out;
> > > > > > > > -            if (!block)
> > > > > > > > -                goto bad_bmap;
> > > > > > > > -
> > > > > > > > -            if (block != first_block + block_in_page) {
> > > > > > > > -                /* Discontiguity */
> > > > > > > > -                probe_block++;
> > > > > > > > -                goto reprobe;
> > > > > > > > -            }
> > > > > > > > -        }
> > > > > > > > -
> > > > > > > > -        first_block >>= (PAGE_SHIFT - inode->i_blkbits);
> > > > > > > > -        if (page_no) {    /* exclude the header page */
> > > > > > > > -            if (first_block < lowest_block)
> > > > > > > > -                lowest_block = first_block;
> > > > > > > > -            if (first_block > highest_block)
> > > > > > > > -                highest_block = first_block;
> > > > > > > > -        }
> > > > > > > > -
> > > > > > > > -        /*
> > > > > > > > -         * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
> > > > > > > > -         */
> > > > > > > > -        ret = add_swap_extent(sis, page_no, 1, first_block);
> > > > > > > > -        if (ret < 0)
> > > > > > > > -            goto out;
> > > > > > > > -        nr_extents += ret;
> > > > > > > > -        page_no++;
> > > > > > > > -        probe_block += blocks_per_page;
> > > > > > > > -reprobe:
> > > > > > > > -        continue;
> > > > > > > > -    }
> > > > > > > > -    ret = nr_extents;
> > > > > > > > -    *span = 1 + highest_block - lowest_block;
> > > > > > > > -    if (page_no == 0)
> > > > > > > > -        page_no = 1;    /* force Empty message */
> > > > > > > > -    sis->max = page_no;
> > > > > > > > -    sis->pages = page_no - 1;
> > > > > > > > -    sis->highest_bit = page_no - 1;
> > > > > > > > -out:
> > > > > > > > -    return ret;
> > > > > > > > -bad_bmap:
> > > > > > > > -    f2fs_err(sbi, "Swapfile has holes\n");
> > > > > > > > -    return -EINVAL;
> > > > > > > > -}
> > > > > > > > -
> > > > > > > >     static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
> > > > > > > >                     sector_t *span)
> > > > > > > >     {
> > > > > > > > -- 
> > > > > > > > 2.29.2
> > > > > .
> > > > > 

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

* Re: [f2fs-dev] [PATCH 1/2] f2fs: swap: remove dead codes
@ 2021-06-07 16:47                 ` Jaegeuk Kim
  0 siblings, 0 replies; 22+ messages in thread
From: Jaegeuk Kim @ 2021-06-07 16:47 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel

On 06/07, Chao Yu wrote:
> On 2021/6/5 7:51, Jaegeuk Kim wrote:
> > On 06/04, Chao Yu wrote:
> > > On 2021/5/27 10:12, Chao Yu wrote:
> > > > On 2021/5/27 9:40, Jaegeuk Kim wrote:
> > > > > On 05/26, Chao Yu wrote:
> > > > > > On 2021/5/26 21:30, Jaegeuk Kim wrote:
> > > > > > > On 05/26, Chao Yu wrote:
> > > > > > > > After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
> > > > > > > > we will never run into original logic of check_swap_activate() before
> > > > > > > > f2fs supports non 4k-sized page, so let's delete those dead codes.
> > > > > > > 
> > > > > > > Why not keeping this for large page support in future maybe?
> > > > > > 
> > > > > > Well, if so, at that time, it would be better to refactor
> > > > > > check_swap_activate_fast() implementation based on f2fs_map_block() rather
> > > > > > than refactoring check_swap_activate() implementation based on low efficient
> > > > > > bmap()?
> > > > > 
> > > > > Let's first check whether we support large page. Have you quickly tested it?
> > > > > If we support it now and current flow is just inefficient, I'd say keeping but
> > > > > refactoring it later.
> > > > 
> > > > Let me check this, but I guess it missed to handle large page in lots of flows,
> > > > not sure where we may panic at... :(
> > > 
> > > I've checked f2fs behavior on 64kb page size in arm64 vm,
> > > - 'ls -ls' will cause f2fs printing "invalid namelen(0), ino:%u, run fsck
> > > to fix." each time.
> > > - after touch a file and umount, fsck reports that image is corrupted.
> > > 
> > > F2FS uses PAGE_SIZE and F2FS_BLKSIZE directly in a lot of places, I doubt
> > > most of places may not consider large page size condition.
> > > 
> > > So, thoughts? I guess we have to admit we do not support 64KB page right
> > > now... :P
> > 
> > Yeah, we need to block the large page support unfortunately. :(
> 
> Could you please queue this series in dev-test?
> 
> Thanks,
> 
> > 
> > > 
> > > Thanks,
> > > 
> > > > 
> > > > Thanks,
> > > > 
> > > > > 
> > > > > > 
> > > > > > Thanks,
> > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > > > > > > ---
> > > > > > > >     fs/f2fs/data.c | 171 +------------------------------------------------
> > > > > > > >     1 file changed, 3 insertions(+), 168 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > > > > > > index 3058c7e28b11..9c23fde93b76 100644
> > > > > > > > --- a/fs/f2fs/data.c
> > > > > > > > +++ b/fs/f2fs/data.c
> > > > > > > > @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
> > > > > > > >     #endif
> > > > > > > >     #ifdef CONFIG_SWAP
> > > > > > > > -static int f2fs_is_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 nr_pblocks;
> > > > > > > > -    unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
> > > > > > > > -    unsigned int not_aligned = 0;
> > > > > > > > -    int ret = 0;
> > > > > > > > -
> > > > > > > > -    cur_lblock = 0;
> > > > > > > > -    last_lblock = bytes_to_blks(inode, i_size_read(inode));
> > > > > > > > -
> > > > > > > > -    while (cur_lblock < last_lblock) {
> > > > > > > > -        struct f2fs_map_blocks map;
> > > > > > > > -
> > > > > > > > -        memset(&map, 0, sizeof(map));
> > > > > > > > -        map.m_lblk = cur_lblock;
> > > > > > > > -        map.m_len = last_lblock - cur_lblock;
> > > > > > > > -        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);
> > > > > > > > -        if (ret)
> > > > > > > > -            goto out;
> > > > > > > > -
> > > > > > > > -        /* hole */
> > > > > > > > -        if (!(map.m_flags & F2FS_MAP_FLAGS)) {
> > > > > > > > -            f2fs_err(sbi, "Swapfile has holes\n");
> > > > > > > > -            ret = -ENOENT;
> > > > > > > > -            goto out;
> > > > > > > > -        }
> > > > > > > > -
> > > > > > > > -        pblock = map.m_pblk;
> > > > > > > > -        nr_pblocks = map.m_len;
> > > > > > > > -
> > > > > > > > -        if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
> > > > > > > > -            nr_pblocks & (blocks_per_sec - 1)) {
> > > > > > > > -            if (f2fs_is_pinned_file(inode)) {
> > > > > > > > -                f2fs_err(sbi, "Swapfile does not align to section");
> > > > > > > > -                ret = -EINVAL;
> > > > > > > > -                goto out;
> > > > > > > > -            }
> > > > > > > > -            not_aligned++;
> > > > > > > > -        }
> > > > > > > > -
> > > > > > > > -        cur_lblock += nr_pblocks;
> > > > > > > > -    }
> > > > > > > > -    if (not_aligned)
> > > > > > > > -        f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
> > > > > > > > -            "\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
> > > > > > > > -            not_aligned);
> > > > > > > > -out:
> > > > > > > > -    return ret;
> > > > > > > > -}
> > > > > > > > -
> > > > > > > > -static int check_swap_activate_fast(struct swap_info_struct *sis,
> > > > > > > > +static int check_swap_activate(struct swap_info_struct *sis,
> > > > > > > >                     struct file *swap_file, sector_t *span)
> > > > > > > >     {
> > > > > > > >         struct address_space *mapping = swap_file->f_mapping;
> > > > > > > > @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
> > > > > > > >         unsigned int not_aligned = 0;
> > > > > > > >         int ret = 0;
> > > > > > > > +    f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);

Let me remove this, since we have this actually.

in init_f2fs_fs(),

	if (PAGE_SIZE != F2FS_BLKSIZE) {
		printk("F2FS not supported on PAGE_SIZE(%lu) != %d\n",
				PAGE_SIZE, F2FS_BLKSIZE);
		return -EINVAL;
	}



> > > > > > > > +
> > > > > > > >         /*
> > > > > > > >          * Map all the blocks into the extent list.  This code doesn't try
> > > > > > > >          * to be very smart.
> > > > > > > > @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
> > > > > > > >         return ret;
> > > > > > > >     }
> > > > > > > > -/* Copied from generic_swapfile_activate() to check any holes */
> > > > > > > > -static int check_swap_activate(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);
> > > > > > > > -    unsigned blocks_per_page;
> > > > > > > > -    unsigned long page_no;
> > > > > > > > -    sector_t probe_block;
> > > > > > > > -    sector_t last_block;
> > > > > > > > -    sector_t lowest_block = -1;
> > > > > > > > -    sector_t highest_block = 0;
> > > > > > > > -    int nr_extents = 0;
> > > > > > > > -    int ret = 0;
> > > > > > > > -
> > > > > > > > -    if (PAGE_SIZE == F2FS_BLKSIZE)
> > > > > > > > -        return check_swap_activate_fast(sis, swap_file, span);
> > > > > > > > -
> > > > > > > > -    ret = f2fs_is_file_aligned(inode);
> > > > > > > > -    if (ret)
> > > > > > > > -        goto out;
> > > > > > > > -
> > > > > > > > -    blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
> > > > > > > > -
> > > > > > > > -    /*
> > > > > > > > -     * Map all the blocks into the extent list.  This code doesn't try
> > > > > > > > -     * to be very smart.
> > > > > > > > -     */
> > > > > > > > -    probe_block = 0;
> > > > > > > > -    page_no = 0;
> > > > > > > > -    last_block = bytes_to_blks(inode, i_size_read(inode));
> > > > > > > > -    while ((probe_block + blocks_per_page) <= last_block &&
> > > > > > > > -            page_no < sis->max) {
> > > > > > > > -        unsigned block_in_page;
> > > > > > > > -        sector_t first_block;
> > > > > > > > -        sector_t block = 0;
> > > > > > > > -
> > > > > > > > -        cond_resched();
> > > > > > > > -
> > > > > > > > -        block = probe_block;
> > > > > > > > -        ret = bmap(inode, &block);
> > > > > > > > -        if (ret)
> > > > > > > > -            goto out;
> > > > > > > > -        if (!block)
> > > > > > > > -            goto bad_bmap;
> > > > > > > > -        first_block = block;
> > > > > > > > -
> > > > > > > > -        /*
> > > > > > > > -         * It must be PAGE_SIZE aligned on-disk
> > > > > > > > -         */
> > > > > > > > -        if (first_block & (blocks_per_page - 1)) {
> > > > > > > > -            probe_block++;
> > > > > > > > -            goto reprobe;
> > > > > > > > -        }
> > > > > > > > -
> > > > > > > > -        for (block_in_page = 1; block_in_page < blocks_per_page;
> > > > > > > > -                    block_in_page++) {
> > > > > > > > -
> > > > > > > > -            block = probe_block + block_in_page;
> > > > > > > > -            ret = bmap(inode, &block);
> > > > > > > > -            if (ret)
> > > > > > > > -                goto out;
> > > > > > > > -            if (!block)
> > > > > > > > -                goto bad_bmap;
> > > > > > > > -
> > > > > > > > -            if (block != first_block + block_in_page) {
> > > > > > > > -                /* Discontiguity */
> > > > > > > > -                probe_block++;
> > > > > > > > -                goto reprobe;
> > > > > > > > -            }
> > > > > > > > -        }
> > > > > > > > -
> > > > > > > > -        first_block >>= (PAGE_SHIFT - inode->i_blkbits);
> > > > > > > > -        if (page_no) {    /* exclude the header page */
> > > > > > > > -            if (first_block < lowest_block)
> > > > > > > > -                lowest_block = first_block;
> > > > > > > > -            if (first_block > highest_block)
> > > > > > > > -                highest_block = first_block;
> > > > > > > > -        }
> > > > > > > > -
> > > > > > > > -        /*
> > > > > > > > -         * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
> > > > > > > > -         */
> > > > > > > > -        ret = add_swap_extent(sis, page_no, 1, first_block);
> > > > > > > > -        if (ret < 0)
> > > > > > > > -            goto out;
> > > > > > > > -        nr_extents += ret;
> > > > > > > > -        page_no++;
> > > > > > > > -        probe_block += blocks_per_page;
> > > > > > > > -reprobe:
> > > > > > > > -        continue;
> > > > > > > > -    }
> > > > > > > > -    ret = nr_extents;
> > > > > > > > -    *span = 1 + highest_block - lowest_block;
> > > > > > > > -    if (page_no == 0)
> > > > > > > > -        page_no = 1;    /* force Empty message */
> > > > > > > > -    sis->max = page_no;
> > > > > > > > -    sis->pages = page_no - 1;
> > > > > > > > -    sis->highest_bit = page_no - 1;
> > > > > > > > -out:
> > > > > > > > -    return ret;
> > > > > > > > -bad_bmap:
> > > > > > > > -    f2fs_err(sbi, "Swapfile has holes\n");
> > > > > > > > -    return -EINVAL;
> > > > > > > > -}
> > > > > > > > -
> > > > > > > >     static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
> > > > > > > >                     sector_t *span)
> > > > > > > >     {
> > > > > > > > -- 
> > > > > > > > 2.29.2
> > > > > .
> > > > > 


_______________________________________________
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] 22+ messages in thread

* Re: [PATCH 1/2] f2fs: swap: remove dead codes
  2021-06-07 16:47                 ` [f2fs-dev] " Jaegeuk Kim
@ 2021-06-07 23:08                   ` Chao Yu
  -1 siblings, 0 replies; 22+ messages in thread
From: Chao Yu @ 2021-06-07 23:08 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel

On 2021/6/8 0:47, Jaegeuk Kim wrote:
> On 06/07, Chao Yu wrote:
>> On 2021/6/5 7:51, Jaegeuk Kim wrote:
>>> On 06/04, Chao Yu wrote:
>>>> On 2021/5/27 10:12, Chao Yu wrote:
>>>>> On 2021/5/27 9:40, Jaegeuk Kim wrote:
>>>>>> On 05/26, Chao Yu wrote:
>>>>>>> On 2021/5/26 21:30, Jaegeuk Kim wrote:
>>>>>>>> On 05/26, Chao Yu wrote:
>>>>>>>>> After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
>>>>>>>>> we will never run into original logic of check_swap_activate() before
>>>>>>>>> f2fs supports non 4k-sized page, so let's delete those dead codes.
>>>>>>>>
>>>>>>>> Why not keeping this for large page support in future maybe?
>>>>>>>
>>>>>>> Well, if so, at that time, it would be better to refactor
>>>>>>> check_swap_activate_fast() implementation based on f2fs_map_block() rather
>>>>>>> than refactoring check_swap_activate() implementation based on low efficient
>>>>>>> bmap()?
>>>>>>
>>>>>> Let's first check whether we support large page. Have you quickly tested it?
>>>>>> If we support it now and current flow is just inefficient, I'd say keeping but
>>>>>> refactoring it later.
>>>>>
>>>>> Let me check this, but I guess it missed to handle large page in lots of flows,
>>>>> not sure where we may panic at... :(
>>>>
>>>> I've checked f2fs behavior on 64kb page size in arm64 vm,
>>>> - 'ls -ls' will cause f2fs printing "invalid namelen(0), ino:%u, run fsck
>>>> to fix." each time.
>>>> - after touch a file and umount, fsck reports that image is corrupted.
>>>>
>>>> F2FS uses PAGE_SIZE and F2FS_BLKSIZE directly in a lot of places, I doubt
>>>> most of places may not consider large page size condition.
>>>>
>>>> So, thoughts? I guess we have to admit we do not support 64KB page right
>>>> now... :P
>>>
>>> Yeah, we need to block the large page support unfortunately. :(
>>
>> Could you please queue this series in dev-test?
>>
>> Thanks,
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>>>>>> ---
>>>>>>>>>      fs/f2fs/data.c | 171 +------------------------------------------------
>>>>>>>>>      1 file changed, 3 insertions(+), 168 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>>>>>>> index 3058c7e28b11..9c23fde93b76 100644
>>>>>>>>> --- a/fs/f2fs/data.c
>>>>>>>>> +++ b/fs/f2fs/data.c
>>>>>>>>> @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
>>>>>>>>>      #endif
>>>>>>>>>      #ifdef CONFIG_SWAP
>>>>>>>>> -static int f2fs_is_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 nr_pblocks;
>>>>>>>>> -    unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
>>>>>>>>> -    unsigned int not_aligned = 0;
>>>>>>>>> -    int ret = 0;
>>>>>>>>> -
>>>>>>>>> -    cur_lblock = 0;
>>>>>>>>> -    last_lblock = bytes_to_blks(inode, i_size_read(inode));
>>>>>>>>> -
>>>>>>>>> -    while (cur_lblock < last_lblock) {
>>>>>>>>> -        struct f2fs_map_blocks map;
>>>>>>>>> -
>>>>>>>>> -        memset(&map, 0, sizeof(map));
>>>>>>>>> -        map.m_lblk = cur_lblock;
>>>>>>>>> -        map.m_len = last_lblock - cur_lblock;
>>>>>>>>> -        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);
>>>>>>>>> -        if (ret)
>>>>>>>>> -            goto out;
>>>>>>>>> -
>>>>>>>>> -        /* hole */
>>>>>>>>> -        if (!(map.m_flags & F2FS_MAP_FLAGS)) {
>>>>>>>>> -            f2fs_err(sbi, "Swapfile has holes\n");
>>>>>>>>> -            ret = -ENOENT;
>>>>>>>>> -            goto out;
>>>>>>>>> -        }
>>>>>>>>> -
>>>>>>>>> -        pblock = map.m_pblk;
>>>>>>>>> -        nr_pblocks = map.m_len;
>>>>>>>>> -
>>>>>>>>> -        if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
>>>>>>>>> -            nr_pblocks & (blocks_per_sec - 1)) {
>>>>>>>>> -            if (f2fs_is_pinned_file(inode)) {
>>>>>>>>> -                f2fs_err(sbi, "Swapfile does not align to section");
>>>>>>>>> -                ret = -EINVAL;
>>>>>>>>> -                goto out;
>>>>>>>>> -            }
>>>>>>>>> -            not_aligned++;
>>>>>>>>> -        }
>>>>>>>>> -
>>>>>>>>> -        cur_lblock += nr_pblocks;
>>>>>>>>> -    }
>>>>>>>>> -    if (not_aligned)
>>>>>>>>> -        f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
>>>>>>>>> -            "\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
>>>>>>>>> -            not_aligned);
>>>>>>>>> -out:
>>>>>>>>> -    return ret;
>>>>>>>>> -}
>>>>>>>>> -
>>>>>>>>> -static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>>>>>> +static int check_swap_activate(struct swap_info_struct *sis,
>>>>>>>>>                      struct file *swap_file, sector_t *span)
>>>>>>>>>      {
>>>>>>>>>          struct address_space *mapping = swap_file->f_mapping;
>>>>>>>>> @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>>>>>>          unsigned int not_aligned = 0;
>>>>>>>>>          int ret = 0;
>>>>>>>>> +    f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
> 
> Let me remove this, since we have this actually.

Looks fine to me.

Thanks,

> 
> in init_f2fs_fs(),
> 
> 	if (PAGE_SIZE != F2FS_BLKSIZE) {
> 		printk("F2FS not supported on PAGE_SIZE(%lu) != %d\n",
> 				PAGE_SIZE, F2FS_BLKSIZE);
> 		return -EINVAL;
> 	}
> 
> 
> 
>>>>>>>>> +
>>>>>>>>>          /*
>>>>>>>>>           * Map all the blocks into the extent list.  This code doesn't try
>>>>>>>>>           * to be very smart.
>>>>>>>>> @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>>>>>>          return ret;
>>>>>>>>>      }
>>>>>>>>> -/* Copied from generic_swapfile_activate() to check any holes */
>>>>>>>>> -static int check_swap_activate(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);
>>>>>>>>> -    unsigned blocks_per_page;
>>>>>>>>> -    unsigned long page_no;
>>>>>>>>> -    sector_t probe_block;
>>>>>>>>> -    sector_t last_block;
>>>>>>>>> -    sector_t lowest_block = -1;
>>>>>>>>> -    sector_t highest_block = 0;
>>>>>>>>> -    int nr_extents = 0;
>>>>>>>>> -    int ret = 0;
>>>>>>>>> -
>>>>>>>>> -    if (PAGE_SIZE == F2FS_BLKSIZE)
>>>>>>>>> -        return check_swap_activate_fast(sis, swap_file, span);
>>>>>>>>> -
>>>>>>>>> -    ret = f2fs_is_file_aligned(inode);
>>>>>>>>> -    if (ret)
>>>>>>>>> -        goto out;
>>>>>>>>> -
>>>>>>>>> -    blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
>>>>>>>>> -
>>>>>>>>> -    /*
>>>>>>>>> -     * Map all the blocks into the extent list.  This code doesn't try
>>>>>>>>> -     * to be very smart.
>>>>>>>>> -     */
>>>>>>>>> -    probe_block = 0;
>>>>>>>>> -    page_no = 0;
>>>>>>>>> -    last_block = bytes_to_blks(inode, i_size_read(inode));
>>>>>>>>> -    while ((probe_block + blocks_per_page) <= last_block &&
>>>>>>>>> -            page_no < sis->max) {
>>>>>>>>> -        unsigned block_in_page;
>>>>>>>>> -        sector_t first_block;
>>>>>>>>> -        sector_t block = 0;
>>>>>>>>> -
>>>>>>>>> -        cond_resched();
>>>>>>>>> -
>>>>>>>>> -        block = probe_block;
>>>>>>>>> -        ret = bmap(inode, &block);
>>>>>>>>> -        if (ret)
>>>>>>>>> -            goto out;
>>>>>>>>> -        if (!block)
>>>>>>>>> -            goto bad_bmap;
>>>>>>>>> -        first_block = block;
>>>>>>>>> -
>>>>>>>>> -        /*
>>>>>>>>> -         * It must be PAGE_SIZE aligned on-disk
>>>>>>>>> -         */
>>>>>>>>> -        if (first_block & (blocks_per_page - 1)) {
>>>>>>>>> -            probe_block++;
>>>>>>>>> -            goto reprobe;
>>>>>>>>> -        }
>>>>>>>>> -
>>>>>>>>> -        for (block_in_page = 1; block_in_page < blocks_per_page;
>>>>>>>>> -                    block_in_page++) {
>>>>>>>>> -
>>>>>>>>> -            block = probe_block + block_in_page;
>>>>>>>>> -            ret = bmap(inode, &block);
>>>>>>>>> -            if (ret)
>>>>>>>>> -                goto out;
>>>>>>>>> -            if (!block)
>>>>>>>>> -                goto bad_bmap;
>>>>>>>>> -
>>>>>>>>> -            if (block != first_block + block_in_page) {
>>>>>>>>> -                /* Discontiguity */
>>>>>>>>> -                probe_block++;
>>>>>>>>> -                goto reprobe;
>>>>>>>>> -            }
>>>>>>>>> -        }
>>>>>>>>> -
>>>>>>>>> -        first_block >>= (PAGE_SHIFT - inode->i_blkbits);
>>>>>>>>> -        if (page_no) {    /* exclude the header page */
>>>>>>>>> -            if (first_block < lowest_block)
>>>>>>>>> -                lowest_block = first_block;
>>>>>>>>> -            if (first_block > highest_block)
>>>>>>>>> -                highest_block = first_block;
>>>>>>>>> -        }
>>>>>>>>> -
>>>>>>>>> -        /*
>>>>>>>>> -         * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
>>>>>>>>> -         */
>>>>>>>>> -        ret = add_swap_extent(sis, page_no, 1, first_block);
>>>>>>>>> -        if (ret < 0)
>>>>>>>>> -            goto out;
>>>>>>>>> -        nr_extents += ret;
>>>>>>>>> -        page_no++;
>>>>>>>>> -        probe_block += blocks_per_page;
>>>>>>>>> -reprobe:
>>>>>>>>> -        continue;
>>>>>>>>> -    }
>>>>>>>>> -    ret = nr_extents;
>>>>>>>>> -    *span = 1 + highest_block - lowest_block;
>>>>>>>>> -    if (page_no == 0)
>>>>>>>>> -        page_no = 1;    /* force Empty message */
>>>>>>>>> -    sis->max = page_no;
>>>>>>>>> -    sis->pages = page_no - 1;
>>>>>>>>> -    sis->highest_bit = page_no - 1;
>>>>>>>>> -out:
>>>>>>>>> -    return ret;
>>>>>>>>> -bad_bmap:
>>>>>>>>> -    f2fs_err(sbi, "Swapfile has holes\n");
>>>>>>>>> -    return -EINVAL;
>>>>>>>>> -}
>>>>>>>>> -
>>>>>>>>>      static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
>>>>>>>>>                      sector_t *span)
>>>>>>>>>      {
>>>>>>>>> -- 
>>>>>>>>> 2.29.2
>>>>>> .
>>>>>>

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

* Re: [f2fs-dev] [PATCH 1/2] f2fs: swap: remove dead codes
@ 2021-06-07 23:08                   ` Chao Yu
  0 siblings, 0 replies; 22+ messages in thread
From: Chao Yu @ 2021-06-07 23:08 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel

On 2021/6/8 0:47, Jaegeuk Kim wrote:
> On 06/07, Chao Yu wrote:
>> On 2021/6/5 7:51, Jaegeuk Kim wrote:
>>> On 06/04, Chao Yu wrote:
>>>> On 2021/5/27 10:12, Chao Yu wrote:
>>>>> On 2021/5/27 9:40, Jaegeuk Kim wrote:
>>>>>> On 05/26, Chao Yu wrote:
>>>>>>> On 2021/5/26 21:30, Jaegeuk Kim wrote:
>>>>>>>> On 05/26, Chao Yu wrote:
>>>>>>>>> After commit af4b6b8edf6a ("f2fs: introduce check_swap_activate_fast()"),
>>>>>>>>> we will never run into original logic of check_swap_activate() before
>>>>>>>>> f2fs supports non 4k-sized page, so let's delete those dead codes.
>>>>>>>>
>>>>>>>> Why not keeping this for large page support in future maybe?
>>>>>>>
>>>>>>> Well, if so, at that time, it would be better to refactor
>>>>>>> check_swap_activate_fast() implementation based on f2fs_map_block() rather
>>>>>>> than refactoring check_swap_activate() implementation based on low efficient
>>>>>>> bmap()?
>>>>>>
>>>>>> Let's first check whether we support large page. Have you quickly tested it?
>>>>>> If we support it now and current flow is just inefficient, I'd say keeping but
>>>>>> refactoring it later.
>>>>>
>>>>> Let me check this, but I guess it missed to handle large page in lots of flows,
>>>>> not sure where we may panic at... :(
>>>>
>>>> I've checked f2fs behavior on 64kb page size in arm64 vm,
>>>> - 'ls -ls' will cause f2fs printing "invalid namelen(0), ino:%u, run fsck
>>>> to fix." each time.
>>>> - after touch a file and umount, fsck reports that image is corrupted.
>>>>
>>>> F2FS uses PAGE_SIZE and F2FS_BLKSIZE directly in a lot of places, I doubt
>>>> most of places may not consider large page size condition.
>>>>
>>>> So, thoughts? I guess we have to admit we do not support 64KB page right
>>>> now... :P
>>>
>>> Yeah, we need to block the large page support unfortunately. :(
>>
>> Could you please queue this series in dev-test?
>>
>> Thanks,
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>>>>>> ---
>>>>>>>>>      fs/f2fs/data.c | 171 +------------------------------------------------
>>>>>>>>>      1 file changed, 3 insertions(+), 168 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>>>>>>> index 3058c7e28b11..9c23fde93b76 100644
>>>>>>>>> --- a/fs/f2fs/data.c
>>>>>>>>> +++ b/fs/f2fs/data.c
>>>>>>>>> @@ -3830,67 +3830,7 @@ int f2fs_migrate_page(struct address_space *mapping,
>>>>>>>>>      #endif
>>>>>>>>>      #ifdef CONFIG_SWAP
>>>>>>>>> -static int f2fs_is_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 nr_pblocks;
>>>>>>>>> -    unsigned int blocks_per_sec = BLKS_PER_SEC(sbi);
>>>>>>>>> -    unsigned int not_aligned = 0;
>>>>>>>>> -    int ret = 0;
>>>>>>>>> -
>>>>>>>>> -    cur_lblock = 0;
>>>>>>>>> -    last_lblock = bytes_to_blks(inode, i_size_read(inode));
>>>>>>>>> -
>>>>>>>>> -    while (cur_lblock < last_lblock) {
>>>>>>>>> -        struct f2fs_map_blocks map;
>>>>>>>>> -
>>>>>>>>> -        memset(&map, 0, sizeof(map));
>>>>>>>>> -        map.m_lblk = cur_lblock;
>>>>>>>>> -        map.m_len = last_lblock - cur_lblock;
>>>>>>>>> -        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);
>>>>>>>>> -        if (ret)
>>>>>>>>> -            goto out;
>>>>>>>>> -
>>>>>>>>> -        /* hole */
>>>>>>>>> -        if (!(map.m_flags & F2FS_MAP_FLAGS)) {
>>>>>>>>> -            f2fs_err(sbi, "Swapfile has holes\n");
>>>>>>>>> -            ret = -ENOENT;
>>>>>>>>> -            goto out;
>>>>>>>>> -        }
>>>>>>>>> -
>>>>>>>>> -        pblock = map.m_pblk;
>>>>>>>>> -        nr_pblocks = map.m_len;
>>>>>>>>> -
>>>>>>>>> -        if ((pblock - main_blkaddr) & (blocks_per_sec - 1) ||
>>>>>>>>> -            nr_pblocks & (blocks_per_sec - 1)) {
>>>>>>>>> -            if (f2fs_is_pinned_file(inode)) {
>>>>>>>>> -                f2fs_err(sbi, "Swapfile does not align to section");
>>>>>>>>> -                ret = -EINVAL;
>>>>>>>>> -                goto out;
>>>>>>>>> -            }
>>>>>>>>> -            not_aligned++;
>>>>>>>>> -        }
>>>>>>>>> -
>>>>>>>>> -        cur_lblock += nr_pblocks;
>>>>>>>>> -    }
>>>>>>>>> -    if (not_aligned)
>>>>>>>>> -        f2fs_warn(sbi, "Swapfile (%u) is not align to section: \n"
>>>>>>>>> -            "\t1) creat(), 2) ioctl(F2FS_IOC_SET_PIN_FILE), 3) fallocate()",
>>>>>>>>> -            not_aligned);
>>>>>>>>> -out:
>>>>>>>>> -    return ret;
>>>>>>>>> -}
>>>>>>>>> -
>>>>>>>>> -static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>>>>>> +static int check_swap_activate(struct swap_info_struct *sis,
>>>>>>>>>                      struct file *swap_file, sector_t *span)
>>>>>>>>>      {
>>>>>>>>>          struct address_space *mapping = swap_file->f_mapping;
>>>>>>>>> @@ -3907,6 +3847,8 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>>>>>>          unsigned int not_aligned = 0;
>>>>>>>>>          int ret = 0;
>>>>>>>>> +    f2fs_bug_on(sbi, PAGE_SIZE != F2FS_BLKSIZE);
> 
> Let me remove this, since we have this actually.

Looks fine to me.

Thanks,

> 
> in init_f2fs_fs(),
> 
> 	if (PAGE_SIZE != F2FS_BLKSIZE) {
> 		printk("F2FS not supported on PAGE_SIZE(%lu) != %d\n",
> 				PAGE_SIZE, F2FS_BLKSIZE);
> 		return -EINVAL;
> 	}
> 
> 
> 
>>>>>>>>> +
>>>>>>>>>          /*
>>>>>>>>>           * Map all the blocks into the extent list.  This code doesn't try
>>>>>>>>>           * to be very smart.
>>>>>>>>> @@ -3986,113 +3928,6 @@ static int check_swap_activate_fast(struct swap_info_struct *sis,
>>>>>>>>>          return ret;
>>>>>>>>>      }
>>>>>>>>> -/* Copied from generic_swapfile_activate() to check any holes */
>>>>>>>>> -static int check_swap_activate(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);
>>>>>>>>> -    unsigned blocks_per_page;
>>>>>>>>> -    unsigned long page_no;
>>>>>>>>> -    sector_t probe_block;
>>>>>>>>> -    sector_t last_block;
>>>>>>>>> -    sector_t lowest_block = -1;
>>>>>>>>> -    sector_t highest_block = 0;
>>>>>>>>> -    int nr_extents = 0;
>>>>>>>>> -    int ret = 0;
>>>>>>>>> -
>>>>>>>>> -    if (PAGE_SIZE == F2FS_BLKSIZE)
>>>>>>>>> -        return check_swap_activate_fast(sis, swap_file, span);
>>>>>>>>> -
>>>>>>>>> -    ret = f2fs_is_file_aligned(inode);
>>>>>>>>> -    if (ret)
>>>>>>>>> -        goto out;
>>>>>>>>> -
>>>>>>>>> -    blocks_per_page = bytes_to_blks(inode, PAGE_SIZE);
>>>>>>>>> -
>>>>>>>>> -    /*
>>>>>>>>> -     * Map all the blocks into the extent list.  This code doesn't try
>>>>>>>>> -     * to be very smart.
>>>>>>>>> -     */
>>>>>>>>> -    probe_block = 0;
>>>>>>>>> -    page_no = 0;
>>>>>>>>> -    last_block = bytes_to_blks(inode, i_size_read(inode));
>>>>>>>>> -    while ((probe_block + blocks_per_page) <= last_block &&
>>>>>>>>> -            page_no < sis->max) {
>>>>>>>>> -        unsigned block_in_page;
>>>>>>>>> -        sector_t first_block;
>>>>>>>>> -        sector_t block = 0;
>>>>>>>>> -
>>>>>>>>> -        cond_resched();
>>>>>>>>> -
>>>>>>>>> -        block = probe_block;
>>>>>>>>> -        ret = bmap(inode, &block);
>>>>>>>>> -        if (ret)
>>>>>>>>> -            goto out;
>>>>>>>>> -        if (!block)
>>>>>>>>> -            goto bad_bmap;
>>>>>>>>> -        first_block = block;
>>>>>>>>> -
>>>>>>>>> -        /*
>>>>>>>>> -         * It must be PAGE_SIZE aligned on-disk
>>>>>>>>> -         */
>>>>>>>>> -        if (first_block & (blocks_per_page - 1)) {
>>>>>>>>> -            probe_block++;
>>>>>>>>> -            goto reprobe;
>>>>>>>>> -        }
>>>>>>>>> -
>>>>>>>>> -        for (block_in_page = 1; block_in_page < blocks_per_page;
>>>>>>>>> -                    block_in_page++) {
>>>>>>>>> -
>>>>>>>>> -            block = probe_block + block_in_page;
>>>>>>>>> -            ret = bmap(inode, &block);
>>>>>>>>> -            if (ret)
>>>>>>>>> -                goto out;
>>>>>>>>> -            if (!block)
>>>>>>>>> -                goto bad_bmap;
>>>>>>>>> -
>>>>>>>>> -            if (block != first_block + block_in_page) {
>>>>>>>>> -                /* Discontiguity */
>>>>>>>>> -                probe_block++;
>>>>>>>>> -                goto reprobe;
>>>>>>>>> -            }
>>>>>>>>> -        }
>>>>>>>>> -
>>>>>>>>> -        first_block >>= (PAGE_SHIFT - inode->i_blkbits);
>>>>>>>>> -        if (page_no) {    /* exclude the header page */
>>>>>>>>> -            if (first_block < lowest_block)
>>>>>>>>> -                lowest_block = first_block;
>>>>>>>>> -            if (first_block > highest_block)
>>>>>>>>> -                highest_block = first_block;
>>>>>>>>> -        }
>>>>>>>>> -
>>>>>>>>> -        /*
>>>>>>>>> -         * We found a PAGE_SIZE-length, PAGE_SIZE-aligned run of blocks
>>>>>>>>> -         */
>>>>>>>>> -        ret = add_swap_extent(sis, page_no, 1, first_block);
>>>>>>>>> -        if (ret < 0)
>>>>>>>>> -            goto out;
>>>>>>>>> -        nr_extents += ret;
>>>>>>>>> -        page_no++;
>>>>>>>>> -        probe_block += blocks_per_page;
>>>>>>>>> -reprobe:
>>>>>>>>> -        continue;
>>>>>>>>> -    }
>>>>>>>>> -    ret = nr_extents;
>>>>>>>>> -    *span = 1 + highest_block - lowest_block;
>>>>>>>>> -    if (page_no == 0)
>>>>>>>>> -        page_no = 1;    /* force Empty message */
>>>>>>>>> -    sis->max = page_no;
>>>>>>>>> -    sis->pages = page_no - 1;
>>>>>>>>> -    sis->highest_bit = page_no - 1;
>>>>>>>>> -out:
>>>>>>>>> -    return ret;
>>>>>>>>> -bad_bmap:
>>>>>>>>> -    f2fs_err(sbi, "Swapfile has holes\n");
>>>>>>>>> -    return -EINVAL;
>>>>>>>>> -}
>>>>>>>>> -
>>>>>>>>>      static int f2fs_swap_activate(struct swap_info_struct *sis, struct file *file,
>>>>>>>>>                      sector_t *span)
>>>>>>>>>      {
>>>>>>>>> -- 
>>>>>>>>> 2.29.2
>>>>>> .
>>>>>>


_______________________________________________
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] 22+ messages in thread

end of thread, other threads:[~2021-06-07 23:09 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-26  6:29 [PATCH 1/2] f2fs: swap: remove dead codes Chao Yu
2021-05-26  6:29 ` [f2fs-dev] " Chao Yu
2021-05-26  6:29 ` [PATCH 2/2] f2fs: swap: support migrating swapfile in aligned write mode Chao Yu
2021-05-26  6:29   ` [f2fs-dev] " Chao Yu
2021-05-26 13:30 ` [PATCH 1/2] f2fs: swap: remove dead codes Jaegeuk Kim
2021-05-26 13:30   ` [f2fs-dev] " Jaegeuk Kim
2021-05-26 15:02   ` Chao Yu
2021-05-26 15:02     ` [f2fs-dev] " Chao Yu
2021-05-27  1:40     ` Jaegeuk Kim
2021-05-27  1:40       ` [f2fs-dev] " Jaegeuk Kim
2021-05-27  2:12       ` Chao Yu
2021-05-27  2:12         ` [f2fs-dev] " Chao Yu
2021-06-04 11:12         ` Chao Yu
2021-06-04 11:12           ` [f2fs-dev] " Chao Yu
2021-06-04 23:51           ` Jaegeuk Kim
2021-06-04 23:51             ` [f2fs-dev] " Jaegeuk Kim
2021-06-07 15:14             ` Chao Yu
2021-06-07 15:14               ` [f2fs-dev] " Chao Yu
2021-06-07 16:47               ` Jaegeuk Kim
2021-06-07 16:47                 ` [f2fs-dev] " Jaegeuk Kim
2021-06-07 23:08                 ` Chao Yu
2021-06-07 23:08                   ` [f2fs-dev] " Chao Yu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.