Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lock plugin #578

Merged
merged 7 commits into from
Apr 18, 2016
Merged

Lock plugin #578

merged 7 commits into from
Apr 18, 2016

Conversation

KurtMi
Copy link
Contributor

@KurtMi KurtMi commented Apr 8, 2016

No description provided.

@@ -0,0 +1,15 @@
- infos = Information about the lock plugin is in keys below
- infos/author = Kurt Micheli <e1026558@student.tuwien.ac.at>
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure you want to use that e-mail address? I think they shut it down when you leave TU.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don not have a proper email, can I have a @libelektra.org mail?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have sent you an e-mail to your new address!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you

@markus2330
Copy link
Contributor

Thank you!

@markus2330
Copy link
Contributor

Btw. nice plugin, is this really your first one? Where there any difficulties?

@markus2330 markus2330 mentioned this pull request Apr 13, 2016
@markus2330
Copy link
Contributor

markus2330 commented Apr 13, 2016

Btw. I think we found a use case for this kind of plugin: When we do not have or should not (NFS) rely on timestamps this could be useful.

Your strategy would already work if only the kdb tool is used, but it would not handle long-living processes that do multiple kdbSet?

So following tasks would be required to make a plugin for NFS-conflict detections out of it:

  • rename it to something less generic, unless you plan to have multiple locking features combined in this plugin (including the global lock proposed)
  • hold lock from first kdbGet() until kdbClose() (for multiple kdbSet())
  • release lock when pid behind the lock is already gone
  • avoid the busy loop

@tom-wa do you have some further suggestions regarding NFS?

@KurtMi
Copy link
Contributor Author

KurtMi commented Apr 15, 2016

I update the plugin in this PR to the ordered global plugin, the simple-lock plugin as you proposed will follow soon.

@KurtMi
Copy link
Contributor Author

KurtMi commented Apr 15, 2016

Btw. yes the first plugin, got the knowledge from reviewing and the copy-template works fine.


A semaphore is used for the synchronisation and the implemented algorithm favors the writer,
because updates should be propagated soon as possible and a `kdbSet` is more expansive than
a `kdbGet`. This expansiveness comes from merging and conflict solving.
Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense to favor the writers (because readers should get up-to-date information), but I cannot follow your explanation. Do you mean expensive instead of expansive? What has the run-time cost to do with preference?

merging is not a part of kdbSet, but validation is.

@markus2330
Copy link
Contributor

Looks really nice! Can you rename it to semlock? Btw. the simple-lock: plugin names should be a-z0-9 only, so maybe longlock?

Did you try to run it with the race test?

@KurtMi
Copy link
Contributor Author

KurtMi commented Apr 18, 2016

Did you try to run it with the race test?

I tried but failed, it seems that the global hooks are being called a little weird.

I inserted printfs in the kdb.c, when the hooks get called and configured the global plugins that semlock is the only plugin and I get this output:

> kdb set /test/asd "asd"
OPEN
OPEN
OPEN
OPEN
PREGETSTORAGE
24966 PRE GET START
24966 PRE GET END
POSTGETSTORAGE
24966 POST GET START
24966 POST GET END
PREGETSTORAGE
24966 PRE GET START
24966 PRE GET END
PREGETSTORAGE         <---why is here a PREGETSTORAGE? 
24966 POST GET START
24966 POST GET END
CLOSE
CLOSE
PREGETSTORAGE
24966 PRE GET START
24966 PRE GET END
POSTGETSTORAGE
24966 POST GET START
24966 POST GET END
Using name system/test/asd
Set string to asd
24966 PRE SET START
24966 PRE SET END
POSTCOMMIT
24966 POST SET START
24966 POST SET END
CLOSE
CLOSE

Note that the PRESETSTORAGE is not printed.

Also I am worried about:

// resolver says that sync is
// not needed, so we
// skip other pre-commit
// plugins

in kdb.c:770. Without releasing set call no other process can make a get.

@markus2330
Copy link
Contributor

The placement of hooks is an ongoing discussion, see #555. I even thought about giving special placement especially for lock plugins (lockgetstorage, ..., unlocksetstorage). This would ensure that locks are always executed before and after any other plugins (without needing to resort of some weird ordering constructs).

Thus these global locking plugins are not really useful as "normal" plugins, we would not really lose something that way. And this way we could place the global plugins specifically in mind that it will always unlock if it was locked before. (and avoid such problems as in kdb.c:770)

@KurtMi @tom-wa what do you think?

@KurtMi
Copy link
Contributor Author

KurtMi commented Apr 18, 2016

We should discuss that topic after the meeting tomorrow. I will create an issue for the longlock, as reminder for me.

@KurtMi KurtMi mentioned this pull request Apr 18, 2016
4 tasks
@KurtMi
Copy link
Contributor Author

KurtMi commented Apr 18, 2016

I adjusted the tags and rating. I think it can be merged.

@markus2330
Copy link
Contributor

Thank you! Let us discuss it tomorrow.

@markus2330 markus2330 merged commit 68733dc into ElektraInitiative:master Apr 18, 2016
@KurtMi KurtMi deleted the lock_plugin branch August 12, 2016 13:08
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.

None yet

2 participants