Change in vdsm[master]: Don't go through lvm for volume size if lv is active

Saggi Mizrahi has uploaded a new change for review.

Change subject: Don't go through lvm for volume size if lv is active
......................................................................

Don't go through lvm for volume size if lv is active

Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
---
M vdsm/storage/blockVolume.py
1 file changed, 8 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/77/677/1
--
To view, visit http://gerrit.ovirt.org/677
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#769502 Do not use lvm to get lv size if lv is active

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: BZ#769502 Do not use lvm to get lv size if lv is active
......................................................................


BZ#769502 Do not use lvm to get lv size if lv is active

Calling LVM is expensive so the less we use it the better.

Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
---
M vdsm/storage/blockVolume.py
1 file changed, 15 insertions(+), 0 deletions(-)

Approvals:
  Dan Kenigsberg: Verified; Looks good to me, approved


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

Gerrit-MessageType: merged
Gerrit-Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#769502 Do not use lvm to get lv size if lv is active

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#769502 Do not use lvm to get lv size if lv is active
......................................................................


Patch Set 8: Verified; Looks good to me, approved

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#769502 Do not use lvm to get lv size if lv is active

Ayal Baron has posted comments on this change.

Change subject: BZ#769502 Do not use lvm to get lv size if lv is active
......................................................................


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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#769502 Do not use lvm to get lv size if lv is active

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#769502 Do not use lvm to get lv size if lv is active
......................................................................


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

(1 inline comment)

just a little question

....................................................
File vdsm/storage/blockVolume.py
Line 85:         if lvm.isLVActive(sdobj.sdUUID, volUUID):
why do you bother to test isLVActive? just try to _getDviceSize and get ENOENT if the lv
is inactive.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#769502 Do not use lvm to get lv size if lv is active

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#769502 Do not use lvm to get lv size if lv is active
......................................................................


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

I do not understand why you need all this noise about _isLVActive becoming isLVActive.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: Do not use lvm to get lv size if lv is active

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Do not use lvm to get lv size if lv is active
......................................................................




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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: Do not use lvm to get lv size if lv is active

Ayal Baron has posted comments on this change.

Change subject: Do not use lvm to get lv size if lv is active
......................................................................


Patch Set 4: No score

Removing my -1 as I don't feel strongly about it, however the _isLVActive function
specifically stated it is meant to be private, I'm leaving it to Eduardo to reason why
and nack if necessary.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: Do not use lvm to get lv size if lv is active

Saggi Mizrahi has posted comments on this change.

Change subject: Do not use lvm to get lv size if lv is active
......................................................................


Patch Set 4: (1 inline comment)

....................................................
File vdsm/storage/blockVolume.py
Line 85:         if lvm.isLVActive(sdobj.sdUUID, volUUID):
There is no logical reason to have this limitation.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: Do not use lvm to get lv size if lv is active

Ayal Baron has posted comments on this change.

Change subject: Do not use lvm to get lv size if lv is active
......................................................................


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

(1 inline comment)

....................................................
File vdsm/storage/blockVolume.py
Line 85:         if lvm.isLVActive(sdobj.sdUUID, volUUID):
again, why call 'isLVActive' at all?
No need to make it public and _getDeviceSize would just fail (we're not likely to
reach this flow when lv is not active anyway)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: Do not use lvm to get lv size if lv is active

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Do not use lvm to get lv size if lv is active
......................................................................


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

(1 inline comment)

....................................................
File vdsm/storage/blockVolume.py
Line 85:         if lvm.isLVActive(sdobj.sdUUID, volUUID):
I believe Ayal wanted to remove this check. Probably something like:

 try:
     return _getDeviceSize(...)
 except:
     return int(int(lvm.getLV(...)))

But in this way we probably lose some feedback about what method was supposed to work.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: Do not use lvm to get lv size if lv is active

Ayal Baron has posted comments on this change.

Change subject: Do not use lvm to get lv size if lv is active
......................................................................


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

(1 inline comment)

....................................................
File vdsm/storage/blockVolume.py
Line 86:             return _getDeviceSize(lvm.lvPath(sdobj.sdUUID, volUUID)) / bs
you should catch and failback to getLV in which case you don't even need to test if
isLVActive and then the main path is even faster.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Saggi Mizrahi has posted comments on this change.

Change subject: Don't go through lvm for volume size if lv is active
......................................................................


Patch Set 1: (2 inline comments)

....................................................
File vdsm/storage/blockVolume.py
Line 60: 
It will get thrown up? I don't see why I should care?
The getLV() function can fail as well?

Line 87: 
Should is a name of a fish. The reality is it isn't. VMs keep calling refresh before
checking the size (which is perfectly acceptable IMHO). Anyway, there is no reason not do
this. The less we use lvm the better.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Eduardo has posted comments on this change.

Change subject: Don't go through lvm for volume size if lv is active
......................................................................


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

(1 inline comment)

....................................................
File vdsm/storage/blockVolume.py
Line 87: 
If the LV is active should be cached. If a real call is done the issue is why the cache
was invalidated.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;

Adam Litke has posted comments on this change.

Change subject: Don't go through lvm for volume size if lv is active
......................................................................


Patch Set 1: (1 inline comment)

....................................................
File vdsm/storage/blockVolume.py
Line 60: 
It shouldn't ever happen, but what would happen to an exception if it happened here?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;

Change in vdsm[master]: Do not use lvm to get lv size if lv is active

Ayal Baron has posted comments on this change.

Change subject: Do not use lvm to get lv size if lv is active
......................................................................


Patch Set 3: (1 inline comment)

....................................................
File vdsm/storage/blockVolume.py
Line 85:         if lvm.isLVActive(sdobj.sdUUID, volUUID):
I see no point in going through lvm cache here at all.  The positive path is that
we're getting the size because this is a disk that belongs to an active domain so
calling lvm is redundant.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e81bf563f07119a775888d6abc3c81f4f7b86a0
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;