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

Merge master-libblockdev to master #260

Merged
merged 153 commits into from May 30, 2017

Conversation

Projects
None yet
7 participants
@vpodzime
Contributor

vpodzime commented May 15, 2017

We are right after a new release. Which should be the last one not requiring libblockdev and not using it for what it could.

This PR is basically a proposal to switch to using libblockdev for everything we can. Gaining speed and clarity, getting rid of many ugly lines of code and sharing common base with other storage-related projects like blivet and hopefully others in future.

All of the changes went through the review process already as they have been added as PRs for the master-libblockdev branch in the recent months. We have also been running our tests on the master-libblockdev branch as well as Cockpit's storage-related tests. None of which are signaling any regressions.

So this PR is really about the discussion and (hopefully) common agreement on taking this step forward. The +/- stats are not as great as we have hoped for, there have been some tests added only to the master-libblockdev branch so in total we are definitely shrinking the codebase here. Plus a lot of complex code was replaced by a similar number of lines which are however very simple few-line functions.

vpodzime and others added some commits Nov 1, 2016

Merge pull request #125 from vojtechtrefny/master_libblockdev-swap2
Use libblockdev swap plugin for swapspace
Use libblockdev as a library not just the plugins
libblockdev has its own logic of loading plugins. It runs checks, picks
preferred plugin implementations over the fallback ones (based on configuration)
etc. There's no reason to link our code directly with the plugins, we can link
the library, initialize it with required plugins when the daemon initializes and
then add extra plugins when modules are loaded.
Merge pull request #132 from vpodzime/master-libblockdev_lib
Use libblockdev as a library not just the plugins
Merge pull request #142 from vojtechtrefny/master-libblockdev_fix-lib…
…blockdev

Fix bd_reinit and g_clear_error calls in btrfs, zram and bcache
Merge branch 'master' into master-libblockdev
Conflicts:
   src/tests/dbus-tests/test_60_partitioning.py
   src/udiskslinuxswapspace.c
Do not start threaded jobs automatically
If a threaded job is started automatically right after it is constructed, the
code using it has no chance to connect the "completed" signal(s) without risking
that the job function run in the thread may already be finished when the
signal(s) is/are connected which may result in the signal handler(s) never being
called. We need to prevent such race condition.
Add a function for running threaded jobs synchronously
So that we can run functions as jobs in places where we need to wait for the
function/job to finish. Just like we do with spawned jobs in many places.
Merge pull request #194 from vpodzime/master-threaded_jobs
Fix and improvement for threaded jobs
Merge branch 'master' into master-libblockdev
Conflicts:
   packaging/udisks2.spec.in
   src/tests/dbus-tests/test_60_partitioning.py

vpodzime and others added some commits May 22, 2017

Also check if libblockdev supports bcache
libblockdev's KBD plugin can be compiled without the bcache
support. So just checking for the plugin's presence is not
enough, we also need to check if it supports bcache to decide
whether we can support it or not.
udisksdaemon.c: Use common function for job creation
Use a common function for the same code which is used in a few
different places.

Signed-off-by: Tony Asleson <tasleson@redhat.com>
Merge pull request #268 from tasleson/udisks_daemon_dupes
udisksdaemon.c: Use common function for job creation
Fix documentation of the daemon job launching functions
The newly created spawned and threade jobs are no longer being
started automatically since commits
fcc8bc3 and
d80ab58. And a simple job has
nothing to start.
Remove "--no-debug" option from udisksd manpage
This option was removed in ce0b1e2
and we forgot to remove it from the manpage. Add "--debug" instead.
Use systemd-defined macros in the spec file template
That's what Fedora packaging guidelines specify. [1][2] Also we
need to use these for all our packages containing .service files,
thus also in udisks2-zram.

And we can also use the %{_udevrulesdir} macro.

[1] http://fedoraproject.org/wiki/Packaging:Systemd#Packaging
[2] https://fedoraproject.org/wiki/Packaging:Scriptlets?rd=Packaging:ScriptletSnippets#Systemd

Resolves: rhbz#1412949
udiskslinuxvolumegroup.c: Add remove/empty device func
The functions handle_remove_device and handle_empty_device are
very similar.  Add common and have each call it instead.

Signed-off-by: Tony Asleson <tasleson@redhat.com>
configure.ac: Save/Restore compiler/linker flags
If you do '--enable-lvm2' you stomp on the complier and linker flags.
I found this because I was unable to build a daemon with debug symbols
and all the modules enabled.

Signed-off-by: Tony Asleson <tasleson@redhat.com>
Merge pull request #271 from vpodzime/fix_daemon_job_docs
Fix documentation of the daemon job launching functions
Merge pull request #272 from tasleson/fix_configure_ac
configure.ac: Save/Restore compiler/linker flags
udiskslinuxblock.c: Use common device handling func.
Signed-off-by: Tony Asleson <tasleson@redhat.com>
Merge pull request #266 from vpodzime/master-libblockdev_epel_fixes
Fixes needed to configure, compile and build on RHEL/CentOS 7
Merge pull request #267 from tasleson/linux_block_dupes
udiskslinuxblock.c: Use common device handling func.
udiskslinuxvolumegroup.c: Add common LV create func.
Signed-off-by: Tony Asleson <tasleson@redhat.com>

udiskslinuxvolumegroup.c: Use common for handle_create_thin_volume

Modify common function handle_create_volume to accomodate creating
thin volumes too.

Signed-off-by: Tony Asleson <tasleson@redhat.com>
Run post actions for udisks2-zram package only on Fedora
udisks2-zram package is only on Fedora
Merge pull request #274 from vojtechtrefny/master-libblockdev_centos-…
…spec-fix

Run post actions for udisks2-zram package only on Fedora
Merge branch 'master' into master-libblockdev
Conflicts:
  packaging/udisks2.spec.in
@vpodzime

This comment has been minimized.

Show comment
Hide comment
@vpodzime

vpodzime May 30, 2017

Contributor

I see no objections and this PR has been here open for comments for long enough, I take it silence means approval. So let's move on with this.

Contributor

vpodzime commented May 30, 2017

I see no objections and this PR has been here open for comments for long enough, I take it silence means approval. So let's move on with this.

@vpodzime vpodzime merged commit 1a7b720 into master May 30, 2017

9 of 12 checks passed

Complete Jenkins Job (private) Build finished.
Details
ci/i686/f_rawhide Build and tests finished: failure
Details
ci/x86_64/f_26 Build and tests finished: failure
Details
All RPM builds (private) Build finished.
Details
ci/i686/f_26 Build and tests finished: success
Details
ci/x86_64/f_rawhide Build and tests finished: success
Details
rpms/i686/f_25 RPM build finished: success
Details
rpms/i686/f_26 RPM build finished: success
Details
rpms/i686/f_rawhide RPM build finished: success
Details
rpms/x86_64/f_25 RPM build finished: success
Details
rpms/x86_64/f_26 RPM build finished: success
Details
rpms/x86_64/f_rawhide RPM build finished: success
Details

@vpodzime vpodzime deleted the master-libblockdev branch May 30, 2017

@vpodzime

This comment has been minimized.

Show comment
Hide comment
@vpodzime

vpodzime May 30, 2017

Contributor

master-libblockdev was merged and removed.

Contributor

vpodzime commented May 30, 2017

master-libblockdev was merged and removed.

@RecklessProcedure

This comment has been minimized.

Show comment
Hide comment
@RecklessProcedure

RecklessProcedure Jun 24, 2017

So, we moved off of udisks1 to get around the LVM dependency because software raid is dangerous. Then the project swallows libblockdev which requires... LVM. Unfortunately, I'll have to recommend its retirement when 2.6.5 is no longer sustainable. Please consider a --without-lvm2 flag for the future. As opinions go, I think leaning on BTRFS / ZFSonLinux would be a much better idea than going anywhere near md.

RecklessProcedure commented Jun 24, 2017

So, we moved off of udisks1 to get around the LVM dependency because software raid is dangerous. Then the project swallows libblockdev which requires... LVM. Unfortunately, I'll have to recommend its retirement when 2.6.5 is no longer sustainable. Please consider a --without-lvm2 flag for the future. As opinions go, I think leaning on BTRFS / ZFSonLinux would be a much better idea than going anywhere near md.

@vojtechtrefny

This comment has been minimized.

Show comment
Hide comment
@vojtechtrefny

vojtechtrefny Jun 24, 2017

Collaborator

@RecklessProcedure Both udisks and libblockdev are modular and you can build them without LVM module/plugin. For udisks just run configure without --enable-lvm2 option and for libblockdev run configure with --without-lvm.

Collaborator

vojtechtrefny commented Jun 24, 2017

@RecklessProcedure Both udisks and libblockdev are modular and you can build them without LVM module/plugin. For udisks just run configure without --enable-lvm2 option and for libblockdev run configure with --without-lvm.

@RecklessProcedure

This comment has been minimized.

Show comment
Hide comment
@RecklessProcedure

RecklessProcedure Jun 24, 2017

My apologies then, there was a post on here about md/lvm being part of the core api. The recent downstream docks also seem to think it's required. I'll have a go at building this latest myself and see if there's a way to keep unneeded/wanted dependencies out of the build. I should have considered this, downstream devs don't always play with configure switches.

RecklessProcedure commented Jun 24, 2017

My apologies then, there was a post on here about md/lvm being part of the core api. The recent downstream docks also seem to think it's required. I'll have a go at building this latest myself and see if there's a way to keep unneeded/wanted dependencies out of the build. I should have considered this, downstream devs don't always play with configure switches.

@vpodzime

This comment has been minimized.

Show comment
Hide comment
@vpodzime

vpodzime Jun 26, 2017

Contributor

My apologies then, there was a post on here about md/lvm being part of the core api.

MD RAID support is part of the core API and core UDisks2. But that hasn't changed in any way recently.

Contributor

vpodzime commented Jun 26, 2017

My apologies then, there was a post on here about md/lvm being part of the core api.

MD RAID support is part of the core API and core UDisks2. But that hasn't changed in any way recently.

@RecklessProcedure

This comment has been minimized.

Show comment
Hide comment
@RecklessProcedure

RecklessProcedure Jun 26, 2017

I'm still looking at it, but the only thing I can say for certain is that building udisks requires no less than 5 additional packages it did not need before. (This is after putting every single "--without-package" that was possible in libblockdev.) Well, at least the Qt ecosystem has enough sense not to require udisks.

I'll note this is not to poke fault at the udisks developers. You're going where features are available. It is just unfortunate that, like so many of the modern Linux initiatives, there is a focus on throwing in everything possible, including the kitchen sink into every package. Small/lightweight is nowhere to be seen, and I wonder how long that will last in a world of battery powered computers. After all, who needs raid, lvm, etc. on a smartphone?

RecklessProcedure commented Jun 26, 2017

I'm still looking at it, but the only thing I can say for certain is that building udisks requires no less than 5 additional packages it did not need before. (This is after putting every single "--without-package" that was possible in libblockdev.) Well, at least the Qt ecosystem has enough sense not to require udisks.

I'll note this is not to poke fault at the udisks developers. You're going where features are available. It is just unfortunate that, like so many of the modern Linux initiatives, there is a focus on throwing in everything possible, including the kitchen sink into every package. Small/lightweight is nowhere to be seen, and I wonder how long that will last in a world of battery powered computers. After all, who needs raid, lvm, etc. on a smartphone?

@M4rtinK

This comment has been minimized.

Show comment
Hide comment
@M4rtinK

M4rtinK Jun 26, 2017

I don't really see this as throwing everything in - it seems to me more like providing a comprehensive API providing suport for wide range of technologies. That way developers that want to use many different storage devices in their application can just use a single consistent API and don't have to use many different libraries, each with a different API. And the libraries themselves are still there, udisks just provides a nice consistent abstraction layer above them.

As for comprehensive API vs battery powered devices - if implemented correctly there should be no real overhead vs an API that suports less, as the parts that are not in use should not have any performance degradation effect.

As for RAID & LVM on a smartphone - I'm writing this on a smarphone that uses LVM - a Jolla C. It has two LVs formatted with EXT4, one for home and one for rootfs. And it's predecessor, the Jolla 1, even used btrfs. But that turned out to be a bad idea due to overall btrfs deficiencies and they switched to LVM + EXT4, which works fine since then AFAIK.

M4rtinK commented Jun 26, 2017

I don't really see this as throwing everything in - it seems to me more like providing a comprehensive API providing suport for wide range of technologies. That way developers that want to use many different storage devices in their application can just use a single consistent API and don't have to use many different libraries, each with a different API. And the libraries themselves are still there, udisks just provides a nice consistent abstraction layer above them.

As for comprehensive API vs battery powered devices - if implemented correctly there should be no real overhead vs an API that suports less, as the parts that are not in use should not have any performance degradation effect.

As for RAID & LVM on a smartphone - I'm writing this on a smarphone that uses LVM - a Jolla C. It has two LVs formatted with EXT4, one for home and one for rootfs. And it's predecessor, the Jolla 1, even used btrfs. But that turned out to be a bad idea due to overall btrfs deficiencies and they switched to LVM + EXT4, which works fine since then AFAIK.

@RecklessProcedure

This comment has been minimized.

Show comment
Hide comment
@RecklessProcedure

RecklessProcedure Jun 26, 2017

I think we can both see that your use of LVM on a smartphone is the exception and not the rule. ;)

RecklessProcedure commented Jun 26, 2017

I think we can both see that your use of LVM on a smartphone is the exception and not the rule. ;)

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