[Libstoragemgmt-devel] [PATCH] Target plugin: Fix volume_create() when pass size less than 512

Discussion:

Gris Ge

2014-09-04 15:48:47 UTC

* Targetd is using LibLVM which do 'size / SECTOR_SIZE'. (SECTOR_SIZE == 512)
This will cause two issue:
1. When less than 512, LibLVM will raise error:
Unable to create LV without size.
2. New volume size is always less or equal to requested.
But LVM definition require new volume size bigger or equal than
requested.

* Just do a 512 round up would workaround this issue.

Signed-off-by: Gris Ge <***@redhat.com>
---
plugin/targetd/targetd.py | 3 +++
1 file changed, 3 insertions(+)

diff --git a/plugin/targetd/targetd.py b/plugin/targetd/targetd.py
index 00fcf49..a144685 100644
--- a/plugin/targetd/targetd.py
+++ b/plugin/targetd/targetd.py
@@ -35,6 +35,7 @@ DEFAULT_USER = "admin"
DEFAULT_PORT = 18700
PATH = "/targetrpc"

+LVM_SECTOR_SIZE = 1 << 9

def handle_errors(method):
def target_wrapper(*args, **kwargs):
@@ -334,6 +335,8 @@ class TargetdStorage(IStorageAreaNetwork, INfs):
if provisioning != Volume.PROVISION_DEFAULT:
raise LsmError(ErrorNumber.INVALID_ARGUMENT,
"Unsupported provisioning")
+ if size_bytes % LVM_SECTOR_SIZE:
+ size_bytes += 512

self._jsonrequest("vol_create", dict(pool=pool.id,
name=volume_name,

--
1.9.3

Tony Asleson

2014-09-04 22:23:44 UTC

Permalink

Is there some benefit doing this vs. just using 512 as the value?
Please make it private too _LVM_SECTOR_SIZE.

Post by Gris Ge
raise LsmError(ErrorNumber.INVALID_ARGUMENT,
"Unsupported provisioning")
+ size_bytes += 512

I believe it would be better to do:

if size_bytes:
remainder = size_bytes % _LVM_SECTOR_SIZE
if remainder:
size_bytes = size_bytes + _LVM_SECTOR_SIZE - remainder
else:
size_bytes = _LVM_SECTOR_SIZE

Then we aren't passing values like 513 to targetd, even if it does end
in the same ultimate result. Also if a client application passed 0, we
would pass along 0 to targetd too.

However, I'm still thinking that the minimum size for a lv is going to
be greater than 1 sector.

Regards,
Tony

Gris Ge

2014-09-12 06:18:46 UTC

Permalink

* Targetd is using LibLVM which do 'size / SECTOR_SIZE'. (SECTOR_SIZE == 512)
This will cause two issue:
1. When less than 512, LibLVM will raise error:
Unable to create LV without size.
2. New volume size is always less or equal to requested.
But LVM definition require new volume size bigger or equal than
requested.

* Just do a 512 round up would workaround this issue.

Changes in V2(Thanks to Tony for suggestions)
* Use 512 instead of 1 >> 9.
* Round up size to 512 before passing to targetd.

Signed-off-by: Gris Ge <***@redhat.com>
---
plugin/targetd/targetd.py | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/plugin/targetd/targetd.py b/plugin/targetd/targetd.py
index 00fcf49..b40aa4c 100644
--- a/plugin/targetd/targetd.py
+++ b/plugin/targetd/targetd.py
@@ -35,6 +35,8 @@ DEFAULT_USER = "admin"
DEFAULT_PORT = 18700
PATH = "/targetrpc"

+# Current sector size in liblvm
+_LVM_SECTOR_SIZE = 512

def handle_errors(method):
def target_wrapper(*args, **kwargs):
@@ -335,6 +337,14 @@ class TargetdStorage(IStorageAreaNetwork, INfs):
raise LsmError(ErrorNumber.INVALID_ARGUMENT,
"Unsupported provisioning")

+ # Make sure size_bytes round up with _LVM_SECTOR_SIZE
+ if size_bytes:
+ remainder = size_bytes % _LVM_SECTOR_SIZE
+ if remainder:
+ size_bytes = size_bytes + _LVM_SECTOR_SIZE - remainder
+ else:
+ size_bytes = _LVM_SECTOR_SIZE
+
self._jsonrequest("vol_create", dict(pool=pool.id,
name=volume_name,
size=size_bytes))

--
1.8.3.1

Tony Asleson

2014-09-12 15:23:09 UTC

Permalink

This looks good, however the minimum size of a LV is the size of a
physical extent (PE), which defaults to 4MiB.

In my opinion lets rename _LVM_SECTOR_SIZE to

_LVM_DEFAULT_PE_SIZE = 4194304

and go with that.

Thanks,
Tony

Post by Gris Ge
* Targetd is using LibLVM which do 'size / SECTOR_SIZE'. (SECTOR_SIZE == 512)
Unable to create LV without size.
2. New volume size is always less or equal to requested.
But LVM definition require new volume size bigger or equal than
requested.
* Just do a 512 round up would workaround this issue.
Changes in V2(Thanks to Tony for suggestions)
* Use 512 instead of 1 >> 9.
* Round up size to 512 before passing to targetd.
---
plugin/targetd/targetd.py | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/plugin/targetd/targetd.py b/plugin/targetd/targetd.py
index 00fcf49..b40aa4c 100644
--- a/plugin/targetd/targetd.py
+++ b/plugin/targetd/targetd.py
@@ -35,6 +35,8 @@ DEFAULT_USER = "admin"
DEFAULT_PORT = 18700
PATH = "/targetrpc"
+# Current sector size in liblvm
+_LVM_SECTOR_SIZE = 512
raise LsmError(ErrorNumber.INVALID_ARGUMENT,
"Unsupported provisioning")
+ # Make sure size_bytes round up with _LVM_SECTOR_SIZE
+ remainder = size_bytes % _LVM_SECTOR_SIZE
+ size_bytes = size_bytes + _LVM_SECTOR_SIZE - remainder
+ size_bytes = _LVM_SECTOR_SIZE
+
self._jsonrequest("vol_create", dict(pool=pool.id,
name=volume_name,
size=size_bytes))

Gris Ge

2014-09-13 05:29:33 UTC

Permalink

Post by Tony Asleson
This looks good, however the minimum size of a LV is the size of a
physical extent (PE), which defaults to 4MiB.
In my opinion lets rename _LVM_SECTOR_SIZE to
_LVM_DEFAULT_PE_SIZE = 4194304

The PE size is the configurable when creating PV.

What if user configure their PE size as 1MiB, then they should expecting
a 1MiB LV when requesting a 1MiB LV via libstoragemgmt.

We should let that targetd or libLVM to handle that.
Actually, with current code of targetd and libLVM, if we request 512B,
we get 4MiB.

This patch is just a workaround on the different size strategy:
* LSM -- Return larger or equal size.
* Targetd(libLVM) -- Return equal or less size.

Post by Tony Asleson
Thanks,
Tony

Thanks.
Best regards.

--
Gris Ge

Tony Asleson

2014-09-15 18:40:50 UTC

Permalink

I see your point, your V2 patch has been committed.

Thanks,
Tony

Post by Gris Ge

The PE size is the configurable when creating PV.
What if user configure their PE size as 1MiB, then they should expecting
a 1MiB LV when requesting a 1MiB LV via libstoragemgmt.
We should let that targetd or libLVM to handle that.
Actually, with current code of targetd and libLVM, if we request 512B,
we get 4MiB.
* LSM -- Return larger or equal size.
* Targetd(libLVM) -- Return equal or less size.

Post by Tony Asleson
Thanks,
Tony

Thanks.
Best regards.

Tony Asleson

2014-09-15 18:40:14 UTC

Permalink

Patch committed.

Thanks,
Tony