Skip to content

Comments

Allow LMDB to access read-only filesystems and file databases#2384

Closed
cypof wants to merge 1 commit intoBVLC:masterfrom
cypof:lmdb_in_read_only_file
Closed

Allow LMDB to access read-only filesystems and file databases#2384
cypof wants to merge 1 commit intoBVLC:masterfrom
cypof:lmdb_in_read_only_file

Conversation

@cypof
Copy link
Member

@cypof cypof commented Apr 28, 2015

Currently LMDB databases have to be stored on RW filesystems, which is an issue in multi-user settings. Also for deployment convenience, this PR enables single-file DBs, instead of folder-based.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should check the return value of stat(2).

@hyc
Copy link

hyc commented Apr 30, 2015

That's not true. LMDB explicitly checks for read-only filesystems when using the MDB_RDONLY flag and mdb_env_open() will succeed without using MDB_NOLOCK. Using MDB_NOLOCK is quite dangerous if the DB isn't on a read-only filesystem. This patch is bogus.

@cypof
Copy link
Member Author

cypof commented Apr 30, 2015

I did the test again, and it doesn't work for me. It probably depends on what they consider a read-only filesystem. Our setup is a single file database owned by root. Users only have read permission. With this patch it works, without it I get:

db.hpp:109] Check failed: mdb_status == 0 (13 vs. 0) Permission denied

@hyc
Copy link

hyc commented Apr 30, 2015

A read-only filesystem is precisely defined. It is one which was mounted with the read-only mount option. CDROMs are an obvious example. It is safe to open a DB on a read-only filesystem without locking because by definition, no one else can alter it.

It is not safe to open a DB on a read-write filesystem without locking, even if only for read access, because there's always the possibility that some other user with write access will modify it. Your example is not a read-only filesystem.

@cypof
Copy link
Member Author

cypof commented Apr 30, 2015

If we do not want to accept that risk for Caffe, it would be very helpful to find another way to open a DB without write permission. We would prefer not to have all datasets on a shared mount writeable to anyone.

@moskewcz
Copy link

our group needed to use a similar patch/workaround when running on ORLN titan for similar reasons. arguably, even with locking caffe's use of lmdb isn't safe/correct (at the application level, for common use cases) if the db is modified during a run. at the least the results would become 'wrong' (for some def. of wrong).

on that note, it might be nice to have some checking/protection for the db staying constant in the case where locking is possible; perhaps the db should be opened not only read-only, but in such a way that writing becomes forbidden. however, i'm not sure offhand if lmdb/leveldb support such a use case. certainly other data layers do not ...

adding MDB_NOLOCK would just seem to change the failure mode, and i'm not convinced it's any worse overall than what might happen currently if db writes occur during a run. going out guessing on a limb, i might guess the common different in failure mode would be a noisy failure instead of a silent one.

on general principle i still might shy faway rom making MDB_NOLOCK the default behaviour, but i see no reason it can't be a data layer option in the same spirit that lmdb currently provides the option. @cypof: maybe you could make a PR to that effect?

@moskewcz
Copy link

also, second issue: i have no particular info/opinion on supporting single-file dbs (although it sounds fine), but i suspect it a separate PR would be preferred.

@hyc
Copy link

hyc commented May 1, 2015

@cypof then use a shared mount that's writable to no one, that's what most people mean when they talk about read-only filesystems in the first place.

@moskewcz in the writable/nolock case if a writer modifies the database while there are active readers, the readers are likely to see invalid data - the writer can overwrite data the readers were looking at, because without locking the writer doesn't know what's being read. Usually this will crash a reader.

@lukeyeager
Copy link
Contributor

@cypof Now that #3088 is merged, this can be closed or split up to only include this change:

this PR enables single-file DBs, instead of folder-based

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants