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