lkml.org 
[lkml]   [2012]   [May]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH V2] UBI: add ubi_lnum_purge function to clear work queue for a lnum
From
Date
On Tue, 2012-05-15 at 09:44 +0200, Joel Reardon wrote:
> This is the second part of a patch to allow UBI to force the erasure of
> particular logical eraseblock numbers. In this patch, a new function,
> ubi_lnum_purge, is added that allows the caller to synchronously erase all
> unmapped erase blocks whose LEB number corresponds to the parameter. This
> requires a previous patch that stores the LEB number in struct ubi_work.
>
> This was tested by disabling the call to do_work in ubi thread, which results
> in the work queue remaining unless explicitly called to remove. UBIFS was
> changed to call ubifs_leb_change 50 times for three different LEBs. Then the
> new function was called to clear the queue for the three differnt LEB numbers
> one at a time. The work queue was dumped each time and the selective removal
> of the particular LEB numbers was observed.
>
> Signed-off-by: Joel Reardon <reardonj@inf.ethz.ch>

No objections in general, and I can merge this as soon as you send the
final version. However, for this version I have several notes.

> +/**
> + * ubi_lnum_purge - synchronously erase unmapped PEBs by LEB number.
> + * @ubi_num: UBI device to erase PEBs
> + * @lnum: the LEB number to erase old unmapped PEBs.
> + *
> + * This function is designed to offer a means to ensure that the contents of
> + * old, unmapped LEBs are securely erased without having to flush the entire
> + * work queue of all erase blocks that need erasure. Simply erasing the block
> + * at the time of unmapped is insufficient, as the wear-levelling subsystem
> + * may have already moved the contents. This function navigates the list of
> + * erase blocks that need erasures, and performs an immediate and synchronous
> + * erasure of any erase block that has held data for this particular @lnum.
> + * This may include eraseblocks that held older versions of the same @lnum.
> + * Returns zero in case of success and a negative error code in case of
> + * failure.
> + */
> +int ubi_lnum_purge(int ubi_num, int lnum)
> +{
> + int err;
> + struct ubi_device *ubi;
> +
> + ubi = ubi_get_device(ubi_num);
> + if (!ubi)
> + return -ENODEV;
> +
> + err = ubi_wl_flush_lnum(ubi, lnum);
> + ubi_put_device(ubi);
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(ubi_lnum_purge);

Please, do not introduce a separate exported function for this. Instead,
add "lnum" argument to "ubi_wl_flush". Preserve the old behavior if lnum
is -1. Document this at the header comment. In your case you also need
to call mtd->sync() I think.

> + dbg_wl("flush lnum %d", lnum);
> + list_for_each_entry_safe(wrk, tmp, &ubi->works, list) {
> + if (wrk->lnum == lnum) {
> + down_read(&ubi->work_sem);
> + spin_lock(&ubi->wl_lock);

But you cannot walk the ubi->works list without holding the spinlock.
Any one may add/remove elements to/from this list concurrently.

Take the work_sem at the beginning. Release at the very end.

Then you can do something like this:

int found = 1;

while (found) {
found = 0;
spin_lock(&ubi->wl_lock);
list_for_each_entry(wrk, tmp, &ubi->works, list) {
if (wrk->lnum == lnum) {
list_del(&wrk->list);
ubi->works_count -= 1;
ubi_assert(ubi->works_count >= 0);
spin_unlock(&ubi->wl_lock);
err = wrk->func(ubi, wrk, 0);
if (err)
return err;
spin_lock(&ubi->wl_lock);
found = 1;
break;
}
spin_unlock(&ubi->wl_lock);
}
--
Best Regards,
Artem Bityutskiy
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2012-05-15 15:01    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site