[MTD] UBI: Per volume update marker

[Artem Bityutskiy]

Alexander,

thank you for the patch, please, find my mostly minor comments below.

On Mon, 2007-01-29 at 17:36 +0100, Alexander Schmidt wrote:
> - * Also, in this UBI implementation we make use of so called update marker when
> - * updating volumes. The update marker is global. This may cause quite
> - * unpleasant UBI usability problems. What we could do is to implement
> - * many-updates-at-a-time support by adding a per-volume "corrupted" flag to the
> - * volume table. This flag would be set before update and cleared after update.
> + * The volume table record also contains a so called update marker, which
> + * indicates whether a volume is under update or not. The update marker is
> + * set and stored on flash on the beginning of an update and deleted afterwards.
> + * This makes UBI recognize aborted updates, which may happen because of
> + * power-offs during updates. Read operations on volumes with pending update
> + * markers get rejected.
>   */
[minor] it may also happen if the updating process was killed

>  /*
> + * Volume updating constants used in volume table records.
> + *
> + * @UBI_VOL_NOUPD: volume is not being updated
> + * @UBI_VOL_UPD: volume is being updated
> + */

[minor] I would better say "volume update started" instead of "volume is
being updated" becaus the marker may be there if the volume is not
really being updated, the update was just interrupted.

> +static ssize_t vol_updating_show(struct class_device *dev, char *buf);

[minor] As we started using "update marker", lets be consistent and name
this, say, 'vol_upd_marker_show()'

>  /*
>   * Class device attributes corresponding to files in
> @@ -224,6 +225,8 @@ static struct class_device_attribute vol
>  	__ATTR(usable_eb_size, S_IRUGO, vol_usable_eb_size_show, NULL);
>  static struct class_device_attribute vol_data_bytes =
>  	__ATTR(data_bytes, S_IRUGO, vol_data_bytes_show, NULL);
> +static struct class_device_attribute vol_updating =
> +	__ATTR(updating, S_IRUGO, vol_updating_show, NULL);

The sysfs file will be named "updating". Probably it is less confusing
and more strict to expose the "update marker" term to the outer world
too.

I'd propose

static struct class_device_attribute vol_upd_marker =
	__ATTR(upd_marker, S_IRUGO, vol_upd_marker_show, NULL);

instead.
 
> 			dbg_upd("volume %d is being updated, update marker "
> +	if (upd->updating && upd->vol_id != vol_id) {
> +		dbg_upd("volume %d is being updated, update marker "
>  				"busy", upd->vol_id);

[minor] Just to make coding style consistent:

dbg_upd("volume %d is being updated, update marker "
	"busy", upd->vol_id);

(aligned) or better

dbg_upd("volume %d is being updated, update marker busy", upd->vol_id);

as there are less then 80 characters per line.

> +
> +	mutex_lock(&vtbl->mutex);
> +	vtr->corrupted = 0;
> +	mutex_unlock(&vtbl->mutex);

vtbl->mutex is a private field and only vtbl unit can touch it. Please,
do ot use it outside of vtbl unit.

See 'struct ubi_vtbl_info' definition - each field is commented as
private/public.

Please, just do not touch the 'corrupted' flag, and do not call
'ubi_vtbl_set_corrupted()' in 'ubi_upd_start()'.

The 'corrupted' flag seems to be half-working, I will fix this after we
commit your patch.

> +
>  	upd->vol_id = -1;
>  	return 0;
>  }
> --- ubi-2.6.orig/drivers/mtd/ubi/vtbl.c
> +++ ubi-2.6/drivers/mtd/ubi/vtbl.c
> @@ -160,6 +160,44 @@ int ubi_vtbl_set_data_len(const struct u
>  	return 0;
>  }
>  
> +int ubi_vtbl_updvol(const struct ubi_info *ubi, int vol_id, int upd_marker)
> +{
> +	int err;
> +	struct ubi_vtbl_vtr tmp_vtr;
> +	struct ubi_vtbl_vtr *vtr;
> +	struct ubi_vtbl_info *vtbl = ubi->vtbl;
> +
> +	ubi_assert(vol_id >= 0 && vol_id < vtbl->vt_slots);
> +	ubi_assert(upd_marker == UBI_VOL_NOUPD || upd_marker == UBI_VOL_UPD);
> +	ubi_assert(!ubi_is_ivol(vol_id));
> +
> +	dbg_vtbl("set update marker for volume %d to %d", vol_id, upd_marker);
> +
> +	/* Ensure that this volume exists */
> +	vtr = ubi_vtbl_get_vtr(ubi, vol_id);
> +	if (IS_ERR(vtr)) {
> +		return PTR_ERR(vtr);
> +	}

[minor] No need to do this. We just stand that the volume must exist and
it is the job of the caller to make sure of this. Just add debugging
check

ubi_assert(ubi->vtbl->vt[vol_id].reserved_pebs == 0);

as other similar functions of the vtbl unit do.
 
> -		vtr->corrupted = 0;
> +		if (vtr->upd_marker) {
> +			ubi_warn("volume %d update was interrupted", i);
> +			vtr->corrupted = 1;
> +		}
> +		else
> +			vtr->corrupted = 0;

Please, remove these changes. I (or we) will correct the corrupted flag.

It makes much more sense to distinguish between "(physically) corrupted"
and "update-interrupted". So we have an update-interrupted volume. We
will prohibit reading from it because it simply makes no sense. Reading
from a corrupted volume will make sense because the user (e.g. FS) could
be able to recover some data. Also, "corrupted" will only applicable to
static volumes because we do not care about dynamic volumes' contents.

Makes sense?

> -	int i, reserved_pebs, alignment, data_pad, vol_type, name_len;
> +	int i, reserved_pebs, alignment, data_pad, vol_type, name_len,
> +	    upd_marker;
[minor]

+	int i, reserved_pebs, alignment, data_pad, vol_type, name_len;
+	int upd_marker;
 
> +	if (unlikely(vtr->updating != UBI_VOL_NOUPD &&
> +		     vtr->updating != UBI_VOL_UPD)) {
> +		ubi_err("bad update marker");
> +		goto bad;
> +	}

We renamed this field. I'd suggest you to test your code with paranoid
checks enabled - this may help to catch bugs you do not even think
about.

Side note: hmm, anyway, 'this paranoid_check_vtr()' function should not
exist in vmt unit - it's not vmt's business. Will need to remove it -
but thats a distinct patch, do not bother please.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)