Change in vdsm[master]: storage: set block schedule elevator using udev

[Alon Bar-Lev]

Alon Bar-Lev has uploaded a new change for review.

Change subject: storage: set block schedule elevator using udev
......................................................................

storage: set block schedule elevator using udev

CURRENT IMPLEMENTATION

block schedule elevator is set by bootstrap using modification to kernel
command-lines.

PROBLEMS IN CURRENT IMPLEMENTATION

1. assumption of grub bootloader.

2. assumption of active kernel.

3. assumption of user not override anything.

4. problem to port.

NEW IMPLEMENTATION

Use udev in order to set the block schedule elevator.

User may override behavior using udev rule, no bootloader dependency or
conflict.

The setting is applied also if vdsm is installed manually.

Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Signed-off-by: Alon Bar-Lev 
---
M vds_bootstrap/vds_bootstrap.py
M vdsm.spec.in
A vdsm/storage/12-vdsm-elevator.rules
M vdsm/storage/Makefile.am
A vdsm/storage/vdsm-elevator.sh
5 files changed, 31 insertions(+), 3 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/00/8700/1

diff --git a/vds_bootstrap/vds_bootstrap.py b/vds_bootstrap/vds_bootstrap.py
index 867d6f4..9353daf 100755
--- a/vds_bootstrap/vds_bootstrap.py
+++ b/vds_bootstrap/vds_bootstrap.py
@@ -435,7 +435,7 @@
         self.message = ''
         self.rc = True

-        args = ['elevator=deadline']
+        args = []
         if rhel6based and not _constantTSC():
             args += ['processor.max_cstate=1']

diff --git a/vdsm.spec.in b/vdsm.spec.in
index 4814172..18a3661 100644
--- a/vdsm.spec.in
+++ b/vdsm.spec.in
@@ -382,9 +382,13 @@
 install -Dm 0755 vdsm/respawn \
                  %{buildroot}%{_datadir}/%{vdsm_name}/respawn

-# Install the lvm rules
+# Install the udev rules
 install -Dm 0644 vdsm/storage/12-vdsm-lvm.rules \
                  %{buildroot}/lib/udev/rules.d/12-vdsm-lvm.rules
+install -Dm 0644 vdsm/storage/12-vdsm-elevator.rules \
+                 %{buildroot}/lib/udev/rules.d/12-vdsm-elevator.rules
+install -Dm 0755 vdsm/storage/vdsm-elevator.sh \
+                 %{buildroot}/lib/udev/vdsm-elevator.sh

install -Dm 0644 vdsm/limits.conf \
                  %{buildroot}/etc/security/limits.d/99-vdsm.conf
@@ -721,6 +725,8 @@
 %{_datadir}/%{vdsm_name}/set-conf-item
 %{python_sitelib}/sos/plugins/vdsm.py*
 /lib/udev/rules.d/12-vdsm-lvm.rules
+/lib/udev/rules.d/12-vdsm-elevator.rules
+/lib/udev/vdsm-elevator.sh
 /etc/security/limits.d/99-vdsm.conf
 %{_mandir}/man8/vdsmd.8*
 %if 0%{?rhel}
diff --git a/vdsm/storage/12-vdsm-elevator.rules b/vdsm/storage/12-vdsm-elevator.rules
new file mode 100644
index 0000000..38a7607
--- /dev/null
+++ b/vdsm/storage/12-vdsm-elevator.rules
@@ -0,0 +1,12 @@
+#
+# Copyright 2012 Red Hat, Inc. and/or its affiliates.
+#
+# Licensed to you under the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.  See the files README and
+# LICENSE_GPL_v2 which accompany this distribution.
+#
+
+# Udev rules for elevator setting.
+
+ACTION=="add|change", SUBSYSTEM=="block", ENV{DEVTYPE}=="disk", RUN+="vdsm-elevator.sh deadline"
diff --git a/vdsm/storage/Makefile.am b/vdsm/storage/Makefile.am
index cff09be..5900802 100644
--- a/vdsm/storage/Makefile.am
+++ b/vdsm/storage/Makefile.am
@@ -65,7 +65,9 @@
    volume.py

EXTRA_DIST = \
-	12-vdsm-lvm.rules
+	12-vdsm-lvm.rules \
+	12-vdsm-elevator.rules \
+	vdsm-elevator.sh

check-local:
    PYTHONDONTWRITEBYTECODE=1 $(PYTHON) $(srcdir)/storage_exception.py
diff --git a/vdsm/storage/vdsm-elevator.sh b/vdsm/storage/vdsm-elevator.sh
new file mode 100755
index 0000000..6f2ff85
--- /dev/null
+++ b/vdsm/storage/vdsm-elevator.sh
@@ -0,0 +1,8 @@
+#!/bin/sh
+
+elevator="$1"
+scheduler="/sys/${DEVPATH}/queue/scheduler"
+
+if [ -w "${scheduler}" ]; then
+    echo "${elevator}" >  "${scheduler}"
+fi

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1: Verified

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg

Mark Wu has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1: I would prefer that you didn't submit this

(1 inline comment)

I agree with that the using udev is better than grub conf. Just a minor suggestion inline.

....................................................
File vdsm/storage/12-vdsm-elevator.rules
Line 8: #
Line 9: 
Line 10: # Udev rules for elevator setting.
Line 11: 
Line 12: ACTION=="add|change", SUBSYSTEM=="block", ENV{DEVTYPE}=="disk", RUN+="vdsm-elevator.sh deadline"
I think we can just use a command line instead of adding new script, like:
RUN+="/bin/sh -c 'echo deadline > /sys$DEVPATH/queue/scheduler'"

For dm devices, we can filter out them by udev key word or just write it unconditionally.

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/storage/12-vdsm-elevator.rules
Line 8: #
Line 9: 
Line 10: # Udev rules for elevator setting.
Line 11: 
Line 12: ACTION=="add|change", SUBSYSTEM=="block", ENV{DEVTYPE}=="disk", RUN+="vdsm-elevator.sh deadline"
1. I don't like shell being executed in udev, too many thing can go wrong.

2. there are no other rules this way.

3. It harder to reuse... for example, user may want to revert:

ACTION=="add|change", SUBSYSTEM=="block", ENV{DEVTYPE}=="disk", KERNEL='sda',RUN+="vdsm-elevator.sh cfq"

Why do you need to filter dm devices? This will just add more complexity.

Thanks.

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

Adam Litke has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1:

ping?

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

Ayal Baron has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1:

I know that we've discussed this, but reviewing the patch (which looks fine by the way) I still think the right solution is to change the default scheduler in boot parameter.

It easily allows user to:
1. change default scheduler
2. target specific devices and change the scheduler.
Both of the above would not require the user to know about vdsm specific udev rules.

In addition, the current rule targets all block devices in the system and if users already have code that changes the scheduler for some devices, this would override it and they might not even be aware of the change (but suffer from a performance hit).

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1:

Thank you Ayal,

Changing boot parameters is distribution and setup dependent. We cannot do this in deterministic way.

Changing this in udev rule does allow the customization you wish to have. Changing the default kernel setting is more intrusive.

The new vdms-bootstrap implementation is not messing up with the bootloader as it needs to be both deterministic and platform independent, please apply this patch.

Thanks,
Alon

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1:

Ayal, I will appreciate your response in shorter cycles.

I prefer this to go to vdsm as all users who install vdsm will enjoy it. It is simple, standard, flexible, customizable, cross platform approach.

Otherwise, I will put this rule in bootstrap. Which is the wrong component for this in my opinion, but will provide the desired solution. It will be sad to see ugly leftovers after the rewrite.

Thanks,
Alon.

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1:

Alon, I think that what troubles Ayal is that currently, if someone was not happy with our block scheduler of choice, he could edit his /etc/grub.conf, and walk away.

With your patch, this is impossible. We change the system scheduler, and as long as vdsm is installed, that's that.

Maybe, if you can find a way to limit the udev rule to ovirt-owned block devices, Ayal would have been happier. (I do not have a shred of an idea how to do this properly. How about a user-editable blacklist of devices were vdsm-elevator.rules does not apply?)

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1:

Hi,

If someone wanted to override the setting for specific disk, he would have written udev rule and not change kernel command-line.

And even if this was the case, we can name our rule to be loaded as earliest to allow existing rule override.

If someone changed the kernel command-line manually he violated our support profile, as we assume deadline, and assume vdsm is a SLAVE hypervisor bootstrapped using supported method.

But if that is *REALLY* what bothering you (someone manually set the kernel command-line with unsupported setting), then I can read the /proc/cmdline in the udev script and simply do nothing if a setting in the kernel command-line exists. Maybe we should also support people who compile their own rhel kernel with different default?

BTW: Current bootstrap implementation will override this manual setting anyway during bootstrap, so we do not support manual changes. But let's ignore reality for now.

I think it is a mistake to go in this route, as we were the one who messed with kernel command-line in the first place. This was invalid decision that was taken at that time. Had we done this today we would have used udev to achieve the same. Because we had done this in kernel command-line we forced the user to [maybe] customize it at same place instead of flexible udev rule. Leaving this in kernel command-line because of previous poor decision is not a great argument if we want to clean up this bootstrap corner.

We can have a text within release notes to alert these advanced users to move their customization to udev, so the 3 (max) people who are effected will know what to do.

As I wrote previously, the question is which component is the owner of setting the iosched, vdsm or bootstrap. If this is rejected from vdsm and go into bootstrap domain, it will be implemented as udev rule too, I believe this belongs to vdsm.

Alon

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1:
> How about a user-editable blacklist of devices were vdsm-elevator.rules does not apply?

As I wrote in the inline comment... you just copy our rule to a different name with higher number and add the statement of your device, for example:

ACTION=="add|change", SUBSYSTEM=="block", ENV{DEVTYPE}=="disk", KERNEL='sda',RUN+="vdsm-elevator.sh cfq"

So if we use udev, we do not actually need to re-invent the wheel of rule base processing... it is all there.

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1: Looks good to me, but someone else must approve

I thought that it's a bit messy to run vdsm-elevator multiple times on non-vdsm devices - it would be nicer to call our script on out devices only.

I'm not advocating the use of kernel command line, I'm saying that it had a planned use case that your suggested solution make a bit harder.

(and I'm not completely sure that this is Ayal's biggest concern, let him speak up)

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1:
> it would be nicer to call our script on out devices only

Sure it will be nice, I totally agree. But the enemy of the good is the best.

Moving global setting from kernel command-line to udev is one step, it improves the integration, determinism and the complexity of the bootstrap process. It does not effect the final outcome.

Accepting this patch is in no way inhibits the next improvement you have in mind. I will be very glad we effect only devices we manage.

There is nothing wrong in stepping small step forward. This step is not for the sake of making the outcome better, but the process.

Next step can improve the outcome.

Thanks,

Alon

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

Juan Hernandez has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1: Looks good to me, but someone else must approve

(1 inline comment)

I think this is a good step forward. I would suggest to add a comment to the vdsm-elevator.sh script explaining the current limitations (modifies all devices, modifies dm devices, etc) so we can remember later to improve it.

....................................................
File vdsm/storage/vdsm-elevator.sh
Line 3: elevator="$1"
Line 4: scheduler="/sys/${DEVPATH}/queue/scheduler"
Line 5: 
Line 6: if [ -w "${scheduler}" ]; then
Line 7:     echo "${elevator}" >  "${scheduler}"
Why the extra space after > ?

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1: (1 inline comment)

Thanks! although the comment should not be in vdsm-elevator.sh as it does not actually set anything... it should be in the rule.

If you provide an exact wording I will prepare another version...

Draft:

# NOTICE: this rule set the block scheduler elevator to deadline on all block device.
 # this is required for optimizing performance for vdsm operations.

....................................................
File vdsm/storage/vdsm-elevator.sh
Line 3: elevator="$1"
Line 4: scheduler="/sys/${DEVPATH}/queue/scheduler"
Line 5: 
Line 6: if [ -w "${scheduler}" ]; then
Line 7:     echo "${elevator}" >  "${scheduler}"
I can fix that... :)

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

Ayal Baron has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1:

My problem is that previously we changed the *default* only and that it was sufficient for a user to echo a different scheduler into "/sys/${DEVPATH}/queue/scheduler" to override (either with udev rule or with a cronjob or as a one time change to test different configurations or any other way).
Now with your solution, the only way a user can override our setting is by writing a udev rule which would override our rule (so not enough to write a rule but have to place it properly relative to ours).
Stating that if we don't push it into vdsm you'll brute force and push it into bootstrap is not the way to go.

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1:

Hello,

We change the default, just using udev to do that.

User can still use EXISTING udev rule or cronjob to override the setting, nothing was changed!

Yes, writing udev is priority based, whoever writes such a rule needs to be aware of the ordering. If this *REALLY* bothers you and you think people that uses udev do not know what they are doing we can put our rule at level 00, so it runs first.

However, I do believe people know what they are doing, and we can count these people who overridden scheduler on one hand.

Anyway is better than placing this udev rule in bootstrap anyway. So I find the suggested solution to be just fine.

Alon

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

Ayal Baron has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1:
> We change the default, just using udev to do that.

udev rules are evaluated after every scan so our rule would override anything the user has in place unless user uses a udevrule to override ours.
> User can still use EXISTING udev rule or cronjob to override the setting, nothing was changed!
> Yes, writing udev is priority based, whoever writes such > a rule needs to be aware of the ordering. If this 
> *REALLY* bothers you and you think people that uses udev > do not know what they are doing we can put our rule at 
> level 00, so it runs first.

It's not that they don't know what they're doing, it's that our rule did not previously exist for them to deal with and that the user now needs to know it exists.

it's that a yum update will now override user settings
and users might not even notice it until someone complains about bad performance (and then start analyzing it).
> Anyway is better than placing this udev rule in bootstrap > anyway. So I find the suggested solution to be just fine.

what suggested solution is that?

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1:

Ayal,

How many people you estimate overridden the setting in udev rule with order less or equal to 12? Is this discussion is practical or theoretical? Won't a release not will resolve the X% that had customization and somehow had order less than 12 (which is really high ordering for customization)?

The solution will be udev rule anyway, either I put it in bootstrap or we apply this patch. New bootstrap implementation will not mess with kernel command-line either case.

Thank you,
Alon

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1:

Ayal, please also refer you to my previous suggestion as well disabling the rule if the kernel command-line exists.

Meaning: if we previously bootstrapped the machine we put the parameter in command-line (or user may changed its value to override), in this case the udev rule becomes void.

This is non standard, unexpected mixed mode solution, but if this helps to push this forward...

Alon

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1:

Another option that may make sense to you is to put udev rule only on fresh rpm install and not upgrade...

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

Ayal Baron has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 1:

I'd +2 that (udev rule on fresh install).

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

Ayal Baron has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm.spec.in
Line 477: 
Line 478: #
Line 479: # Mark the existance of the rules before upgrade
Line 480: #
Line 481: rules="/lib/udev/rules.d/12-vdsm-elevator.rules"
I may be missing something but how would this file exist before installing? this patch introduces the file for the first time...
For upgrade shouldn't you be checking for the existence of: 12-vdsm-lvm.rules and if it exists delete 12-vdsm-elevator.rules (i.e. reverse the logic below in %post)
Line 482: if [ "$1" = 2 -a -f "${rules}" ]; then
Line 483:     touch "${rules}.lock"
Line 484: fi
Line 485:

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm.spec.in
Line 477: 
Line 478: #
Line 479: # Mark the existance of the rules before upgrade
Line 480: #
Line 481: rules="/lib/udev/rules.d/12-vdsm-elevator.rules"
There are two sequences we need to support:

vdsm-without-udev installed
 vdsm-with-udev upgrade
 result=no rule

vdsm-with-udev installed
 vdsm-with-udev upgrade
 result=rule of upgrade

So if udev rule exists before upgrade we keep it after upgrade.

If udev rule is missing before upgrade we remove it after upgrade.

I don't really understand your command, but facing the above, where do you see a problem?
Line 482: if [ "$1" = 2 -a -f "${rules}" ]; then
Line 483:     touch "${rules}.lock"
Line 484: fi
Line 485:

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi

Ayal Baron has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 2: (2 inline comments)

....................................................
File vdsm.spec.in
Line 478: #
Line 479: # Mark the existance of the rules before upgrade
Line 480: #
Line 481: rules="/lib/udev/rules.d/12-vdsm-elevator.rules"
Line 482: if [ "$1" = 2 -a -f "${rules}" ]; then
Ack on the above, I previously missed that you limit this to upgrade only.
Line 483:     touch "${rules}.lock"
Line 484: fi
Line 485: 
Line 486: %post

Line 515:     rules="/lib/udev/rules.d/12-vdsm-elevator.rules"
Line 516:     if [ -f "${rules}.lock" ]; then
Line 517:         rm -f "${rules}.lock"
Line 518:     else
Line 519:         rm -f "${rules}"
why do you keep the lock file in this case?
wouldn't this cause the subsequent upgrade to install the rules?
Line 520:     fi
Line 521: fi
Line 522: 
Line 523: exit 0

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm.spec.in
Line 515:     rules="/lib/udev/rules.d/12-vdsm-elevator.rules"
Line 516:     if [ -f "${rules}.lock" ]; then
Line 517:         rm -f "${rules}.lock"
Line 518:     else
Line 519:         rm -f "${rules}"
Right. this is the whole point. Had we done this all over again, we would have just put the rules. Every time the package was updated the rules were also updated.

This is what differ rules that ar within /lib/udev and /etc/udev... they are controlled by the package not the user.
Line 520:     fi
Line 521: fi
Line 522: 
Line 523: exit 0

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi

Ayal Baron has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 2: Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 2:

Hello All,

Recently I was asked to add tuned and set virtualization profile during bootstrap.

Tuned performance profile which is dependency of virtualization profiles sets the deadline queue manager.

Looking at the tuned sources it seems like it does not interact with udev, and I could not find any poll loop, so I don't know if it has any ability to detect dynamic devices. If it does, than we should evaluate if vdsm should or should not assume that there is an tuning entity such as tuned and do not perform any customization.

Alon

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 2:

Noam: I need an answer to the above.

Thanks!

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Doron Fediuck 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Noam Slomianko 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi

Noam Slomianko has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 2:

Seems documentation on the tune package is out of date and needs to be updated, but looking at usr/lib/tuned/virtual-host/tuned.conf it seems that it doesn't do much besides changing memory management priorities.

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Doron Fediuck 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Noam Slomianko 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 2:

No... it has parent profile of performance which sets deadline scheduler.

The question is if it also applies to dynamic volumes. You can try this easily using usb mass storage device.

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Doron Fediuck 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Noam Slomianko 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 2:

People: the tuned does not currently support dynamic apply of policy, thus any new block device will not be set up using its policy.

Two options:

1. We can expect this setting to be done out-side of vdsm by tool X (as tuned) and so bootstrap code is responsible for setting up the system, currently it will put this udev rule.

2. We can argue that this setting is so important that we want to enforce it for all vdsm installations.

What do you think?

Alon

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Doron Fediuck 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Noam Slomianko 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi

Doron Fediuck has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 2:

Can we use cron to re-apply the policy using tuend or similar until tuned is fixed?

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Doron Fediuck 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Noam Slomianko 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 2:
> Can we use cron to re-apply the policy using tuend or similar until tuned is fixed?

Polling wastes CPU and power, this is not a valid solution. If tuned is not ready for prime time we should not use it.

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Doron Fediuck 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Noam Slomianko 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi

[Alon Bar-Lev]

Alon Bar-Lev has abandoned this change.

Change subject: storage: set block schedule elevator using udev
......................................................................

Patch Set 2: Abandoned

Consider tuned or any other host tuning mechanism future, I will put similar rule within bootstrap process until these tools stabilize.

--
To view, visit http://gerrit.ovirt.org/8700
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I0a8de1c861bf4570509599b6f47235ed38cc424d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Alon Bar-Lev 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Doron Fediuck 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Itamar Heim 
Gerrit-Reviewer: Juan Hernandez 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Noam Slomianko 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi