| Message ID | 1417347340-6872-4-git-send-email-richard@nod.at |
|---|---|
| State | Accepted |
| Headers | show |
Hi Richard, On 11/30/2014 1:35 PM, Richard Weinberger wrote: > If ubi_update_fastmap() fails notify the user. > This is not a hard error as ubi_update_fastmap() makes sure that upon failure > the current on-flash fastmap will no be used upon next UBI attach. > > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > drivers/mtd/ubi/wl.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c > index 523d8a4..7821342 100644 > --- a/drivers/mtd/ubi/wl.c > +++ b/drivers/mtd/ubi/wl.c > @@ -657,7 +657,11 @@ again: > * refill the WL pool synchronous. */ > if (pool->used == pool->size || wl_pool->used == wl_pool->size) { > spin_unlock(&ubi->wl_lock); > - ubi_update_fastmap(ubi); > + ret = ubi_update_fastmap(ubi); > + if (ret) { > + ubi_msg(ubi, "Unable to write a new fastmap: %i", ret); > + return -ENOSPC; Why do you fail the whole function (ubi_wl_get_peb) if fastmap update failed? Its possible that the fm_pools were refilled correctly, and the actual fastmap_write failed, so there is nothing preventing the user to get peb allocated and continue working. You invalidate the fastmap, so if powercut occurs a full scan will be performed. So its possible to allocate from fm_pools (although fastmap is not valid on disc) and try writing fastmap again when the pools filled up. I'm for the ubi_msg but against "return -ENOSPC;" > + } > spin_lock(&ubi->wl_lock); > } > > Thanks, Tanya Brokhman
Am 07.12.2014 um 14:59 schrieb Tanya Brokhman: > Hi Richard, > > On 11/30/2014 1:35 PM, Richard Weinberger wrote: >> If ubi_update_fastmap() fails notify the user. >> This is not a hard error as ubi_update_fastmap() makes sure that upon failure >> the current on-flash fastmap will no be used upon next UBI attach. >> >> Signed-off-by: Richard Weinberger <richard@nod.at> >> --- >> drivers/mtd/ubi/wl.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c >> index 523d8a4..7821342 100644 >> --- a/drivers/mtd/ubi/wl.c >> +++ b/drivers/mtd/ubi/wl.c >> @@ -657,7 +657,11 @@ again: >> * refill the WL pool synchronous. */ >> if (pool->used == pool->size || wl_pool->used == wl_pool->size) { >> spin_unlock(&ubi->wl_lock); >> - ubi_update_fastmap(ubi); >> + ret = ubi_update_fastmap(ubi); >> + if (ret) { >> + ubi_msg(ubi, "Unable to write a new fastmap: %i", ret); >> + return -ENOSPC; > > Why do you fail the whole function (ubi_wl_get_peb) if fastmap update failed? Its possible that the fm_pools were refilled correctly, and the actual fastmap_write failed, so there > is nothing preventing the user to get peb allocated and continue working. You invalidate the fastmap, so if powercut occurs a full scan will be performed. So its possible to > allocate from fm_pools (although fastmap is not valid on disc) and try writing fastmap again when the pools filled up. > I'm for the ubi_msg but against "return -ENOSPC;" Maybe the case you've described is powercut safe, but there can be other unsafe cases. Let's stay on the safe side and be paranoid, it does not hurt. If fastmap has proven stable we can start with tricky optimizations. Thanks, //richard
On 12/7/2014 4:22 PM, Richard Weinberger wrote: > Am 07.12.2014 um 14:59 schrieb Tanya Brokhman: >> Hi Richard, >> >> On 11/30/2014 1:35 PM, Richard Weinberger wrote: >>> If ubi_update_fastmap() fails notify the user. >>> This is not a hard error as ubi_update_fastmap() makes sure that upon failure >>> the current on-flash fastmap will no be used upon next UBI attach. >>> >>> Signed-off-by: Richard Weinberger <richard@nod.at> >>> --- >>> drivers/mtd/ubi/wl.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c >>> index 523d8a4..7821342 100644 >>> --- a/drivers/mtd/ubi/wl.c >>> +++ b/drivers/mtd/ubi/wl.c >>> @@ -657,7 +657,11 @@ again: >>> * refill the WL pool synchronous. */ >>> if (pool->used == pool->size || wl_pool->used == wl_pool->size) { >>> spin_unlock(&ubi->wl_lock); >>> - ubi_update_fastmap(ubi); >>> + ret = ubi_update_fastmap(ubi); >>> + if (ret) { >>> + ubi_msg(ubi, "Unable to write a new fastmap: %i", ret); >>> + return -ENOSPC; >> >> Why do you fail the whole function (ubi_wl_get_peb) if fastmap update failed? Its possible that the fm_pools were refilled correctly, and the actual fastmap_write failed, so there >> is nothing preventing the user to get peb allocated and continue working. You invalidate the fastmap, so if powercut occurs a full scan will be performed. So its possible to >> allocate from fm_pools (although fastmap is not valid on disc) and try writing fastmap again when the pools filled up. >> I'm for the ubi_msg but against "return -ENOSPC;" > > Maybe the case you've described is powercut safe, but there can be other unsafe cases. > Let's stay on the safe side and be paranoid, it does not hurt. > If fastmap has proven stable we can start with tricky optimizations. I'm sorry that I'm being petty here but the commit msg states that you "notify the user in case of update fastamap failure". It says nothing about you failing ubi_wl_get_peb as well. And this is a major change. At least divide this into 2 patches (so I can disagree to the function failing and agree to the msg to user :) ) > Thanks, > //richard > Thanks, Tanya Brokhman
Hi! Am 08.12.2014 um 07:58 schrieb Tanya Brokhman: > On 12/7/2014 4:22 PM, Richard Weinberger wrote: >> Am 07.12.2014 um 14:59 schrieb Tanya Brokhman: >>> Hi Richard, >>> >>> On 11/30/2014 1:35 PM, Richard Weinberger wrote: >>>> If ubi_update_fastmap() fails notify the user. >>>> This is not a hard error as ubi_update_fastmap() makes sure that upon failure >>>> the current on-flash fastmap will no be used upon next UBI attach. >>>> >>>> Signed-off-by: Richard Weinberger <richard@nod.at> >>>> --- >>>> drivers/mtd/ubi/wl.c | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c >>>> index 523d8a4..7821342 100644 >>>> --- a/drivers/mtd/ubi/wl.c >>>> +++ b/drivers/mtd/ubi/wl.c >>>> @@ -657,7 +657,11 @@ again: >>>> * refill the WL pool synchronous. */ >>>> if (pool->used == pool->size || wl_pool->used == wl_pool->size) { >>>> spin_unlock(&ubi->wl_lock); >>>> - ubi_update_fastmap(ubi); >>>> + ret = ubi_update_fastmap(ubi); >>>> + if (ret) { >>>> + ubi_msg(ubi, "Unable to write a new fastmap: %i", ret); >>>> + return -ENOSPC; >>> >>> Why do you fail the whole function (ubi_wl_get_peb) if fastmap update failed? Its possible that the fm_pools were refilled correctly, and the actual fastmap_write failed, so there >>> is nothing preventing the user to get peb allocated and continue working. You invalidate the fastmap, so if powercut occurs a full scan will be performed. So its possible to >>> allocate from fm_pools (although fastmap is not valid on disc) and try writing fastmap again when the pools filled up. >>> I'm for the ubi_msg but against "return -ENOSPC;" >> >> Maybe the case you've described is powercut safe, but there can be other unsafe cases. >> Let's stay on the safe side and be paranoid, it does not hurt. >> If fastmap has proven stable we can start with tricky optimizations. > > I'm sorry that I'm being petty here but the commit msg states that you "notify the user in case of update fastamap failure". It says nothing about you failing ubi_wl_get_peb as > well. And this is a major change. At least divide this into 2 patches (so I can disagree to the function failing and agree to the msg to user :) ) With user I meant users of that function. Thanks, //richard
On 12/8/2014 11:11 AM, Richard Weinberger wrote: > Hi! > > Am 08.12.2014 um 07:58 schrieb Tanya Brokhman: >> On 12/7/2014 4:22 PM, Richard Weinberger wrote: >>> Am 07.12.2014 um 14:59 schrieb Tanya Brokhman: >>>> Hi Richard, >>>> >>>> On 11/30/2014 1:35 PM, Richard Weinberger wrote: >>>>> If ubi_update_fastmap() fails notify the user. >>>>> This is not a hard error as ubi_update_fastmap() makes sure that upon failure >>>>> the current on-flash fastmap will no be used upon next UBI attach. >>>>> >>>>> Signed-off-by: Richard Weinberger <richard@nod.at> >>>>> --- >>>>> drivers/mtd/ubi/wl.c | 6 +++++- >>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c >>>>> index 523d8a4..7821342 100644 >>>>> --- a/drivers/mtd/ubi/wl.c >>>>> +++ b/drivers/mtd/ubi/wl.c >>>>> @@ -657,7 +657,11 @@ again: >>>>> * refill the WL pool synchronous. */ >>>>> if (pool->used == pool->size || wl_pool->used == wl_pool->size) { >>>>> spin_unlock(&ubi->wl_lock); >>>>> - ubi_update_fastmap(ubi); >>>>> + ret = ubi_update_fastmap(ubi); >>>>> + if (ret) { >>>>> + ubi_msg(ubi, "Unable to write a new fastmap: %i", ret); >>>>> + return -ENOSPC; >>>> >>>> Why do you fail the whole function (ubi_wl_get_peb) if fastmap update failed? Its possible that the fm_pools were refilled correctly, and the actual fastmap_write failed, so there >>>> is nothing preventing the user to get peb allocated and continue working. You invalidate the fastmap, so if powercut occurs a full scan will be performed. So its possible to >>>> allocate from fm_pools (although fastmap is not valid on disc) and try writing fastmap again when the pools filled up. >>>> I'm for the ubi_msg but against "return -ENOSPC;" >>> >>> Maybe the case you've described is powercut safe, but there can be other unsafe cases. >>> Let's stay on the safe side and be paranoid, it does not hurt. >>> If fastmap has proven stable we can start with tricky optimizations. >> >> I'm sorry that I'm being petty here but the commit msg states that you "notify the user in case of update fastamap failure". It says nothing about you failing ubi_wl_get_peb as >> well. And this is a major change. At least divide this into 2 patches (so I can disagree to the function failing and agree to the msg to user :) ) > > With user I meant users of that function. I still don't like it. Leaving this one for Artem... sorry > > Thanks, > //richard > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ > Thanks, Tanya Brokhman
Am 08.12.2014 um 14:00 schrieb Tanya Brokhman: >>>> Maybe the case you've described is powercut safe, but there can be other unsafe cases. >>>> Let's stay on the safe side and be paranoid, it does not hurt. >>>> If fastmap has proven stable we can start with tricky optimizations. >>> >>> I'm sorry that I'm being petty here but the commit msg states that you "notify the user in case of update fastamap failure". It says nothing about you failing ubi_wl_get_peb as >>> well. And this is a major change. At least divide this into 2 patches (so I can disagree to the function failing and agree to the msg to user :) ) >> >> With user I meant users of that function. > > I still don't like it. > Leaving this one for Artem... sorry As I said, as soon we all consider fastmap mature and stable we can start with optimizations. But as of now I want all logic to be on the safe side. This is how all fastmap code is designed. Thanks, //richard
Am 08.12.2014 um 14:00 schrieb Tanya Brokhman: >>>>> Why do you fail the whole function (ubi_wl_get_peb) if fastmap update failed? Its possible that the fm_pools were refilled correctly, and the actual fastmap_write failed, so >>>>> there >>>>> is nothing preventing the user to get peb allocated and continue working. You invalidate the fastmap, so if powercut occurs a full scan will be performed. So its possible to >>>>> allocate from fm_pools (although fastmap is not valid on disc) and try writing fastmap again when the pools filled up. >>>>> I'm for the ubi_msg but against "return -ENOSPC;" >>>> >>>> Maybe the case you've described is powercut safe, but there can be other unsafe cases. >>>> Let's stay on the safe side and be paranoid, it does not hurt. >>>> If fastmap has proven stable we can start with tricky optimizations. >>> >>> I'm sorry that I'm being petty here but the commit msg states that you "notify the user in case of update fastamap failure". It says nothing about you failing ubi_wl_get_peb as >>> well. And this is a major change. At least divide this into 2 patches (so I can disagree to the function failing and agree to the msg to user :) ) >> >> With user I meant users of that function. > > I still don't like it. > Leaving this one for Artem... sorry BTW: With my latest patch applied "[PATCH] UBI: Fastmap: Fix possible fastmap inconsistency" your assumption that we can have the pools refilled in case if an ubi_update_fastmap() error is no longer correct. Before my patch ubi_update_fastmap() the pools have been refilled much too early, this is an bug and got fixed. Thanks, //richard
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c index 523d8a4..7821342 100644 --- a/drivers/mtd/ubi/wl.c +++ b/drivers/mtd/ubi/wl.c @@ -657,7 +657,11 @@ again: * refill the WL pool synchronous. */ if (pool->used == pool->size || wl_pool->used == wl_pool->size) { spin_unlock(&ubi->wl_lock); - ubi_update_fastmap(ubi); + ret = ubi_update_fastmap(ubi); + if (ret) { + ubi_msg(ubi, "Unable to write a new fastmap: %i", ret); + return -ENOSPC; + } spin_lock(&ubi->wl_lock); }
If ubi_update_fastmap() fails notify the user. This is not a hard error as ubi_update_fastmap() makes sure that upon failure the current on-flash fastmap will no be used upon next UBI attach. Signed-off-by: Richard Weinberger <richard@nod.at> --- drivers/mtd/ubi/wl.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)