<tt><font size=2>Hello Ben,</font></tt>
<br>
<br>
<br><font size=2 face="sans-serif">OK, I will </font><tt><font size=2>modify
code to call domap() on the device<br>
if its wwid is different from its merge_id.</font></tt>
<br>
<br><font size=2 face="sans-serif">I'll resend a serials of patch later,</font>
<br><font size=2 face="sans-serif">Thank you for your patience. </font>
<br>
<br><font size=2 face="sans-serif">Regards,</font>
<br><font size=2 face="sans-serif">Tang Junhui</font>
<br>
<br>
<br>
<br>
<br>
<br><font size=1 color=#5f5f5f face="sans-serif">发件人:
</font><font size=1 face="sans-serif">"Benjamin
Marzinski" <bmarzins@redhat.com></font>
<br><font size=1 color=#5f5f5f face="sans-serif">收件人:
</font><font size=1 face="sans-serif">tang.junhui@zte.com.cn,
</font>
<br><font size=1 color=#5f5f5f face="sans-serif">抄送:
</font><font size=1 face="sans-serif">tang.wenjun3@zte.com.cn,
zhang.kai16@zte.com.cn, dm-devel-bounces@redhat.com, dm-devel@redhat.com,
bart.vanassche@sandisk.com, mwilck@suse.com</font>
<br><font size=1 color=#5f5f5f face="sans-serif">日期:
</font><font size=1 face="sans-serif">2017/01/07
00:11</font>
<br><font size=1 color=#5f5f5f face="sans-serif">主题:
</font><font size=1 face="sans-serif">Re: [dm-devel]
[PATCH 01/12] libmultipath: add wwid for "struct uevent" to record
wwid of uevent</font>
<br><font size=1 color=#5f5f5f face="sans-serif">发件人:
</font><font size=1 face="sans-serif">dm-devel-bounces@redhat.com</font>
<br>
<hr noshade>
<br>
<br>
<br><tt><font size=2>On Fri, Jan 06, 2017 at 08:59:27AM +0800, tang.junhui@zte.com.cn
wrote:<br>
> Hello Ben,<br>
> <br>
> > > The premise is that,
devices whose "merge_id" is the same have<br>
> the<br>
> > > same "wwid".
I think this premise can be established.<br>
> <br>
> > So, if you have device where the merge_id
doesn't equal the wwid, you<br>
> > call domap on that device, as it it
wasn't part of a merged set?<br>
> <br>
> Maybe I did not say clearly. "merge_id" is
only used for uevents merging,<br>
> and<br>
> it has nothing to do with "wwid". I only primse
devices whose "merge_id"<br>
> are<br>
> same have same "wwid" (Sorry, maybe I had a
misleading statement in<br>
> previous email).<br>
> "merge_id" and "wwid" can be same
or not same. The merged uevents are<br>
> still<br>
> processed as bellow patch I send previous:<br>
> [dm-devel] [PATCH 11/12] multipathd: proccess merged
uevents<br>
<br>
Let me back up. Say you have a batch of merged uevents<br>
<br>
"add path1, path2, path3"<br>
<br>
where path1 and path2 are on path3's merge_node list. This means that<br>
when you call uev_add_path() for path1 and path2, you will call it with<br>
need_do_map set to 0, and no multipath device will be created. The<br>
multipath device will be created when you call uev_add_path() for path3,<br>
with need_no_map set to 1. Now lets say that when you call pathinfo on<br>
path2, you get a different wwid than your merge_id. path1 and path3
get<br>
the same wwid as their merge_id. In this situation you would call domap<br>
on path1 and path3, but nothing would create the multipath device for<br>
path2.<br>
<br>
Like I said, this is unlikely to happen. But it could. Imagine that<br>
there was some device that presented itself like a scsi device, but<br>
ID_SERIAL could have the same value for devices that should not be<br>
multipathed together. To figure out which devices actually should be<br>
multipathed together, you need to call pathinfo and get the correct<br>
wwid. In this case you would find that you had incorrectly batched<br>
devices with different wwids together.<br>
<br>
To solve this, you would need to make sure to call domap on the device<br>
if its wwid ended up being different from its merge_id. The issue<br>
that my last email pointed out was that if the main uevent, path3 in the<br>
above example, was the only one to have a different wwid, then you would<br>
need to make sure to call domap on the other devices as well. Otherwise<br>
you would create a multipath device for path3, but not path1 and path2.<br>
<br>
If you don't handle these cases, you will only work correctly on the<br>
(admittedly vastly more common) case where even if the wwid and merge_id<br>
don't match, the wwid is still the same for all the batched devices.<br>
But if you are already assuming that the merge_id will correctly group<br>
the devices, then what is the purpose in fetching the wwid at all? It<br>
may be different from the merge_id, but it will still group the devices<br>
the same way. There's nothing special about the wwid value itself. It is<br>
just a tool to correctly group the devices. So, I would say that you<br>
should either not grab the wwid at all if you got it from the uevent, or<br>
do grab the wwid, but be prepared to deal with the case where not all of<br>
the devices that you have batched having the same wwid.<br>
<br>
Does that make more sense?<br>
<br>
-Ben<br>
<br>
> <br>
> Regards,<br>
> Tang Junhui<br>
> <br>
> 发件人: "Benjamin
Marzinski" <bmarzins@redhat.com><br>
> 收件人: tang.junhui@zte.com.cn,<br>
> 抄送: mwilck@suse.com, bart.vanassche@sandisk.com,<br>
> dm-devel@redhat.com, dm-devel-bounces@redhat.com, tang.wenjun3@zte.com.cn,<br>
> zhang.kai16@zte.com.cn<br>
> 日期: 2017/01/06 01:37<br>
> 主题: Re: Re: [dm-devel]
[PATCH 01/12] libmultipath: add wwid for<br>
> "struct uevent" to record wwid of uevent<br>
> <br>
> --------------------------------------------------------------------------<br>
> <br>
> On Thu, Jan 05, 2017 at 11:10:43AM +0800, tang.junhui@zte.com.cn
wrote:<br>
> > Hello Ben,<br>
> ><br>
> > I know what you concern about. Maybe
we can change the "wwid" member<br>
> > in "struct uevent" to another
name such as "merge_id" to only<br>
> > identify which uevents can merge together.
The value of "merge_id"<br>
> can<br>
> > set by "ID_SERIAL" or "ID_UID"
in uevent. If we get the "merge_id"<br>
> > from uevents, we can merge and process
it as merging way, otherwise<br>
> > we process it as old way (processed
one by one).<br>
> <br>
> > And we revert this patch:<br>
> > "[dm-devel] [PATCH 12/12] libmultipath:
use existing wwid when wwid<br>
> > has already been existed in uevent"<br>
> > thus wwid of each path will get by
methods of configuration as old<br>
> > ways.<br>
> ><br>
> > The premise is that, devices whose
"merge_id" is the same have the<br>
> > same "wwid". I think this
premise can be established.<br>
> <br>
> So, if you have device where the merge_id doesn't equal
the wwid, you<br>
> call domap on that device, as it it wasn't part of a
merged set?<br>
> <br>
> That would work, but you would have to be careful to
deal with the case<br>
> where the last uevent in the set, the one that would
be used to call<br>
> domap for the whole merged set, suddenly gets processed
all on it's own,<br>
> because it had a different wwid from the rest of the
set. In that case<br>
> You would need to make sure that something called domap
on the other<br>
> members of the merged set. Now, I admit that this
is a very unlikely<br>
> thing to happen.<br>
> <br>
> I thought about proposing that we revert patch 12, but
it does add<br>
> complexity if you find that only some of your merged
devices change<br>
> wwid. Also, if all of them change to the same new wwid,
you just skipped<br>
> merging when you should be able to. But I'm not against
reverting it, as<br>
> long as we handle everything o.k. Reverting it would
mean that we don't<br>
> have to worry about removing a capability that multipath
previously had.<br>
> There's just a trade-off between supporting a capability
(that we don't<br>
> know if anyone uses), and maintaining less complex code.<br>
> <br>
> As long as it is possible to configure what uevent variable
you check<br>
> for by device type, I don't know of a situation where
this will cause<br>
> real world problems. It's possible that some users are
changing<br>
> uid_attribute for only certain scsi-based arrays, but
I don't know of<br>
> any cases. If somebody reading this knows of any cases,
please speak up.<br>
> But I would like to have the variable you check be configurable
so that<br>
> it's flexible with regards to new device types or the
possiblity of<br>
> differing uevent variable names either between distributions
or over<br>
> time. Does that sound reasonable?<br>
> <br>
> -Ben<br>
> <br>
> ><br>
> > how do you think about the above proposal?<br>
> ><br>
> > > The other option
would be to not actually merge the uevents, but<br>
> > simply<br>
> > > run through the filtered
but unmerged list of uevents, and skip<br>
> the<br>
> > > domap stuff but remember
the maps that need pushing to<br>
> device-mapper.<br>
> > > Once you are done
processing all the uevents, except for<br>
> updating the<br>
> > > maps in device-mapper,
you go back and update all the maps that<br>
> need<br>
> > > updating. There's
more code refactoring in this approach, but it<br>
> > keeps<br>
> > > the uid being set
in pathinfo, where you have all the<br>
> information<br>
> > > necessary to set
it using uid_attribute, getuid, or specialized<br>
> code<br>
> > > like rbd uses.<br>
> > I think it is ok, but a little complex.
and if we can get rid of the<br>
> > "wwid" issue, we do not need
to do so.<br>
> ><br>
> > Regards<br>
> > Tang Junhui<br>
> ><br>
> > 发件人:
"Benjamin Marzinski" <bmarzins@redhat.com><br>
> > 收件人:
tang.junhui@zte.com.cn,<br>
> > 抄送: tang.wenjun3@zte.com.cn,
zhang.kai16@zte.com.cn,<br>
> > dm-devel@redhat.com, bart.vanassche@sandisk.com,
mwilck@suse.com<br>
> > 日期:
2017/01/05 02:23<br>
> > 主题: Re:
[dm-devel] [PATCH 01/12] libmultipath: add wwid for<br>
> > "struct uevent" to record
wwid of uevent<br>
> > 发件人: dm-devel-bounces@redhat.com<br>
> ><br>
> > <br>
> --------------------------------------------------------------------------<br>
> ><br>
> > On Wed, Jan 04, 2017 at 02:56:46PM
+0800, tang.junhui@zte.com.cn<br>
> wrote:<br>
> > > Hello Ben,<br>
> > ><br>
> > > > Right now, multipath
users are allowed configure devices to<br>
> set the<br>
> > wwid<br>
> > > > based on any
udev environment variable (or even use a callout,<br>
> > although<br>
> > > > this is deprecated).
With this patch, that breaks.<br>
> > > Does WWID obtained
by different methods change? If it changes,<br>
> we<br>
> > would<br>
> > > better to modify
code to keep it no change.<br>
> ><br>
> > Yes. users can pick any udev environment
variable (and currently, any<br>
> > callout program that they write) to
use as the wwid.<br>
> ><br>
> > > > If the udev
sets<br>
> > > > ID_SERIAL for
a device, that is its wwid, right?<br>
> > > Yes<br>
> > ><br>
> > > > Do you know
if rbd<br>
> > > > devices have
ID_SERIAL set?<br>
> > > WWID has different
label in uevents for different devices, I<br>
> only<br>
> > test for<br>
> > > SCSI devices.<br>
> ><br>
> > Where is that check? I only see a check
for whether ID_SERIAL exists.<br>
> If<br>
> > it exists on things that are not scsi
devices, you will set the wwid<br>
> for<br>
> > these devices with it as well, even
if it doesn't work for them.<br>
> ><br>
> > > Now we do not support
rbd divice for uevents merging, these<br>
> > > device process as
old way, it has no harm in logic. If we need<br>
> to<br>
> > merge<br>
> > > rbd uevents for these
devices, we can add code to get wwid from<br>
> > uevents<br>
> > > and it can be supported
easily.<br>
> ><br>
> > Look at get_rbd_uid(), and how it's
called. rbd devices don't even<br>
> use<br>
> > the getuid callout or uid_attribute.
They use special code called by<br>
> > get_uid.<br>
> ><br>
> > Now you could add explicit checks so
we only check ID_SERIAL for scsi<br>
> > devices, ID_UID for dasd devices, and
never set the wwid otherwise.<br>
> You<br>
> > could even make the attribute we check
configurable by device type<br>
> with<br>
> > an option like<br>
> ><br>
> > uid_attrs "sd:ID_SERIAL dasd:ID_UID"<br>
> ><br>
> > in the defaults section, that would
set up mappings between device<br>
> types<br>
> > and uevent attributes to check for
the uid. But this would be on per<br>
> > device types, not per storage array,
like it currently is.<br>
> uid_attribute<br>
> > and getuid attribute would only ever
be used for device types that<br>
> > weren't in the uid_attrs list.<br>
> ><br>
> > The other option would be to not actually
merge the uevents, but<br>
> simply<br>
> > run through the filtered but unmerged
list of uevents, and skip the<br>
> > domap stuff but remember the maps that
need pushing to device-mapper.<br>
> > Once you are done processing all the
uevents, except for updating the<br>
> > maps in device-mapper, you go back
and update all the maps that need<br>
> > updating. There's more code refactoring
in this approach, but it<br>
> keeps<br>
> > the uid being set in pathinfo, where
you have all the information<br>
> > necessary to set it using uid_attribute,
getuid, or specialized code<br>
> > like rbd uses.<br>
> ><br>
> > As long as we make sure that we are
only checking specific uevent<br>
> > attributes for specific device types,
I'm o.k. with your way, but we<br>
> are<br>
> > losing flexibility that multipath has
always had in regards to<br>
> setting<br>
> > the wwid. I want to point that out
so that anyone who needs this<br>
> knows<br>
> > that it is changing.<br>
> ><br>
> > -Ben<br>
> ><br>
> > > Regards<br>
> > > Tang Junhui<br>
> > ><br>
> > > ������:
"Benjamin Marzinski" <bmarzins@redhat.com><br>
> > > �ռ���:
tang.junhui@zte.com.cn,<br>
> > > ����:
christophe.varoqui@opensvc.com, hare@suse.de,<br>
> > > mwilck@suse.com,
bart.vanassche@sandisk.com,<br>
> dm-devel@redhat.com,<br>
> > > zhang.kai16@zte.com.cn,
tang.wenjun3@zte.com.cn<br>
> > > ����:
2017/01/04 06:03<br>
> > > ����:
Re: [PATCH 01/12] libmultipath: add wwid for<br>
> "struct<br>
> > uevent"<br>
> > > to record wwid of
uevent<br>
> > ><br>
> > > <br>
> > <br>
> --------------------------------------------------------------------------<br>
> > ><br>
> > > On Tue, Dec 27, 2016
at 04:03:18PM +0800, tang.junhui@zte.com.cn<br>
> > wrote:<br>
> > > > From: tang.junhui
<tang.junhui@zte.com.cn><br>
> > > ><br>
> > > > Add "char
*wwid" to point WWID of uevent. This member<br>
> identifies<br>
> > > > the LUN ID which
the path belongs to, and it is used for<br>
> merging<br>
> > > > uevents. WWID
possibly did not exist in uevent yet, so ->wwid<br>
> > > > would be NULL,
those uevents would not be merged, but be<br>
> proccessed<br>
> > > > as old way.<br>
> > ><br>
> > > Right now, multipath
users are allowed configure devices to set<br>
> the<br>
> > wwid<br>
> > > based on any udev
environment variable (or even use a callout,<br>
> > although<br>
> > > this is deprecated).
With this patch, that breaks. If the udev<br>
> sets<br>
> > > ID_SERIAL for a device,
that is its wwid, right? Do you know if<br>
> rbd<br>
> > > devices have ID_SERIAL
set? If so, this change will break them.<br>
> Even<br>
> > if<br>
> > > this change doesn't
break any devices in their default<br>
> > configurations,<br>
> > > we would need to
disallow changing how the wwid is set for this<br>
> patch<br>
> > > to be safe.<br>
> > ><br>
> > > -Ben<br>
> > ><br>
> > > ><br>
> > > > Change-Id: Ie6b076363b3735dc7de10184b27fa799b499af0e<br>
> > > > Signed-off-by:
tang.junhui <tang.junhui@zte.com.cn><br>
> > > > ---<br>
> > > > libmultipath/uevent.c
| 2 ++<br>
> > > > libmultipath/uevent.h
| 1 +<br>
> > > > 2 files
changed, 3 insertions(+)<br>
> > > ><br>
> > > > diff --git a/libmultipath/uevent.c
b/libmultipath/uevent.c<br>
> > > > index 7edcce1..ef1bafe
100644<br>
> > > > --- a/libmultipath/uevent.c<br>
> > > > +++ b/libmultipath/uevent.c<br>
> > > > @@ -424,6 +424,8
@@ struct uevent<br>
> *uevent_from_udev_device(struct<br>
> > > udev_device *dev)<br>
> > > >
<br>
> uev->devpath =<br>
> > > uev->envp[i] +
8;<br>
> > > >
if (strcmp(name, "ACTION")<br>
> == 0)<br>
> > > >
<br>
> uev->action =<br>
> > > uev->envp[i] +
7;<br>
> > > > +
if (strcmp(name,<br>
> "ID_SERIAL") ==<br>
> > 0)<br>
> > > > +
uev->wwid<br>
> =<br>
> > > uev->envp[i] +
10;<br>
> > > >
i++;<br>
> > > >
if (i == HOTPLUG_NUM_ENVP -<br>
> 1)<br>
> > > >
break;<br>
> > > > diff --git a/libmultipath/uevent.h
b/libmultipath/uevent.h<br>
> > > > index 9d22dcd..7bfccef
100644<br>
> > > > --- a/libmultipath/uevent.h<br>
> > > > +++ b/libmultipath/uevent.h<br>
> > > > @@ -22,6 +22,7
@@ struct uevent {<br>
> > > >
char *devpath;<br>
> > > >
char *action;<br>
> > > >
char *kernel;<br>
> > > > +
char *wwid;<br>
> > > >
unsigned long seqnum;<br>
> > > >
char *envp[HOTPLUG_NUM_ENVP];<br>
> > > > };<br>
> > > > --<br>
> > > > 2.8.1.windows.1<br>
> > > ><br>
> ><br>
> > --<br>
> > dm-devel mailing list<br>
> > dm-devel@redhat.com<br>
> > [1][1]</font></tt><a href="https://www.redhat.com/mailman/listinfo/dm-devel"><tt><font size=2>https://www.redhat.com/mailman/listinfo/dm-devel</font></tt></a><tt><font size=2><br>
> ><br>
> > References<br>
> ><br>
> > Visible links<br>
> > 1. [2]</font></tt><a href="https://www.redhat.com/mailman/listinfo/dm-devel"><tt><font size=2>https://www.redhat.com/mailman/listinfo/dm-devel</font></tt></a><tt><font size=2><br>
> <br>
> References<br>
> <br>
> Visible links<br>
> 1. </font></tt><a href="https://www.redhat.com/mailman/listinfo/dm-devel"><tt><font size=2>https://www.redhat.com/mailman/listinfo/dm-devel</font></tt></a><tt><font size=2><br>
> 2. </font></tt><a href="https://www.redhat.com/mailman/listinfo/dm-devel"><tt><font size=2>https://www.redhat.com/mailman/listinfo/dm-devel</font></tt></a><tt><font size=2><br>
<br>
--<br>
dm-devel mailing list<br>
dm-devel@redhat.com<br>
</font></tt><a href="https://www.redhat.com/mailman/listinfo/dm-devel"><tt><font size=2>https://www.redhat.com/mailman/listinfo/dm-devel</font></tt></a>
<br>