[PATCH] mtd: core: only increment ecc_stats.badblocks on confirmed good->bad transition

[PATCH] mtd: core: only increment ecc_stats.badblocks on confirmed good->bad transition

[Wang Zhaolong]

Repeatedly marking the same eraseblock bad inflates
mtd->ecc_stats.badblocks because mtd_block_markbad() unconditionally
increments the counter on success, while some implementations (e.g.
NAND) return 0 both when the block was already bad and when it has just
been marked[1].

Fix by probing the bad-block state before and after calling
->_block_markbad() (when _block_isbad is available) and only increment
the counter on a confirmed good->bad transition. If _block_isbad is not
implemented, be conservative and do not increment.

Keep the logic centralized in mtdcore; the markbad path is not a hot
path, so the extra IO is acceptable.

Link: https://lore.kernel.org/all/ef573188-9815-4a6b-bad1-3d8ff7c9b16f@huaweicloud.com/ [1]
Signed-off-by: Wang Zhaolong <wangzhaolong@huaweicloud.com>
---
 drivers/mtd/mtdcore.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 5ba9a741f5ac..a6d38da651e9 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -2338,10 +2338,12 @@ EXPORT_SYMBOL_GPL(mtd_block_isbad);
 
 int mtd_block_markbad(struct mtd_info *mtd, loff_t ofs)
 {
 	struct mtd_info *master = mtd_get_master(mtd);
 	int ret;
+	loff_t moffs;
+	int oldbad = -1;
 
 	if (!master->_block_markbad)
 		return -EOPNOTSUPP;
 	if (ofs < 0 || ofs >= mtd->size)
 		return -EINVAL;
@@ -2349,17 +2351,35 @@ int mtd_block_markbad(struct mtd_info *mtd, loff_t ofs)
 		return -EROFS;
 
 	if (mtd->flags & MTD_SLC_ON_MLC_EMULATION)
 		ofs = (loff_t)mtd_div_by_eb(ofs, mtd) * master->erasesize;
 
-	ret = master->_block_markbad(master, mtd_get_master_ofs(mtd, ofs));
+	moffs = mtd_get_master_ofs(mtd, ofs);
+
+	/* Pre-check: remember current state if available. */
+	if (master->_block_isbad)
+		oldbad = master->_block_isbad(master, moffs);
+
+	ret = master->_block_markbad(master, moffs);
 	if (ret)
 		return ret;
 
-	while (mtd->parent) {
-		mtd->ecc_stats.badblocks++;
-		mtd = mtd->parent;
+	/*
+	 * Post-check and bump stats only on a confirmed good->bad transition.
+	 * If _block_isbad is not implemented, be conservative and do not bump.
+	 */
+	if (master->_block_isbad) {
+		/* If it was already bad, nothing to do. */
+		if (oldbad > 0)
+			return 0;
+
+		if (master->_block_isbad(master, moffs) > 0) {
+			while (mtd->parent) {
+				mtd->ecc_stats.badblocks++;
+				mtd = mtd->parent;
+			}
+		}
 	}
 
 	return 0;
 }
 EXPORT_SYMBOL_GPL(mtd_block_markbad);
-- 
2.39.2

Re: [PATCH] mtd: core: only increment ecc_stats.badblocks on confirmed good->bad transition

[Miquel Raynal]

Hello Wang,

> @@ -2349,17 +2351,35 @@ int mtd_block_markbad(struct mtd_info *mtd, loff_t ofs)
>  		return -EROFS;
>  
>  	if (mtd->flags & MTD_SLC_ON_MLC_EMULATION)
>  		ofs = (loff_t)mtd_div_by_eb(ofs, mtd) * master->erasesize;
>  
> -	ret = master->_block_markbad(master, mtd_get_master_ofs(mtd, ofs));
> +	moffs = mtd_get_master_ofs(mtd, ofs);
> +
> +	/* Pre-check: remember current state if available. */
> +	if (master->_block_isbad)
> +		oldbad = master->_block_isbad(master, moffs);
> +
> +	ret = master->_block_markbad(master, moffs);
>  	if (ret)
>  		return ret;
>  
> -	while (mtd->parent) {
> -		mtd->ecc_stats.badblocks++;
> -		mtd = mtd->parent;
> +	/*
> +	 * Post-check and bump stats only on a confirmed good->bad transition.
> +	 * If _block_isbad is not implemented, be conservative and do not bump.
> +	 */
> +	if (master->_block_isbad) {
> +		/* If it was already bad, nothing to do. */
> +		if (oldbad > 0)
> +			return 0;
> +
> +		if (master->_block_isbad(master, moffs) > 0) {
> +			while (mtd->parent) {
> +				mtd->ecc_stats.badblocks++;
> +				mtd = mtd->parent;
> +			}
> +		}

I don't think you can assume the block was already bad and must not be
accounted as a new bad block if you cannot verify that. In this case we
must remain conservative and tell userspace a new block was marked bad,
I believe.

Said otherwise, the { while () badblocks++ } block shall remain outside
of the if (_block_isbad) condition and remain untouched. Just bail out
early if you are sure this is not needed.

Thanks,
Miquèl

Re: [PATCH] mtd: core: only increment ecc_stats.badblocks on confirmed good->bad transition

[Wang Zhaolong]

Hi Miquèl,


> Hello Wang,
> 
>> @@ -2349,17 +2351,35 @@ int mtd_block_markbad(struct mtd_info *mtd, loff_t ofs)
>>   		return -EROFS;
>>   
>>   	if (mtd->flags & MTD_SLC_ON_MLC_EMULATION)
>>   		ofs = (loff_t)mtd_div_by_eb(ofs, mtd) * master->erasesize;
>>   
>> -	ret = master->_block_markbad(master, mtd_get_master_ofs(mtd, ofs));
>> +	moffs = mtd_get_master_ofs(mtd, ofs);
>> +
>> +	/* Pre-check: remember current state if available. */
>> +	if (master->_block_isbad)
>> +		oldbad = master->_block_isbad(master, moffs);
>> +
>> +	ret = master->_block_markbad(master, moffs);
>>   	if (ret)
>>   		return ret;
>>   
>> -	while (mtd->parent) {
>> -		mtd->ecc_stats.badblocks++;
>> -		mtd = mtd->parent;
>> +	/*
>> +	 * Post-check and bump stats only on a confirmed good->bad transition.
>> +	 * If _block_isbad is not implemented, be conservative and do not bump.
>> +	 */
>> +	if (master->_block_isbad) {
>> +		/* If it was already bad, nothing to do. */
>> +		if (oldbad > 0)
>> +			return 0;
>> +
>> +		if (master->_block_isbad(master, moffs) > 0) {
>> +			while (mtd->parent) {
>> +				mtd->ecc_stats.badblocks++;
>> +				mtd = mtd->parent;
>> +			}
>> +		}
> 
> I don't think you can assume the block was already bad and must not be
> accounted as a new bad block if you cannot verify that. In this case we
> must remain conservative and tell userspace a new block was marked bad,
> I believe.

Thanks for the review and the clear guidance.

My intent was to avoid inflating badblocks by only bumping the counter on a
confirmed good->bad transition. I tried to ground the decision on observable
state (pre/post isbad) rather than return codes, and to avoid assuming success
implies immediate visibility across drivers. That’s why the increment was
tied to verification and skipped when _block_isbad was unavailable.

Your point about being conservative towards userspace is well taken: if we
cannot verify, we should still account the bad block. Skipping the increment
only when we can prove the block was already bad makes the contract clearer
and avoids under-reporting. Keeping the increment outside the if (_block_isbad)
block also results in simpler, more readable code.


> Said otherwise, the { while () badblocks++ } block shall remain outside
> of the if (_block_isbad) condition and remain untouched. Just bail out
> early if you are sure this is not needed.
> 

I’ll send a V2 shortly that:

- Checks old state when _block_isbad exists and bails out early if already bad
- Otherwise calls ->_block_markbad() and increments the counter on success, with
   the increment left outside of the conditional as you suggested

Thanks again for the direction.

Best regards,
Wang Zhaolong

Re: [PATCH] mtd: core: only increment ecc_stats.badblocks on confirmed good->bad transition

[Miquel Raynal]

Hello,

>> Said otherwise, the { while () badblocks++ } block shall remain outside
>> of the if (_block_isbad) condition and remain untouched. Just bail out
>> early if you are sure this is not needed.
>> 
>
> I’ll send a V2 shortly that:
>
> - Checks old state when _block_isbad exists and bails out early if already bad
> - Otherwise calls ->_block_markbad() and increments the counter on success, with
>   the increment left outside of the conditional as you suggested

LGTM.

Thanks,
Miquèl