[PATCH V3 0/2] Add thin lv and thin pool creation support

[PATCH V3 0/2] Add thin lv and thin pool creation support

[M. Mohan Kumar]

From: "M. Mohan Kumar" <mohan@in.ibm.com>

Add thin lv and thin pool creation support to lvm2app.

Changes from V2:
* Specify percentage for low water mark threshold instead of number of
  blocks
* man page update for lvcreate

Changes from previous version:
* Add support to specify data block size and low water mark thresold for
newly created thin pool.
* Added support for specifying lowwatermark parameter to thin pool
creation in lvcreate command

M. Mohan Kumar (2):
  lvm2app: Add thin and thin pool lv creation
  master - thin: Add low water mark parameter to pool creation

 lib/metadata/lv_manip.c          |   2 +-
 lib/metadata/metadata-exported.h |   1 +
 liblvm/lvm2app.h                 |  57 +++++++++++++++++++
 liblvm/lvm_lv.c                  | 116 +++++++++++++++++++++++++++++++++++++++
 man/lvcreate.8.in                |  11 +++-
 tools/args.h                     |   1 +
 tools/commands.h                 |   6 +-
 tools/lvconvert.c                |   1 +
 tools/lvcreate.c                 |   7 ++-
 tools/toollib.c                  |  11 ++++
 tools/toollib.h                  |   1 +
 11 files changed, 209 insertions(+), 5 deletions(-)

-- 
1.7.11.7

[PATCH 1/2] lvm2app: Add thin and thin pool lv creation

[M. Mohan Kumar]

From: "M. Mohan Kumar" <mohan@in.ibm.com>

Add thin and thin pool lv creation support to lvm library

Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
 lib/metadata/lv_manip.c          |   2 +-
 lib/metadata/metadata-exported.h |   1 +
 liblvm/lvm2app.h                 |  57 +++++++++++++++++++
 liblvm/lvm_lv.c                  | 116 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 175 insertions(+), 1 deletion(-)

diff --git a/lib/metadata/lv_manip.c b/lib/metadata/lv_manip.c
index c1437eb..4dd5ebf 100644
--- a/lib/metadata/lv_manip.c
+++ b/lib/metadata/lv_manip.c
@@ -4518,7 +4518,7 @@ static struct logical_volume *_lv_create_an_lv(struct volume_group *vg, struct l
 		first_seg(lv)->chunk_size = lp->chunk_size;
 		first_seg(lv)->discards = lp->discards;
 		/* FIXME: use lowwatermark  via lvm.conf global for all thinpools ? */
-		first_seg(lv)->low_water_mark = 0;
+		first_seg(lv)->low_water_mark = lp->low_water_mark;
 	} else if (seg_is_thin_volume(lp)) {
 		pool_lv = first_seg(lv)->pool_lv;
 
diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index ffec129..40519f5 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -617,6 +617,7 @@ struct lvcreate_params {
 	uint64_t voriginsize; /* snapshot */
 	uint32_t poolmetadataextents; /* thin pool */
 	uint64_t poolmetadatasize; /* thin pool */
+	uint64_t low_water_mark;  /* thin_pool */
 	struct dm_list *pvh; /* all */
 
 	uint32_t permission; /* all */
diff --git a/liblvm/lvm2app.h b/liblvm/lvm2app.h
index 93a78c3..1a91f64 100644
--- a/liblvm/lvm2app.h
+++ b/liblvm/lvm2app.h
@@ -1400,6 +1400,63 @@ int lvm_lv_resize(const lv_t lv, uint64_t new_size);
  */
 lv_t lvm_lv_snapshot(const lv_t lv, const char *snap_name, uint64_t max_snap_size);
 
+
+/**
+ * Create a thinpool in a given VG
+ *
+ * \param   vg
+ * Volume Group handle.
+ *
+ * \param   pool_name
+ * Name of the pool.
+ *
+ * \param   size
+ * size of the pool
+ *
+ * \param   chunk_size
+ * data block size of the pool
+ * DM_THIN_MIN_DATA_BLOCK_SIZE < chunk_size < DM_THIN_MAX_DATA_BLOCK_SIZE
+ *
+ * \param  threshold in percentage
+ * When percentage of free blocks in the pool reaches <= this thresold a dm
+ * event is sent. For example if threshold is specified 25, an dm event will be
+ * generated when the percentage of free blocks goes <= 25%.
+ *
+ * Note: when 0 is passed as threshold, an event will be generated only when all
+ * blocks are consumed in the pool.
+ *
+ * \return
+ * Valid lv pointer on success, else NULL on error.
+ *
+ */
+lv_t lvm_lv_thinpool(const vg_t vg, const char *pool_name, uint64_t size,
+		     uint32_t chunk_size, int threshold);
+
+/**
+ * Create a thin LV in a given VG & thin pool
+ *
+ * \param   vg
+ * Volume Group handle.
+ *
+ * \param   pool_name
+ * Name of the pool.
+ *
+ * \param lvname
+ * Name of the LV to create
+ *
+ * \param   size
+ * Size of logical volume
+ *
+ * \return
+ * Valid lv pointer on success, else NULL on error.
+ *
+ */
+
+lv_t lvm_lv_thin(const vg_t vg, const char *pool_name, const char *lvname,
+		uint64_t size);
+
+
+
 /************************** physical volume handling ************************/
 
 /**
diff --git a/liblvm/lvm_lv.c b/liblvm/lvm_lv.c
index 91948a6..ecb5f54 100644
--- a/liblvm/lvm_lv.c
+++ b/liblvm/lvm_lv.c
@@ -350,3 +350,119 @@ lv_t lvm_lv_snapshot(const lv_t lv, const char *snap_name, uint64_t max_snap_siz
 		return NULL;
 	return (lv_t) lvl->lv;
 }
+
+/* Set defaults for thin pool specific LV parameters */
+static void _lv_set_pool_params(struct lvcreate_params *lp,
+				vg_t vg, const char *pool,
+				   uint64_t extents)
+{
+	_lv_set_default_params(lp, vg, NULL, extents);
+
+	lp->pool = pool;
+
+	lp->create_thin_pool = 1;
+	lp->segtype = get_segtype_from_string(vg->cmd, "thin-pool");
+	lp->stripes = 1;
+	lp->poolmetadatasize = extents * vg->extent_size / lp->chunk_size *
+		(SECTOR_SIZE / UINT64_C(64));
+	lp->poolmetadataextents = extents_from_size(vg->cmd, lp->poolmetadatasize,
+						   vg->extent_size);
+}
+
+lv_t lvm_lv_thinpool(const vg_t vg, const char *pool, uint64_t size,
+		     uint32_t chunk_size, int threshold)
+{
+	struct lvcreate_params lp = { 0 };
+	uint64_t extents = 0;
+	struct lv_list *lvl = NULL;
+	uint64_t lwmb = 0;
+
+	if (vg_read_error(vg))
+		return NULL;
+	if (!vg_check_write_mode(vg))
+		return NULL;
+
+	if (threshold < 0 || threshold > 100) {
+		log_error("Invalid threshold, should be between 0-100");
+		return NULL;
+	}
+	if (!(extents = extents_from_size(vg->cmd, size / SECTOR_SIZE,
+					  vg->extent_size))) {
+		log_error("Unable to create LV thin pool without size.");
+		return NULL;
+	}
+
+	if (chunk_size)
+		lp.chunk_size = chunk_size;
+	else
+		lp.chunk_size = DEFAULT_THIN_POOL_CHUNK_SIZE * 2;
+
+	if (lp.chunk_size < DM_THIN_MIN_DATA_BLOCK_SIZE ||
+	    lp.chunk_size > DM_THIN_MAX_DATA_BLOCK_SIZE) {
+		log_error("Invalid chunk_size");
+		return NULL;
+	}
+
+	_lv_set_pool_params(&lp, vg, pool, extents);
+
+	/*
+	  threshold specified in percentage, convert that into
+	  number of blocks
+	*/
+	lp.low_water_mark = size / chunk_size * threshold / 100;
+	if (!lp.segtype)
+		return_NULL;
+	if (!lv_create_single(vg, &lp))
+		return_NULL;
+	if (!(lvl = find_lv_in_vg(vg, pool)))
+		return NULL;
+	return (lv_t) lvl->lv;
+}
+
+/* Set defaults for thin LV specific parameters */
+static void _lv_set_thin_params(struct lvcreate_params *lp,
+				vg_t vg, const char *pool,
+				const char *lvname,
+				uint64_t extents)
+{
+	_lv_set_default_params(lp, vg, lvname, extents);
+
+	lp->thin = 1;
+	lp->pool = pool;
+	lp->segtype = get_segtype_from_string(vg->cmd, "thin");
+
+	lp->voriginsize = extents * vg->extent_size;
+	lp->voriginextents = extents_from_size(vg->cmd, lp->voriginsize,
+						   vg->extent_size);
+
+	lp->stripes = 1;
+}
+
+lv_t lvm_lv_thin(const vg_t vg, const char *pool,
+		 const char *lvname, uint64_t size)
+{
+	struct lvcreate_params lp = { 0 };
+	uint64_t extents = 0;
+	struct lv_list *lvl = NULL;
+
+	if (vg_read_error(vg))
+		return NULL;
+	if (!vg_check_write_mode(vg))
+		return NULL;
+
+	if (!(extents = extents_from_size(vg->cmd, size / SECTOR_SIZE,
+					  vg->extent_size))) {
+		log_error("Unable to create thin LV without size.");
+		return NULL;
+	}
+
+	_lv_set_thin_params(&lp, vg, pool, lvname, extents);
+
+	if (!lp.segtype)
+		return_NULL;
+	if (!lv_create_single(vg, &lp))
+		return_NULL;
+	if (!(lvl = find_lv_in_vg(vg, pool)))
+		return NULL;
+	return (lv_t) lvl->lv;
+}
-- 
1.7.11.7

[PATCH 1/2] lvm2app: Add thin and thin pool lv creation

[Zdenek Kabelac]

Dne 12.2.2013 18:24, M. Mohan Kumar napsal(a):
> From: "M. Mohan Kumar" <mohan@in.ibm.com>
>
> Add thin and thin pool lv creation support to lvm library
>
> Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
> ---
>   lib/metadata/lv_manip.c          |   2 +-
>   lib/metadata/metadata-exported.h |   1 +
>   liblvm/lvm2app.h                 |  57 +++++++++++++++++++
>   liblvm/lvm_lv.c                  | 116 +++++++++++++++++++++++++++++++++++++++
>   4 files changed, 175 insertions(+), 1 deletion(-)
>
> diff --git a/lib/metadata/lv_manip.c b/lib/metadata/lv_manip.c
> index c1437eb..4dd5ebf 100644
> --- a/lib/metadata/lv_manip.c
> +++ b/lib/metadata/lv_manip.c
> @@ -4518,7 +4518,7 @@ static struct logical_volume *_lv_create_an_lv(struct volume_group *vg, struct l
>   		first_seg(lv)->chunk_size = lp->chunk_size;
>   		first_seg(lv)->discards = lp->discards;
>   		/* FIXME: use lowwatermark  via lvm.conf global for all thinpools ? */
> -		first_seg(lv)->low_water_mark = 0;
> +		first_seg(lv)->low_water_mark = lp->low_water_mark;

low_water_mark is not supported yet in the lvm2 stack,
and there is not yet clear definition how is it going to be used.

For now dmeventd is based on 10sec polling interval.

However there needs to be made clean definition for various error path,
and we have to also support 'clean' shutdown for those paths according
to defined policy.


> +	  threshold specified in percentage, convert that into
> +	  number of blocks
> +	*/
> +	lp.low_water_mark = size / chunk_size * threshold / 100;
> +	if (!lp.segtype)
> +		return_NULL;
> +	if (!lv_create_single(vg, &lp))
> +		return_NULL;
> +	if (!(lvl = find_lv_in_vg(vg, pool)))
> +		return NULL;
> +	return (lv_t) lvl->lv;
> +}
> +
> +/* Set defaults for thin LV specific parameters */
> +static void _lv_set_thin_params(struct lvcreate_params *lp,
> +				vg_t vg, const char *pool,
> +				const char *lvname,
> +				uint64_t extents)
> +{
> +	_lv_set_default_params(lp, vg, lvname, extents);
> +
> +	lp->thin = 1;
> +	lp->pool = pool;
> +	lp->segtype = get_segtype_from_string(vg->cmd, "thin");
> +
> +	lp->voriginsize = extents * vg->extent_size;
> +	lp->voriginextents = extents_from_size(vg->cmd, lp->voriginsize,
> +						   vg->extent_size);
> +
> +	lp->stripes = 1;
> +}
> +
> +lv_t lvm_lv_thin(const vg_t vg, const char *pool,
> +		 const char *lvname, uint64_t size)
> +{
> +	struct lvcreate_params lp = { 0 };

I think the lvm2app needs some extension here and rather
make the 'lvcreate_params' structure a visible object with some methods,
and use just one universal function call to create all types
of LVs - so we can better match the internal lvm2 processing.
i.e. there is going to be support for creation of thin
lvs with external origins - so adding  lvm API call for every
possible use case of thin pool is IMHO less clean then maintenance
of 'lvcreate_params' object separately from lvm_lv_create() call.

Similar abstraction needs to be made for i.e. lvconvert command,
which has an internal  lvconvert_params.

Zdenek

[PATCH 1/2] lvm2app: Add thin and thin pool lv creation

[M. Mohan Kumar]

Zdenek Kabelac <zkabelac@redhat.com> writes:

> Dne 12.2.2013 18:24, M. Mohan Kumar napsal(a):
>>   		first_seg(lv)->chunk_size = lp->chunk_size;
>>   		first_seg(lv)->discards = lp->discards;
>>   		/* FIXME: use lowwatermark  via lvm.conf global for all thinpools ? */
>> -		first_seg(lv)->low_water_mark = 0;
>> +		first_seg(lv)->low_water_mark = lp->low_water_mark;
>
> low_water_mark is not supported yet in the lvm2 stack,
> and there is not yet clear definition how is it going to be used.
>

IMHO there is no harm in adding support to specify low water mark during
thin pool creation. 

> For now dmeventd is based on 10sec polling interval.
>

Is it not possible to use the 'thin-pool' threshold dm-event instead of
10-sec polling in dmeventd? 

> However there needs to be made clean definition for various error path,
> and we have to also support 'clean' shutdown for those paths according
> to defined policy.
>

Could you please explain it further?

>> +lv_t lvm_lv_thin(const vg_t vg, const char *pool,
>> +		 const char *lvname, uint64_t size)
>> +{
>> +	struct lvcreate_params lp = { 0 };
>
> I think the lvm2app needs some extension here and rather
> make the 'lvcreate_params' structure a visible object with some methods,
> and use just one universal function call to create all types
> of LVs - so we can better match the internal lvm2 processing.
> i.e. there is going to be support for creation of thin
> lvs with external origins - so adding  lvm API call for every
> possible use case of thin pool is IMHO less clean then maintenance
> of 'lvcreate_params' object separately from lvm_lv_create() call.
>

I agree for a need of a single lv create function capable of creating
any LV type and I already posted a RFC version of that patch a month
ago.

But providing a simpler interface to create a specific type of LV may
also be a valid usecase, where an user does not want to
customize/specify each parameters in LV creation.

IIUC by 'support for creation of thin lvs with external origins' you
mean creating a thin snapshot LV from a linear LV. In this case
lvm_lv_snapshot function needs to be changed, while this function
lvm_lv_thin creates only thin Lvs (not thin snapshots) 

[PATCH 1/2] lvm2app: Add thin and thin pool lv creation

[Zdenek Kabelac]

Dne 28.2.2013 12:22, M. Mohan Kumar napsal(a):
> Zdenek Kabelac <zkabelac@redhat.com> writes:
>
>> Dne 12.2.2013 18:24, M. Mohan Kumar napsal(a):
>>>    		first_seg(lv)->chunk_size = lp->chunk_size;
>>>    		first_seg(lv)->discards = lp->discards;
>>>    		/* FIXME: use lowwatermark  via lvm.conf global for all thinpools ? */
>>> -		first_seg(lv)->low_water_mark = 0;
>>> +		first_seg(lv)->low_water_mark = lp->low_water_mark;
>>
>> low_water_mark is not supported yet in the lvm2 stack,
>> and there is not yet clear definition how is it going to be used.
>>
>
> IMHO there is no harm in adding support to specify low water mark during
> thin pool creation.

It's not yet even decided what would be the unit on lvm side. It could be 
percentage either as int - or  percent_t, or the size in blocks or something 
like that.

>
>> For now dmeventd is based on 10sec polling interval.
>>
>
> Is it not possible to use the 'thin-pool' threshold dm-event instead of
> 10-sec polling in dmeventd?

That's the key problem here. We need to have policy defined - since i.e.
if you set watermark on 75% and you start the pool already 80% full
(and you don't know the fullness until pool is started) there would be no 
message -> no trial to expand the pool. On the other hand it's valid request 
to start thinpool which is above the threshold - but how it should behave in 
this case must be driven by some rules. There could be 'soft' watermark - to 
run even tools like 'fstrim' and then there could be 'hard' watermark to 
support activation only in 'read-only' mode or even avoid activation 
completely. As you can see there could be tons of options but on the other 
hand we should avoid creating some hardly configurable policies here where 
user would need to spend week to even understand them.


>> However there needs to be made clean definition for various error path,
>> and we have to also support 'clean' shutdown for those paths according
>> to defined policy.
>>
>
> Could you please explain it further?

One of current problems in lvm2 is if you have overfilled pool - it stays
stuck awaiting on pool extension - and there is no way to shutdown
machine 'cleanly' in this state (i.e. user has no drive to attach, or simply 
needs to reboot)


> I agree for a need of a single lv create function capable of creating
> any LV type and I already posted a RFC version of that patch a month
> ago.
>
> But providing a simpler interface to create a specific type of LV may
> also be a valid usecase, where an user does not want to
> customize/specify each parameters in LV creation.

Simplier interfaces have the problem - it could make the future extension of 
interface much harder if you want to keep them supported.

i.e. lot's of decision in lvm2 is currently based on lvm.conf.
But lvm2api is currently missing these - so this needs to be addressed as well.


>
> IIUC by 'support for creation of thin lvs with external origins' you
> mean creating a thin snapshot LV from a linear LV. In this case
> lvm_lv_snapshot function needs to be changed, while this function
> lvm_lv_thin creates only thin Lvs (not thin snapshots)

As you could see yourself - it's getting to be a lvm2api user's problem to 
figure out, how to actually create a specific LV - API user would need to scan 
number of 'lvcreate' function and try to figure out which function to call, 
instead of having single call with  'lvcreate_params' - so IMHO I'd prefer to 
avoid creating this 'array' of 'simple' interfaces even though all those 
functions could be possible later turned into wrappers calling one universal 
function - but why not start directly with the proper way.

Zdenek

[PATCH 1/2] lvm2app: Add thin and thin pool lv creation

[Tony Asleson]

On 02/28/2013 09:35 AM, Zdenek Kabelac wrote:
> As you could see yourself - it's getting to be a lvm2api user's problem
> to figure out, how to actually create a specific LV - API user would
> need to scan number of 'lvcreate' function and try to figure out which
> function to call, instead of having single call with  'lvcreate_params'
> - so IMHO I'd prefer to avoid creating this 'array' of 'simple'
> interfaces even though all those functions could be possible later
> turned into wrappers calling one universal function - but why not start
> directly with the proper way.

Obviously there are pros and cons to either approach.  In my opinion
having a single function call that takes 20 parameters is no easier to
use than a 20 separate function calls.  The complexity is present in both.

In my opinion it would be useful to have a simple function that
satisfies 90% of what most users use and then have one or two addition
functions that satisfy the other corner cases.

To date I believe the following options have been proposed (please correct):

1. Separate functions with varying amounts of increased functionality
2. One function that does everything
  a. Expose large structure for parameters
  b. Expose string with one or parameters embedded in it
  c. Set/get functions on an opaque data structure which en-composes all
the options (eg. getsockopt/setsockopt)

I believe that whatever we choose needs to be able to accommodate future
change as new things will be added over time.

For each of these options I see the following concerns:

1. No one seems particularly interested in this option, but it would
work.  Downside is big name space and complexity of figuring out which
one you want.

2a. Exposing a large structure has the problem of how you extend it over
time.  Do you create a new structure and create a new function that
takes that new structure or do you pass a void type with a size and
select the structure based on size etc.  Once you expose it, it can
never change.

2b. Using a string for parameters introduces a different set of issues.
 The client needs to be able to correctly create the string.  The
library needs to parse the string.  You lose type safety and you don't
know until run-time that the parameters are correct/incorrect.

2c. This option requires more code, but allows you to change the
implementation of the API and allows you to extend the API easily.

Obviously it is easy to see that I favor option 2c.  This is also the
suggested way by others as well (https://github.com/shepjeng/libabc).

Comments?

Regards,
Tony

[PATCH 1/2] lvm2app: Add thin and thin pool lv creation

[Zdenek Kabelac]

Dne 28.2.2013 21:33, Tony Asleson napsal(a):
> On 02/28/2013 09:35 AM, Zdenek Kabelac wrote:
>> As you could see yourself - it's getting to be a lvm2api user's problem
>> to figure out, how to actually create a specific LV - API user would
>> need to scan number of 'lvcreate' function and try to figure out which
>> function to call, instead of having single call with  'lvcreate_params'
>> - so IMHO I'd prefer to avoid creating this 'array' of 'simple'
>> interfaces even though all those functions could be possible later
>> turned into wrappers calling one universal function - but why not start
>> directly with the proper way.
>
> Obviously there are pros and cons to either approach.  In my opinion
> having a single function call that takes 20 parameters is no easier to
> use than a 20 separate function calls.  The complexity is present in both.
>
> In my opinion it would be useful to have a simple function that
> satisfies 90% of what most users use and then have one or two addition
> functions that satisfy the other corner cases.
>
> To date I believe the following options have been proposed (please correct):
>
> 1. Separate functions with varying amounts of increased functionality
> 2. One function that does everything
>    a. Expose large structure for parameters
>    b. Expose string with one or parameters embedded in it
>    c. Set/get functions on an opaque data structure which en-composes all
> the options (eg. getsockopt/setsockopt)
>
> I believe that whatever we choose needs to be able to accommodate future
> change as new things will be added over time.
>
> 2a. Exposing a large structure has the problem of how you extend it over
> time.  Do you create a new structure and create a new function that
> takes that new structure or do you pass a void type with a size and
> select the structure based on size etc.  Once you expose it, it can
> never change.
>
> 2b. Using a string for parameters introduces a different set of issues.
>   The client needs to be able to correctly create the string.  The
> library needs to parse the string.  You lose type safety and you don't
> know until run-time that the parameters are correct/incorrect.
>
> 2c. This option requires more code, but allows you to change the
> implementation of the API and allows you to extend the API easily.
>
> Obviously it is easy to see that I favor option 2c.  This is also the
> suggested way by others as well (https://github.com/shepjeng/libabc).

API should never expose structure - only handlers, so obviously 2c is the only 
way.

My idea here would be - we may add multiple different functions
how to create  'lvcreate_params' 'object'.

So there could be a simple function call to prepare such object
for thin volume creation - or for snapshot of an LV.
And then having separate 'tiny' functions  get/set for individual options
we support.

But there should be one entrance to 'lvm_lvcreate' function with such object.

API should also be replaceable for the current API used by lvm commands,
so those should be converted to use lvm library over the time as well.

So that's why I'm proposing to go with  lvcreate_params way - since this
way we may get relatively nice mapping.

Though the command are doing massive 'validation' of options - and the 
question is - is this validation supposed to be happing also with lvm2api?
If so - that's another thing which will require some thinking.


Zdenek

[PATCH 1/2] lvm2app: Add thin and thin pool lv creation

[Tony Asleson]

On 02/28/2013 03:57 PM, Zdenek Kabelac wrote:
> Dne 28.2.2013 21:33, Tony Asleson napsal(a):
>> On 02/28/2013 09:35 AM, Zdenek Kabelac wrote:
>>> As you could see yourself - it's getting to be a lvm2api user's problem
>>> to figure out, how to actually create a specific LV - API user would
>>> need to scan number of 'lvcreate' function and try to figure out which
>>> function to call, instead of having single call with  'lvcreate_params'
>>> - so IMHO I'd prefer to avoid creating this 'array' of 'simple'
>>> interfaces even though all those functions could be possible later
>>> turned into wrappers calling one universal function - but why not start
>>> directly with the proper way.
>>
>> Obviously there are pros and cons to either approach.  In my opinion
>> having a single function call that takes 20 parameters is no easier to
>> use than a 20 separate function calls.  The complexity is present in
>> both.
>>
>> In my opinion it would be useful to have a simple function that
>> satisfies 90% of what most users use and then have one or two addition
>> functions that satisfy the other corner cases.
>>
>> To date I believe the following options have been proposed (please
>> correct):
>>
>> 1. Separate functions with varying amounts of increased functionality
>> 2. One function that does everything
>>    a. Expose large structure for parameters
>>    b. Expose string with one or parameters embedded in it
>>    c. Set/get functions on an opaque data structure which en-composes all
>> the options (eg. getsockopt/setsockopt)
>>
>> I believe that whatever we choose needs to be able to accommodate future
>> change as new things will be added over time.
>>
>> 2a. Exposing a large structure has the problem of how you extend it over
>> time.  Do you create a new structure and create a new function that
>> takes that new structure or do you pass a void type with a size and
>> select the structure based on size etc.  Once you expose it, it can
>> never change.
>>
>> 2b. Using a string for parameters introduces a different set of issues.
>>   The client needs to be able to correctly create the string.  The
>> library needs to parse the string.  You lose type safety and you don't
>> know until run-time that the parameters are correct/incorrect.
>>
>> 2c. This option requires more code, but allows you to change the
>> implementation of the API and allows you to extend the API easily.
>>
>> Obviously it is easy to see that I favor option 2c.  This is also the
>> suggested way by others as well (https://github.com/shepjeng/libabc).
> 
> API should never expose structure - only handlers, so obviously 2c is
> the only way.
> 
> My idea here would be - we may add multiple different functions
> how to create  'lvcreate_params' 'object'.

I initially was thinking the same way, but now I'm asking myself is it
any easier to have a bunch of initializer functions to create an object
that has to be passed to one function that does the work instead of
number of functions?  With this new object we have yet one more piece of
memory on the heap that needs to be cleaned up to prevent leaks after
the call is completed.  Not a horrible requirement, but kind of a pain
for the simple generic use cases.

Realistically, how many different default constructor/initializer are we
looking at?

> So there could be a simple function call to prepare such object
> for thin volume creation - or for snapshot of an LV.
> And then having separate 'tiny' functions  get/set for individual options
> we support.

In OOA/OOP having many get and set functions for a class can be
indicative of a poor design.  Ideally objects should have data and
behavior.  In this case we have an object for the sole purpose as an
encapsulated parameter passer.  Ideally the get/set functions would only
be available for those options that are common to all lv parameters.
Otherwise a user could be setting or getting a value that has absolutely
no meaning in their context.  We could achieve this by having the
initializers functions take parameters that only pertain to them, but we
are then creating complexity for non advanced use cases who just want to
use the defaults.

We could also create a different object parameter type for each use
case, but then we end up having more functions to pass the parameter too
if we want to maintain type safety.  However, we would ensure that
get/set functions would not get called spuriously.

> But there should be one entrance to 'lvm_lvcreate' function with such
> object.
> 
> API should also be replaceable for the current API used by lvm commands,
> so those should be converted to use lvm library over the time as well.
> 
> So that's why I'm proposing to go with  lvcreate_params way - since this
> way we may get relatively nice mapping.
> 
> Though the command are doing massive 'validation' of options - and the
> question is - is this validation supposed to be happing also with lvm2api?
> If so - that's another thing which will require some thinking.

The validation should be done in one spot for both.  In my opinion the
code should parse the arguments and then call the appropriate function
which in turn validates the values.  Otherwise we have duplicate code
and more opportunities to fix the same bug more than once.

At the moment I've kind of exhausted everything I can think of.  Does
anyone else have any ideas that would lend itself to an API that can
accommodate a plethora of configurable options, allow a sane default for
common uses cases and not create any potential opportunities for
incorrect use?

Regards,
Tony

[PATCH 1/2] lvm2app: Add thin and thin pool lv creation

[Andy Grover]

On 04/25/2013 04:14 PM, Tony Asleson wrote:

> At the moment I've kind of exhausted everything I can think of.  Does
> anyone else have any ideas that would lend itself to an API that can
> accommodate a plethora of configurable options, allow a sane default for
> common uses cases and not create any potential opportunities for
> incorrect use?

I just want an API so I can create thinp LVs from a pool from scratch, 
and create thinp LVs based on another thinp LV.

I don't care about options. Use the defaults. Let's get something going 
and then iterate.

-- Andy

[PATCH 2/2] master - thin: Add low water mark parameter to pool creation

[M. Mohan Kumar]

From: "M. Mohan Kumar" <mohan@in.ibm.com>

Add --lowwatermark parameter to thin-pool creation. It enables sending
dmevent when the percentage of free blocks in the pool reaches below this
percent

Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
 man/lvcreate.8.in | 11 ++++++++++-
 tools/args.h      |  1 +
 tools/commands.h  |  6 ++++--
 tools/lvconvert.c |  1 +
 tools/lvcreate.c  |  7 ++++++-
 tools/toollib.c   | 11 +++++++++++
 tools/toollib.h   |  1 +
 7 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/man/lvcreate.8.in b/man/lvcreate.8.in
index da4c1fe..6a6ebcc 100644
--- a/man/lvcreate.8.in
+++ b/man/lvcreate.8.in
@@ -57,7 +57,9 @@ lvcreate \- create a logical volume in an existing volume group
 .RB [ \-\-discards
 .RI { ignore | nopassdown | passdown }]
 .RB [ \-\-poolmetadatasize
-.IR ThinPoolMetadataSize [ bBsSkKmMgG ]]]
+.IR ThinPoolMetadataSize [ bBsSkKmMgG ]]
+.RB [ \-\-lowwatermark
+.IR ThinPoolLowWaterMarkThreshold in percentage ]]
 .RB [ \-\-thinpool
 .IR ThinPoolLogicalVolume { Name | Path }]
 .RB [ \-\-type
@@ -246,6 +248,13 @@ Supported value is in range between 2MiB and 16GiB.
 Default value is  (Pool_LV_size / Pool_LV_chunk_size * 64b).
 Default unit is megabytes.
 .TP
+.IR \fB\-\-lowwatermark " " ThinPoolLowWaterMarkThreshold in percentage
+Set the low water mark threshold of thin pool logical volume.
+Supported value is in range between 0 and 100.
+Default value is 0, meaning a dm event will be generated only when
+all blocks in the pool are consumed. A value of 25 means dm event will
+be generated when percentage of free blocks in the pool becomes <= 25%
+.TP
 .IR \fB\-r ", " \fB\-\-readahead " {" ReadAheadSectors | auto | none }
 Set read ahead sector count of this logical volume.
 For volume groups with metadata in lvm1 format, this must
diff --git a/tools/args.h b/tools/args.h
index d4d6c40..93657cc 100644
--- a/tools/args.h
+++ b/tools/args.h
@@ -77,6 +77,7 @@ arg(force_long_ARG, '\0', "force", NULL, ARG_COUNTABLE)
 arg(stripes_long_ARG, '\0', "stripes", int_arg, 0)
 arg(sysinit_ARG, '\0', "sysinit", NULL, 0)
 arg(thinpool_ARG, '\0', "thinpool", string_arg, 0)
+arg(lowwatermark_ARG, '\0', "lowwatermark", int_arg, 0)
 
 /* Allow some variations */
 arg(resizable_ARG, '\0', "resizable", yes_no_arg, 0)
diff --git a/tools/commands.h b/tools/commands.h
index 986539e..0a5972b 100644
--- a/tools/commands.h
+++ b/tools/commands.h
@@ -185,7 +185,8 @@ xx(lvcreate,
    "\t[-r|--readahead ReadAheadSectors|auto|none]\n"
    "\t[-R|--regionsize MirrorLogRegionSize]\n"
    "\t[-T|--thin  [-c|--chunksize  ChunkSize]\n"
-   "\t  [--discards {ignore|nopassdown|passdown}]\n"
+   "\t  [--discards {ignore|nopassdown|passdown}] \n"
+   "\t  [--lowwatermark low_water_mark threshold in percentage of total number of blocks] \n"
    "\t  [--poolmetadatasize MetadataSize[bBsSkKmMgG]]]\n"
    "\t[--thinpool ThinPoolLogicalVolume{Name|Path}]\n"
    "\t[-t|--test]\n"
@@ -232,7 +233,8 @@ xx(lvcreate,
    monitor_ARG, name_ARG, nosync_ARG, noudevsync_ARG, permission_ARG,
    persistent_ARG, readahead_ARG, regionsize_ARG, size_ARG, snapshot_ARG,
    stripes_ARG, stripesize_ARG, test_ARG, thin_ARG, thinpool_ARG, type_ARG,
-   virtualoriginsize_ARG, poolmetadatasize_ARG, virtualsize_ARG, zero_ARG)
+   virtualoriginsize_ARG, poolmetadatasize_ARG, virtualsize_ARG, zero_ARG,
+   lowwatermark_ARG)
 
 xx(lvdisplay,
    "Display information about a logical volume",
diff --git a/tools/lvconvert.c b/tools/lvconvert.c
index 5bda00f..9c0ed99 100644
--- a/tools/lvconvert.c
+++ b/tools/lvconvert.c
@@ -352,6 +352,7 @@ static int _read_params(struct lvconvert_params *lp, struct cmd_context *cmd,
 				     &lp->chunk_size,
 				     &lp->discards,
 				     &lp->poolmetadata_size,
+				     NULL,
 				     &lp->zero))
 			return_0;
 
diff --git a/tools/lvcreate.c b/tools/lvcreate.c
index 78292f7..41358f8 100644
--- a/tools/lvcreate.c
+++ b/tools/lvcreate.c
@@ -799,7 +799,8 @@ static int _lvcreate_params(struct lvcreate_params *lp,
 	    !get_stripe_params(cmd, &lp->stripes, &lp->stripe_size) ||
 	    (lp->create_thin_pool &&
 	     !get_pool_params(cmd, &lp->chunk_size, &lp->discards,
-			      &lp->poolmetadatasize, &lp->zero)) ||
+			      &lp->poolmetadatasize, &lp->low_water_mark,
+			      &lp->zero)) ||
 	    !_read_mirror_params(lp, cmd) ||
 	    !_read_raid_params(lp, cmd))
 		return_0;
@@ -926,6 +927,10 @@ static int _check_thin_parameters(struct volume_group *vg, struct lvcreate_param
 			log_error("--zero may only be specified when allocating the thin pool.");
 			return 0;
 		}
+		if (arg_count(vg->cmd, lowwatermark_ARG)) {
+			log_error("--lowwatermark may only be specified when allocating the thin pool.");
+			return 0;
+		}
 	}
 
 	if (lp->create_thin_pool && lp->pool) {
diff --git a/tools/toollib.c b/tools/toollib.c
index 669d772..627c2ec 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -1525,9 +1525,11 @@ int get_pool_params(struct cmd_context *cmd,
 		    uint32_t *chunk_size,
 		    thin_discards_t *discards,
 		    uint64_t *pool_metadata_size,
+		    uint64_t *lowwatermark,
 		    int *zero)
 {
 	const char *dstr;
+	int lwm;
 
 	if (arg_count(cmd, zero_ARG)) {
 		*zero = strcmp(arg_str_value(cmd, zero_ARG, "y"), "n");
@@ -1577,6 +1579,15 @@ int get_pool_params(struct cmd_context *cmd,
 	}
 	*pool_metadata_size = arg_uint64_value(cmd, poolmetadatasize_ARG, UINT64_C(0));
 
+	if (lowwatermark) {
+		if (arg_sign_value(cmd, lowwatermark_ARG, SIGN_NONE) == SIGN_MINUS) {
+			log_error("Negative low water mark threshold is invalid.");
+			return 0;
+		}
+
+		lwm = arg_uint_value(cmd, lowwatermark_ARG, UINT32_C(0));
+		*lowwatermark = *pool_metadata_size / *chunk_size * lwm * 100;
+	}
 	return 1;
 }
 
diff --git a/tools/toollib.h b/tools/toollib.h
index 80c01fd..1b7a03e 100644
--- a/tools/toollib.h
+++ b/tools/toollib.h
@@ -115,6 +115,7 @@ int get_pool_params(struct cmd_context *cmd,
 		    uint32_t *chunk_size,
 		    thin_discards_t *discards,
 		    uint64_t *pool_metadata_size,
+		    uint64_t *lowwatermark,
 		    int *zero);
 int update_pool_params(struct cmd_context *cmd, unsigned attr,
 		       uint32_t data_extents, uint32_t extent_size,
-- 
1.7.11.7