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@xxxxxxxxxx>
收件人: tang.junhui@xxxxxxxxxx,
抄送: tang.wenjun3@xxxxxxxxxx, zhang.kai16@xxxxxxxxxx, dm-devel@xxxxxxxxxx, bart.vanassche@xxxxxxxxxxx, mwilck@xxxxxxxx
日期: 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@xxxxxxxxxx
On Wed, Jan 04, 2017 at 02:56:46PM +0800, tang.junhui@xxxxxxxxxx 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@xxxxxxxxxx>
> �ռ���: tang.junhui@xxxxxxxxxx,
> ����: christophe.varoqui@xxxxxxxxxxx, hare@xxxxxxx,
> mwilck@xxxxxxxx, bart.vanassche@xxxxxxxxxxx, dm-devel@xxxxxxxxxx,
> zhang.kai16@xxxxxxxxxx, tang.wenjun3@xxxxxxxxxx
> ����: 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@xxxxxxxxxx wrote:
> > From: tang.junhui <tang.junhui@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
发件人: "Benjamin Marzinski" <bmarzins@xxxxxxxxxx>
收件人: tang.junhui@xxxxxxxxxx,
抄送: tang.wenjun3@xxxxxxxxxx, zhang.kai16@xxxxxxxxxx, dm-devel@xxxxxxxxxx, bart.vanassche@xxxxxxxxxxx, mwilck@xxxxxxxx
日期: 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@xxxxxxxxxx
On Wed, Jan 04, 2017 at 02:56:46PM +0800, tang.junhui@xxxxxxxxxx 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@xxxxxxxxxx>
> �ռ���: tang.junhui@xxxxxxxxxx,
> ����: christophe.varoqui@xxxxxxxxxxx, hare@xxxxxxx,
> mwilck@xxxxxxxx, bart.vanassche@xxxxxxxxxxx, dm-devel@xxxxxxxxxx,
> zhang.kai16@xxxxxxxxxx, tang.wenjun3@xxxxxxxxxx
> ����: 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@xxxxxxxxxx wrote:
> > From: tang.junhui <tang.junhui@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
-- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
