Artem,
this is pile 2 of 7.
[PATCH 1/6] UBI: Fastmap: Fix memory leaks while closing the WL
[PATCH 2/6] UBI: Fastmap: Don't allocate new ubi_wl_entry objects
[PATCH 3/6] UBI: Fastmap: Notify user in case of an
[PATCH 4/6] UBI: Fastmap: Wrap fastmap specific function in a ifdef
[PATCH 5/6] UBI: Fastmap: Fix fastmap usage in ubi_volume_notify()
[PATCH 6/6] UBI: Fastmap: Fix memory leak while attaching
Thanks,
//richard
Add a ubi_fastmap_close() to free all resources used by fastmap
at WL shutdown.
Signed-off-by: Richard Weinberger <[email protected]>
---
drivers/mtd/ubi/wl.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index c2822f7..47b215f 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -2064,6 +2064,23 @@ static void protection_queue_destroy(struct ubi_device *ubi)
}
}
+static void ubi_fastmap_close(struct ubi_device *ubi)
+{
+#ifdef CONFIG_MTD_UBI_FASTMAP
+ int i;
+
+ flush_work(&ubi->fm_work);
+ return_unused_pool_pebs(ubi, &ubi->fm_pool);
+ return_unused_pool_pebs(ubi, &ubi->fm_wl_pool);
+
+ if (ubi->fm) {
+ for (i = 0; i < ubi->fm->used_blocks; i++)
+ kfree(ubi->fm->e[i]);
+ }
+ kfree(ubi->fm);
+#endif
+}
+
/**
* ubi_wl_close - close the wear-leveling sub-system.
* @ubi: UBI device description object
@@ -2071,9 +2088,7 @@ static void protection_queue_destroy(struct ubi_device *ubi)
void ubi_wl_close(struct ubi_device *ubi)
{
dbg_wl("close the WL sub-system");
-#ifdef CONFIG_MTD_UBI_FASTMAP
- flush_work(&ubi->fm_work);
-#endif
+ ubi_fastmap_close(ubi);
shutdown_work(ubi);
protection_queue_destroy(ubi);
tree_destroy(&ubi->used);
--
1.8.4.5
Currently we leak a few ubi_ainf_pebs while attaching.
Signed-off-by: Richard Weinberger <[email protected]>
---
drivers/mtd/ubi/attach.c | 61 +++++++++++++++++++++++++----------------------
drivers/mtd/ubi/fastmap.c | 13 ----------
2 files changed, 33 insertions(+), 41 deletions(-)
diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 9d2e16f..32cad5c 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -1301,6 +1301,30 @@ out_ech:
return err;
}
+static struct ubi_attach_info *alloc_ai(const char *slab_name)
+{
+ struct ubi_attach_info *ai;
+
+ ai = kzalloc(sizeof(struct ubi_attach_info), GFP_KERNEL);
+ if (!ai)
+ return ai;
+
+ INIT_LIST_HEAD(&ai->corr);
+ INIT_LIST_HEAD(&ai->free);
+ INIT_LIST_HEAD(&ai->erase);
+ INIT_LIST_HEAD(&ai->alien);
+ ai->volumes = RB_ROOT;
+ ai->aeb_slab_cache = kmem_cache_create(slab_name,
+ sizeof(struct ubi_ainf_peb),
+ 0, 0, NULL);
+ if (!ai->aeb_slab_cache) {
+ kfree(ai);
+ ai = NULL;
+ }
+
+ return ai;
+}
+
#ifdef CONFIG_MTD_UBI_FASTMAP
/**
@@ -1313,7 +1337,7 @@ out_ech:
* UBI_NO_FASTMAP denotes that no fastmap was found.
* UBI_BAD_FASTMAP denotes that the found fastmap was invalid.
*/
-static int scan_fast(struct ubi_device *ubi, struct ubi_attach_info *ai)
+static int scan_fast(struct ubi_device *ubi, struct ubi_attach_info **ai)
{
int err, pnum, fm_anchor = -1;
unsigned long long max_sqnum = 0;
@@ -1334,7 +1358,7 @@ static int scan_fast(struct ubi_device *ubi, struct ubi_attach_info *ai)
cond_resched();
dbg_gen("process PEB %d", pnum);
- err = scan_peb(ubi, ai, pnum, &vol_id, &sqnum);
+ err = scan_peb(ubi, *ai, pnum, &vol_id, &sqnum);
if (err < 0)
goto out_vidh;
@@ -1350,7 +1374,12 @@ static int scan_fast(struct ubi_device *ubi, struct ubi_attach_info *ai)
if (fm_anchor < 0)
return UBI_NO_FASTMAP;
- return ubi_scan_fastmap(ubi, ai, fm_anchor);
+ destroy_ai(*ai);
+ *ai = alloc_ai("ubi_aeb_slab_cache");
+ if (!*ai)
+ return -ENOMEM;
+
+ return ubi_scan_fastmap(ubi, *ai, fm_anchor);
out_vidh:
ubi_free_vid_hdr(ubi, vidh);
@@ -1362,30 +1391,6 @@ out:
#endif
-static struct ubi_attach_info *alloc_ai(const char *slab_name)
-{
- struct ubi_attach_info *ai;
-
- ai = kzalloc(sizeof(struct ubi_attach_info), GFP_KERNEL);
- if (!ai)
- return ai;
-
- INIT_LIST_HEAD(&ai->corr);
- INIT_LIST_HEAD(&ai->free);
- INIT_LIST_HEAD(&ai->erase);
- INIT_LIST_HEAD(&ai->alien);
- ai->volumes = RB_ROOT;
- ai->aeb_slab_cache = kmem_cache_create(slab_name,
- sizeof(struct ubi_ainf_peb),
- 0, 0, NULL);
- if (!ai->aeb_slab_cache) {
- kfree(ai);
- ai = NULL;
- }
-
- return ai;
-}
-
/**
* ubi_attach - attach an MTD device.
* @ubi: UBI device descriptor
@@ -1413,7 +1418,7 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
if (force_scan)
err = scan_all(ubi, ai, 0);
else {
- err = scan_fast(ubi, ai);
+ err = scan_fast(ubi, &ai);
if (err > 0) {
if (err != UBI_NO_FASTMAP) {
destroy_ai(ai);
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 9507702..600c4f9 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -561,21 +561,8 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
INIT_LIST_HEAD(&used);
INIT_LIST_HEAD(&free);
INIT_LIST_HEAD(&eba_orphans);
- INIT_LIST_HEAD(&ai->corr);
- INIT_LIST_HEAD(&ai->free);
- INIT_LIST_HEAD(&ai->erase);
- INIT_LIST_HEAD(&ai->alien);
- ai->volumes = RB_ROOT;
ai->min_ec = UBI_MAX_ERASECOUNTER;
- ai->aeb_slab_cache = kmem_cache_create("ubi_ainf_peb_slab",
- sizeof(struct ubi_ainf_peb),
- 0, 0, NULL);
- if (!ai->aeb_slab_cache) {
- ret = -ENOMEM;
- goto fail;
- }
-
fmsb = (struct ubi_fm_sb *)(fm_raw);
ai->max_sqnum = fmsb->sqnum;
fm_pos += sizeof(struct ubi_fm_sb);
--
1.8.4.5
There is no need to switch to ro mode if ubi_update_fastmap() fails.
Also get rid of the ifdef.
Signed-off-by: Richard Weinberger <[email protected]>
---
drivers/mtd/ubi/build.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 3405be4..3152331 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -154,23 +154,22 @@ static struct device_attribute dev_mtd_num =
*/
int ubi_volume_notify(struct ubi_device *ubi, struct ubi_volume *vol, int ntype)
{
+ int ret;
struct ubi_notification nt;
ubi_do_get_device_info(ubi, &nt.di);
ubi_do_get_volume_info(ubi, vol, &nt.vi);
-#ifdef CONFIG_MTD_UBI_FASTMAP
switch (ntype) {
case UBI_VOLUME_ADDED:
case UBI_VOLUME_REMOVED:
case UBI_VOLUME_RESIZED:
case UBI_VOLUME_RENAMED:
- if (ubi_update_fastmap(ubi)) {
- ubi_err(ubi, "Unable to update fastmap!");
- ubi_ro_mode(ubi);
- }
+ ret = ubi_update_fastmap(ubi);
+ if (ret)
+ ubi_msg(ubi, "Unable to write a new fastmap: %i", ret);
}
-#endif
+
return blocking_notifier_call_chain(&ubi_notifiers, ntype, &nt);
}
--
1.8.4.5
...such that we can implement NOP variants of some functions.
This will help to reduce fastmap specific ifdefs in other c files.
Signed-off-by: Richard Weinberger <[email protected]>
---
drivers/mtd/ubi/ubi.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index d672412..6fadf34 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -864,10 +864,14 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct ubi_ainf_peb *aeb,
int pnum, const struct ubi_vid_hdr *vid_hdr);
/* fastmap.c */
+#ifdef CONFIG_MTD_UBI_FASTMAP
size_t ubi_calc_fm_size(struct ubi_device *ubi);
int ubi_update_fastmap(struct ubi_device *ubi);
int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
int fm_anchor);
+#else
+static inline int ubi_update_fastmap(struct ubi_device *ubi) { return 0; }
+#endif
/* block.c */
#ifdef CONFIG_MTD_UBI_BLOCK
--
1.8.4.5
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 <[email protected]>
---
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;
+ }
spin_lock(&ubi->wl_lock);
}
--
1.8.4.5
There is no need to allocate new ones every time, we can reuse
the existing ones.
This makes the code cleaner and more easy to follow.
Signed-off-by: Richard Weinberger <[email protected]>
---
drivers/mtd/ubi/fastmap.c | 31 +++++--------------------------
drivers/mtd/ubi/wl.c | 11 +++++++----
2 files changed, 12 insertions(+), 30 deletions(-)
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index db3defd..9507702 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -1446,19 +1446,6 @@ int ubi_update_fastmap(struct ubi_device *ubi)
}
new_fm->used_blocks = ubi->fm_size / ubi->leb_size;
-
- for (i = 0; i < new_fm->used_blocks; i++) {
- new_fm->e[i] = kmem_cache_alloc(ubi_wl_entry_slab, GFP_KERNEL);
- if (!new_fm->e[i]) {
- while (i--)
- kfree(new_fm->e[i]);
-
- kfree(new_fm);
- mutex_unlock(&ubi->fm_mutex);
- return -ENOMEM;
- }
- }
-
old_fm = ubi->fm;
ubi->fm = NULL;
@@ -1494,12 +1481,9 @@ int ubi_update_fastmap(struct ubi_device *ubi)
ubi_err(ubi, "could not erase old fastmap PEB");
goto err;
}
-
- new_fm->e[i]->pnum = old_fm->e[i]->pnum;
- new_fm->e[i]->ec = old_fm->e[i]->ec;
+ new_fm->e[i] = old_fm->e[i];
} else {
- new_fm->e[i]->pnum = tmp_e->pnum;
- new_fm->e[i]->ec = tmp_e->ec;
+ new_fm->e[i] = tmp_e;
if (old_fm)
ubi_wl_put_fm_peb(ubi, old_fm->e[i], i,
@@ -1524,16 +1508,13 @@ int ubi_update_fastmap(struct ubi_device *ubi)
i, 0);
goto err;
}
-
- new_fm->e[0]->pnum = old_fm->e[0]->pnum;
+ new_fm->e[0] = old_fm->e[0];
new_fm->e[0]->ec = ret;
} else {
/* we've got a new anchor PEB, return the old one */
ubi_wl_put_fm_peb(ubi, old_fm->e[0], 0,
old_fm->to_be_tortured[0]);
-
- new_fm->e[0]->pnum = tmp_e->pnum;
- new_fm->e[0]->ec = tmp_e->ec;
+ new_fm->e[0] = tmp_e;
}
} else {
if (!tmp_e) {
@@ -1546,9 +1527,7 @@ int ubi_update_fastmap(struct ubi_device *ubi)
ret = -ENOSPC;
goto err;
}
-
- new_fm->e[0]->pnum = tmp_e->pnum;
- new_fm->e[0]->ec = tmp_e->ec;
+ new_fm->e[0] = tmp_e;
}
down_write(&ubi->work_sem);
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 47b215f..523d8a4 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -1014,9 +1014,6 @@ int ubi_wl_put_fm_peb(struct ubi_device *ubi, struct ubi_wl_entry *fm_e,
e = fm_e;
ubi_assert(e->ec >= 0);
ubi->lookuptbl[pnum] = e;
- } else {
- e->ec = fm_e->ec;
- kfree(fm_e);
}
spin_unlock(&ubi->wl_lock);
@@ -2008,9 +2005,15 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai)
dbg_wl("found %i PEBs", found_pebs);
- if (ubi->fm)
+ if (ubi->fm) {
ubi_assert(ubi->good_peb_count == \
found_pebs + ubi->fm->used_blocks);
+
+ for (i = 0; i < ubi->fm->used_blocks; i++) {
+ e = ubi->fm->e[i];
+ ubi->lookuptbl[e->pnum] = e;
+ }
+ }
else
ubi_assert(ubi->good_peb_count == found_pebs);
--
1.8.4.5
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
> Add a ubi_fastmap_close() to free all resources used by fastmap
> at WL shutdown.
>
> Signed-off-by: Richard Weinberger <[email protected]>
> ---
> drivers/mtd/ubi/wl.c | 21 ++++++++++++++++++---
> 1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
> index c2822f7..47b215f 100644
> --- a/drivers/mtd/ubi/wl.c
> +++ b/drivers/mtd/ubi/wl.c
> @@ -2064,6 +2064,23 @@ static void protection_queue_destroy(struct ubi_device *ubi)
> }
> }
>
> +static void ubi_fastmap_close(struct ubi_device *ubi)
> +{
> +#ifdef CONFIG_MTD_UBI_FASTMAP
> + int i;
> +
> + flush_work(&ubi->fm_work);
> + return_unused_pool_pebs(ubi, &ubi->fm_pool);
> + return_unused_pool_pebs(ubi, &ubi->fm_wl_pool);
> +
> + if (ubi->fm) {
> + for (i = 0; i < ubi->fm->used_blocks; i++)
> + kfree(ubi->fm->e[i]);
> + }
> + kfree(ubi->fm);
kfree(ubi->fm_buf)?
> +#endif
> +}
> +
> /**
> * ubi_wl_close - close the wear-leveling sub-system.
> * @ubi: UBI device description object
> @@ -2071,9 +2088,7 @@ static void protection_queue_destroy(struct ubi_device *ubi)
> void ubi_wl_close(struct ubi_device *ubi)
> {
> dbg_wl("close the WL sub-system");
> -#ifdef CONFIG_MTD_UBI_FASTMAP
> - flush_work(&ubi->fm_work);
> -#endif
> + ubi_fastmap_close(ubi);
> shutdown_work(ubi);
> protection_queue_destroy(ubi);
> tree_destroy(&ubi->used);
>
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Am 07.12.2014 um 09:13 schrieb Tanya Brokhman:
> On 11/30/2014 1:35 PM, Richard Weinberger wrote:
>> Add a ubi_fastmap_close() to free all resources used by fastmap
>> at WL shutdown.
>>
>> Signed-off-by: Richard Weinberger <[email protected]>
>> ---
>> drivers/mtd/ubi/wl.c | 21 ++++++++++++++++++---
>> 1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
>> index c2822f7..47b215f 100644
>> --- a/drivers/mtd/ubi/wl.c
>> +++ b/drivers/mtd/ubi/wl.c
>> @@ -2064,6 +2064,23 @@ static void protection_queue_destroy(struct ubi_device *ubi)
>> }
>> }
>>
>> +static void ubi_fastmap_close(struct ubi_device *ubi)
>> +{
>> +#ifdef CONFIG_MTD_UBI_FASTMAP
>> + int i;
>> +
>> + flush_work(&ubi->fm_work);
>> + return_unused_pool_pebs(ubi, &ubi->fm_pool);
>> + return_unused_pool_pebs(ubi, &ubi->fm_wl_pool);
>> +
>> + if (ubi->fm) {
>> + for (i = 0; i < ubi->fm->used_blocks; i++)
>> + kfree(ubi->fm->e[i]);
>> + }
>> + kfree(ubi->fm);
>
> kfree(ubi->fm_buf)?
No this is not a typo, I kfree() here ubi->fm by design.
What am I missing? :)
Thanks,
//richard
On 12/7/2014 11:54 AM, Richard Weinberger wrote:
> Am 07.12.2014 um 09:13 schrieb Tanya Brokhman:
>> On 11/30/2014 1:35 PM, Richard Weinberger wrote:
>>> Add a ubi_fastmap_close() to free all resources used by fastmap
>>> at WL shutdown.
>>>
>>> Signed-off-by: Richard Weinberger <[email protected]>
>>> ---
>>> drivers/mtd/ubi/wl.c | 21 ++++++++++++++++++---
>>> 1 file changed, 18 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
>>> index c2822f7..47b215f 100644
>>> --- a/drivers/mtd/ubi/wl.c
>>> +++ b/drivers/mtd/ubi/wl.c
>>> @@ -2064,6 +2064,23 @@ static void protection_queue_destroy(struct ubi_device *ubi)
>>> }
>>> }
>>>
>>> +static void ubi_fastmap_close(struct ubi_device *ubi)
>>> +{
>>> +#ifdef CONFIG_MTD_UBI_FASTMAP
>>> + int i;
>>> +
>>> + flush_work(&ubi->fm_work);
>>> + return_unused_pool_pebs(ubi, &ubi->fm_pool);
>>> + return_unused_pool_pebs(ubi, &ubi->fm_wl_pool);
>>> +
>>> + if (ubi->fm) {
>>> + for (i = 0; i < ubi->fm->used_blocks; i++)
>>> + kfree(ubi->fm->e[i]);
>>> + }
>>> + kfree(ubi->fm);
>>
>> kfree(ubi->fm_buf)?
>
> No this is not a typo, I kfree() here ubi->fm by design.
> What am I missing? :)
I think you missed freeing ubi->fm_buf, before (not instead) you free
ubi->fm :)
>
> Thanks,
> //richard
>
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Am 07.12.2014 um 12:32 schrieb Tanya Brokhman:
> On 12/7/2014 11:54 AM, Richard Weinberger wrote:
>> Am 07.12.2014 um 09:13 schrieb Tanya Brokhman:
>>> On 11/30/2014 1:35 PM, Richard Weinberger wrote:
>>>> Add a ubi_fastmap_close() to free all resources used by fastmap
>>>> at WL shutdown.
>>>>
>>>> Signed-off-by: Richard Weinberger <[email protected]>
>>>> ---
>>>> drivers/mtd/ubi/wl.c | 21 ++++++++++++++++++---
>>>> 1 file changed, 18 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
>>>> index c2822f7..47b215f 100644
>>>> --- a/drivers/mtd/ubi/wl.c
>>>> +++ b/drivers/mtd/ubi/wl.c
>>>> @@ -2064,6 +2064,23 @@ static void protection_queue_destroy(struct ubi_device *ubi)
>>>> }
>>>> }
>>>>
>>>> +static void ubi_fastmap_close(struct ubi_device *ubi)
>>>> +{
>>>> +#ifdef CONFIG_MTD_UBI_FASTMAP
>>>> + int i;
>>>> +
>>>> + flush_work(&ubi->fm_work);
>>>> + return_unused_pool_pebs(ubi, &ubi->fm_pool);
>>>> + return_unused_pool_pebs(ubi, &ubi->fm_wl_pool);
>>>> +
>>>> + if (ubi->fm) {
>>>> + for (i = 0; i < ubi->fm->used_blocks; i++)
>>>> + kfree(ubi->fm->e[i]);
>>>> + }
>>>> + kfree(ubi->fm);
>>>
>>> kfree(ubi->fm_buf)?
>>
>> No this is not a typo, I kfree() here ubi->fm by design.
>> What am I missing? :)
>
> I think you missed freeing ubi->fm_buf, before (not instead) you free ubi->fm :)
No. fm_buf is vfree()d upon detach time.
Thanks,
//richard
On 12/7/2014 1:34 PM, Richard Weinberger wrote:
> Am 07.12.2014 um 12:32 schrieb Tanya Brokhman:
>> On 12/7/2014 11:54 AM, Richard Weinberger wrote:
>>> Am 07.12.2014 um 09:13 schrieb Tanya Brokhman:
>>>> On 11/30/2014 1:35 PM, Richard Weinberger wrote:
>>>>> Add a ubi_fastmap_close() to free all resources used by fastmap
>>>>> at WL shutdown.
>>>>>
>>>>> Signed-off-by: Richard Weinberger <[email protected]>
>>>>> ---
>>>>> drivers/mtd/ubi/wl.c | 21 ++++++++++++++++++---
>>>>> 1 file changed, 18 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
>>>>> index c2822f7..47b215f 100644
>>>>> --- a/drivers/mtd/ubi/wl.c
>>>>> +++ b/drivers/mtd/ubi/wl.c
>>>>> @@ -2064,6 +2064,23 @@ static void protection_queue_destroy(struct ubi_device *ubi)
>>>>> }
>>>>> }
>>>>>
>>>>> +static void ubi_fastmap_close(struct ubi_device *ubi)
>>>>> +{
>>>>> +#ifdef CONFIG_MTD_UBI_FASTMAP
>>>>> + int i;
>>>>> +
>>>>> + flush_work(&ubi->fm_work);
>>>>> + return_unused_pool_pebs(ubi, &ubi->fm_pool);
>>>>> + return_unused_pool_pebs(ubi, &ubi->fm_wl_pool);
>>>>> +
>>>>> + if (ubi->fm) {
>>>>> + for (i = 0; i < ubi->fm->used_blocks; i++)
>>>>> + kfree(ubi->fm->e[i]);
>>>>> + }
>>>>> + kfree(ubi->fm);
>>>>
>>>> kfree(ubi->fm_buf)?
>>>
>>> No this is not a typo, I kfree() here ubi->fm by design.
>>> What am I missing? :)
>>
>> I think you missed freeing ubi->fm_buf, before (not instead) you free ubi->fm :)
>
> No. fm_buf is vfree()d upon detach time.
you're right, found it at ubi_detach_mtd_dev() several lines after
calling ubi_wl_close(). But if you're creating a fastmap-close dedicated
function, fm_buf should be freed in it as it is fastmap related.
>
> Thanks,
> //richard
>
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Am 07.12.2014 um 14:26 schrieb Tanya Brokhman:
> you're right, found it at ubi_detach_mtd_dev() several lines after calling ubi_wl_close(). But if you're creating a fastmap-close dedicated function, fm_buf should be freed in it
> as it is fastmap related.
The idea was to have WL related stuff in this function.
But I can add more into it in a subsequent patch.
Thanks,
//richard
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
> There is no need to allocate new ones every time, we can reuse
> the existing ones.
> This makes the code cleaner and more easy to follow.
>
> Signed-off-by: Richard Weinberger <[email protected]>
Reviewed-by: Tanya Brokhman <[email protected]>
> ---
> drivers/mtd/ubi/fastmap.c | 31 +++++--------------------------
> drivers/mtd/ubi/wl.c | 11 +++++++----
> 2 files changed, 12 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index db3defd..9507702 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -1446,19 +1446,6 @@ int ubi_update_fastmap(struct ubi_device *ubi)
> }
>
> new_fm->used_blocks = ubi->fm_size / ubi->leb_size;
> -
> - for (i = 0; i < new_fm->used_blocks; i++) {
> - new_fm->e[i] = kmem_cache_alloc(ubi_wl_entry_slab, GFP_KERNEL);
> - if (!new_fm->e[i]) {
> - while (i--)
> - kfree(new_fm->e[i]);
> -
> - kfree(new_fm);
> - mutex_unlock(&ubi->fm_mutex);
> - return -ENOMEM;
> - }
> - }
> -
> old_fm = ubi->fm;
> ubi->fm = NULL;
>
> @@ -1494,12 +1481,9 @@ int ubi_update_fastmap(struct ubi_device *ubi)
> ubi_err(ubi, "could not erase old fastmap PEB");
> goto err;
> }
> -
> - new_fm->e[i]->pnum = old_fm->e[i]->pnum;
> - new_fm->e[i]->ec = old_fm->e[i]->ec;
> + new_fm->e[i] = old_fm->e[i];
> } else {
> - new_fm->e[i]->pnum = tmp_e->pnum;
> - new_fm->e[i]->ec = tmp_e->ec;
> + new_fm->e[i] = tmp_e;
>
> if (old_fm)
> ubi_wl_put_fm_peb(ubi, old_fm->e[i], i,
> @@ -1524,16 +1508,13 @@ int ubi_update_fastmap(struct ubi_device *ubi)
> i, 0);
> goto err;
> }
> -
> - new_fm->e[0]->pnum = old_fm->e[0]->pnum;
> + new_fm->e[0] = old_fm->e[0];
> new_fm->e[0]->ec = ret;
> } else {
> /* we've got a new anchor PEB, return the old one */
> ubi_wl_put_fm_peb(ubi, old_fm->e[0], 0,
> old_fm->to_be_tortured[0]);
> -
> - new_fm->e[0]->pnum = tmp_e->pnum;
> - new_fm->e[0]->ec = tmp_e->ec;
> + new_fm->e[0] = tmp_e;
> }
> } else {
> if (!tmp_e) {
> @@ -1546,9 +1527,7 @@ int ubi_update_fastmap(struct ubi_device *ubi)
> ret = -ENOSPC;
> goto err;
> }
> -
> - new_fm->e[0]->pnum = tmp_e->pnum;
> - new_fm->e[0]->ec = tmp_e->ec;
> + new_fm->e[0] = tmp_e;
> }
>
> down_write(&ubi->work_sem);
> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
> index 47b215f..523d8a4 100644
> --- a/drivers/mtd/ubi/wl.c
> +++ b/drivers/mtd/ubi/wl.c
> @@ -1014,9 +1014,6 @@ int ubi_wl_put_fm_peb(struct ubi_device *ubi, struct ubi_wl_entry *fm_e,
> e = fm_e;
> ubi_assert(e->ec >= 0);
> ubi->lookuptbl[pnum] = e;
> - } else {
> - e->ec = fm_e->ec;
> - kfree(fm_e);
> }
>
> spin_unlock(&ubi->wl_lock);
> @@ -2008,9 +2005,15 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai)
>
> dbg_wl("found %i PEBs", found_pebs);
>
> - if (ubi->fm)
> + if (ubi->fm) {
> ubi_assert(ubi->good_peb_count == \
> found_pebs + ubi->fm->used_blocks);
> +
> + for (i = 0; i < ubi->fm->used_blocks; i++) {
> + e = ubi->fm->e[i];
> + ubi->lookuptbl[e->pnum] = e;
> + }
> + }
> else
> ubi_assert(ubi->good_peb_count == found_pebs);
>
>
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
> ---
> 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
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
> ...such that we can implement NOP variants of some functions.
> This will help to reduce fastmap specific ifdefs in other c files.
>
> Signed-off-by: Richard Weinberger <[email protected]>
Reviewed-by: Tanya Brokhman <[email protected]>
> ---
> drivers/mtd/ubi/ubi.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
> index d672412..6fadf34 100644
> --- a/drivers/mtd/ubi/ubi.h
> +++ b/drivers/mtd/ubi/ubi.h
> @@ -864,10 +864,14 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct ubi_ainf_peb *aeb,
> int pnum, const struct ubi_vid_hdr *vid_hdr);
>
> /* fastmap.c */
> +#ifdef CONFIG_MTD_UBI_FASTMAP
> size_t ubi_calc_fm_size(struct ubi_device *ubi);
> int ubi_update_fastmap(struct ubi_device *ubi);
> int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
> int fm_anchor);
> +#else
> +static inline int ubi_update_fastmap(struct ubi_device *ubi) { return 0; }
> +#endif
>
> /* block.c */
> #ifdef CONFIG_MTD_UBI_BLOCK
>
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
> There is no need to switch to ro mode if ubi_update_fastmap() fails.
> Also get rid of the ifdef.
>
> Signed-off-by: Richard Weinberger <[email protected]>
Reviewed-by: Tanya Brokhman <[email protected]>
> ---
> drivers/mtd/ubi/build.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index 3405be4..3152331 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -154,23 +154,22 @@ static struct device_attribute dev_mtd_num =
> */
> int ubi_volume_notify(struct ubi_device *ubi, struct ubi_volume *vol, int ntype)
> {
> + int ret;
> struct ubi_notification nt;
>
> ubi_do_get_device_info(ubi, &nt.di);
> ubi_do_get_volume_info(ubi, vol, &nt.vi);
>
> -#ifdef CONFIG_MTD_UBI_FASTMAP
> switch (ntype) {
> case UBI_VOLUME_ADDED:
> case UBI_VOLUME_REMOVED:
> case UBI_VOLUME_RESIZED:
> case UBI_VOLUME_RENAMED:
> - if (ubi_update_fastmap(ubi)) {
> - ubi_err(ubi, "Unable to update fastmap!");
> - ubi_ro_mode(ubi);
> - }
> + ret = ubi_update_fastmap(ubi);
> + if (ret)
> + ubi_msg(ubi, "Unable to write a new fastmap: %i", ret);
> }
> -#endif
> +
> return blocking_notifier_call_chain(&ubi_notifiers, ntype, &nt);
> }
>
>
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
>> ---
>> 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 <[email protected]>
>>> ---
>>> 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
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
> There is no need to allocate new ones every time, we can reuse
> the existing ones.
> This makes the code cleaner and more easy to follow.
>
> Signed-off-by: Richard Weinberger <[email protected]>
> ---
> drivers/mtd/ubi/fastmap.c | 31 +++++--------------------------
> drivers/mtd/ubi/wl.c | 11 +++++++----
> 2 files changed, 12 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index db3defd..9507702 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -1446,19 +1446,6 @@ int ubi_update_fastmap(struct ubi_device *ubi)
> }
>
> new_fm->used_blocks = ubi->fm_size / ubi->leb_size;
Not related to this patch, but looking at this function it got me
thinking: why do we need to re-calculate new_fm->used_blocks (and check
calculated value) each time? fm_size doesn't changed at runtime.
leb_size sure does not, so fm->used_blocks can be ubi device parameter
and calculated & tested only once, and not each time we write fastmap.
Correct?
> -
> - for (i = 0; i < new_fm->used_blocks; i++) {
> - new_fm->e[i] = kmem_cache_alloc(ubi_wl_entry_slab, GFP_KERNEL);
> - if (!new_fm->e[i]) {
> - while (i--)
> - kfree(new_fm->e[i]);
> -
> - kfree(new_fm);
> - mutex_unlock(&ubi->fm_mutex);
> - return -ENOMEM;
> - }
> - }
> -
> old_fm = ubi->fm;
> ubi->fm = NULL;
>
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
>>>> ---
>>>> 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
Am 08.12.2014 um 09:36 schrieb Tanya Brokhman:
> On 11/30/2014 1:35 PM, Richard Weinberger wrote:
>> There is no need to allocate new ones every time, we can reuse
>> the existing ones.
>> This makes the code cleaner and more easy to follow.
>>
>> Signed-off-by: Richard Weinberger <[email protected]>
>> ---
>> drivers/mtd/ubi/fastmap.c | 31 +++++--------------------------
>> drivers/mtd/ubi/wl.c | 11 +++++++----
>> 2 files changed, 12 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
>> index db3defd..9507702 100644
>> --- a/drivers/mtd/ubi/fastmap.c
>> +++ b/drivers/mtd/ubi/fastmap.c
>> @@ -1446,19 +1446,6 @@ int ubi_update_fastmap(struct ubi_device *ubi)
>> }
>>
>> new_fm->used_blocks = ubi->fm_size / ubi->leb_size;
>
> Not related to this patch, but looking at this function it got me thinking: why do we need to re-calculate new_fm->used_blocks (and check calculated value) each time? fm_size
> doesn't changed at runtime. leb_size sure does not, so fm->used_blocks can be ubi device parameter and calculated & tested only once, and not each time we write fastmap. Correct?
If you dig deeper into the patch set you'll notice that the fastmap size can change. :-)
Thanks,
//richard
On 12/8/2014 11:14 AM, Richard Weinberger wrote:
> Am 08.12.2014 um 09:36 schrieb Tanya Brokhman:
>> On 11/30/2014 1:35 PM, Richard Weinberger wrote:
>>> There is no need to allocate new ones every time, we can reuse
>>> the existing ones.
>>> This makes the code cleaner and more easy to follow.
>>>
>>> Signed-off-by: Richard Weinberger <[email protected]>
>>> ---
>>> drivers/mtd/ubi/fastmap.c | 31 +++++--------------------------
>>> drivers/mtd/ubi/wl.c | 11 +++++++----
>>> 2 files changed, 12 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
>>> index db3defd..9507702 100644
>>> --- a/drivers/mtd/ubi/fastmap.c
>>> +++ b/drivers/mtd/ubi/fastmap.c
>>> @@ -1446,19 +1446,6 @@ int ubi_update_fastmap(struct ubi_device *ubi)
>>> }
>>>
>>> new_fm->used_blocks = ubi->fm_size / ubi->leb_size;
>>
>> Not related to this patch, but looking at this function it got me thinking: why do we need to re-calculate new_fm->used_blocks (and check calculated value) each time? fm_size
>> doesn't changed at runtime. leb_size sure does not, so fm->used_blocks can be ubi device parameter and calculated & tested only once, and not each time we write fastmap. Correct?
>
> If you dig deeper into the patch set you'll notice that the fastmap size can change. :-)
>
oh, ok. sorry, still in the middle of the review.
> Thanks,
> //richard
>
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Am 08.12.2014 um 10:37 schrieb Tanya Brokhman:
> On 12/8/2014 11:14 AM, Richard Weinberger wrote:
>> Am 08.12.2014 um 09:36 schrieb Tanya Brokhman:
>>> On 11/30/2014 1:35 PM, Richard Weinberger wrote:
>>>> There is no need to allocate new ones every time, we can reuse
>>>> the existing ones.
>>>> This makes the code cleaner and more easy to follow.
>>>>
>>>> Signed-off-by: Richard Weinberger <[email protected]>
>>>> ---
>>>> drivers/mtd/ubi/fastmap.c | 31 +++++--------------------------
>>>> drivers/mtd/ubi/wl.c | 11 +++++++----
>>>> 2 files changed, 12 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
>>>> index db3defd..9507702 100644
>>>> --- a/drivers/mtd/ubi/fastmap.c
>>>> +++ b/drivers/mtd/ubi/fastmap.c
>>>> @@ -1446,19 +1446,6 @@ int ubi_update_fastmap(struct ubi_device *ubi)
>>>> }
>>>>
>>>> new_fm->used_blocks = ubi->fm_size / ubi->leb_size;
>>>
>>> Not related to this patch, but looking at this function it got me thinking: why do we need to re-calculate new_fm->used_blocks (and check calculated value) each time? fm_size
>>> doesn't changed at runtime. leb_size sure does not, so fm->used_blocks can be ubi device parameter and calculated & tested only once, and not each time we write fastmap. Correct?
>>
>> If you dig deeper into the patch set you'll notice that the fastmap size can change. :-)
>>
>
> oh, ok. sorry, still in the middle of the review.
This is perfectly fine.
I really a appreciate your reviews!
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 <[email protected]>
>>>>> ---
>>>>> 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
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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
On Sun, Nov 30, 2014 at 12:35:35PM +0100, Richard Weinberger wrote:
> Add a ubi_fastmap_close() to free all resources used by fastmap
> at WL shutdown.
>
> Signed-off-by: Richard Weinberger <[email protected]>
Either for this patch, or for a v3 if you move the
-flush_work/+ubi_fastmap_close to shutdown_work:
Tested-by: Guido Mart?nez <[email protected]>
Reviewed-by: Guido Mart?nez <[email protected]>
> ---
> drivers/mtd/ubi/wl.c | 21 ++++++++++++++++++---
> 1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
> index c2822f7..47b215f 100644
> --- a/drivers/mtd/ubi/wl.c
> +++ b/drivers/mtd/ubi/wl.c
> @@ -2064,6 +2064,23 @@ static void protection_queue_destroy(struct ubi_device *ubi)
> }
> }
>
> +static void ubi_fastmap_close(struct ubi_device *ubi)
> +{
> +#ifdef CONFIG_MTD_UBI_FASTMAP
> + int i;
> +
> + flush_work(&ubi->fm_work);
> + return_unused_pool_pebs(ubi, &ubi->fm_pool);
> + return_unused_pool_pebs(ubi, &ubi->fm_wl_pool);
> +
> + if (ubi->fm) {
> + for (i = 0; i < ubi->fm->used_blocks; i++)
> + kfree(ubi->fm->e[i]);
> + }
> + kfree(ubi->fm);
> +#endif
> +}
> +
> /**
> * ubi_wl_close - close the wear-leveling sub-system.
> * @ubi: UBI device description object
> @@ -2071,9 +2088,7 @@ static void protection_queue_destroy(struct ubi_device *ubi)
> void ubi_wl_close(struct ubi_device *ubi)
> {
> dbg_wl("close the WL sub-system");
> -#ifdef CONFIG_MTD_UBI_FASTMAP
> - flush_work(&ubi->fm_work);
> -#endif
> + ubi_fastmap_close(ubi);
> shutdown_work(ubi);
> protection_queue_destroy(ubi);
> tree_destroy(&ubi->used);
--
Guido Mart?nez, VanguardiaSur
http://www.vanguardiasur.com.ar
Hi Richard,
On Sun, Nov 30, 2014 at 12:35:36PM +0100, Richard Weinberger wrote:
> There is no need to allocate new ones every time, we can reuse
> the existing ones.
> This makes the code cleaner and more easy to follow.
>
> Signed-off-by: Richard Weinberger <[email protected]>
> ---
> drivers/mtd/ubi/fastmap.c | 31 +++++--------------------------
> drivers/mtd/ubi/wl.c | 11 +++++++----
> 2 files changed, 12 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index db3defd..9507702 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -1446,19 +1446,6 @@ int ubi_update_fastmap(struct ubi_device *ubi)
> }
>
> new_fm->used_blocks = ubi->fm_size / ubi->leb_size;
> -
> - for (i = 0; i < new_fm->used_blocks; i++) {
> - new_fm->e[i] = kmem_cache_alloc(ubi_wl_entry_slab, GFP_KERNEL);
> - if (!new_fm->e[i]) {
> - while (i--)
> - kfree(new_fm->e[i]);
> -
> - kfree(new_fm);
> - mutex_unlock(&ubi->fm_mutex);
> - return -ENOMEM;
> - }
> - }
> -
> old_fm = ubi->fm;
> ubi->fm = NULL;
>
> @@ -1494,12 +1481,9 @@ int ubi_update_fastmap(struct ubi_device *ubi)
> ubi_err(ubi, "could not erase old fastmap PEB");
> goto err;
> }
> -
> - new_fm->e[i]->pnum = old_fm->e[i]->pnum;
> - new_fm->e[i]->ec = old_fm->e[i]->ec;
> + new_fm->e[i] = old_fm->e[i];
> } else {
> - new_fm->e[i]->pnum = tmp_e->pnum;
> - new_fm->e[i]->ec = tmp_e->ec;
> + new_fm->e[i] = tmp_e;
>
> if (old_fm)
> ubi_wl_put_fm_peb(ubi, old_fm->e[i], i,
> @@ -1524,16 +1508,13 @@ int ubi_update_fastmap(struct ubi_device *ubi)
> i, 0);
> goto err;
> }
> -
> - new_fm->e[0]->pnum = old_fm->e[0]->pnum;
> + new_fm->e[0] = old_fm->e[0];
> new_fm->e[0]->ec = ret;
> } else {
> /* we've got a new anchor PEB, return the old one */
> ubi_wl_put_fm_peb(ubi, old_fm->e[0], 0,
> old_fm->to_be_tortured[0]);
> -
> - new_fm->e[0]->pnum = tmp_e->pnum;
> - new_fm->e[0]->ec = tmp_e->ec;
> + new_fm->e[0] = tmp_e;
> }
> } else {
> if (!tmp_e) {
> @@ -1546,9 +1527,7 @@ int ubi_update_fastmap(struct ubi_device *ubi)
> ret = -ENOSPC;
> goto err;
> }
> -
> - new_fm->e[0]->pnum = tmp_e->pnum;
> - new_fm->e[0]->ec = tmp_e->ec;
> + new_fm->e[0] = tmp_e;
> }
>
> down_write(&ubi->work_sem);
> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
> index 47b215f..523d8a4 100644
> --- a/drivers/mtd/ubi/wl.c
> +++ b/drivers/mtd/ubi/wl.c
> @@ -1014,9 +1014,6 @@ int ubi_wl_put_fm_peb(struct ubi_device *ubi, struct ubi_wl_entry *fm_e,
> e = fm_e;
> ubi_assert(e->ec >= 0);
> ubi->lookuptbl[pnum] = e;
> - } else {
> - e->ec = fm_e->ec;
> - kfree(fm_e);
> }
>
> spin_unlock(&ubi->wl_lock);
> @@ -2008,9 +2005,15 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai)
>
> dbg_wl("found %i PEBs", found_pebs);
>
> - if (ubi->fm)
> + if (ubi->fm) {
> ubi_assert(ubi->good_peb_count == \
> found_pebs + ubi->fm->used_blocks);
> +
> + for (i = 0; i < ubi->fm->used_blocks; i++) {
> + e = ubi->fm->e[i];
> + ubi->lookuptbl[e->pnum] = e;
> + }
> + }
Should this be in a separate patch? The commit log doesn't mention it.
Looks good otherwise!
> else
> ubi_assert(ubi->good_peb_count == found_pebs);
--
Guido Mart?nez, VanguardiaSur
http://www.vanguardiasur.com.ar
Guido,
Am 18.12.2014 um 02:18 schrieb Guido Mart?nez:
> Hi Richard,
>
> On Sun, Nov 30, 2014 at 12:35:36PM +0100, Richard Weinberger wrote:
>> There is no need to allocate new ones every time, we can reuse
>> the existing ones.
>> This makes the code cleaner and more easy to follow.
>>
>> Signed-off-by: Richard Weinberger <[email protected]>
>> ---
>> drivers/mtd/ubi/fastmap.c | 31 +++++--------------------------
>> drivers/mtd/ubi/wl.c | 11 +++++++----
>> 2 files changed, 12 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
>> index db3defd..9507702 100644
>> --- a/drivers/mtd/ubi/fastmap.c
>> +++ b/drivers/mtd/ubi/fastmap.c
>> @@ -1446,19 +1446,6 @@ int ubi_update_fastmap(struct ubi_device *ubi)
>> }
>>
>> new_fm->used_blocks = ubi->fm_size / ubi->leb_size;
>> -
>> - for (i = 0; i < new_fm->used_blocks; i++) {
>> - new_fm->e[i] = kmem_cache_alloc(ubi_wl_entry_slab, GFP_KERNEL);
>> - if (!new_fm->e[i]) {
>> - while (i--)
>> - kfree(new_fm->e[i]);
>> -
>> - kfree(new_fm);
>> - mutex_unlock(&ubi->fm_mutex);
>> - return -ENOMEM;
>> - }
>> - }
>> -
>> old_fm = ubi->fm;
>> ubi->fm = NULL;
>>
>> @@ -1494,12 +1481,9 @@ int ubi_update_fastmap(struct ubi_device *ubi)
>> ubi_err(ubi, "could not erase old fastmap PEB");
>> goto err;
>> }
>> -
>> - new_fm->e[i]->pnum = old_fm->e[i]->pnum;
>> - new_fm->e[i]->ec = old_fm->e[i]->ec;
>> + new_fm->e[i] = old_fm->e[i];
>> } else {
>> - new_fm->e[i]->pnum = tmp_e->pnum;
>> - new_fm->e[i]->ec = tmp_e->ec;
>> + new_fm->e[i] = tmp_e;
>>
>> if (old_fm)
>> ubi_wl_put_fm_peb(ubi, old_fm->e[i], i,
>> @@ -1524,16 +1508,13 @@ int ubi_update_fastmap(struct ubi_device *ubi)
>> i, 0);
>> goto err;
>> }
>> -
>> - new_fm->e[0]->pnum = old_fm->e[0]->pnum;
>> + new_fm->e[0] = old_fm->e[0];
>> new_fm->e[0]->ec = ret;
>> } else {
>> /* we've got a new anchor PEB, return the old one */
>> ubi_wl_put_fm_peb(ubi, old_fm->e[0], 0,
>> old_fm->to_be_tortured[0]);
>> -
>> - new_fm->e[0]->pnum = tmp_e->pnum;
>> - new_fm->e[0]->ec = tmp_e->ec;
>> + new_fm->e[0] = tmp_e;
>> }
>> } else {
>> if (!tmp_e) {
>> @@ -1546,9 +1527,7 @@ int ubi_update_fastmap(struct ubi_device *ubi)
>> ret = -ENOSPC;
>> goto err;
>> }
>> -
>> - new_fm->e[0]->pnum = tmp_e->pnum;
>> - new_fm->e[0]->ec = tmp_e->ec;
>> + new_fm->e[0] = tmp_e;
>> }
>>
>> down_write(&ubi->work_sem);
>> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
>> index 47b215f..523d8a4 100644
>> --- a/drivers/mtd/ubi/wl.c
>> +++ b/drivers/mtd/ubi/wl.c
>> @@ -1014,9 +1014,6 @@ int ubi_wl_put_fm_peb(struct ubi_device *ubi, struct ubi_wl_entry *fm_e,
>> e = fm_e;
>> ubi_assert(e->ec >= 0);
>> ubi->lookuptbl[pnum] = e;
>> - } else {
>> - e->ec = fm_e->ec;
>> - kfree(fm_e);
>> }
>>
>> spin_unlock(&ubi->wl_lock);
>> @@ -2008,9 +2005,15 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai)
>>
>> dbg_wl("found %i PEBs", found_pebs);
>>
>> - if (ubi->fm)
>> + if (ubi->fm) {
>> ubi_assert(ubi->good_peb_count == \
>> found_pebs + ubi->fm->used_blocks);
>> +
>> + for (i = 0; i < ubi->fm->used_blocks; i++) {
>> + e = ubi->fm->e[i];
>> + ubi->lookuptbl[e->pnum] = e;
>> + }
>> + }
> Should this be in a separate patch? The commit log doesn't mention it.
Hmm, looks like a fragment from the memleak fix.
I've split up a lot of patches, maybe some hunks sneaked into other patches.
Anyway, will fixup!
Thanks a lot for reviewing!
//richard