[PATCH] Refactor lib/ code to allow deferred PV labelling

[PATCH] Refactor lib/ code to allow deferred PV labelling

[Petr Rockai]

Hi,

this patch is a major part of a 623808 fix, making it possible to create
the PV structures in memory in an lvmlib app, and only writing the
labels upon a vg_write. Among other useful properties introduced by this
option, it should make it possible to get a pe_start out of the library
without actually creating a VG on disks, which was the original use case
for this change.

When this is in, I will follow up with the lvmlib side of the necessary
changes, which should be relatively simple in comparison.

Yours,
   Petr

-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvmlib-pvcreate.diff
Type: text/x-diff
Size: 12493 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20110427/aeccdc28/attachment.bin>
-------------- next part --------------

-- 
id' Ash = Ash; id' Dust = Dust; id' _ = undefined

[PATCH] Refactor lib/ code to allow deferred PV labelling

[Alasdair G Kergon]

On Wed, Apr 27, 2011 at 02:08:49PM +0200, Peter Rockai wrote:
> --- old-lvmlib-pvcreate/lib/format1/format1.c	2011-04-27 14:02:11.000000000 +0200
> +++ new-lvmlib-pvcreate/lib/format1/format1.c	2011-04-27 14:02:11.000000000 +0200

> @@ -444,12 +452,18 @@ static int _format1_pv_write(const struc

> +	pv->pe_size = pe_size;
> +	pv->pe_count = pe_count;
> +	pv->pe_start = pe_start;

How does this interact with pvchange --uuid, pvresize etc.?
Is it tested with non-default settings?
Are further simplifications possible now, or is this even unmasking 
an existing bug where code forgets that pv_write update those fields?
(Needs an audit before this goes in.)

> +++ new-lvmlib-pvcreate/lib/metadata/metadata-exported.h	2011-04-27 14:02:11.000000000 +0200
> +#define UNLABELLED_PV           0x80000000U     /* PV -this PV had no label written yet */

(Internal only but best to add to flags.c anyway?)

> @@ -51,6 +51,9 @@ struct physical_volume {

> +	/* NB. Only useful/used when status & UNLABELLED_PV! */
> +	int64_t label_sector;

> @@ -59,6 +65,13 @@ struct volume_group {
> +	struct dm_list pvs_to_create;

These are really just temporary places to store things - not used as an intrinsic
part of the structs themselves.  How difficult is it to handle them
separately outside the structs?  Or can we enforce rules that they are always
cleared before the relevant metadata is cached or written?

There should be an internal consistency check that the appropriate orphan
lock is still held before the PV is written to in this manner.

Any vg_validation checks possible?

How easy is it to convert the other vg-based instances of pv_write over
to the new method?

Alasdair

[PATCH] Refactor lib/ code to allow deferred PV labelling

[Peter Rajnoha]

On 04/27/2011 08:24 PM +0100, Alasdair G Kergon wrote:
>> @@ -51,6 +51,9 @@ struct physical_volume {
> 
>> +	/* NB. Only useful/used when status & UNLABELLED_PV! */
>> +	int64_t label_sector;
> 
>> @@ -59,6 +65,13 @@ struct volume_group {
>> +	struct dm_list pvs_to_create;
> 
> These are really just temporary places to store things - not used as an intrinsic
> part of the structs themselves.  How difficult is it to handle them
> separately outside the structs?

Well, normally, we store that in PV-based format instance context
(the 'struct text_fid_pv_context' that is stored in 'private' field in
'struct format_instance). But if that PV is part of the VG, we use a
common VG-based format instance - so we can't store that PV info there
separately for each PV...

So how about using 'struct pvcreate_params' we attach to 'struct pv_to_create'
in add_pv_to_vg fn:

  if (pv->status & UNLABELLED_PV) {
                if (!(pvc = dm_pool_zalloc(mem, sizeof(*pvc)))) {
                        log_error("pv_to_create allocation for '%s' failed", pv_name);
                        return 0;
                }    
                pvc->pv = pv;
                pvc->pp = pp;
                ...
  ...


Can we use that to store that label sector info? Would that work?

If we don't find a place for that label sector other than the struct PV
itself as proposed in this patch, we can also remove the 'struct text_fid_pv_context'.
The text_fid_pv_context is here to store the label_sector info only. At least for now.
I wanted to use the PV struct for that as well the other day, like you do here... ;)
It's useless to store that in two places at the same time.

Peter

[PATCH] Refactor lib/ code to allow deferred PV labelling

[Petr Rockai]

(resending with the hopefully correct recipient list)

Peter Rajnoha <prajnoha@redhat.com> writes:
>> These are really just temporary places to store things - not used as an intrinsic
>> part of the structs themselves.  How difficult is it to handle them
>> separately outside the structs?
>
> Well, normally, we store that in PV-based format instance context
> (the 'struct text_fid_pv_context' that is stored in 'private' field in
> 'struct format_instance). But if that PV is part of the VG, we use a
> common VG-based format instance - so we can't store that PV info there
> separately for each PV...

Indeed, which is why I didn't use that (although I initially tried).

> So how about using 'struct pvcreate_params' we attach to 'struct pv_to_create'
> in add_pv_to_vg fn:
>
>   if (pv->status & UNLABELLED_PV) {
>                 if (!(pvc = dm_pool_zalloc(mem, sizeof(*pvc)))) {
>                         log_error("pv_to_create allocation for '%s' failed", pv_name);
>                         return 0;
>                 }    
>                 pvc->pv = pv;
>                 pvc->pp = pp;
>                 ...
>   ...
>
>
> Can we use that to store that label sector info? Would that work?

Probably, although it would make things somewhat more complicated yet. I
don't see anything wrong with storing that inside PV: it is per-PV
anyway. I agree that the PV struct could use some reorganisation, but
there is absolutely no problem in storing these things inside. They may
need to be clearly marked (maybe relegated into a sub-structure) but
that's all.

And if the info does not belong to the PV, it even less belongs to
pvcreate_params. So while it (in practice) can go in either, it belongs
more rightly to PV.

> If we don't find a place for that label sector other than the struct
> PV itself as proposed in this patch, we can also remove the 'struct
> text_fid_pv_context'.  The text_fid_pv_context is here to store the
> label_sector info only. At least for now.  I wanted to use the PV
> struct for that as well the other day, like you do here... ;) It's
> useless to store that in two places at the same time.

Right, I was wondering if moving the thing out of the PV is worth the
big pile of code that does the text_fid_pv_context bookkeeping. I would
say not. And I need it in the PV anyway. So I vote for removing
text_fid_pv_context.

Yours,
   Petr

-- 
id' Ash = Ash; id' Dust = Dust; id' _ = undefined