[Q] ubi->beb_rsvd_pebs calculations

[Q] ubi->beb_rsvd_pebs calculations

[Shmulik Ladkani]

Hi Artem,

While working on improving reserved bad PEBs calculations, I encounted
some code portion whose reasoning can't be easily understood.

I'd be happy if you can comment on the following:

from 'erase_worker' in wl.c:

> 	/* It is %-EIO, the PEB went bad */
> 
> 	if (!ubi->bad_allowed) {
> 		ubi_err("bad physical eraseblock %d detected", pnum);
> 		goto out_ro;
> 	}
> 
> 	spin_lock(&ubi->volumes_lock);
> 	need = ubi->beb_rsvd_level - ubi->beb_rsvd_pebs + 1;
> 	if (need > 0) {
> 		need = ubi->avail_pebs >= need ? need : ubi->avail_pebs;
> 		ubi->avail_pebs -= need;
> 		ubi->rsvd_pebs += need;
> 		ubi->beb_rsvd_pebs += need;
> 		if (need > 0)
> 			ubi_msg("reserve more %d PEBs", need);
> 	}

Why attempting to reclaim available PEBs into the 'beb_rsvd_pebs' here
within 'erase_worker'?
If 'beb_rsvd_pebs' was already below the required level, how could it be
that suddenly during 'erase_worker' more PEBs will be available?

The 2 other places which perform this "reclaim" are volume shrink and
volume remove. Within these places reclaim makes sense, as the
shrink/remove operations result in increasing the available PEBs.
(BTW the same code reclaiming beb_rsvd_pebs is duplicated, I've unified
all places).

However here in 'erase_worker' I expect either 'beb_rsvd_pebs' to already be
at the required level, or to be less than the required level - but in
the latter case reclaiming seems as a no-op.

What do you say?
Would you consider removing this "beb_rsvd_pebs reclaim" block?

Regards,
Shmulik

Re: [Q] ubi->beb_rsvd_pebs calculations

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 311 bytes --]

On Sun, 2012-07-01 at 19:52 +0300, Shmulik Ladkani wrote:
> What do you say?
> Would you consider removing this "beb_rsvd_pebs reclaim" block?

Yes, sounds like you are right. Also, with your new proposals all the
"reclaim" blocks should go away at the end, right?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Q] ubi->beb_rsvd_pebs calculations

[Shmulik Ladkani]

Hi Artem,

On Tue, 03 Jul 2012 13:53:47 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Sun, 2012-07-01 at 19:52 +0300, Shmulik Ladkani wrote:
> > What do you say?
> > Would you consider removing this "beb_rsvd_pebs reclaim" block?
> 
> Yes, sounds like you are right.

Good. This mysterious "reclaim" in 'erase_worker' actually interferes
with my new scheme.

BTW removing it from 'erase_worker' means that every place in the code
(present and future) that produces available PEBs must always attempt
refilling 'beb_rsvd_pebs'.
Currently this is already done in volume remove and volume shrink.

> Also, with your new proposals all the
> "reclaim" blocks should go away at the end, right?

Well, yes, my intention is to remove the "beb_rsvd_pebs reclaim" done in
'erase_worker'.
However, the attempt refilling the 'beb_rsvd_pebs' after volume
remove/shrink will still exist.

I guess the picture is a bit vague ;)
Better the code talk.
I hope to submit the patch series in few days.

BTW dealing with all possible cases and corner cases is remarkebly
delicate. Prepare yourself for a "fun" review ;)

Regards,
Shmulik

Re: [Q] ubi->beb_rsvd_pebs calculations

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 315 bytes --]

On Tue, 2012-07-03 at 15:33 +0300, Shmulik Ladkani wrote:
> However, the attempt refilling the 'beb_rsvd_pebs' after volume
> remove/shrink will still exist.

Ah, right, it is needed in case you had smaller %, but then changed the
config/attch option and increased it.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Q] ubi->beb_rsvd_pebs calculations

[Shmulik Ladkani]

On Tue, 03 Jul 2012 15:42:24 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Tue, 2012-07-03 at 15:33 +0300, Shmulik Ladkani wrote:
> > However, the attempt refilling the 'beb_rsvd_pebs' after volume
> > remove/shrink will still exist.
> 
> Ah, right, it is needed in case you had smaller %, but then changed the
> config/attch option and increased it.

Yep.
All being taken care of, hopefully :)

Re: [Q] ubi->beb_rsvd_pebs calculations

[Shmulik Ladkani]

Hi Artem,

One more thing.

In my patchset I'm intorducing CONFIG_MTD_UBI_BEB_LIMIT which replaces
CONFIG_MTD_UBI_BEB_RESERVE.

I decided to use a different name as the semantics got different:

CONFIG_MTD_UBI_BEB_RESERVE specified how many physical eraseblocks
will be reserved for bad eraseblock handling (percents of total number
of good flash eraseblocks).

Whereas CONFIG_MTD_UBI_BEB_LIMIT specifies the maximum bad eraseblocks
UBI expects on the ubi device (percents of total number of flash
eraseblocks).

(Later on, the attach ioctl can support a per-ubi-device limit,
overriding the configured default).

Some might argue that the old name CONFIG_MTD_UBI_BEB_RESERVE can still
be used, describing the new semantic as well.

What's your opinion on the CONFIGs name change? Is it needed?

Regards,
Shmulik

Re: [Q] ubi->beb_rsvd_pebs calculations

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 197 bytes --]

On Tue, 2012-07-03 at 19:01 +0300, Shmulik Ladkani wrote:
> What's your opinion on the CONFIGs name change? Is it needed?

Sounds reasonable, thank you!

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]