Issue 8021 - Wrong mdb_env_sync() with at least MDB_RDONLY
Summary: Wrong mdb_env_sync() with at least MDB_RDONLY
Status: RESOLVED PARTIAL
Alias: None
Product: OpenLDAP
Classification: Unclassified
Component: slapd (show other issues)
Version: unspecified
Hardware: All All
: --- normal
Target Milestone: ---
Assignee: OpenLDAP project
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-01-08 19:52 UTC by Hallvard Furuseth
Modified: 2015-11-30 18:26 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Hallvard Furuseth 2015-01-08 19:52:38 UTC
Full_Name: Hallvard B Furuseth
Version: lmdb 0.9.14
OS: 
URL: 
Submission from: (NULL) (81.191.45.22)
Submitted by: hallvard


mdb_env_sync() works with MDB_RDONLY at least on Unix, but
it syncs MDB_WRITEMAP changes incorrectly: fdatasync instead
of msync because mdb_env_open() cleared MDB_WRITEMAP.

Users are likely doing the same thing anyway: Using MDB_RDONLY
instead of MDB_RDONLY|MDB_WRITEMAP on WRITEMAP-written DBs.

Finally, writers committing with and without WRITEMAP will
sync each others' commits incorrectly.  lmdb.h does warn they
will "not cooperate well", but it should warn it breaks ACID.

LMDBv2 can instead fix all this with 2 bits in MDB_meta:
"Changed with/without WRITEMAP since last sync".

In LMDBv1, I think the smallest-impact fix is for env_sync
to both msync and fdatasync when RDONLY is set and WRITEMAP
is clear.  A greedier fix would be to always sync both ways
when the user calls env_sync.

Either way, it may be best to keep WRITEMAP when RDONLY.
Only env_map() needs to deal with that.


Pushing suggestions except doc to "mdb/wmap-fix" in my repo.
Comment 1 Howard Chu 2015-01-08 19:59:56 UTC
h.b.furuseth@usit.uio.no wrote:
> Full_Name: Hallvard B Furuseth
> Version: lmdb 0.9.14
> OS:
> URL:
> Submission from: (NULL) (81.191.45.22)
> Submitted by: hallvard
>
>
> mdb_env_sync() works with MDB_RDONLY at least on Unix, but
> it syncs MDB_WRITEMAP changes incorrectly: fdatasync instead
> of msync because mdb_env_open() cleared MDB_WRITEMAP.
>
> Users are likely doing the same thing anyway: Using MDB_RDONLY
> instead of MDB_RDONLY|MDB_WRITEMAP on WRITEMAP-written DBs.

mdb_env_sync() shouldn't even be doing any work at all if the env was 
opened RDONLY. We should return nop in that case. RDONLY means RDONLY! 
No writes, no calls that will alter the storage layer.

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/

Comment 2 Hallvard Furuseth 2015-01-08 20:15:46 UTC
On 08/01/15 20:59, Howard Chu wrote:
> mdb_env_sync() shouldn't even be doing any work at all if the env was
> opened RDONLY. We should return nop in that case. RDONLY means RDONLY!

Currently it syncs changes written by another process.

Comment 3 Hallvard Furuseth 2015-01-08 20:18:23 UTC
On 08/01/15 20:52, h.b.furuseth@usit.uio.no wrote:
> Users are likely doing the same thing anyway: Using MDB_RDONLY
> instead of MDB_RDONLY|MDB_WRITEMAP on WRITEMAP-written DBs.

Er, I mean if any users are doing this.  But users do
come up with things that the developers never thought of.

Comment 4 Howard Chu 2015-01-09 11:26:21 UTC
changed notes
changed state Open to Test
moved from Incoming to Software Bugs
Comment 5 OpenLDAP project 2015-05-19 10:15:27 UTC
partial fix in master.  Revisit for LMDBv2.
Comment 6 Hallvard Furuseth 2015-05-19 10:15:27 UTC
changed notes
Comment 7 Quanah Gibson-Mount 2015-11-30 18:26:29 UTC
changed state Test to Closed
Comment 8 Quanah Gibson-Mount 2015-11-30 18:26:41 UTC
changed state Closed to Partial