[PATCH v1] f2fs: fix to ensure queued discard commands are properly issued

[PATCH v1] f2fs: fix to ensure queued discard commands are properly issued

[Yohan Joung]

F2FS zone storage requires discard and reset zone for each conventional,
zoned device.
In the current configuration, Discard granularity is set to the zone
size but queuing is inserted into the pend list with a maximum size of the
segment size As a result queued commands cannot be issued.
so we are restorting discard granularity to its original state

Signed-off-by: Yohan Joung <yohan.joung@sk.com>
---
 fs/f2fs/segment.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index c282e8a0a2ec..938bf5144ae8 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2320,10 +2320,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.25.1

Re: [PATCH v1] f2fs: fix to ensure queued discard commands are properly issued

[Chao Yu]

On 2/4/25 22:53, Yohan Joung wrote:
> F2FS zone storage requires discard and reset zone for each conventional,
> zoned device.
> In the current configuration, Discard granularity is set to the zone
> size but queuing is inserted into the pend list with a maximum size of the
> segment size As a result queued commands cannot be issued.
> so we are restorting discard granularity to its original state

It seems commit 4f993264fe29 ("f2fs: introduce discard_unit mount option")
introduced a bug: when we enable discard_unit=section option, it will set
.discard_granularity to BLKS_PER_SEC(), however discard granularity only
supports [1, 512], once section size is not equal to segment size, it will
cause bug. blkzoned feature became the victim since it use
discard_unit=section option by default.

What:           /sys/fs/f2fs/<disk>/discard_granularity
Date:           July 2017
Contact:        "Chao Yu" <yuchao0@huawei.com>
Description:    Controls discard granularity of inner discard thread. Inner thread
                will not issue discards with size that is smaller than granularity.
                The unit size is one block(4KB), now only support configuring
                in range of [1, 512]. Default value is 16.
                For small devices, default value is 1.

What about this?

Subject: [PATCH] f2fs: fix to set .discard_granularity correctly

commit 4f993264fe29 ("f2fs: introduce discard_unit mount option") introduced
a bug, when we enable discard_unit=section option, it will set
.discard_granularity to BLKS_PER_SEC(), however discard granularity only
supports [1, 512], once section size is not equal to segment size, it will
cause issue_discard_thread() in DPOLICY_BG mode will not select discard entry
w/ any granularity to issue.

Fixes: 4f993264fe29 ("f2fs: introduce discard_unit mount option")
Signed-off-by: Yohan Joung <yohan.joung@sk.com>
Signed-off-by: Chao Yu <chao@kernel.org>
---
 fs/f2fs/segment.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 6ebe25eafafa..2b415926641f 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2320,10 +2320,9 @@ 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)
+	if (F2FS_OPTION(sbi).discard_unit == DISCARD_UNIT_SEGMENT ||
+		F2FS_OPTION(sbi).discard_unit == DISCARD_UNIT_SECTION)
 		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: [PATCH v1] f2fs: fix to ensure queued discard commands are properly issued

[Daeho Jeong]

On Mon, Feb 17, 2025 at 4:38 AM Chao Yu <chao@kernel.org> wrote:
>
> On 2/4/25 22:53, Yohan Joung wrote:
> > F2FS zone storage requires discard and reset zone for each conventional,
> > zoned device.
> > In the current configuration, Discard granularity is set to the zone
> > size but queuing is inserted into the pend list with a maximum size of the
> > segment size As a result queued commands cannot be issued.
> > so we are restorting discard granularity to its original state
>
> It seems commit 4f993264fe29 ("f2fs: introduce discard_unit mount option")
> introduced a bug: when we enable discard_unit=section option, it will set
> .discard_granularity to BLKS_PER_SEC(), however discard granularity only
> supports [1, 512], once section size is not equal to segment size, it will
> cause bug. blkzoned feature became the victim since it use
> discard_unit=section option by default.
>
> What:           /sys/fs/f2fs/<disk>/discard_granularity
> Date:           July 2017
> Contact:        "Chao Yu" <yuchao0@huawei.com>
> Description:    Controls discard granularity of inner discard thread. Inner thread
>                 will not issue discards with size that is smaller than granularity.
>                 The unit size is one block(4KB), now only support configuring
>                 in range of [1, 512]. Default value is 16.
>                 For small devices, default value is 1.
>
> What about this?
>
> Subject: [PATCH] f2fs: fix to set .discard_granularity correctly
>
> commit 4f993264fe29 ("f2fs: introduce discard_unit mount option") introduced
> a bug, when we enable discard_unit=section option, it will set
> .discard_granularity to BLKS_PER_SEC(), however discard granularity only
> supports [1, 512], once section size is not equal to segment size, it will
> cause issue_discard_thread() in DPOLICY_BG mode will not select discard entry
> w/ any granularity to issue.
>
> Fixes: 4f993264fe29 ("f2fs: introduce discard_unit mount option")
> Signed-off-by: Yohan Joung <yohan.joung@sk.com>
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>  fs/f2fs/segment.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 6ebe25eafafa..2b415926641f 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2320,10 +2320,9 @@ 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)
> +       if (F2FS_OPTION(sbi).discard_unit == DISCARD_UNIT_SEGMENT ||
> +               F2FS_OPTION(sbi).discard_unit == DISCARD_UNIT_SECTION)
>                 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
>

Reviewed-by: Daeho Jeong <daehojeong@google.com>

>

Re: [PATCH v1] f2fs: fix to ensure queued discard commands are properly issued

[Chao Yu]

On 2/21/25 23:56, Daeho Jeong wrote:
> On Mon, Feb 17, 2025 at 4:38 AM Chao Yu <chao@kernel.org> wrote:
>>
>> On 2/4/25 22:53, Yohan Joung wrote:
>>> F2FS zone storage requires discard and reset zone for each conventional,
>>> zoned device.
>>> In the current configuration, Discard granularity is set to the zone
>>> size but queuing is inserted into the pend list with a maximum size of the
>>> segment size As a result queued commands cannot be issued.
>>> so we are restorting discard granularity to its original state
>>
>> It seems commit 4f993264fe29 ("f2fs: introduce discard_unit mount option")
>> introduced a bug: when we enable discard_unit=section option, it will set
>> .discard_granularity to BLKS_PER_SEC(), however discard granularity only
>> supports [1, 512], once section size is not equal to segment size, it will
>> cause bug. blkzoned feature became the victim since it use
>> discard_unit=section option by default.
>>
>> What:           /sys/fs/f2fs/<disk>/discard_granularity
>> Date:           July 2017
>> Contact:        "Chao Yu" <yuchao0@huawei.com>
>> Description:    Controls discard granularity of inner discard thread. Inner thread
>>                 will not issue discards with size that is smaller than granularity.
>>                 The unit size is one block(4KB), now only support configuring
>>                 in range of [1, 512]. Default value is 16.
>>                 For small devices, default value is 1.
>>
>> What about this?
>>
>> Subject: [PATCH] f2fs: fix to set .discard_granularity correctly
>>
>> commit 4f993264fe29 ("f2fs: introduce discard_unit mount option") introduced
>> a bug, when we enable discard_unit=section option, it will set
>> .discard_granularity to BLKS_PER_SEC(), however discard granularity only
>> supports [1, 512], once section size is not equal to segment size, it will
>> cause issue_discard_thread() in DPOLICY_BG mode will not select discard entry
>> w/ any granularity to issue.
>>
>> Fixes: 4f993264fe29 ("f2fs: introduce discard_unit mount option")
>> Signed-off-by: Yohan Joung <yohan.joung@sk.com>
>> Signed-off-by: Chao Yu <chao@kernel.org>
>> ---
>>  fs/f2fs/segment.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 6ebe25eafafa..2b415926641f 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -2320,10 +2320,9 @@ 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)
>> +       if (F2FS_OPTION(sbi).discard_unit == DISCARD_UNIT_SEGMENT ||
>> +               F2FS_OPTION(sbi).discard_unit == DISCARD_UNIT_SECTION)
>>                 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
>>
> 
> Reviewed-by: Daeho Jeong <daehojeong@google.com>

Thanks for the review, let me send a formal patch.

Thanks,

> 
>>