| Message ID | 20201001071824.24995-1-colyli@suse.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] mmc: core: don't set limits.discard_granularity as 0 | expand |
On Thu, Oct 1, 2020 at 9:18 AM Coly Li <colyli@suse.de> wrote: > > In mmc_queue_setup_discard() the mmc driver queue's discard_granularity > might be set as 0 (when card->pref_erase > max_discard) while the mmc > device still declares to support discard operation. This is buggy and > triggered the following kernel warning message, > > WARNING: CPU: 0 PID: 135 at __blkdev_issue_discard+0x200/0x294 > CPU: 0 PID: 135 Comm: f2fs_discard-17 Not tainted 5.9.0-rc6 #1 > Hardware name: Google Kevin (DT) > pstate: 00000005 (nzcv daif -PAN -UAO BTYPE=--) > pc : __blkdev_issue_discard+0x200/0x294 > lr : __blkdev_issue_discard+0x54/0x294 > sp : ffff800011dd3b10 > x29: ffff800011dd3b10 x28: 0000000000000000 x27: ffff800011dd3cc4 x26: ffff800011dd3e18 x25: 000000000004e69b x24: 0000000000000c40 x23: ffff0000f1deaaf0 x22: ffff0000f2849200 x21: 00000000002734d8 x20: 0000000000000008 x19: 0000000000000000 x18: 0000000000000000 x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 x14: 0000000000000394 x13: 0000000000000000 x12: 0000000000000000 x11: 0000000000000000 x10: 00000000000008b0 x9 : ffff800011dd3cb0 x8 : 000000000004e69b x7 : 0000000000000000 x6 : ffff0000f1926400 x5 : ffff0000f1940800 x4 : 0000000000000000 x3 : 0000000000000c40 x2 : 0000000000000008 x1 : 00000000002734d8 x0 : 0000000000000000 Call trace: > __blkdev_issue_discard+0x200/0x294 > __submit_discard_cmd+0x128/0x374 > __issue_discard_cmd_orderly+0x188/0x244 > __issue_discard_cmd+0x2e8/0x33c > issue_discard_thread+0xe8/0x2f0 > kthread+0x11c/0x120 > ret_from_fork+0x10/0x1c > ---[ end trace e4c8023d33dfe77a ]--- > > This patch fixes the issue by setting discard_granularity as SECTOR_SIZE > instead of 0 when (card->pref_erase > max_discard) is true. Now no more > complain from __blkdev_issue_discard() for the improper value of discard > granularity. > > This issue is exposed after commit b35fd7422c2f ("block: check queue's > limits.discard_granularity in __blkdev_issue_discard()"), a "Fixes:" tag > is also added for the commit to make sure people won't miss this patch > after applying the change of __blkdev_issue_discard(). > > Fixes: e056a1b5b67b ("mmc: queue: let host controllers specify maximum discard timeout") > Fixes: b35fd7422c2f ("block: check queue's limits.discard_granularity in __blkdev_issue_discard()"). > Reported-by: Vicente Bergas <vicencb@gmail.com> Hi Coly, Adrian, would you like me to reply with a tested-by? before testing it: what are the chances of losing data? Regards, Vicente. > Signed-off-by: Coly Li <colyli@suse.de> > Acked-by: Adrian Hunter <adrian.hunter@intel.com> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > --- > Changelog, > v3, add Fixes tag for both commits. > v2, change commit id of the Fixes tag. > v1, initial version. > > drivers/mmc/core/queue.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c > index 6c022ef0f84d..350d0cc4ee62 100644 > --- a/drivers/mmc/core/queue.c > +++ b/drivers/mmc/core/queue.c > @@ -190,7 +190,7 @@ static void mmc_queue_setup_discard(struct request_queue *q, > q->limits.discard_granularity = card->pref_erase << 9; > /* granularity must not be greater than max. discard */ > if (card->pref_erase > max_discard) > - q->limits.discard_granularity = 0; > + q->limits.discard_granularity = SECTOR_SIZE; > if (mmc_can_secure_erase_trim(card)) > blk_queue_flag_set(QUEUE_FLAG_SECERASE, q); > } > -- > 2.26.2 >
On 1/10/20 11:38 am, Vicente Bergas wrote: > On Thu, Oct 1, 2020 at 9:18 AM Coly Li <colyli@suse.de> wrote: >> >> In mmc_queue_setup_discard() the mmc driver queue's discard_granularity >> might be set as 0 (when card->pref_erase > max_discard) while the mmc >> device still declares to support discard operation. This is buggy and >> triggered the following kernel warning message, >> >> WARNING: CPU: 0 PID: 135 at __blkdev_issue_discard+0x200/0x294 >> CPU: 0 PID: 135 Comm: f2fs_discard-17 Not tainted 5.9.0-rc6 #1 >> Hardware name: Google Kevin (DT) >> pstate: 00000005 (nzcv daif -PAN -UAO BTYPE=--) >> pc : __blkdev_issue_discard+0x200/0x294 >> lr : __blkdev_issue_discard+0x54/0x294 >> sp : ffff800011dd3b10 >> x29: ffff800011dd3b10 x28: 0000000000000000 x27: ffff800011dd3cc4 x26: ffff800011dd3e18 x25: 000000000004e69b x24: 0000000000000c40 x23: ffff0000f1deaaf0 x22: ffff0000f2849200 x21: 00000000002734d8 x20: 0000000000000008 x19: 0000000000000000 x18: 0000000000000000 x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 x14: 0000000000000394 x13: 0000000000000000 x12: 0000000000000000 x11: 0000000000000000 x10: 00000000000008b0 x9 : ffff800011dd3cb0 x8 : 000000000004e69b x7 : 0000000000000000 x6 : ffff0000f1926400 x5 : ffff0000f1940800 x4 : 0000000000000000 x3 : 0000000000000c40 x2 : 0000000000000008 x1 : 00000000002734d8 x0 : 0000000000000000 Call trace: >> __blkdev_issue_discard+0x200/0x294 >> __submit_discard_cmd+0x128/0x374 >> __issue_discard_cmd_orderly+0x188/0x244 >> __issue_discard_cmd+0x2e8/0x33c >> issue_discard_thread+0xe8/0x2f0 >> kthread+0x11c/0x120 >> ret_from_fork+0x10/0x1c >> ---[ end trace e4c8023d33dfe77a ]--- >> >> This patch fixes the issue by setting discard_granularity as SECTOR_SIZE >> instead of 0 when (card->pref_erase > max_discard) is true. Now no more >> complain from __blkdev_issue_discard() for the improper value of discard >> granularity. >> >> This issue is exposed after commit b35fd7422c2f ("block: check queue's >> limits.discard_granularity in __blkdev_issue_discard()"), a "Fixes:" tag >> is also added for the commit to make sure people won't miss this patch >> after applying the change of __blkdev_issue_discard(). >> >> Fixes: e056a1b5b67b ("mmc: queue: let host controllers specify maximum discard timeout") >> Fixes: b35fd7422c2f ("block: check queue's limits.discard_granularity in __blkdev_issue_discard()"). >> Reported-by: Vicente Bergas <vicencb@gmail.com> > > Hi Coly, Adrian, > would you like me to reply with a tested-by? > before testing it: what are the chances of losing data? Very low I would hope, but that is no reason not to have backups :-) > Regards, > Vicente. > >> Signed-off-by: Coly Li <colyli@suse.de> >> Acked-by: Adrian Hunter <adrian.hunter@intel.com> >> Cc: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> Changelog, >> v3, add Fixes tag for both commits. >> v2, change commit id of the Fixes tag. >> v1, initial version. >> >> drivers/mmc/core/queue.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c >> index 6c022ef0f84d..350d0cc4ee62 100644 >> --- a/drivers/mmc/core/queue.c >> +++ b/drivers/mmc/core/queue.c >> @@ -190,7 +190,7 @@ static void mmc_queue_setup_discard(struct request_queue *q, >> q->limits.discard_granularity = card->pref_erase << 9; >> /* granularity must not be greater than max. discard */ >> if (card->pref_erase > max_discard) >> - q->limits.discard_granularity = 0; >> + q->limits.discard_granularity = SECTOR_SIZE; >> if (mmc_can_secure_erase_trim(card)) >> blk_queue_flag_set(QUEUE_FLAG_SECERASE, q); >> } >> -- >> 2.26.2 >>
On Thu, Oct 1, 2020 at 11:07 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 1/10/20 11:38 am, Vicente Bergas wrote: > > On Thu, Oct 1, 2020 at 9:18 AM Coly Li <colyli@suse.de> wrote: > >> > >> In mmc_queue_setup_discard() the mmc driver queue's discard_granularity > >> might be set as 0 (when card->pref_erase > max_discard) while the mmc > >> device still declares to support discard operation. This is buggy and > >> triggered the following kernel warning message, > >> > >> WARNING: CPU: 0 PID: 135 at __blkdev_issue_discard+0x200/0x294 > >> CPU: 0 PID: 135 Comm: f2fs_discard-17 Not tainted 5.9.0-rc6 #1 > >> Hardware name: Google Kevin (DT) > >> pstate: 00000005 (nzcv daif -PAN -UAO BTYPE=--) > >> pc : __blkdev_issue_discard+0x200/0x294 > >> lr : __blkdev_issue_discard+0x54/0x294 > >> sp : ffff800011dd3b10 > >> x29: ffff800011dd3b10 x28: 0000000000000000 x27: ffff800011dd3cc4 x26: ffff800011dd3e18 x25: 000000000004e69b x24: 0000000000000c40 x23: ffff0000f1deaaf0 x22: ffff0000f2849200 x21: 00000000002734d8 x20: 0000000000000008 x19: 0000000000000000 x18: 0000000000000000 x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 x14: 0000000000000394 x13: 0000000000000000 x12: 0000000000000000 x11: 0000000000000000 x10: 00000000000008b0 x9 : ffff800011dd3cb0 x8 : 000000000004e69b x7 : 0000000000000000 x6 : ffff0000f1926400 x5 : ffff0000f1940800 x4 : 0000000000000000 x3 : 0000000000000c40 x2 : 0000000000000008 x1 : 00000000002734d8 x0 : 0000000000000000 Call trace: > >> __blkdev_issue_discard+0x200/0x294 > >> __submit_discard_cmd+0x128/0x374 > >> __issue_discard_cmd_orderly+0x188/0x244 > >> __issue_discard_cmd+0x2e8/0x33c > >> issue_discard_thread+0xe8/0x2f0 > >> kthread+0x11c/0x120 > >> ret_from_fork+0x10/0x1c > >> ---[ end trace e4c8023d33dfe77a ]--- > >> > >> This patch fixes the issue by setting discard_granularity as SECTOR_SIZE > >> instead of 0 when (card->pref_erase > max_discard) is true. Now no more > >> complain from __blkdev_issue_discard() for the improper value of discard > >> granularity. > >> > >> This issue is exposed after commit b35fd7422c2f ("block: check queue's > >> limits.discard_granularity in __blkdev_issue_discard()"), a "Fixes:" tag > >> is also added for the commit to make sure people won't miss this patch > >> after applying the change of __blkdev_issue_discard(). > >> > >> Fixes: e056a1b5b67b ("mmc: queue: let host controllers specify maximum discard timeout") > >> Fixes: b35fd7422c2f ("block: check queue's limits.discard_granularity in __blkdev_issue_discard()"). > >> Reported-by: Vicente Bergas <vicencb@gmail.com> > > > > Hi Coly, Adrian, > > would you like me to reply with a tested-by? > > before testing it: what are the chances of losing data? > > Very low I would hope, but that is no reason not to have backups :-) I do have backup, it is for the hassle of reinstalling the OS and restoring the backup. I'll test it later, maybe today. > > Regards, > > Vicente. > > > >> Signed-off-by: Coly Li <colyli@suse.de> > >> Acked-by: Adrian Hunter <adrian.hunter@intel.com> > >> Cc: Ulf Hansson <ulf.hansson@linaro.org> > >> --- > >> Changelog, > >> v3, add Fixes tag for both commits. > >> v2, change commit id of the Fixes tag. > >> v1, initial version. > >> > >> drivers/mmc/core/queue.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c > >> index 6c022ef0f84d..350d0cc4ee62 100644 > >> --- a/drivers/mmc/core/queue.c > >> +++ b/drivers/mmc/core/queue.c > >> @@ -190,7 +190,7 @@ static void mmc_queue_setup_discard(struct request_queue *q, > >> q->limits.discard_granularity = card->pref_erase << 9; > >> /* granularity must not be greater than max. discard */ Just out of curiosity, wouldn't it be more performant something like: q->limits.discard_granularity = minimum(card->pref_erase, max_discard) << 9; or is just that SECTOR_SIZE is a number guaranteed to work? > >> if (card->pref_erase > max_discard) > >> - q->limits.discard_granularity = 0; > >> + q->limits.discard_granularity = SECTOR_SIZE; > >> if (mmc_can_secure_erase_trim(card)) > >> blk_queue_flag_set(QUEUE_FLAG_SECERASE, q); > >> } > >> -- > >> 2.26.2 > >> >
On 2020/10/1 16:38, Vicente Bergas wrote: > On Thu, Oct 1, 2020 at 9:18 AM Coly Li <colyli@suse.de> wrote: >> >> In mmc_queue_setup_discard() the mmc driver queue's discard_granularity >> might be set as 0 (when card->pref_erase > max_discard) while the mmc >> device still declares to support discard operation. This is buggy and >> triggered the following kernel warning message, >> >> WARNING: CPU: 0 PID: 135 at __blkdev_issue_discard+0x200/0x294 >> CPU: 0 PID: 135 Comm: f2fs_discard-17 Not tainted 5.9.0-rc6 #1 >> Hardware name: Google Kevin (DT) >> pstate: 00000005 (nzcv daif -PAN -UAO BTYPE=--) >> pc : __blkdev_issue_discard+0x200/0x294 >> lr : __blkdev_issue_discard+0x54/0x294 >> sp : ffff800011dd3b10 >> x29: ffff800011dd3b10 x28: 0000000000000000 x27: ffff800011dd3cc4 x26: ffff800011dd3e18 x25: 000000000004e69b x24: 0000000000000c40 x23: ffff0000f1deaaf0 x22: ffff0000f2849200 x21: 00000000002734d8 x20: 0000000000000008 x19: 0000000000000000 x18: 0000000000000000 x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 x14: 0000000000000394 x13: 0000000000000000 x12: 0000000000000000 x11: 0000000000000000 x10: 00000000000008b0 x9 : ffff800011dd3cb0 x8 : 000000000004e69b x7 : 0000000000000000 x6 : ffff0000f1926400 x5 : ffff0000f1940800 x4 : 0000000000000000 x3 : 0000000000000c40 x2 : 0000000000000008 x1 : 00000000002734d8 x0 : 0000000000000000 Call trace: >> __blkdev_issue_discard+0x200/0x294 >> __submit_discard_cmd+0x128/0x374 >> __issue_discard_cmd_orderly+0x188/0x244 >> __issue_discard_cmd+0x2e8/0x33c >> issue_discard_thread+0xe8/0x2f0 >> kthread+0x11c/0x120 >> ret_from_fork+0x10/0x1c >> ---[ end trace e4c8023d33dfe77a ]--- >> >> This patch fixes the issue by setting discard_granularity as SECTOR_SIZE >> instead of 0 when (card->pref_erase > max_discard) is true. Now no more >> complain from __blkdev_issue_discard() for the improper value of discard >> granularity. >> >> This issue is exposed after commit b35fd7422c2f ("block: check queue's >> limits.discard_granularity in __blkdev_issue_discard()"), a "Fixes:" tag >> is also added for the commit to make sure people won't miss this patch >> after applying the change of __blkdev_issue_discard(). >> >> Fixes: e056a1b5b67b ("mmc: queue: let host controllers specify maximum discard timeout") >> Fixes: b35fd7422c2f ("block: check queue's limits.discard_granularity in __blkdev_issue_discard()"). >> Reported-by: Vicente Bergas <vicencb@gmail.com> > > Hi Coly, Adrian, > would you like me to reply with a tested-by? > before testing it: what are the chances of losing data? Hi Vicente, Please do :-) I will wait for your reply and post a v4 version. Thanks. Coly Li >> Signed-off-by: Coly Li <colyli@suse.de> >> Acked-by: Adrian Hunter <adrian.hunter@intel.com> >> Cc: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> Changelog, >> v3, add Fixes tag for both commits. >> v2, change commit id of the Fixes tag. >> v1, initial version. >> >> drivers/mmc/core/queue.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c >> index 6c022ef0f84d..350d0cc4ee62 100644 >> --- a/drivers/mmc/core/queue.c >> +++ b/drivers/mmc/core/queue.c >> @@ -190,7 +190,7 @@ static void mmc_queue_setup_discard(struct request_queue *q, >> q->limits.discard_granularity = card->pref_erase << 9; >> /* granularity must not be greater than max. discard */ >> if (card->pref_erase > max_discard) >> - q->limits.discard_granularity = 0; >> + q->limits.discard_granularity = SECTOR_SIZE; >> if (mmc_can_secure_erase_trim(card)) >> blk_queue_flag_set(QUEUE_FLAG_SECERASE, q); >> } >> -- >> 2.26.2 >>
On 2020/10/1 17:27, Vicente Bergas wrote: > On Thu, Oct 1, 2020 at 11:07 AM Adrian Hunter <adrian.hunter@intel.com> wrote: >> >> On 1/10/20 11:38 am, Vicente Bergas wrote: >>> On Thu, Oct 1, 2020 at 9:18 AM Coly Li <colyli@suse.de> wrote: >>>> >>>> In mmc_queue_setup_discard() the mmc driver queue's discard_granularity >>>> might be set as 0 (when card->pref_erase > max_discard) while the mmc >>>> device still declares to support discard operation. This is buggy and >>>> triggered the following kernel warning message, >>>> >>>> WARNING: CPU: 0 PID: 135 at __blkdev_issue_discard+0x200/0x294 >>>> CPU: 0 PID: 135 Comm: f2fs_discard-17 Not tainted 5.9.0-rc6 #1 >>>> Hardware name: Google Kevin (DT) >>>> pstate: 00000005 (nzcv daif -PAN -UAO BTYPE=--) >>>> pc : __blkdev_issue_discard+0x200/0x294 >>>> lr : __blkdev_issue_discard+0x54/0x294 >>>> sp : ffff800011dd3b10 >>>> x29: ffff800011dd3b10 x28: 0000000000000000 x27: ffff800011dd3cc4 x26: ffff800011dd3e18 x25: 000000000004e69b x24: 0000000000000c40 x23: ffff0000f1deaaf0 x22: ffff0000f2849200 x21: 00000000002734d8 x20: 0000000000000008 x19: 0000000000000000 x18: 0000000000000000 x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 x14: 0000000000000394 x13: 0000000000000000 x12: 0000000000000000 x11: 0000000000000000 x10: 00000000000008b0 x9 : ffff800011dd3cb0 x8 : 000000000004e69b x7 : 0000000000000000 x6 : ffff0000f1926400 x5 : ffff0000f1940800 x4 : 0000000000000000 x3 : 0000000000000c40 x2 : 0000000000000008 x1 : 00000000002734d8 x0 : 0000000000000000 Call trace: >>>> __blkdev_issue_discard+0x200/0x294 >>>> __submit_discard_cmd+0x128/0x374 >>>> __issue_discard_cmd_orderly+0x188/0x244 >>>> __issue_discard_cmd+0x2e8/0x33c >>>> issue_discard_thread+0xe8/0x2f0 >>>> kthread+0x11c/0x120 >>>> ret_from_fork+0x10/0x1c >>>> ---[ end trace e4c8023d33dfe77a ]--- >>>> >>>> This patch fixes the issue by setting discard_granularity as SECTOR_SIZE >>>> instead of 0 when (card->pref_erase > max_discard) is true. Now no more >>>> complain from __blkdev_issue_discard() for the improper value of discard >>>> granularity. >>>> >>>> This issue is exposed after commit b35fd7422c2f ("block: check queue's >>>> limits.discard_granularity in __blkdev_issue_discard()"), a "Fixes:" tag >>>> is also added for the commit to make sure people won't miss this patch >>>> after applying the change of __blkdev_issue_discard(). >>>> >>>> Fixes: e056a1b5b67b ("mmc: queue: let host controllers specify maximum discard timeout") >>>> Fixes: b35fd7422c2f ("block: check queue's limits.discard_granularity in __blkdev_issue_discard()"). >>>> Reported-by: Vicente Bergas <vicencb@gmail.com> >>> >>> Hi Coly, Adrian, >>> would you like me to reply with a tested-by? >>> before testing it: what are the chances of losing data? >> >> Very low I would hope, but that is no reason not to have backups :-) > > I do have backup, it is for the hassle of reinstalling the OS and > restoring the backup. > I'll test it later, maybe today. > >>> Regards, >>> Vicente. >>> >>>> Signed-off-by: Coly Li <colyli@suse.de> >>>> Acked-by: Adrian Hunter <adrian.hunter@intel.com> >>>> Cc: Ulf Hansson <ulf.hansson@linaro.org> >>>> --- >>>> Changelog, >>>> v3, add Fixes tag for both commits. >>>> v2, change commit id of the Fixes tag. >>>> v1, initial version. >>>> >>>> drivers/mmc/core/queue.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c >>>> index 6c022ef0f84d..350d0cc4ee62 100644 >>>> --- a/drivers/mmc/core/queue.c >>>> +++ b/drivers/mmc/core/queue.c >>>> @@ -190,7 +190,7 @@ static void mmc_queue_setup_discard(struct request_queue *q, >>>> q->limits.discard_granularity = card->pref_erase << 9; >>>> /* granularity must not be greater than max. discard */ > > Just out of curiosity, wouldn't it be more performant something like: > q->limits.discard_granularity = minimum(card->pref_erase, max_discard) << 9; > or is just that SECTOR_SIZE is a number guaranteed to work? > The discard_granularity is a hint for the minimal acceptable length of the discard request. The discard request might be accepted by device when the discard request covers a discard_granularity range. The discard_granularity rage is the minimum available discard request length, it is a LBA range which starts on discard_granularity aligned LBA, and length is discard_granularity. Therefore setting discard_granularity to SECTOR_SIZE won't hurt discard performance, the discard bio split does not depend on it. It is a hint for a minimal acceptable length of discard request. Coly Li
On 1/10/20 12:27 pm, Vicente Bergas wrote: > On Thu, Oct 1, 2020 at 11:07 AM Adrian Hunter <adrian.hunter@intel.com> wrote: >> >> On 1/10/20 11:38 am, Vicente Bergas wrote: >>> On Thu, Oct 1, 2020 at 9:18 AM Coly Li <colyli@suse.de> wrote: >>>> >>>> In mmc_queue_setup_discard() the mmc driver queue's discard_granularity >>>> might be set as 0 (when card->pref_erase > max_discard) while the mmc >>>> device still declares to support discard operation. This is buggy and >>>> triggered the following kernel warning message, >>>> >>>> WARNING: CPU: 0 PID: 135 at __blkdev_issue_discard+0x200/0x294 >>>> CPU: 0 PID: 135 Comm: f2fs_discard-17 Not tainted 5.9.0-rc6 #1 >>>> Hardware name: Google Kevin (DT) >>>> pstate: 00000005 (nzcv daif -PAN -UAO BTYPE=--) >>>> pc : __blkdev_issue_discard+0x200/0x294 >>>> lr : __blkdev_issue_discard+0x54/0x294 >>>> sp : ffff800011dd3b10 >>>> x29: ffff800011dd3b10 x28: 0000000000000000 x27: ffff800011dd3cc4 x26: ffff800011dd3e18 x25: 000000000004e69b x24: 0000000000000c40 x23: ffff0000f1deaaf0 x22: ffff0000f2849200 x21: 00000000002734d8 x20: 0000000000000008 x19: 0000000000000000 x18: 0000000000000000 x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 x14: 0000000000000394 x13: 0000000000000000 x12: 0000000000000000 x11: 0000000000000000 x10: 00000000000008b0 x9 : ffff800011dd3cb0 x8 : 000000000004e69b x7 : 0000000000000000 x6 : ffff0000f1926400 x5 : ffff0000f1940800 x4 : 0000000000000000 x3 : 0000000000000c40 x2 : 0000000000000008 x1 : 00000000002734d8 x0 : 0000000000000000 Call trace: >>>> __blkdev_issue_discard+0x200/0x294 >>>> __submit_discard_cmd+0x128/0x374 >>>> __issue_discard_cmd_orderly+0x188/0x244 >>>> __issue_discard_cmd+0x2e8/0x33c >>>> issue_discard_thread+0xe8/0x2f0 >>>> kthread+0x11c/0x120 >>>> ret_from_fork+0x10/0x1c >>>> ---[ end trace e4c8023d33dfe77a ]--- >>>> >>>> This patch fixes the issue by setting discard_granularity as SECTOR_SIZE >>>> instead of 0 when (card->pref_erase > max_discard) is true. Now no more >>>> complain from __blkdev_issue_discard() for the improper value of discard >>>> granularity. >>>> >>>> This issue is exposed after commit b35fd7422c2f ("block: check queue's >>>> limits.discard_granularity in __blkdev_issue_discard()"), a "Fixes:" tag >>>> is also added for the commit to make sure people won't miss this patch >>>> after applying the change of __blkdev_issue_discard(). >>>> >>>> Fixes: e056a1b5b67b ("mmc: queue: let host controllers specify maximum discard timeout") >>>> Fixes: b35fd7422c2f ("block: check queue's limits.discard_granularity in __blkdev_issue_discard()"). >>>> Reported-by: Vicente Bergas <vicencb@gmail.com> >>> >>> Hi Coly, Adrian, >>> would you like me to reply with a tested-by? >>> before testing it: what are the chances of losing data? >> >> Very low I would hope, but that is no reason not to have backups :-) > > I do have backup, it is for the hassle of reinstalling the OS and > restoring the backup. > I'll test it later, maybe today. > >>> Regards, >>> Vicente. >>> >>>> Signed-off-by: Coly Li <colyli@suse.de> >>>> Acked-by: Adrian Hunter <adrian.hunter@intel.com> >>>> Cc: Ulf Hansson <ulf.hansson@linaro.org> >>>> --- >>>> Changelog, >>>> v3, add Fixes tag for both commits. >>>> v2, change commit id of the Fixes tag. >>>> v1, initial version. >>>> >>>> drivers/mmc/core/queue.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c >>>> index 6c022ef0f84d..350d0cc4ee62 100644 >>>> --- a/drivers/mmc/core/queue.c >>>> +++ b/drivers/mmc/core/queue.c >>>> @@ -190,7 +190,7 @@ static void mmc_queue_setup_discard(struct request_queue *q, >>>> q->limits.discard_granularity = card->pref_erase << 9; >>>> /* granularity must not be greater than max. discard */ > > Just out of curiosity, wouldn't it be more performant something like: > q->limits.discard_granularity = minimum(card->pref_erase, max_discard) << 9; > or is just that SECTOR_SIZE is a number guaranteed to work? SECTOR_SIZE is guaranteed to work. It is preferable, but not necessary, to align to the devices internal erase block size. > >>>> if (card->pref_erase > max_discard) >>>> - q->limits.discard_granularity = 0; >>>> + q->limits.discard_granularity = SECTOR_SIZE; >>>> if (mmc_can_secure_erase_trim(card)) >>>> blk_queue_flag_set(QUEUE_FLAG_SECERASE, q); >>>> } >>>> -- >>>> 2.26.2 >>>> >>
On Thursday, October 1, 2020 9:18:24 AM CEST, Coly Li wrote: > In mmc_queue_setup_discard() the mmc driver queue's discard_granularity > might be set as 0 (when card->pref_erase > max_discard) while the mmc > device still declares to support discard operation. This is buggy and > triggered the following kernel warning message, > > WARNING: CPU: 0 PID: 135 at __blkdev_issue_discard+0x200/0x294 > CPU: 0 PID: 135 Comm: f2fs_discard-17 Not tainted 5.9.0-rc6 #1 > Hardware name: Google Kevin (DT) > pstate: 00000005 (nzcv daif -PAN -UAO BTYPE=--) > pc : __blkdev_issue_discard+0x200/0x294 > lr : __blkdev_issue_discard+0x54/0x294 > sp : ffff800011dd3b10 > x29: ffff800011dd3b10 x28: 0000000000000000 x27: > ffff800011dd3cc4 x26: ffff800011dd3e18 x25: 000000000004e69b > x24: 0000000000000c40 x23: ffff0000f1deaaf0 x22: > ffff0000f2849200 x21: 00000000002734d8 x20: 0000000000000008 > x19: 0000000000000000 x18: 0000000000000000 x17: > 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 > x14: 0000000000000394 x13: 0000000000000000 x12: > 0000000000000000 x11: 0000000000000000 x10: 00000000000008b0 x9 > : ffff800011dd3cb0 x8 : 000000000004e69b x7 : 0000000000000000 > x6 : ffff0000f1926400 x5 : ffff0000f1940800 x4 : > 0000000000000000 x3 : 0000000000000c40 x2 : 0000000000000008 x1 > : 00000000002734d8 x0 : 0000000000000000 Call trace: > __blkdev_issue_discard+0x200/0x294 > __submit_discard_cmd+0x128/0x374 > __issue_discard_cmd_orderly+0x188/0x244 > __issue_discard_cmd+0x2e8/0x33c > issue_discard_thread+0xe8/0x2f0 > kthread+0x11c/0x120 > ret_from_fork+0x10/0x1c > ---[ end trace e4c8023d33dfe77a ]--- > > This patch fixes the issue by setting discard_granularity as SECTOR_SIZE > instead of 0 when (card->pref_erase > max_discard) is true. Now no more > complain from __blkdev_issue_discard() for the improper value of discard > granularity. > > This issue is exposed after commit b35fd7422c2f ("block: check queue's > limits.discard_granularity in __blkdev_issue_discard()"), a "Fixes:" tag > is also added for the commit to make sure people won't miss this patch > after applying the change of __blkdev_issue_discard(). > > Fixes: e056a1b5b67b ("mmc: queue: let host controllers specify > maximum discard timeout") > Fixes: b35fd7422c2f ("block: check queue's > limits.discard_granularity in __blkdev_issue_discard()"). > Reported-by: Vicente Bergas <vicencb@gmail.com> > Signed-off-by: Coly Li <colyli@suse.de> > Acked-by: Adrian Hunter <adrian.hunter@intel.com> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > --- > Changelog, > v3, add Fixes tag for both commits. > v2, change commit id of the Fixes tag. > v1, initial version. > > drivers/mmc/core/queue.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c > index 6c022ef0f84d..350d0cc4ee62 100644 > --- a/drivers/mmc/core/queue.c > +++ b/drivers/mmc/core/queue.c > @@ -190,7 +190,7 @@ static void mmc_queue_setup_discard(struct > request_queue *q, > q->limits.discard_granularity = card->pref_erase << 9; > /* granularity must not be greater than max. discard */ > if (card->pref_erase > max_discard) > - q->limits.discard_granularity = 0; > + q->limits.discard_granularity = SECTOR_SIZE; > if (mmc_can_secure_erase_trim(card)) > blk_queue_flag_set(QUEUE_FLAG_SECERASE, q); > } Tested on rk3399-gru-kevin with f2fs filesystem over the mmc driver, that is, the same setup that reproduced the issue. The kernel warning message is no longer reported. So, Tested-by: Vicente Bergas <vicencb@gmail.com>
On 2020/10/2 02:47, Vicente Bergas wrote: > On Thursday, October 1, 2020 9:18:24 AM CEST, Coly Li wrote: >> In mmc_queue_setup_discard() the mmc driver queue's discard_granularity >> might be set as 0 (when card->pref_erase > max_discard) while the mmc >> device still declares to support discard operation. This is buggy and >> triggered the following kernel warning message, >> >> WARNING: CPU: 0 PID: 135 at __blkdev_issue_discard+0x200/0x294 >> CPU: 0 PID: 135 Comm: f2fs_discard-17 Not tainted 5.9.0-rc6 #1 >> Hardware name: Google Kevin (DT) >> pstate: 00000005 (nzcv daif -PAN -UAO BTYPE=--) >> pc : __blkdev_issue_discard+0x200/0x294 >> lr : __blkdev_issue_discard+0x54/0x294 >> sp : ffff800011dd3b10 >> x29: ffff800011dd3b10 x28: 0000000000000000 x27: ffff800011dd3cc4 x26: >> ffff800011dd3e18 x25: 000000000004e69b x24: 0000000000000c40 x23: >> ffff0000f1deaaf0 x22: ffff0000f2849200 x21: 00000000002734d8 x20: >> 0000000000000008 x19: 0000000000000000 x18: 0000000000000000 x17: >> 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 x14: >> 0000000000000394 x13: 0000000000000000 x12: 0000000000000000 x11: >> 0000000000000000 x10: 00000000000008b0 x9 : ffff800011dd3cb0 x8 : >> 000000000004e69b x7 : 0000000000000000 x6 : ffff0000f1926400 x5 : >> ffff0000f1940800 x4 : 0000000000000000 x3 : 0000000000000c40 x2 : >> 0000000000000008 x1 : 00000000002734d8 x0 : 0000000000000000 Call trace: >> __blkdev_issue_discard+0x200/0x294 >> __submit_discard_cmd+0x128/0x374 >> __issue_discard_cmd_orderly+0x188/0x244 >> __issue_discard_cmd+0x2e8/0x33c >> issue_discard_thread+0xe8/0x2f0 >> kthread+0x11c/0x120 >> ret_from_fork+0x10/0x1c >> ---[ end trace e4c8023d33dfe77a ]--- >> >> This patch fixes the issue by setting discard_granularity as SECTOR_SIZE >> instead of 0 when (card->pref_erase > max_discard) is true. Now no more >> complain from __blkdev_issue_discard() for the improper value of discard >> granularity. >> >> This issue is exposed after commit b35fd7422c2f ("block: check queue's >> limits.discard_granularity in __blkdev_issue_discard()"), a "Fixes:" tag >> is also added for the commit to make sure people won't miss this patch >> after applying the change of __blkdev_issue_discard(). >> >> Fixes: e056a1b5b67b ("mmc: queue: let host controllers specify maximum >> discard timeout") >> Fixes: b35fd7422c2f ("block: check queue's limits.discard_granularity >> in __blkdev_issue_discard()"). >> Reported-by: Vicente Bergas <vicencb@gmail.com> >> Signed-off-by: Coly Li <colyli@suse.de> >> Acked-by: Adrian Hunter <adrian.hunter@intel.com> >> Cc: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> Changelog, >> v3, add Fixes tag for both commits. >> v2, change commit id of the Fixes tag. >> v1, initial version. >> >> drivers/mmc/core/queue.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c >> index 6c022ef0f84d..350d0cc4ee62 100644 >> --- a/drivers/mmc/core/queue.c >> +++ b/drivers/mmc/core/queue.c >> @@ -190,7 +190,7 @@ static void mmc_queue_setup_discard(struct >> request_queue *q, >> q->limits.discard_granularity = card->pref_erase << 9; >> /* granularity must not be greater than max. discard */ >> if (card->pref_erase > max_discard) >> - q->limits.discard_granularity = 0; >> + q->limits.discard_granularity = SECTOR_SIZE; >> if (mmc_can_secure_erase_trim(card)) >> blk_queue_flag_set(QUEUE_FLAG_SECERASE, q); >> } > > Tested on rk3399-gru-kevin with f2fs filesystem over the mmc driver, that > is, the same setup that reproduced the issue. > The kernel warning message is no longer reported. > So, > Tested-by: Vicente Bergas <vicencb@gmail.com> > Hi Vicente, Thank you very much! Coly Li
diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c index 6c022ef0f84d..350d0cc4ee62 100644 --- a/drivers/mmc/core/queue.c +++ b/drivers/mmc/core/queue.c @@ -190,7 +190,7 @@ static void mmc_queue_setup_discard(struct request_queue *q, q->limits.discard_granularity = card->pref_erase << 9; /* granularity must not be greater than max. discard */ if (card->pref_erase > max_discard) - q->limits.discard_granularity = 0; + q->limits.discard_granularity = SECTOR_SIZE; if (mmc_can_secure_erase_trim(card)) blk_queue_flag_set(QUEUE_FLAG_SECERASE, q); }
In mmc_queue_setup_discard() the mmc driver queue's discard_granularity might be set as 0 (when card->pref_erase > max_discard) while the mmc device still declares to support discard operation. This is buggy and triggered the following kernel warning message, WARNING: CPU: 0 PID: 135 at __blkdev_issue_discard+0x200/0x294 CPU: 0 PID: 135 Comm: f2fs_discard-17 Not tainted 5.9.0-rc6 #1 Hardware name: Google Kevin (DT) pstate: 00000005 (nzcv daif -PAN -UAO BTYPE=--) pc : __blkdev_issue_discard+0x200/0x294 lr : __blkdev_issue_discard+0x54/0x294 sp : ffff800011dd3b10 x29: ffff800011dd3b10 x28: 0000000000000000 x27: ffff800011dd3cc4 x26: ffff800011dd3e18 x25: 000000000004e69b x24: 0000000000000c40 x23: ffff0000f1deaaf0 x22: ffff0000f2849200 x21: 00000000002734d8 x20: 0000000000000008 x19: 0000000000000000 x18: 0000000000000000 x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 x14: 0000000000000394 x13: 0000000000000000 x12: 0000000000000000 x11: 0000000000000000 x10: 00000000000008b0 x9 : ffff800011dd3cb0 x8 : 000000000004e69b x7 : 0000000000000000 x6 : ffff0000f1926400 x5 : ffff0000f1940800 x4 : 0000000000000000 x3 : 0000000000000c40 x2 : 0000000000000008 x1 : 00000000002734d8 x0 : 0000000000000000 Call trace: __blkdev_issue_discard+0x200/0x294 __submit_discard_cmd+0x128/0x374 __issue_discard_cmd_orderly+0x188/0x244 __issue_discard_cmd+0x2e8/0x33c issue_discard_thread+0xe8/0x2f0 kthread+0x11c/0x120 ret_from_fork+0x10/0x1c ---[ end trace e4c8023d33dfe77a ]--- This patch fixes the issue by setting discard_granularity as SECTOR_SIZE instead of 0 when (card->pref_erase > max_discard) is true. Now no more complain from __blkdev_issue_discard() for the improper value of discard granularity. This issue is exposed after commit b35fd7422c2f ("block: check queue's limits.discard_granularity in __blkdev_issue_discard()"), a "Fixes:" tag is also added for the commit to make sure people won't miss this patch after applying the change of __blkdev_issue_discard(). Fixes: e056a1b5b67b ("mmc: queue: let host controllers specify maximum discard timeout") Fixes: b35fd7422c2f ("block: check queue's limits.discard_granularity in __blkdev_issue_discard()"). Reported-by: Vicente Bergas <vicencb@gmail.com> Signed-off-by: Coly Li <colyli@suse.de> Acked-by: Adrian Hunter <adrian.hunter@intel.com> Cc: Ulf Hansson <ulf.hansson@linaro.org> --- Changelog, v3, add Fixes tag for both commits. v2, change commit id of the Fixes tag. v1, initial version. drivers/mmc/core/queue.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)