linux-kernel - Re: [RFC/PATCH] ubi: Replace wear leveling thread with a workqueue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CALF0-+VLju8MNu=T9_qVZJsQK6w7H9gq_qnTKTySKPQnZi5QTA@mail.gmail.com>
Date:	Fri, 23 Nov 2012 20:42:35 -0300
From:	Ezequiel Garcia <elezegarcia@...il.com>
To:	Shmulik Ladkani <shmulik.ladkani@...il.com>
Cc:	linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Artem Bityutskiy <dedekind1@...il.com>
Subject: Re: [RFC/PATCH] ubi: Replace wear leveling thread with a workqueue

Hi Shmulik,

Thanks for the review.

On Fri, Nov 23, 2012 at 6:26 PM, Shmulik Ladkani
<shmulik.ladkani@...il.com> wrote:
[...]
>> -             spin_lock(&ubi->wl_lock);
>> -             if (list_empty(&ubi->works) || ubi->ro_mode ||
>
> Originally, 'ubi_thread' did nothing if 'ubi->ro_mode'.
> This filtering is missing from 'ubi_wl_do_work' implementation.
> How do we guarantee 'ubi_wl_do_work' is never queued when in RO mode?
>

I believe that we do this through ubi->wq_enabled. See here:

static void __schedule_ubi_work(struct ubi_device *ubi, struct ubi_work *wrk)
{
        spin_lock(&ubi->wl_lock);
        list_add_tail(&wrk->list, &ubi->works);
        ubi_assert(ubi->works_count >= 0);
        ubi->works_count += 1;
        if (ubi->wq_enabled && !ubi_dbg_is_bgt_disabled(ubi))
                queue_work(ubi->workqueue, &ubi->work);
        spin_unlock(&ubi->wl_lock);
}

As you can see from above, work is queued only if ubi->wq_enabled
(which should be a boolean and probably with inverse logic,
but that's not important).
Unless I've missed something, this patch doesn't change that.

[...]
>> +     if (failures++ > WL_MAX_FAILURES) {
>> +             /*
>> +              * Too many failures, disable the workqueue and
>> +              * switch to read-only mode.
>> +              */
>
> This condition will never be met (after your change), since 'failures'
> is local to 'ubi_wl_do_work' (per work invocation).
>
> Formerly, 'failures' was local to 'ubi_thread' (per ubi device's
> thread), hence it was possible that several 'do_works()' calls have
> failed during thread's lifetime, reaching the WL_MAX_FAILURES limit.
>
> If we'd like to preseve the 'failures' semantics, 'failures' should be
> an 'ubi_device' property.
>

Ah, totally right. I overlooked and assumed 'failures' was per-device.
Now I see it's local scoped. Will fix in V2.

> One last thing:
> Some variables and functions (debug and sysfs) are still named *bgt*,
> which is confusing.
>

I made specific reference to that in the commit message.
I didn't want to change any userspace related to prevent
userspace tools/scripts from stop working.

If we all agree (and specially Artem) I can rename everything
to something like wq_foo, wq_bar.

Thanks again!

    Ezequiel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/