[Stale] Compare identical environments by using a `same_file::Handle` by darnuria · Pull Request #179 · meilisearch/heed

darnuria · 2023-07-09T10:11:54Z

This PR implements this suggestion #145 (comment) and try to fix #180.

Stalled because the current implementation with same_file open files since windows needs it, but lmdb itself use posix locks with fnctl syscall on unixes.

We use the same_file::Handle struct that compares dev_t and ino_t instead of raw paths internally to make the comparison more accurate. On Windows, you should read how it works 😅.

EDITS: Edited to add the caveat found during implementing the idea.

Caveat of this PR

Tl;DR Keeping FileDescriptor open, may trouble flock/fcntl works inside lmdb and screw multi-process lock with lmdb.

From comment: #179 (comment)

Unix ideal: just check (dev_t, ino_t)

Unix way

stat kind of function clean nice no File Descriptor keeped.
Note: that same file keep open a File Descriptor open!

https://github.com/BurntSushi/same-file/blob/master/src/unix.rs#L10

Windows

An other same-file rely on winapi32, involving Filehandles.

Lmdb limitation

For sure it may break the flock()ing (not the best system api..), and the multi-process features of lmdb is really handy.

Checked-out lmdb sources:

About flocking in lmdb.h the sentense you pointed changed with:

commit 77845345ca9bf3854fd9da60a3e3b0527fa9c76a
Author: Hallvard Furuseth <hallvard@openldap.org>
Date:   Tue Sep 27 07:03:42 2016 +0200

    ITS#8505 Clarify fork() caveat, mdb_env_get_fd(), flock->fcntl.

diff --git a/libraries/liblmdb/lmdb.h b/libraries/liblmdb/lmdb.h
index 30d5862..319fcf6 100644
--- a/libraries/liblmdb/lmdb.h
+++ b/libraries/liblmdb/lmdb.h
@@ -97,11 +97,12 @@
  *    transactions.  Each transaction belongs to one thread.  See below.
  *    The #MDB_NOTLS flag changes this for read-only transactions.
  *
- *  - Use an MDB_env* in the process which opened it, without fork()ing.
+ *  - Use an MDB_env* in the process which opened it, not after fork().
  *
  *  - Do not have open an LMDB database twice in the same process at
  *    the same time.  Not even from a plain open() call - close()ing it
- *    breaks flock() advisory locking.
+ *    breaks fcntl() advisory locking.  (It is OK to reopen it after
+ *    fork() - exec*(), since the lockfile has FD_CLOEXEC set.)
  *
  *  - Avoid long-lived transactions.  Read transactions prevent
  *    reuse of pages freed by newer write transactions, thus the

Kerollmops · 2023-07-09T12:31:32Z

Thank you @darnuria,

It looks like the same_file::Handle implements Eq + Hash, which makes it a good candidate for replacing the current PathBuf key in the OPENED_ENV hashmap. What do you think?

heed/src/env.rs

darnuria · 2023-07-10T13:23:03Z

heed/src/env.rs

+/// Mind that Handle currently open the file, so avoid writing through the fd held by [same_file::Handle],
+/// since the file will also be opened by LMDB.
+static OPENED_ENV: Lazy<RwLock<HashMap<Handle, EnvEntry>>> = Lazy::new(RwLock::default);


Leaved a mark since Handle open the file and hold a file descriptor https://github.com/BurntSushi/same-file/blob/master/src/unix.rs#L10

Ok, so can we add a comment about it? I mean a really documentation comment in the lib.rs file. Under an Important/Remark header, please? I don't know how LMDB behaves towards that.

I am not sure that we really care about this sentence as we keep it open the whole env-lifetime right?

Do not have open an LMDB database twice in the same process at the same time. Not even from a plain open() call - close()ing it breaks flock() advisory locking.

(backported in description in case this PR got stuck/closed to help somebody)
Yeah my worries comes from that line, and my first idea to just check (dev_t, ino_t)

For sure it may break the flock()ing (not the best system api..), and the multi-process features of lmdb is really handy.

Checked-out lmdb sources:

https://github.com/LMDB/lmdb/blob/3947014aed7ffe39a79991fa7fb5b234da47ad1a/libraries/liblmdb/mdb.c#L4786-L4799

https://github.com/LMDB/lmdb/blob/3947014aed7ffe39a79991fa7fb5b234da47ad1a/libraries/liblmdb/mdb.c#L2998-L3010

https://github.com/LMDB/lmdb/blob/3947014aed7ffe39a79991fa7fb5b234da47ad1a/libraries/liblmdb/mdb.c#L5146-L5156

https://github.com/LMDB/lmdb/blob/3947014aed7ffe39a79991fa7fb5b234da47ad1a/libraries/liblmdb/lmdb.h#L102-L105

About flocking in lmdb.h the sentense you pointed changed with:

commit 77845345ca9bf3854fd9da60a3e3b0527fa9c76a Author: Hallvard Furuseth <hallvard@openldap.org> Date: Tue Sep 27 07:03:42 2016 +0200 ITS#8505 Clarify fork() caveat, mdb_env_get_fd(), flock->fcntl. diff --git a/libraries/liblmdb/lmdb.h b/libraries/liblmdb/lmdb.h index 30d5862..319fcf6 100644 --- a/libraries/liblmdb/lmdb.h +++ b/libraries/liblmdb/lmdb.h @@ -97,11 +97,12 @@ * transactions. Each transaction belongs to one thread. See below. * The #MDB_NOTLS flag changes this for read-only transactions. * - * - Use an MDB_env* in the process which opened it, without fork()ing. + * - Use an MDB_env* in the process which opened it, not after fork(). * * - Do not have open an LMDB database twice in the same process at * the same time. Not even from a plain open() call - close()ing it - * breaks flock() advisory locking. + * breaks fcntl() advisory locking. (It is OK to reopen it after + * fork() - exec*(), since the lockfile has FD_CLOEXEC set.) * * - Avoid long-lived transactions. Read transactions prevent * reuse of pages freed by newer write transactions, thus the

So, what do we conclude? Can we still use the same_file::Handle struct? If not, what do we plan? Is it even possible to get the dev_t and ino_t without opening the file?

stat function family (except the fstatat) check metadata and return and don't maintain a file descriptor.

It's the kind of function used by metadata in rust std, sadly posix never standardized a "device / FSID" so ino_t/dev_t is left to implementation detail.

For windows it's more https://docs.rs/winapi-util/latest/winapi_util/file/fn.information.html
https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getfileinformationbyhandle

heed/src/env.rs

Kerollmops · 2023-07-10T14:09:28Z

I read somewhere that hard links on Windows require Administrator privileges...

darnuria · 2023-07-10T14:14:52Z

(not directly related to hardlink) Yeah windows CI had troble with theses two: https://github.com/meilisearch/heed/actions/runs/5508749200/jobs/10041079210?pr=179#step:4:140

Kerollmops · 2023-07-10T15:07:33Z

Do you know if we can run a certain pipeline in the CI as admin or Windows admin?

Samefile abstract away differences between Unixes and windows to check if two files lead to the same file on the underlying filesystem. Now it should follow through soft-links, hard-links and file moved.

Precise that Handle open the file.

heed/src/env.rs

darnuria · 2023-07-11T12:23:32Z

Do you know if we can run a certain pipeline in the CI as admin or Windows admin?

Don't know, also discovering that windows way of doing is really complicated. 🤡, like moving dir or getting file information.

darnuria · 2023-07-11T12:26:08Z

heed/src/env.rs

+        assert_eq!(env_name, env.path());
+
+        let env_renamed = dir.path().join("serafina.mdb");
+        std::fs::rename(&env_name, &env_renamed).unwrap();


for the windows part maybe the little UNIX api? https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/rename-wrename?view=msvc-170

Kerollmops · 2023-07-11T15:25:39Z

Don't know, also discovering that windows way of doing is really complicated. 🤡, like moving dir or getting file information.

I would like to skip the tests that require Admin rights on Windows as I don't have time to make them work in the CI for now 😬 What do you think?

darnuria force-pushed the open_env_device_inode branch 3 times, most recently from 0fba17a to 993fe55 Compare July 9, 2023 16:08

darnuria commented Jul 9, 2023

View reviewed changes

heed/src/env.rs Show resolved Hide resolved

heed/src/env.rs Show resolved Hide resolved

heed/src/env.rs Show resolved Hide resolved

heed/src/env.rs Show resolved Hide resolved

darnuria marked this pull request as ready for review July 9, 2023 16:12

Kerollmops added the breaking A change that is breaking the semver label Jul 10, 2023

Kerollmops requested changes Jul 10, 2023

View reviewed changes

heed/src/env.rs Show resolved Hide resolved

Kerollmops changed the title ~~Check env with device and Inode instead of paths.~~ Compare identical environments by using a same_file::Handle Jul 10, 2023

darnuria commented Jul 10, 2023

View reviewed changes

Kerollmops added this to the v0.20.0 milestone Jul 10, 2023

darnuria mentioned this pull request Jul 10, 2023

Checking for already opened LMDB Environments #180

Open

5 tasks

darnuria added 5 commits July 10, 2023 17:20

Check env file already open with samefile.

e880cf7

Samefile abstract away differences between Unixes and windows to check if two files lead to the same file on the underlying filesystem. Now it should follow through soft-links, hard-links and file moved.

Apply Review suggestion: deconstruct explicitly handle.

092ab54

Fixup documentation of the OPENED_ENV.

ded5dd8

Precise that Handle open the file.

Review: we don't need canonnicalize anymore.

86da304

Fix hardlink test.

295d159

darnuria commented Jul 10, 2023

View reviewed changes

heed/src/env.rs Outdated Show resolved Hide resolved

darnuria force-pushed the open_env_device_inode branch from 129ba1d to f840921 Compare July 10, 2023 16:17

darnuria added 3 commits July 11, 2023 11:28

fixup! document path changes

d720f09

Fixup! Hardlink will not work reviewed my unixbasics

b065e91

Fixup! Integrate suggestion from review about the tests.

641d229

darnuria force-pushed the open_env_device_inode branch from f840921 to 641d229 Compare July 11, 2023 09:37

darnuria added 3 commits July 11, 2023 12:11

Fix symlink no subdir

833e433

Fix windows details.

911ba89

Windows moving dir is just a burden.

83b4386

darnuria commented Jul 11, 2023

View reviewed changes

darnuria marked this pull request as draft July 20, 2023 16:06

darnuria changed the title ~~Compare identical environments by using a same_file::Handle~~ [Stale] Compare identical environments by using a same_file::Handle Jul 20, 2023

Kerollmops modified the milestones: v0.20.0, v0.21.0 Feb 18, 2024

Kerollmops removed this from the v0.21.0 🗝️ milestone Mar 6, 2025

Conversation

darnuria commented Jul 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Caveat of this PR

Unix way

Windows

Lmdb limitation

Uh oh!

Kerollmops commented Jul 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

darnuria Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

Kerollmops Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

darnuria Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kerollmops Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

darnuria Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Kerollmops commented Jul 10, 2023

Uh oh!

darnuria commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kerollmops commented Jul 10, 2023

Uh oh!

Uh oh!

darnuria commented Jul 11, 2023

Uh oh!

darnuria Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

Kerollmops commented Jul 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

darnuria commented Jul 9, 2023 •

edited

Loading

Kerollmops commented Jul 9, 2023 •

edited

Loading

darnuria Jul 10, 2023 •

edited

Loading

darnuria Jul 11, 2023 •

edited

Loading

darnuria commented Jul 10, 2023 •

edited

Loading