Closed Bug 872442 Opened 11 years ago Closed 6 years ago

[Buri]Low-storage and FOTA / Gecko / Gaia updates

Categories

(Firefox OS Graveyard :: General, defect, P1)

Product:
Firefox OS Graveyard
Component:
General
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(blocking-b2g:-)

Status:
RESOLVED WONTFIX
Project Flags:
blocking-b2g -

People

(Reporter: sync-1, Unassigned)

Details

AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.019.105
 Firefox os  v1.0.1
 Mozilla build ID:20130512070209
 
 
 +++ This bug was initially created as a clone of Bug #453995 +++
 
 This bug report comes in complement to the existing low-storage bugs (Mozilla
 bug #861898). Current bugs are supposed to prevent the phone from going into a
 deadly situation by monitoring disk space and switching to a read-only / delete-only mode when one reaches the low-space situation.
 
 However, there are also some risks to make the disk full during FOTA or Gecko/Gaia update. I am not speaking about copying the files into the /system partition. I am talking about what happens just after, when Gecko is relaunched and finds out there is a new version. Some files will be copied into the /data partition, and some critical files will be updated (e.g. the list of installed apps)
 
 If the end-user is already in low-space situation, or close to the low-space situation, many things could happen such as entering the low-space situation during these update operations, making the disk completely full, having problems writing critical files, being able to copy only half of the update from /system to /data, etc.
 
 ---
 
 We would like to have Mozilla's expert opinion on this matter. Has it been already considered ? Tested ? What is the strategy to avoid problems ?
 
 Surely if we lack of space, we may not be able to run the update perfectly. But in all cases, we shall exit the update process with a stable and consistent set of data, and with enough margin to continue our read-only / delete-only mode (e.g. enough space for SQLite journal files and any other important backup files)
 
 Thanks.
 
 ++++++++++ end of initial bug #453995 description ++++++++++
blocking-b2g: --- → tef?
I think this is a request for information rather than a bug, right? I think Marshall might be able to provide further information.
Flags: needinfo?(marshall)
Whiteboard: [tef-triage]
Technically this isn't really a bug. When we get the information, close this.
(In reply to Jason Smith [:jsmith] from comment #2)
> Technically this isn't really a bug. When we get the information, close this.

Clearing the tef nom here, re-nom if needed,
blocking-b2g: tef? → -
Please, this can be a critical bug, which may cause phone totally die if we don't have any protection for it.

Could QA team add test case for this?
Flags: needinfo?(tchung)
Hi Amelie,

i think the problem here is that this is not an actionable bug; which is why it was tef-'d.  We dont use bugzilla to track QA requests if it isn't tied to a actual defect.  That said, if the question is if we can create a few testcase scenarios around low-storage, it would be better asked via email (which bug 861898 already was addressed to QA).   

Anyway, we dont actually have testcases for FOTA, because it was never clear if we were going to host FOTA. (i think OEMs own this)  Hopefully marshall can address some thoughts.   however, Jason smith is already working on some low storage testcases for bug 861898, and we'll address testcases via email and be sure to include you guys with updates.
Flags: needinfo?(tchung)
I'm closing this though cause this isn't valid. I am working on test cases and working on testing this, however.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → INVALID
Dear Fabrice,

Could you please have a look for our question here?

Thanks,
Amelie
Flags: needinfo?(fabrice)
I am not a FOTA/OTA expert, but for what I can see at [1] we might already have some protections for this case. It seems that we try to use the external storage or the /data partition as a fallback in case that the external storage is not available, not writable or has not enough space to host the update files. We also check if the /data partition has enough space for the update. However it feels safer if we could also check the status of the disk reported by the DiskSpaceWatcher [2] and even add its low threshold [3] to the required space for the update [4].

I would appreciate Dave's or Marshall's expertise here :)

[1] https://mxr.mozilla.org/mozilla-central/source/b2g/components/DirectoryProvider.js#105
[2] https://mxr.mozilla.org/mozilla-central/source/toolkit/components/diskspacewatcher/nsIDiskSpaceWatcher.idl#16
[3] https://mxr.mozilla.org/mozilla-central/source/hal/gonk/GonkDiskSpaceWatcher.cpp#103
[4] https://mxr.mozilla.org/mozilla-central/source/b2g/components/DirectoryProvider.js#152
Flags: needinfo?(dhylands)
The updater currently ensures that twice the space required for the update is available.

Currently, the updater looks in /sdcard, and then /data, and I believe that there is a bug to have it also look in /cache (at least for FOTA updates).

It should definitely be possible to add in the threshold for /data, especially if they're specified using a preference (which appears to be the case, based on [3] from the previous comment.

So we just need to add [2] to [5] when we check LOCAL_DIR

[5] https://mxr.mozilla.org/mozilla-central/source/b2g/components/DirectoryProvider.js#122
Flags: needinfo?(dhylands)
Dave: Are you saying that the updater checks that there's twice the amount of storage available in /data as it's expecting to write to /data?

Or is it checking that the total storage available in /data + /sdcard is twice what it needs to temporarily create?

Which volumes do we generally write to during an update? Comment 0 indicates that we need to write to /data. And we're of course writing to /system. Are there more?

Also, for my understanding, why does it check for twice the amount needed? Is that because it only has an approximate estimate of what it needs? Or to account for things like that files always use a whole number of blocks and so require additional space compared to their size in the update package?
Also, I don't think this is an invalid bug. It's a bug to ask us to investigate something. We file those all the time.

However I agree that this shouldn't be a blocker until we find that there are actual problems here, and that those problems are bad enough that they should be blockers.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(In reply to Jonas Sicking (:sicking) from comment #11)
> Also, I don't think this is an invalid bug. It's a bug to ask us to
> investigate something. We file those all the time.

Bugzilla isn't a support forum. These items are better discussed over email with appropriate parties to cc-ed, probably on dev-b2g.

> 
> However I agree that this shouldn't be a blocker until we find that there
> are actual problems here, and that those problems are bad enough that they
> should be blockers.

It can't be a blocker because there's nothing actionable here. This isn't even a meta bug for tracking.

I won't reclose the bug explicitly here, but I don't agree we should keep discussion forums in bugzilla better suited for dev-b2g.
(In reply to sync-1 from comment #0)

>  Surely if we lack of space, we may not be able to run the update perfectly.
> But in all cases, we shall exit the update process with a stable and
> consistent set of data, and with enough margin to continue our read-only /
> delete-only mode (e.g. enough space for SQLite journal files and any other
> important backup files)

It's true that we could do better when we update the 3rd party preloaded apps after a gecko+gaia update. Currently we would just end up with unusable apps, but we don't properly remove them from the registry in such cases.
Flags: needinfo?(fabrice)
(In reply to Jonas Sicking (:sicking) from comment #10)
> Dave: Are you saying that the updater checks that there's twice the amount
> of storage available in /data as it's expecting to write to /data?
> 
> Or is it checking that the total storage available in /data + /sdcard is
> twice what it needs to temporarily create?

So I took a closer look at this. What the code does is it looks for a volume with twice as much free space as the size of the update.mar file:
https://mxr.mozilla.org/mozilla-central/source/b2g/components/DirectoryProvider.js#152

I think that this is actually incorrect.

The general process that the updater goes through is something like this:
1 - download the update (needs 1x update.mar size)
2 - Unpacks the update. This takes place on /system and may need 1x the uncompressed size of the update.mar file plus a bit more (for directory overheads)
3 - b2g is restarted to actually apply the update, which essentially just does a directory rename, and removal of the old files.

So we really need 1 x compressed size of the update.mar file in the place where the update.mar file is stored, and 1 x uncompressed size of the update.mar file plus some overhead in /system, and a small amount of space in /data where we store the update.xml file and some state.

> Which volumes do we generally write to during an update? Comment 0 indicates
> that we need to write to /data. And we're of course writing to /system. Are
> there more?

We try to store the update.mar to the sdcard if we can, then fallback to /data. There is talk of also adding /cache in there somewhere.

There is a small amount of state data maintained on /data regardless of where the update.mar file is located, and the update is unpacked into /system.

> Also, for my understanding, why does it check for twice the amount needed?
> Is that because it only has an approximate estimate of what it needs? Or to
> account for things like that files always use a whole number of blocks and
> so require additional space compared to their size in the update package?

I actually don't know. That code was there when I started working on the updater, and I just kept that part of the decision.

I'm going to needinfo marshall to see if he knows.
> 3 - b2g is restarted to actually apply the update,
 > which essentially just does a directory rename,
 > and removal of the old files.
 
 
 In our understanding (maybe we are wrong ?), there is also a 4th step:
 
 4 - Gecko finds out an update has just been installed. Some files (3rd party packaged apps) may be copied from the /system to the /data partition, where they will live, be updateable and deleteable.
 
 ---
 
 The initial intent of this discussion was about this 4th step. We haven't seen, in previous low-storage discussion and patches, anything about this step. There were patches to detect the low-storage situation, patches to limit indexedDB and localStorage abilities while in low-storage, but nothing obvious about updating applications following a FOTA / OTA update.
 
 We fear that the copy operation for these 3rd party packaged apps (from /system to /data) doesn't care about the low-storage situation, doesn't care about the desired 5MB margin, and that we would eventually end up with a disk free space much lower than the 5MB margin.
Whiteboard: [tef-triage]
(In reply to sync-1 from comment #15)
> > 3 - b2g is restarted to actually apply the update,
>  > which essentially just does a directory rename,
>  > and removal of the old files.
>  
>  
>  In our understanding (maybe we are wrong ?), there is also a 4th step:
>  
>  4 - Gecko finds out an update has just been installed. Some files (3rd
> party packaged apps) may be copied from the /system to the /data partition,
> where they will live, be updateable and deleteable.

So I think that there is some confusion about system update versus app update. The steps I was describing are for a system update. 3rd party apps are not included in system updates.

App updates follow a totally different code path, even though they share a common UI.
(In reply to Dave Hylands [:dhylands] from comment #16)

> 
> So I think that there is some confusion about system update versus app
> update. The steps I was describing are for a system update. 3rd party apps
> are not included in system updates.

That's not true. We ship 3rd party preloaded apps in system updates, and this list can change from an update to another, or the new versions can be bigger.

Currently we fail silently to copy these apps to /data if we have not enough space. We could do something smarter, but I'm not sure exactly what.
(In reply to Fabrice Desré [:fabrice] from comment #17)
> (In reply to Dave Hylands [:dhylands] from comment #16)
> 
> > 
> > So I think that there is some confusion about system update versus app
> > update. The steps I was describing are for a system update. 3rd party apps
> > are not included in system updates.
> 
> That's not true. We ship 3rd party preloaded apps in system updates, and
> this list can change from an update to another, or the new versions can be
> bigger.
> 
> Currently we fail silently to copy these apps to /data if we have not enough
> space. We could do something smarter, but I'm not sure exactly what.

OK- so I wasn't aware of that.

Regardless, it looks like the free space checks in the updater need to be revisited.

1 - We need to make sure that there is sufficient free space in the /system partition to store the uncompressed files.

2 - We need to make sure that there is sufficient free space in /data for 3rd party apps which we may copy over. How do we identify these?

3 - We need to make sure that there is sufficient free space to store the update.mar file whereever it is that we download it to.

I think that we may need to split the free space checks into 2 places, since I'm not sure we can do them all before we download the update.mar file.
(In reply to Dave Hylands [:dhylands] from comment #18)
> 
> 2 - We need to make sure that there is sufficient free space in /data for
> 3rd party apps which we may copy over. How do we identify these?

During install of a packaged app, we already check if the size requested in the mini-manifest doesn't go past the disk space threshold at install time. During download, I think the stat function from the device storage API is checked periodically to ensure that we have sufficient space. If we fail to have storage, as Fabrice states, we fail silently in the update process. The better approach would probably fire a download error to indicate we ran out of storage, which I might already have a bug filed on this.
(In reply to Dave Hylands [:dhylands] from comment #14)
> So we really need 1 x compressed size of the update.mar file in the place
> where the update.mar file is stored, and 1 x uncompressed size of the
> update.mar file plus some overhead in /system, and a small amount of space
> in /data where we store the update.xml file and some state.
> 

This is definitely more accurate than 2x, but will still require some fuzzy math (maybe 1.1x the uncompressed size for safety?). Requiring the uncompressed size in the calculation also requires the MAR to be downloaded, which means there will need to be multiple disk space checks..


> > Also, for my understanding, why does it check for twice the amount needed?
> > Is that because it only has an approximate estimate of what it needs? Or to
> > account for things like that files always use a whole number of blocks and
> > so require additional space compared to their size in the update package?
> 
> I actually don't know. That code was there when I started working on the
> updater, and I just kept that part of the decision.
> 
> I'm going to needinfo marshall to see if he knows.

IIRC this was to account for background apps downloading data and the like.
Flags: needinfo?(marshall)
Firefox OS is not being worked on
Status: REOPENED → RESOLVED
Closed: 11 years ago → 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.