linux-lvm.redhat.com archive mirror
 help / color / mirror / Atom feed
From: Zdenek Kabelac <zkabelac@redhat.com>
To: LVM general discussion and development <linux-lvm@redhat.com>,
	Zhao Heming <heming.zhao@suse.com>
Cc: teigland@redhat.com
Subject: Re: [linux-lvm] [PATCH 1/2] metadata: check pv->dev null when setting PARTIAL_LV
Date: Fri, 11 Sep 2020 14:17:47 +0200	[thread overview]
Message-ID: <45b6ac96-64b3-eaaf-aec3-f1864d66a801@redhat.com> (raw)
In-Reply-To: <1599752247-30793-1-git-send-email-heming.zhao@suse.com>

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

  reply	other threads:[~2020-09-11 12:17 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2020-09-11 13:59   ` heming.zhao
2020-09-11 14:32     ` Zdenek Kabelac

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=45b6ac96-64b3-eaaf-aec3-f1864d66a801@redhat.com \
    --to=zkabelac@redhat.com \
    --cc=heming.zhao@suse.com \
    --cc=linux-lvm@redhat.com \
    --cc=teigland@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).