Message ID | 1585400477-65705-1-git-send-email-nixiaoming@huawei.com |
---|---|
State | Superseded |
Delegated to: | Miquel Raynal |
Headers | show |
Series | mtd:clear cache_state to avoid writing to bad clocks repeatedly | expand |
Hi Xiaoming, Xiaoming Ni <nixiaoming@huawei.com> wrote on Sat, 28 Mar 2020 21:01:17 +0800: > The function call process is as follows: > mtd_blktrans_work() > while (1) > do_blktrans_request() > mtdblock_writesect() > do_cached_write() > write_cached_data() /*if cache_state is STATE_DIRTY*/ > erase_write() > > write_cached_data() returns failure without modifying cache_state > and cache_offset. so when do_cached_write() is called again, > write_cached_data() will be called again to perform erase_write() > on the same cache_offset. > > but if this cache_offset points to a bad block, erase_write() will > always return -EIO. Writing to this mtdblk is equivalent to losing > the current data, and repeatedly writing to the bad block. > > Repeatedly writing a bad block has no real benefits, > but brings some negative effects: > 1 Lost subsequent data > 2 Loss of flash device life > 3 erase_write() bad blocks are very time-consuming. for example: > the function do_erase_oneblock() in chips/cfi_cmdset_0020.c or > chips/cfi_cmdset_0002.c may take more than 20 seconds to return > > Therefore, when erase_write() returns -EIO in write_cached_data(), > clear cache_state to avoid writing to bad clocks repeatedly. blocks > > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com> > --- > drivers/mtd/mtdblock.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/mtdblock.c b/drivers/mtd/mtdblock.c > index 078e0f6..98c25d6 100644 > --- a/drivers/mtd/mtdblock.c > +++ b/drivers/mtd/mtdblock.c > @@ -89,8 +89,6 @@ static int write_cached_data (struct mtdblk_dev *mtdblk) > > ret = erase_write (mtd, mtdblk->cache_offset, > mtdblk->cache_size, mtdblk->cache_data); > - if (ret) > - return ret; > > /* > * Here we could arguably set the cache state to STATE_CLEAN. > @@ -98,9 +96,14 @@ static int write_cached_data (struct mtdblk_dev *mtdblk) > * be notified if this content is altered on the flash by other > * means. Let's declare it empty and leave buffering tasks to > * the buffer cache instead. > + * > + * if this cache_offset points to a bad block Can you start your sentences with a capital letter please? * If > + * data cannot be written to the device. > + * clear cache_state to avoid writing to bad clocks repeatedly * Clear And also please break your lines à 80, not 70. > */ > - mtdblk->cache_state = STATE_EMPTY; > - return 0; > + if (ret == 0 || ret == -EIO) > + mtdblk->cache_state = STATE_EMPTY; > + return ret; > } > > Otherwise looks good to me. With the above addressed: Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> Thanks, Miquèl
Hi Xiaoming, Xiaoming Ni <nixiaoming@huawei.com> wrote on Sat, 28 Mar 2020 21:01:17 +0800: Please also fix the title. "mtd: clear ..." Thanks, Miquèl
On 2020/3/30 15:53, Miquel Raynal wrote: > Hi Xiaoming, > > Xiaoming Ni <nixiaoming@huawei.com> wrote on Sat, 28 Mar 2020 21:01:17 > +0800: > >> The function call process is as follows: >> mtd_blktrans_work() >> while (1) .... >> + * >> + * if this cache_offset points to a bad block > > Can you start your sentences with a capital letter please? > > * If > >> + * data cannot be written to the device. >> + * clear cache_state to avoid writing to bad clocks repeatedly > > * Clear > > And also please break your lines à 80, not 70. > >> */ >> - mtdblk->cache_state = STATE_EMPTY; >> - return 0; >> + if (ret == 0 || ret == -EIO) >> + mtdblk->cache_state = STATE_EMPTY; Should I add a warning print for EIO here >> + return ret; >> } >> >> > > Otherwise looks good to me. > > With the above addressed: > > Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > Thanks, > Miquèl > > . Thanks for your advice, I will send v2 patch later Thanks, Xiaoming Ni
Hi Xiaoming, Xiaoming Ni <nixiaoming@huawei.com> wrote on Mon, 30 Mar 2020 16:44:29 +0800: > On 2020/3/30 15:53, Miquel Raynal wrote: > > Hi Xiaoming, > > > > Xiaoming Ni <nixiaoming@huawei.com> wrote on Sat, 28 Mar 2020 21:01:17 > > +0800: > > > >> The function call process is as follows: > >> mtd_blktrans_work() > >> while (1) > .... > > >> + * > >> + * if this cache_offset points to a bad block > > > > Can you start your sentences with a capital letter please? > > > > * If > > > >> + * data cannot be written to the device. > >> + * clear cache_state to avoid writing to bad clocks repeatedly > > > > * Clear > > > > And also please break your lines à 80, not 70. > > > >> */ > >> - mtdblk->cache_state = STATE_EMPTY; > >> - return 0; > >> + if (ret == 0 || ret == -EIO) > >> + mtdblk->cache_state = STATE_EMPTY; > Should I add a warning print for EIO here I don't think so. Thanks, Miquèl
diff --git a/drivers/mtd/mtdblock.c b/drivers/mtd/mtdblock.c index 078e0f6..98c25d6 100644 --- a/drivers/mtd/mtdblock.c +++ b/drivers/mtd/mtdblock.c @@ -89,8 +89,6 @@ static int write_cached_data (struct mtdblk_dev *mtdblk) ret = erase_write (mtd, mtdblk->cache_offset, mtdblk->cache_size, mtdblk->cache_data); - if (ret) - return ret; /* * Here we could arguably set the cache state to STATE_CLEAN. @@ -98,9 +96,14 @@ static int write_cached_data (struct mtdblk_dev *mtdblk) * be notified if this content is altered on the flash by other * means. Let's declare it empty and leave buffering tasks to * the buffer cache instead. + * + * if this cache_offset points to a bad block + * data cannot be written to the device. + * clear cache_state to avoid writing to bad clocks repeatedly */ - mtdblk->cache_state = STATE_EMPTY; - return 0; + if (ret == 0 || ret == -EIO) + mtdblk->cache_state = STATE_EMPTY; + return ret; }
The function call process is as follows: mtd_blktrans_work() while (1) do_blktrans_request() mtdblock_writesect() do_cached_write() write_cached_data() /*if cache_state is STATE_DIRTY*/ erase_write() write_cached_data() returns failure without modifying cache_state and cache_offset. so when do_cached_write() is called again, write_cached_data() will be called again to perform erase_write() on the same cache_offset. but if this cache_offset points to a bad block, erase_write() will always return -EIO. Writing to this mtdblk is equivalent to losing the current data, and repeatedly writing to the bad block. Repeatedly writing a bad block has no real benefits, but brings some negative effects: 1 Lost subsequent data 2 Loss of flash device life 3 erase_write() bad blocks are very time-consuming. for example: the function do_erase_oneblock() in chips/cfi_cmdset_0020.c or chips/cfi_cmdset_0002.c may take more than 20 seconds to return Therefore, when erase_write() returns -EIO in write_cached_data(), clear cache_state to avoid writing to bad clocks repeatedly. Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com> --- drivers/mtd/mtdblock.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)