| Message ID | 1481656818-21141-1-git-send-email-gwendal@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 13/12/16 21:20, Gwendal Grignou wrote: > Working with a SDHCI controller and a Toshiba 16GB THGBMFG7C2LBAIL. > The Toshiba part has a large erase group size and a long erase timeout. > It barely fits within the controller timeout, but mmc reports a > 512B discard size to the block layer instead of the expected 4MB: > /sys/block/mmcblk0/queue/discard_max_bytes = 512 > > The host caps[0] is 0x...b2, so host->max_busy_timeout is set to > 2684.35ms: (1 << 27 / ((0xb2 & 0x3f) * 1000). > > In Toshiba EXT_CSD, HC_ERASE_GRP_SIZE is set to 7 (2100ms erase timeout) > HC_ERASE_GRP_SIZE is set to 8, (8192 512B sectors erase size). > > In mmc_do_calc_max_discard(), as a safety, for eMMC device, number of > erase groups that could safely be erased with the host timeout (qty) was > reduced by 1. It was not for safety - it was to allow for crossing an erase group boundary. But the current code is quite different now, so please have a look at that because this code does not apply anymore. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 13, 2016 at 11:19 PM, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 13/12/16 21:20, Gwendal Grignou wrote: >> Working with a SDHCI controller and a Toshiba 16GB THGBMFG7C2LBAIL. >> The Toshiba part has a large erase group size and a long erase timeout. >> It barely fits within the controller timeout, but mmc reports a >> 512B discard size to the block layer instead of the expected 4MB: >> /sys/block/mmcblk0/queue/discard_max_bytes = 512 >> >> The host caps[0] is 0x...b2, so host->max_busy_timeout is set to >> 2684.35ms: (1 << 27 / ((0xb2 & 0x3f) * 1000). >> >> In Toshiba EXT_CSD, HC_ERASE_GRP_SIZE is set to 7 (2100ms erase timeout) >> HC_ERASE_GRP_SIZE is set to 8, (8192 512B sectors erase size). >> >> In mmc_do_calc_max_discard(), as a safety, for eMMC device, number of >> erase groups that could safely be erased with the host timeout (qty) was >> reduced by 1. > > It was not for safety - it was to allow for crossing an erase group boundary. Got it. > > But the current code is quite different now, so please have a look at that > because this code does not apply anymore. Indeed, looking at the block layer, it is paying attention not to cross boundary, issuing an extra erase command when necessary. in blkdev_issue_discard(), we use bdev_discard_alignment to be sure erasing within a partition still start on a erase group boundary. It has been addressed with c6e66634 (block: split discard into aligned requests) and following changes. > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14/12/16 20:13, Gwendal Grignou wrote: > On Tue, Dec 13, 2016 at 11:19 PM, Adrian Hunter <adrian.hunter@intel.com> wrote: >> On 13/12/16 21:20, Gwendal Grignou wrote: >>> Working with a SDHCI controller and a Toshiba 16GB THGBMFG7C2LBAIL. >>> The Toshiba part has a large erase group size and a long erase timeout. >>> It barely fits within the controller timeout, but mmc reports a >>> 512B discard size to the block layer instead of the expected 4MB: >>> /sys/block/mmcblk0/queue/discard_max_bytes = 512 >>> >>> The host caps[0] is 0x...b2, so host->max_busy_timeout is set to >>> 2684.35ms: (1 << 27 / ((0xb2 & 0x3f) * 1000). >>> >>> In Toshiba EXT_CSD, HC_ERASE_GRP_SIZE is set to 7 (2100ms erase timeout) >>> HC_ERASE_GRP_SIZE is set to 8, (8192 512B sectors erase size). >>> >>> In mmc_do_calc_max_discard(), as a safety, for eMMC device, number of >>> erase groups that could safely be erased with the host timeout (qty) was >>> reduced by 1. >> >> It was not for safety - it was to allow for crossing an erase group boundary. > Got it. >> >> But the current code is quite different now, so please have a look at that >> because this code does not apply anymore. > Indeed, looking at the block layer, it is paying attention not to > cross boundary, issuing an extra erase command when necessary. > in blkdev_issue_discard(), we use bdev_discard_alignment to be sure > erasing within a partition still start on a erase group boundary. > It has been addressed with c6e66634 (block: split discard into aligned > requests) and following changes. I think you are misreading that patch. It won't split a discard unless it exceeds the maximum discard size. So a discard of 2 sectors crossing an erase group boundary is not split unless the max discard is 1. The mmc core now does a split since commit 642c28ab86f7 ("mmc: core: Optimize case for exactly one erase-group budget") -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 15, 2016 at 1:46 AM, <hisanao.kinkawa@toshiba.co.jp> wrote: > On 2016/12/15 17:14, Adrian Hunter wrote: > But the current code is quite different now, so please have a look at that > because this code does not apply anymore. > ... > The mmc core > now does a split since commit 642c28ab86f7 ("mmc: core: Optimize case for > exactly one erase-group budget") You are correct. I should have looked at upstream kernel. My patch is incomplete and the issue has already been addressed with 642c28ab86f7. Gwendal. > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 64f603e..b5a1194 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -2397,16 +2397,13 @@ static unsigned int mmc_do_calc_max_discard(struct mmc_card *card, if (!qty) return 0; - if (qty == 1) - return 1; - /* Convert qty to sectors */ if (card->erase_shift) - max_discard = --qty << card->erase_shift; + max_discard = qty << card->erase_shift; else if (mmc_card_sd(card)) max_discard = qty; else - max_discard = --qty * card->erase_size; + max_discard = qty * card->erase_size; return max_discard; }
Working with a SDHCI controller and a Toshiba 16GB THGBMFG7C2LBAIL. The Toshiba part has a large erase group size and a long erase timeout. It barely fits within the controller timeout, but mmc reports a 512B discard size to the block layer instead of the expected 4MB: /sys/block/mmcblk0/queue/discard_max_bytes = 512 The host caps[0] is 0x...b2, so host->max_busy_timeout is set to 2684.35ms: (1 << 27 / ((0xb2 & 0x3f) * 1000). In Toshiba EXT_CSD, HC_ERASE_GRP_SIZE is set to 7 (2100ms erase timeout) HC_ERASE_GRP_SIZE is set to 8, (8192 512B sectors erase size). In mmc_do_calc_max_discard(), as a safety, for eMMC device, number of erase groups that could safely be erased with the host timeout (qty) was reduced by 1. This extra safety is not necessary, given these numbers are already the worst case scenario. Moreover, in the case of this Toshiba part, given 2100 is just below 2684, qty was set to 1, we would only report one sector (512b) to the block layer, making erasing/trimming painfully slow. With this fix, using Hynix HBG4e, check that discard_max_bytes increases from 1536kB to 2048kB (qty was set to 4: each erase group size of 1024 sectors have a timeout of 600ms). Signed-off-by: Gwendal Grignou <gwendal@chromium.org> --- drivers/mmc/core/core.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)