Re: [PATCH] Add UBI volume rename functionality

[Stefano Babic]

Hi Hamish,

On 11/05/2017 13:14, Hamish Guthrie wrote:
> This patch enables functionality to handle double copy with fallback on
> UBI volumes. A firmware element, for example u-boot may be updated using a
> double copy with fallback strategy. In the flash layout there should be two
> volumes provided for the firmware element (i.e. u-boot, u-boot_r). The
> replacement element should be targeted to the replacement volume and on
> completion of the copy, the volume will be atomically renamed.
>

Well, a double-copy with fallback is already possible with SWUpdate, but
in a very different way. This is done using selections - it is a very
used pattern with UBI and NAND.

But let's see your proposal. I try to understand if this is power-off safe.

This at this example: we have a SWU with several images, some of them
are using your new feature. An update is started, and some volumes are
updated and renamed.

At this point, we experience a power-cut. Because the rename is done at
the end of the copy, we can have the very bad situation where a part of
the images are installed and *activated* (due to the rename), some of
them not. The target could not be operative again, if there are some
incompatibilities. This breaks the atomicity of the update.

What do you think ?

> The sw-description entry for this would look like:
> {
> filename = "u-boot.img";
> volume = "u-boot_r";
> replaces = "u-boot";
> name = "u-boot";
> version = "2017.03-r3";
> install-if-different = true;
> },
>

Should the two volumes belong to the same MTD or not ?

> This indicates to swupdate that the image u-boot.img should be written
> to the UBI volume u-boot_r. Once this has been completed, the volume u-boot_r
> is renamed to u-boot and u-boot is renamed to u-boot_r. This operation is
> fully atomic.
> ---
> doc/source/sw-description.rst | 12 +++++++++
> handlers/ubivol_handler.c | 62 ++++++++++++++++++++++++++++++++++++++++---
> include/swupdate.h | 1 +
> parser/parser.c | 5 +++-
> 4 files changed, 76 insertions(+), 4 deletions(-)
>
> diff --git a/doc/source/sw-description.rst b/doc/source/sw-description.rst
> index c128c24..e3e2a96 100644
> --- a/doc/source/sw-description.rst
> +++ b/doc/source/sw-description.rst
> @@ -575,6 +575,18 @@ There are 4 main sections inside sw-description:
> | volume | string | images | Just if type = "ubivol". UBI volume |
> | | | | where image must be installed. |
> +-------------+----------+------------+---------------------------------------+
> + | replaces | string | images | Just if type = "ubivol". This is the |
> + | | | | name of the volume this volume will |
> + | | | | be renamed to. This would typically |
> + | | | | be used when doing a dual volume |
> + | | | | replacement, for example, updating |
> + | | | | swupdate, we have 2 UBI volumes |
> + | | | | swupdate and swupdate_r, we install |
> + | | | | the new image into swupdate_r and on |
> + | | | | completion of the installation we |
> + | | | | atomically swap the names of the UBI |
> + | | | | volumes. |
> + +-------------+----------+------------+---------------------------------------+
> | ubipartition| string | images | Just if type = "ubivol". Volume to be |
> | | | | created or adjusted with a new size |
> +-------------+----------+------------+---------------------------------------+
> diff --git a/handlers/ubivol_handler.c b/handlers/ubivol_handler.c
> index bf58658..b3a4f4c 100644
> --- a/handlers/ubivol_handler.c
> +++ b/handlers/ubivol_handler.c
> @@ -46,6 +46,23 @@ static struct ubi_part *search_volume(const char *str, struct ubilist *list)
> return NULL;
> }
>
> +static struct ubi_part *find_volume(const char *str)
> +{
> + struct flash_description *flash = get_flash_info();
> + struct mtd_info *mtd_info = &flash->mtd;
> + struct mtd_ubi_info *mtd_ubi_info;
> + struct ubi_part *ubivol;
> + int i;
> +
> + for (i = mtd_info->lowest_mtd_num; i <= mtd_info->highest_mtd_num; i++) {
> + mtd_ubi_info = &flash->mtd_info[i];
> + LIST_FOREACH(ubivol, &mtd_ubi_info->ubi_partitions, next)
> + if (strcmp(ubivol->vol_info.name, str) == 0)
> + return ubivol;

There is already a search_volume() function, that then should be factorized.

> + }
> + return NULL;
> +}



> +
> static int update_volume(libubi_t libubi, struct img_type *img,
> struct ubi_vol_info *vol)
> {
> @@ -104,11 +121,50 @@ static int update_volume(libubi_t libubi, struct img_type *img,
>
> TRACE("Updating UBI : %s %lld\n",
> img->fname, img->size);
> - if (copyimage(&fdout, img, NULL) < 0) {
> + err = copyimage(&fdout, img, NULL);
> + close(fdout);
> + if (err) {
> ERROR("Error copying extracted file");
> - err = -1;
> + return -1;
> }
> - close(fdout);
> +
> + if (strlen(img->replaces)) {
> + struct ubi_part *volfrom = find_volume(img->volname);
> + struct ubi_part *volto = find_volume(img->replaces);
> +
> + if (volfrom && volto) {
> + struct ubi_rnvol_req rnvol;
> + int count = 0;
> + int i;
> + char masternode[64];
> +
> + snprintf(masternode, sizeof(masternode), "/dev/ubi%d",
> + vol->dev_num);
> +
> + TRACE("Renaming volume %s to %s", img->volname, img->replaces);
> + rnvol.ents[count].vol_id = volfrom->vol_info.vol_id;
> + rnvol.ents[count].name_len = strlen(img->replaces);
> + strcpy(rnvol.ents[count++].name, img->replaces);
> +
> + rnvol.ents[count].vol_id = volto->vol_info.vol_id;
> + rnvol.ents[count].name_len = strlen(img->volname);
> + strcpy(rnvol.ents[count++].name, img->volname);
> +
> + rnvol.count = count;
> +
> + err = ubi_rnvols(libubi, masternode, &rnvol);
> + } else {
> + if (!volfrom)
> + ERROR("Cannot find volume %s", img->volname);
> + if (!volto)
> + ERROR("Cannot find volume %s", img->replaces);
> + err = -1;
> + }
> +
> + if (err != 0)
> + ERROR("Unable to rename volume %s to %s, error %d", img->volname, img->replaces, err);
> + }
> +
> return err;
> }
>
> diff --git a/include/swupdate.h b/include/swupdate.h
> index 6d71333..9243910 100644
> --- a/include/swupdate.h
> +++ b/include/swupdate.h
> @@ -64,6 +64,7 @@ struct img_type {
> char type[SWUPDATE_GENERAL_STRING_SIZE]; /* Handler name */
> char fname[MAX_IMAGE_FNAME]; /* Filename in CPIO archive */
> char volname[MAX_VOLNAME]; /* Useful for UBI */
> + char replaces[MAX_VOLNAME]; /* volume to replace */
> char device[MAX_VOLNAME]; /* device associated with image if any */
> char path[MAX_IMAGE_FNAME]; /* Path where image must be installed */
> char type_data[SWUPDATE_GENERAL_STRING_SIZE]; /* Data for handler */
> diff --git a/parser/parser.c b/parser/parser.c
> index 9a28b2c..4d1265f 100644
> --- a/parser/parser.c
> +++ b/parser/parser.c
> @@ -389,6 +389,7 @@ static void parse_images(parsertype p, void *cfg, struct swupdate_cfg *swcfg)
> GET_FIELD_STRING(p, elem, "version", image->id.version);
> GET_FIELD_STRING(p, elem, "filename", image->fname);
> GET_FIELD_STRING(p, elem, "volume", image->volname);
> + GET_FIELD_STRING(p, elem, "replaces", image->replaces);
> GET_FIELD_STRING(p, elem, "device", image->device);
> GET_FIELD_STRING(p, elem, "mtdname", image->path);
> GET_FIELD_STRING(p, elem, "type", image->type);
> @@ -408,7 +409,7 @@ static void parse_images(parsertype p, void *cfg, struct swupdate_cfg *swcfg)
> get_field(p, elem, "install-if-different", &image->id.install_if_different);
> get_field(p, elem, "encrypted", &image->is_encrypted);
>
> - TRACE("Found %sImage %s %s: %s in %s : %s for handler %s%s %s\n",
> + TRACE("Found %sImage %s %s: %s in %s : %s%s%s for handler %s%s %s\n",
> image->compressed ? "compressed " : "",
> image->id.name,
> image->id.version,
> @@ -416,6 +417,8 @@ static void parse_images(parsertype p, void *cfg, struct swupdate_cfg *swcfg)
> strlen(image->volname) ? "volume" : "device",
> strlen(image->volname) ? image->volname :
> strlen(image->path) ? image->path : image->device,
> + strlen(image->replaces) ? " replacing " : "",
> + strlen(image->replaces) ? image->replaces : "",
> strlen(image->type) ? image->type : "NOT FOUND",
> image->install_directly ? " (installed from stream)" : "",
> (strlen(image->id.name) && image->id.install_if_different) ?
>

Best regards,
Stefano Babic


--
=====================================================================
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=====================================================================

[Hamish Guthrie]

Hi Stefano,

>
> Well, a double-copy with fallback is already possible with SWUpdate, but in a very
> different way. This is done using selections - it is a very used pattern with UBI and
> NAND.
>
> But let's see your proposal. I try to understand if this is power-off safe.
>
> This at this example: we have a SWU with several images, some of them are using
> your new feature. An update is started, and some volumes are updated and renamed.
>
> At this point, we experience a power-cut. Because the rename is done at the end of
> the copy, we can have the very bad situation where a part of the images are installed
> and *activated* (due to the rename), some of them not. The target could not be
> operative again, if there are some incompatibilities. This breaks the atomicity of the
> update.
>
> What do you think ?

I see nowhere in the swupdate documentation which describes a double-copy with fallback scenario, I have also just searched the documentation for selection and I found one reference which has nothing to do with double-copy with fallback. Could you point me to the correct section of the documentation as I seem to be missing it?

In our application, we are looking at having both u-boot and swupdate having double-copy with fallback. There is no dependency between the two, so if u-boot is updated correctly, but the swupdate update fails, there is no consequence. All we are concerned about is individual binary firmware elements being power-fail safe.

>
> > The sw-description entry for this would look like:
> > {
> > filename = "u-boot.img";
> > volume = "u-boot_r";
> > replaces = "u-boot";
> > name = "u-boot";
> > version = "2017.03-r3";
> > install-if-different = true;
> > },
> >
>
> Should the two volumes belong to the same MTD or not ?

Yes they should. According to the UBI documentation, it is advisable to have a single UBI partition on any particular flash device because there is extra overhead for each additional mtd partition.

> >
> > +static struct ubi_part *find_volume(const char *str) {

> > + struct flash_description *flash = get_flash_info();
> > + struct mtd_info *mtd_info = &flash->mtd;
> > + struct mtd_ubi_info *mtd_ubi_info;
> > + struct ubi_part *ubivol;
> > + int i;
> > +
> > + for (i = mtd_info->lowest_mtd_num; i <= mtd_info->highest_mtd_num; i++) {
> > + mtd_ubi_info = &flash->mtd_info[i];
> > + LIST_FOREACH(ubivol, &mtd_ubi_info->ubi_partitions, next)
> > + if (strcmp(ubivol->vol_info.name, str) == 0)
> > + return ubivol;
>
> There is already a search_volume() function, that then should be factorized.

I will refactor.

With kind regards
Hamish

[Stefano Babic]

Hi Hamish,

On 11/05/2017 18:33, Hamish Guthrie wrote:
> Hi Stefano,
>
>>
>> Well, a double-copy with fallback is already possible with
>> SWUpdate, but in a very different way. This is done using
>> selections - it is a very used pattern with UBI and NAND.
>>
>> But let's see your proposal. I try to understand if this is
>> power-off safe.
>>
>> This at this example: we have a SWU with several images, some of
>> them are using your new feature. An update is started, and some
>> volumes are updated and renamed.
>>
>> At this point, we experience a power-cut. Because the rename is
>> done at the end of the copy, we can have the very bad situation
>> where a part of the images are installed and *activated* (due to
>> the rename), some of them not. The target could not be operative
>> again, if there are some incompatibilities. This breaks the
>> atomicity of the update.
>>
>> What do you think ?
>
> I see nowhere in the swupdate documentation which describes a
> double-copy with fallback scenario,


> I have also just searched the
> documentation for selection and I found one reference which has
> nothing to do with double-copy with fallback. Could you point me to
> the correct section of the documentation as I seem to be missing it?
>

Under Software collections. sw-descriptions contains the description for
each copy (copy-A and copy-B). Or for many copies, the way is very
flexible. It is also possible that copies are on different storage, for
example one copy on a UBI volume, the second one on a eMMC (or NOR, or
whatever..). There is a simple example in pseudo-language.

Under examples/description/multi-copy there is an example for
double-copy on SD / eMMC. Same applies to UBI volumes.

> In our application, we are looking at having both u-boot and swupdate
> having double-copy with fallback.

I have understood this, but it is just a use case that cannot be
generalized. The feature as it is can be used for kernel and rootfs, and
they depend together. Or with FPGA images, and so on.

One strict requirement for an update agent is to be atomic: everything
updated or nothing. This patch seems to me that is breaking the concept.

> There is no dependency between the
> two, so if u-boot is updated correctly, but the swupdate update
> fails, there is no consequence.

Yes. But there is a strict dependency in your project. We cannot say
that any SWU can work with this feature.

Even you cannot know if after some time (years ?) another developer in
your company will take over the project and use the feature for
something else.

> All we are concerned about is
> individual binary firmware elements being power-fail safe.

It is not enough. Kernel is a binary image, rootfs is a binary image,
FPGA images are binary firmware. It is not enough that just one of them
is power-fail safe. A release consists on all elements that are tested
together exactly in that version.

>
>>
>>> The sw-description entry for this would look like: { filename =
>>> "u-boot.img"; volume = "u-boot_r"; replaces = "u-boot"; name =
>>> "u-boot"; version = "2017.03-r3"; install-if-different = true;
>>> },
>>>
>>
>> Should the two volumes belong to the same MTD or not ?
>
> Yes they should. According to the UBI documentation, it is advisable
> to have a single UBI partition on any particular flash device because
> there is extra overhead for each additional mtd partition.

Yes, but there is no constraint...and no MTD is set.

Volume is searched in all MTDs in find_volume(), and volumes can belong
to different MTDs. And rename should not work, then.

>
>>>
>>> +static struct ubi_part *find_volume(const char *str) { + struct
>>> flash_description *flash = get_flash_info(); + struct mtd_info
>>> *mtd_info = &flash->mtd; + struct mtd_ubi_info *mtd_ubi_info; +
>>> struct ubi_part *ubivol; + int i; + + for (i =
>>> mtd_info->lowest_mtd_num; i <= mtd_info->highest_mtd_num; i++) {
>>> + mtd_ubi_info = &flash->mtd_info[i]; + LIST_FOREACH(ubivol,
>>> &mtd_ubi_info->ubi_partitions, next) + if
>>> (strcmp(ubivol->vol_info.name, str) == 0) + return ubivol;
>>
>> There is already a search_volume() function, that then should be
>> factorized.
>
> I will refactor.
>