f2fs: deliver the accumulated 'issued' to __issue_discard_cmd_orderly() to meet the max_requests limit

[PATCH v3] f2fs: deliver the accumulated 'issued' to __issue_discard_cmd_orderly() to meet the max_requests limit

Posted by Yuwei Guan 2 years, 9 months ago

Any of the following scenarios will send more than the number of
max_requests at a time, which will not meet the design of the
max_requests limit.

- Set max_ordered_discard larger than discard_granularity from userspace.
- It is a small size device, discard_granularity can be tuned to 1 in
  f2fs_tuning_parameters().

We need to deliver the accumulated @issued to __issue_discard_cmd_orderly()
to meet the max_requests limit.

BTW, convert the parameter type of @issued in __submit_discard_cmd().

Signed-off-by: Yuwei Guan <Yuwei.Guan@zeekrlife.com>
Cc: Bagas Sanjaya <bagasdotme@gmail.com>
---
 fs/f2fs/segment.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index a9099a754dd2..5268938466f5 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1097,8 +1097,7 @@ static void __update_discard_tree_range(struct f2fs_sb_info *sbi,
 /* this function is copied from blkdev_issue_discard from block/blk-lib.c */
 static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
 						struct discard_policy *dpolicy,
-						struct discard_cmd *dc,
-						unsigned int *issued)
+						struct discard_cmd *dc, int *issued)
 {
 	struct block_device *bdev = dc->bdev;
 	unsigned int max_discard_blocks =
@@ -1379,8 +1378,8 @@ static void __queue_discard_cmd(struct f2fs_sb_info *sbi,
 	mutex_unlock(&SM_I(sbi)->dcc_info->cmd_lock);
 }
 
-static unsigned int __issue_discard_cmd_orderly(struct f2fs_sb_info *sbi,
-					struct discard_policy *dpolicy)
+static void __issue_discard_cmd_orderly(struct f2fs_sb_info *sbi,
+					struct discard_policy *dpolicy, int *issued)
 {
 	struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
 	struct discard_cmd *prev_dc = NULL, *next_dc = NULL;
@@ -1388,7 +1387,6 @@ static unsigned int __issue_discard_cmd_orderly(struct f2fs_sb_info *sbi,
 	struct discard_cmd *dc;
 	struct blk_plug plug;
 	unsigned int pos = dcc->next_pos;
-	unsigned int issued = 0;
 	bool io_interrupted = false;
 
 	mutex_lock(&dcc->cmd_lock);
@@ -1415,9 +1413,9 @@ static unsigned int __issue_discard_cmd_orderly(struct f2fs_sb_info *sbi,
 		}
 
 		dcc->next_pos = dc->lstart + dc->len;
-		err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
+		err = __submit_discard_cmd(sbi, dpolicy, dc, issued);
 
-		if (issued >= dpolicy->max_requests)
+		if (*issued >= dpolicy->max_requests)
 			break;
 next:
 		node = rb_next(&dc->rb_node);
@@ -1433,10 +1431,8 @@ static unsigned int __issue_discard_cmd_orderly(struct f2fs_sb_info *sbi,
 
 	mutex_unlock(&dcc->cmd_lock);
 
-	if (!issued && io_interrupted)
-		issued = -1;
-
-	return issued;
+	if (!(*issued) && io_interrupted)
+		*issued = -1;
 }
 static unsigned int __wait_all_discard_cmd(struct f2fs_sb_info *sbi,
 					struct discard_policy *dpolicy);
@@ -1464,8 +1460,10 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
 		if (i + 1 < dpolicy->granularity)
 			break;
 
-		if (i + 1 < dcc->max_ordered_discard && dpolicy->ordered)
-			return __issue_discard_cmd_orderly(sbi, dpolicy);
+		if (i + 1 < dcc->max_ordered_discard && dpolicy->ordered) {
+			__issue_discard_cmd_orderly(sbi, dpolicy, &issued);
+			return issued;
+		}
 
 		pend_list = &dcc->pend_list[i];
 
-- 
2.34.1

Re: [PATCH v3] f2fs: deliver the accumulated 'issued' to __issue_discard_cmd_orderly() to meet the max_requests limit

Posted by Chao Yu 2 years, 9 months ago

On 2022/12/13 17:34, Yuwei Guan wrote:
> Any of the following scenarios will send more than the number of
> max_requests at a time, which will not meet the design of the
> max_requests limit.
> 
> - Set max_ordered_discard larger than discard_granularity from userspace.
> - It is a small size device, discard_granularity can be tuned to 1 in
>    f2fs_tuning_parameters().
> 
> We need to deliver the accumulated @issued to __issue_discard_cmd_orderly()
> to meet the max_requests limit.
> 
> BTW, convert the parameter type of @issued in __submit_discard_cmd().
> 
> Signed-off-by: Yuwei Guan <Yuwei.Guan@zeekrlife.com>
> Cc: Bagas Sanjaya <bagasdotme@gmail.com>

For the code part, it looks good to me.

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

> ---
>   fs/f2fs/segment.c | 24 +++++++++++-------------
>   1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index a9099a754dd2..5268938466f5 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1097,8 +1097,7 @@ static void __update_discard_tree_range(struct f2fs_sb_info *sbi,
>   /* this function is copied from blkdev_issue_discard from block/blk-lib.c */
>   static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
>   						struct discard_policy *dpolicy,
> -						struct discard_cmd *dc,
> -						unsigned int *issued)
> +						struct discard_cmd *dc, int *issued)
>   {
>   	struct block_device *bdev = dc->bdev;
>   	unsigned int max_discard_blocks =
> @@ -1379,8 +1378,8 @@ static void __queue_discard_cmd(struct f2fs_sb_info *sbi,
>   	mutex_unlock(&SM_I(sbi)->dcc_info->cmd_lock);
>   }
>   
> -static unsigned int __issue_discard_cmd_orderly(struct f2fs_sb_info *sbi,
> -					struct discard_policy *dpolicy)
> +static void __issue_discard_cmd_orderly(struct f2fs_sb_info *sbi,
> +					struct discard_policy *dpolicy, int *issued)
>   {
>   	struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
>   	struct discard_cmd *prev_dc = NULL, *next_dc = NULL;
> @@ -1388,7 +1387,6 @@ static unsigned int __issue_discard_cmd_orderly(struct f2fs_sb_info *sbi,
>   	struct discard_cmd *dc;
>   	struct blk_plug plug;
>   	unsigned int pos = dcc->next_pos;
> -	unsigned int issued = 0;
>   	bool io_interrupted = false;
>   
>   	mutex_lock(&dcc->cmd_lock);
> @@ -1415,9 +1413,9 @@ static unsigned int __issue_discard_cmd_orderly(struct f2fs_sb_info *sbi,
>   		}
>   
>   		dcc->next_pos = dc->lstart + dc->len;
> -		err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> +		err = __submit_discard_cmd(sbi, dpolicy, dc, issued);
>   
> -		if (issued >= dpolicy->max_requests)
> +		if (*issued >= dpolicy->max_requests)
>   			break;
>   next:
>   		node = rb_next(&dc->rb_node);
> @@ -1433,10 +1431,8 @@ static unsigned int __issue_discard_cmd_orderly(struct f2fs_sb_info *sbi,
>   
>   	mutex_unlock(&dcc->cmd_lock);
>   
> -	if (!issued && io_interrupted)
> -		issued = -1;
> -
> -	return issued;
> +	if (!(*issued) && io_interrupted)
> +		*issued = -1;
>   }
>   static unsigned int __wait_all_discard_cmd(struct f2fs_sb_info *sbi,
>   					struct discard_policy *dpolicy);
> @@ -1464,8 +1460,10 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>   		if (i + 1 < dpolicy->granularity)
>   			break;
>   
> -		if (i + 1 < dcc->max_ordered_discard && dpolicy->ordered)
> -			return __issue_discard_cmd_orderly(sbi, dpolicy);
> +		if (i + 1 < dcc->max_ordered_discard && dpolicy->ordered) {
> +			__issue_discard_cmd_orderly(sbi, dpolicy, &issued);
> +			return issued;
> +		}
>   
>   		pend_list = &dcc->pend_list[i];
>

RE: [PATCH v3] f2fs: deliver the accumulated 'issued' to __issue_discard_cmd_orderly() to meet the max_requests limit

Posted by 管玉伟(竹官) 2 years, 8 months ago

> 
> 
> On 2022/12/13 17:34, Yuwei Guan wrote:
> > Any of the following scenarios will send more than the number of
> > max_requests at a time, which will not meet the design of the
> > max_requests limit.
> >
> > - Set max_ordered_discard larger than discard_granularity from userspace.
> > - It is a small size device, discard_granularity can be tuned to 1 in
> >    f2fs_tuning_parameters().
> >
> > We need to deliver the accumulated @issued to
> > __issue_discard_cmd_orderly() to meet the max_requests limit.
> >
> > BTW, convert the parameter type of @issued in __submit_discard_cmd().
> >
> > Signed-off-by: Yuwei Guan <Yuwei.Guan@zeekrlife.com>
> > Cc: Bagas Sanjaya <bagasdotme@gmail.com>
> 
> For the code part, it looks good to me.
> 
> Reviewed-by: Chao Yu <chao@kernel.org>
> 
Hi Jeageuk,

Could you help to review this commit.

Thanks.
> Thanks,
> 
> > ---
> >   fs/f2fs/segment.c | 24 +++++++++++-------------
> >   1 file changed, 11 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index
> > a9099a754dd2..5268938466f5 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -1097,8 +1097,7 @@ static void __update_discard_tree_range(struct
> f2fs_sb_info *sbi,
> >   /* this function is copied from blkdev_issue_discard from block/blk-lib.c
> */
> >   static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> >                                               struct discard_policy *dpolicy,
> > -                                             struct discard_cmd *dc,
> > -                                             unsigned int *issued)
> > +                                             struct discard_cmd *dc,
> > + int *issued)
> >   {
> >       struct block_device *bdev = dc->bdev;
> >       unsigned int max_discard_blocks = @@ -1379,8 +1378,8 @@ static
> > void __queue_discard_cmd(struct f2fs_sb_info *sbi,
> >       mutex_unlock(&SM_I(sbi)->dcc_info->cmd_lock);
> >   }
> >
> > -static unsigned int __issue_discard_cmd_orderly(struct f2fs_sb_info *sbi,
> > -                                     struct discard_policy *dpolicy)
> > +static void __issue_discard_cmd_orderly(struct f2fs_sb_info *sbi,
> > +                                     struct discard_policy *dpolicy,
> > +int *issued)
> >   {
> >       struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> >       struct discard_cmd *prev_dc = NULL, *next_dc = NULL; @@ -1388,7
> > +1387,6 @@ static unsigned int __issue_discard_cmd_orderly(struct
> f2fs_sb_info *sbi,
> >       struct discard_cmd *dc;
> >       struct blk_plug plug;
> >       unsigned int pos = dcc->next_pos;
> > -     unsigned int issued = 0;
> >       bool io_interrupted = false;
> >
> >       mutex_lock(&dcc->cmd_lock);
> > @@ -1415,9 +1413,9 @@ static unsigned int
> __issue_discard_cmd_orderly(struct f2fs_sb_info *sbi,
> >               }
> >
> >               dcc->next_pos = dc->lstart + dc->len;
> > -             err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > +             err = __submit_discard_cmd(sbi, dpolicy, dc, issued);
> >
> > -             if (issued >= dpolicy->max_requests)
> > +             if (*issued >= dpolicy->max_requests)
> >                       break;
> >   next:
> >               node = rb_next(&dc->rb_node); @@ -1433,10 +1431,8 @@
> > static unsigned int __issue_discard_cmd_orderly(struct f2fs_sb_info
> > *sbi,
> >
> >       mutex_unlock(&dcc->cmd_lock);
> >
> > -     if (!issued && io_interrupted)
> > -             issued = -1;
> > -
> > -     return issued;
> > +     if (!(*issued) && io_interrupted)
> > +             *issued = -1;
> >   }
> >   static unsigned int __wait_all_discard_cmd(struct f2fs_sb_info *sbi,
> >                                       struct discard_policy *dpolicy);
> > @@ -1464,8 +1460,10 @@ static int __issue_discard_cmd(struct
> f2fs_sb_info *sbi,
> >               if (i + 1 < dpolicy->granularity)
> >                       break;
> >
> > -             if (i + 1 < dcc->max_ordered_discard && dpolicy->ordered)
> > -                     return __issue_discard_cmd_orderly(sbi, dpolicy);
> > +             if (i + 1 < dcc->max_ordered_discard && dpolicy->ordered) {
> > +                     __issue_discard_cmd_orderly(sbi, dpolicy, &issued);
> > +                     return issued;
> > +             }
> >
> >               pend_list = &dcc->pend_list[i];
> >