Re: [MTD] UBI: Per volume update marker

[Artem Bityutskiy <dedekind@infradead.org>]

Hi Alexander,

On Mon, 2007-01-29 at 11:47 +0100, Alexander Schmidt wrote:
> here is the second version of the patch, containing updates of the
> documentation in vtbl.h and upd.h. We decided not to integrate handling of
> the old update marker volume, as the scenario of booting a new kernel on a
> flash device containing an old update marker is definately rare.

I have several notes.

>   * along with this program; if not, write to the Free Software
>   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>   *
> - * Author: Artem B. Bityutskiy
> + * Authors: Artem B. Bityutskiy
> + *          Alexander Schmidt

May you please instead add something like

+ * Jan 2006: Alexander Schmidt, implemented per-volume update.

which is what people usually do in these cases.

>  /**
> @@ -230,6 +245,7 @@ void ubi_vtbl_close(const struct ubi_inf
>   * @last_eb_bytes: how many bytes are stored in the last logical eraseblock
>   * @used_bytes: how many bytes of data this volume contains
>   * @corrupted: non-zero if the data is corrupted
> + * @updating: non-zero if volume is under update
>   *
>   * Note, the @usable_leb_size field is not stored on flash, as it is easily
>   * calculated with help of the @data_pad field. But it is just very handy, so
> @@ -250,6 +266,7 @@ struct ubi_vtbl_vtr {
>  	int last_eb_bytes;
>  	long long used_bytes;
>  	int corrupted;
> +	int updating;
>  };

In the code, in comments you still call this flag update marker. I am
not sure, but may be it makes sense to call this field 'upd_marker'
instead? It looks more coherent to the comments, but on the other hand
it does not look very self-documenting. Hmm... We need ask Andreas, he
is a naming-expert :-)
 
> --- ubi-2.6.orig/drivers/mtd/ubi/sysfs.c
> +++ ubi-2.6/drivers/mtd/ubi/sysfs.c
> @@ -79,7 +79,6 @@ static ssize_t dev_avail_eraseblocks_sho
>  static ssize_t dev_total_eraseblocks_show(struct class_device *dev, char *buf);
>  static ssize_t dev_volumes_count_show(struct class_device *dev, char *buf);
>  static ssize_t dev_max_ec_show(struct class_device *dev, char *buf);
> -static ssize_t dev_update_show(struct class_device *dev, char *buf);
>  static ssize_t dev_reserved_for_bad_show(struct class_device *dev, char *buf);
>  static ssize_t dev_bad_peb_count_show(struct class_device *dev, char *buf);
>  static ssize_t dev_max_vol_count_show(struct class_device *dev, char *buf);
> @@ -98,8 +97,6 @@ static struct class_device_attribute dev
>  	__ATTR(volumes_count, S_IRUGO, dev_volumes_count_show, NULL);
>  static struct class_device_attribute dev_max_ec =
>  	__ATTR(max_ec, S_IRUGO, dev_max_ec_show, NULL);
> -static struct class_device_attribute dev_update =
> -	__ATTR(update, S_IRUGO, dev_update_show, NULL);

AFAIU, in your implementation only one update at a time is possible, so
I think it is better not to remove this attribute but export the ID of
the volume which is currently being updated.

This allows you to find "all update-interrupted" volumes and to avoid
including a volume which is really being updated now to this list.

>  void ubi_sysfs_vol_close(struct ubi_uif_volume *vol)
>  {
> +	class_device_remove_file(&vol->dev, &vol_updating);

But this new flag is anyway needed.

>  /**
> + * ubi_vmt_updvol - set or remove the update marker for a volume.
> + *
> + * @ubi: the UBI device description object
> + * @vol_id: ID of the volume to re-size
> + * @updating: new value for the update marker
> + *
> + * This function returns zero in case of success, and a negative error code in
> + * case of failure.
> + */
> +int ubi_vmt_updvol(const struct ubi_info *ubi, int vol_id, int updating);
> +
> +/**

Please, do not utilize vmt unit from upd unit at all. Utilize vtbl unit
directly.

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