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

LVM cache detection, manipulation and reporting #156

Merged
merged 3 commits into from
Jun 25, 2015

Conversation

vpodzime
Copy link
Contributor

These commits add generic classes for Cache monitoring, manipulation and reporting, the implementation for the LVM Cache and pieces of code needed for the LVMLogicalVolumeDevice class to support cached LVs.

The reason why cached LV is not a new, separate class is that when the cache is detached, such (formerly) cached LV becomes a normal LV like any other and its not possible to change the type/class of an existing object. Thus the LVMCache class is added instead the instances of which are referenced by cached LVs and provide the cache-related functionality to them. The same approach will be later used for BcacheDevice and BcacheCache classes (a device can be formatted as a BcacheDevice with no cache attached to it).

Inheriting classes will implement specific functionality for LVM cache and Bcache.
@vpodzime vpodzime force-pushed the master-lvm_cache_actually branch 2 times, most recently from 6c27daf to 113058f Compare June 22, 2015 07:33
@jkonecny12
Copy link
Contributor

I think this is ok.
I have one question wouldn't be better if you move some code to that abstract base classes? Now they are empty and you will end up with the parts of the same code in all child classes I think.

@vpodzime
Copy link
Contributor Author

I have one question wouldn't be better if you move some code to that abstract base classes? Now they are empty and you will end up with the parts of the same code in all child classes I think.

Any example? The problem is that e.g. the stats reported for Bcache look quite different from stats reported for the LVM cache.

@jkonecny12
Copy link
Contributor

I'm not so familiar with the problematic to tell you.
I think the best will be to wait and you will see that when you will implement the other classes.

@jkonecny12
Copy link
Contributor

I'm no blivet expert but I think this is ok :)

@jkonecny12 jkonecny12 added the ACK label Jun 24, 2015

self._cache = LVMCache(self, size=pool.size, exists=True)

return self._cache
Copy link
Contributor

Choose a reason for hiding this comment

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

This always creates an LVMCache, no? So then the cached property above will always be True for existing LVs. I think it just needs an if pool is not None: guarding the assignment to self._cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, good catch, thanks! It would actually result in a traceback when accessing the size attribute of pool, but that doesn't matter. Fixing and updating the PR with this change.

@dwlehman
Copy link
Contributor

Other than the minor problem in LVMLogicalVolumeDevice.cache this looks good to me.

This way LVMLogicalVolumeDevice can report whether they are cached, provide
information related to their cache and attach cache pool to themselves so that
they become cached.
vpodzime added a commit that referenced this pull request Jun 25, 2015
LVM cache detection, manipulation and reporting
@vpodzime vpodzime merged commit 7b7770e into storaged-project:master Jun 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants