[PATCH 1/4] f2fs: avoid very large discard command

[PATCH 1/4] f2fs: avoid very large discard command

[Jaegeuk Kim]

This patch adds MAX_DISCARD_BLOCKS() to avoid issuing too much large single
discard command.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/f2fs.h    | 3 ++-
 fs/f2fs/segment.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index d076b94530bc..5f3fe97df055 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -133,7 +133,8 @@ enum {
 		(SM_I(sbi)->trim_sections * (sbi)->segs_per_sec)
 #define BATCHED_TRIM_BLOCKS(sbi)	\
 		(BATCHED_TRIM_SEGMENTS(sbi) << (sbi)->log_blocks_per_seg)
-
+#define MAX_DISCARD_BLOCKS(sbi)						\
+		((1 << (sbi)->log_blocks_per_seg) * (sbi)->segs_per_sec)
 #define DISCARD_ISSUE_RATE	8
 #define DEF_CP_INTERVAL			60	/* 60 secs */
 #define DEF_IDLE_INTERVAL		5	/* 5 secs */
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index fe434cd872b4..567019940e9b 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -886,7 +886,8 @@ static void __add_discard_entry(struct f2fs_sb_info *sbi,
 	if (!list_empty(head)) {
 		last = list_last_entry(head, struct discard_entry, list);
 		if (START_BLOCK(sbi, cpc->trim_start) + start ==
-						last->blkaddr + last->len) {
+				last->blkaddr + last->len &&
+				last->len < MAX_DISCARD_BLOCKS(sbi)) {
 			last->len += end - start;
 			goto done;
 		}
-- 
2.11.0

[PATCH 2/4] f2fs: much larger batched trim_fs job

[Jaegeuk Kim]

We have a kernel thread to issue discard commands, so we can increase the
number of batched discard sections. By default, now it becomes 4GB range.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/f2fs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 5f3fe97df055..8f3171f12fa5 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -128,7 +128,7 @@ enum {
 	CP_DISCARD,
 };
 
-#define DEF_BATCHED_TRIM_SECTIONS	2
+#define DEF_BATCHED_TRIM_SECTIONS	2048
 #define BATCHED_TRIM_SEGMENTS(sbi)	\
 		(SM_I(sbi)->trim_sections * (sbi)->segs_per_sec)
 #define BATCHED_TRIM_BLOCKS(sbi)	\
-- 
2.11.0

[PATCH 3/4] f2fs: wait for discard completion after submission

[Jaegeuk Kim]

We don't need to wait for each discard commands when unmounting the image.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/segment.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 567019940e9b..d76e49c3d1b4 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -676,8 +676,12 @@ void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
 	struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
 	struct list_head *wait_list = &(dcc->discard_cmd_list);
 	struct discard_cmd *dc, *tmp;
+	struct blk_plug plug;
 
 	mutex_lock(&dcc->cmd_lock);
+
+	blk_start_plug(&plug);
+
 	list_for_each_entry_safe(dc, tmp, wait_list, list) {
 
 		if (blkaddr == NULL_ADDR) {
@@ -686,9 +690,6 @@ void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
 				submit_bio(dc->bio);
 				atomic_inc(&dcc->submit_discard);
 			}
-			wait_for_completion_io(&dc->wait);
-
-			__remove_discard_cmd(sbi, dc);
 			continue;
 		}
 
@@ -699,6 +700,15 @@ void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
 				__remove_discard_cmd(sbi, dc);
 		}
 	}
+	blk_finish_plug(&plug);
+
+	/* this comes from f2fs_put_super */
+	if (blkaddr == NULL_ADDR) {
+		list_for_each_entry_safe(dc, tmp, wait_list, list) {
+			wait_for_completion_io(&dc->wait);
+			__remove_discard_cmd(sbi, dc);
+		}
+	}
 	mutex_unlock(&dcc->cmd_lock);
 }
 
-- 
2.11.0

[PATCH 4/4] f2fs: check discard alignment only for SEQWRITE zones

[Jaegeuk Kim]

For converntional zones, we don't need to align discard commands to exact zone
size.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/segment.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index d76e49c3d1b4..6fff1c58b3b1 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -793,24 +793,13 @@ static int __f2fs_issue_discard_async(struct f2fs_sb_info *sbi,
 static int __f2fs_issue_discard_zone(struct f2fs_sb_info *sbi,
 		struct block_device *bdev, block_t blkstart, block_t blklen)
 {
-	sector_t nr_sects = SECTOR_FROM_BLOCK(blklen);
-	sector_t sector;
+	sector_t sector, nr_sects;
 	int devi = 0;
 
 	if (sbi->s_ndevs) {
 		devi = f2fs_target_device_index(sbi, blkstart);
 		blkstart -= FDEV(devi).start_blk;
 	}
-	sector = SECTOR_FROM_BLOCK(blkstart);
-
-	if (sector & (bdev_zone_sectors(bdev) - 1) ||
-	    nr_sects != bdev_zone_sectors(bdev)) {
-		f2fs_msg(sbi->sb, KERN_INFO,
-			"(%d) %s: Unaligned discard attempted (block %x + %x)",
-			devi, sbi->s_ndevs ? FDEV(devi).path: "",
-			blkstart, blklen);
-		return -EIO;
-	}
 
 	/*
 	 * We need to know the type of the zone: for conventional zones,
@@ -825,6 +814,17 @@ static int __f2fs_issue_discard_zone(struct f2fs_sb_info *sbi,
 		return __f2fs_issue_discard_async(sbi, bdev, blkstart, blklen);
 	case BLK_ZONE_TYPE_SEQWRITE_REQ:
 	case BLK_ZONE_TYPE_SEQWRITE_PREF:
+		sector = SECTOR_FROM_BLOCK(blkstart);
+		nr_sects = SECTOR_FROM_BLOCK(blklen);
+
+		if (sector & (bdev_zone_sectors(bdev) - 1) ||
+				nr_sects != bdev_zone_sectors(bdev)) {
+			f2fs_msg(sbi->sb, KERN_INFO,
+				"(%d) %s: Unaligned discard attempted (block %x + %x)",
+				devi, sbi->s_ndevs ? FDEV(devi).path: "",
+				blkstart, blklen);
+			return -EIO;
+		}
 		trace_f2fs_issue_reset_zone(bdev, blkstart);
 		return blkdev_reset_zones(bdev, sector,
 					  nr_sects, GFP_NOFS);
-- 
2.11.0

Re: [PATCH 1/4] f2fs: avoid very large discard command

[Christoph Hellwig]

On Wed, Feb 22, 2017 at 08:28:47PM -0800, Jaegeuk Kim wrote:
> This patch adds MAX_DISCARD_BLOCKS() to avoid issuing too much large single
> discard command.

Needs an explanation in the code on why this number was chosen.
In doubt I suspect it should be a quirk in the driver for the device,
and not something decided by the fs.

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/f2fs.h    | 3 ++-
>  fs/f2fs/segment.c | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index d076b94530bc..5f3fe97df055 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -133,7 +133,8 @@ enum {
>  		(SM_I(sbi)->trim_sections * (sbi)->segs_per_sec)
>  #define BATCHED_TRIM_BLOCKS(sbi)	\
>  		(BATCHED_TRIM_SEGMENTS(sbi) << (sbi)->log_blocks_per_seg)
> -
> +#define MAX_DISCARD_BLOCKS(sbi)						\
> +		((1 << (sbi)->log_blocks_per_seg) * (sbi)->segs_per_sec)
>  #define DISCARD_ISSUE_RATE	8
>  #define DEF_CP_INTERVAL			60	/* 60 secs */
>  #define DEF_IDLE_INTERVAL		5	/* 5 secs */
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index fe434cd872b4..567019940e9b 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -886,7 +886,8 @@ static void __add_discard_entry(struct f2fs_sb_info *sbi,
>  	if (!list_empty(head)) {
>  		last = list_last_entry(head, struct discard_entry, list);
>  		if (START_BLOCK(sbi, cpc->trim_start) + start ==
> -						last->blkaddr + last->len) {
> +				last->blkaddr + last->len &&
> +				last->len < MAX_DISCARD_BLOCKS(sbi)) {
>  			last->len += end - start;
>  			goto done;
>  		}
> -- 
> 2.11.0
> 
---end quoted text---

Re: [PATCH 1/4] f2fs: avoid very large discard command

[Jaegeuk Kim]

On 02/23, Christoph Hellwig wrote:
> On Wed, Feb 22, 2017 at 08:28:47PM -0800, Jaegeuk Kim wrote:
> > This patch adds MAX_DISCARD_BLOCKS() to avoid issuing too much large single
> > discard command.
> 
> Needs an explanation in the code on why this number was chosen.

No need to add a trivial comment, since it's the section size which we can
easily translate it.

> In doubt I suspect it should be a quirk in the driver for the device,
> and not something decided by the fs.

The block allocator checks all the issued bios whether any of them contains
the newly allocated address or not. If one large discard command was issued,
it needs to wait for its completion, even if we want to allocate part of its
address space.

In addition, HM-SMR is required to issue discard/reset in a unit of section
size.

Thanks,

> 
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/f2fs.h    | 3 ++-
> >  fs/f2fs/segment.c | 3 ++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index d076b94530bc..5f3fe97df055 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -133,7 +133,8 @@ enum {
> >  		(SM_I(sbi)->trim_sections * (sbi)->segs_per_sec)
> >  #define BATCHED_TRIM_BLOCKS(sbi)	\
> >  		(BATCHED_TRIM_SEGMENTS(sbi) << (sbi)->log_blocks_per_seg)
> > -
> > +#define MAX_DISCARD_BLOCKS(sbi)						\
> > +		((1 << (sbi)->log_blocks_per_seg) * (sbi)->segs_per_sec)
> >  #define DISCARD_ISSUE_RATE	8
> >  #define DEF_CP_INTERVAL			60	/* 60 secs */
> >  #define DEF_IDLE_INTERVAL		5	/* 5 secs */
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index fe434cd872b4..567019940e9b 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -886,7 +886,8 @@ static void __add_discard_entry(struct f2fs_sb_info *sbi,
> >  	if (!list_empty(head)) {
> >  		last = list_last_entry(head, struct discard_entry, list);
> >  		if (START_BLOCK(sbi, cpc->trim_start) + start ==
> > -						last->blkaddr + last->len) {
> > +				last->blkaddr + last->len &&
> > +				last->len < MAX_DISCARD_BLOCKS(sbi)) {
> >  			last->len += end - start;
> >  			goto done;
> >  		}
> > -- 
> > 2.11.0
> > 
> ---end quoted text---

Re: [f2fs-dev] [PATCH 1/4] f2fs: avoid very large discard command

[Chao Yu]

On 2017/2/23 12:28, Jaegeuk Kim wrote:
> This patch adds MAX_DISCARD_BLOCKS() to avoid issuing too much large single
> discard command.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

This patch set looks good to me, please add reviewed-by into all patches. :)

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