Re: [MTD] UBI: Per volume update marker

Artem Bityutskiy
2007-01-29T13:12:28Z

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.