| Message ID | 1246069888.14164.53.camel@chandra-ubuntu (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
Chandra Seetharaman wrote: > Create the sysfs file, dh_state even if the new SCSI device is not > in the any of the device handler's internal lists. > > Signed-Off-by: Chandra Seetharaman <sekharan@us.ibm.com> > --- > drivers/scsi/device_handler/scsi_dh.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > Index: linux-2.6.31-rc1/drivers/scsi/device_handler/scsi_dh.c > =================================================================== > --- linux-2.6.31-rc1.orig/drivers/scsi/device_handler/scsi_dh.c > +++ linux-2.6.31-rc1/drivers/scsi/device_handler/scsi_dh.c > @@ -304,18 +304,15 @@ static int scsi_dh_notifier(struct notif > sdev = to_scsi_device(dev); > > if (action == BUS_NOTIFY_ADD_DEVICE) { > + err = device_create_file(dev, &scsi_dh_state_attr); > + /* don't care about err */ > devinfo = device_handler_match(NULL, sdev); > - if (!devinfo) > - goto out; > - > - err = scsi_dh_handler_attach(sdev, devinfo); > - if (!err) > - err = device_create_file(dev, &scsi_dh_state_attr); > + if (devinfo) > + err = scsi_dh_handler_attach(sdev, devinfo); > } else if (action == BUS_NOTIFY_DEL_DEVICE) { > device_remove_file(dev, &scsi_dh_state_attr); > scsi_dh_handler_detach(sdev, NULL); > } > -out: > return err; > } > > > > -- > dm-devel mailing list > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel NACK. This will create sysfs attributes even if the attach() failed for other reason like a generic error. So we'll end up with device handler attributes and no device handler attached. Not a good idea. Cheers, Hannes
On Mon, 2009-06-29 at 09:40 +0200, Hannes Reinecke wrote: > Chandra Seetharaman wrote: > > Create the sysfs file, dh_state even if the new SCSI device is not > > in the any of the device handler's internal lists. > > > > Signed-Off-by: Chandra Seetharaman <sekharan@us.ibm.com> > > --- > > drivers/scsi/device_handler/scsi_dh.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > Index: linux-2.6.31-rc1/drivers/scsi/device_handler/scsi_dh.c > > =================================================================== > > --- linux-2.6.31-rc1.orig/drivers/scsi/device_handler/scsi_dh.c > > +++ linux-2.6.31-rc1/drivers/scsi/device_handler/scsi_dh.c > > @@ -304,18 +304,15 @@ static int scsi_dh_notifier(struct notif > > sdev = to_scsi_device(dev); > > > > if (action == BUS_NOTIFY_ADD_DEVICE) { > > + err = device_create_file(dev, &scsi_dh_state_attr); > > + /* don't care about err */ > > devinfo = device_handler_match(NULL, sdev); > > - if (!devinfo) > > - goto out; > > - > > - err = scsi_dh_handler_attach(sdev, devinfo); > > - if (!err) > > - err = device_create_file(dev, &scsi_dh_state_attr); > > + if (devinfo) > > + err = scsi_dh_handler_attach(sdev, devinfo); > > } else if (action == BUS_NOTIFY_DEL_DEVICE) { > > device_remove_file(dev, &scsi_dh_state_attr); > > scsi_dh_handler_detach(sdev, NULL); > > } > > -out: > > return err; > > } > > > > > > > > -- > > dm-devel mailing list > > dm-devel@redhat.com > > https://www.redhat.com/mailman/listinfo/dm-devel > NACK. > > This will create sysfs attributes even if the attach() > failed for other reason like a generic error. So we'll end > up with device handler attributes and no device handler attached. > > Not a good idea. It strikes me that what is really being reinvented here is driver attachment with driver attribute files ... Now all of this could be avoided if we had multiple attachment of drivers ... James -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, 2009-06-29 at 09:40 +0200, Hannes Reinecke wrote: > Chandra Seetharaman wrote: > > Create the sysfs file, dh_state even if the new SCSI device is not > > in the any of the device handler's internal lists. > > > > Signed-Off-by: Chandra Seetharaman <sekharan@us.ibm.com> > > --- > > drivers/scsi/device_handler/scsi_dh.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > Index: linux-2.6.31-rc1/drivers/scsi/device_handler/scsi_dh.c > > =================================================================== > > --- linux-2.6.31-rc1.orig/drivers/scsi/device_handler/scsi_dh.c > > +++ linux-2.6.31-rc1/drivers/scsi/device_handler/scsi_dh.c > > @@ -304,18 +304,15 @@ static int scsi_dh_notifier(struct notif > > sdev = to_scsi_device(dev); > > > > if (action == BUS_NOTIFY_ADD_DEVICE) { > > + err = device_create_file(dev, &scsi_dh_state_attr); > > + /* don't care about err */ > > devinfo = device_handler_match(NULL, sdev); > > - if (!devinfo) > > - goto out; > > - > > - err = scsi_dh_handler_attach(sdev, devinfo); > > - if (!err) > > - err = device_create_file(dev, &scsi_dh_state_attr); > > + if (devinfo) > > + err = scsi_dh_handler_attach(sdev, devinfo); > > } else if (action == BUS_NOTIFY_DEL_DEVICE) { > > device_remove_file(dev, &scsi_dh_state_attr); > > scsi_dh_handler_detach(sdev, NULL); > > } > > -out: > > return err; > > } > > > > > > > > -- > > dm-devel mailing list > > dm-devel@redhat.com > > https://www.redhat.com/mailman/listinfo/dm-devel > NACK. > > This will create sysfs attributes even if the attach() > failed for other reason like a generic error. So we'll end > up with device handler attributes and no device handler attached. we do not have to worry about if attach failed or succeeded. We will have "detached" if there is no device handler is attached to a device. Basically, existence of this file simply means that scsi_dh module is active, only the contents of this file will indicate if any device handler is attached or not. We are just creating the file dh_state for each SCSI device that exists and that is the only way non-in-built devices can be attached to a handler by the end-user. Do note that we do create this file for all the existing scsi devices when the scsi_dh module is inserted (see scsi_dh_init()). > > Not a good idea. I do not agree, and we do need to have this fix for dh_state file to be functionally useful. > > Cheers, > > Hannes -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
James Bottomley wrote: > On Mon, 2009-06-29 at 09:40 +0200, Hannes Reinecke wrote: >> Chandra Seetharaman wrote: >>> Create the sysfs file, dh_state even if the new SCSI device is not >>> in the any of the device handler's internal lists. >>> >>> Signed-Off-by: Chandra Seetharaman <sekharan@us.ibm.com> >>> --- >>> drivers/scsi/device_handler/scsi_dh.c | 11 ++++------- >>> 1 file changed, 4 insertions(+), 7 deletions(-) >>> >>> Index: linux-2.6.31-rc1/drivers/scsi/device_handler/scsi_dh.c >>> =================================================================== >>> --- linux-2.6.31-rc1.orig/drivers/scsi/device_handler/scsi_dh.c >>> +++ linux-2.6.31-rc1/drivers/scsi/device_handler/scsi_dh.c >>> @@ -304,18 +304,15 @@ static int scsi_dh_notifier(struct notif >>> sdev = to_scsi_device(dev); >>> >>> if (action == BUS_NOTIFY_ADD_DEVICE) { >>> + err = device_create_file(dev, &scsi_dh_state_attr); >>> + /* don't care about err */ >>> devinfo = device_handler_match(NULL, sdev); >>> - if (!devinfo) >>> - goto out; >>> - >>> - err = scsi_dh_handler_attach(sdev, devinfo); >>> - if (!err) >>> - err = device_create_file(dev, &scsi_dh_state_attr); >>> + if (devinfo) >>> + err = scsi_dh_handler_attach(sdev, devinfo); >>> } else if (action == BUS_NOTIFY_DEL_DEVICE) { >>> device_remove_file(dev, &scsi_dh_state_attr); >>> scsi_dh_handler_detach(sdev, NULL); >>> } >>> -out: >>> return err; >>> } >>> >>> >>> >> NACK. >> >> This will create sysfs attributes even if the attach() >> failed for other reason like a generic error. So we'll end >> up with device handler attributes and no device handler attached. >> >> Not a good idea. > > It strikes me that what is really being reinvented here is driver > attachment with driver attribute files ... > > Now all of this could be avoided if we had multiple attachment of > drivers ... > I heard you. If you find someone to fix the blasted iSCSI I/O stall for me ... Cheers, Hannes
Hannes, any comments ? chandra On Mon, 2009-06-29 at 12:30 -0700, Chandra Seetharaman wrote: > On Mon, 2009-06-29 at 09:40 +0200, Hannes Reinecke wrote: > > Chandra Seetharaman wrote: > > > Create the sysfs file, dh_state even if the new SCSI device is not > > > in the any of the device handler's internal lists. > > > > > > Signed-Off-by: Chandra Seetharaman <sekharan@us.ibm.com> > > > --- > > > drivers/scsi/device_handler/scsi_dh.c | 11 ++++------- > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > Index: linux-2.6.31-rc1/drivers/scsi/device_handler/scsi_dh.c > > > =================================================================== > > > --- linux-2.6.31-rc1.orig/drivers/scsi/device_handler/scsi_dh.c > > > +++ linux-2.6.31-rc1/drivers/scsi/device_handler/scsi_dh.c > > > @@ -304,18 +304,15 @@ static int scsi_dh_notifier(struct notif > > > sdev = to_scsi_device(dev); > > > > > > if (action == BUS_NOTIFY_ADD_DEVICE) { > > > + err = device_create_file(dev, &scsi_dh_state_attr); > > > + /* don't care about err */ > > > devinfo = device_handler_match(NULL, sdev); > > > - if (!devinfo) > > > - goto out; > > > - > > > - err = scsi_dh_handler_attach(sdev, devinfo); > > > - if (!err) > > > - err = device_create_file(dev, &scsi_dh_state_attr); > > > + if (devinfo) > > > + err = scsi_dh_handler_attach(sdev, devinfo); > > > } else if (action == BUS_NOTIFY_DEL_DEVICE) { > > > device_remove_file(dev, &scsi_dh_state_attr); > > > scsi_dh_handler_detach(sdev, NULL); > > > } > > > -out: > > > return err; > > > } > > > > > > > > > > > > -- > > > dm-devel mailing list > > > dm-devel@redhat.com > > > https://www.redhat.com/mailman/listinfo/dm-devel > > NACK. > > > > This will create sysfs attributes even if the attach() > > failed for other reason like a generic error. So we'll end > > up with device handler attributes and no device handler attached. > > we do not have to worry about if attach failed or succeeded. We will > have "detached" if there is no device handler is attached to a device. > > Basically, existence of this file simply means that scsi_dh module is > active, only the contents of this file will indicate if any device > handler is attached or not. > > We are just creating the file dh_state for each SCSI device that exists > and that is the only way non-in-built devices can be attached to a > handler by the end-user. > > Do note that we do create this file for all the existing scsi devices > when the scsi_dh module is inserted (see scsi_dh_init()). > > > > > Not a good idea. > > I do not agree, and we do need to have this fix for dh_state file to be > functionally useful. > > > > > Cheers, > > > > Hannes -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Hi Hannes, Can you comment on this please. If you are convinced with my justification, please send an ACK. Thanks & Regards, chandra On Mon, 2009-06-29 at 12:30 -0700, Chandra Seetharaman wrote: > On Mon, 2009-06-29 at 09:40 +0200, Hannes Reinecke wrote: > > Chandra Seetharaman wrote: > > > Create the sysfs file, dh_state even if the new SCSI device is not > > > in the any of the device handler's internal lists. > > > > > > Signed-Off-by: Chandra Seetharaman <sekharan@us.ibm.com> > > > --- > > > drivers/scsi/device_handler/scsi_dh.c | 11 ++++------- > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > Index: linux-2.6.31-rc1/drivers/scsi/device_handler/scsi_dh.c > > > =================================================================== > > > --- linux-2.6.31-rc1.orig/drivers/scsi/device_handler/scsi_dh.c > > > +++ linux-2.6.31-rc1/drivers/scsi/device_handler/scsi_dh.c > > > @@ -304,18 +304,15 @@ static int scsi_dh_notifier(struct notif > > > sdev = to_scsi_device(dev); > > > > > > if (action == BUS_NOTIFY_ADD_DEVICE) { > > > + err = device_create_file(dev, &scsi_dh_state_attr); > > > + /* don't care about err */ > > > devinfo = device_handler_match(NULL, sdev); > > > - if (!devinfo) > > > - goto out; > > > - > > > - err = scsi_dh_handler_attach(sdev, devinfo); > > > - if (!err) > > > - err = device_create_file(dev, &scsi_dh_state_attr); > > > + if (devinfo) > > > + err = scsi_dh_handler_attach(sdev, devinfo); > > > } else if (action == BUS_NOTIFY_DEL_DEVICE) { > > > device_remove_file(dev, &scsi_dh_state_attr); > > > scsi_dh_handler_detach(sdev, NULL); > > > } > > > -out: > > > return err; > > > } > > > > > > > > > > > > -- > > > dm-devel mailing list > > > dm-devel@redhat.com > > > https://www.redhat.com/mailman/listinfo/dm-devel > > NACK. > > > > This will create sysfs attributes even if the attach() > > failed for other reason like a generic error. So we'll end > > up with device handler attributes and no device handler attached. > > we do not have to worry about if attach failed or succeeded. We will > have "detached" if there is no device handler is attached to a device. > > Basically, existence of this file simply means that scsi_dh module is > active, only the contents of this file will indicate if any device > handler is attached or not. > > We are just creating the file dh_state for each SCSI device that exists > and that is the only way non-in-built devices can be attached to a > handler by the end-user. > > Do note that we do create this file for all the existing scsi devices > when the scsi_dh module is inserted (see scsi_dh_init()). > > > > > Not a good idea. > > I do not agree, and we do need to have this fix for dh_state file to be > functionally useful. > > > > > Cheers, > > > > Hannes -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Chandra Seetharaman wrote: > Hi Hannes, > > Can you comment on this please. > > If you are convinced with my justification, please send an ACK. > > Thanks & Regards, > > chandra > On Mon, 2009-06-29 at 12:30 -0700, Chandra Seetharaman wrote: >> On Mon, 2009-06-29 at 09:40 +0200, Hannes Reinecke wrote: >>> Chandra Seetharaman wrote: >>>> Create the sysfs file, dh_state even if the new SCSI device is not >>>> in the any of the device handler's internal lists. >>>> >>>> Signed-Off-by: Chandra Seetharaman <sekharan@us.ibm.com> >>>> --- >>>> drivers/scsi/device_handler/scsi_dh.c | 11 ++++------- >>>> 1 file changed, 4 insertions(+), 7 deletions(-) >>>> >>>> Index: linux-2.6.31-rc1/drivers/scsi/device_handler/scsi_dh.c >>>> =================================================================== >>>> --- linux-2.6.31-rc1.orig/drivers/scsi/device_handler/scsi_dh.c >>>> +++ linux-2.6.31-rc1/drivers/scsi/device_handler/scsi_dh.c >>>> @@ -304,18 +304,15 @@ static int scsi_dh_notifier(struct notif >>>> sdev = to_scsi_device(dev); >>>> >>>> if (action == BUS_NOTIFY_ADD_DEVICE) { >>>> + err = device_create_file(dev, &scsi_dh_state_attr); >>>> + /* don't care about err */ >>>> devinfo = device_handler_match(NULL, sdev); >>>> - if (!devinfo) >>>> - goto out; >>>> - >>>> - err = scsi_dh_handler_attach(sdev, devinfo); >>>> - if (!err) >>>> - err = device_create_file(dev, &scsi_dh_state_attr); >>>> + if (devinfo) >>>> + err = scsi_dh_handler_attach(sdev, devinfo); >>>> } else if (action == BUS_NOTIFY_DEL_DEVICE) { >>>> device_remove_file(dev, &scsi_dh_state_attr); >>>> scsi_dh_handler_detach(sdev, NULL); >>>> } >>>> -out: >>>> return err; >>>> } >>>> >>>> >>>> >>>> -- >>>> dm-devel mailing list >>>> dm-devel@redhat.com >>>> https://www.redhat.com/mailman/listinfo/dm-devel >>> NACK. >>> >>> This will create sysfs attributes even if the attach() >>> failed for other reason like a generic error. So we'll end >>> up with device handler attributes and no device handler attached. >> we do not have to worry about if attach failed or succeeded. We will >> have "detached" if there is no device handler is attached to a device. >> >> Basically, existence of this file simply means that scsi_dh module is >> active, only the contents of this file will indicate if any device >> handler is attached or not. >> >> We are just creating the file dh_state for each SCSI device that exists >> and that is the only way non-in-built devices can be attached to a >> handler by the end-user. >> >> Do note that we do create this file for all the existing scsi devices >> when the scsi_dh module is inserted (see scsi_dh_init()). >> Right. So you're changing the semantics from the 'dh_state' attribute to mean 'scsi_dh' is active, not 'a scsi_dh module has been attached'. Ok, that seems reasonable. Acked-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
Index: linux-2.6.31-rc1/drivers/scsi/device_handler/scsi_dh.c =================================================================== --- linux-2.6.31-rc1.orig/drivers/scsi/device_handler/scsi_dh.c +++ linux-2.6.31-rc1/drivers/scsi/device_handler/scsi_dh.c @@ -304,18 +304,15 @@ static int scsi_dh_notifier(struct notif sdev = to_scsi_device(dev); if (action == BUS_NOTIFY_ADD_DEVICE) { + err = device_create_file(dev, &scsi_dh_state_attr); + /* don't care about err */ devinfo = device_handler_match(NULL, sdev); - if (!devinfo) - goto out; - - err = scsi_dh_handler_attach(sdev, devinfo); - if (!err) - err = device_create_file(dev, &scsi_dh_state_attr); + if (devinfo) + err = scsi_dh_handler_attach(sdev, devinfo); } else if (action == BUS_NOTIFY_DEL_DEVICE) { device_remove_file(dev, &scsi_dh_state_attr); scsi_dh_handler_detach(sdev, NULL); } -out: return err; }
Create the sysfs file, dh_state even if the new SCSI device is not in the any of the device handler's internal lists. Signed-Off-by: Chandra Seetharaman <sekharan@us.ibm.com> --- drivers/scsi/device_handler/scsi_dh.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel