* [PATCH 1/3] mtd: ubi: fixup error correction in do_sync_erase()
@ 2015-11-26 20:23 Sebastian Andrzej Siewior
2015-11-26 20:23 ` [PATCH 2/3] mtd: ubi: don't leak e if schedule_erase() fails Sebastian Andrzej Siewior
2015-11-26 20:23 ` [PATCH 3/3] mtd: ubi: wl: avoid erasing a PEB which is empty Sebastian Andrzej Siewior
0 siblings, 2 replies; 5+ messages in thread
From: Sebastian Andrzej Siewior @ 2015-11-26 20:23 UTC (permalink / raw)
To: Artem Bityutskiy
Cc: Richard Weinberger, linux-mtd, tglx, Sebastian Andrzej Siewior
Since fastmap we gained do_sync_erase(). This function can return an error
and its error handling isn't obvious. First the memory allocation for
struct ubi_work can fail and as such struct ubi_wl_entry is leaked.
However if the memory allocation succeeds then the tail function takes
care of the struct ubi_wl_entry. A free here could result in a double
free.
To make the error handling simpler, I split the tail function into one
piece which does the work and another which frees the struct ubi_work
which is passed as argument. As result do_sync_erase() can keep the
struct on stack and we get rid of one error source.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
drivers/mtd/ubi/wl.c | 52 ++++++++++++++++++++++++++++------------------------
1 file changed, 28 insertions(+), 24 deletions(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index eb4489f9082f..f73233fa737c 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -603,6 +603,7 @@ static int schedule_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
return 0;
}
+static int __erase_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk);
/**
* do_sync_erase - run the erase worker synchronously.
* @ubi: UBI device description object
@@ -615,20 +616,16 @@ static int schedule_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
static int do_sync_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
int vol_id, int lnum, int torture)
{
- struct ubi_work *wl_wrk;
+ struct ubi_work wl_wrk;
dbg_wl("sync erase of PEB %i", e->pnum);
- wl_wrk = kmalloc(sizeof(struct ubi_work), GFP_NOFS);
- if (!wl_wrk)
- return -ENOMEM;
+ wl_wrk.e = e;
+ wl_wrk.vol_id = vol_id;
+ wl_wrk.lnum = lnum;
+ wl_wrk.torture = torture;
- wl_wrk->e = e;
- wl_wrk->vol_id = vol_id;
- wl_wrk->lnum = lnum;
- wl_wrk->torture = torture;
-
- return erase_worker(ubi, wl_wrk, 0);
+ return __erase_worker(ubi, &wl_wrk);
}
/**
@@ -1014,7 +1011,7 @@ static int ensure_wear_leveling(struct ubi_device *ubi, int nested)
}
/**
- * erase_worker - physical eraseblock erase worker function.
+ * __erase_worker - physical eraseblock erase worker function.
* @ubi: UBI device description object
* @wl_wrk: the work object
* @shutdown: non-zero if the worker has to free memory and exit
@@ -1025,8 +1022,7 @@ static int ensure_wear_leveling(struct ubi_device *ubi, int nested)
* needed. Returns zero in case of success and a negative error code in case of
* failure.
*/
-static int erase_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk,
- int shutdown)
+static int __erase_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk)
{
struct ubi_wl_entry *e = wl_wrk->e;
int pnum = e->pnum;
@@ -1034,21 +1030,11 @@ static int erase_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk,
int lnum = wl_wrk->lnum;
int err, available_consumed = 0;
- if (shutdown) {
- dbg_wl("cancel erasure of PEB %d EC %d", pnum, e->ec);
- kfree(wl_wrk);
- wl_entry_destroy(ubi, e);
- return 0;
- }
-
dbg_wl("erase PEB %d EC %d LEB %d:%d",
pnum, e->ec, wl_wrk->vol_id, wl_wrk->lnum);
err = sync_erase(ubi, e, wl_wrk->torture);
if (!err) {
- /* Fine, we've erased it successfully */
- kfree(wl_wrk);
-
spin_lock(&ubi->wl_lock);
wl_tree_add(e, &ubi->free);
ubi->free_count++;
@@ -1066,7 +1052,6 @@ static int erase_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk,
}
ubi_err(ubi, "failed to erase PEB %d, error %d", pnum, err);
- kfree(wl_wrk);
if (err == -EINTR || err == -ENOMEM || err == -EAGAIN ||
err == -EBUSY) {
@@ -1150,6 +1135,25 @@ static int erase_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk,
return err;
}
+static int erase_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk,
+ int shutdown)
+{
+ int ret;
+
+ if (shutdown) {
+ struct ubi_wl_entry *e = wl_wrk->e;
+
+ dbg_wl("cancel erasure of PEB %d EC %d", e->pnum, e->ec);
+ kfree(wl_wrk);
+ wl_entry_destroy(ubi, e);
+ return 0;
+ }
+
+ ret = __erase_worker(ubi, wl_wrk);
+ kfree(wl_wrk);
+ return ret;
+}
+
/**
* ubi_wl_put_peb - return a PEB to the wear-leveling sub-system.
* @ubi: UBI device description object
--
2.6.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/3] mtd: ubi: don't leak e if schedule_erase() fails
2015-11-26 20:23 [PATCH 1/3] mtd: ubi: fixup error correction in do_sync_erase() Sebastian Andrzej Siewior
@ 2015-11-26 20:23 ` Sebastian Andrzej Siewior
2015-11-26 20:23 ` [PATCH 3/3] mtd: ubi: wl: avoid erasing a PEB which is empty Sebastian Andrzej Siewior
1 sibling, 0 replies; 5+ messages in thread
From: Sebastian Andrzej Siewior @ 2015-11-26 20:23 UTC (permalink / raw)
To: Artem Bityutskiy
Cc: Richard Weinberger, linux-mtd, tglx, Sebastian Andrzej Siewior
If __erase_worker() fails to erase the EB and schedule_erase() fails as
well to do anything about it then we go RO. But that is not a reason to
leak the e argument here. Therefore clean up e.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
drivers/mtd/ubi/wl.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index f73233fa737c..56065632a5b8 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -1060,6 +1060,7 @@ static int __erase_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk)
/* Re-schedule the LEB for erasure */
err1 = schedule_erase(ubi, e, vol_id, lnum, 0);
if (err1) {
+ wl_entry_destroy(ubi, e);
err = err1;
goto out_ro;
}
--
2.6.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/3] mtd: ubi: wl: avoid erasing a PEB which is empty
2015-11-26 20:23 [PATCH 1/3] mtd: ubi: fixup error correction in do_sync_erase() Sebastian Andrzej Siewior
2015-11-26 20:23 ` [PATCH 2/3] mtd: ubi: don't leak e if schedule_erase() fails Sebastian Andrzej Siewior
@ 2015-11-26 20:23 ` Sebastian Andrzej Siewior
2015-12-01 19:46 ` Richard Weinberger
1 sibling, 1 reply; 5+ messages in thread
From: Sebastian Andrzej Siewior @ 2015-11-26 20:23 UTC (permalink / raw)
To: Artem Bityutskiy
Cc: Richard Weinberger, linux-mtd, tglx, Sebastian Andrzej Siewior
wear_leveling_worker() currently unconditionally puts a PEB on erase in
the error case even it just been taken from the free_list and never
used.
In case the PEB was never used it can be put back on the free list
saving a precious erase cycle.
v1…v2:
- to_leb_clean -> dst_leb_clean
- use the nested option for ensure_wear_leveling()
- do_sync_erase() can't go -ENOMEM so we can just go into
RO-mode now.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
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 56065632a5b8..17ec948ac40e 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -628,6 +628,7 @@ static int do_sync_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
return __erase_worker(ubi, &wl_wrk);
}
+static int ensure_wear_leveling(struct ubi_device *ubi, int nested);
/**
* wear_leveling_worker - wear-leveling worker function.
* @ubi: UBI device description object
@@ -649,6 +650,7 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk,
#endif
struct ubi_wl_entry *e1, *e2;
struct ubi_vid_hdr *vid_hdr;
+ int dst_leb_clean = 0;
kfree(wrk);
if (shutdown)
@@ -753,6 +755,7 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk,
err = ubi_io_read_vid_hdr(ubi, e1->pnum, vid_hdr, 0);
if (err && err != UBI_IO_BITFLIPS) {
+ dst_leb_clean = 1;
if (err == UBI_IO_FF) {
/*
* We are trying to move PEB without a VID header. UBI
@@ -798,10 +801,12 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk,
* protection queue.
*/
protect = 1;
+ dst_leb_clean = 1;
goto out_not_moved;
}
if (err == MOVE_RETRY) {
scrubbing = 1;
+ dst_leb_clean = 1;
goto out_not_moved;
}
if (err == MOVE_TARGET_BITFLIPS || err == MOVE_TARGET_WR_ERR ||
@@ -827,6 +832,7 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk,
ubi->erroneous_peb_count);
goto out_error;
}
+ dst_leb_clean = 1;
erroneous = 1;
goto out_not_moved;
}
@@ -897,15 +903,24 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk,
wl_tree_add(e1, &ubi->scrub);
else
wl_tree_add(e1, &ubi->used);
+ if (dst_leb_clean) {
+ wl_tree_add(e2, &ubi->free);
+ ubi->free_count++;
+ }
+
ubi_assert(!ubi->move_to_put);
ubi->move_from = ubi->move_to = NULL;
ubi->wl_scheduled = 0;
spin_unlock(&ubi->wl_lock);
ubi_free_vid_hdr(ubi, vid_hdr);
- err = do_sync_erase(ubi, e2, vol_id, lnum, torture);
- if (err)
- goto out_ro;
+ if (dst_leb_clean) {
+ ensure_wear_leveling(ubi, 1);
+ } else {
+ err = do_sync_erase(ubi, e2, vol_id, lnum, torture);
+ if (err)
+ goto out_ro;
+ }
mutex_unlock(&ubi->move_mutex);
return 0;
--
2.6.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] mtd: ubi: wl: avoid erasing a PEB which is empty
2015-11-26 20:23 ` [PATCH 3/3] mtd: ubi: wl: avoid erasing a PEB which is empty Sebastian Andrzej Siewior
@ 2015-12-01 19:46 ` Richard Weinberger
2015-12-02 11:56 ` Artem Bityutskiy
0 siblings, 1 reply; 5+ messages in thread
From: Richard Weinberger @ 2015-12-01 19:46 UTC (permalink / raw)
To: Sebastian Andrzej Siewior, Artem Bityutskiy; +Cc: linux-mtd, tglx
Am 26.11.2015 um 21:23 schrieb Sebastian Andrzej Siewior:
> wear_leveling_worker() currently unconditionally puts a PEB on erase in
> the error case even it just been taken from the free_list and never
> used.
> In case the PEB was never used it can be put back on the free list
> saving a precious erase cycle.
>
> v1…v2:
> - to_leb_clean -> dst_leb_clean
> - use the nested option for ensure_wear_leveling()
> - do_sync_erase() can't go -ENOMEM so we can just go into
> RO-mode now.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Thanks a lot for the error handling cleanup and this improvement!
All three patches look good to me.
Artem, unless you have objections I'd merge them.
Thanks,
//richard
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] mtd: ubi: wl: avoid erasing a PEB which is empty
2015-12-01 19:46 ` Richard Weinberger
@ 2015-12-02 11:56 ` Artem Bityutskiy
0 siblings, 0 replies; 5+ messages in thread
From: Artem Bityutskiy @ 2015-12-02 11:56 UTC (permalink / raw)
To: Richard Weinberger, Sebastian Andrzej Siewior; +Cc: linux-mtd, tglx
On Tue, 2015-12-01 at 20:46 +0100, Richard Weinberger wrote:
> Am 26.11.2015 um 21:23 schrieb Sebastian Andrzej Siewior:
> > wear_leveling_worker() currently unconditionally puts a PEB on
> > erase in
> > the error case even it just been taken from the free_list and never
> > used.
> > In case the PEB was never used it can be put back on the free list
> > saving a precious erase cycle.
> >
> > v1…v2:
> > - to_leb_clean -> dst_leb_clean
> > - use the nested option for ensure_wear_leveling()
> > - do_sync_erase() can't go -ENOMEM so we can just go into
> > RO-mode now.
> >
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>
> Thanks a lot for the error handling cleanup and this improvement!
> All three patches look good to me.
> Artem, unless you have objections I'd merge them.
Sure, thanks!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-12-02 11:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-26 20:23 [PATCH 1/3] mtd: ubi: fixup error correction in do_sync_erase() Sebastian Andrzej Siewior
2015-11-26 20:23 ` [PATCH 2/3] mtd: ubi: don't leak e if schedule_erase() fails Sebastian Andrzej Siewior
2015-11-26 20:23 ` [PATCH 3/3] mtd: ubi: wl: avoid erasing a PEB which is empty Sebastian Andrzej Siewior
2015-12-01 19:46 ` Richard Weinberger
2015-12-02 11:56 ` Artem Bityutskiy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.