1. Patchew
  2. linux
  3. View series

[PATCH] f2fs: do not use granularity control for segment or section unit discard

Daeho Jeong posted 1 patch 9 months, 4 weeks ago 
fs/f2fs/segment.c | 29 ++++++++++++++++++++---------
1 file changed, 20 insertions(+), 9 deletions(-)
[PATCH] f2fs: do not use granularity control for segment or section unit discard
Posted by Daeho Jeong 9 months, 4 weeks ago 
From: Daeho Jeong <daehojeong@google.com>

When we support segment or section unit discard, we should only focus on
how actively we submit discard commands for only one type of size, such
as segment or section. In this case, we don't have to manage smaller
sized discards.

Reported-by: Yohan Joung <yohan.joung@sk.com>
Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
 fs/f2fs/segment.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index c282e8a0a2ec..4316ff7aa0d1 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1661,12 +1661,20 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
 				f2fs_time_over(sbi, UMOUNT_DISCARD_TIMEOUT))
 			break;
 
-		if (i + 1 < dpolicy->granularity)
-			break;
+		/*
+		 * Do not granularity control for segment or section
+		 * unit discard, since we have only one type of discard length.
+		 */
+		if (f2fs_block_unit_discard(sbi)) {
+			if (i + 1 < dpolicy->granularity)
+				break;
 
-		if (i + 1 < dcc->max_ordered_discard && dpolicy->ordered) {
-			__issue_discard_cmd_orderly(sbi, dpolicy, &issued);
-			return issued;
+			if (i + 1 < dcc->max_ordered_discard &&
+					dpolicy->ordered) {
+				__issue_discard_cmd_orderly(sbi, dpolicy,
+						&issued);
+				return issued;
+			}
 		}
 
 		pend_list = &dcc->pend_list[i];
@@ -1701,6 +1709,13 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
 
 		if (issued >= dpolicy->max_requests || io_interrupted)
 			break;
+
+		/*
+		 * We only use the largest discard unit for segment or
+		 * section unit discard.
+		 */
+		if (!f2fs_block_unit_discard(sbi))
+			break;
 	}
 
 	if (dpolicy->type == DPOLICY_UMOUNT && issued) {
@@ -2320,10 +2335,6 @@ static int create_discard_cmd_control(struct f2fs_sb_info *sbi)
 	dcc->discard_granularity = DEFAULT_DISCARD_GRANULARITY;
 	dcc->max_ordered_discard = DEFAULT_MAX_ORDERED_DISCARD_GRANULARITY;
 	dcc->discard_io_aware = DPOLICY_IO_AWARE_ENABLE;
-	if (F2FS_OPTION(sbi).discard_unit == DISCARD_UNIT_SEGMENT)
-		dcc->discard_granularity = BLKS_PER_SEG(sbi);
-	else if (F2FS_OPTION(sbi).discard_unit == DISCARD_UNIT_SECTION)
-		dcc->discard_granularity = BLKS_PER_SEC(sbi);
 
 	INIT_LIST_HEAD(&dcc->entry_list);
 	for (i = 0; i < MAX_PLIST_NUM; i++)
-- 
2.48.1.601.g30ceb7b040-goog
Re: [f2fs-dev] [PATCH] f2fs: do not use granularity control for segment or section unit discard
Posted by Chao Yu 9 months, 4 weeks ago 
On 2025/2/20 23:49, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> When we support segment or section unit discard, we should only focus on
> how actively we submit discard commands for only one type of size, such
> as segment or section. In this case, we don't have to manage smaller
> sized discards.
> 
> Reported-by: Yohan Joung <yohan.joung@sk.com>
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
>   fs/f2fs/segment.c | 29 ++++++++++++++++++++---------
>   1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index c282e8a0a2ec..4316ff7aa0d1 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1661,12 +1661,20 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>   				f2fs_time_over(sbi, UMOUNT_DISCARD_TIMEOUT))
>   			break;
>   
> -		if (i + 1 < dpolicy->granularity)
> -			break;
> +		/*
> +		 * Do not granularity control for segment or section
> +		 * unit discard, since we have only one type of discard length.
> +		 */
> +		if (f2fs_block_unit_discard(sbi)) {
> +			if (i + 1 < dpolicy->granularity)
> +				break;
>   
> -		if (i + 1 < dcc->max_ordered_discard && dpolicy->ordered) {
> -			__issue_discard_cmd_orderly(sbi, dpolicy, &issued);
> -			return issued;
> +			if (i + 1 < dcc->max_ordered_discard &&
> +					dpolicy->ordered) {
> +				__issue_discard_cmd_orderly(sbi, dpolicy,
> +						&issued);
> +				return issued;
> +			}
>   		}
>   
>   		pend_list = &dcc->pend_list[i];
> @@ -1701,6 +1709,13 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>   
>   		if (issued >= dpolicy->max_requests || io_interrupted)
>   			break;
> +
> +		/*
> +		 * We only use the largest discard unit for segment or
> +		 * section unit discard.
> +		 */
> +		if (!f2fs_block_unit_discard(sbi))
> +			break;
>   	}
>   
>   	if (dpolicy->type == DPOLICY_UMOUNT && issued) {
> @@ -2320,10 +2335,6 @@ static int create_discard_cmd_control(struct f2fs_sb_info *sbi)
>   	dcc->discard_granularity = DEFAULT_DISCARD_GRANULARITY;
>   	dcc->max_ordered_discard = DEFAULT_MAX_ORDERED_DISCARD_GRANULARITY;
>   	dcc->discard_io_aware = DPOLICY_IO_AWARE_ENABLE;
> -	if (F2FS_OPTION(sbi).discard_unit == DISCARD_UNIT_SEGMENT)
> -		dcc->discard_granularity = BLKS_PER_SEG(sbi);
> -	else if (F2FS_OPTION(sbi).discard_unit == DISCARD_UNIT_SECTION)
> -		dcc->discard_granularity = BLKS_PER_SEC(sbi);

Hi Daeho,

I think this bug was introduced by commit 4f993264fe29 ("f2fs: introduce
discard_unit mount option"), since it set discard_granularity to section
size incorrectly for discard_unit=section mount option, once section size
is large than segment size, discard_granularity will be larger than 512.

However, w/ current implementation, we only support range of [1, 512] for
discard_granularity parameter, resulting in failing to submitting all
dicards.

So, what do you think of setting discard_granularity to 512 for both
discard_unit=segment and discard_unit=section mount option, as I proposed
in [1]? Then, discard_thread in DPOLICY_BG mode can submit those large-sized
discards.

[1] https://lore.kernel.org/linux-f2fs-devel/53598146-1f01-41ad-980e-9f4b989e81ab@kernel.org/

Thanks,

>   
>   	INIT_LIST_HEAD(&dcc->entry_list);
>   	for (i = 0; i < MAX_PLIST_NUM; i++)
Re: [f2fs-dev] [PATCH] f2fs: do not use granularity control for segment or section unit discard
Posted by Daeho Jeong 9 months, 3 weeks ago 
On Thu, Feb 20, 2025 at 6:19 PM Chao Yu <chao@kernel.org> wrote:
>
> On 2025/2/20 23:49, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@google.com>
> >
> > When we support segment or section unit discard, we should only focus on
> > how actively we submit discard commands for only one type of size, such
> > as segment or section. In this case, we don't have to manage smaller
> > sized discards.
> >
> > Reported-by: Yohan Joung <yohan.joung@sk.com>
> > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > ---
> >   fs/f2fs/segment.c | 29 ++++++++++++++++++++---------
> >   1 file changed, 20 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index c282e8a0a2ec..4316ff7aa0d1 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -1661,12 +1661,20 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> >                               f2fs_time_over(sbi, UMOUNT_DISCARD_TIMEOUT))
> >                       break;
> >
> > -             if (i + 1 < dpolicy->granularity)
> > -                     break;
> > +             /*
> > +              * Do not granularity control for segment or section
> > +              * unit discard, since we have only one type of discard length.
> > +              */
> > +             if (f2fs_block_unit_discard(sbi)) {
> > +                     if (i + 1 < dpolicy->granularity)
> > +                             break;
> >
> > -             if (i + 1 < dcc->max_ordered_discard && dpolicy->ordered) {
> > -                     __issue_discard_cmd_orderly(sbi, dpolicy, &issued);
> > -                     return issued;
> > +                     if (i + 1 < dcc->max_ordered_discard &&
> > +                                     dpolicy->ordered) {
> > +                             __issue_discard_cmd_orderly(sbi, dpolicy,
> > +                                             &issued);
> > +                             return issued;
> > +                     }
> >               }
> >
> >               pend_list = &dcc->pend_list[i];
> > @@ -1701,6 +1709,13 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> >
> >               if (issued >= dpolicy->max_requests || io_interrupted)
> >                       break;
> > +
> > +             /*
> > +              * We only use the largest discard unit for segment or
> > +              * section unit discard.
> > +              */
> > +             if (!f2fs_block_unit_discard(sbi))
> > +                     break;
> >       }
> >
> >       if (dpolicy->type == DPOLICY_UMOUNT && issued) {
> > @@ -2320,10 +2335,6 @@ static int create_discard_cmd_control(struct f2fs_sb_info *sbi)
> >       dcc->discard_granularity = DEFAULT_DISCARD_GRANULARITY;
> >       dcc->max_ordered_discard = DEFAULT_MAX_ORDERED_DISCARD_GRANULARITY;
> >       dcc->discard_io_aware = DPOLICY_IO_AWARE_ENABLE;
> > -     if (F2FS_OPTION(sbi).discard_unit == DISCARD_UNIT_SEGMENT)
> > -             dcc->discard_granularity = BLKS_PER_SEG(sbi);
> > -     else if (F2FS_OPTION(sbi).discard_unit == DISCARD_UNIT_SECTION)
> > -             dcc->discard_granularity = BLKS_PER_SEC(sbi);
>
> Hi Daeho,
>
> I think this bug was introduced by commit 4f993264fe29 ("f2fs: introduce
> discard_unit mount option"), since it set discard_granularity to section
> size incorrectly for discard_unit=section mount option, once section size
> is large than segment size, discard_granularity will be larger than 512.
>
> However, w/ current implementation, we only support range of [1, 512] for
> discard_granularity parameter, resulting in failing to submitting all
> dicards.
>
> So, what do you think of setting discard_granularity to 512 for both
> discard_unit=segment and discard_unit=section mount option, as I proposed
> in [1]? Then, discard_thread in DPOLICY_BG mode can submit those large-sized
> discards.
>
> [1] https://lore.kernel.org/linux-f2fs-devel/53598146-1f01-41ad-980e-9f4b989e81ab@kernel.org/

Yes, it makes sense. Thanks.

>
> Thanks,
>
> >
> >       INIT_LIST_HEAD(&dcc->entry_list);
> >       for (i = 0; i < MAX_PLIST_NUM; i++)
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.