Refactor lib/ code to allow deferred PV labelling
PATCH 1/1 Refactor lib/ code to allow deferred PV labelling
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 --------------
Signature · 1 line
id' Ash = Ash; id' Dust = Dust; id' _ = undefined
PATCH 1/1 Refactor lib/ code to allow deferred PV labelling
lib/format1/format1.c static int _format1_pv_write(const struc · line 455
Quoted 8 lines · Peter Rockai
lib/format1/format1.c +3
-0@@ -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.)
lib/format1/format1.c static int _format1_pv_write(const struc · line 457
Quoted 2 lines
lib/format1/format1.c +2
-0+++ 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?)
lib/format1/format1.c struct volume_group { · line 65
Quoted 7 lines
lib/format1/format1.c +3
-0@@ -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 1/1 Refactor lib/ code to allow deferred PV labelling
Quoted 11 lines · Alasdair G Kergon
Review struct volume_group { · line 65
Quoted 7 lines
(quoted diff) +3
-0@@ -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 1/1 Refactor lib/ code to allow deferred PV labelling
(resending with the hopefully correct recipient list)
Quoted 9 lines · Peter Rajnoha
Quoted 3 lines
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 'structformat_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).
Quoted 14 lines
So how about using 'struct
pvcreate_params' we attach to 'structpv_to_create' inadd_pv_to_vgfn: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; ... ...
Quoted 1 line
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.
Quoted 6 lines
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'. Thetext_fid_pv_contextis here to store thelabel_sectorinfo 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
Signature · 1 line
id' Ash = Ash; id' Dust = Dust; id' _ = undefined