[PATCH v4 1/3]mmc: set max_discard_sectors value for mmc queue

[PATCH v4 1/3]mmc: set max_discard_sectors value for mmc queue

[Chuanxiao Dong]

max_discard_sectors value is UINT_MAX which means kernel block layer can pass
down unlimited sectors to MMC driver to erase. But erasing so many sectors may
delay some other important I/O requests. This is not preferred.

So use 'pref_erase' to set a suitable max_discard_sectors value for mmc queue to
avoid erasing too many sectors at one time.

Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com>
---
 drivers/mmc/card/queue.c |    3 ++-
 drivers/mmc/core/core.c  |   20 ++++++++++++++++++++
 include/linux/mmc/core.h |    1 +
 3 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index 4e42d03..6c13859 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -131,7 +131,8 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card, spinlock_t *lock
 	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, mq->queue);
 	if (mmc_can_erase(card)) {
 		queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, mq->queue);
-		mq->queue->limits.max_discard_sectors = UINT_MAX;
+		mq->queue->limits.max_discard_sectors =
+			mmc_set_max_discard_sectors(card);
 		if (card->erased_byte == 0)
 			mq->queue->limits.discard_zeroes_data = 1;
 		if (!mmc_can_trim(card) && is_power_of_2(card->erase_size)) {
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 34a7e8c..0eb27aa 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1470,6 +1470,26 @@ int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
 }
 EXPORT_SYMBOL(mmc_erase_group_aligned);
 
+/*
+ * Set max_discard_sectors for mmc queue.
+ * max_discard_sectors will determine how many sectors can be erased at one
+ * time. Sometimes user may want to erase a large area of SD or MMC card, it may
+ * take long time which delay some other important I/O requests. So we would
+ * better set some values for max_discard_sectors which can forbid block layer
+ * pass too many sectors at one time.
+ *
+ * 'pref_erase' is initialized for this purpose. We can use it to set a preferred
+ * value for it.
+ */
+int mmc_set_max_discard_sectors(struct mmc_card *card)
+{
+	if (card->pref_erase)
+		return card->pref_erase;
+	else
+		return UINT_MAX;
+}
+EXPORT_SYMBOL(mmc_set_max_discard_sectors);
+
 int mmc_set_blocklen(struct mmc_card *card, unsigned int blocklen)
 {
 	struct mmc_command cmd;
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 64e013f..a218d57 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -152,6 +152,7 @@ extern int mmc_can_trim(struct mmc_card *card);
 extern int mmc_can_secure_erase_trim(struct mmc_card *card);
 extern int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
 				   unsigned int nr);
+extern int mmc_set_max_discard_sectors(struct mmc_card *card);
 
 extern int mmc_set_blocklen(struct mmc_card *card, unsigned int blocklen);
 
-- 
1.6.6.1

Re: [PATCH v4 1/3]mmc: set max_discard_sectors value for mmc queue

[Arnd Bergmann]

On Saturday 12 February 2011 07:22:14 Chuanxiao Dong wrote:
> max_discard_sectors value is UINT_MAX which means kernel block layer can pass
> down unlimited sectors to MMC driver to erase. But erasing so many sectors may
> delay some other important I/O requests. This is not preferred.
> 
> So use 'pref_erase' to set a suitable max_discard_sectors value for mmc queue to
> avoid erasing too many sectors at one time.
> 
> Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com>

I'm not sure about this one. pref_erase on SDHC cards should be the *minimum*
unit you can erase in one request, not the maximum. Erasing an arbitrary
number of allocation units on an SDHC card should complete almost instantly,
because it only needs to update a single table with the allocation units.

Discarding partial allocation units will take a lot longer, because the
card then has to copy over the remaining blocks.

	Arnd

RE: [PATCH v4 1/3]mmc: set max_discard_sectors value for mmc queue

[Dong, Chuanxiao]

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Saturday, February 12, 2011 4:38 PM
> To: Dong, Chuanxiao
> Cc: linux-mmc@vger.kernel.org; cjb@laptop.org; linux-kernel@vger.kernel.org;
> akpm@linux-foundation.org; adrian.hunter@nokia.com
> Subject: Re: [PATCH v4 1/3]mmc: set max_discard_sectors value for mmc queue
> 
> On Saturday 12 February 2011 07:22:14 Chuanxiao Dong wrote:
> > max_discard_sectors value is UINT_MAX which means kernel block layer can pass
> > down unlimited sectors to MMC driver to erase. But erasing so many sectors may
> > delay some other important I/O requests. This is not preferred.
> >
> > So use 'pref_erase' to set a suitable max_discard_sectors value for mmc queue to
> > avoid erasing too many sectors at one time.
> >
> > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com>
> 
> I'm not sure about this one. pref_erase on SDHC cards should be the *minimum*
> unit you can erase in one request, not the maximum. Erasing an arbitrary
Hi Arnd,
I found the comment in mmc_init_erase() function which explains the 'pref_erase':
'pref_erase' is defined as a guide to limit erases to that size and alignment.
So I think it is not the minimum unit driver can erase, also not the maximum erase unit. It just a guide erase size for driver which can avoid holding host controller too long to response other I/O requests.

> number of allocation units on an SDHC card should complete almost instantly,
> because it only needs to update a single table with the allocation units.
>
> Discarding partial allocation units will take a lot longer, because the
> card then has to copy over the remaining blocks.
Is discarding started when card is idle or right after erase command? Can it cause some other I/O requests delayed?

Re: [PATCH v4 1/3]mmc: set max_discard_sectors value for mmc queue

[Arnd Bergmann]

On Saturday 12 February 2011 11:42:51 Dong, Chuanxiao wrote:
> > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > On Saturday 12 February 2011 07:22:14 Chuanxiao Dong wrote:
> > > max_discard_sectors value is UINT_MAX which means kernel block layer can pass
> > > down unlimited sectors to MMC driver to erase. But erasing so many sectors may
> > > delay some other important I/O requests. This is not preferred.
> > >
> > > So use 'pref_erase' to set a suitable max_discard_sectors value for mmc queue to
> > > avoid erasing too many sectors at one time.
> > >
> > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com>
> > 
> > I'm not sure about this one. pref_erase on SDHC cards should be the *minimum*
> > unit you can erase in one request, not the maximum. Erasing an arbitrary
>
> I found the comment in mmc_init_erase() function which explains the 'pref_erase':
> 'pref_erase' is defined as a guide to limit erases to that size and alignment.
> So I think it is not the minimum unit driver can erase, also not the maximum 
> erase unit. It just a guide erase size for driver which can avoid holding host 
> controller too long to response other I/O requests.

Ok, I see. Adrian introduced the function and the comment last year,
maybe he can comment on this some more. For all cards I've seen,
multi-AU erases on SDHC cards are really fast, though the standard
clearly allows for very slow erases.

My feeling is that the function is suboptimal right now, because it assumes
that one AU is the best size. If a card reports that it can erase
many AUs quickly (large N_erase, small T_erase, relatively large T_offset),
we should report that to the user.

> > number of allocation units on an SDHC card should complete almost instantly,
> > because it only needs to update a single table with the allocation units.
> >
> > Discarding partial allocation units will take a lot longer, because the
> > card then has to copy over the remaining blocks.
>
> Is discarding started when card is idle or right after erase command?
> Can it cause some other I/O requests delayed?

As far as I can tell, most operations on the card are synchronous when it's
writing. The erase command on a partial AU should not return until the
remaining data is copied to a new location, so it will take some time, but
physically the AU can be done in the background, if reading from it simply
returns a zero buffer rather than accessing the old data. Write-after-erase
might have to wait for the erase to complete, but good cards could also
hide that.

	Arnd

RE: [PATCH v4 1/3]mmc: set max_discard_sectors value for mmc queue

[Dong, Chuanxiao]

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Sunday, February 13, 2011 2:05 AM
> To: Dong, Chuanxiao
> Cc: linux-mmc@vger.kernel.org; cjb@laptop.org; linux-kernel@vger.kernel.org;
> akpm@linux-foundation.org; adrian.hunter@nokia.com
> Subject: Re: [PATCH v4 1/3]mmc: set max_discard_sectors value for mmc queue
> 
> On Saturday 12 February 2011 11:42:51 Dong, Chuanxiao wrote:
> > > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > On Saturday 12 February 2011 07:22:14 Chuanxiao Dong wrote:
> > > > max_discard_sectors value is UINT_MAX which means kernel block layer can
> pass
> > > > down unlimited sectors to MMC driver to erase. But erasing so many sectors
> may
> > > > delay some other important I/O requests. This is not preferred.
> > > >
> > > > So use 'pref_erase' to set a suitable max_discard_sectors value for mmc
> queue to
> > > > avoid erasing too many sectors at one time.
> > > >
> > > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com>
> > >
> > > I'm not sure about this one. pref_erase on SDHC cards should be the
> *minimum*
> > > unit you can erase in one request, not the maximum. Erasing an arbitrary
> >
> > I found the comment in mmc_init_erase() function which explains the
> 'pref_erase':
> > 'pref_erase' is defined as a guide to limit erases to that size and alignment.
> > So I think it is not the minimum unit driver can erase, also not the maximum
> > erase unit. It just a guide erase size for driver which can avoid holding host
> > controller too long to response other I/O requests.
> 
> Ok, I see. Adrian introduced the function and the comment last year,
> maybe he can comment on this some more. For all cards I've seen,
> multi-AU erases on SDHC cards are really fast, though the standard
> clearly allows for very slow erases.
> 
> My feeling is that the function is suboptimal right now, because it assumes
> that one AU is the best size. If a card reports that it can erase
> many AUs quickly (large N_erase, small T_erase, relatively large T_offset),
> we should report that to the user.
> 
> > > number of allocation units on an SDHC card should complete almost instantly,
> > > because it only needs to update a single table with the allocation units.
> > >
> > > Discarding partial allocation units will take a lot longer, because the
> > > card then has to copy over the remaining blocks.
> >
> > Is discarding started when card is idle or right after erase command?
> > Can it cause some other I/O requests delayed?
> 
> As far as I can tell, most operations on the card are synchronous when it's
> writing. The erase command on a partial AU should not return until the
> remaining data is copied to a new location, so it will take some time, but
> physically the AU can be done in the background, if reading from it simply
> returns a zero buffer rather than accessing the old data. Write-after-erase
> might have to wait for the erase to complete, but good cards could also
> hide that.
Thanks, Arnd.
When I do trim with a 32GB eMMC card in my platform, sometimes I can get the 10s timeout errors but sometimes not. I am not much clear about the "discarding partial AU will take a lot longer". If this action is hide for driver, then I think from driver side, the UINT_MAX value for max_discard_sectors will be OK. But if this action sometimes need driver to wait for some hardware interrupt, then I think the UINT_MAX value is not preferred.
Arnd, have any suggestion of dealing this? What I thought is using other value instead of using UINT_MAX.

Thanks
Chuanxiao

Re: [PATCH v4 1/3]mmc: set max_discard_sectors value for mmc queue

[Arnd Bergmann]

On Monday 14 February 2011, Dong, Chuanxiao wrote:
> When I do trim with a 32GB eMMC card in my platform, sometimes I can get the 10s
> timeout errors but sometimes not. I am not much clear about the "discarding partial
> AU will take a lot longer". If this action is hide for driver, then I think from 
> driver side, the UINT_MAX value for max_discard_sectors will be OK. But if this action
> sometimes need driver to wait for some hardware interrupt, then I think the UINT_MAX
> value is not preferred.
>
> Arnd, have any suggestion of dealing this? What I thought is using other value
> instead of using UINT_MAX.

I'm not too familiar with the eMMC spec, but it should have a way to calculate
a maximum trim timeout like SD 3.0 does for AU erases. When I've seen the timeouts
with SDHCI (missing your patch), it was always a bug in the driver, and the
erase was already completed before the driver even started waiting for the
interrupt.

10 seconds still sounds like a reasonable timeout, and we should probably
not issue any requests that might take longer than that, so I think the
interesting question is how to determine a good value for pref_erase,
so we can still take your patch.

	Arnd

Re: [PATCH v4 1/3]mmc: set max_discard_sectors value for mmc queue

[Adrian Hunter]

On 12/02/11 08:22, ext Chuanxiao Dong wrote:
> max_discard_sectors value is UINT_MAX which means kernel block layer can pass
> down unlimited sectors to MMC driver to erase. But erasing so many sectors may
> delay some other important I/O requests. This is not preferred.
>
> So use 'pref_erase' to set a suitable max_discard_sectors value for mmc queue to
> avoid erasing too many sectors at one time.
>
> Signed-off-by: Chuanxiao Dong<chuanxiao.dong@intel.com>
> ---
>   drivers/mmc/card/queue.c |    3 ++-
>   drivers/mmc/core/core.c  |   20 ++++++++++++++++++++
>   include/linux/mmc/core.h |    1 +
>   3 files changed, 23 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> index 4e42d03..6c13859 100644
> --- a/drivers/mmc/card/queue.c
> +++ b/drivers/mmc/card/queue.c
> @@ -131,7 +131,8 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card, spinlock_t *lock
>   	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, mq->queue);
>   	if (mmc_can_erase(card)) {
>   		queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, mq->queue);
> -		mq->queue->limits.max_discard_sectors = UINT_MAX;
> +		mq->queue->limits.max_discard_sectors =
> +			mmc_set_max_discard_sectors(card);
>   		if (card->erased_byte == 0)
>   			mq->queue->limits.discard_zeroes_data = 1;
>   		if (!mmc_can_trim(card)&&  is_power_of_2(card->erase_size)) {
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 34a7e8c..0eb27aa 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1470,6 +1470,26 @@ int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
>   }
>   EXPORT_SYMBOL(mmc_erase_group_aligned);
>
> +/*
> + * Set max_discard_sectors for mmc queue.
> + * max_discard_sectors will determine how many sectors can be erased at one
> + * time. Sometimes user may want to erase a large area of SD or MMC card, it may
> + * take long time which delay some other important I/O requests. So we would
> + * better set some values for max_discard_sectors which can forbid block layer
> + * pass too many sectors at one time.
> + *
> + * 'pref_erase' is initialized for this purpose. We can use it to set a preferred
> + * value for it.
> + */
> +int mmc_set_max_discard_sectors(struct mmc_card *card)
> +{
> +	if (card->pref_erase)
> +		return card->pref_erase;

This will cause orders of magnitude decrease in secure erase
performance for eMMC.

> +	else
> +		return UINT_MAX;
> +}
> +EXPORT_SYMBOL(mmc_set_max_discard_sectors);
> +
>   int mmc_set_blocklen(struct mmc_card *card, unsigned int blocklen)
>   {
>   	struct mmc_command cmd;
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 64e013f..a218d57 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -152,6 +152,7 @@ extern int mmc_can_trim(struct mmc_card *card);
>   extern int mmc_can_secure_erase_trim(struct mmc_card *card);
>   extern int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
>   				   unsigned int nr);
> +extern int mmc_set_max_discard_sectors(struct mmc_card *card);
>
>   extern int mmc_set_blocklen(struct mmc_card *card, unsigned int blocklen);
>