linux-lvm.redhat.com archive mirror
 help / color / mirror / Atom feed
* [linux-lvm] [PATCH 1/2] metadata: check pv->dev null when setting PARTIAL_LV
@ 2020-09-10 15:37 Zhao Heming
  2020-09-11 12:17 ` Zdenek Kabelac
  0 siblings, 1 reply; 4+ messages in thread
From: Zhao Heming @ 2020-09-10 15:37 UTC (permalink / raw)
  To: linux-lvm; +Cc: zkabelac, teigland, Zhao Heming

The code in vg_read():
```
if (missing_pv_dev || missing_pv_flag)
    vg_mark_partial_lvs(vg, 1);
```
the missing_pv_dev not zero when pv->dev is null.
the missing_pv_flag not zero when pv->dev is not null but status MISSING_PV is true.
any above condition will trigger code to set PARTIAL_LV.
So in _lv_mark_if_partial_single(), there should add  '|| (!pv->dev)' case.

Below comment by David:
And the MISSING_PV flag was not used consistently, so there were cases
where pv->dev was null but the flag was not set. So to check for null dev
until it's more confidence in how that flag is used.

Signed-off-by: Zhao Heming <heming.zhao@suse.com>
---
 lib/metadata/metadata.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 8b8c491..5f444a6 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -2048,11 +2048,13 @@ static int _lv_mark_if_partial_single(struct logical_volume *lv, void *data)
 	unsigned s;
 	struct _lv_mark_if_partial_baton baton = { .partial = 0 };
 	struct lv_segment *lvseg;
+	struct physical_volume *pv;
 
 	dm_list_iterate_items(lvseg, &lv->segments) {
 		for (s = 0; s < lvseg->area_count; ++s) {
 			if (seg_type(lvseg, s) == AREA_PV) {
-				if (is_missing_pv(seg_pv(lvseg, s)))
+				pv = seg_pv(lvseg, s);
+				if (is_missing_pv(pv) || !pv->dev)
 					lv->status |= PARTIAL_LV;
 			}
 		}
-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [linux-lvm] [PATCH 1/2] metadata: check pv->dev null when setting PARTIAL_LV
  2020-09-10 15:37 [linux-lvm] [PATCH 1/2] metadata: check pv->dev null when setting PARTIAL_LV Zhao Heming
@ 2020-09-11 12:17 ` Zdenek Kabelac
  2020-09-11 13:59   ` heming.zhao
  0 siblings, 1 reply; 4+ messages in thread
From: Zdenek Kabelac @ 2020-09-11 12:17 UTC (permalink / raw)
  To: LVM general discussion and development, Zhao Heming; +Cc: teigland

Dne 10. 09. 20 v 17:37 Zhao Heming napsal(a):
> The code in vg_read():
> ```
> if (missing_pv_dev || missing_pv_flag)
>      vg_mark_partial_lvs(vg, 1);
> ```
> the missing_pv_dev not zero when pv->dev is null.
> the missing_pv_flag not zero when pv->dev is not null but status MISSING_PV is true.
> any above condition will trigger code to set PARTIAL_LV.
> So in _lv_mark_if_partial_single(), there should add  '|| (!pv->dev)' case.
> 
> Below comment by David:
> And the MISSING_PV flag was not used consistently, so there were cases
> where pv->dev was null but the flag was not set. So to check for null dev
> until it's more confidence in how that flag is used.


Hi

While the .gitignore patch is no problem, this one is somewhat puzzling.

Do you have an reproducible test case where you can exercise this code path?

It seems more logical if we move flag correctly marked for PV
so is_missing_pv() works - as if it does not - we would have to spread test 
for  pv->dev!=NULL  check everywhere, which is not really wanted.

So what we need to check here is all assings of pv->dev needs to handle
MISSING_PV flag properly.

Zdenek

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [linux-lvm] [PATCH 1/2] metadata: check pv->dev null when setting PARTIAL_LV
  2020-09-11 12:17 ` Zdenek Kabelac
@ 2020-09-11 13:59   ` heming.zhao
  2020-09-11 14:32     ` Zdenek Kabelac
  0 siblings, 1 reply; 4+ messages in thread
From: heming.zhao @ 2020-09-11 13:59 UTC (permalink / raw)
  To: Zdenek Kabelac, LVM general discussion and development; +Cc: teigland



On 9/11/20 8:17 PM, Zdenek Kabelac wrote:
> Dne 10. 09. 20 v 17:37 Zhao Heming napsal(a):
>> The code in vg_read():
>> ```
>> if (missing_pv_dev || missing_pv_flag)
>> ���� vg_mark_partial_lvs(vg, 1);
>> ```
>> the missing_pv_dev not zero when pv->dev is null.
>> the missing_pv_flag not zero when pv->dev is not null but status MISSING_PV is true.
>> any above condition will trigger code to set PARTIAL_LV.
>> So in _lv_mark_if_partial_single(), there should add� '|| (!pv->dev)' case.
>>
>> Below comment by David:
>> And the MISSING_PV flag was not used consistently, so there were cases
>> where pv->dev was null but the flag was not set. So to check for null dev
>> until it's more confidence in how that flag is used.
> 
> 
> Hi
> 
> While the .gitignore patch is no problem, this one is somewhat puzzling.
> 
> Do you have an reproducible test case where you can exercise this code path?
> 
> It seems more logical if we move flag correctly marked for PV
> so is_missing_pv() works - as if it does not - we would have to spread test for� pv->dev!=NULL� check everywhere, which is not really wanted.
> 
> So what we need to check here is all assings of pv->dev needs to handle
> MISSING_PV flag properly.
> 
> Zdenek
> 

I don't have test case. 
There are some code or comments about not consistent issue.
e.g.
1> in _check_devs_used_correspond_with_vg()
        /*
         * FIXME: It's not clear if the meaning
         * of "missing" should always include the
         * !pv->dev case, or if "missing" is the
         * more narrow case where VG metadata has
         * been written with the MISSING flag.
         */

2> in vg_read()
```
     * The PV's device may be present while the PV for the device has the
     * MISSING_PV flag set in the metadata.  This happened because the VG
     * was written while this dev was missing, so the MISSING flag was
     * written in the metadata for PV.  Now the device has reappeared.
     * However, the VG has changed since the device was last present, and
     * if the device has outdated data it may not be safe to just start
     * using it again.
```

3> in _check_pv_ext(), after calling is_missing_pv(), the function
still access (!pvl->pv->dev).

```
    dm_list_iterate_items(pvl, &vg->pvs) {
        if (is_missing_pv(pvl->pv))
            continue;

        /* is_missing_pv doesn't catch NULL dev */
        if (!pvl->pv->dev)
            continue;
```

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [linux-lvm] [PATCH 1/2] metadata: check pv->dev null when setting PARTIAL_LV
  2020-09-11 13:59   ` heming.zhao
@ 2020-09-11 14:32     ` Zdenek Kabelac
  0 siblings, 0 replies; 4+ messages in thread
From: Zdenek Kabelac @ 2020-09-11 14:32 UTC (permalink / raw)
  To: heming.zhao, LVM general discussion and development; +Cc: teigland

Dne 11. 09. 20 v 15:59 heming.zhao@suse.com napsal(a):
> 
> 
> On 9/11/20 8:17 PM, Zdenek Kabelac wrote:
>> Dne 10. 09. 20 v 17:37 Zhao Heming napsal(a):
>>> The code in vg_read():
>>> ```
>>> if (missing_pv_dev || missing_pv_flag)
>>>  ���� vg_mark_partial_lvs(vg, 1);
>>> ```
>>> the missing_pv_dev not zero when pv->dev is null.
>>> the missing_pv_flag not zero when pv->dev is not null but status MISSING_PV is true.
>>> any above condition will trigger code to set PARTIAL_LV.
>>> So in _lv_mark_if_partial_single(), there should add� '|| (!pv->dev)' case.
>>>
>>> Below comment by David:
>>> And the MISSING_PV flag was not used consistently, so there were cases
>>> where pv->dev was null but the flag was not set. So to check for null dev
>>> until it's more confidence in how that flag is used.
>>
>>
>> Hi
>>
>> While the .gitignore patch is no problem, this one is somewhat puzzling.
>>
>> Do you have an reproducible test case where you can exercise this code path?
>>
>> It seems more logical if we move flag correctly marked for PV
>> so is_missing_pv() works - as if it does not - we would have to spread test for� pv->dev!=NULL� check everywhere, which is not really wanted.
>>
>> So what we need to check here is all assings of pv->dev needs to handle
>> MISSING_PV flag properly.
>>
>> Zdenek
>>
> 
> I don't have test case.
> There are some code or comments about not consistent issue.

Yep - we know there is some problem somewhere.

There are several commits which may need closer inspecation as they just seem 
to be reacting on some hidden problem in device handling:

2f29765e7fd1135d070310683cf486f07d041c81
98d420200e16b450b6b7e33b83bdf36a59196d6d
607858538132a33a27039e0ff4796b1a7d9f4f32

> e.g.
> 1> in _check_devs_used_correspond_with_vg()
>          /*
>           * FIXME: It's not clear if the meaning
>           * of "missing" should always include the
>           * !pv->dev case, or if "missing" is the
>           * more narrow case where VG metadata has
>           * been written with the MISSING flag.
>           */

MISSING may come from metadata - so even if we can see the device - once
it's marked in lvm2 metadata - we can't work with it - unless it's
vgextend --restoremissing.

> 
> 2> in vg_read()
> ```
>       * The PV's device may be present while the PV for the device has the
>       * MISSING_PV flag set in the metadata.  This happened because the VG
>       * was written while this dev was missing, so the MISSING flag was
>       * written in the metadata for PV.  Now the device has reappeared.
>       * However, the VG has changed since the device was last present, and
>       * if the device has outdated data it may not be safe to just start
>       * using it again.
> ```

Existing logic shell mark as MISSING all PVs that are not available
and preserve already missing PVs as well.

What is currently not well defined is the behavior with new raids - where
a 'temporary' missing device should not be handled by lvm2 and left
for actual md core to be 'covered' - but IMHO I don't think this can work in 
long term - but in some case  handling of MISSING is simply still evolving.

Zdenek

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-09-11 14:32 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-10 15:37 [linux-lvm] [PATCH 1/2] metadata: check pv->dev null when setting PARTIAL_LV Zhao Heming
2020-09-11 12:17 ` Zdenek Kabelac
2020-09-11 13:59   ` heming.zhao
2020-09-11 14:32     ` Zdenek Kabelac

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).