f2fs: reassign new segment for mode=lfs

This patch fixes wrong tracepoints in terms of op and op_flags.

Signed-off-by Jaegeuk Kim <[email protected]>

This patch adds final submit_bio() tracepoint.

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/data.c              |  1 +
 include/trace/events/f2fs.h | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

fs/f2fs/data.c +1 -0

index 2c5df1dc1479..54da4e2f1318 100644
@@ -175,6 +175,7 @@ static inline void __submit_bio(struct f2fs_sb_info *sbi,
 			current->plug && (type == DATA || type == NODE))
 			blk_finish_plug(current->plug);
 	}
+	trace_f2fs_submit_bio(sbi->sb, bio);
 	submit_bio(bio);
 }
 

include/trace/events/f2fs.h +0 -0

index 4c942599581b..094d92b52450 100644

include/trace/events/f2fs.h +28 -0

+
+	TP_PROTO(struct super_block *sb, struct bio *bio),
+
+	TP_ARGS(sb, bio),
+
+	TP_STRUCT__entry(
+		__field(dev_t,	dev)
+		__field(int,	op)
+		__field(int,	op_flags)
+		__field(sector_t,	sector)
+		__field(unsigned int,	size)
+	),
+
+	TP_fast_assign(
+		__entry->dev		= sb->s_dev;
+		__entry->op		= bio->bi_opf & REQ_OP_MASK;
+		__entry->op_flags	= bio->bi_opf;
+		__entry->sector		= bio->bi_iter.bi_sector;
+		__entry->size		= bio->bi_iter.bi_size;
+	),
+
+	TP_printk("dev = (%d,%d), rw = %s%s, sector = %lld, size = %u",
+		show_dev(__entry),
+		show_bio_type(__entry->op, __entry->op_flags),
+		(unsigned long long)__entry->sector,
+		__entry->size)
+);
+
 TRACE_EVENT(f2fs_write_begin,
 
 	TP_PROTO(struct inode *inode, loff_t pos, unsigned int len,

From f5c6ab0f19fb627ebf28ab6a18c0a32b4c8f914f Mon Sep 17 00:00:00 2001

Change log from v1:
- remove F2FS_OP
- share single tracing structure, f2fs__bio
- split f2fs_prepare_read/write_io and f2fs_submit_read/write_io

Quoted 1 line

From f5c6ab0f19fb627ebf28ab6a18c0a32b4c8f914f Mon Sep 17 00:00:00 2001

From: Jaegeuk Kim <[email protected]>
Date: Wed, 21 Dec 2016 12:13:03 -0800
Subject: [PATCH] f2fs: add submit_bio tracepoint

This patch adds final submit_bio() tracepoint.

Signed-off-by Jaegeuk Kim <[email protected]>

This patch implements IO alignment by filling dummy blocks in DATA and NODE write bios. If we can guarantee, for example, 32KB or 64KB for such the IOs, we can eliminate underlying dummy page problem which FTL conducts in order to close MLC or TLC partial written pages.

Note that,
- it requires "-o mode=lfs".
- IO size should be power of 2, not exceed BIO_MAX_PAGES, 256.
- read IO is still 4KB.
- do checkpoint at fsync, if dummy NODE page was written.

Hi Jaegeuk,

Quoted 5 lines · Jaegeuk Kim

This patch implements IO alignment by filling dummy blocks in DATA and NODE write bios. If we can guarantee, for example, 32KB or 64KB for such the IOs, we can eliminate underlying dummy page problem which FTL conducts in order to close MLC or TLC partial written pages.

Quoted 5 lines

Note that,
- it requires "-o mode=lfs".
- IO size should be power of 2, not exceed BIO_MAX_PAGES, 256.
- read IO is still 4KB.
- do checkpoint at fsync, if dummy NODE page was written.

Which scenario we can benefit from? Any numbers?

I doubt that there are some potential side-effect points:
- write amplification will be more serious than before
- free space will be more fragmented since dummy blocks is separated in whole
address space
- there is less chance to merge small(unaligned) IOs in block layer

Thoughts?

Thanks,

Quoted 14 lines · Chao Yu

Hi Jaegeuk,

Quoted 10 lines · Jaegeuk Kim

This patch implements IO alignment by filling dummy blocks in DATA and NODE write bios. If we can guarantee, for example, 32KB or 64KB for such the IOs, we can eliminate underlying dummy page problem which FTL conducts in order to close MLC or TLC partial written pages.

Note that,
- it requires "-o mode=lfs".
- IO size should be power of 2, not exceed BIO_MAX_PAGES, 256.
- read IO is still 4KB.
- do checkpoint at fsync, if dummy NODE page was written.

Quoted 1 line

Which scenario we can benefit from? Any numbers?

I described it in the patch. This is not targetting for performance improvement for now, but to address the dummy page write problem in FTL so that we can later implement very simple host-level FTL on top of open-channel SSD.

Quoted 6 lines

I doubt that there are some potential side-effect points:
- write amplification will be more serious than before
- free space will be more fragmented since dummy blocks is separated in whole
address space
- there is less chance to merge small(unaligned) IOs in block layer

I agree, so I just added this as a mount option experimentally. One point would be that, if f2fs doesn't do this, FTL should do it. So I think, from the system point of view, f2fs is a better layer to do it.

Thanks,

Quoted 4 lines

Thoughts?

Thanks,

.

This patch adds to show the max number of atomic operations which are conducting concurrently.

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/debug.c |  7 +++++++
 fs/f2fs/f2fs.h  | 17 +++++++++++++++++
 fs/f2fs/file.c  | 12 +++++++++---
 3 files changed, 33 insertions(+), 3 deletions(-)

fs/f2fs/debug.c +7 -0

index fbd5184140d0..29cdf0c1da1d 100644
@@ -50,6 +50,8 @@ static void update_general_status(struct f2fs_sb_info *sbi)
 	si->ndirty_files = sbi->ndirty_inode[FILE_INODE];
 	si->ndirty_all = sbi->ndirty_inode[DIRTY_META];
 	si->inmem_pages = get_pages(sbi, F2FS_INMEM_PAGES);
+	si->aw_cnt = atomic_read(&sbi->aw_cnt);
+	si->max_aw_cnt = atomic_read(&sbi->max_aw_cnt);
 	si->nr_wb_cp_data = get_pages(sbi, F2FS_WB_CP_DATA);
 	si->nr_wb_data = get_pages(sbi, F2FS_WB_DATA);
 	si->total_count = (int)sbi->user_block_count / sbi->blocks_per_seg;

@@ -256,6 +258,8 @@ static int stat_show(struct seq_file *s, void *v)
 			   si->inline_dir);
 		seq_printf(s, "  - Orphan Inode: %u\n",
 			   si->orphans);
+		seq_printf(s, "  - Atomic write count: %4d (Max. %4d)\n",
+			   si->aw_cnt, si->max_aw_cnt);
 		seq_printf(s, "\nMain area: %d segs, %d secs %d zones\n",
 			   si->main_area_segs, si->main_area_sections,
 			   si->main_area_zones);

@@ -414,6 +418,9 @@ int f2fs_build_stats(struct f2fs_sb_info *sbi)
 	atomic_set(&sbi->inline_dir, 0);
 	atomic_set(&sbi->inplace_count, 0);
 
+	atomic_set(&sbi->aw_cnt, 0);
+	atomic_set(&sbi->max_aw_cnt, 0);
+
 	mutex_lock(&f2fs_stat_mutex);
 	list_add_tail(&si->stat_list, &f2fs_stat_list);
 	mutex_unlock(&f2fs_stat_mutex);

fs/f2fs/f2fs.h +17 -0

index 0d7eaddef4a0..bdcfe2a9b532 100644
@@ -884,6 +884,8 @@ struct f2fs_sb_info {
 	atomic_t inline_xattr;			/* # of inline_xattr inodes */
 	atomic_t inline_inode;			/* # of inline_data inodes */
 	atomic_t inline_dir;			/* # of inline_dentry inodes */
+	atomic_t aw_cnt;			/* # of atomic writes */
+	atomic_t max_aw_cnt;			/* max # of atomic writes */
 	int bg_gc;				/* background gc calls */
 	unsigned int ndirty_inode[NR_INODE_TYPE];	/* # of dirty inodes */
 #endif

@@ -2236,6 +2238,7 @@ struct f2fs_stat_info {
 	int total_count, utilization;
 	int bg_gc, nr_wb_cp_data, nr_wb_data;
 	int inline_xattr, inline_inode, inline_dir, orphans;
+	int aw_cnt, max_aw_cnt;
 	unsigned int valid_count, valid_node_count, valid_inode_count, discard_blks;
 	unsigned int bimodal, avg_vblocks;
 	int util_free, util_valid, util_invalid;

@@ -2307,6 +2310,17 @@ static inline struct f2fs_stat_info *F2FS_STAT(struct f2fs_sb_info *sbi)
 		((sbi)->block_count[(curseg)->alloc_type]++)
 #define stat_inc_inplace_blocks(sbi)					\
 		(atomic_inc(&(sbi)->inplace_count))
+#define stat_inc_atomic_write(inode)					\
+		(atomic_inc(&F2FS_I_SB(inode)->aw_cnt));
+#define stat_dec_atomic_write(inode)					\
+		(atomic_dec(&F2FS_I_SB(inode)->aw_cnt));
+#define stat_update_max_atomic_write(inode)				\
+	do {								\
+		int cur = atomic_read(&F2FS_I_SB(inode)->aw_cnt);	\
+		int max = atomic_read(&F2FS_I_SB(inode)->max_aw_cnt);	\
+		if (cur > max)						\
+			atomic_set(&F2FS_I_SB(inode)->max_aw_cnt, cur);	\
+	} while (0)
 #define stat_inc_seg_count(sbi, type, gc_type)				\
 	do {								\
 		struct f2fs_stat_info *si = F2FS_STAT(sbi);		\

@@ -2360,6 +2374,9 @@ void f2fs_destroy_root_stats(void);
 #define stat_dec_inline_inode(inode)
 #define stat_inc_inline_dir(inode)
 #define stat_dec_inline_dir(inode)
+#define stat_inc_atomic_write(inode)
+#define stat_dec_atomic_write(inode)
+#define stat_update_max_atomic_write(inode)
 #define stat_inc_seg_type(sbi, curseg)
 #define stat_inc_block_count(sbi, curseg)
 #define stat_inc_inplace_blocks(sbi)

fs/f2fs/file.c +6 -2

index 49f10dce817d..826fefc05fb1 100644
@@ -1542,6 +1542,8 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 	if (ret)
 		clear_inode_flag(inode, FI_ATOMIC_FILE);
 out:
+	stat_inc_atomic_write(inode);
+	stat_update_max_atomic_write(inode);
 	inode_unlock(inode);
 	mnt_drop_write_file(filp);
 	return ret;

@@ -1571,9 +1573,11 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
 			set_inode_flag(inode, FI_ATOMIC_FILE);
 			goto err_out;
 		}
+		ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true);
+		stat_dec_atomic_write(inode);
+	} else {
+		ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true);
 	}
-
-	ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true);
 err_out:
 	inode_unlock(inode);
 	mnt_drop_write_file(filp);

From 5464f744fe28187b3d71ba6c3cf983659a569e9a Mon Sep 17 00:00:00 2001

Chang log from v1:
- move stat_dec_atomic_write(inode); into drop_inmem_pages()

Quoted 1 line

From 5464f744fe28187b3d71ba6c3cf983659a569e9a Mon Sep 17 00:00:00 2001

From: Jaegeuk Kim <[email protected]>
Date: Wed, 28 Dec 2016 13:55:09 -0800
Subject: [PATCH] f2fs: show the max number of atomic operations

This patch adds to show the max number of atomic operations which are conducting concurrently.

Signed-off-by Jaegeuk Kim <[email protected]>

This patch fixes the renaming bug on encrypted filenames, which was pointed by

(ext4: don't allow encrypted operations without keys)

Cc Theodore Ts'o <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

This patch relaxes async discard commands to avoid waiting its end_io during checkpoint.
Instead of waiting them during checkpoint, it will be done when actually reusing them.

Test on initial partition of nvme drive.

 # time fstrim /mnt/test

Before : 6.158s
After : 4.822s

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/checkpoint.c |  3 ---
 fs/f2fs/f2fs.h       |  4 +++-
 fs/f2fs/segment.c    | 25 +++++++++++++++++++++----
 fs/f2fs/super.c      |  3 +++
 4 files changed, 27 insertions(+), 8 deletions(-)

fs/f2fs/checkpoint.c +0 -3

index 34bfe2b494ae..1a9ba69a22ba 100644
@@ -1254,7 +1254,6 @@ int write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 		f2fs_bug_on(sbi, prefree_segments(sbi));
 		flush_sit_entries(sbi, cpc);
 		clear_prefree_segments(sbi, cpc);
-		f2fs_wait_all_discard_bio(sbi);
 		unblock_operations(sbi);
 		goto out;
 	}

@@ -1278,8 +1277,6 @@ int write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	else
 		clear_prefree_segments(sbi, cpc);
 
-	f2fs_wait_all_discard_bio(sbi);
-
 	unblock_operations(sbi);
 	stat_inc_cp_count(sbi->stat_info);
 

fs/f2fs/f2fs.h +3 -1

index bdcfe2a9b532..f2f40fce9d31 100644
@@ -183,6 +183,8 @@ struct discard_entry {
 
 struct bio_entry {
 	struct list_head list;
+	unsigned int start_segno;
+	unsigned int end_segno;
 	struct bio *bio;
 	struct completion event;
 	int error;

@@ -2111,7 +2113,7 @@ void destroy_flush_cmd_control(struct f2fs_sb_info *, bool);
 void invalidate_blocks(struct f2fs_sb_info *, block_t);
 bool is_checkpointed_data(struct f2fs_sb_info *, block_t);
 void refresh_sit_entry(struct f2fs_sb_info *, block_t, block_t);
-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *);
+void f2fs_wait_discard_bio(struct f2fs_sb_info *, unsigned int);
 void clear_prefree_segments(struct f2fs_sb_info *, struct cp_control *);
 void release_discard_addrs(struct f2fs_sb_info *);
 int npages_for_summary_flush(struct f2fs_sb_info *, bool);

fs/f2fs/segment.c +6 -2

index 6d197f0c8151..9a38424c3c1f 100644
@@ -624,20 +624,24 @@ static void locate_dirty_segment(struct f2fs_sb_info *sbi, unsigned int segno)
 }
 
 static struct bio_entry *__add_bio_entry(struct f2fs_sb_info *sbi,
-							struct bio *bio)
+			struct bio *bio, unsigned int start_segno,
+					unsigned int end_segno)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be = f2fs_kmem_cache_alloc(bio_entry_slab, GFP_NOFS);
 
 	INIT_LIST_HEAD(&be->list);
 	be->bio = bio;
+	be->start_segno = start_segno;
+	be->end_segno = end_segno;
 	init_completion(&be->event);
 	list_add_tail(&be->list, wait_list);
 
 	return be;
 }
 
-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
+/* This should be covered by global mutex, &sit_i->sentry_lock */
+void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, unsigned int segno)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be, *tmp;

fs/f2fs/segment.c +12 -1

+					segno == NULL_SEGNO)
+				wait_for_completion_io(&be->event);
+			else
+				continue;
+		}
+
 		err = be->error;
 		if (err == -EOPNOTSUPP)
 			err = 0;

@@ -674,6 +686,8 @@ static int __f2fs_issue_discard_async(struct f2fs_sb_info *sbi,
 		struct block_device *bdev, block_t blkstart, block_t blklen)
 {
 	struct bio *bio = NULL;
+	unsigned int start_segno = GET_SEGNO(sbi, blkstart);
+	unsigned int end_segno = GET_SEGNO(sbi, blkstart + blklen);
 	int err;
 
 	trace_f2fs_issue_discard(sbi->sb, blkstart, blklen);

@@ -688,7 +702,8 @@ static int __f2fs_issue_discard_async(struct f2fs_sb_info *sbi,
 				SECTOR_FROM_BLOCK(blklen),
 				GFP_NOFS, 0, &bio);
 	if (!err && bio) {
-		struct bio_entry *be = __add_bio_entry(sbi, bio);
+		struct bio_entry *be = __add_bio_entry(sbi, bio,
+					start_segno, end_segno);
 
 		bio->bi_private = be;
 		bio->bi_end_io = f2fs_submit_bio_wait_endio;

@@ -1574,6 +1589,8 @@ void allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
 
 	*new_blkaddr = NEXT_FREE_BLKADDR(sbi, curseg);
 
+	f2fs_wait_discard_bio(sbi, GET_SEGNO(sbi, *new_blkaddr));
+
 	/*
 	 * __add_sum_entry should be resided under the curseg_mutex
 	 * because, this function updates a summary entry in the

fs/f2fs/super.c +3 -0

index f3697f97e527..16e2bc5209bb 100644
@@ -770,6 +770,9 @@ static void f2fs_put_super(struct super_block *sb)
 		write_checkpoint(sbi, &cpc);
 	}
 
+	/* be sure to wait for any on-going discard commands */
+	f2fs_wait_discard_bio(sbi, NULL_SEGNO);
+
 	/* write_checkpoint can update stat informaion */
 	f2fs_destroy_stats(sbi);
 

Quoted 9 lines · Jaegeuk Kim

This patch relaxes async discard commands to avoid waiting its end_io during checkpoint.
Instead of waiting them during checkpoint, it will be done when actually reusing them.

Test on initial partition of nvme drive.

 # time fstrim /mnt/test

Quoted 4 lines

Before : 6.158s
After : 4.822s

Signed-off-by Jaegeuk Kim <[email protected]>

Reviewed-by Chao Yu <[email protected]>

One comment below,

Quoted 10 lines

fs/f2fs/segment.c +2 -1

-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
+/* This should be covered by global mutex, &sit_i->sentry_lock */
+void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, unsigned int segno)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be, *tmp;

@@ -646,7 +650,15 @@ void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
 		struct bio *bio = be->bio;
 		int err;
 

fs/f2fs/segment.c void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi) · line 655

Quoted 4 lines

fs/f2fs/segment.c +3 -1

-		wait_for_completion_io(&be->event);
+		if (!completion_done(&be->event)) {
+			if ((be->start_segno >= segno &&
+					be->end_segno <= segno) ||

segno >= be->start_segno && segno < be->end_segno ?

Quoted 10 lines · Chao Yu

Quoted 9 lines · Jaegeuk Kim

This patch relaxes async discard commands to avoid waiting its end_io during checkpoint.
Instead of waiting them during checkpoint, it will be done when actually reusing them.

Test on initial partition of nvme drive.

 # time fstrim /mnt/test

Quoted 8 lines

Quoted 4 lines

Before : 6.158s
After : 4.822s

Signed-off-by Jaegeuk Kim <[email protected]>

Reviewed-by Chao Yu <[email protected]>

One comment below,

I still have a comment on this patch.

Quoted 10 lines

Quoted 10 lines

fs/f2fs/segment.c +2 -1

-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
+/* This should be covered by global mutex, &sit_i->sentry_lock */
+void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, unsigned int segno)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be, *tmp;

@@ -646,7 +650,15 @@ void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
 		struct bio *bio = be->bio;
 		int err;
 

Quoted 6 lines

fs/f2fs/segment.c void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi) · line 655

Quoted 4 lines

fs/f2fs/segment.c +3 -1

-		wait_for_completion_io(&be->event);
+		if (!completion_done(&be->event)) {
+			if ((be->start_segno >= segno &&
+					be->end_segno <= segno) ||

segno >= be->start_segno && segno < be->end_segno ?

Can you check this?

Thanks,

Hi Jaegeuk,

I can see patch named ("f2fs: call f2fs_wait_all_discard_bio for an error case") was merged in dev-test, but I think it's no needed to change error case handling like this since f2fs_wait_all_discard_bio should always be called after clear_prefree_segments.

Thanks,

Quoted 43 lines · Chao Yu

Quoted 10 lines · Chao Yu

Quoted 9 lines · Jaegeuk Kim

This patch relaxes async discard commands to avoid waiting its end_io during checkpoint.
Instead of waiting them during checkpoint, it will be done when actually reusing them.

Test on initial partition of nvme drive.

 # time fstrim /mnt/test

Quoted 8 lines

Quoted 4 lines

Before : 6.158s
After : 4.822s

Signed-off-by Jaegeuk Kim <[email protected]>

Reviewed-by Chao Yu <[email protected]>

One comment below,

I still have a comment on this patch.

Quoted 10 lines

Quoted 10 lines

fs/f2fs/segment.c +2 -1

-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
+/* This should be covered by global mutex, &sit_i->sentry_lock */
+void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, unsigned int segno)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be, *tmp;

@@ -646,7 +650,15 @@ void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
 		struct bio *bio = be->bio;
 		int err;
 

Quoted 6 lines

fs/f2fs/segment.c void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi) · line 655

Quoted 4 lines

fs/f2fs/segment.c +3 -1

-		wait_for_completion_io(&be->event);
+		if (!completion_done(&be->event)) {
+			if ((be->start_segno >= segno &&
+					be->end_segno <= segno) ||

segno >= be->start_segno && segno < be->end_segno ?

Can you check this?

Thanks,

Quoted 10 lines · Chao Yu

Quoted 9 lines · Jaegeuk Kim

This patch relaxes async discard commands to avoid waiting its end_io during checkpoint.
Instead of waiting them during checkpoint, it will be done when actually reusing them.

Test on initial partition of nvme drive.

 # time fstrim /mnt/test

Quoted 8 lines

Quoted 4 lines

Before : 6.158s
After : 4.822s

Signed-off-by Jaegeuk Kim <[email protected]>

Reviewed-by Chao Yu <[email protected]>

One comment below,

I still have a comment on this patch.

Quoted 10 lines

Quoted 10 lines

fs/f2fs/segment.c +2 -1

-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
+/* This should be covered by global mutex, &sit_i->sentry_lock */
+void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, unsigned int segno)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be, *tmp;

@@ -646,7 +650,15 @@ void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
 		struct bio *bio = be->bio;
 		int err;
 

Quoted 6 lines

fs/f2fs/segment.c void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi) · line 655

Quoted 4 lines

fs/f2fs/segment.c +3 -1

-		wait_for_completion_io(&be->event);
+		if (!completion_done(&be->event)) {
+			if ((be->start_segno >= segno &&
+					be->end_segno <= segno) ||

segno >= be->start_segno && segno < be->end_segno ?

Can you check this?

Quoted 32 lines · Chao Yu

Quoted 10 lines · Chao Yu

Quoted 9 lines · Jaegeuk Kim

This patch relaxes async discard commands to avoid waiting its end_io during checkpoint.
Instead of waiting them during checkpoint, it will be done when actually reusing them.

Test on initial partition of nvme drive.

 # time fstrim /mnt/test

Quoted 8 lines

Quoted 4 lines

Before : 6.158s
After : 4.822s

Signed-off-by Jaegeuk Kim <[email protected]>

Reviewed-by Chao Yu <[email protected]>

One comment below,

I still have a comment on this patch.

Quoted 10 lines

Quoted 10 lines

fs/f2fs/segment.c +2 -1

-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
+/* This should be covered by global mutex, &sit_i->sentry_lock */
+void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, unsigned int segno)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be, *tmp;

@@ -646,7 +650,15 @@ void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
 		struct bio *bio = be->bio;
 		int err;
 

Quoted 6 lines

Quoted 6 lines

fs/f2fs/segment.c void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi) · line 655

Quoted 4 lines

fs/f2fs/segment.c +3 -1

-		wait_for_completion_io(&be->event);
+		if (!completion_done(&be->event)) {
+			if ((be->start_segno >= segno &&
+					be->end_segno <= segno) ||

segno >= be->start_segno && segno < be->end_segno ?

Quoted 2 lines

Can you check this?

The be->start_segno and be->end_segno are assigned by:

unsigned int start_segno = GET_SEGNO(sbi, blkstart); unsigned int end_segno = GET_SEGNO(sbi, blkstart + blklen);

in __f2fs_issue_discard_async().

So, the problem comes when, for example, blkstart = 0 and blklen = 512.

I should have got the end_segno by:

unsigned int end_segno = GET_SEGNO(sbi, blkstart + blklen - 1);

Thanks,

.

Quoted 33 lines · Jaegeuk Kim

Quoted 32 lines · Chao Yu

Quoted 10 lines · Chao Yu

Quoted 9 lines · Jaegeuk Kim

This patch relaxes async discard commands to avoid waiting its end_io during checkpoint.
Instead of waiting them during checkpoint, it will be done when actually reusing them.

Test on initial partition of nvme drive.

 # time fstrim /mnt/test

Quoted 8 lines

Quoted 4 lines

Before : 6.158s
After : 4.822s

Signed-off-by Jaegeuk Kim <[email protected]>

Reviewed-by Chao Yu <[email protected]>

One comment below,

I still have a comment on this patch.

Quoted 10 lines

Quoted 10 lines

(quoted diff) +2 -1

-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
+/* This should be covered by global mutex, &sit_i->sentry_lock */
+void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, unsigned int segno)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be, *tmp;

@@ -646,7 +650,15 @@ void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
 		struct bio *bio = be->bio;
 		int err;
 

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Review void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi) · line 655

Quoted 4 lines

(quoted diff) +3 -1

-		wait_for_completion_io(&be->event);
+		if (!completion_done(&be->event)) {
+			if ((be->start_segno >= segno &&
+					be->end_segno <= segno) ||

segno >= be->start_segno && segno < be->end_segno ?

Still can not understand this judgment condition, we should wait completion of discard command only when segno is locate in range of [start_segno, end_segno]?

But now, this condition can be true only when segno, start_segno, end_segno have equal value.

Please correct me if I'm wrong.

From fe24461eedd62815e0c56317f010a3a6e3004434 Mon Sep 17 00:00:00 2001

Hi Chao,

Quoted 44 lines · Chao Yu

Quoted 33 lines · Jaegeuk Kim

Quoted 32 lines · Chao Yu

Quoted 10 lines · Chao Yu

Quoted 9 lines · Jaegeuk Kim

This patch relaxes async discard commands to avoid waiting its end_io during checkpoint.
Instead of waiting them during checkpoint, it will be done when actually reusing them.

Test on initial partition of nvme drive.

 # time fstrim /mnt/test

Quoted 8 lines

Quoted 4 lines

Before : 6.158s
After : 4.822s

Signed-off-by Jaegeuk Kim <[email protected]>

Reviewed-by Chao Yu <[email protected]>

One comment below,

I still have a comment on this patch.

Quoted 10 lines

Quoted 10 lines

fs/f2fs/segment.c +2 -1

-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
+/* This should be covered by global mutex, &sit_i->sentry_lock */
+void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, unsigned int segno)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be, *tmp;

@@ -646,7 +650,15 @@ void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
 		struct bio *bio = be->bio;
 		int err;
 

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

fs/f2fs/segment.c void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)

Quoted 4 lines

fs/f2fs/segment.c +3 -1

-		wait_for_completion_io(&be->event);
+		if (!completion_done(&be->event)) {
+			if ((be->start_segno >= segno &&
+					be->end_segno <= segno) ||

segno >= be->start_segno && segno < be->end_segno ?

Still can not understand this judgment condition, we should wait completion of discard command only when segno is locate in range of [start_segno, end_segno]?

Quoted 4 lines

But now, this condition can be true only when segno, start_segno, end_segno have equal value.

Please correct me if I'm wrong.

Urg. I rewrote it to use block addresses.

How about this?

Hi Chao,

Quoted 41 lines · Chao Yu

Quoted 33 lines · Jaegeuk Kim

Quoted 32 lines · Chao Yu

Quoted 10 lines · Chao Yu

Quoted 9 lines · Jaegeuk Kim

This patch relaxes async discard commands to avoid waiting its end_io during checkpoint.
Instead of waiting them during checkpoint, it will be done when actually reusing them.

Test on initial partition of nvme drive.

 # time fstrim /mnt/test

Quoted 8 lines

Quoted 4 lines

Before : 6.158s
After : 4.822s

Signed-off-by Jaegeuk Kim <[email protected]>

Reviewed-by Chao Yu <[email protected]>

One comment below,

I still have a comment on this patch.

Quoted 10 lines

Quoted 10 lines

fs/f2fs/segment.c +2 -1

-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
+/* This should be covered by global mutex, &sit_i->sentry_lock */
+void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, unsigned int segno)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be, *tmp;

@@ -646,7 +650,15 @@ void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
 		struct bio *bio = be->bio;
 		int err;
 

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

fs/f2fs/segment.c void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)

Quoted 4 lines

fs/f2fs/segment.c +3 -1

-		wait_for_completion_io(&be->event);
+		if (!completion_done(&be->event)) {
+			if ((be->start_segno >= segno &&
+					be->end_segno <= segno) ||

segno >= be->start_segno && segno < be->end_segno ?

Quoted 7 lines

Still can not understand this judgment condition, we should wait completion of discard command only when segno is locate in range of [start_segno, end_segno]?

But now, this condition can be true only when segno, start_segno, end_segno have equal value.

Please correct me if I'm wrong.

Urg. I rewrote it to use block addresses.

How about this?

Quoted 1 line

From fe24461eedd62815e0c56317f010a3a6e3004434 Mon Sep 17 00:00:00 2001

From: Jaegeuk Kim <[email protected]>
Date: Thu, 29 Dec 2016 14:07:53 -0800
Subject: [PATCH] f2fs: relax async discard commands more

This patch relaxes async discard commands to avoid waiting its end_io during checkpoint.
Instead of waiting them during checkpoint, it will be done when actually reusing them.

Test on initial partition of nvme drive.

 # time fstrim /mnt/test

Before : 6.158s
After : 4.822s

Reviewed-by Chao Yu <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/checkpoint.c |  7 ++-----
 fs/f2fs/f2fs.h       |  4 +++-
 fs/f2fs/segment.c    | 23 +++++++++++++++++++----
 fs/f2fs/super.c      |  3 +++
 4 files changed, 27 insertions(+), 10 deletions(-)

fs/f2fs/checkpoint.c +2 -5

index f73ee9534d83..1a9ba69a22ba 100644
@@ -1254,7 +1254,6 @@ int write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 		f2fs_bug_on(sbi, prefree_segments(sbi));
 		flush_sit_entries(sbi, cpc);
 		clear_prefree_segments(sbi, cpc);
-		f2fs_wait_all_discard_bio(sbi);
 		unblock_operations(sbi);
 		goto out;
 	}

@@ -1273,12 +1272,10 @@ int write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 
 	/* unlock all the fs_lock[] in do_checkpoint() */
 	err = do_checkpoint(sbi, cpc);
-	if (err) {
+	if (err)
 		release_discard_addrs(sbi);
-	} else {
+	else
 		clear_prefree_segments(sbi, cpc);
-		f2fs_wait_all_discard_bio(sbi);
-	}
 
 	unblock_operations(sbi);
 	stat_inc_cp_count(sbi->stat_info);

fs/f2fs/f2fs.h +3 -1

index bdcfe2a9b532..e0db895fd84c 100644
@@ -183,6 +183,8 @@ struct discard_entry {
 
 struct bio_entry {
 	struct list_head list;
+	block_t lstart;
+	block_t len;
 	struct bio *bio;
 	struct completion event;
 	int error;

@@ -2111,7 +2113,7 @@ void destroy_flush_cmd_control(struct f2fs_sb_info *, bool);
 void invalidate_blocks(struct f2fs_sb_info *, block_t);
 bool is_checkpointed_data(struct f2fs_sb_info *, block_t);
 void refresh_sit_entry(struct f2fs_sb_info *, block_t, block_t);
-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *);
+void f2fs_wait_discard_bio(struct f2fs_sb_info *, block_t);
 void clear_prefree_segments(struct f2fs_sb_info *, struct cp_control *);
 void release_discard_addrs(struct f2fs_sb_info *);
 int npages_for_summary_flush(struct f2fs_sb_info *, bool);

fs/f2fs/segment.c +19 -4

index 485b9118b418..672bcdefe836 100644
@@ -625,20 +625,23 @@ static void locate_dirty_segment(struct f2fs_sb_info *sbi, unsigned int segno)
 }
 
 static struct bio_entry *__add_bio_entry(struct f2fs_sb_info *sbi,
-							struct bio *bio)
+			struct bio *bio, block_t lstart, block_t len)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be = f2fs_kmem_cache_alloc(bio_entry_slab, GFP_NOFS);
 
 	INIT_LIST_HEAD(&be->list);
 	be->bio = bio;
+	be->lstart = lstart;
+	be->len = len;
 	init_completion(&be->event);
 	list_add_tail(&be->list, wait_list);
 
 	return be;
 }
 
-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
+/* This should be covered by global mutex, &sit_i->sentry_lock */
+void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be, *tmp;

@@ -647,7 +650,15 @@ void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
 		struct bio *bio = be->bio;
 		int err;
 
-		wait_for_completion_io(&be->event);
+		if (!completion_done(&be->event)) {
+			if ((be->lstart <= blkaddr &&
+					blkaddr < be->lstart + be->len) ||
+					blkaddr == NULL_ADDR)
+				wait_for_completion_io(&be->event);
+			else
+				continue;
+		}
+
 		err = be->error;
 		if (err == -EOPNOTSUPP)
 			err = 0;

@@ -675,6 +686,7 @@ static int __f2fs_issue_discard_async(struct f2fs_sb_info *sbi,
 		struct block_device *bdev, block_t blkstart, block_t blklen)
 {
 	struct bio *bio = NULL;
+	block_t lblkstart = blkstart;
 	int err;
 
 	trace_f2fs_issue_discard(sbi->sb, blkstart, blklen);

@@ -689,7 +701,8 @@ static int __f2fs_issue_discard_async(struct f2fs_sb_info *sbi,
 				SECTOR_FROM_BLOCK(blklen),
 				GFP_NOFS, 0, &bio);
 	if (!err && bio) {
-		struct bio_entry *be = __add_bio_entry(sbi, bio);
+		struct bio_entry *be = __add_bio_entry(sbi, bio,
+						lblkstart, blklen);
 
 		bio->bi_private = be;
 		bio->bi_end_io = f2fs_submit_bio_wait_endio;

@@ -1575,6 +1588,8 @@ void allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
 
 	*new_blkaddr = NEXT_FREE_BLKADDR(sbi, curseg);
 
+	f2fs_wait_discard_bio(sbi, *new_blkaddr);
+
 	/*
 	 * __add_sum_entry should be resided under the curseg_mutex
 	 * because, this function updates a summary entry in the

fs/f2fs/super.c +3 -0

index f3697f97e527..461c29043aec 100644
@@ -770,6 +770,9 @@ static void f2fs_put_super(struct super_block *sb)
 		write_checkpoint(sbi, &cpc);
 	}
 
+	/* be sure to wait for any on-going discard commands */
+	f2fs_wait_discard_bio(sbi, NULL_ADDR);
+
 	/* write_checkpoint can update stat informaion */
 	f2fs_destroy_stats(sbi);
 

Quoted 68 lines · Jaegeuk Kim

Hi Chao,

Quoted 41 lines · Chao Yu

Quoted 33 lines · Jaegeuk Kim

Quoted 32 lines · Chao Yu

Quoted 10 lines · Chao Yu

Quoted 9 lines · Jaegeuk Kim

This patch relaxes async discard commands to avoid waiting its end_io during checkpoint.
Instead of waiting them during checkpoint, it will be done when actually reusing them.

Test on initial partition of nvme drive.

 # time fstrim /mnt/test

Quoted 8 lines

Quoted 4 lines

Before : 6.158s
After : 4.822s

Signed-off-by Jaegeuk Kim <[email protected]>

Reviewed-by Chao Yu <[email protected]>

One comment below,

I still have a comment on this patch.

Quoted 10 lines

Quoted 10 lines

fs/f2fs/segment.c +2 -1

-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
+/* This should be covered by global mutex, &sit_i->sentry_lock */
+void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, unsigned int segno)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be, *tmp;

@@ -646,7 +650,15 @@ void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
 		struct bio *bio = be->bio;
 		int err;
 

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

fs/f2fs/segment.c void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)

Quoted 4 lines

fs/f2fs/segment.c +3 -1

-		wait_for_completion_io(&be->event);
+		if (!completion_done(&be->event)) {
+			if ((be->start_segno >= segno &&
+					be->end_segno <= segno) ||

segno >= be->start_segno && segno < be->end_segno ?

Quoted 7 lines

Still can not understand this judgment condition, we should wait completion of discard command only when segno is locate in range of [start_segno, end_segno]?

But now, this condition can be true only when segno, start_segno, end_segno have equal value.

Please correct me if I'm wrong.

Urg. I rewrote it to use block addresses.

How about this?

Quoted 1 line

From fe24461eedd62815e0c56317f010a3a6e3004434 Mon Sep 17 00:00:00 2001

From: Jaegeuk Kim <[email protected]>
Date: Thu, 29 Dec 2016 14:07:53 -0800
Subject: [PATCH] f2fs: relax async discard commands more

This patch relaxes async discard commands to avoid waiting its end_io during checkpoint.
Instead of waiting them during checkpoint, it will be done when actually reusing them.

Test on initial partition of nvme drive.

Quoted 4 lines

 # time fstrim /mnt/test

Before : 6.158s
After : 4.822s

We should wait for completion of all discard IOs in fstrim, since we can't guarantee discard command can be handled successfully by device after we return success to caller. Right?

Thanks,

Quoted 139 lines

Reviewed-by Chao Yu <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/checkpoint.c |  7 ++-----
 fs/f2fs/f2fs.h       |  4 +++-
 fs/f2fs/segment.c    | 23 +++++++++++++++++++----
 fs/f2fs/super.c      |  3 +++
 4 files changed, 27 insertions(+), 10 deletions(-)

fs/f2fs/checkpoint.c +2 -5

index f73ee9534d83..1a9ba69a22ba 100644
@@ -1254,7 +1254,6 @@ int write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 		f2fs_bug_on(sbi, prefree_segments(sbi));
 		flush_sit_entries(sbi, cpc);
 		clear_prefree_segments(sbi, cpc);
-		f2fs_wait_all_discard_bio(sbi);
 		unblock_operations(sbi);
 		goto out;
 	}

@@ -1273,12 +1272,10 @@ int write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 
 	/* unlock all the fs_lock[] in do_checkpoint() */
 	err = do_checkpoint(sbi, cpc);
-	if (err) {
+	if (err)
 		release_discard_addrs(sbi);
-	} else {
+	else
 		clear_prefree_segments(sbi, cpc);
-		f2fs_wait_all_discard_bio(sbi);
-	}
 
 	unblock_operations(sbi);
 	stat_inc_cp_count(sbi->stat_info);

fs/f2fs/f2fs.h +3 -1

index bdcfe2a9b532..e0db895fd84c 100644
@@ -183,6 +183,8 @@ struct discard_entry {
 
 struct bio_entry {
 	struct list_head list;
+	block_t lstart;
+	block_t len;
 	struct bio *bio;
 	struct completion event;
 	int error;

@@ -2111,7 +2113,7 @@ void destroy_flush_cmd_control(struct f2fs_sb_info *, bool);
 void invalidate_blocks(struct f2fs_sb_info *, block_t);
 bool is_checkpointed_data(struct f2fs_sb_info *, block_t);
 void refresh_sit_entry(struct f2fs_sb_info *, block_t, block_t);
-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *);
+void f2fs_wait_discard_bio(struct f2fs_sb_info *, block_t);
 void clear_prefree_segments(struct f2fs_sb_info *, struct cp_control *);
 void release_discard_addrs(struct f2fs_sb_info *);
 int npages_for_summary_flush(struct f2fs_sb_info *, bool);

fs/f2fs/segment.c +19 -4

index 485b9118b418..672bcdefe836 100644
@@ -625,20 +625,23 @@ static void locate_dirty_segment(struct f2fs_sb_info *sbi, unsigned int segno)
 }
 
 static struct bio_entry *__add_bio_entry(struct f2fs_sb_info *sbi,
-							struct bio *bio)
+			struct bio *bio, block_t lstart, block_t len)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be = f2fs_kmem_cache_alloc(bio_entry_slab, GFP_NOFS);
 
 	INIT_LIST_HEAD(&be->list);
 	be->bio = bio;
+	be->lstart = lstart;
+	be->len = len;
 	init_completion(&be->event);
 	list_add_tail(&be->list, wait_list);
 
 	return be;
 }
 
-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
+/* This should be covered by global mutex, &sit_i->sentry_lock */
+void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be, *tmp;

@@ -647,7 +650,15 @@ void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
 		struct bio *bio = be->bio;
 		int err;
 
-		wait_for_completion_io(&be->event);
+		if (!completion_done(&be->event)) {
+			if ((be->lstart <= blkaddr &&
+					blkaddr < be->lstart + be->len) ||
+					blkaddr == NULL_ADDR)
+				wait_for_completion_io(&be->event);
+			else
+				continue;
+		}
+
 		err = be->error;
 		if (err == -EOPNOTSUPP)
 			err = 0;

@@ -675,6 +686,7 @@ static int __f2fs_issue_discard_async(struct f2fs_sb_info *sbi,
 		struct block_device *bdev, block_t blkstart, block_t blklen)
 {
 	struct bio *bio = NULL;
+	block_t lblkstart = blkstart;
 	int err;
 
 	trace_f2fs_issue_discard(sbi->sb, blkstart, blklen);

@@ -689,7 +701,8 @@ static int __f2fs_issue_discard_async(struct f2fs_sb_info *sbi,
 				SECTOR_FROM_BLOCK(blklen),
 				GFP_NOFS, 0, &bio);
 	if (!err && bio) {
-		struct bio_entry *be = __add_bio_entry(sbi, bio);
+		struct bio_entry *be = __add_bio_entry(sbi, bio,
+						lblkstart, blklen);
 
 		bio->bi_private = be;
 		bio->bi_end_io = f2fs_submit_bio_wait_endio;

@@ -1575,6 +1588,8 @@ void allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
 
 	*new_blkaddr = NEXT_FREE_BLKADDR(sbi, curseg);
 
+	f2fs_wait_discard_bio(sbi, *new_blkaddr);
+
 	/*
 	 * __add_sum_entry should be resided under the curseg_mutex
 	 * because, this function updates a summary entry in the

fs/f2fs/super.c +0 -0

index f3697f97e527..461c29043aec 100644

fs/f2fs/super.c static void f2fs_put_super(struct super_block *sb) · line 778

Quoted 9 lines

fs/f2fs/super.c +3 -0

@@ -770,6 +770,9 @@ static void f2fs_put_super(struct super_block *sb)
 		write_checkpoint(sbi, &cpc);
 	}
 
+	/* be sure to wait for any on-going discard commands */
+	f2fs_wait_discard_bio(sbi, NULL_ADDR);
+
 	/* write_checkpoint can update stat informaion */
 	f2fs_destroy_stats(sbi);
 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Quoted 74 lines · Chao Yu

Quoted 72 lines · Jaegeuk Kim

Hi Chao,

Quoted 41 lines · Chao Yu

Quoted 33 lines · Jaegeuk Kim

Quoted 32 lines · Chao Yu

Quoted 10 lines · Chao Yu

Quoted 9 lines · Jaegeuk Kim

This patch relaxes async discard commands to avoid waiting its end_io during checkpoint.
Instead of waiting them during checkpoint, it will be done when actually reusing them.

Test on initial partition of nvme drive.

 # time fstrim /mnt/test

Quoted 8 lines

Quoted 4 lines

Before : 6.158s
After : 4.822s

Signed-off-by Jaegeuk Kim <[email protected]>

Reviewed-by Chao Yu <[email protected]>

One comment below,

I still have a comment on this patch.

Quoted 10 lines

Quoted 10 lines

fs/f2fs/segment.c +2 -1

-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
+/* This should be covered by global mutex, &sit_i->sentry_lock */
+void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, unsigned int segno)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be, *tmp;

@@ -646,7 +650,15 @@ void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
 		struct bio *bio = be->bio;
 		int err;
 

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

fs/f2fs/segment.c void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)

Quoted 4 lines

fs/f2fs/segment.c +3 -1

-		wait_for_completion_io(&be->event);
+		if (!completion_done(&be->event)) {
+			if ((be->start_segno >= segno &&
+					be->end_segno <= segno) ||

segno >= be->start_segno && segno < be->end_segno ?

Quoted 7 lines

Still can not understand this judgment condition, we should wait completion of discard command only when segno is locate in range of [start_segno, end_segno]?

But now, this condition can be true only when segno, start_segno, end_segno have equal value.

Please correct me if I'm wrong.

Urg. I rewrote it to use block addresses.

How about this?

Quoted 1 line

From fe24461eedd62815e0c56317f010a3a6e3004434 Mon Sep 17 00:00:00 2001

From: Jaegeuk Kim <[email protected]>
Date: Thu, 29 Dec 2016 14:07:53 -0800
Subject: [PATCH] f2fs: relax async discard commands more

This patch relaxes async discard commands to avoid waiting its end_io during checkpoint.
Instead of waiting them during checkpoint, it will be done when actually reusing them.

Test on initial partition of nvme drive.

 # time fstrim /mnt/test

Before : 6.158s
After : 4.822s

Quoted 3 lines

We should wait for completion of all discard IOs in fstrim, since we can't guarantee discard command can be handled successfully by device after we return success to caller. Right?

Do we have to guarantee? I think trim commands just try to give a hint to flash storage for its healthier status, since it doesn't hurt filesystem consistency and device/filesystem can freely ignore the commands as well.

If we want to guarantee discarding region, we may need to issue BLKSECDISCARD.

Thanks,

Quoted 144 lines

Thanks,

Quoted 141 lines

Reviewed-by Chao Yu <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/checkpoint.c |  7 ++-----
 fs/f2fs/f2fs.h       |  4 +++-
 fs/f2fs/segment.c    | 23 +++++++++++++++++++----
 fs/f2fs/super.c      |  3 +++
 4 files changed, 27 insertions(+), 10 deletions(-)

fs/f2fs/checkpoint.c +2 -5

index f73ee9534d83..1a9ba69a22ba 100644
@@ -1254,7 +1254,6 @@ int write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 		f2fs_bug_on(sbi, prefree_segments(sbi));
 		flush_sit_entries(sbi, cpc);
 		clear_prefree_segments(sbi, cpc);
-		f2fs_wait_all_discard_bio(sbi);
 		unblock_operations(sbi);
 		goto out;
 	}

@@ -1273,12 +1272,10 @@ int write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 
 	/* unlock all the fs_lock[] in do_checkpoint() */
 	err = do_checkpoint(sbi, cpc);
-	if (err) {
+	if (err)
 		release_discard_addrs(sbi);
-	} else {
+	else
 		clear_prefree_segments(sbi, cpc);
-		f2fs_wait_all_discard_bio(sbi);
-	}
 
 	unblock_operations(sbi);
 	stat_inc_cp_count(sbi->stat_info);

fs/f2fs/f2fs.h +3 -1

index bdcfe2a9b532..e0db895fd84c 100644
@@ -183,6 +183,8 @@ struct discard_entry {
 
 struct bio_entry {
 	struct list_head list;
+	block_t lstart;
+	block_t len;
 	struct bio *bio;
 	struct completion event;
 	int error;

@@ -2111,7 +2113,7 @@ void destroy_flush_cmd_control(struct f2fs_sb_info *, bool);
 void invalidate_blocks(struct f2fs_sb_info *, block_t);
 bool is_checkpointed_data(struct f2fs_sb_info *, block_t);
 void refresh_sit_entry(struct f2fs_sb_info *, block_t, block_t);
-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *);
+void f2fs_wait_discard_bio(struct f2fs_sb_info *, block_t);
 void clear_prefree_segments(struct f2fs_sb_info *, struct cp_control *);
 void release_discard_addrs(struct f2fs_sb_info *);
 int npages_for_summary_flush(struct f2fs_sb_info *, bool);

fs/f2fs/segment.c +19 -4

index 485b9118b418..672bcdefe836 100644
@@ -625,20 +625,23 @@ static void locate_dirty_segment(struct f2fs_sb_info *sbi, unsigned int segno)
 }
 
 static struct bio_entry *__add_bio_entry(struct f2fs_sb_info *sbi,
-							struct bio *bio)
+			struct bio *bio, block_t lstart, block_t len)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be = f2fs_kmem_cache_alloc(bio_entry_slab, GFP_NOFS);
 
 	INIT_LIST_HEAD(&be->list);
 	be->bio = bio;
+	be->lstart = lstart;
+	be->len = len;
 	init_completion(&be->event);
 	list_add_tail(&be->list, wait_list);
 
 	return be;
 }
 
-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
+/* This should be covered by global mutex, &sit_i->sentry_lock */
+void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be, *tmp;

@@ -647,7 +650,15 @@ void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
 		struct bio *bio = be->bio;
 		int err;
 
-		wait_for_completion_io(&be->event);
+		if (!completion_done(&be->event)) {
+			if ((be->lstart <= blkaddr &&
+					blkaddr < be->lstart + be->len) ||
+					blkaddr == NULL_ADDR)
+				wait_for_completion_io(&be->event);
+			else
+				continue;
+		}
+
 		err = be->error;
 		if (err == -EOPNOTSUPP)
 			err = 0;

@@ -675,6 +686,7 @@ static int __f2fs_issue_discard_async(struct f2fs_sb_info *sbi,
 		struct block_device *bdev, block_t blkstart, block_t blklen)
 {
 	struct bio *bio = NULL;
+	block_t lblkstart = blkstart;
 	int err;
 
 	trace_f2fs_issue_discard(sbi->sb, blkstart, blklen);

@@ -689,7 +701,8 @@ static int __f2fs_issue_discard_async(struct f2fs_sb_info *sbi,
 				SECTOR_FROM_BLOCK(blklen),
 				GFP_NOFS, 0, &bio);
 	if (!err && bio) {
-		struct bio_entry *be = __add_bio_entry(sbi, bio);
+		struct bio_entry *be = __add_bio_entry(sbi, bio,
+						lblkstart, blklen);
 
 		bio->bi_private = be;
 		bio->bi_end_io = f2fs_submit_bio_wait_endio;

@@ -1575,6 +1588,8 @@ void allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
 
 	*new_blkaddr = NEXT_FREE_BLKADDR(sbi, curseg);
 
+	f2fs_wait_discard_bio(sbi, *new_blkaddr);
+
 	/*
 	 * __add_sum_entry should be resided under the curseg_mutex
 	 * because, this function updates a summary entry in the

fs/f2fs/super.c +0 -0

index f3697f97e527..461c29043aec 100644
@@ -770,6 +770,9 @@ static void f2fs_put_super(struct super_block *sb)
 		write_checkpoint(sbi, &cpc);
 	}
 

Quoted 16 lines

fs/f2fs/super.c static void f2fs_put_super(struct super_block *sb) · line 778

Quoted 7 lines

fs/f2fs/super.c +3 -0

+	/* be sure to wait for any on-going discard commands */
+	f2fs_wait_discard_bio(sbi, NULL_ADDR);
+
 	/* write_checkpoint can update stat informaion */
 	f2fs_destroy_stats(sbi);
 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

.

Quoted 75 lines · Jaegeuk Kim

Quoted 74 lines · Chao Yu

Quoted 72 lines · Jaegeuk Kim

Hi Chao,

Quoted 41 lines · Chao Yu

Quoted 33 lines · Jaegeuk Kim

Quoted 32 lines · Chao Yu

Quoted 10 lines · Chao Yu

Quoted 9 lines · Jaegeuk Kim

This patch relaxes async discard commands to avoid waiting its end_io during checkpoint.
Instead of waiting them during checkpoint, it will be done when actually reusing them.

Test on initial partition of nvme drive.

 # time fstrim /mnt/test

Quoted 8 lines

Quoted 4 lines

Before : 6.158s
After : 4.822s

Signed-off-by Jaegeuk Kim <[email protected]>

Reviewed-by Chao Yu <[email protected]>

One comment below,

I still have a comment on this patch.

Quoted 10 lines

Quoted 10 lines

fs/f2fs/segment.c +2 -1

-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
+/* This should be covered by global mutex, &sit_i->sentry_lock */
+void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, unsigned int segno)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be, *tmp;

@@ -646,7 +650,15 @@ void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
 		struct bio *bio = be->bio;
 		int err;
 

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

fs/f2fs/segment.c void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)

Quoted 4 lines

fs/f2fs/segment.c +3 -1

-		wait_for_completion_io(&be->event);
+		if (!completion_done(&be->event)) {
+			if ((be->start_segno >= segno &&
+					be->end_segno <= segno) ||

segno >= be->start_segno && segno < be->end_segno ?

Quoted 7 lines

Still can not understand this judgment condition, we should wait completion of discard command only when segno is locate in range of [start_segno, end_segno]?

But now, this condition can be true only when segno, start_segno, end_segno have equal value.

Please correct me if I'm wrong.

Urg. I rewrote it to use block addresses.

How about this?

Quoted 1 line

From fe24461eedd62815e0c56317f010a3a6e3004434 Mon Sep 17 00:00:00 2001

From: Jaegeuk Kim <[email protected]>
Date: Thu, 29 Dec 2016 14:07:53 -0800
Subject: [PATCH] f2fs: relax async discard commands more

This patch relaxes async discard commands to avoid waiting its end_io during checkpoint.
Instead of waiting them during checkpoint, it will be done when actually reusing them.

Test on initial partition of nvme drive.

 # time fstrim /mnt/test

Before : 6.158s
After : 4.822s

Quoted 7 lines

Quoted 3 lines

We should wait for completion of all discard IOs in fstrim, since we can't guarantee discard command can be handled successfully by device after we return success to caller. Right?

Do we have to guarantee? I think trim commands just try to give a hint to flash storage for its healthier status, since it doesn't hurt filesystem consistency and device/filesystem can freely ignore the commands as well.

In manual of fstrim/BLKDISCARD, there is no explicit description that we should guarantee that discard ioctl being designed as synchronization, but trim interface in both block layer and most filesystems is designed as synchronization, so what about keep consistent with them now until there is requirement? then we can add a new async trim interface in ioctl.

Quoted 2 lines

If we want to guarantee discarding region, we may need to issue BLKSECDISCARD.

Hmm.. I know that BLKSECDISCARD will trigger erasing operation in lower layer FTL, but now it is also being designed as synchronized interface.

Thanks,

Quoted 164 lines

Thanks,

Quoted 144 lines

Thanks,

Quoted 141 lines

Reviewed-by Chao Yu <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/checkpoint.c |  7 ++-----
 fs/f2fs/f2fs.h       |  4 +++-
 fs/f2fs/segment.c    | 23 +++++++++++++++++++----
 fs/f2fs/super.c      |  3 +++
 4 files changed, 27 insertions(+), 10 deletions(-)

fs/f2fs/checkpoint.c +2 -5

index f73ee9534d83..1a9ba69a22ba 100644
@@ -1254,7 +1254,6 @@ int write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 		f2fs_bug_on(sbi, prefree_segments(sbi));
 		flush_sit_entries(sbi, cpc);
 		clear_prefree_segments(sbi, cpc);
-		f2fs_wait_all_discard_bio(sbi);
 		unblock_operations(sbi);
 		goto out;
 	}

@@ -1273,12 +1272,10 @@ int write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 
 	/* unlock all the fs_lock[] in do_checkpoint() */
 	err = do_checkpoint(sbi, cpc);
-	if (err) {
+	if (err)
 		release_discard_addrs(sbi);
-	} else {
+	else
 		clear_prefree_segments(sbi, cpc);
-		f2fs_wait_all_discard_bio(sbi);
-	}
 
 	unblock_operations(sbi);
 	stat_inc_cp_count(sbi->stat_info);

fs/f2fs/f2fs.h +3 -1

index bdcfe2a9b532..e0db895fd84c 100644
@@ -183,6 +183,8 @@ struct discard_entry {
 
 struct bio_entry {
 	struct list_head list;
+	block_t lstart;
+	block_t len;
 	struct bio *bio;
 	struct completion event;
 	int error;

@@ -2111,7 +2113,7 @@ void destroy_flush_cmd_control(struct f2fs_sb_info *, bool);
 void invalidate_blocks(struct f2fs_sb_info *, block_t);
 bool is_checkpointed_data(struct f2fs_sb_info *, block_t);
 void refresh_sit_entry(struct f2fs_sb_info *, block_t, block_t);
-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *);
+void f2fs_wait_discard_bio(struct f2fs_sb_info *, block_t);
 void clear_prefree_segments(struct f2fs_sb_info *, struct cp_control *);
 void release_discard_addrs(struct f2fs_sb_info *);
 int npages_for_summary_flush(struct f2fs_sb_info *, bool);

fs/f2fs/segment.c +19 -4

index 485b9118b418..672bcdefe836 100644
@@ -625,20 +625,23 @@ static void locate_dirty_segment(struct f2fs_sb_info *sbi, unsigned int segno)
 }
 
 static struct bio_entry *__add_bio_entry(struct f2fs_sb_info *sbi,
-							struct bio *bio)
+			struct bio *bio, block_t lstart, block_t len)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be = f2fs_kmem_cache_alloc(bio_entry_slab, GFP_NOFS);
 
 	INIT_LIST_HEAD(&be->list);
 	be->bio = bio;
+	be->lstart = lstart;
+	be->len = len;
 	init_completion(&be->event);
 	list_add_tail(&be->list, wait_list);
 
 	return be;
 }
 
-void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
+/* This should be covered by global mutex, &sit_i->sentry_lock */
+void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
 {
 	struct list_head *wait_list = &(SM_I(sbi)->wait_list);
 	struct bio_entry *be, *tmp;

@@ -647,7 +650,15 @@ void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
 		struct bio *bio = be->bio;
 		int err;
 
-		wait_for_completion_io(&be->event);
+		if (!completion_done(&be->event)) {
+			if ((be->lstart <= blkaddr &&
+					blkaddr < be->lstart + be->len) ||
+					blkaddr == NULL_ADDR)
+				wait_for_completion_io(&be->event);
+			else
+				continue;
+		}
+
 		err = be->error;
 		if (err == -EOPNOTSUPP)
 			err = 0;

@@ -675,6 +686,7 @@ static int __f2fs_issue_discard_async(struct f2fs_sb_info *sbi,
 		struct block_device *bdev, block_t blkstart, block_t blklen)
 {
 	struct bio *bio = NULL;
+	block_t lblkstart = blkstart;
 	int err;
 
 	trace_f2fs_issue_discard(sbi->sb, blkstart, blklen);

@@ -689,7 +701,8 @@ static int __f2fs_issue_discard_async(struct f2fs_sb_info *sbi,
 				SECTOR_FROM_BLOCK(blklen),
 				GFP_NOFS, 0, &bio);
 	if (!err && bio) {
-		struct bio_entry *be = __add_bio_entry(sbi, bio);
+		struct bio_entry *be = __add_bio_entry(sbi, bio,
+						lblkstart, blklen);
 
 		bio->bi_private = be;
 		bio->bi_end_io = f2fs_submit_bio_wait_endio;

@@ -1575,6 +1588,8 @@ void allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
 
 	*new_blkaddr = NEXT_FREE_BLKADDR(sbi, curseg);
 
+	f2fs_wait_discard_bio(sbi, *new_blkaddr);
+
 	/*
 	 * __add_sum_entry should be resided under the curseg_mutex
 	 * because, this function updates a summary entry in the

fs/f2fs/super.c +0 -0

index f3697f97e527..461c29043aec 100644
@@ -770,6 +770,9 @@ static void f2fs_put_super(struct super_block *sb)
 		write_checkpoint(sbi, &cpc);
 	}
 

Quoted 16 lines

fs/f2fs/super.c static void f2fs_put_super(struct super_block *sb) · line 778

Quoted 7 lines

fs/f2fs/super.c +3 -0

+	/* be sure to wait for any on-going discard commands */
+	f2fs_wait_discard_bio(sbi, NULL_ADDR);
+
 	/* write_checkpoint can update stat informaion */
 	f2fs_destroy_stats(sbi);
 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Quoted 2 lines

.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

The f2fs_trim_fs() doesn't need to do checkpoint if there are newly allocated data blocks only which didn't change the critical checkpoint data such as nat and sit entries.

Signed-off-by Jaegeuk Kim <[email protected]>

If there is no candidate to submit discard command during f2sf_trim_fs, let's return without checkpoint.

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/checkpoint.c |  5 +++++
 fs/f2fs/f2fs.h       |  1 +
 fs/f2fs/segment.c    | 28 +++++++++++++++++++++++-----
 3 files changed, 29 insertions(+), 5 deletions(-)

fs/f2fs/checkpoint.c +5 -0

index 917b5c5053ae..ccea40763d9d 100644
@@ -1249,6 +1249,11 @@ int write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 
 	/* this is the case of multiple fstrims without any changes */
 	if (cpc->reason == CP_DISCARD) {
+		if (!exist_trim_candidates(sbi, cpc)) {
+			unblock_operations(sbi);
+			goto out;
+		}
+
 		if (NM_I(sbi)->dirty_nat_cnt == 0 &&
 				SIT_I(sbi)->dirty_sentries == 0 &&
 				prefree_segments(sbi) == 0) {

fs/f2fs/f2fs.h +1 -0

index f2f40fce9d31..04d113f0d6bf 100644
@@ -2119,6 +2119,7 @@ void release_discard_addrs(struct f2fs_sb_info *);
 int npages_for_summary_flush(struct f2fs_sb_info *, bool);
 void allocate_new_segments(struct f2fs_sb_info *);
 int f2fs_trim_fs(struct f2fs_sb_info *, struct fstrim_range *);
+bool exist_trim_candidates(struct f2fs_sb_info *, struct cp_control *);
 struct page *get_sum_page(struct f2fs_sb_info *, unsigned int);
 void update_meta_page(struct f2fs_sb_info *, void *, block_t);
 void write_meta_page(struct f2fs_sb_info *, struct page *);

fs/f2fs/segment.c +8 -3

index 9a38424c3c1f..82078734f379 100644
@@ -836,7 +836,8 @@ static void __add_discard_entry(struct f2fs_sb_info *sbi,
 	SM_I(sbi)->nr_discards += end - start;
 }
 
-static void add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc)
+static bool add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc,
+							bool check_only)
 {
 	int entries = SIT_VBLOCK_MAP_SIZE / sizeof(unsigned long);
 	int max_blocks = sbi->blocks_per_seg;

@@ -850,12 +851,12 @@ static void add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	int i;
 
 	if (se->valid_blocks == max_blocks || !f2fs_discard_en(sbi))
-		return;
+		return false;
 
 	if (!force) {
 		if (!test_opt(sbi, DISCARD) || !se->valid_blocks ||
 		    SM_I(sbi)->nr_discards >= SM_I(sbi)->max_discards)
-			return;
+			return false;
 	}
 
 	/* SIT_VBLOCK_MAP_SIZE should be multiple of sizeof(unsigned long) */

@@ -873,8 +874,12 @@ static void add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 					&& (end - start) < cpc->trim_minlen)
 			continue;
 
+		if (check_only)
+			return true;
+
 		__add_discard_entry(sbi, cpc, se, start, end);
 	}
+	return false;
 }
 
 void release_discard_addrs(struct f2fs_sb_info *sbi)

fs/f2fs/segment.c +3 -1

+}
+
 int f2fs_trim_fs(struct f2fs_sb_info *sbi, struct fstrim_range *range)
 {
 	__u64 start = F2FS_BYTES_TO_BLK(range->start);

@@ -2251,7 +2269,7 @@ void flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 			/* add discard candidates */
 			if (cpc->reason != CP_DISCARD) {
 				cpc->trim_start = segno;
-				add_discard_addrs(sbi, cpc);
+				add_discard_addrs(sbi, cpc, false);
 			}
 
 			if (to_journal) {

Quoted 4 lines · Jaegeuk Kim

If there is no candidate to submit discard command during f2sf_trim_fs, let's return without checkpoint.

Signed-off-by Jaegeuk Kim <[email protected]>

Reviewed-by Chao Yu <[email protected]>

Please see comments below.

Quoted 77 lines

---
 fs/f2fs/checkpoint.c |  5 +++++
 fs/f2fs/f2fs.h       |  1 +
 fs/f2fs/segment.c    | 28 +++++++++++++++++++++++-----
 3 files changed, 29 insertions(+), 5 deletions(-)

fs/f2fs/checkpoint.c +5 -0

index 917b5c5053ae..ccea40763d9d 100644
@@ -1249,6 +1249,11 @@ int write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 
 	/* this is the case of multiple fstrims without any changes */
 	if (cpc->reason == CP_DISCARD) {
+		if (!exist_trim_candidates(sbi, cpc)) {
+			unblock_operations(sbi);
+			goto out;
+		}
+
 		if (NM_I(sbi)->dirty_nat_cnt == 0 &&
 				SIT_I(sbi)->dirty_sentries == 0 &&
 				prefree_segments(sbi) == 0) {

fs/f2fs/f2fs.h +1 -0

index f2f40fce9d31..04d113f0d6bf 100644
@@ -2119,6 +2119,7 @@ void release_discard_addrs(struct f2fs_sb_info *);
 int npages_for_summary_flush(struct f2fs_sb_info *, bool);
 void allocate_new_segments(struct f2fs_sb_info *);
 int f2fs_trim_fs(struct f2fs_sb_info *, struct fstrim_range *);
+bool exist_trim_candidates(struct f2fs_sb_info *, struct cp_control *);
 struct page *get_sum_page(struct f2fs_sb_info *, unsigned int);
 void update_meta_page(struct f2fs_sb_info *, void *, block_t);
 void write_meta_page(struct f2fs_sb_info *, struct page *);

fs/f2fs/segment.c +8 -3

index 9a38424c3c1f..82078734f379 100644
@@ -836,7 +836,8 @@ static void __add_discard_entry(struct f2fs_sb_info *sbi,
 	SM_I(sbi)->nr_discards += end - start;
 }
 
-static void add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc)
+static bool add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc,
+							bool check_only)
 {
 	int entries = SIT_VBLOCK_MAP_SIZE / sizeof(unsigned long);
 	int max_blocks = sbi->blocks_per_seg;

@@ -850,12 +851,12 @@ static void add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	int i;
 
 	if (se->valid_blocks == max_blocks || !f2fs_discard_en(sbi))
-		return;
+		return false;
 
 	if (!force) {
 		if (!test_opt(sbi, DISCARD) || !se->valid_blocks ||
 		    SM_I(sbi)->nr_discards >= SM_I(sbi)->max_discards)
-			return;
+			return false;
 	}
 
 	/* SIT_VBLOCK_MAP_SIZE should be multiple of sizeof(unsigned long) */

@@ -873,8 +874,12 @@ static void add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 					&& (end - start) < cpc->trim_minlen)
 			continue;
 
+		if (check_only)
+			return true;
+
 		__add_discard_entry(sbi, cpc, se, start, end);
 	}
+	return false;
 }
 
 void release_discard_addrs(struct f2fs_sb_info *sbi)

fs/f2fs/segment.c static const struct segment_allocation default_salloc_ops = { · line 1465

Quoted 6 lines

fs/f2fs/segment.c +3 -0

@@ -1455,6 +1460,19 @@ static const struct segment_allocation default_salloc_ops = {
 	.allocate_segment = allocate_segment_by_default,
 };
 
+bool exist_trim_candidates(struct f2fs_sb_info *sbi, struct cp_control *cpc)
+{
+	__u64 trim_start = cpc->trim_start;

bool has_candidate = false;

fs/f2fs/segment.c static const struct segment_allocation default_salloc_ops = { · line 1470

Quoted 5 lines

fs/f2fs/segment.c +5 -0

+
+	mutex_lock(&SIT_I(sbi)->sentry_lock);
+	for (; trim_start <= cpc->trim_end; trim_start++)
+		if (add_discard_addrs(sbi, cpc, true))
+			break;

for (; cpc->trim_start <= cpc->trim_end; cpc->trim_start++)

	if (add_discard_addrs(sbi, cpc, true)) {
		has_candidate = true;
		break;
	}

cpc->trim_start = trim_start;

fs/f2fs/segment.c static const struct segment_allocation default_salloc_ops = { · line 1473

Quoted 3 lines

fs/f2fs/segment.c +3 -0

+	mutex_unlock(&SIT_I(sbi)->sentry_lock);
+
+	return trim_start <= cpc->trim_end;

return has_candidate;

Thanks,

Quoted 16 lines

fs/f2fs/segment.c +3 -1

+}
+
 int f2fs_trim_fs(struct f2fs_sb_info *sbi, struct fstrim_range *range)
 {
 	__u64 start = F2FS_BYTES_TO_BLK(range->start);

@@ -2251,7 +2269,7 @@ void flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 			/* add discard candidates */
 			if (cpc->reason != CP_DISCARD) {
 				cpc->trim_start = segno;
-				add_discard_addrs(sbi, cpc);
+				add_discard_addrs(sbi, cpc, false);
 			}
 
 			if (to_journal) {

fs/f2fs/segment.c void flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc) · line 2313

Quoted 8 lines

fs/f2fs/segment.c +1 -1

@@ -2289,7 +2307,7 @@ void flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 		__u64 trim_start = cpc->trim_start;
 
 		for (; cpc->trim_start <= cpc->trim_end; cpc->trim_start++)
-			add_discard_addrs(sbi, cpc);
+			add_discard_addrs(sbi, cpc, false);
 
 		cpc->trim_start = trim_start;
 	}

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel