diff mbox series

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

Message ID 20250220154904.2698964-1-daeho43@gmail.com (mailing list archive)
State New
Headers show
Series [f2fs-dev] f2fs: do not use granularity control for segment or section unit discard | expand

Commit Message

Daeho Jeong Feb. 20, 2025, 3:49 p.m. UTC 
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(-)

Comments

Chao Yu Feb. 21, 2025, 2:19 a.m. UTC | #1 
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++)
Daeho Jeong Feb. 21, 2025, 3:54 p.m. UTC | #2 
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++)
>
diff mbox series

Patch

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++)