Closed Bug 1472722 Opened 2 years ago Closed Last year

Use unix-excl vfs by default

Categories

(Toolkit :: Storage, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file)

We currently use unix-excl only if storage.nfs_filesystem is set to true, but there doesn't seem to be anything preventing us from using it by default

The advantages would be:
1. we can avoid the memory mapped -shm files in wal mode (and crashes like bug 1471041)
2. users on nfs wouldn't have to discover existence of the above pref
3. protection from external apps touching the db while we are running

Disadvantages:
1. We couldn't use the same database from different processes (afaict we never do)
This makes sense to me.  Do you want to flip the pref or just change the code that uses the pref at https://searchfox.org/mozilla-central/rev/ed2763bea882619ccb48b0aecc54e523d2bdd2ae/storage/TelemetryVFS.cpp#877 to be the logic we always use and eliminate the pref?  A SHIELD pref-flip study is also possible, but seems overkill given that the only impacted platforms are Linux and OS X and "unix-excl" is low risk[1].

1: As per the SQLite source and elsewhere, SQLite tracks locks internally per-process, so in-process interference is ruled out.  Additionally, the locking mechanism of "unix-excl" operates in the same manner as "unix", it just escalates to EXCLUSIVE immediately.  The only expected fallout from the change is that use of the sqlite3 CLI against an actively running Firefox on Linux/OS X will result in errors that previously wouldn't have occurred.  This isn't a big impediment to debugging, as it was always advisable to only investigate SQLite databases while the profile was not in use.

Example sqlite3 failures:
```
$ sqlite3 webappsstore.sqlite
SQLite version 3.22.0 2018-01-22 18:45:57
Enter ".help" for usage hints.
sqlite> .schema
Error: database is locked
sqlite> .dump
PRAGMA foreign_keys=OFF;
BEGIN TRANSACTION;
/**** ERROR: (5) database is locked *****/
ROLLBACK; -- due to errors
sqlite> 
```
(In reply to Andrew Sutherland [:asuth] from comment #1)
> Do you want to flip the pref or just change the
> code that uses the pref

I initially thought to remove the pref and just hardcode unix-excl, then I read your point about debugging while Firefox runs. I don't think that's a critical need, but it's possible someone needs to debug a db state on-fly in certain situations.
Thus, I'm currently thinking to introduce an inverted hidden pref, something like storage.allowMultiProcessAccess.

> A SHIELD pref-flip study is
> also possible, but seems overkill given that the only impacted platforms are
> Linux and OS X and "unix-excl" is low risk[1].

Yes, I would also consider that if we'd introduce a Windows-excl thing, to be on the safe side. Doesn't look necessary for this case.
Use the exclusive VFS on unix systems, so that:
1. we can avoid the memory mapped -shm files in wal mode
2. we gain more compatibility with nfs shares
3. we gain some protection from third parties touching open dbs

On the other side it won't be possible anymore to use an open database from a
different process (like the Sqlite command line), for which we provide an hidden
pref: storage.multiProcessAccess.enabled
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Comment on attachment 8989925 [details]
Bug 1472722 - Use the unix-excl Sqlite VFS by default. r=asuth

Andrew Sutherland [:asuth] has approved the revision.

https://phabricator.services.mozilla.com/D1964
Attachment #8989925 - Flags: review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/91c6a8ba1579
Use the unix-excl Sqlite VFS by default. r=asuth
I assume the Android test tries to access signons.sqlite from a separate process while Firefox is running? Or could the test close Firefox first?
I don't know much about our Android test harness in java.
Should we use the old non-exclusive vfs on Android in general, or just flip the pref for tests?
Flags: needinfo?(mak77) → needinfo?(vivekb.balakrishnan)
looks like this is the only failing test, so I'd probably go for a pref flip just for this test, but I'd like to listen to your opinion first.
Looks like vivek is no more active, not sure who to ask to, :nalexander was active in this area some time ago... let's try.
Flags: needinfo?(vivekb.balakrishnan) → needinfo?(nalexander)
(In reply to Marco Bonardo [::mak] from comment #7)
> I assume the Android test tries to access signons.sqlite from a separate
> process while Firefox is running? Or could the test close Firefox first?
> I don't know much about our Android test harness in java.
> Should we use the old non-exclusive vfs on Android in general, or just flip
> the pref for tests?

Android always runs a separate passwords process, so there is no option to do something test-only.  That is, we must continue to support the use case of a non-Gecko process and the Gecko process both accessing the same passwords DB concurrently.

I think that means that we should retain the existing non-exclusive VFS on Android in general.

The _best_ way to solve this would be to replace the logins provider on Android entirely, but that's not going to happen, since Fennec is in maintenance mode.
Flags: needinfo?(nalexander)
I guess we could set the new pref to true in mobile.js
Comment on attachment 8989925 [details]
Bug 1472722 - Use the unix-excl Sqlite VFS by default. r=asuth

Nick Alexander :nalexander has approved the revision.

https://phabricator.services.mozilla.com/D1964
Attachment #8989925 - Flags: review+
Since Phabricator doesn't echo comments in bugzilla, I will also answer to Nick question here:
> Is there any sort of migration procedure for prefs that have new meanings now? I.e., folks using storage.nfs_filesystem will suddenly find themselves with a worse locking story. Does that concern you or is the overhead of back-compat not worth the effort?

Those users are not a problem here, because in the new world the storage.nfs_filesystem behavior is the default, so they won't notice a difference. On the other side users with issues on nfs that don't know about the pref will have Firefox working immediately.
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/b3494bf0c3d3
Use the unix-excl Sqlite VFS by default. r=nalexander,asuth
https://hg.mozilla.org/mozilla-central/rev/b3494bf0c3d3
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.