* [PATCH v3] f2fs: fix long latency due to discard during umount
@ 2020-03-30 6:45 Sahitya Tummala
2020-03-30 8:57 ` kbuild test robot
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Sahitya Tummala @ 2020-03-30 6:45 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu, linux-f2fs-devel; +Cc: Sahitya Tummala, linux-kernel
F2FS already has a default timeout of 5 secs for discards that
can be issued during umount, but it can take more than the 5 sec
timeout if the underlying UFS device queue is already full and there
are no more available free tags to be used. In that case, submit_bio()
will wait for the already queued discard requests to complete to get
a free tag, which can potentially take way more than 5 sec.
Fix this by submitting the discard requests with REQ_NOWAIT
flags during umount. This will return -EAGAIN for UFS queue/tag full
scenario without waiting in the context of submit_bio(). The FS can
then handle these requests by retrying again within the stipulated
discard timeout period to avoid long latencies.
Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
---
v3:
-Handle the regression reported by Chao with v2.
-simplify the logic to split the dc with multiple bios incase any bio returns
EAGAIN and retry those new dc within 5 sec timeout.
fs/f2fs/segment.c | 65 +++++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 51 insertions(+), 14 deletions(-)
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index fb3e531..55d18c7 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1029,13 +1029,16 @@ static void f2fs_submit_discard_endio(struct bio *bio)
struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
unsigned long flags;
- dc->error = blk_status_to_errno(bio->bi_status);
-
spin_lock_irqsave(&dc->lock, flags);
+ if (!dc->error)
+ dc->error = blk_status_to_errno(bio->bi_status);
+
dc->bio_ref--;
- if (!dc->bio_ref && dc->state == D_SUBMIT) {
- dc->state = D_DONE;
- complete_all(&dc->wait);
+ if (!dc->bio_ref) {
+ if (dc->error || dc->state == D_SUBMIT) {
+ dc->state = D_DONE;
+ complete_all(&dc->wait);
+ }
}
spin_unlock_irqrestore(&dc->lock, flags);
bio_put(bio);
@@ -1124,10 +1127,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
struct list_head *wait_list = (dpolicy->type == DPOLICY_FSTRIM) ?
&(dcc->fstrim_list) : &(dcc->wait_list);
- int flag = dpolicy->sync ? REQ_SYNC : 0;
+ int flag;
block_t lstart, start, len, total_len;
int err = 0;
+ flag = dpolicy->sync ? REQ_SYNC : 0;
+ flag |= dpolicy->type == DPOLICY_UMOUNT ? REQ_NOWAIT : 0;
+
if (dc->state != D_PREP)
return 0;
@@ -1192,10 +1198,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
dc->bio_ref++;
spin_unlock_irqrestore(&dc->lock, flags);
- atomic_inc(&dcc->queued_discard);
- dc->queued++;
- list_move_tail(&dc->list, wait_list);
-
/* sanity check on discard range */
__check_sit_bitmap(sbi, lstart, lstart + len);
@@ -1203,6 +1205,29 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
bio->bi_end_io = f2fs_submit_discard_endio;
bio->bi_opf |= flag;
submit_bio(bio);
+ if (flag & REQ_NOWAIT) {
+ if (dc->error == -EAGAIN) {
+ spin_lock_irqsave(&dc->lock, flags);
+ dc->len -= len;
+ if (!dc->len) {
+ dc->len = total_len;
+ dc->state = D_PREP;
+ reinit_completion(&dc->wait);
+ } else {
+ dcc->undiscard_blks -= total_len;
+ if (dc->state == D_PARTIAL)
+ dc->state = D_SUBMIT;
+ }
+ err = dc->error;
+ dc->error = 0;
+ spin_unlock_irqrestore(&dc->lock, flags);
+ break;
+ }
+ }
+
+ atomic_inc(&dcc->queued_discard);
+ dc->queued++;
+ list_move_tail(&dc->list, wait_list);
atomic_inc(&dcc->issued_discard);
@@ -1214,8 +1239,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
len = total_len;
}
- if (!err && len)
- __update_discard_tree_range(sbi, bdev, lstart, start, len);
+ if ((!err || err == -EAGAIN) && total_len && dc->start != start)
+ __update_discard_tree_range(sbi, bdev, lstart, start,
+ total_len);
return err;
}
@@ -1470,12 +1496,15 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
struct list_head *pend_list;
struct discard_cmd *dc, *tmp;
struct blk_plug plug;
- int i, issued = 0;
+ int i, err, issued = 0;
bool io_interrupted = false;
+ bool retry;
if (dpolicy->timeout != 0)
f2fs_update_time(sbi, dpolicy->timeout);
+retry:
+ retry = false;
for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
if (dpolicy->timeout != 0 &&
f2fs_time_over(sbi, dpolicy->timeout))
@@ -1509,7 +1538,12 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
break;
}
- __submit_discard_cmd(sbi, dpolicy, dc, &issued);
+ err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
+ if (err == -EAGAIN) {
+ congestion_wait(BLK_RW_ASYNC,
+ DEFAULT_IO_TIMEOUT);
+ retry = true;
+ }
if (issued >= dpolicy->max_requests)
break;
@@ -1522,6 +1556,9 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
break;
}
+ if (retry)
+ goto retry;
+
if (!issued && io_interrupted)
issued = -1;
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v3] f2fs: fix long latency due to discard during umount
2020-03-30 6:45 [PATCH v3] f2fs: fix long latency due to discard during umount Sahitya Tummala
@ 2020-03-30 8:57 ` kbuild test robot
2020-03-30 9:58 ` kbuild test robot
2020-03-31 18:46 ` [f2fs-dev] " Jaegeuk Kim
2 siblings, 0 replies; 19+ messages in thread
From: kbuild test robot @ 2020-03-30 8:57 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 3992 bytes --]
Hi Sahitya,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on v5.6]
[cannot apply to f2fs/dev-test next-20200327]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Sahitya-Tummala/f2fs-fix-long-latency-due-to-discard-during-umount/20200330-151252
base: 7111951b8d4973bda27ff663f2cf18b663d15b48
config: nios2-randconfig-a001-20200329 (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=9.3.0 make.cross ARCH=nios2
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
fs/f2fs/segment.c: In function '__issue_discard_cmd':
>> fs/f2fs/segment.c:1542:7: error: 'DEFAULT_IO_TIMEOUT' undeclared (first use in this function); did you mean 'BLK_DEFAULT_SG_TIMEOUT'?
1542 | DEFAULT_IO_TIMEOUT);
| ^~~~~~~~~~~~~~~~~~
| BLK_DEFAULT_SG_TIMEOUT
fs/f2fs/segment.c:1542:7: note: each undeclared identifier is reported only once for each function it appears in
vim +1542 fs/f2fs/segment.c
1489
1490 static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
1491 struct discard_policy *dpolicy)
1492 {
1493 struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
1494 struct list_head *pend_list;
1495 struct discard_cmd *dc, *tmp;
1496 struct blk_plug plug;
1497 int i, err, issued = 0;
1498 bool io_interrupted = false;
1499 bool retry;
1500
1501 if (dpolicy->timeout != 0)
1502 f2fs_update_time(sbi, dpolicy->timeout);
1503
1504 retry:
1505 retry = false;
1506 for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
1507 if (dpolicy->timeout != 0 &&
1508 f2fs_time_over(sbi, dpolicy->timeout))
1509 break;
1510
1511 if (i + 1 < dpolicy->granularity)
1512 break;
1513
1514 if (i < DEFAULT_DISCARD_GRANULARITY && dpolicy->ordered)
1515 return __issue_discard_cmd_orderly(sbi, dpolicy);
1516
1517 pend_list = &dcc->pend_list[i];
1518
1519 mutex_lock(&dcc->cmd_lock);
1520 if (list_empty(pend_list))
1521 goto next;
1522 if (unlikely(dcc->rbtree_check))
1523 f2fs_bug_on(sbi, !f2fs_check_rb_tree_consistence(sbi,
1524 &dcc->root));
1525 blk_start_plug(&plug);
1526 list_for_each_entry_safe(dc, tmp, pend_list, list) {
1527 f2fs_bug_on(sbi, dc->state != D_PREP);
1528
1529 if (dpolicy->timeout != 0 &&
1530 f2fs_time_over(sbi, dpolicy->timeout))
1531 break;
1532
1533 if (dpolicy->io_aware && i < dpolicy->io_aware_gran &&
1534 !is_idle(sbi, DISCARD_TIME)) {
1535 io_interrupted = true;
1536 break;
1537 }
1538
1539 err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
1540 if (err == -EAGAIN) {
1541 congestion_wait(BLK_RW_ASYNC,
> 1542 DEFAULT_IO_TIMEOUT);
1543 retry = true;
1544 }
1545
1546 if (issued >= dpolicy->max_requests)
1547 break;
1548 }
1549 blk_finish_plug(&plug);
1550 next:
1551 mutex_unlock(&dcc->cmd_lock);
1552
1553 if (issued >= dpolicy->max_requests || io_interrupted)
1554 break;
1555 }
1556
1557 if (retry)
1558 goto retry;
1559
1560 if (!issued && io_interrupted)
1561 issued = -1;
1562
1563 return issued;
1564 }
1565
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 22926 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] f2fs: fix long latency due to discard during umount
2020-03-30 6:45 [PATCH v3] f2fs: fix long latency due to discard during umount Sahitya Tummala
2020-03-30 8:57 ` kbuild test robot
@ 2020-03-30 9:58 ` kbuild test robot
2020-03-31 18:46 ` [f2fs-dev] " Jaegeuk Kim
2 siblings, 0 replies; 19+ messages in thread
From: kbuild test robot @ 2020-03-30 9:58 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 3753 bytes --]
Hi Sahitya,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on v5.6]
[cannot apply to f2fs/dev-test next-20200327]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Sahitya-Tummala/f2fs-fix-long-latency-due-to-discard-during-umount/20200330-151252
base: 7111951b8d4973bda27ff663f2cf18b663d15b48
config: x86_64-randconfig-s0-20200330 (attached as .config)
compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
fs/f2fs/segment.c: In function '__issue_discard_cmd':
>> fs/f2fs/segment.c:1542:7: error: 'DEFAULT_IO_TIMEOUT' undeclared (first use in this function)
DEFAULT_IO_TIMEOUT);
^
fs/f2fs/segment.c:1542:7: note: each undeclared identifier is reported only once for each function it appears in
vim +/DEFAULT_IO_TIMEOUT +1542 fs/f2fs/segment.c
1489
1490 static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
1491 struct discard_policy *dpolicy)
1492 {
1493 struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
1494 struct list_head *pend_list;
1495 struct discard_cmd *dc, *tmp;
1496 struct blk_plug plug;
1497 int i, err, issued = 0;
1498 bool io_interrupted = false;
1499 bool retry;
1500
1501 if (dpolicy->timeout != 0)
1502 f2fs_update_time(sbi, dpolicy->timeout);
1503
1504 retry:
1505 retry = false;
1506 for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
1507 if (dpolicy->timeout != 0 &&
1508 f2fs_time_over(sbi, dpolicy->timeout))
1509 break;
1510
1511 if (i + 1 < dpolicy->granularity)
1512 break;
1513
1514 if (i < DEFAULT_DISCARD_GRANULARITY && dpolicy->ordered)
1515 return __issue_discard_cmd_orderly(sbi, dpolicy);
1516
1517 pend_list = &dcc->pend_list[i];
1518
1519 mutex_lock(&dcc->cmd_lock);
1520 if (list_empty(pend_list))
1521 goto next;
1522 if (unlikely(dcc->rbtree_check))
1523 f2fs_bug_on(sbi, !f2fs_check_rb_tree_consistence(sbi,
1524 &dcc->root));
1525 blk_start_plug(&plug);
1526 list_for_each_entry_safe(dc, tmp, pend_list, list) {
1527 f2fs_bug_on(sbi, dc->state != D_PREP);
1528
1529 if (dpolicy->timeout != 0 &&
1530 f2fs_time_over(sbi, dpolicy->timeout))
1531 break;
1532
1533 if (dpolicy->io_aware && i < dpolicy->io_aware_gran &&
1534 !is_idle(sbi, DISCARD_TIME)) {
1535 io_interrupted = true;
1536 break;
1537 }
1538
1539 err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
1540 if (err == -EAGAIN) {
1541 congestion_wait(BLK_RW_ASYNC,
> 1542 DEFAULT_IO_TIMEOUT);
1543 retry = true;
1544 }
1545
1546 if (issued >= dpolicy->max_requests)
1547 break;
1548 }
1549 blk_finish_plug(&plug);
1550 next:
1551 mutex_unlock(&dcc->cmd_lock);
1552
1553 if (issued >= dpolicy->max_requests || io_interrupted)
1554 break;
1555 }
1556
1557 if (retry)
1558 goto retry;
1559
1560 if (!issued && io_interrupted)
1561 issued = -1;
1562
1563 return issued;
1564 }
1565
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 38257 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] f2fs: fix long latency due to discard during umount
2020-03-30 6:45 [PATCH v3] f2fs: fix long latency due to discard during umount Sahitya Tummala
@ 2020-03-31 18:46 ` Jaegeuk Kim
2020-03-30 9:58 ` kbuild test robot
2020-03-31 18:46 ` [f2fs-dev] " Jaegeuk Kim
2 siblings, 0 replies; 19+ messages in thread
From: Jaegeuk Kim @ 2020-03-31 18:46 UTC (permalink / raw)
To: Sahitya Tummala; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
On 03/30, Sahitya Tummala wrote:
> F2FS already has a default timeout of 5 secs for discards that
> can be issued during umount, but it can take more than the 5 sec
> timeout if the underlying UFS device queue is already full and there
> are no more available free tags to be used. In that case, submit_bio()
> will wait for the already queued discard requests to complete to get
> a free tag, which can potentially take way more than 5 sec.
>
> Fix this by submitting the discard requests with REQ_NOWAIT
> flags during umount. This will return -EAGAIN for UFS queue/tag full
> scenario without waiting in the context of submit_bio(). The FS can
> then handle these requests by retrying again within the stipulated
> discard timeout period to avoid long latencies.
Sorry, Sahitya, but, do we really need to do like this? How about just
controlling # of outstanding discarding bios in __issue_discard_cmd()?
>
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> ---
> v3:
> -Handle the regression reported by Chao with v2.
> -simplify the logic to split the dc with multiple bios incase any bio returns
> EAGAIN and retry those new dc within 5 sec timeout.
>
> fs/f2fs/segment.c | 65 +++++++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 51 insertions(+), 14 deletions(-)
>
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index fb3e531..55d18c7 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1029,13 +1029,16 @@ static void f2fs_submit_discard_endio(struct bio *bio)
> struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> unsigned long flags;
>
> - dc->error = blk_status_to_errno(bio->bi_status);
> -
> spin_lock_irqsave(&dc->lock, flags);
> + if (!dc->error)
> + dc->error = blk_status_to_errno(bio->bi_status);
> +
> dc->bio_ref--;
> - if (!dc->bio_ref && dc->state == D_SUBMIT) {
> - dc->state = D_DONE;
> - complete_all(&dc->wait);
> + if (!dc->bio_ref) {
> + if (dc->error || dc->state == D_SUBMIT) {
> + dc->state = D_DONE;
> + complete_all(&dc->wait);
> + }
> }
> spin_unlock_irqrestore(&dc->lock, flags);
> bio_put(bio);
> @@ -1124,10 +1127,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> struct list_head *wait_list = (dpolicy->type == DPOLICY_FSTRIM) ?
> &(dcc->fstrim_list) : &(dcc->wait_list);
> - int flag = dpolicy->sync ? REQ_SYNC : 0;
> + int flag;
> block_t lstart, start, len, total_len;
> int err = 0;
>
> + flag = dpolicy->sync ? REQ_SYNC : 0;
> + flag |= dpolicy->type == DPOLICY_UMOUNT ? REQ_NOWAIT : 0;
> +
> if (dc->state != D_PREP)
> return 0;
>
> @@ -1192,10 +1198,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> dc->bio_ref++;
> spin_unlock_irqrestore(&dc->lock, flags);
>
> - atomic_inc(&dcc->queued_discard);
> - dc->queued++;
> - list_move_tail(&dc->list, wait_list);
> -
> /* sanity check on discard range */
> __check_sit_bitmap(sbi, lstart, lstart + len);
>
> @@ -1203,6 +1205,29 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> bio->bi_end_io = f2fs_submit_discard_endio;
> bio->bi_opf |= flag;
> submit_bio(bio);
> + if (flag & REQ_NOWAIT) {
> + if (dc->error == -EAGAIN) {
> + spin_lock_irqsave(&dc->lock, flags);
> + dc->len -= len;
> + if (!dc->len) {
> + dc->len = total_len;
> + dc->state = D_PREP;
> + reinit_completion(&dc->wait);
> + } else {
> + dcc->undiscard_blks -= total_len;
> + if (dc->state == D_PARTIAL)
> + dc->state = D_SUBMIT;
> + }
> + err = dc->error;
> + dc->error = 0;
> + spin_unlock_irqrestore(&dc->lock, flags);
> + break;
> + }
> + }
> +
> + atomic_inc(&dcc->queued_discard);
> + dc->queued++;
> + list_move_tail(&dc->list, wait_list);
>
> atomic_inc(&dcc->issued_discard);
>
> @@ -1214,8 +1239,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> len = total_len;
> }
>
> - if (!err && len)
> - __update_discard_tree_range(sbi, bdev, lstart, start, len);
> + if ((!err || err == -EAGAIN) && total_len && dc->start != start)
> + __update_discard_tree_range(sbi, bdev, lstart, start,
> + total_len);
> return err;
> }
>
> @@ -1470,12 +1496,15 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> struct list_head *pend_list;
> struct discard_cmd *dc, *tmp;
> struct blk_plug plug;
> - int i, issued = 0;
> + int i, err, issued = 0;
> bool io_interrupted = false;
> + bool retry;
>
> if (dpolicy->timeout != 0)
> f2fs_update_time(sbi, dpolicy->timeout);
>
> +retry:
> + retry = false;
> for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> if (dpolicy->timeout != 0 &&
> f2fs_time_over(sbi, dpolicy->timeout))
> @@ -1509,7 +1538,12 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> break;
> }
>
> - __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> + err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> + if (err == -EAGAIN) {
> + congestion_wait(BLK_RW_ASYNC,
> + DEFAULT_IO_TIMEOUT);
> + retry = true;
> + }
>
> if (issued >= dpolicy->max_requests)
> break;
> @@ -1522,6 +1556,9 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> break;
> }
>
> + if (retry)
> + goto retry;
> +
> if (!issued && io_interrupted)
> issued = -1;
>
> --
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: fix long latency due to discard during umount
@ 2020-03-31 18:46 ` Jaegeuk Kim
0 siblings, 0 replies; 19+ messages in thread
From: Jaegeuk Kim @ 2020-03-31 18:46 UTC (permalink / raw)
To: Sahitya Tummala; +Cc: linux-kernel, linux-f2fs-devel
On 03/30, Sahitya Tummala wrote:
> F2FS already has a default timeout of 5 secs for discards that
> can be issued during umount, but it can take more than the 5 sec
> timeout if the underlying UFS device queue is already full and there
> are no more available free tags to be used. In that case, submit_bio()
> will wait for the already queued discard requests to complete to get
> a free tag, which can potentially take way more than 5 sec.
>
> Fix this by submitting the discard requests with REQ_NOWAIT
> flags during umount. This will return -EAGAIN for UFS queue/tag full
> scenario without waiting in the context of submit_bio(). The FS can
> then handle these requests by retrying again within the stipulated
> discard timeout period to avoid long latencies.
Sorry, Sahitya, but, do we really need to do like this? How about just
controlling # of outstanding discarding bios in __issue_discard_cmd()?
>
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> ---
> v3:
> -Handle the regression reported by Chao with v2.
> -simplify the logic to split the dc with multiple bios incase any bio returns
> EAGAIN and retry those new dc within 5 sec timeout.
>
> fs/f2fs/segment.c | 65 +++++++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 51 insertions(+), 14 deletions(-)
>
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index fb3e531..55d18c7 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1029,13 +1029,16 @@ static void f2fs_submit_discard_endio(struct bio *bio)
> struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> unsigned long flags;
>
> - dc->error = blk_status_to_errno(bio->bi_status);
> -
> spin_lock_irqsave(&dc->lock, flags);
> + if (!dc->error)
> + dc->error = blk_status_to_errno(bio->bi_status);
> +
> dc->bio_ref--;
> - if (!dc->bio_ref && dc->state == D_SUBMIT) {
> - dc->state = D_DONE;
> - complete_all(&dc->wait);
> + if (!dc->bio_ref) {
> + if (dc->error || dc->state == D_SUBMIT) {
> + dc->state = D_DONE;
> + complete_all(&dc->wait);
> + }
> }
> spin_unlock_irqrestore(&dc->lock, flags);
> bio_put(bio);
> @@ -1124,10 +1127,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> struct list_head *wait_list = (dpolicy->type == DPOLICY_FSTRIM) ?
> &(dcc->fstrim_list) : &(dcc->wait_list);
> - int flag = dpolicy->sync ? REQ_SYNC : 0;
> + int flag;
> block_t lstart, start, len, total_len;
> int err = 0;
>
> + flag = dpolicy->sync ? REQ_SYNC : 0;
> + flag |= dpolicy->type == DPOLICY_UMOUNT ? REQ_NOWAIT : 0;
> +
> if (dc->state != D_PREP)
> return 0;
>
> @@ -1192,10 +1198,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> dc->bio_ref++;
> spin_unlock_irqrestore(&dc->lock, flags);
>
> - atomic_inc(&dcc->queued_discard);
> - dc->queued++;
> - list_move_tail(&dc->list, wait_list);
> -
> /* sanity check on discard range */
> __check_sit_bitmap(sbi, lstart, lstart + len);
>
> @@ -1203,6 +1205,29 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> bio->bi_end_io = f2fs_submit_discard_endio;
> bio->bi_opf |= flag;
> submit_bio(bio);
> + if (flag & REQ_NOWAIT) {
> + if (dc->error == -EAGAIN) {
> + spin_lock_irqsave(&dc->lock, flags);
> + dc->len -= len;
> + if (!dc->len) {
> + dc->len = total_len;
> + dc->state = D_PREP;
> + reinit_completion(&dc->wait);
> + } else {
> + dcc->undiscard_blks -= total_len;
> + if (dc->state == D_PARTIAL)
> + dc->state = D_SUBMIT;
> + }
> + err = dc->error;
> + dc->error = 0;
> + spin_unlock_irqrestore(&dc->lock, flags);
> + break;
> + }
> + }
> +
> + atomic_inc(&dcc->queued_discard);
> + dc->queued++;
> + list_move_tail(&dc->list, wait_list);
>
> atomic_inc(&dcc->issued_discard);
>
> @@ -1214,8 +1239,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> len = total_len;
> }
>
> - if (!err && len)
> - __update_discard_tree_range(sbi, bdev, lstart, start, len);
> + if ((!err || err == -EAGAIN) && total_len && dc->start != start)
> + __update_discard_tree_range(sbi, bdev, lstart, start,
> + total_len);
> return err;
> }
>
> @@ -1470,12 +1496,15 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> struct list_head *pend_list;
> struct discard_cmd *dc, *tmp;
> struct blk_plug plug;
> - int i, issued = 0;
> + int i, err, issued = 0;
> bool io_interrupted = false;
> + bool retry;
>
> if (dpolicy->timeout != 0)
> f2fs_update_time(sbi, dpolicy->timeout);
>
> +retry:
> + retry = false;
> for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> if (dpolicy->timeout != 0 &&
> f2fs_time_over(sbi, dpolicy->timeout))
> @@ -1509,7 +1538,12 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> break;
> }
>
> - __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> + err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> + if (err == -EAGAIN) {
> + congestion_wait(BLK_RW_ASYNC,
> + DEFAULT_IO_TIMEOUT);
> + retry = true;
> + }
>
> if (issued >= dpolicy->max_requests)
> break;
> @@ -1522,6 +1556,9 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> break;
> }
>
> + if (retry)
> + goto retry;
> +
> if (!issued && io_interrupted)
> issued = -1;
>
> --
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] f2fs: fix long latency due to discard during umount
2020-03-31 18:46 ` [f2fs-dev] " Jaegeuk Kim
@ 2020-04-01 9:22 ` Sahitya Tummala
-1 siblings, 0 replies; 19+ messages in thread
From: Sahitya Tummala @ 2020-04-01 9:22 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel, stummala
Hi Jaegeuk,
On Tue, Mar 31, 2020 at 11:46:55AM -0700, Jaegeuk Kim wrote:
> On 03/30, Sahitya Tummala wrote:
> > F2FS already has a default timeout of 5 secs for discards that
> > can be issued during umount, but it can take more than the 5 sec
> > timeout if the underlying UFS device queue is already full and there
> > are no more available free tags to be used. In that case, submit_bio()
> > will wait for the already queued discard requests to complete to get
> > a free tag, which can potentially take way more than 5 sec.
> >
> > Fix this by submitting the discard requests with REQ_NOWAIT
> > flags during umount. This will return -EAGAIN for UFS queue/tag full
> > scenario without waiting in the context of submit_bio(). The FS can
> > then handle these requests by retrying again within the stipulated
> > discard timeout period to avoid long latencies.
>
> Sorry, Sahitya, but, do we really need to do like this? How about just
> controlling # of outstanding discarding bios in __issue_discard_cmd()?
Do you mean something like this?
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 1a62b27..860dd43 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1099,7 +1099,7 @@ static void __init_discard_policy(struct f2fs_sb_info *sbi,
} else if (discard_type == DPOLICY_FSTRIM) {
dpolicy->io_aware = false;
} else if (discard_type == DPOLICY_UMOUNT) {
- dpolicy->max_requests = UINT_MAX;
+ dpolicy->max_requests = 30;
dpolicy->io_aware = false;
/* we need to issue all to keep CP_TRIMMED_FLAG */
dpolicy->granularity = 1;
@@ -1470,12 +1470,14 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
struct list_head *pend_list;
struct discard_cmd *dc, *tmp;
struct blk_plug plug;
- int i, issued = 0;
+ int i, issued;
bool io_interrupted = false;
if (dpolicy->timeout != 0)
f2fs_update_time(sbi, dpolicy->timeout);
+retry:
+ issued = 0;
for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
if (dpolicy->timeout != 0 &&
f2fs_time_over(sbi, dpolicy->timeout))
@@ -1522,6 +1524,11 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
break;
}
+ if (dpolicy->type == DPOLICY_UMOUNT && issued) {
+ __wait_all_discard_cmd(sbi, dpolicy);
+ goto retry;
+ }
+
if (!issued && io_interrupted)
issued = -1;
Thanks,
>
> >
> > Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> > ---
> > v3:
> > -Handle the regression reported by Chao with v2.
> > -simplify the logic to split the dc with multiple bios incase any bio returns
> > EAGAIN and retry those new dc within 5 sec timeout.
> >
> > fs/f2fs/segment.c | 65 +++++++++++++++++++++++++++++++++++++++++++------------
> > 1 file changed, 51 insertions(+), 14 deletions(-)
> >
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index fb3e531..55d18c7 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -1029,13 +1029,16 @@ static void f2fs_submit_discard_endio(struct bio *bio)
> > struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> > unsigned long flags;
> >
> > - dc->error = blk_status_to_errno(bio->bi_status);
> > -
> > spin_lock_irqsave(&dc->lock, flags);
> > + if (!dc->error)
> > + dc->error = blk_status_to_errno(bio->bi_status);
> > +
> > dc->bio_ref--;
> > - if (!dc->bio_ref && dc->state == D_SUBMIT) {
> > - dc->state = D_DONE;
> > - complete_all(&dc->wait);
> > + if (!dc->bio_ref) {
> > + if (dc->error || dc->state == D_SUBMIT) {
> > + dc->state = D_DONE;
> > + complete_all(&dc->wait);
> > + }
> > }
> > spin_unlock_irqrestore(&dc->lock, flags);
> > bio_put(bio);
> > @@ -1124,10 +1127,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> > struct list_head *wait_list = (dpolicy->type == DPOLICY_FSTRIM) ?
> > &(dcc->fstrim_list) : &(dcc->wait_list);
> > - int flag = dpolicy->sync ? REQ_SYNC : 0;
> > + int flag;
> > block_t lstart, start, len, total_len;
> > int err = 0;
> >
> > + flag = dpolicy->sync ? REQ_SYNC : 0;
> > + flag |= dpolicy->type == DPOLICY_UMOUNT ? REQ_NOWAIT : 0;
> > +
> > if (dc->state != D_PREP)
> > return 0;
> >
> > @@ -1192,10 +1198,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > dc->bio_ref++;
> > spin_unlock_irqrestore(&dc->lock, flags);
> >
> > - atomic_inc(&dcc->queued_discard);
> > - dc->queued++;
> > - list_move_tail(&dc->list, wait_list);
> > -
> > /* sanity check on discard range */
> > __check_sit_bitmap(sbi, lstart, lstart + len);
> >
> > @@ -1203,6 +1205,29 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > bio->bi_end_io = f2fs_submit_discard_endio;
> > bio->bi_opf |= flag;
> > submit_bio(bio);
> > + if (flag & REQ_NOWAIT) {
> > + if (dc->error == -EAGAIN) {
> > + spin_lock_irqsave(&dc->lock, flags);
> > + dc->len -= len;
> > + if (!dc->len) {
> > + dc->len = total_len;
> > + dc->state = D_PREP;
> > + reinit_completion(&dc->wait);
> > + } else {
> > + dcc->undiscard_blks -= total_len;
> > + if (dc->state == D_PARTIAL)
> > + dc->state = D_SUBMIT;
> > + }
> > + err = dc->error;
> > + dc->error = 0;
> > + spin_unlock_irqrestore(&dc->lock, flags);
> > + break;
> > + }
> > + }
> > +
> > + atomic_inc(&dcc->queued_discard);
> > + dc->queued++;
> > + list_move_tail(&dc->list, wait_list);
> >
> > atomic_inc(&dcc->issued_discard);
> >
> > @@ -1214,8 +1239,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > len = total_len;
> > }
> >
> > - if (!err && len)
> > - __update_discard_tree_range(sbi, bdev, lstart, start, len);
> > + if ((!err || err == -EAGAIN) && total_len && dc->start != start)
> > + __update_discard_tree_range(sbi, bdev, lstart, start,
> > + total_len);
> > return err;
> > }
> >
> > @@ -1470,12 +1496,15 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > struct list_head *pend_list;
> > struct discard_cmd *dc, *tmp;
> > struct blk_plug plug;
> > - int i, issued = 0;
> > + int i, err, issued = 0;
> > bool io_interrupted = false;
> > + bool retry;
> >
> > if (dpolicy->timeout != 0)
> > f2fs_update_time(sbi, dpolicy->timeout);
> >
> > +retry:
> > + retry = false;
> > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > if (dpolicy->timeout != 0 &&
> > f2fs_time_over(sbi, dpolicy->timeout))
> > @@ -1509,7 +1538,12 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > break;
> > }
> >
> > - __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > + err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > + if (err == -EAGAIN) {
> > + congestion_wait(BLK_RW_ASYNC,
> > + DEFAULT_IO_TIMEOUT);
> > + retry = true;
> > + }
> >
> > if (issued >= dpolicy->max_requests)
> > break;
> > @@ -1522,6 +1556,9 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > break;
> > }
> >
> > + if (retry)
> > + goto retry;
> > +
> > if (!issued && io_interrupted)
> > issued = -1;
> >
> > --
> > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
--
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: fix long latency due to discard during umount
@ 2020-04-01 9:22 ` Sahitya Tummala
0 siblings, 0 replies; 19+ messages in thread
From: Sahitya Tummala @ 2020-04-01 9:22 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel
Hi Jaegeuk,
On Tue, Mar 31, 2020 at 11:46:55AM -0700, Jaegeuk Kim wrote:
> On 03/30, Sahitya Tummala wrote:
> > F2FS already has a default timeout of 5 secs for discards that
> > can be issued during umount, but it can take more than the 5 sec
> > timeout if the underlying UFS device queue is already full and there
> > are no more available free tags to be used. In that case, submit_bio()
> > will wait for the already queued discard requests to complete to get
> > a free tag, which can potentially take way more than 5 sec.
> >
> > Fix this by submitting the discard requests with REQ_NOWAIT
> > flags during umount. This will return -EAGAIN for UFS queue/tag full
> > scenario without waiting in the context of submit_bio(). The FS can
> > then handle these requests by retrying again within the stipulated
> > discard timeout period to avoid long latencies.
>
> Sorry, Sahitya, but, do we really need to do like this? How about just
> controlling # of outstanding discarding bios in __issue_discard_cmd()?
Do you mean something like this?
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 1a62b27..860dd43 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1099,7 +1099,7 @@ static void __init_discard_policy(struct f2fs_sb_info *sbi,
} else if (discard_type == DPOLICY_FSTRIM) {
dpolicy->io_aware = false;
} else if (discard_type == DPOLICY_UMOUNT) {
- dpolicy->max_requests = UINT_MAX;
+ dpolicy->max_requests = 30;
dpolicy->io_aware = false;
/* we need to issue all to keep CP_TRIMMED_FLAG */
dpolicy->granularity = 1;
@@ -1470,12 +1470,14 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
struct list_head *pend_list;
struct discard_cmd *dc, *tmp;
struct blk_plug plug;
- int i, issued = 0;
+ int i, issued;
bool io_interrupted = false;
if (dpolicy->timeout != 0)
f2fs_update_time(sbi, dpolicy->timeout);
+retry:
+ issued = 0;
for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
if (dpolicy->timeout != 0 &&
f2fs_time_over(sbi, dpolicy->timeout))
@@ -1522,6 +1524,11 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
break;
}
+ if (dpolicy->type == DPOLICY_UMOUNT && issued) {
+ __wait_all_discard_cmd(sbi, dpolicy);
+ goto retry;
+ }
+
if (!issued && io_interrupted)
issued = -1;
Thanks,
>
> >
> > Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> > ---
> > v3:
> > -Handle the regression reported by Chao with v2.
> > -simplify the logic to split the dc with multiple bios incase any bio returns
> > EAGAIN and retry those new dc within 5 sec timeout.
> >
> > fs/f2fs/segment.c | 65 +++++++++++++++++++++++++++++++++++++++++++------------
> > 1 file changed, 51 insertions(+), 14 deletions(-)
> >
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index fb3e531..55d18c7 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -1029,13 +1029,16 @@ static void f2fs_submit_discard_endio(struct bio *bio)
> > struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> > unsigned long flags;
> >
> > - dc->error = blk_status_to_errno(bio->bi_status);
> > -
> > spin_lock_irqsave(&dc->lock, flags);
> > + if (!dc->error)
> > + dc->error = blk_status_to_errno(bio->bi_status);
> > +
> > dc->bio_ref--;
> > - if (!dc->bio_ref && dc->state == D_SUBMIT) {
> > - dc->state = D_DONE;
> > - complete_all(&dc->wait);
> > + if (!dc->bio_ref) {
> > + if (dc->error || dc->state == D_SUBMIT) {
> > + dc->state = D_DONE;
> > + complete_all(&dc->wait);
> > + }
> > }
> > spin_unlock_irqrestore(&dc->lock, flags);
> > bio_put(bio);
> > @@ -1124,10 +1127,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> > struct list_head *wait_list = (dpolicy->type == DPOLICY_FSTRIM) ?
> > &(dcc->fstrim_list) : &(dcc->wait_list);
> > - int flag = dpolicy->sync ? REQ_SYNC : 0;
> > + int flag;
> > block_t lstart, start, len, total_len;
> > int err = 0;
> >
> > + flag = dpolicy->sync ? REQ_SYNC : 0;
> > + flag |= dpolicy->type == DPOLICY_UMOUNT ? REQ_NOWAIT : 0;
> > +
> > if (dc->state != D_PREP)
> > return 0;
> >
> > @@ -1192,10 +1198,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > dc->bio_ref++;
> > spin_unlock_irqrestore(&dc->lock, flags);
> >
> > - atomic_inc(&dcc->queued_discard);
> > - dc->queued++;
> > - list_move_tail(&dc->list, wait_list);
> > -
> > /* sanity check on discard range */
> > __check_sit_bitmap(sbi, lstart, lstart + len);
> >
> > @@ -1203,6 +1205,29 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > bio->bi_end_io = f2fs_submit_discard_endio;
> > bio->bi_opf |= flag;
> > submit_bio(bio);
> > + if (flag & REQ_NOWAIT) {
> > + if (dc->error == -EAGAIN) {
> > + spin_lock_irqsave(&dc->lock, flags);
> > + dc->len -= len;
> > + if (!dc->len) {
> > + dc->len = total_len;
> > + dc->state = D_PREP;
> > + reinit_completion(&dc->wait);
> > + } else {
> > + dcc->undiscard_blks -= total_len;
> > + if (dc->state == D_PARTIAL)
> > + dc->state = D_SUBMIT;
> > + }
> > + err = dc->error;
> > + dc->error = 0;
> > + spin_unlock_irqrestore(&dc->lock, flags);
> > + break;
> > + }
> > + }
> > +
> > + atomic_inc(&dcc->queued_discard);
> > + dc->queued++;
> > + list_move_tail(&dc->list, wait_list);
> >
> > atomic_inc(&dcc->issued_discard);
> >
> > @@ -1214,8 +1239,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > len = total_len;
> > }
> >
> > - if (!err && len)
> > - __update_discard_tree_range(sbi, bdev, lstart, start, len);
> > + if ((!err || err == -EAGAIN) && total_len && dc->start != start)
> > + __update_discard_tree_range(sbi, bdev, lstart, start,
> > + total_len);
> > return err;
> > }
> >
> > @@ -1470,12 +1496,15 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > struct list_head *pend_list;
> > struct discard_cmd *dc, *tmp;
> > struct blk_plug plug;
> > - int i, issued = 0;
> > + int i, err, issued = 0;
> > bool io_interrupted = false;
> > + bool retry;
> >
> > if (dpolicy->timeout != 0)
> > f2fs_update_time(sbi, dpolicy->timeout);
> >
> > +retry:
> > + retry = false;
> > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > if (dpolicy->timeout != 0 &&
> > f2fs_time_over(sbi, dpolicy->timeout))
> > @@ -1509,7 +1538,12 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > break;
> > }
> >
> > - __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > + err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > + if (err == -EAGAIN) {
> > + congestion_wait(BLK_RW_ASYNC,
> > + DEFAULT_IO_TIMEOUT);
> > + retry = true;
> > + }
> >
> > if (issued >= dpolicy->max_requests)
> > break;
> > @@ -1522,6 +1556,9 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > break;
> > }
> >
> > + if (retry)
> > + goto retry;
> > +
> > if (!issued && io_interrupted)
> > issued = -1;
> >
> > --
> > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
--
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v3] f2fs: fix long latency due to discard during umount
2020-04-01 9:22 ` [f2fs-dev] " Sahitya Tummala
@ 2020-04-02 9:32 ` Chao Yu
-1 siblings, 0 replies; 19+ messages in thread
From: Chao Yu @ 2020-04-02 9:32 UTC (permalink / raw)
To: Sahitya Tummala, Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel
On 2020/4/1 17:22, Sahitya Tummala wrote:
> Hi Jaegeuk,
>
> On Tue, Mar 31, 2020 at 11:46:55AM -0700, Jaegeuk Kim wrote:
>> On 03/30, Sahitya Tummala wrote:
>>> F2FS already has a default timeout of 5 secs for discards that
>>> can be issued during umount, but it can take more than the 5 sec
>>> timeout if the underlying UFS device queue is already full and there
>>> are no more available free tags to be used. In that case, submit_bio()
>>> will wait for the already queued discard requests to complete to get
>>> a free tag, which can potentially take way more than 5 sec.
>>>
>>> Fix this by submitting the discard requests with REQ_NOWAIT
>>> flags during umount. This will return -EAGAIN for UFS queue/tag full
>>> scenario without waiting in the context of submit_bio(). The FS can
>>> then handle these requests by retrying again within the stipulated
>>> discard timeout period to avoid long latencies.
>>
>> Sorry, Sahitya, but, do we really need to do like this? How about just
>> controlling # of outstanding discarding bios in __issue_discard_cmd()?
>
> Do you mean something like this?
>
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 1a62b27..860dd43 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1099,7 +1099,7 @@ static void __init_discard_policy(struct f2fs_sb_info *sbi,
> } else if (discard_type == DPOLICY_FSTRIM) {
> dpolicy->io_aware = false;
> } else if (discard_type == DPOLICY_UMOUNT) {
> - dpolicy->max_requests = UINT_MAX;
> + dpolicy->max_requests = 30;
8 or 16?
It looks more simple than previous implementation.
Thanks,
> dpolicy->io_aware = false;
> /* we need to issue all to keep CP_TRIMMED_FLAG */
> dpolicy->granularity = 1;
> @@ -1470,12 +1470,14 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> struct list_head *pend_list;
> struct discard_cmd *dc, *tmp;
> struct blk_plug plug;
> - int i, issued = 0;
> + int i, issued;
> bool io_interrupted = false;
>
> if (dpolicy->timeout != 0)
> f2fs_update_time(sbi, dpolicy->timeout);
>
> +retry:
> + issued = 0;
> for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> if (dpolicy->timeout != 0 &&
> f2fs_time_over(sbi, dpolicy->timeout))
> @@ -1522,6 +1524,11 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> break;
> }
>
> + if (dpolicy->type == DPOLICY_UMOUNT && issued) {
> + __wait_all_discard_cmd(sbi, dpolicy);
> + goto retry;
> + }
> +
> if (!issued && io_interrupted)
> issued = -1;
>
> Thanks,
>
>>
>>>
>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>> ---
>>> v3:
>>> -Handle the regression reported by Chao with v2.
>>> -simplify the logic to split the dc with multiple bios incase any bio returns
>>> EAGAIN and retry those new dc within 5 sec timeout.
>>>
>>> fs/f2fs/segment.c | 65 +++++++++++++++++++++++++++++++++++++++++++------------
>>> 1 file changed, 51 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index fb3e531..55d18c7 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -1029,13 +1029,16 @@ static void f2fs_submit_discard_endio(struct bio *bio)
>>> struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
>>> unsigned long flags;
>>>
>>> - dc->error = blk_status_to_errno(bio->bi_status);
>>> -
>>> spin_lock_irqsave(&dc->lock, flags);
>>> + if (!dc->error)
>>> + dc->error = blk_status_to_errno(bio->bi_status);
>>> +
>>> dc->bio_ref--;
>>> - if (!dc->bio_ref && dc->state == D_SUBMIT) {
>>> - dc->state = D_DONE;
>>> - complete_all(&dc->wait);
>>> + if (!dc->bio_ref) {
>>> + if (dc->error || dc->state == D_SUBMIT) {
>>> + dc->state = D_DONE;
>>> + complete_all(&dc->wait);
>>> + }
>>> }
>>> spin_unlock_irqrestore(&dc->lock, flags);
>>> bio_put(bio);
>>> @@ -1124,10 +1127,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
>>> struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
>>> struct list_head *wait_list = (dpolicy->type == DPOLICY_FSTRIM) ?
>>> &(dcc->fstrim_list) : &(dcc->wait_list);
>>> - int flag = dpolicy->sync ? REQ_SYNC : 0;
>>> + int flag;
>>> block_t lstart, start, len, total_len;
>>> int err = 0;
>>>
>>> + flag = dpolicy->sync ? REQ_SYNC : 0;
>>> + flag |= dpolicy->type == DPOLICY_UMOUNT ? REQ_NOWAIT : 0;
>>> +
>>> if (dc->state != D_PREP)
>>> return 0;
>>>
>>> @@ -1192,10 +1198,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
>>> dc->bio_ref++;
>>> spin_unlock_irqrestore(&dc->lock, flags);
>>>
>>> - atomic_inc(&dcc->queued_discard);
>>> - dc->queued++;
>>> - list_move_tail(&dc->list, wait_list);
>>> -
>>> /* sanity check on discard range */
>>> __check_sit_bitmap(sbi, lstart, lstart + len);
>>>
>>> @@ -1203,6 +1205,29 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
>>> bio->bi_end_io = f2fs_submit_discard_endio;
>>> bio->bi_opf |= flag;
>>> submit_bio(bio);
>>> + if (flag & REQ_NOWAIT) {
>>> + if (dc->error == -EAGAIN) {
>>> + spin_lock_irqsave(&dc->lock, flags);
>>> + dc->len -= len;
>>> + if (!dc->len) {
>>> + dc->len = total_len;
>>> + dc->state = D_PREP;
>>> + reinit_completion(&dc->wait);
>>> + } else {
>>> + dcc->undiscard_blks -= total_len;
>>> + if (dc->state == D_PARTIAL)
>>> + dc->state = D_SUBMIT;
>>> + }
>>> + err = dc->error;
>>> + dc->error = 0;
>>> + spin_unlock_irqrestore(&dc->lock, flags);
>>> + break;
>>> + }
>>> + }
>>> +
>>> + atomic_inc(&dcc->queued_discard);
>>> + dc->queued++;
>>> + list_move_tail(&dc->list, wait_list);
>>>
>>> atomic_inc(&dcc->issued_discard);
>>>
>>> @@ -1214,8 +1239,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
>>> len = total_len;
>>> }
>>>
>>> - if (!err && len)
>>> - __update_discard_tree_range(sbi, bdev, lstart, start, len);
>>> + if ((!err || err == -EAGAIN) && total_len && dc->start != start)
>>> + __update_discard_tree_range(sbi, bdev, lstart, start,
>>> + total_len);
>>> return err;
>>> }
>>>
>>> @@ -1470,12 +1496,15 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>>> struct list_head *pend_list;
>>> struct discard_cmd *dc, *tmp;
>>> struct blk_plug plug;
>>> - int i, issued = 0;
>>> + int i, err, issued = 0;
>>> bool io_interrupted = false;
>>> + bool retry;
>>>
>>> if (dpolicy->timeout != 0)
>>> f2fs_update_time(sbi, dpolicy->timeout);
>>>
>>> +retry:
>>> + retry = false;
>>> for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
>>> if (dpolicy->timeout != 0 &&
>>> f2fs_time_over(sbi, dpolicy->timeout))
>>> @@ -1509,7 +1538,12 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>>> break;
>>> }
>>>
>>> - __submit_discard_cmd(sbi, dpolicy, dc, &issued);
>>> + err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
>>> + if (err == -EAGAIN) {
>>> + congestion_wait(BLK_RW_ASYNC,
>>> + DEFAULT_IO_TIMEOUT);
>>> + retry = true;
>>> + }
>>>
>>> if (issued >= dpolicy->max_requests)
>>> break;
>>> @@ -1522,6 +1556,9 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>>> break;
>>> }
>>>
>>> + if (retry)
>>> + goto retry;
>>> +
>>> if (!issued && io_interrupted)
>>> issued = -1;
>>>
>>> --
>>> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: fix long latency due to discard during umount
@ 2020-04-02 9:32 ` Chao Yu
0 siblings, 0 replies; 19+ messages in thread
From: Chao Yu @ 2020-04-02 9:32 UTC (permalink / raw)
To: Sahitya Tummala, Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel
On 2020/4/1 17:22, Sahitya Tummala wrote:
> Hi Jaegeuk,
>
> On Tue, Mar 31, 2020 at 11:46:55AM -0700, Jaegeuk Kim wrote:
>> On 03/30, Sahitya Tummala wrote:
>>> F2FS already has a default timeout of 5 secs for discards that
>>> can be issued during umount, but it can take more than the 5 sec
>>> timeout if the underlying UFS device queue is already full and there
>>> are no more available free tags to be used. In that case, submit_bio()
>>> will wait for the already queued discard requests to complete to get
>>> a free tag, which can potentially take way more than 5 sec.
>>>
>>> Fix this by submitting the discard requests with REQ_NOWAIT
>>> flags during umount. This will return -EAGAIN for UFS queue/tag full
>>> scenario without waiting in the context of submit_bio(). The FS can
>>> then handle these requests by retrying again within the stipulated
>>> discard timeout period to avoid long latencies.
>>
>> Sorry, Sahitya, but, do we really need to do like this? How about just
>> controlling # of outstanding discarding bios in __issue_discard_cmd()?
>
> Do you mean something like this?
>
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 1a62b27..860dd43 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1099,7 +1099,7 @@ static void __init_discard_policy(struct f2fs_sb_info *sbi,
> } else if (discard_type == DPOLICY_FSTRIM) {
> dpolicy->io_aware = false;
> } else if (discard_type == DPOLICY_UMOUNT) {
> - dpolicy->max_requests = UINT_MAX;
> + dpolicy->max_requests = 30;
8 or 16?
It looks more simple than previous implementation.
Thanks,
> dpolicy->io_aware = false;
> /* we need to issue all to keep CP_TRIMMED_FLAG */
> dpolicy->granularity = 1;
> @@ -1470,12 +1470,14 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> struct list_head *pend_list;
> struct discard_cmd *dc, *tmp;
> struct blk_plug plug;
> - int i, issued = 0;
> + int i, issued;
> bool io_interrupted = false;
>
> if (dpolicy->timeout != 0)
> f2fs_update_time(sbi, dpolicy->timeout);
>
> +retry:
> + issued = 0;
> for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> if (dpolicy->timeout != 0 &&
> f2fs_time_over(sbi, dpolicy->timeout))
> @@ -1522,6 +1524,11 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> break;
> }
>
> + if (dpolicy->type == DPOLICY_UMOUNT && issued) {
> + __wait_all_discard_cmd(sbi, dpolicy);
> + goto retry;
> + }
> +
> if (!issued && io_interrupted)
> issued = -1;
>
> Thanks,
>
>>
>>>
>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>> ---
>>> v3:
>>> -Handle the regression reported by Chao with v2.
>>> -simplify the logic to split the dc with multiple bios incase any bio returns
>>> EAGAIN and retry those new dc within 5 sec timeout.
>>>
>>> fs/f2fs/segment.c | 65 +++++++++++++++++++++++++++++++++++++++++++------------
>>> 1 file changed, 51 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index fb3e531..55d18c7 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -1029,13 +1029,16 @@ static void f2fs_submit_discard_endio(struct bio *bio)
>>> struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
>>> unsigned long flags;
>>>
>>> - dc->error = blk_status_to_errno(bio->bi_status);
>>> -
>>> spin_lock_irqsave(&dc->lock, flags);
>>> + if (!dc->error)
>>> + dc->error = blk_status_to_errno(bio->bi_status);
>>> +
>>> dc->bio_ref--;
>>> - if (!dc->bio_ref && dc->state == D_SUBMIT) {
>>> - dc->state = D_DONE;
>>> - complete_all(&dc->wait);
>>> + if (!dc->bio_ref) {
>>> + if (dc->error || dc->state == D_SUBMIT) {
>>> + dc->state = D_DONE;
>>> + complete_all(&dc->wait);
>>> + }
>>> }
>>> spin_unlock_irqrestore(&dc->lock, flags);
>>> bio_put(bio);
>>> @@ -1124,10 +1127,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
>>> struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
>>> struct list_head *wait_list = (dpolicy->type == DPOLICY_FSTRIM) ?
>>> &(dcc->fstrim_list) : &(dcc->wait_list);
>>> - int flag = dpolicy->sync ? REQ_SYNC : 0;
>>> + int flag;
>>> block_t lstart, start, len, total_len;
>>> int err = 0;
>>>
>>> + flag = dpolicy->sync ? REQ_SYNC : 0;
>>> + flag |= dpolicy->type == DPOLICY_UMOUNT ? REQ_NOWAIT : 0;
>>> +
>>> if (dc->state != D_PREP)
>>> return 0;
>>>
>>> @@ -1192,10 +1198,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
>>> dc->bio_ref++;
>>> spin_unlock_irqrestore(&dc->lock, flags);
>>>
>>> - atomic_inc(&dcc->queued_discard);
>>> - dc->queued++;
>>> - list_move_tail(&dc->list, wait_list);
>>> -
>>> /* sanity check on discard range */
>>> __check_sit_bitmap(sbi, lstart, lstart + len);
>>>
>>> @@ -1203,6 +1205,29 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
>>> bio->bi_end_io = f2fs_submit_discard_endio;
>>> bio->bi_opf |= flag;
>>> submit_bio(bio);
>>> + if (flag & REQ_NOWAIT) {
>>> + if (dc->error == -EAGAIN) {
>>> + spin_lock_irqsave(&dc->lock, flags);
>>> + dc->len -= len;
>>> + if (!dc->len) {
>>> + dc->len = total_len;
>>> + dc->state = D_PREP;
>>> + reinit_completion(&dc->wait);
>>> + } else {
>>> + dcc->undiscard_blks -= total_len;
>>> + if (dc->state == D_PARTIAL)
>>> + dc->state = D_SUBMIT;
>>> + }
>>> + err = dc->error;
>>> + dc->error = 0;
>>> + spin_unlock_irqrestore(&dc->lock, flags);
>>> + break;
>>> + }
>>> + }
>>> +
>>> + atomic_inc(&dcc->queued_discard);
>>> + dc->queued++;
>>> + list_move_tail(&dc->list, wait_list);
>>>
>>> atomic_inc(&dcc->issued_discard);
>>>
>>> @@ -1214,8 +1239,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
>>> len = total_len;
>>> }
>>>
>>> - if (!err && len)
>>> - __update_discard_tree_range(sbi, bdev, lstart, start, len);
>>> + if ((!err || err == -EAGAIN) && total_len && dc->start != start)
>>> + __update_discard_tree_range(sbi, bdev, lstart, start,
>>> + total_len);
>>> return err;
>>> }
>>>
>>> @@ -1470,12 +1496,15 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>>> struct list_head *pend_list;
>>> struct discard_cmd *dc, *tmp;
>>> struct blk_plug plug;
>>> - int i, issued = 0;
>>> + int i, err, issued = 0;
>>> bool io_interrupted = false;
>>> + bool retry;
>>>
>>> if (dpolicy->timeout != 0)
>>> f2fs_update_time(sbi, dpolicy->timeout);
>>>
>>> +retry:
>>> + retry = false;
>>> for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
>>> if (dpolicy->timeout != 0 &&
>>> f2fs_time_over(sbi, dpolicy->timeout))
>>> @@ -1509,7 +1538,12 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>>> break;
>>> }
>>>
>>> - __submit_discard_cmd(sbi, dpolicy, dc, &issued);
>>> + err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
>>> + if (err == -EAGAIN) {
>>> + congestion_wait(BLK_RW_ASYNC,
>>> + DEFAULT_IO_TIMEOUT);
>>> + retry = true;
>>> + }
>>>
>>> if (issued >= dpolicy->max_requests)
>>> break;
>>> @@ -1522,6 +1556,9 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>>> break;
>>> }
>>>
>>> + if (retry)
>>> + goto retry;
>>> +
>>> if (!issued && io_interrupted)
>>> issued = -1;
>>>
>>> --
>>> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] f2fs: fix long latency due to discard during umount
2020-04-01 9:22 ` [f2fs-dev] " Sahitya Tummala
@ 2020-04-03 17:19 ` Jaegeuk Kim
-1 siblings, 0 replies; 19+ messages in thread
From: Jaegeuk Kim @ 2020-04-03 17:19 UTC (permalink / raw)
To: Sahitya Tummala; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
On 04/01, Sahitya Tummala wrote:
> Hi Jaegeuk,
>
> On Tue, Mar 31, 2020 at 11:46:55AM -0700, Jaegeuk Kim wrote:
> > On 03/30, Sahitya Tummala wrote:
> > > F2FS already has a default timeout of 5 secs for discards that
> > > can be issued during umount, but it can take more than the 5 sec
> > > timeout if the underlying UFS device queue is already full and there
> > > are no more available free tags to be used. In that case, submit_bio()
> > > will wait for the already queued discard requests to complete to get
> > > a free tag, which can potentially take way more than 5 sec.
> > >
> > > Fix this by submitting the discard requests with REQ_NOWAIT
> > > flags during umount. This will return -EAGAIN for UFS queue/tag full
> > > scenario without waiting in the context of submit_bio(). The FS can
> > > then handle these requests by retrying again within the stipulated
> > > discard timeout period to avoid long latencies.
> >
> > Sorry, Sahitya, but, do we really need to do like this? How about just
> > controlling # of outstanding discarding bios in __issue_discard_cmd()?
>
> Do you mean something like this?
>
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 1a62b27..860dd43 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1099,7 +1099,7 @@ static void __init_discard_policy(struct f2fs_sb_info *sbi,
> } else if (discard_type == DPOLICY_FSTRIM) {
> dpolicy->io_aware = false;
> } else if (discard_type == DPOLICY_UMOUNT) {
> - dpolicy->max_requests = UINT_MAX;
> + dpolicy->max_requests = 30;
Can we use max queue depth of the block device?
> dpolicy->io_aware = false;
> /* we need to issue all to keep CP_TRIMMED_FLAG */
> dpolicy->granularity = 1;
> @@ -1470,12 +1470,14 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> struct list_head *pend_list;
> struct discard_cmd *dc, *tmp;
> struct blk_plug plug;
> - int i, issued = 0;
> + int i, issued;
> bool io_interrupted = false;
>
> if (dpolicy->timeout != 0)
> f2fs_update_time(sbi, dpolicy->timeout);
>
> +retry:
> + issued = 0;
> for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> if (dpolicy->timeout != 0 &&
> f2fs_time_over(sbi, dpolicy->timeout))
> @@ -1522,6 +1524,11 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> break;
> }
>
> + if (dpolicy->type == DPOLICY_UMOUNT && issued) {
> + __wait_all_discard_cmd(sbi, dpolicy);
> + goto retry;
> + }
> +
> if (!issued && io_interrupted)
> issued = -1;
>
> Thanks,
>
> >
> > >
> > > Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> > > ---
> > > v3:
> > > -Handle the regression reported by Chao with v2.
> > > -simplify the logic to split the dc with multiple bios incase any bio returns
> > > EAGAIN and retry those new dc within 5 sec timeout.
> > >
> > > fs/f2fs/segment.c | 65 +++++++++++++++++++++++++++++++++++++++++++------------
> > > 1 file changed, 51 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > index fb3e531..55d18c7 100644
> > > --- a/fs/f2fs/segment.c
> > > +++ b/fs/f2fs/segment.c
> > > @@ -1029,13 +1029,16 @@ static void f2fs_submit_discard_endio(struct bio *bio)
> > > struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> > > unsigned long flags;
> > >
> > > - dc->error = blk_status_to_errno(bio->bi_status);
> > > -
> > > spin_lock_irqsave(&dc->lock, flags);
> > > + if (!dc->error)
> > > + dc->error = blk_status_to_errno(bio->bi_status);
> > > +
> > > dc->bio_ref--;
> > > - if (!dc->bio_ref && dc->state == D_SUBMIT) {
> > > - dc->state = D_DONE;
> > > - complete_all(&dc->wait);
> > > + if (!dc->bio_ref) {
> > > + if (dc->error || dc->state == D_SUBMIT) {
> > > + dc->state = D_DONE;
> > > + complete_all(&dc->wait);
> > > + }
> > > }
> > > spin_unlock_irqrestore(&dc->lock, flags);
> > > bio_put(bio);
> > > @@ -1124,10 +1127,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> > > struct list_head *wait_list = (dpolicy->type == DPOLICY_FSTRIM) ?
> > > &(dcc->fstrim_list) : &(dcc->wait_list);
> > > - int flag = dpolicy->sync ? REQ_SYNC : 0;
> > > + int flag;
> > > block_t lstart, start, len, total_len;
> > > int err = 0;
> > >
> > > + flag = dpolicy->sync ? REQ_SYNC : 0;
> > > + flag |= dpolicy->type == DPOLICY_UMOUNT ? REQ_NOWAIT : 0;
> > > +
> > > if (dc->state != D_PREP)
> > > return 0;
> > >
> > > @@ -1192,10 +1198,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > dc->bio_ref++;
> > > spin_unlock_irqrestore(&dc->lock, flags);
> > >
> > > - atomic_inc(&dcc->queued_discard);
> > > - dc->queued++;
> > > - list_move_tail(&dc->list, wait_list);
> > > -
> > > /* sanity check on discard range */
> > > __check_sit_bitmap(sbi, lstart, lstart + len);
> > >
> > > @@ -1203,6 +1205,29 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > bio->bi_end_io = f2fs_submit_discard_endio;
> > > bio->bi_opf |= flag;
> > > submit_bio(bio);
> > > + if (flag & REQ_NOWAIT) {
> > > + if (dc->error == -EAGAIN) {
> > > + spin_lock_irqsave(&dc->lock, flags);
> > > + dc->len -= len;
> > > + if (!dc->len) {
> > > + dc->len = total_len;
> > > + dc->state = D_PREP;
> > > + reinit_completion(&dc->wait);
> > > + } else {
> > > + dcc->undiscard_blks -= total_len;
> > > + if (dc->state == D_PARTIAL)
> > > + dc->state = D_SUBMIT;
> > > + }
> > > + err = dc->error;
> > > + dc->error = 0;
> > > + spin_unlock_irqrestore(&dc->lock, flags);
> > > + break;
> > > + }
> > > + }
> > > +
> > > + atomic_inc(&dcc->queued_discard);
> > > + dc->queued++;
> > > + list_move_tail(&dc->list, wait_list);
> > >
> > > atomic_inc(&dcc->issued_discard);
> > >
> > > @@ -1214,8 +1239,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > len = total_len;
> > > }
> > >
> > > - if (!err && len)
> > > - __update_discard_tree_range(sbi, bdev, lstart, start, len);
> > > + if ((!err || err == -EAGAIN) && total_len && dc->start != start)
> > > + __update_discard_tree_range(sbi, bdev, lstart, start,
> > > + total_len);
> > > return err;
> > > }
> > >
> > > @@ -1470,12 +1496,15 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > struct list_head *pend_list;
> > > struct discard_cmd *dc, *tmp;
> > > struct blk_plug plug;
> > > - int i, issued = 0;
> > > + int i, err, issued = 0;
> > > bool io_interrupted = false;
> > > + bool retry;
> > >
> > > if (dpolicy->timeout != 0)
> > > f2fs_update_time(sbi, dpolicy->timeout);
> > >
> > > +retry:
> > > + retry = false;
> > > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > > if (dpolicy->timeout != 0 &&
> > > f2fs_time_over(sbi, dpolicy->timeout))
> > > @@ -1509,7 +1538,12 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > break;
> > > }
> > >
> > > - __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > + err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > + if (err == -EAGAIN) {
> > > + congestion_wait(BLK_RW_ASYNC,
> > > + DEFAULT_IO_TIMEOUT);
> > > + retry = true;
> > > + }
> > >
> > > if (issued >= dpolicy->max_requests)
> > > break;
> > > @@ -1522,6 +1556,9 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > break;
> > > }
> > >
> > > + if (retry)
> > > + goto retry;
> > > +
> > > if (!issued && io_interrupted)
> > > issued = -1;
> > >
> > > --
> > > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
> > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
>
> --
> --
> Sent by a consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: fix long latency due to discard during umount
@ 2020-04-03 17:19 ` Jaegeuk Kim
0 siblings, 0 replies; 19+ messages in thread
From: Jaegeuk Kim @ 2020-04-03 17:19 UTC (permalink / raw)
To: Sahitya Tummala; +Cc: linux-kernel, linux-f2fs-devel
On 04/01, Sahitya Tummala wrote:
> Hi Jaegeuk,
>
> On Tue, Mar 31, 2020 at 11:46:55AM -0700, Jaegeuk Kim wrote:
> > On 03/30, Sahitya Tummala wrote:
> > > F2FS already has a default timeout of 5 secs for discards that
> > > can be issued during umount, but it can take more than the 5 sec
> > > timeout if the underlying UFS device queue is already full and there
> > > are no more available free tags to be used. In that case, submit_bio()
> > > will wait for the already queued discard requests to complete to get
> > > a free tag, which can potentially take way more than 5 sec.
> > >
> > > Fix this by submitting the discard requests with REQ_NOWAIT
> > > flags during umount. This will return -EAGAIN for UFS queue/tag full
> > > scenario without waiting in the context of submit_bio(). The FS can
> > > then handle these requests by retrying again within the stipulated
> > > discard timeout period to avoid long latencies.
> >
> > Sorry, Sahitya, but, do we really need to do like this? How about just
> > controlling # of outstanding discarding bios in __issue_discard_cmd()?
>
> Do you mean something like this?
>
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 1a62b27..860dd43 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1099,7 +1099,7 @@ static void __init_discard_policy(struct f2fs_sb_info *sbi,
> } else if (discard_type == DPOLICY_FSTRIM) {
> dpolicy->io_aware = false;
> } else if (discard_type == DPOLICY_UMOUNT) {
> - dpolicy->max_requests = UINT_MAX;
> + dpolicy->max_requests = 30;
Can we use max queue depth of the block device?
> dpolicy->io_aware = false;
> /* we need to issue all to keep CP_TRIMMED_FLAG */
> dpolicy->granularity = 1;
> @@ -1470,12 +1470,14 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> struct list_head *pend_list;
> struct discard_cmd *dc, *tmp;
> struct blk_plug plug;
> - int i, issued = 0;
> + int i, issued;
> bool io_interrupted = false;
>
> if (dpolicy->timeout != 0)
> f2fs_update_time(sbi, dpolicy->timeout);
>
> +retry:
> + issued = 0;
> for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> if (dpolicy->timeout != 0 &&
> f2fs_time_over(sbi, dpolicy->timeout))
> @@ -1522,6 +1524,11 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> break;
> }
>
> + if (dpolicy->type == DPOLICY_UMOUNT && issued) {
> + __wait_all_discard_cmd(sbi, dpolicy);
> + goto retry;
> + }
> +
> if (!issued && io_interrupted)
> issued = -1;
>
> Thanks,
>
> >
> > >
> > > Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> > > ---
> > > v3:
> > > -Handle the regression reported by Chao with v2.
> > > -simplify the logic to split the dc with multiple bios incase any bio returns
> > > EAGAIN and retry those new dc within 5 sec timeout.
> > >
> > > fs/f2fs/segment.c | 65 +++++++++++++++++++++++++++++++++++++++++++------------
> > > 1 file changed, 51 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > index fb3e531..55d18c7 100644
> > > --- a/fs/f2fs/segment.c
> > > +++ b/fs/f2fs/segment.c
> > > @@ -1029,13 +1029,16 @@ static void f2fs_submit_discard_endio(struct bio *bio)
> > > struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> > > unsigned long flags;
> > >
> > > - dc->error = blk_status_to_errno(bio->bi_status);
> > > -
> > > spin_lock_irqsave(&dc->lock, flags);
> > > + if (!dc->error)
> > > + dc->error = blk_status_to_errno(bio->bi_status);
> > > +
> > > dc->bio_ref--;
> > > - if (!dc->bio_ref && dc->state == D_SUBMIT) {
> > > - dc->state = D_DONE;
> > > - complete_all(&dc->wait);
> > > + if (!dc->bio_ref) {
> > > + if (dc->error || dc->state == D_SUBMIT) {
> > > + dc->state = D_DONE;
> > > + complete_all(&dc->wait);
> > > + }
> > > }
> > > spin_unlock_irqrestore(&dc->lock, flags);
> > > bio_put(bio);
> > > @@ -1124,10 +1127,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> > > struct list_head *wait_list = (dpolicy->type == DPOLICY_FSTRIM) ?
> > > &(dcc->fstrim_list) : &(dcc->wait_list);
> > > - int flag = dpolicy->sync ? REQ_SYNC : 0;
> > > + int flag;
> > > block_t lstart, start, len, total_len;
> > > int err = 0;
> > >
> > > + flag = dpolicy->sync ? REQ_SYNC : 0;
> > > + flag |= dpolicy->type == DPOLICY_UMOUNT ? REQ_NOWAIT : 0;
> > > +
> > > if (dc->state != D_PREP)
> > > return 0;
> > >
> > > @@ -1192,10 +1198,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > dc->bio_ref++;
> > > spin_unlock_irqrestore(&dc->lock, flags);
> > >
> > > - atomic_inc(&dcc->queued_discard);
> > > - dc->queued++;
> > > - list_move_tail(&dc->list, wait_list);
> > > -
> > > /* sanity check on discard range */
> > > __check_sit_bitmap(sbi, lstart, lstart + len);
> > >
> > > @@ -1203,6 +1205,29 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > bio->bi_end_io = f2fs_submit_discard_endio;
> > > bio->bi_opf |= flag;
> > > submit_bio(bio);
> > > + if (flag & REQ_NOWAIT) {
> > > + if (dc->error == -EAGAIN) {
> > > + spin_lock_irqsave(&dc->lock, flags);
> > > + dc->len -= len;
> > > + if (!dc->len) {
> > > + dc->len = total_len;
> > > + dc->state = D_PREP;
> > > + reinit_completion(&dc->wait);
> > > + } else {
> > > + dcc->undiscard_blks -= total_len;
> > > + if (dc->state == D_PARTIAL)
> > > + dc->state = D_SUBMIT;
> > > + }
> > > + err = dc->error;
> > > + dc->error = 0;
> > > + spin_unlock_irqrestore(&dc->lock, flags);
> > > + break;
> > > + }
> > > + }
> > > +
> > > + atomic_inc(&dcc->queued_discard);
> > > + dc->queued++;
> > > + list_move_tail(&dc->list, wait_list);
> > >
> > > atomic_inc(&dcc->issued_discard);
> > >
> > > @@ -1214,8 +1239,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > len = total_len;
> > > }
> > >
> > > - if (!err && len)
> > > - __update_discard_tree_range(sbi, bdev, lstart, start, len);
> > > + if ((!err || err == -EAGAIN) && total_len && dc->start != start)
> > > + __update_discard_tree_range(sbi, bdev, lstart, start,
> > > + total_len);
> > > return err;
> > > }
> > >
> > > @@ -1470,12 +1496,15 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > struct list_head *pend_list;
> > > struct discard_cmd *dc, *tmp;
> > > struct blk_plug plug;
> > > - int i, issued = 0;
> > > + int i, err, issued = 0;
> > > bool io_interrupted = false;
> > > + bool retry;
> > >
> > > if (dpolicy->timeout != 0)
> > > f2fs_update_time(sbi, dpolicy->timeout);
> > >
> > > +retry:
> > > + retry = false;
> > > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > > if (dpolicy->timeout != 0 &&
> > > f2fs_time_over(sbi, dpolicy->timeout))
> > > @@ -1509,7 +1538,12 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > break;
> > > }
> > >
> > > - __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > + err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > + if (err == -EAGAIN) {
> > > + congestion_wait(BLK_RW_ASYNC,
> > > + DEFAULT_IO_TIMEOUT);
> > > + retry = true;
> > > + }
> > >
> > > if (issued >= dpolicy->max_requests)
> > > break;
> > > @@ -1522,6 +1556,9 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > break;
> > > }
> > >
> > > + if (retry)
> > > + goto retry;
> > > +
> > > if (!issued && io_interrupted)
> > > issued = -1;
> > >
> > > --
> > > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
> > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
>
> --
> --
> Sent by a consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] f2fs: fix long latency due to discard during umount
2020-04-03 17:19 ` [f2fs-dev] " Jaegeuk Kim
(?)
@ 2020-04-08 9:00 ` Sahitya Tummala
2020-04-09 2:29 ` [f2fs-dev] " Jaegeuk Kim
-1 siblings, 1 reply; 19+ messages in thread
From: Sahitya Tummala @ 2020-04-08 9:00 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel, stummala
Hi Jaegeuk,
On Fri, Apr 03, 2020 at 10:19:43AM -0700, Jaegeuk Kim wrote:
> On 04/01, Sahitya Tummala wrote:
> > Hi Jaegeuk,
> >
> > On Tue, Mar 31, 2020 at 11:46:55AM -0700, Jaegeuk Kim wrote:
> > > On 03/30, Sahitya Tummala wrote:
> > > > F2FS already has a default timeout of 5 secs for discards that
> > > > can be issued during umount, but it can take more than the 5 sec
> > > > timeout if the underlying UFS device queue is already full and there
> > > > are no more available free tags to be used. In that case, submit_bio()
> > > > will wait for the already queued discard requests to complete to get
> > > > a free tag, which can potentially take way more than 5 sec.
> > > >
> > > > Fix this by submitting the discard requests with REQ_NOWAIT
> > > > flags during umount. This will return -EAGAIN for UFS queue/tag full
> > > > scenario without waiting in the context of submit_bio(). The FS can
> > > > then handle these requests by retrying again within the stipulated
> > > > discard timeout period to avoid long latencies.
> > >
> > > Sorry, Sahitya, but, do we really need to do like this? How about just
> > > controlling # of outstanding discarding bios in __issue_discard_cmd()?
> >
> > Do you mean something like this?
> >
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index 1a62b27..860dd43 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -1099,7 +1099,7 @@ static void __init_discard_policy(struct f2fs_sb_info *sbi,
> > } else if (discard_type == DPOLICY_FSTRIM) {
> > dpolicy->io_aware = false;
> > } else if (discard_type == DPOLICY_UMOUNT) {
> > - dpolicy->max_requests = UINT_MAX;
> > + dpolicy->max_requests = 30;
>
> Can we use max queue depth of the block device?
I think it should be limited to 8 or 16 as Chao suggested, so that we can have
better control on the given timeout value? Thoughts?
Thanks,
>
> > dpolicy->io_aware = false;
> > /* we need to issue all to keep CP_TRIMMED_FLAG */
> > dpolicy->granularity = 1;
> > @@ -1470,12 +1470,14 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > struct list_head *pend_list;
> > struct discard_cmd *dc, *tmp;
> > struct blk_plug plug;
> > - int i, issued = 0;
> > + int i, issued;
> > bool io_interrupted = false;
> >
> > if (dpolicy->timeout != 0)
> > f2fs_update_time(sbi, dpolicy->timeout);
> >
> > +retry:
> > + issued = 0;
> > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > if (dpolicy->timeout != 0 &&
> > f2fs_time_over(sbi, dpolicy->timeout))
> > @@ -1522,6 +1524,11 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > break;
> > }
> >
> > + if (dpolicy->type == DPOLICY_UMOUNT && issued) {
> > + __wait_all_discard_cmd(sbi, dpolicy);
> > + goto retry;
> > + }
> > +
> > if (!issued && io_interrupted)
> > issued = -1;
> >
> > Thanks,
> >
> > >
> > > >
> > > > Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> > > > ---
> > > > v3:
> > > > -Handle the regression reported by Chao with v2.
> > > > -simplify the logic to split the dc with multiple bios incase any bio returns
> > > > EAGAIN and retry those new dc within 5 sec timeout.
> > > >
> > > > fs/f2fs/segment.c | 65 +++++++++++++++++++++++++++++++++++++++++++------------
> > > > 1 file changed, 51 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > > index fb3e531..55d18c7 100644
> > > > --- a/fs/f2fs/segment.c
> > > > +++ b/fs/f2fs/segment.c
> > > > @@ -1029,13 +1029,16 @@ static void f2fs_submit_discard_endio(struct bio *bio)
> > > > struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> > > > unsigned long flags;
> > > >
> > > > - dc->error = blk_status_to_errno(bio->bi_status);
> > > > -
> > > > spin_lock_irqsave(&dc->lock, flags);
> > > > + if (!dc->error)
> > > > + dc->error = blk_status_to_errno(bio->bi_status);
> > > > +
> > > > dc->bio_ref--;
> > > > - if (!dc->bio_ref && dc->state == D_SUBMIT) {
> > > > - dc->state = D_DONE;
> > > > - complete_all(&dc->wait);
> > > > + if (!dc->bio_ref) {
> > > > + if (dc->error || dc->state == D_SUBMIT) {
> > > > + dc->state = D_DONE;
> > > > + complete_all(&dc->wait);
> > > > + }
> > > > }
> > > > spin_unlock_irqrestore(&dc->lock, flags);
> > > > bio_put(bio);
> > > > @@ -1124,10 +1127,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> > > > struct list_head *wait_list = (dpolicy->type == DPOLICY_FSTRIM) ?
> > > > &(dcc->fstrim_list) : &(dcc->wait_list);
> > > > - int flag = dpolicy->sync ? REQ_SYNC : 0;
> > > > + int flag;
> > > > block_t lstart, start, len, total_len;
> > > > int err = 0;
> > > >
> > > > + flag = dpolicy->sync ? REQ_SYNC : 0;
> > > > + flag |= dpolicy->type == DPOLICY_UMOUNT ? REQ_NOWAIT : 0;
> > > > +
> > > > if (dc->state != D_PREP)
> > > > return 0;
> > > >
> > > > @@ -1192,10 +1198,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > dc->bio_ref++;
> > > > spin_unlock_irqrestore(&dc->lock, flags);
> > > >
> > > > - atomic_inc(&dcc->queued_discard);
> > > > - dc->queued++;
> > > > - list_move_tail(&dc->list, wait_list);
> > > > -
> > > > /* sanity check on discard range */
> > > > __check_sit_bitmap(sbi, lstart, lstart + len);
> > > >
> > > > @@ -1203,6 +1205,29 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > bio->bi_end_io = f2fs_submit_discard_endio;
> > > > bio->bi_opf |= flag;
> > > > submit_bio(bio);
> > > > + if (flag & REQ_NOWAIT) {
> > > > + if (dc->error == -EAGAIN) {
> > > > + spin_lock_irqsave(&dc->lock, flags);
> > > > + dc->len -= len;
> > > > + if (!dc->len) {
> > > > + dc->len = total_len;
> > > > + dc->state = D_PREP;
> > > > + reinit_completion(&dc->wait);
> > > > + } else {
> > > > + dcc->undiscard_blks -= total_len;
> > > > + if (dc->state == D_PARTIAL)
> > > > + dc->state = D_SUBMIT;
> > > > + }
> > > > + err = dc->error;
> > > > + dc->error = 0;
> > > > + spin_unlock_irqrestore(&dc->lock, flags);
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + atomic_inc(&dcc->queued_discard);
> > > > + dc->queued++;
> > > > + list_move_tail(&dc->list, wait_list);
> > > >
> > > > atomic_inc(&dcc->issued_discard);
> > > >
> > > > @@ -1214,8 +1239,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > len = total_len;
> > > > }
> > > >
> > > > - if (!err && len)
> > > > - __update_discard_tree_range(sbi, bdev, lstart, start, len);
> > > > + if ((!err || err == -EAGAIN) && total_len && dc->start != start)
> > > > + __update_discard_tree_range(sbi, bdev, lstart, start,
> > > > + total_len);
> > > > return err;
> > > > }
> > > >
> > > > @@ -1470,12 +1496,15 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > struct list_head *pend_list;
> > > > struct discard_cmd *dc, *tmp;
> > > > struct blk_plug plug;
> > > > - int i, issued = 0;
> > > > + int i, err, issued = 0;
> > > > bool io_interrupted = false;
> > > > + bool retry;
> > > >
> > > > if (dpolicy->timeout != 0)
> > > > f2fs_update_time(sbi, dpolicy->timeout);
> > > >
> > > > +retry:
> > > > + retry = false;
> > > > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > > > if (dpolicy->timeout != 0 &&
> > > > f2fs_time_over(sbi, dpolicy->timeout))
> > > > @@ -1509,7 +1538,12 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > break;
> > > > }
> > > >
> > > > - __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > > + err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > > + if (err == -EAGAIN) {
> > > > + congestion_wait(BLK_RW_ASYNC,
> > > > + DEFAULT_IO_TIMEOUT);
> > > > + retry = true;
> > > > + }
> > > >
> > > > if (issued >= dpolicy->max_requests)
> > > > break;
> > > > @@ -1522,6 +1556,9 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > break;
> > > > }
> > > >
> > > > + if (retry)
> > > > + goto retry;
> > > > +
> > > > if (!issued && io_interrupted)
> > > > issued = -1;
> > > >
> > > > --
> > > > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
> > > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
> >
> > --
> > --
> > Sent by a consultant of the Qualcomm Innovation Center, Inc.
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] f2fs: fix long latency due to discard during umount
2020-04-08 9:00 ` Sahitya Tummala
@ 2020-04-09 2:29 ` Jaegeuk Kim
0 siblings, 0 replies; 19+ messages in thread
From: Jaegeuk Kim @ 2020-04-09 2:29 UTC (permalink / raw)
To: Sahitya Tummala; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
On 04/08, Sahitya Tummala wrote:
> Hi Jaegeuk,
>
> On Fri, Apr 03, 2020 at 10:19:43AM -0700, Jaegeuk Kim wrote:
> > On 04/01, Sahitya Tummala wrote:
> > > Hi Jaegeuk,
> > >
> > > On Tue, Mar 31, 2020 at 11:46:55AM -0700, Jaegeuk Kim wrote:
> > > > On 03/30, Sahitya Tummala wrote:
> > > > > F2FS already has a default timeout of 5 secs for discards that
> > > > > can be issued during umount, but it can take more than the 5 sec
> > > > > timeout if the underlying UFS device queue is already full and there
> > > > > are no more available free tags to be used. In that case, submit_bio()
> > > > > will wait for the already queued discard requests to complete to get
> > > > > a free tag, which can potentially take way more than 5 sec.
> > > > >
> > > > > Fix this by submitting the discard requests with REQ_NOWAIT
> > > > > flags during umount. This will return -EAGAIN for UFS queue/tag full
> > > > > scenario without waiting in the context of submit_bio(). The FS can
> > > > > then handle these requests by retrying again within the stipulated
> > > > > discard timeout period to avoid long latencies.
> > > >
> > > > Sorry, Sahitya, but, do we really need to do like this? How about just
> > > > controlling # of outstanding discarding bios in __issue_discard_cmd()?
> > >
> > > Do you mean something like this?
> > >
> > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > index 1a62b27..860dd43 100644
> > > --- a/fs/f2fs/segment.c
> > > +++ b/fs/f2fs/segment.c
> > > @@ -1099,7 +1099,7 @@ static void __init_discard_policy(struct f2fs_sb_info *sbi,
> > > } else if (discard_type == DPOLICY_FSTRIM) {
> > > dpolicy->io_aware = false;
> > > } else if (discard_type == DPOLICY_UMOUNT) {
> > > - dpolicy->max_requests = UINT_MAX;
> > > + dpolicy->max_requests = 30;
> >
> > Can we use max queue depth of the block device?
>
> I think it should be limited to 8 or 16 as Chao suggested, so that we can have
> better control on the given timeout value? Thoughts?
Where is 8 or 16 coming from? What about SSD? Sorry, it's unclear to me.
>
> Thanks,
>
> >
> > > dpolicy->io_aware = false;
> > > /* we need to issue all to keep CP_TRIMMED_FLAG */
> > > dpolicy->granularity = 1;
> > > @@ -1470,12 +1470,14 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > struct list_head *pend_list;
> > > struct discard_cmd *dc, *tmp;
> > > struct blk_plug plug;
> > > - int i, issued = 0;
> > > + int i, issued;
> > > bool io_interrupted = false;
> > >
> > > if (dpolicy->timeout != 0)
> > > f2fs_update_time(sbi, dpolicy->timeout);
> > >
> > > +retry:
> > > + issued = 0;
> > > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > > if (dpolicy->timeout != 0 &&
> > > f2fs_time_over(sbi, dpolicy->timeout))
> > > @@ -1522,6 +1524,11 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > break;
> > > }
> > >
> > > + if (dpolicy->type == DPOLICY_UMOUNT && issued) {
> > > + __wait_all_discard_cmd(sbi, dpolicy);
> > > + goto retry;
> > > + }
> > > +
> > > if (!issued && io_interrupted)
> > > issued = -1;
> > >
> > > Thanks,
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> > > > > ---
> > > > > v3:
> > > > > -Handle the regression reported by Chao with v2.
> > > > > -simplify the logic to split the dc with multiple bios incase any bio returns
> > > > > EAGAIN and retry those new dc within 5 sec timeout.
> > > > >
> > > > > fs/f2fs/segment.c | 65 +++++++++++++++++++++++++++++++++++++++++++------------
> > > > > 1 file changed, 51 insertions(+), 14 deletions(-)
> > > > >
> > > > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > > > index fb3e531..55d18c7 100644
> > > > > --- a/fs/f2fs/segment.c
> > > > > +++ b/fs/f2fs/segment.c
> > > > > @@ -1029,13 +1029,16 @@ static void f2fs_submit_discard_endio(struct bio *bio)
> > > > > struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> > > > > unsigned long flags;
> > > > >
> > > > > - dc->error = blk_status_to_errno(bio->bi_status);
> > > > > -
> > > > > spin_lock_irqsave(&dc->lock, flags);
> > > > > + if (!dc->error)
> > > > > + dc->error = blk_status_to_errno(bio->bi_status);
> > > > > +
> > > > > dc->bio_ref--;
> > > > > - if (!dc->bio_ref && dc->state == D_SUBMIT) {
> > > > > - dc->state = D_DONE;
> > > > > - complete_all(&dc->wait);
> > > > > + if (!dc->bio_ref) {
> > > > > + if (dc->error || dc->state == D_SUBMIT) {
> > > > > + dc->state = D_DONE;
> > > > > + complete_all(&dc->wait);
> > > > > + }
> > > > > }
> > > > > spin_unlock_irqrestore(&dc->lock, flags);
> > > > > bio_put(bio);
> > > > > @@ -1124,10 +1127,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> > > > > struct list_head *wait_list = (dpolicy->type == DPOLICY_FSTRIM) ?
> > > > > &(dcc->fstrim_list) : &(dcc->wait_list);
> > > > > - int flag = dpolicy->sync ? REQ_SYNC : 0;
> > > > > + int flag;
> > > > > block_t lstart, start, len, total_len;
> > > > > int err = 0;
> > > > >
> > > > > + flag = dpolicy->sync ? REQ_SYNC : 0;
> > > > > + flag |= dpolicy->type == DPOLICY_UMOUNT ? REQ_NOWAIT : 0;
> > > > > +
> > > > > if (dc->state != D_PREP)
> > > > > return 0;
> > > > >
> > > > > @@ -1192,10 +1198,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > dc->bio_ref++;
> > > > > spin_unlock_irqrestore(&dc->lock, flags);
> > > > >
> > > > > - atomic_inc(&dcc->queued_discard);
> > > > > - dc->queued++;
> > > > > - list_move_tail(&dc->list, wait_list);
> > > > > -
> > > > > /* sanity check on discard range */
> > > > > __check_sit_bitmap(sbi, lstart, lstart + len);
> > > > >
> > > > > @@ -1203,6 +1205,29 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > bio->bi_end_io = f2fs_submit_discard_endio;
> > > > > bio->bi_opf |= flag;
> > > > > submit_bio(bio);
> > > > > + if (flag & REQ_NOWAIT) {
> > > > > + if (dc->error == -EAGAIN) {
> > > > > + spin_lock_irqsave(&dc->lock, flags);
> > > > > + dc->len -= len;
> > > > > + if (!dc->len) {
> > > > > + dc->len = total_len;
> > > > > + dc->state = D_PREP;
> > > > > + reinit_completion(&dc->wait);
> > > > > + } else {
> > > > > + dcc->undiscard_blks -= total_len;
> > > > > + if (dc->state == D_PARTIAL)
> > > > > + dc->state = D_SUBMIT;
> > > > > + }
> > > > > + err = dc->error;
> > > > > + dc->error = 0;
> > > > > + spin_unlock_irqrestore(&dc->lock, flags);
> > > > > + break;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + atomic_inc(&dcc->queued_discard);
> > > > > + dc->queued++;
> > > > > + list_move_tail(&dc->list, wait_list);
> > > > >
> > > > > atomic_inc(&dcc->issued_discard);
> > > > >
> > > > > @@ -1214,8 +1239,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > len = total_len;
> > > > > }
> > > > >
> > > > > - if (!err && len)
> > > > > - __update_discard_tree_range(sbi, bdev, lstart, start, len);
> > > > > + if ((!err || err == -EAGAIN) && total_len && dc->start != start)
> > > > > + __update_discard_tree_range(sbi, bdev, lstart, start,
> > > > > + total_len);
> > > > > return err;
> > > > > }
> > > > >
> > > > > @@ -1470,12 +1496,15 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > struct list_head *pend_list;
> > > > > struct discard_cmd *dc, *tmp;
> > > > > struct blk_plug plug;
> > > > > - int i, issued = 0;
> > > > > + int i, err, issued = 0;
> > > > > bool io_interrupted = false;
> > > > > + bool retry;
> > > > >
> > > > > if (dpolicy->timeout != 0)
> > > > > f2fs_update_time(sbi, dpolicy->timeout);
> > > > >
> > > > > +retry:
> > > > > + retry = false;
> > > > > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > > > > if (dpolicy->timeout != 0 &&
> > > > > f2fs_time_over(sbi, dpolicy->timeout))
> > > > > @@ -1509,7 +1538,12 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > break;
> > > > > }
> > > > >
> > > > > - __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > > > + err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > > > + if (err == -EAGAIN) {
> > > > > + congestion_wait(BLK_RW_ASYNC,
> > > > > + DEFAULT_IO_TIMEOUT);
> > > > > + retry = true;
> > > > > + }
> > > > >
> > > > > if (issued >= dpolicy->max_requests)
> > > > > break;
> > > > > @@ -1522,6 +1556,9 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > break;
> > > > > }
> > > > >
> > > > > + if (retry)
> > > > > + goto retry;
> > > > > +
> > > > > if (!issued && io_interrupted)
> > > > > issued = -1;
> > > > >
> > > > > --
> > > > > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
> > > > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
> > >
> > > --
> > > --
> > > Sent by a consultant of the Qualcomm Innovation Center, Inc.
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
> --
> --
> Sent by a consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: fix long latency due to discard during umount
@ 2020-04-09 2:29 ` Jaegeuk Kim
0 siblings, 0 replies; 19+ messages in thread
From: Jaegeuk Kim @ 2020-04-09 2:29 UTC (permalink / raw)
To: Sahitya Tummala; +Cc: linux-kernel, linux-f2fs-devel
On 04/08, Sahitya Tummala wrote:
> Hi Jaegeuk,
>
> On Fri, Apr 03, 2020 at 10:19:43AM -0700, Jaegeuk Kim wrote:
> > On 04/01, Sahitya Tummala wrote:
> > > Hi Jaegeuk,
> > >
> > > On Tue, Mar 31, 2020 at 11:46:55AM -0700, Jaegeuk Kim wrote:
> > > > On 03/30, Sahitya Tummala wrote:
> > > > > F2FS already has a default timeout of 5 secs for discards that
> > > > > can be issued during umount, but it can take more than the 5 sec
> > > > > timeout if the underlying UFS device queue is already full and there
> > > > > are no more available free tags to be used. In that case, submit_bio()
> > > > > will wait for the already queued discard requests to complete to get
> > > > > a free tag, which can potentially take way more than 5 sec.
> > > > >
> > > > > Fix this by submitting the discard requests with REQ_NOWAIT
> > > > > flags during umount. This will return -EAGAIN for UFS queue/tag full
> > > > > scenario without waiting in the context of submit_bio(). The FS can
> > > > > then handle these requests by retrying again within the stipulated
> > > > > discard timeout period to avoid long latencies.
> > > >
> > > > Sorry, Sahitya, but, do we really need to do like this? How about just
> > > > controlling # of outstanding discarding bios in __issue_discard_cmd()?
> > >
> > > Do you mean something like this?
> > >
> > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > index 1a62b27..860dd43 100644
> > > --- a/fs/f2fs/segment.c
> > > +++ b/fs/f2fs/segment.c
> > > @@ -1099,7 +1099,7 @@ static void __init_discard_policy(struct f2fs_sb_info *sbi,
> > > } else if (discard_type == DPOLICY_FSTRIM) {
> > > dpolicy->io_aware = false;
> > > } else if (discard_type == DPOLICY_UMOUNT) {
> > > - dpolicy->max_requests = UINT_MAX;
> > > + dpolicy->max_requests = 30;
> >
> > Can we use max queue depth of the block device?
>
> I think it should be limited to 8 or 16 as Chao suggested, so that we can have
> better control on the given timeout value? Thoughts?
Where is 8 or 16 coming from? What about SSD? Sorry, it's unclear to me.
>
> Thanks,
>
> >
> > > dpolicy->io_aware = false;
> > > /* we need to issue all to keep CP_TRIMMED_FLAG */
> > > dpolicy->granularity = 1;
> > > @@ -1470,12 +1470,14 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > struct list_head *pend_list;
> > > struct discard_cmd *dc, *tmp;
> > > struct blk_plug plug;
> > > - int i, issued = 0;
> > > + int i, issued;
> > > bool io_interrupted = false;
> > >
> > > if (dpolicy->timeout != 0)
> > > f2fs_update_time(sbi, dpolicy->timeout);
> > >
> > > +retry:
> > > + issued = 0;
> > > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > > if (dpolicy->timeout != 0 &&
> > > f2fs_time_over(sbi, dpolicy->timeout))
> > > @@ -1522,6 +1524,11 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > break;
> > > }
> > >
> > > + if (dpolicy->type == DPOLICY_UMOUNT && issued) {
> > > + __wait_all_discard_cmd(sbi, dpolicy);
> > > + goto retry;
> > > + }
> > > +
> > > if (!issued && io_interrupted)
> > > issued = -1;
> > >
> > > Thanks,
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> > > > > ---
> > > > > v3:
> > > > > -Handle the regression reported by Chao with v2.
> > > > > -simplify the logic to split the dc with multiple bios incase any bio returns
> > > > > EAGAIN and retry those new dc within 5 sec timeout.
> > > > >
> > > > > fs/f2fs/segment.c | 65 +++++++++++++++++++++++++++++++++++++++++++------------
> > > > > 1 file changed, 51 insertions(+), 14 deletions(-)
> > > > >
> > > > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > > > index fb3e531..55d18c7 100644
> > > > > --- a/fs/f2fs/segment.c
> > > > > +++ b/fs/f2fs/segment.c
> > > > > @@ -1029,13 +1029,16 @@ static void f2fs_submit_discard_endio(struct bio *bio)
> > > > > struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> > > > > unsigned long flags;
> > > > >
> > > > > - dc->error = blk_status_to_errno(bio->bi_status);
> > > > > -
> > > > > spin_lock_irqsave(&dc->lock, flags);
> > > > > + if (!dc->error)
> > > > > + dc->error = blk_status_to_errno(bio->bi_status);
> > > > > +
> > > > > dc->bio_ref--;
> > > > > - if (!dc->bio_ref && dc->state == D_SUBMIT) {
> > > > > - dc->state = D_DONE;
> > > > > - complete_all(&dc->wait);
> > > > > + if (!dc->bio_ref) {
> > > > > + if (dc->error || dc->state == D_SUBMIT) {
> > > > > + dc->state = D_DONE;
> > > > > + complete_all(&dc->wait);
> > > > > + }
> > > > > }
> > > > > spin_unlock_irqrestore(&dc->lock, flags);
> > > > > bio_put(bio);
> > > > > @@ -1124,10 +1127,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> > > > > struct list_head *wait_list = (dpolicy->type == DPOLICY_FSTRIM) ?
> > > > > &(dcc->fstrim_list) : &(dcc->wait_list);
> > > > > - int flag = dpolicy->sync ? REQ_SYNC : 0;
> > > > > + int flag;
> > > > > block_t lstart, start, len, total_len;
> > > > > int err = 0;
> > > > >
> > > > > + flag = dpolicy->sync ? REQ_SYNC : 0;
> > > > > + flag |= dpolicy->type == DPOLICY_UMOUNT ? REQ_NOWAIT : 0;
> > > > > +
> > > > > if (dc->state != D_PREP)
> > > > > return 0;
> > > > >
> > > > > @@ -1192,10 +1198,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > dc->bio_ref++;
> > > > > spin_unlock_irqrestore(&dc->lock, flags);
> > > > >
> > > > > - atomic_inc(&dcc->queued_discard);
> > > > > - dc->queued++;
> > > > > - list_move_tail(&dc->list, wait_list);
> > > > > -
> > > > > /* sanity check on discard range */
> > > > > __check_sit_bitmap(sbi, lstart, lstart + len);
> > > > >
> > > > > @@ -1203,6 +1205,29 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > bio->bi_end_io = f2fs_submit_discard_endio;
> > > > > bio->bi_opf |= flag;
> > > > > submit_bio(bio);
> > > > > + if (flag & REQ_NOWAIT) {
> > > > > + if (dc->error == -EAGAIN) {
> > > > > + spin_lock_irqsave(&dc->lock, flags);
> > > > > + dc->len -= len;
> > > > > + if (!dc->len) {
> > > > > + dc->len = total_len;
> > > > > + dc->state = D_PREP;
> > > > > + reinit_completion(&dc->wait);
> > > > > + } else {
> > > > > + dcc->undiscard_blks -= total_len;
> > > > > + if (dc->state == D_PARTIAL)
> > > > > + dc->state = D_SUBMIT;
> > > > > + }
> > > > > + err = dc->error;
> > > > > + dc->error = 0;
> > > > > + spin_unlock_irqrestore(&dc->lock, flags);
> > > > > + break;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + atomic_inc(&dcc->queued_discard);
> > > > > + dc->queued++;
> > > > > + list_move_tail(&dc->list, wait_list);
> > > > >
> > > > > atomic_inc(&dcc->issued_discard);
> > > > >
> > > > > @@ -1214,8 +1239,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > len = total_len;
> > > > > }
> > > > >
> > > > > - if (!err && len)
> > > > > - __update_discard_tree_range(sbi, bdev, lstart, start, len);
> > > > > + if ((!err || err == -EAGAIN) && total_len && dc->start != start)
> > > > > + __update_discard_tree_range(sbi, bdev, lstart, start,
> > > > > + total_len);
> > > > > return err;
> > > > > }
> > > > >
> > > > > @@ -1470,12 +1496,15 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > struct list_head *pend_list;
> > > > > struct discard_cmd *dc, *tmp;
> > > > > struct blk_plug plug;
> > > > > - int i, issued = 0;
> > > > > + int i, err, issued = 0;
> > > > > bool io_interrupted = false;
> > > > > + bool retry;
> > > > >
> > > > > if (dpolicy->timeout != 0)
> > > > > f2fs_update_time(sbi, dpolicy->timeout);
> > > > >
> > > > > +retry:
> > > > > + retry = false;
> > > > > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > > > > if (dpolicy->timeout != 0 &&
> > > > > f2fs_time_over(sbi, dpolicy->timeout))
> > > > > @@ -1509,7 +1538,12 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > break;
> > > > > }
> > > > >
> > > > > - __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > > > + err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > > > + if (err == -EAGAIN) {
> > > > > + congestion_wait(BLK_RW_ASYNC,
> > > > > + DEFAULT_IO_TIMEOUT);
> > > > > + retry = true;
> > > > > + }
> > > > >
> > > > > if (issued >= dpolicy->max_requests)
> > > > > break;
> > > > > @@ -1522,6 +1556,9 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > break;
> > > > > }
> > > > >
> > > > > + if (retry)
> > > > > + goto retry;
> > > > > +
> > > > > if (!issued && io_interrupted)
> > > > > issued = -1;
> > > > >
> > > > > --
> > > > > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
> > > > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
> > >
> > > --
> > > --
> > > Sent by a consultant of the Qualcomm Innovation Center, Inc.
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
> --
> --
> Sent by a consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] f2fs: fix long latency due to discard during umount
2020-04-09 2:29 ` [f2fs-dev] " Jaegeuk Kim
(?)
@ 2020-04-09 11:47 ` Sahitya Tummala
2020-04-13 16:52 ` [f2fs-dev] " Jaegeuk Kim
-1 siblings, 1 reply; 19+ messages in thread
From: Sahitya Tummala @ 2020-04-09 11:47 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel, stummala
On Wed, Apr 08, 2020 at 07:29:28PM -0700, Jaegeuk Kim wrote:
> On 04/08, Sahitya Tummala wrote:
> > Hi Jaegeuk,
> >
> > On Fri, Apr 03, 2020 at 10:19:43AM -0700, Jaegeuk Kim wrote:
> > > On 04/01, Sahitya Tummala wrote:
> > > > Hi Jaegeuk,
> > > >
> > > > On Tue, Mar 31, 2020 at 11:46:55AM -0700, Jaegeuk Kim wrote:
> > > > > On 03/30, Sahitya Tummala wrote:
> > > > > > F2FS already has a default timeout of 5 secs for discards that
> > > > > > can be issued during umount, but it can take more than the 5 sec
> > > > > > timeout if the underlying UFS device queue is already full and there
> > > > > > are no more available free tags to be used. In that case, submit_bio()
> > > > > > will wait for the already queued discard requests to complete to get
> > > > > > a free tag, which can potentially take way more than 5 sec.
> > > > > >
> > > > > > Fix this by submitting the discard requests with REQ_NOWAIT
> > > > > > flags during umount. This will return -EAGAIN for UFS queue/tag full
> > > > > > scenario without waiting in the context of submit_bio(). The FS can
> > > > > > then handle these requests by retrying again within the stipulated
> > > > > > discard timeout period to avoid long latencies.
> > > > >
> > > > > Sorry, Sahitya, but, do we really need to do like this? How about just
> > > > > controlling # of outstanding discarding bios in __issue_discard_cmd()?
> > > >
> > > > Do you mean something like this?
> > > >
> > > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > > index 1a62b27..860dd43 100644
> > > > --- a/fs/f2fs/segment.c
> > > > +++ b/fs/f2fs/segment.c
> > > > @@ -1099,7 +1099,7 @@ static void __init_discard_policy(struct f2fs_sb_info *sbi,
> > > > } else if (discard_type == DPOLICY_FSTRIM) {
> > > > dpolicy->io_aware = false;
> > > > } else if (discard_type == DPOLICY_UMOUNT) {
> > > > - dpolicy->max_requests = UINT_MAX;
> > > > + dpolicy->max_requests = 30;
> > >
> > > Can we use max queue depth of the block device?
> >
> > I think it should be limited to 8 or 16 as Chao suggested, so that we can have
> > better control on the given timeout value? Thoughts?
>
> Where is 8 or 16 coming from? What about SSD? Sorry, it's unclear to me.
With this patch we now wait for a batch of discard requests in __issue_discard_cmd()
with a upper timeout of 5 sec. So, I thought that having a smaller batch of
discard requests would help us to avoid queuing more requests and end up waiting
for more time in __wait_all_discard_cmd(). Today we have DEF_MAX_DISCARD_REQUEST
as 8 for default max discard requests. If it too less, may be for umount
we can use 16 instead. But the idea is to avoid more wait time.
If you would like to propose to use max queue depth, we can get it from bdev as
sbi->sb->s_bdev->bd_queue->queue_depth. Is it okay to use it in our FS? or you
think it should be hardcoded to 32 (as most UFS/eMMC devices will have this as
its queue depth)? Please let me know.
Thanks,
>
> >
> > Thanks,
> >
> > >
> > > > dpolicy->io_aware = false;
> > > > /* we need to issue all to keep CP_TRIMMED_FLAG */
> > > > dpolicy->granularity = 1;
> > > > @@ -1470,12 +1470,14 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > struct list_head *pend_list;
> > > > struct discard_cmd *dc, *tmp;
> > > > struct blk_plug plug;
> > > > - int i, issued = 0;
> > > > + int i, issued;
> > > > bool io_interrupted = false;
> > > >
> > > > if (dpolicy->timeout != 0)
> > > > f2fs_update_time(sbi, dpolicy->timeout);
> > > >
> > > > +retry:
> > > > + issued = 0;
> > > > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > > > if (dpolicy->timeout != 0 &&
> > > > f2fs_time_over(sbi, dpolicy->timeout))
> > > > @@ -1522,6 +1524,11 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > break;
> > > > }
> > > >
> > > > + if (dpolicy->type == DPOLICY_UMOUNT && issued) {
> > > > + __wait_all_discard_cmd(sbi, dpolicy);
> > > > + goto retry;
> > > > + }
> > > > +
> > > > if (!issued && io_interrupted)
> > > > issued = -1;
> > > >
> > > > Thanks,
> > > >
> > > > >
> > > > > >
> > > > > > Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> > > > > > ---
> > > > > > v3:
> > > > > > -Handle the regression reported by Chao with v2.
> > > > > > -simplify the logic to split the dc with multiple bios incase any bio returns
> > > > > > EAGAIN and retry those new dc within 5 sec timeout.
> > > > > >
> > > > > > fs/f2fs/segment.c | 65 +++++++++++++++++++++++++++++++++++++++++++------------
> > > > > > 1 file changed, 51 insertions(+), 14 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > > > > index fb3e531..55d18c7 100644
> > > > > > --- a/fs/f2fs/segment.c
> > > > > > +++ b/fs/f2fs/segment.c
> > > > > > @@ -1029,13 +1029,16 @@ static void f2fs_submit_discard_endio(struct bio *bio)
> > > > > > struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> > > > > > unsigned long flags;
> > > > > >
> > > > > > - dc->error = blk_status_to_errno(bio->bi_status);
> > > > > > -
> > > > > > spin_lock_irqsave(&dc->lock, flags);
> > > > > > + if (!dc->error)
> > > > > > + dc->error = blk_status_to_errno(bio->bi_status);
> > > > > > +
> > > > > > dc->bio_ref--;
> > > > > > - if (!dc->bio_ref && dc->state == D_SUBMIT) {
> > > > > > - dc->state = D_DONE;
> > > > > > - complete_all(&dc->wait);
> > > > > > + if (!dc->bio_ref) {
> > > > > > + if (dc->error || dc->state == D_SUBMIT) {
> > > > > > + dc->state = D_DONE;
> > > > > > + complete_all(&dc->wait);
> > > > > > + }
> > > > > > }
> > > > > > spin_unlock_irqrestore(&dc->lock, flags);
> > > > > > bio_put(bio);
> > > > > > @@ -1124,10 +1127,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> > > > > > struct list_head *wait_list = (dpolicy->type == DPOLICY_FSTRIM) ?
> > > > > > &(dcc->fstrim_list) : &(dcc->wait_list);
> > > > > > - int flag = dpolicy->sync ? REQ_SYNC : 0;
> > > > > > + int flag;
> > > > > > block_t lstart, start, len, total_len;
> > > > > > int err = 0;
> > > > > >
> > > > > > + flag = dpolicy->sync ? REQ_SYNC : 0;
> > > > > > + flag |= dpolicy->type == DPOLICY_UMOUNT ? REQ_NOWAIT : 0;
> > > > > > +
> > > > > > if (dc->state != D_PREP)
> > > > > > return 0;
> > > > > >
> > > > > > @@ -1192,10 +1198,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > dc->bio_ref++;
> > > > > > spin_unlock_irqrestore(&dc->lock, flags);
> > > > > >
> > > > > > - atomic_inc(&dcc->queued_discard);
> > > > > > - dc->queued++;
> > > > > > - list_move_tail(&dc->list, wait_list);
> > > > > > -
> > > > > > /* sanity check on discard range */
> > > > > > __check_sit_bitmap(sbi, lstart, lstart + len);
> > > > > >
> > > > > > @@ -1203,6 +1205,29 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > bio->bi_end_io = f2fs_submit_discard_endio;
> > > > > > bio->bi_opf |= flag;
> > > > > > submit_bio(bio);
> > > > > > + if (flag & REQ_NOWAIT) {
> > > > > > + if (dc->error == -EAGAIN) {
> > > > > > + spin_lock_irqsave(&dc->lock, flags);
> > > > > > + dc->len -= len;
> > > > > > + if (!dc->len) {
> > > > > > + dc->len = total_len;
> > > > > > + dc->state = D_PREP;
> > > > > > + reinit_completion(&dc->wait);
> > > > > > + } else {
> > > > > > + dcc->undiscard_blks -= total_len;
> > > > > > + if (dc->state == D_PARTIAL)
> > > > > > + dc->state = D_SUBMIT;
> > > > > > + }
> > > > > > + err = dc->error;
> > > > > > + dc->error = 0;
> > > > > > + spin_unlock_irqrestore(&dc->lock, flags);
> > > > > > + break;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + atomic_inc(&dcc->queued_discard);
> > > > > > + dc->queued++;
> > > > > > + list_move_tail(&dc->list, wait_list);
> > > > > >
> > > > > > atomic_inc(&dcc->issued_discard);
> > > > > >
> > > > > > @@ -1214,8 +1239,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > len = total_len;
> > > > > > }
> > > > > >
> > > > > > - if (!err && len)
> > > > > > - __update_discard_tree_range(sbi, bdev, lstart, start, len);
> > > > > > + if ((!err || err == -EAGAIN) && total_len && dc->start != start)
> > > > > > + __update_discard_tree_range(sbi, bdev, lstart, start,
> > > > > > + total_len);
> > > > > > return err;
> > > > > > }
> > > > > >
> > > > > > @@ -1470,12 +1496,15 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > struct list_head *pend_list;
> > > > > > struct discard_cmd *dc, *tmp;
> > > > > > struct blk_plug plug;
> > > > > > - int i, issued = 0;
> > > > > > + int i, err, issued = 0;
> > > > > > bool io_interrupted = false;
> > > > > > + bool retry;
> > > > > >
> > > > > > if (dpolicy->timeout != 0)
> > > > > > f2fs_update_time(sbi, dpolicy->timeout);
> > > > > >
> > > > > > +retry:
> > > > > > + retry = false;
> > > > > > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > > > > > if (dpolicy->timeout != 0 &&
> > > > > > f2fs_time_over(sbi, dpolicy->timeout))
> > > > > > @@ -1509,7 +1538,12 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > break;
> > > > > > }
> > > > > >
> > > > > > - __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > > > > + err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > > > > + if (err == -EAGAIN) {
> > > > > > + congestion_wait(BLK_RW_ASYNC,
> > > > > > + DEFAULT_IO_TIMEOUT);
> > > > > > + retry = true;
> > > > > > + }
> > > > > >
> > > > > > if (issued >= dpolicy->max_requests)
> > > > > > break;
> > > > > > @@ -1522,6 +1556,9 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > break;
> > > > > > }
> > > > > >
> > > > > > + if (retry)
> > > > > > + goto retry;
> > > > > > +
> > > > > > if (!issued && io_interrupted)
> > > > > > issued = -1;
> > > > > >
> > > > > > --
> > > > > > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
> > > > > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
> > > >
> > > > --
> > > > --
> > > > Sent by a consultant of the Qualcomm Innovation Center, Inc.
> > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
> >
> > --
> > --
> > Sent by a consultant of the Qualcomm Innovation Center, Inc.
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] f2fs: fix long latency due to discard during umount
2020-04-09 11:47 ` Sahitya Tummala
@ 2020-04-13 16:52 ` Jaegeuk Kim
0 siblings, 0 replies; 19+ messages in thread
From: Jaegeuk Kim @ 2020-04-13 16:52 UTC (permalink / raw)
To: Sahitya Tummala; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
On 04/09, Sahitya Tummala wrote:
> On Wed, Apr 08, 2020 at 07:29:28PM -0700, Jaegeuk Kim wrote:
> > On 04/08, Sahitya Tummala wrote:
> > > Hi Jaegeuk,
> > >
> > > On Fri, Apr 03, 2020 at 10:19:43AM -0700, Jaegeuk Kim wrote:
> > > > On 04/01, Sahitya Tummala wrote:
> > > > > Hi Jaegeuk,
> > > > >
> > > > > On Tue, Mar 31, 2020 at 11:46:55AM -0700, Jaegeuk Kim wrote:
> > > > > > On 03/30, Sahitya Tummala wrote:
> > > > > > > F2FS already has a default timeout of 5 secs for discards that
> > > > > > > can be issued during umount, but it can take more than the 5 sec
> > > > > > > timeout if the underlying UFS device queue is already full and there
> > > > > > > are no more available free tags to be used. In that case, submit_bio()
> > > > > > > will wait for the already queued discard requests to complete to get
> > > > > > > a free tag, which can potentially take way more than 5 sec.
> > > > > > >
> > > > > > > Fix this by submitting the discard requests with REQ_NOWAIT
> > > > > > > flags during umount. This will return -EAGAIN for UFS queue/tag full
> > > > > > > scenario without waiting in the context of submit_bio(). The FS can
> > > > > > > then handle these requests by retrying again within the stipulated
> > > > > > > discard timeout period to avoid long latencies.
> > > > > >
> > > > > > Sorry, Sahitya, but, do we really need to do like this? How about just
> > > > > > controlling # of outstanding discarding bios in __issue_discard_cmd()?
> > > > >
> > > > > Do you mean something like this?
> > > > >
> > > > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > > > index 1a62b27..860dd43 100644
> > > > > --- a/fs/f2fs/segment.c
> > > > > +++ b/fs/f2fs/segment.c
> > > > > @@ -1099,7 +1099,7 @@ static void __init_discard_policy(struct f2fs_sb_info *sbi,
> > > > > } else if (discard_type == DPOLICY_FSTRIM) {
> > > > > dpolicy->io_aware = false;
> > > > > } else if (discard_type == DPOLICY_UMOUNT) {
> > > > > - dpolicy->max_requests = UINT_MAX;
> > > > > + dpolicy->max_requests = 30;
> > > >
> > > > Can we use max queue depth of the block device?
> > >
> > > I think it should be limited to 8 or 16 as Chao suggested, so that we can have
> > > better control on the given timeout value? Thoughts?
> >
> > Where is 8 or 16 coming from? What about SSD? Sorry, it's unclear to me.
>
> With this patch we now wait for a batch of discard requests in __issue_discard_cmd()
> with a upper timeout of 5 sec. So, I thought that having a smaller batch of
> discard requests would help us to avoid queuing more requests and end up waiting
> for more time in __wait_all_discard_cmd(). Today we have DEF_MAX_DISCARD_REQUEST
> as 8 for default max discard requests. If it too less, may be for umount
> we can use 16 instead. But the idea is to avoid more wait time.
Ok, then, it seems we can just remove this line to use dpolicy->max_requests,
and retry issuing all the discard commands like below. Later, I thnk it'd be
fine to tune the max_requests based on bd_queue->queue_depth.
>
> If you would like to propose to use max queue depth, we can get it from bdev as
> sbi->sb->s_bdev->bd_queue->queue_depth. Is it okay to use it in our FS? or you
> think it should be hardcoded to 32 (as most UFS/eMMC devices will have this as
> its queue depth)? Please let me know.
>
> Thanks,
>
> >
> > >
> > > Thanks,
> > >
> > > >
> > > > > dpolicy->io_aware = false;
> > > > > /* we need to issue all to keep CP_TRIMMED_FLAG */
> > > > > dpolicy->granularity = 1;
> > > > > @@ -1470,12 +1470,14 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > struct list_head *pend_list;
> > > > > struct discard_cmd *dc, *tmp;
> > > > > struct blk_plug plug;
> > > > > - int i, issued = 0;
> > > > > + int i, issued;
> > > > > bool io_interrupted = false;
> > > > >
> > > > > if (dpolicy->timeout != 0)
> > > > > f2fs_update_time(sbi, dpolicy->timeout);
> > > > >
> > > > > +retry:
> > > > > + issued = 0;
> > > > > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > > > > if (dpolicy->timeout != 0 &&
> > > > > f2fs_time_over(sbi, dpolicy->timeout))
> > > > > @@ -1522,6 +1524,11 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > break;
> > > > > }
> > > > >
> > > > > + if (dpolicy->type == DPOLICY_UMOUNT && issued) {
> > > > > + __wait_all_discard_cmd(sbi, dpolicy);
> > > > > + goto retry;
> > > > > + }
> > > > > +
> > > > > if (!issued && io_interrupted)
> > > > > issued = -1;
> > > > >
> > > > > Thanks,
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> > > > > > > ---
> > > > > > > v3:
> > > > > > > -Handle the regression reported by Chao with v2.
> > > > > > > -simplify the logic to split the dc with multiple bios incase any bio returns
> > > > > > > EAGAIN and retry those new dc within 5 sec timeout.
> > > > > > >
> > > > > > > fs/f2fs/segment.c | 65 +++++++++++++++++++++++++++++++++++++++++++------------
> > > > > > > 1 file changed, 51 insertions(+), 14 deletions(-)
> > > > > > >
> > > > > > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > > > > > index fb3e531..55d18c7 100644
> > > > > > > --- a/fs/f2fs/segment.c
> > > > > > > +++ b/fs/f2fs/segment.c
> > > > > > > @@ -1029,13 +1029,16 @@ static void f2fs_submit_discard_endio(struct bio *bio)
> > > > > > > struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> > > > > > > unsigned long flags;
> > > > > > >
> > > > > > > - dc->error = blk_status_to_errno(bio->bi_status);
> > > > > > > -
> > > > > > > spin_lock_irqsave(&dc->lock, flags);
> > > > > > > + if (!dc->error)
> > > > > > > + dc->error = blk_status_to_errno(bio->bi_status);
> > > > > > > +
> > > > > > > dc->bio_ref--;
> > > > > > > - if (!dc->bio_ref && dc->state == D_SUBMIT) {
> > > > > > > - dc->state = D_DONE;
> > > > > > > - complete_all(&dc->wait);
> > > > > > > + if (!dc->bio_ref) {
> > > > > > > + if (dc->error || dc->state == D_SUBMIT) {
> > > > > > > + dc->state = D_DONE;
> > > > > > > + complete_all(&dc->wait);
> > > > > > > + }
> > > > > > > }
> > > > > > > spin_unlock_irqrestore(&dc->lock, flags);
> > > > > > > bio_put(bio);
> > > > > > > @@ -1124,10 +1127,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> > > > > > > struct list_head *wait_list = (dpolicy->type == DPOLICY_FSTRIM) ?
> > > > > > > &(dcc->fstrim_list) : &(dcc->wait_list);
> > > > > > > - int flag = dpolicy->sync ? REQ_SYNC : 0;
> > > > > > > + int flag;
> > > > > > > block_t lstart, start, len, total_len;
> > > > > > > int err = 0;
> > > > > > >
> > > > > > > + flag = dpolicy->sync ? REQ_SYNC : 0;
> > > > > > > + flag |= dpolicy->type == DPOLICY_UMOUNT ? REQ_NOWAIT : 0;
> > > > > > > +
> > > > > > > if (dc->state != D_PREP)
> > > > > > > return 0;
> > > > > > >
> > > > > > > @@ -1192,10 +1198,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > dc->bio_ref++;
> > > > > > > spin_unlock_irqrestore(&dc->lock, flags);
> > > > > > >
> > > > > > > - atomic_inc(&dcc->queued_discard);
> > > > > > > - dc->queued++;
> > > > > > > - list_move_tail(&dc->list, wait_list);
> > > > > > > -
> > > > > > > /* sanity check on discard range */
> > > > > > > __check_sit_bitmap(sbi, lstart, lstart + len);
> > > > > > >
> > > > > > > @@ -1203,6 +1205,29 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > bio->bi_end_io = f2fs_submit_discard_endio;
> > > > > > > bio->bi_opf |= flag;
> > > > > > > submit_bio(bio);
> > > > > > > + if (flag & REQ_NOWAIT) {
> > > > > > > + if (dc->error == -EAGAIN) {
> > > > > > > + spin_lock_irqsave(&dc->lock, flags);
> > > > > > > + dc->len -= len;
> > > > > > > + if (!dc->len) {
> > > > > > > + dc->len = total_len;
> > > > > > > + dc->state = D_PREP;
> > > > > > > + reinit_completion(&dc->wait);
> > > > > > > + } else {
> > > > > > > + dcc->undiscard_blks -= total_len;
> > > > > > > + if (dc->state == D_PARTIAL)
> > > > > > > + dc->state = D_SUBMIT;
> > > > > > > + }
> > > > > > > + err = dc->error;
> > > > > > > + dc->error = 0;
> > > > > > > + spin_unlock_irqrestore(&dc->lock, flags);
> > > > > > > + break;
> > > > > > > + }
> > > > > > > + }
> > > > > > > +
> > > > > > > + atomic_inc(&dcc->queued_discard);
> > > > > > > + dc->queued++;
> > > > > > > + list_move_tail(&dc->list, wait_list);
> > > > > > >
> > > > > > > atomic_inc(&dcc->issued_discard);
> > > > > > >
> > > > > > > @@ -1214,8 +1239,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > len = total_len;
> > > > > > > }
> > > > > > >
> > > > > > > - if (!err && len)
> > > > > > > - __update_discard_tree_range(sbi, bdev, lstart, start, len);
> > > > > > > + if ((!err || err == -EAGAIN) && total_len && dc->start != start)
> > > > > > > + __update_discard_tree_range(sbi, bdev, lstart, start,
> > > > > > > + total_len);
> > > > > > > return err;
> > > > > > > }
> > > > > > >
> > > > > > > @@ -1470,12 +1496,15 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > struct list_head *pend_list;
> > > > > > > struct discard_cmd *dc, *tmp;
> > > > > > > struct blk_plug plug;
> > > > > > > - int i, issued = 0;
> > > > > > > + int i, err, issued = 0;
> > > > > > > bool io_interrupted = false;
> > > > > > > + bool retry;
> > > > > > >
> > > > > > > if (dpolicy->timeout != 0)
> > > > > > > f2fs_update_time(sbi, dpolicy->timeout);
> > > > > > >
> > > > > > > +retry:
> > > > > > > + retry = false;
> > > > > > > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > > > > > > if (dpolicy->timeout != 0 &&
> > > > > > > f2fs_time_over(sbi, dpolicy->timeout))
> > > > > > > @@ -1509,7 +1538,12 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > break;
> > > > > > > }
> > > > > > >
> > > > > > > - __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > > > > > + err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > > > > > + if (err == -EAGAIN) {
> > > > > > > + congestion_wait(BLK_RW_ASYNC,
> > > > > > > + DEFAULT_IO_TIMEOUT);
> > > > > > > + retry = true;
> > > > > > > + }
> > > > > > >
> > > > > > > if (issued >= dpolicy->max_requests)
> > > > > > > break;
> > > > > > > @@ -1522,6 +1556,9 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > break;
> > > > > > > }
> > > > > > >
> > > > > > > + if (retry)
> > > > > > > + goto retry;
> > > > > > > +
> > > > > > > if (!issued && io_interrupted)
> > > > > > > issued = -1;
> > > > > > >
> > > > > > > --
> > > > > > > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
> > > > > > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
> > > > >
> > > > > --
> > > > > --
> > > > > Sent by a consultant of the Qualcomm Innovation Center, Inc.
> > > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
> > >
> > > --
> > > --
> > > Sent by a consultant of the Qualcomm Innovation Center, Inc.
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
> --
> --
> Sent by a consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: fix long latency due to discard during umount
@ 2020-04-13 16:52 ` Jaegeuk Kim
0 siblings, 0 replies; 19+ messages in thread
From: Jaegeuk Kim @ 2020-04-13 16:52 UTC (permalink / raw)
To: Sahitya Tummala; +Cc: linux-kernel, linux-f2fs-devel
On 04/09, Sahitya Tummala wrote:
> On Wed, Apr 08, 2020 at 07:29:28PM -0700, Jaegeuk Kim wrote:
> > On 04/08, Sahitya Tummala wrote:
> > > Hi Jaegeuk,
> > >
> > > On Fri, Apr 03, 2020 at 10:19:43AM -0700, Jaegeuk Kim wrote:
> > > > On 04/01, Sahitya Tummala wrote:
> > > > > Hi Jaegeuk,
> > > > >
> > > > > On Tue, Mar 31, 2020 at 11:46:55AM -0700, Jaegeuk Kim wrote:
> > > > > > On 03/30, Sahitya Tummala wrote:
> > > > > > > F2FS already has a default timeout of 5 secs for discards that
> > > > > > > can be issued during umount, but it can take more than the 5 sec
> > > > > > > timeout if the underlying UFS device queue is already full and there
> > > > > > > are no more available free tags to be used. In that case, submit_bio()
> > > > > > > will wait for the already queued discard requests to complete to get
> > > > > > > a free tag, which can potentially take way more than 5 sec.
> > > > > > >
> > > > > > > Fix this by submitting the discard requests with REQ_NOWAIT
> > > > > > > flags during umount. This will return -EAGAIN for UFS queue/tag full
> > > > > > > scenario without waiting in the context of submit_bio(). The FS can
> > > > > > > then handle these requests by retrying again within the stipulated
> > > > > > > discard timeout period to avoid long latencies.
> > > > > >
> > > > > > Sorry, Sahitya, but, do we really need to do like this? How about just
> > > > > > controlling # of outstanding discarding bios in __issue_discard_cmd()?
> > > > >
> > > > > Do you mean something like this?
> > > > >
> > > > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > > > index 1a62b27..860dd43 100644
> > > > > --- a/fs/f2fs/segment.c
> > > > > +++ b/fs/f2fs/segment.c
> > > > > @@ -1099,7 +1099,7 @@ static void __init_discard_policy(struct f2fs_sb_info *sbi,
> > > > > } else if (discard_type == DPOLICY_FSTRIM) {
> > > > > dpolicy->io_aware = false;
> > > > > } else if (discard_type == DPOLICY_UMOUNT) {
> > > > > - dpolicy->max_requests = UINT_MAX;
> > > > > + dpolicy->max_requests = 30;
> > > >
> > > > Can we use max queue depth of the block device?
> > >
> > > I think it should be limited to 8 or 16 as Chao suggested, so that we can have
> > > better control on the given timeout value? Thoughts?
> >
> > Where is 8 or 16 coming from? What about SSD? Sorry, it's unclear to me.
>
> With this patch we now wait for a batch of discard requests in __issue_discard_cmd()
> with a upper timeout of 5 sec. So, I thought that having a smaller batch of
> discard requests would help us to avoid queuing more requests and end up waiting
> for more time in __wait_all_discard_cmd(). Today we have DEF_MAX_DISCARD_REQUEST
> as 8 for default max discard requests. If it too less, may be for umount
> we can use 16 instead. But the idea is to avoid more wait time.
Ok, then, it seems we can just remove this line to use dpolicy->max_requests,
and retry issuing all the discard commands like below. Later, I thnk it'd be
fine to tune the max_requests based on bd_queue->queue_depth.
>
> If you would like to propose to use max queue depth, we can get it from bdev as
> sbi->sb->s_bdev->bd_queue->queue_depth. Is it okay to use it in our FS? or you
> think it should be hardcoded to 32 (as most UFS/eMMC devices will have this as
> its queue depth)? Please let me know.
>
> Thanks,
>
> >
> > >
> > > Thanks,
> > >
> > > >
> > > > > dpolicy->io_aware = false;
> > > > > /* we need to issue all to keep CP_TRIMMED_FLAG */
> > > > > dpolicy->granularity = 1;
> > > > > @@ -1470,12 +1470,14 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > struct list_head *pend_list;
> > > > > struct discard_cmd *dc, *tmp;
> > > > > struct blk_plug plug;
> > > > > - int i, issued = 0;
> > > > > + int i, issued;
> > > > > bool io_interrupted = false;
> > > > >
> > > > > if (dpolicy->timeout != 0)
> > > > > f2fs_update_time(sbi, dpolicy->timeout);
> > > > >
> > > > > +retry:
> > > > > + issued = 0;
> > > > > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > > > > if (dpolicy->timeout != 0 &&
> > > > > f2fs_time_over(sbi, dpolicy->timeout))
> > > > > @@ -1522,6 +1524,11 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > break;
> > > > > }
> > > > >
> > > > > + if (dpolicy->type == DPOLICY_UMOUNT && issued) {
> > > > > + __wait_all_discard_cmd(sbi, dpolicy);
> > > > > + goto retry;
> > > > > + }
> > > > > +
> > > > > if (!issued && io_interrupted)
> > > > > issued = -1;
> > > > >
> > > > > Thanks,
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> > > > > > > ---
> > > > > > > v3:
> > > > > > > -Handle the regression reported by Chao with v2.
> > > > > > > -simplify the logic to split the dc with multiple bios incase any bio returns
> > > > > > > EAGAIN and retry those new dc within 5 sec timeout.
> > > > > > >
> > > > > > > fs/f2fs/segment.c | 65 +++++++++++++++++++++++++++++++++++++++++++------------
> > > > > > > 1 file changed, 51 insertions(+), 14 deletions(-)
> > > > > > >
> > > > > > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > > > > > index fb3e531..55d18c7 100644
> > > > > > > --- a/fs/f2fs/segment.c
> > > > > > > +++ b/fs/f2fs/segment.c
> > > > > > > @@ -1029,13 +1029,16 @@ static void f2fs_submit_discard_endio(struct bio *bio)
> > > > > > > struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> > > > > > > unsigned long flags;
> > > > > > >
> > > > > > > - dc->error = blk_status_to_errno(bio->bi_status);
> > > > > > > -
> > > > > > > spin_lock_irqsave(&dc->lock, flags);
> > > > > > > + if (!dc->error)
> > > > > > > + dc->error = blk_status_to_errno(bio->bi_status);
> > > > > > > +
> > > > > > > dc->bio_ref--;
> > > > > > > - if (!dc->bio_ref && dc->state == D_SUBMIT) {
> > > > > > > - dc->state = D_DONE;
> > > > > > > - complete_all(&dc->wait);
> > > > > > > + if (!dc->bio_ref) {
> > > > > > > + if (dc->error || dc->state == D_SUBMIT) {
> > > > > > > + dc->state = D_DONE;
> > > > > > > + complete_all(&dc->wait);
> > > > > > > + }
> > > > > > > }
> > > > > > > spin_unlock_irqrestore(&dc->lock, flags);
> > > > > > > bio_put(bio);
> > > > > > > @@ -1124,10 +1127,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> > > > > > > struct list_head *wait_list = (dpolicy->type == DPOLICY_FSTRIM) ?
> > > > > > > &(dcc->fstrim_list) : &(dcc->wait_list);
> > > > > > > - int flag = dpolicy->sync ? REQ_SYNC : 0;
> > > > > > > + int flag;
> > > > > > > block_t lstart, start, len, total_len;
> > > > > > > int err = 0;
> > > > > > >
> > > > > > > + flag = dpolicy->sync ? REQ_SYNC : 0;
> > > > > > > + flag |= dpolicy->type == DPOLICY_UMOUNT ? REQ_NOWAIT : 0;
> > > > > > > +
> > > > > > > if (dc->state != D_PREP)
> > > > > > > return 0;
> > > > > > >
> > > > > > > @@ -1192,10 +1198,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > dc->bio_ref++;
> > > > > > > spin_unlock_irqrestore(&dc->lock, flags);
> > > > > > >
> > > > > > > - atomic_inc(&dcc->queued_discard);
> > > > > > > - dc->queued++;
> > > > > > > - list_move_tail(&dc->list, wait_list);
> > > > > > > -
> > > > > > > /* sanity check on discard range */
> > > > > > > __check_sit_bitmap(sbi, lstart, lstart + len);
> > > > > > >
> > > > > > > @@ -1203,6 +1205,29 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > bio->bi_end_io = f2fs_submit_discard_endio;
> > > > > > > bio->bi_opf |= flag;
> > > > > > > submit_bio(bio);
> > > > > > > + if (flag & REQ_NOWAIT) {
> > > > > > > + if (dc->error == -EAGAIN) {
> > > > > > > + spin_lock_irqsave(&dc->lock, flags);
> > > > > > > + dc->len -= len;
> > > > > > > + if (!dc->len) {
> > > > > > > + dc->len = total_len;
> > > > > > > + dc->state = D_PREP;
> > > > > > > + reinit_completion(&dc->wait);
> > > > > > > + } else {
> > > > > > > + dcc->undiscard_blks -= total_len;
> > > > > > > + if (dc->state == D_PARTIAL)
> > > > > > > + dc->state = D_SUBMIT;
> > > > > > > + }
> > > > > > > + err = dc->error;
> > > > > > > + dc->error = 0;
> > > > > > > + spin_unlock_irqrestore(&dc->lock, flags);
> > > > > > > + break;
> > > > > > > + }
> > > > > > > + }
> > > > > > > +
> > > > > > > + atomic_inc(&dcc->queued_discard);
> > > > > > > + dc->queued++;
> > > > > > > + list_move_tail(&dc->list, wait_list);
> > > > > > >
> > > > > > > atomic_inc(&dcc->issued_discard);
> > > > > > >
> > > > > > > @@ -1214,8 +1239,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > len = total_len;
> > > > > > > }
> > > > > > >
> > > > > > > - if (!err && len)
> > > > > > > - __update_discard_tree_range(sbi, bdev, lstart, start, len);
> > > > > > > + if ((!err || err == -EAGAIN) && total_len && dc->start != start)
> > > > > > > + __update_discard_tree_range(sbi, bdev, lstart, start,
> > > > > > > + total_len);
> > > > > > > return err;
> > > > > > > }
> > > > > > >
> > > > > > > @@ -1470,12 +1496,15 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > struct list_head *pend_list;
> > > > > > > struct discard_cmd *dc, *tmp;
> > > > > > > struct blk_plug plug;
> > > > > > > - int i, issued = 0;
> > > > > > > + int i, err, issued = 0;
> > > > > > > bool io_interrupted = false;
> > > > > > > + bool retry;
> > > > > > >
> > > > > > > if (dpolicy->timeout != 0)
> > > > > > > f2fs_update_time(sbi, dpolicy->timeout);
> > > > > > >
> > > > > > > +retry:
> > > > > > > + retry = false;
> > > > > > > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > > > > > > if (dpolicy->timeout != 0 &&
> > > > > > > f2fs_time_over(sbi, dpolicy->timeout))
> > > > > > > @@ -1509,7 +1538,12 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > break;
> > > > > > > }
> > > > > > >
> > > > > > > - __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > > > > > + err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > > > > > + if (err == -EAGAIN) {
> > > > > > > + congestion_wait(BLK_RW_ASYNC,
> > > > > > > + DEFAULT_IO_TIMEOUT);
> > > > > > > + retry = true;
> > > > > > > + }
> > > > > > >
> > > > > > > if (issued >= dpolicy->max_requests)
> > > > > > > break;
> > > > > > > @@ -1522,6 +1556,9 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > break;
> > > > > > > }
> > > > > > >
> > > > > > > + if (retry)
> > > > > > > + goto retry;
> > > > > > > +
> > > > > > > if (!issued && io_interrupted)
> > > > > > > issued = -1;
> > > > > > >
> > > > > > > --
> > > > > > > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
> > > > > > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
> > > > >
> > > > > --
> > > > > --
> > > > > Sent by a consultant of the Qualcomm Innovation Center, Inc.
> > > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
> > >
> > > --
> > > --
> > > Sent by a consultant of the Qualcomm Innovation Center, Inc.
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
> --
> --
> Sent by a consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: fix long latency due to discard during umount
2020-04-13 16:52 ` [f2fs-dev] " Jaegeuk Kim
@ 2020-04-14 17:45 ` Jaegeuk Kim
-1 siblings, 0 replies; 19+ messages in thread
From: Jaegeuk Kim @ 2020-04-14 17:45 UTC (permalink / raw)
To: Sahitya Tummala; +Cc: linux-kernel, linux-f2fs-devel
Hi Sahitya,
Could you please post the revised patch?
Thanks,
On 04/13, Jaegeuk Kim wrote:
> On 04/09, Sahitya Tummala wrote:
> > On Wed, Apr 08, 2020 at 07:29:28PM -0700, Jaegeuk Kim wrote:
> > > On 04/08, Sahitya Tummala wrote:
> > > > Hi Jaegeuk,
> > > >
> > > > On Fri, Apr 03, 2020 at 10:19:43AM -0700, Jaegeuk Kim wrote:
> > > > > On 04/01, Sahitya Tummala wrote:
> > > > > > Hi Jaegeuk,
> > > > > >
> > > > > > On Tue, Mar 31, 2020 at 11:46:55AM -0700, Jaegeuk Kim wrote:
> > > > > > > On 03/30, Sahitya Tummala wrote:
> > > > > > > > F2FS already has a default timeout of 5 secs for discards that
> > > > > > > > can be issued during umount, but it can take more than the 5 sec
> > > > > > > > timeout if the underlying UFS device queue is already full and there
> > > > > > > > are no more available free tags to be used. In that case, submit_bio()
> > > > > > > > will wait for the already queued discard requests to complete to get
> > > > > > > > a free tag, which can potentially take way more than 5 sec.
> > > > > > > >
> > > > > > > > Fix this by submitting the discard requests with REQ_NOWAIT
> > > > > > > > flags during umount. This will return -EAGAIN for UFS queue/tag full
> > > > > > > > scenario without waiting in the context of submit_bio(). The FS can
> > > > > > > > then handle these requests by retrying again within the stipulated
> > > > > > > > discard timeout period to avoid long latencies.
> > > > > > >
> > > > > > > Sorry, Sahitya, but, do we really need to do like this? How about just
> > > > > > > controlling # of outstanding discarding bios in __issue_discard_cmd()?
> > > > > >
> > > > > > Do you mean something like this?
> > > > > >
> > > > > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > > > > index 1a62b27..860dd43 100644
> > > > > > --- a/fs/f2fs/segment.c
> > > > > > +++ b/fs/f2fs/segment.c
> > > > > > @@ -1099,7 +1099,7 @@ static void __init_discard_policy(struct f2fs_sb_info *sbi,
> > > > > > } else if (discard_type == DPOLICY_FSTRIM) {
> > > > > > dpolicy->io_aware = false;
> > > > > > } else if (discard_type == DPOLICY_UMOUNT) {
> > > > > > - dpolicy->max_requests = UINT_MAX;
> > > > > > + dpolicy->max_requests = 30;
> > > > >
> > > > > Can we use max queue depth of the block device?
> > > >
> > > > I think it should be limited to 8 or 16 as Chao suggested, so that we can have
> > > > better control on the given timeout value? Thoughts?
> > >
> > > Where is 8 or 16 coming from? What about SSD? Sorry, it's unclear to me.
> >
> > With this patch we now wait for a batch of discard requests in __issue_discard_cmd()
> > with a upper timeout of 5 sec. So, I thought that having a smaller batch of
> > discard requests would help us to avoid queuing more requests and end up waiting
> > for more time in __wait_all_discard_cmd(). Today we have DEF_MAX_DISCARD_REQUEST
> > as 8 for default max discard requests. If it too less, may be for umount
> > we can use 16 instead. But the idea is to avoid more wait time.
>
> Ok, then, it seems we can just remove this line to use dpolicy->max_requests,
> and retry issuing all the discard commands like below. Later, I thnk it'd be
> fine to tune the max_requests based on bd_queue->queue_depth.
>
> >
> > If you would like to propose to use max queue depth, we can get it from bdev as
> > sbi->sb->s_bdev->bd_queue->queue_depth. Is it okay to use it in our FS? or you
> > think it should be hardcoded to 32 (as most UFS/eMMC devices will have this as
> > its queue depth)? Please let me know.
> >
> > Thanks,
> >
> > >
> > > >
> > > > Thanks,
> > > >
> > > > >
> > > > > > dpolicy->io_aware = false;
> > > > > > /* we need to issue all to keep CP_TRIMMED_FLAG */
> > > > > > dpolicy->granularity = 1;
> > > > > > @@ -1470,12 +1470,14 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > struct list_head *pend_list;
> > > > > > struct discard_cmd *dc, *tmp;
> > > > > > struct blk_plug plug;
> > > > > > - int i, issued = 0;
> > > > > > + int i, issued;
> > > > > > bool io_interrupted = false;
> > > > > >
> > > > > > if (dpolicy->timeout != 0)
> > > > > > f2fs_update_time(sbi, dpolicy->timeout);
> > > > > >
> > > > > > +retry:
> > > > > > + issued = 0;
> > > > > > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > > > > > if (dpolicy->timeout != 0 &&
> > > > > > f2fs_time_over(sbi, dpolicy->timeout))
> > > > > > @@ -1522,6 +1524,11 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > break;
> > > > > > }
> > > > > >
> > > > > > + if (dpolicy->type == DPOLICY_UMOUNT && issued) {
> > > > > > + __wait_all_discard_cmd(sbi, dpolicy);
> > > > > > + goto retry;
> > > > > > + }
> > > > > > +
> > > > > > if (!issued && io_interrupted)
> > > > > > issued = -1;
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> > > > > > > > ---
> > > > > > > > v3:
> > > > > > > > -Handle the regression reported by Chao with v2.
> > > > > > > > -simplify the logic to split the dc with multiple bios incase any bio returns
> > > > > > > > EAGAIN and retry those new dc within 5 sec timeout.
> > > > > > > >
> > > > > > > > fs/f2fs/segment.c | 65 +++++++++++++++++++++++++++++++++++++++++++------------
> > > > > > > > 1 file changed, 51 insertions(+), 14 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > > > > > > index fb3e531..55d18c7 100644
> > > > > > > > --- a/fs/f2fs/segment.c
> > > > > > > > +++ b/fs/f2fs/segment.c
> > > > > > > > @@ -1029,13 +1029,16 @@ static void f2fs_submit_discard_endio(struct bio *bio)
> > > > > > > > struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> > > > > > > > unsigned long flags;
> > > > > > > >
> > > > > > > > - dc->error = blk_status_to_errno(bio->bi_status);
> > > > > > > > -
> > > > > > > > spin_lock_irqsave(&dc->lock, flags);
> > > > > > > > + if (!dc->error)
> > > > > > > > + dc->error = blk_status_to_errno(bio->bi_status);
> > > > > > > > +
> > > > > > > > dc->bio_ref--;
> > > > > > > > - if (!dc->bio_ref && dc->state == D_SUBMIT) {
> > > > > > > > - dc->state = D_DONE;
> > > > > > > > - complete_all(&dc->wait);
> > > > > > > > + if (!dc->bio_ref) {
> > > > > > > > + if (dc->error || dc->state == D_SUBMIT) {
> > > > > > > > + dc->state = D_DONE;
> > > > > > > > + complete_all(&dc->wait);
> > > > > > > > + }
> > > > > > > > }
> > > > > > > > spin_unlock_irqrestore(&dc->lock, flags);
> > > > > > > > bio_put(bio);
> > > > > > > > @@ -1124,10 +1127,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > > struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> > > > > > > > struct list_head *wait_list = (dpolicy->type == DPOLICY_FSTRIM) ?
> > > > > > > > &(dcc->fstrim_list) : &(dcc->wait_list);
> > > > > > > > - int flag = dpolicy->sync ? REQ_SYNC : 0;
> > > > > > > > + int flag;
> > > > > > > > block_t lstart, start, len, total_len;
> > > > > > > > int err = 0;
> > > > > > > >
> > > > > > > > + flag = dpolicy->sync ? REQ_SYNC : 0;
> > > > > > > > + flag |= dpolicy->type == DPOLICY_UMOUNT ? REQ_NOWAIT : 0;
> > > > > > > > +
> > > > > > > > if (dc->state != D_PREP)
> > > > > > > > return 0;
> > > > > > > >
> > > > > > > > @@ -1192,10 +1198,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > > dc->bio_ref++;
> > > > > > > > spin_unlock_irqrestore(&dc->lock, flags);
> > > > > > > >
> > > > > > > > - atomic_inc(&dcc->queued_discard);
> > > > > > > > - dc->queued++;
> > > > > > > > - list_move_tail(&dc->list, wait_list);
> > > > > > > > -
> > > > > > > > /* sanity check on discard range */
> > > > > > > > __check_sit_bitmap(sbi, lstart, lstart + len);
> > > > > > > >
> > > > > > > > @@ -1203,6 +1205,29 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > > bio->bi_end_io = f2fs_submit_discard_endio;
> > > > > > > > bio->bi_opf |= flag;
> > > > > > > > submit_bio(bio);
> > > > > > > > + if (flag & REQ_NOWAIT) {
> > > > > > > > + if (dc->error == -EAGAIN) {
> > > > > > > > + spin_lock_irqsave(&dc->lock, flags);
> > > > > > > > + dc->len -= len;
> > > > > > > > + if (!dc->len) {
> > > > > > > > + dc->len = total_len;
> > > > > > > > + dc->state = D_PREP;
> > > > > > > > + reinit_completion(&dc->wait);
> > > > > > > > + } else {
> > > > > > > > + dcc->undiscard_blks -= total_len;
> > > > > > > > + if (dc->state == D_PARTIAL)
> > > > > > > > + dc->state = D_SUBMIT;
> > > > > > > > + }
> > > > > > > > + err = dc->error;
> > > > > > > > + dc->error = 0;
> > > > > > > > + spin_unlock_irqrestore(&dc->lock, flags);
> > > > > > > > + break;
> > > > > > > > + }
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + atomic_inc(&dcc->queued_discard);
> > > > > > > > + dc->queued++;
> > > > > > > > + list_move_tail(&dc->list, wait_list);
> > > > > > > >
> > > > > > > > atomic_inc(&dcc->issued_discard);
> > > > > > > >
> > > > > > > > @@ -1214,8 +1239,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > > len = total_len;
> > > > > > > > }
> > > > > > > >
> > > > > > > > - if (!err && len)
> > > > > > > > - __update_discard_tree_range(sbi, bdev, lstart, start, len);
> > > > > > > > + if ((!err || err == -EAGAIN) && total_len && dc->start != start)
> > > > > > > > + __update_discard_tree_range(sbi, bdev, lstart, start,
> > > > > > > > + total_len);
> > > > > > > > return err;
> > > > > > > > }
> > > > > > > >
> > > > > > > > @@ -1470,12 +1496,15 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > > struct list_head *pend_list;
> > > > > > > > struct discard_cmd *dc, *tmp;
> > > > > > > > struct blk_plug plug;
> > > > > > > > - int i, issued = 0;
> > > > > > > > + int i, err, issued = 0;
> > > > > > > > bool io_interrupted = false;
> > > > > > > > + bool retry;
> > > > > > > >
> > > > > > > > if (dpolicy->timeout != 0)
> > > > > > > > f2fs_update_time(sbi, dpolicy->timeout);
> > > > > > > >
> > > > > > > > +retry:
> > > > > > > > + retry = false;
> > > > > > > > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > > > > > > > if (dpolicy->timeout != 0 &&
> > > > > > > > f2fs_time_over(sbi, dpolicy->timeout))
> > > > > > > > @@ -1509,7 +1538,12 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > > break;
> > > > > > > > }
> > > > > > > >
> > > > > > > > - __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > > > > > > + err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > > > > > > + if (err == -EAGAIN) {
> > > > > > > > + congestion_wait(BLK_RW_ASYNC,
> > > > > > > > + DEFAULT_IO_TIMEOUT);
> > > > > > > > + retry = true;
> > > > > > > > + }
> > > > > > > >
> > > > > > > > if (issued >= dpolicy->max_requests)
> > > > > > > > break;
> > > > > > > > @@ -1522,6 +1556,9 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > > break;
> > > > > > > > }
> > > > > > > >
> > > > > > > > + if (retry)
> > > > > > > > + goto retry;
> > > > > > > > +
> > > > > > > > if (!issued && io_interrupted)
> > > > > > > > issued = -1;
> > > > > > > >
> > > > > > > > --
> > > > > > > > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
> > > > > > > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
> > > > > >
> > > > > > --
> > > > > > --
> > > > > > Sent by a consultant of the Qualcomm Innovation Center, Inc.
> > > > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
> > > >
> > > > --
> > > > --
> > > > Sent by a consultant of the Qualcomm Innovation Center, Inc.
> > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
> >
> > --
> > --
> > Sent by a consultant of the Qualcomm Innovation Center, Inc.
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: fix long latency due to discard during umount
@ 2020-04-14 17:45 ` Jaegeuk Kim
0 siblings, 0 replies; 19+ messages in thread
From: Jaegeuk Kim @ 2020-04-14 17:45 UTC (permalink / raw)
To: Sahitya Tummala; +Cc: linux-kernel, linux-f2fs-devel
Hi Sahitya,
Could you please post the revised patch?
Thanks,
On 04/13, Jaegeuk Kim wrote:
> On 04/09, Sahitya Tummala wrote:
> > On Wed, Apr 08, 2020 at 07:29:28PM -0700, Jaegeuk Kim wrote:
> > > On 04/08, Sahitya Tummala wrote:
> > > > Hi Jaegeuk,
> > > >
> > > > On Fri, Apr 03, 2020 at 10:19:43AM -0700, Jaegeuk Kim wrote:
> > > > > On 04/01, Sahitya Tummala wrote:
> > > > > > Hi Jaegeuk,
> > > > > >
> > > > > > On Tue, Mar 31, 2020 at 11:46:55AM -0700, Jaegeuk Kim wrote:
> > > > > > > On 03/30, Sahitya Tummala wrote:
> > > > > > > > F2FS already has a default timeout of 5 secs for discards that
> > > > > > > > can be issued during umount, but it can take more than the 5 sec
> > > > > > > > timeout if the underlying UFS device queue is already full and there
> > > > > > > > are no more available free tags to be used. In that case, submit_bio()
> > > > > > > > will wait for the already queued discard requests to complete to get
> > > > > > > > a free tag, which can potentially take way more than 5 sec.
> > > > > > > >
> > > > > > > > Fix this by submitting the discard requests with REQ_NOWAIT
> > > > > > > > flags during umount. This will return -EAGAIN for UFS queue/tag full
> > > > > > > > scenario without waiting in the context of submit_bio(). The FS can
> > > > > > > > then handle these requests by retrying again within the stipulated
> > > > > > > > discard timeout period to avoid long latencies.
> > > > > > >
> > > > > > > Sorry, Sahitya, but, do we really need to do like this? How about just
> > > > > > > controlling # of outstanding discarding bios in __issue_discard_cmd()?
> > > > > >
> > > > > > Do you mean something like this?
> > > > > >
> > > > > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > > > > index 1a62b27..860dd43 100644
> > > > > > --- a/fs/f2fs/segment.c
> > > > > > +++ b/fs/f2fs/segment.c
> > > > > > @@ -1099,7 +1099,7 @@ static void __init_discard_policy(struct f2fs_sb_info *sbi,
> > > > > > } else if (discard_type == DPOLICY_FSTRIM) {
> > > > > > dpolicy->io_aware = false;
> > > > > > } else if (discard_type == DPOLICY_UMOUNT) {
> > > > > > - dpolicy->max_requests = UINT_MAX;
> > > > > > + dpolicy->max_requests = 30;
> > > > >
> > > > > Can we use max queue depth of the block device?
> > > >
> > > > I think it should be limited to 8 or 16 as Chao suggested, so that we can have
> > > > better control on the given timeout value? Thoughts?
> > >
> > > Where is 8 or 16 coming from? What about SSD? Sorry, it's unclear to me.
> >
> > With this patch we now wait for a batch of discard requests in __issue_discard_cmd()
> > with a upper timeout of 5 sec. So, I thought that having a smaller batch of
> > discard requests would help us to avoid queuing more requests and end up waiting
> > for more time in __wait_all_discard_cmd(). Today we have DEF_MAX_DISCARD_REQUEST
> > as 8 for default max discard requests. If it too less, may be for umount
> > we can use 16 instead. But the idea is to avoid more wait time.
>
> Ok, then, it seems we can just remove this line to use dpolicy->max_requests,
> and retry issuing all the discard commands like below. Later, I thnk it'd be
> fine to tune the max_requests based on bd_queue->queue_depth.
>
> >
> > If you would like to propose to use max queue depth, we can get it from bdev as
> > sbi->sb->s_bdev->bd_queue->queue_depth. Is it okay to use it in our FS? or you
> > think it should be hardcoded to 32 (as most UFS/eMMC devices will have this as
> > its queue depth)? Please let me know.
> >
> > Thanks,
> >
> > >
> > > >
> > > > Thanks,
> > > >
> > > > >
> > > > > > dpolicy->io_aware = false;
> > > > > > /* we need to issue all to keep CP_TRIMMED_FLAG */
> > > > > > dpolicy->granularity = 1;
> > > > > > @@ -1470,12 +1470,14 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > struct list_head *pend_list;
> > > > > > struct discard_cmd *dc, *tmp;
> > > > > > struct blk_plug plug;
> > > > > > - int i, issued = 0;
> > > > > > + int i, issued;
> > > > > > bool io_interrupted = false;
> > > > > >
> > > > > > if (dpolicy->timeout != 0)
> > > > > > f2fs_update_time(sbi, dpolicy->timeout);
> > > > > >
> > > > > > +retry:
> > > > > > + issued = 0;
> > > > > > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > > > > > if (dpolicy->timeout != 0 &&
> > > > > > f2fs_time_over(sbi, dpolicy->timeout))
> > > > > > @@ -1522,6 +1524,11 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > break;
> > > > > > }
> > > > > >
> > > > > > + if (dpolicy->type == DPOLICY_UMOUNT && issued) {
> > > > > > + __wait_all_discard_cmd(sbi, dpolicy);
> > > > > > + goto retry;
> > > > > > + }
> > > > > > +
> > > > > > if (!issued && io_interrupted)
> > > > > > issued = -1;
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> > > > > > > > ---
> > > > > > > > v3:
> > > > > > > > -Handle the regression reported by Chao with v2.
> > > > > > > > -simplify the logic to split the dc with multiple bios incase any bio returns
> > > > > > > > EAGAIN and retry those new dc within 5 sec timeout.
> > > > > > > >
> > > > > > > > fs/f2fs/segment.c | 65 +++++++++++++++++++++++++++++++++++++++++++------------
> > > > > > > > 1 file changed, 51 insertions(+), 14 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > > > > > > index fb3e531..55d18c7 100644
> > > > > > > > --- a/fs/f2fs/segment.c
> > > > > > > > +++ b/fs/f2fs/segment.c
> > > > > > > > @@ -1029,13 +1029,16 @@ static void f2fs_submit_discard_endio(struct bio *bio)
> > > > > > > > struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> > > > > > > > unsigned long flags;
> > > > > > > >
> > > > > > > > - dc->error = blk_status_to_errno(bio->bi_status);
> > > > > > > > -
> > > > > > > > spin_lock_irqsave(&dc->lock, flags);
> > > > > > > > + if (!dc->error)
> > > > > > > > + dc->error = blk_status_to_errno(bio->bi_status);
> > > > > > > > +
> > > > > > > > dc->bio_ref--;
> > > > > > > > - if (!dc->bio_ref && dc->state == D_SUBMIT) {
> > > > > > > > - dc->state = D_DONE;
> > > > > > > > - complete_all(&dc->wait);
> > > > > > > > + if (!dc->bio_ref) {
> > > > > > > > + if (dc->error || dc->state == D_SUBMIT) {
> > > > > > > > + dc->state = D_DONE;
> > > > > > > > + complete_all(&dc->wait);
> > > > > > > > + }
> > > > > > > > }
> > > > > > > > spin_unlock_irqrestore(&dc->lock, flags);
> > > > > > > > bio_put(bio);
> > > > > > > > @@ -1124,10 +1127,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > > struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> > > > > > > > struct list_head *wait_list = (dpolicy->type == DPOLICY_FSTRIM) ?
> > > > > > > > &(dcc->fstrim_list) : &(dcc->wait_list);
> > > > > > > > - int flag = dpolicy->sync ? REQ_SYNC : 0;
> > > > > > > > + int flag;
> > > > > > > > block_t lstart, start, len, total_len;
> > > > > > > > int err = 0;
> > > > > > > >
> > > > > > > > + flag = dpolicy->sync ? REQ_SYNC : 0;
> > > > > > > > + flag |= dpolicy->type == DPOLICY_UMOUNT ? REQ_NOWAIT : 0;
> > > > > > > > +
> > > > > > > > if (dc->state != D_PREP)
> > > > > > > > return 0;
> > > > > > > >
> > > > > > > > @@ -1192,10 +1198,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > > dc->bio_ref++;
> > > > > > > > spin_unlock_irqrestore(&dc->lock, flags);
> > > > > > > >
> > > > > > > > - atomic_inc(&dcc->queued_discard);
> > > > > > > > - dc->queued++;
> > > > > > > > - list_move_tail(&dc->list, wait_list);
> > > > > > > > -
> > > > > > > > /* sanity check on discard range */
> > > > > > > > __check_sit_bitmap(sbi, lstart, lstart + len);
> > > > > > > >
> > > > > > > > @@ -1203,6 +1205,29 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > > bio->bi_end_io = f2fs_submit_discard_endio;
> > > > > > > > bio->bi_opf |= flag;
> > > > > > > > submit_bio(bio);
> > > > > > > > + if (flag & REQ_NOWAIT) {
> > > > > > > > + if (dc->error == -EAGAIN) {
> > > > > > > > + spin_lock_irqsave(&dc->lock, flags);
> > > > > > > > + dc->len -= len;
> > > > > > > > + if (!dc->len) {
> > > > > > > > + dc->len = total_len;
> > > > > > > > + dc->state = D_PREP;
> > > > > > > > + reinit_completion(&dc->wait);
> > > > > > > > + } else {
> > > > > > > > + dcc->undiscard_blks -= total_len;
> > > > > > > > + if (dc->state == D_PARTIAL)
> > > > > > > > + dc->state = D_SUBMIT;
> > > > > > > > + }
> > > > > > > > + err = dc->error;
> > > > > > > > + dc->error = 0;
> > > > > > > > + spin_unlock_irqrestore(&dc->lock, flags);
> > > > > > > > + break;
> > > > > > > > + }
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + atomic_inc(&dcc->queued_discard);
> > > > > > > > + dc->queued++;
> > > > > > > > + list_move_tail(&dc->list, wait_list);
> > > > > > > >
> > > > > > > > atomic_inc(&dcc->issued_discard);
> > > > > > > >
> > > > > > > > @@ -1214,8 +1239,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > > len = total_len;
> > > > > > > > }
> > > > > > > >
> > > > > > > > - if (!err && len)
> > > > > > > > - __update_discard_tree_range(sbi, bdev, lstart, start, len);
> > > > > > > > + if ((!err || err == -EAGAIN) && total_len && dc->start != start)
> > > > > > > > + __update_discard_tree_range(sbi, bdev, lstart, start,
> > > > > > > > + total_len);
> > > > > > > > return err;
> > > > > > > > }
> > > > > > > >
> > > > > > > > @@ -1470,12 +1496,15 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > > struct list_head *pend_list;
> > > > > > > > struct discard_cmd *dc, *tmp;
> > > > > > > > struct blk_plug plug;
> > > > > > > > - int i, issued = 0;
> > > > > > > > + int i, err, issued = 0;
> > > > > > > > bool io_interrupted = false;
> > > > > > > > + bool retry;
> > > > > > > >
> > > > > > > > if (dpolicy->timeout != 0)
> > > > > > > > f2fs_update_time(sbi, dpolicy->timeout);
> > > > > > > >
> > > > > > > > +retry:
> > > > > > > > + retry = false;
> > > > > > > > for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> > > > > > > > if (dpolicy->timeout != 0 &&
> > > > > > > > f2fs_time_over(sbi, dpolicy->timeout))
> > > > > > > > @@ -1509,7 +1538,12 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > > break;
> > > > > > > > }
> > > > > > > >
> > > > > > > > - __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > > > > > > + err = __submit_discard_cmd(sbi, dpolicy, dc, &issued);
> > > > > > > > + if (err == -EAGAIN) {
> > > > > > > > + congestion_wait(BLK_RW_ASYNC,
> > > > > > > > + DEFAULT_IO_TIMEOUT);
> > > > > > > > + retry = true;
> > > > > > > > + }
> > > > > > > >
> > > > > > > > if (issued >= dpolicy->max_requests)
> > > > > > > > break;
> > > > > > > > @@ -1522,6 +1556,9 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> > > > > > > > break;
> > > > > > > > }
> > > > > > > >
> > > > > > > > + if (retry)
> > > > > > > > + goto retry;
> > > > > > > > +
> > > > > > > > if (!issued && io_interrupted)
> > > > > > > > issued = -1;
> > > > > > > >
> > > > > > > > --
> > > > > > > > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
> > > > > > > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
> > > > > >
> > > > > > --
> > > > > > --
> > > > > > Sent by a consultant of the Qualcomm Innovation Center, Inc.
> > > > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
> > > >
> > > > --
> > > > --
> > > > Sent by a consultant of the Qualcomm Innovation Center, Inc.
> > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
> >
> > --
> > --
> > Sent by a consultant of the Qualcomm Innovation Center, Inc.
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2020-04-14 17:46 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-30 6:45 [PATCH v3] f2fs: fix long latency due to discard during umount Sahitya Tummala
2020-03-30 8:57 ` kbuild test robot
2020-03-30 9:58 ` kbuild test robot
2020-03-31 18:46 ` Jaegeuk Kim
2020-03-31 18:46 ` [f2fs-dev] " Jaegeuk Kim
2020-04-01 9:22 ` Sahitya Tummala
2020-04-01 9:22 ` [f2fs-dev] " Sahitya Tummala
2020-04-02 9:32 ` Chao Yu
2020-04-02 9:32 ` [f2fs-dev] " Chao Yu
2020-04-03 17:19 ` Jaegeuk Kim
2020-04-03 17:19 ` [f2fs-dev] " Jaegeuk Kim
2020-04-08 9:00 ` Sahitya Tummala
2020-04-09 2:29 ` Jaegeuk Kim
2020-04-09 2:29 ` [f2fs-dev] " Jaegeuk Kim
2020-04-09 11:47 ` Sahitya Tummala
2020-04-13 16:52 ` Jaegeuk Kim
2020-04-13 16:52 ` [f2fs-dev] " Jaegeuk Kim
2020-04-14 17:45 ` Jaegeuk Kim
2020-04-14 17:45 ` Jaegeuk Kim
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.