Closed Bug 1539549 Opened 1 year ago Closed 1 year ago

update cert blocklist in single transaction

Categories

(Core :: Security: PSM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: myk, Assigned: myk)

Details

Attachments

(1 file)

The fix for bug 1538372 migrates revocations.txt to the new rkv-based cert store in a single transaction to reduce disk I/O and improve performance. We should do the same for blocklist updates (i.e. the updateCertBlocklist() function in services/common/blocklist-clients.js).

The bulk insert/drop support that Nan Jiang just landed for the kvstore API in bug 1522638 could be useful or instructive here.

NB: extra care should be taken when writing a large dataset in a single transaction. LMDB might return MDB_TXN_FULL errors from mdb_put or mdb_cursor_put.

Unfortunately, LMDB doesn't specify the exact # of maximum dirty pages that a single transaction could possibly generate (I used to run into this when writing a 1GB dataset to a dup_sort store), though it also tries to avoid the issue if the consumer is not using MDB_MULTIPLE store or the nested transactions. See more details at here.

I think a few things we could do here:

  • Always check and handle errors from store.put() and its variants
  • If we still run into MDB_TXN_FULL errors, we can split the write into multiple sub-writes with multiple transactions. The downside is that it requires some extra bookkeeping, and could be hard to maintain the all-or-nothing support in certain cases.

(In reply to Nan Jiang [:nanj] from comment #1)

NB: extra care should be taken when writing a large dataset in a single transaction. LMDB might return MDB_TXN_FULL errors from mdb_put or mdb_cursor_put.

I guess the fix for bug 1538372 could also run into this issue when migrating revocations.txt.

  • Always check and handle errors from store.put() and its variants

We're doing this for cert_storage, but only by propagating the error to the caller. In the case of cert_storage that will be some consumer of the nsICertStorage XPCOM API.

  • If we still run into MDB_TXN_FULL errors, we can split the write into multiple sub-writes with multiple transactions. The downside is that it requires some extra bookkeeping, and could be hard to maintain the all-or-nothing support in certain cases.

Perhaps it would be possible to commit the transaction at that point and then create a new one?

(In reply to Myk Melez [:myk] [@mykmelez] from comment #2)

  • If we still run into MDB_TXN_FULL errors, we can split the write into multiple sub-writes with multiple transactions. The downside is that it requires some extra bookkeeping, and could be hard to maintain the all-or-nothing support in certain cases.

Perhaps it would be possible to commit the transaction at that point and then create a new one?

Yes, we could possibly try that. Although it might fail as well since that commit would write some metadata for that transaction.

Since we're no using nested transactions nor the MDB_MULTIPLE data type, I'm inclined to believe that it's unlikely for us to run into this particular issue.

Assignee: nobody → myk
Status: NEW → ASSIGNED
Pushed by myk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0b745e9ecd50
update cert blocklist using single transaction r=keeler
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.