Re: [PATCH] ubi: Add mean_ec sysfs node and accounting

[Rob Lee <rdlee.upstream@xxxxxxxxx>]

I just noticed a few non-optimal and invalid things I'm doing here:

- I should not add a new ec+count variable.  I should be using
good_peb_count instead.
- The current implementation uses a ec_sum that doesn't get
recalculated when blocks go bad.  The sum should be recalculated each
time as previously good blocks that contributed to the sum may go bad.
This recalculation could occur once on attachment and then adjusted
each time a block goes bad, or, on each ec change which allows not
adding a new variable.

Given these problem, I will make and send a v2 shortly.


On Wed, Oct 23, 2019 at 9:48 AM Rob Lee <rdlee.upstream@xxxxxxxxx> wrote:
>
> This patch adds the necessary runtime accounting to keep mean_ec
> up to date as well as code to export mean_ec to sysfs. While
> mean_ec already existed as part of the ubi attach structure, it
> was currently not exported to sysfs nor is its value update after
> device attachment.
>
> From a production device point of view, tracking the overall wear
> rate is critical to determine wear lifetime and catching abusive
> write behavior to the storage device that may lead to excessive
> wear.  The currently available sysfs information, max_ec, has a
> rate of change that is somewhat erratic and thus isn't a sufficient
> for this purpose.
>
> Signed-off-by: Rob Lee <rdlee.upstream@xxxxxxxxx>
> ---
>  drivers/mtd/ubi/attach.c | 2 ++
>  drivers/mtd/ubi/build.c  | 5 +++++
>  drivers/mtd/ubi/ubi.h    | 2 ++
>  drivers/mtd/ubi/wl.c     | 6 ++++++
>  4 files changed, 15 insertions(+)
>
> diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
> index 10b2459..a5af3be 100644
> --- a/drivers/mtd/ubi/attach.c
> +++ b/drivers/mtd/ubi/attach.c
> @@ -1596,6 +1596,8 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
>         ubi->corr_peb_count = ai->corr_peb_count;
>         ubi->max_ec = ai->max_ec;
>         ubi->mean_ec = ai->mean_ec;
> +       ubi->ec_count = ai->ec_count;
> +       ubi->ec_sum = ai->ec_sum;
>         dbg_gen("max. sequence number:       %llu", ai->max_sqnum);
>
>         err = ubi_read_volume_table(ubi, ai);
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index d636bbe..24d48bf 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -126,6 +126,8 @@ static struct device_attribute dev_volumes_count =
>         __ATTR(volumes_count, S_IRUGO, dev_attribute_show, NULL);
>  static struct device_attribute dev_max_ec =
>         __ATTR(max_ec, S_IRUGO, dev_attribute_show, NULL);
> +static struct device_attribute dev_mean_ec =
> +       __ATTR(mean_ec, S_IRUGO, dev_attribute_show, NULL);
>  static struct device_attribute dev_reserved_for_bad =
>         __ATTR(reserved_for_bad, S_IRUGO, dev_attribute_show, NULL);
>  static struct device_attribute dev_bad_peb_count =
> @@ -364,6 +366,8 @@ static ssize_t dev_attribute_show(struct device *dev,
>                 ret = sprintf(buf, "%d\n", ubi->vol_count - UBI_INT_VOL_COUNT);
>         else if (attr == &dev_max_ec)
>                 ret = sprintf(buf, "%d\n", ubi->max_ec);
> +       else if (attr == &dev_mean_ec)
> +               ret = sprintf(buf, "%d\n", ubi->mean_ec);
>         else if (attr == &dev_reserved_for_bad)
>                 ret = sprintf(buf, "%d\n", ubi->beb_rsvd_pebs);
>         else if (attr == &dev_bad_peb_count)
> @@ -391,6 +395,7 @@ static struct attribute *ubi_dev_attrs[] = {
>         &dev_total_eraseblocks.attr,
>         &dev_volumes_count.attr,
>         &dev_max_ec.attr,
> +       &dev_mean_ec.attr,
>         &dev_reserved_for_bad.attr,
>         &dev_bad_peb_count.attr,
>         &dev_max_vol_count.attr,
> diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
> index 721b6aa..e188ace 100644
> --- a/drivers/mtd/ubi/ubi.h
> +++ b/drivers/mtd/ubi/ubi.h
> @@ -580,6 +580,8 @@ struct ubi_device {
>         int max_ec;
>         /* Note, mean_ec is not updated run-time - should be fixed */
>         int mean_ec;
> +       unsigned int ec_count;
> +       uint64_t ec_sum;
>
>         /* EBA sub-system's stuff */
>         unsigned long long global_sqnum;
> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
> index 3fcdefe..a8641a1 100644
> --- a/drivers/mtd/ubi/wl.c
> +++ b/drivers/mtd/ubi/wl.c
> @@ -449,6 +449,7 @@ static int sync_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
>         int err;
>         struct ubi_ec_hdr *ec_hdr;
>         unsigned long long ec = e->ec;
> +       int ec_incr;
>
>         dbg_wl("erase PEB %d, old EC %llu", e->pnum, ec);
>
> @@ -465,6 +466,7 @@ static int sync_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
>                 goto out_free;
>
>         ec += err;
> +       ec_incr = err;
>         if (ec > UBI_MAX_ERASECOUNTER) {
>                 /*
>                  * Erase counter overflow. Upgrade UBI and use 64-bit
> @@ -486,6 +488,10 @@ static int sync_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
>
>         e->ec = ec;
>         spin_lock(&ubi->wl_lock);
> +       ubi->ec_sum += ec_incr;
> +       if (likely(ubi->ec_count))
> +               ubi->mean_ec = div_u64(ubi->ec_sum, ubi->ec_count);
> +
>         if (e->ec > ubi->max_ec)
>                 ubi->max_ec = e->ec;
>         spin_unlock(&ubi->wl_lock);
> --
> 2.7.4
>

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/