1472722 - Use unix-excl vfs by default

Status: RESOLVED FIXED

(Toolkit :: Storage, enhancement, P2)

Description

[Marco Bonardo [:mak]]

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)

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

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> 
```

Comment 2

[Marco Bonardo [:mak]]

(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.

Comment 3

[Marco Bonardo [:mak]]

Attachment: Bug 1472722 - Use the unix-excl Sqlite VFS by default. r=asuth

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

Comment 4

[Phabricator Automation]

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91c6a8ba1579
Use the unix-excl Sqlite VFS by default. r=asuth

Comment 6

[Andreea Pavel [:apavel]]

Backed out for failing android at testPasswordProvider 

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=91c6a8ba15799071c33c87ef002526192560021d

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=187321769&repo=autoland&lineNumber=1517

Backout: https://hg.mozilla.org/integration/autoland/rev/749a2803d87557484ad3cc8b10cd955e0eb45ffc

Comment 7

[Marco Bonardo [:mak]]

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?

Comment 8

[Marco Bonardo [:mak]]

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.

Comment 9

[Marco Bonardo [:mak]]

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.

Comment 10

[Nick Alexander :nalexander [he/him]]

(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.

Comment 11

[Marco Bonardo [:mak]]

I guess we could set the new pref to true in mobile.js

Comment 12

[Phabricator Automation]

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

Comment 13

[Marco Bonardo [:mak]]

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.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3494bf0c3d3
Use the unix-excl Sqlite VFS by default. r=nalexander,asuth

Comment 15

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/b3494bf0c3d3