| Message ID | 20220125162441.2226-1-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] scsi: make "access_state" sysfs attribute always visible | expand |
On Tue, Jan 25, 2022 at 05:24:41PM +0100, mwilck@suse.com wrote: > From: Martin Wilck <mwilck@suse.com> > > If a SCSI device handler module is loaded after some SCSI devices > have already been probed (e.g. via request_module() by dm-multipath), > the "access_state" and "preferred_path" sysfs attributes remain invisible for > these devices, although the handler is attached and live. The reason is > that the visibility is only checked when the sysfs attribute group is > first created. This results in an inconsistent user experience depending > on the load order of SCSI low-level drivers vs. device handler modules. > > This patch changes user space API: attempting to read the "access_state" > or "preferred_path" attributes will now result in -EINVAL rather than > -ENODEV for devices that have no device handler, and tests for the existence > of these attributes will have a different result. Sounds fine: Reviewed-by: Christoph Hellwig <hch@lst.de>
On 1/25/22 08:24, mwilck@suse.com wrote: > From: Martin Wilck <mwilck@suse.com> > > If a SCSI device handler module is loaded after some SCSI devices > have already been probed (e.g. via request_module() by dm-multipath), > the "access_state" and "preferred_path" sysfs attributes remain invisible for > these devices, although the handler is attached and live. The reason is > that the visibility is only checked when the sysfs attribute group is > first created. This results in an inconsistent user experience depending > on the load order of SCSI low-level drivers vs. device handler modules. Isn't this something that should be fixed in the sysfs code rather than in the SCSI core? If this issue affects SCSI I assume that it will also affect other sysfs users. Thanks, Bart.
On 1/27/22 18:28, Bart Van Assche wrote: > On 1/25/22 08:24, mwilck@suse.com wrote: >> From: Martin Wilck <mwilck@suse.com> >> >> If a SCSI device handler module is loaded after some SCSI devices >> have already been probed (e.g. via request_module() by dm-multipath), >> the "access_state" and "preferred_path" sysfs attributes remain >> invisible for >> these devices, although the handler is attached and live. The reason is >> that the visibility is only checked when the sysfs attribute group is >> first created. This results in an inconsistent user experience depending >> on the load order of SCSI low-level drivers vs. device handler modules. > > Isn't this something that should be fixed in the sysfs code rather than > in the SCSI core? If this issue affects SCSI I assume that it will also > affect other sysfs users. > Urgh. Rather not. That particular code affects the visibility of sysfs attributes; they are created statically in device_add(), so it won't even be created if it's not visible. Reworking that would mean a rework of the entire drivers/base code. And not to mention a change in behaviour, as some drivers might well rely on the current behaviour. But if you feel up to it ... Cheers, Hannes
On Thu, 2022-01-27 at 09:28 -0800, Bart Van Assche wrote: > On 1/25/22 08:24, mwilck@suse.com wrote: > > From: Martin Wilck <mwilck@suse.com> > > > > If a SCSI device handler module is loaded after some SCSI devices > > have already been probed (e.g. via request_module() by dm- > > multipath), > > the "access_state" and "preferred_path" sysfs attributes remain > > invisible for > > these devices, although the handler is attached and live. The > > reason is > > that the visibility is only checked when the sysfs attribute group > > is > > first created. This results in an inconsistent user experience > > depending > > on the load order of SCSI low-level drivers vs. device handler > > modules. > > Isn't this something that should be fixed in the sysfs code rather > than > in the SCSI core? If this issue affects SCSI I assume that it will > also > affect other sysfs users. Well, there's sysfs_update_groups() which could be used for this purpose in principle, I suppose. But there's no API for calling it in the driver core (there is no device_update_groups() or or device_update_attrs()), probably for good reason. Making the attribute visible even if there's no device handler is simpler, and less error-prone. IOW, I agree with Hannes. Regards, Martin
On 25.01.22 18:24, mwilck@suse.com wrote: > From: Martin Wilck <mwilck@suse.com> > > If a SCSI device handler module is loaded after some SCSI devices > have already been probed (e.g. via request_module() by dm-multipath), > the "access_state" and "preferred_path" sysfs attributes remain invisible for > these devices, although the handler is attached and live. The reason is > that the visibility is only checked when the sysfs attribute group is > first created. This results in an inconsistent user experience depending > on the load order of SCSI low-level drivers vs. device handler modules. > I suppose you looked at sysfs_update_group(), and it's not a good fit? > This patch changes user space API: attempting to read the "access_state" > or "preferred_path" attributes will now result in -EINVAL rather than > -ENODEV for devices that have no device handler, and tests for the existence > of these attributes will have a different result. > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > drivers/scsi/scsi_sysfs.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c > index f1e0c131b77c..226a50944c00 100644 > --- a/drivers/scsi/scsi_sysfs.c > +++ b/drivers/scsi/scsi_sysfs.c > @@ -1228,14 +1228,6 @@ static umode_t scsi_sdev_attr_is_visible(struct kobject *kobj, > !sdev->host->hostt->change_queue_depth) > return 0; > > -#ifdef CONFIG_SCSI_DH > - if (attr == &dev_attr_access_state.attr && > - !sdev->handler) > - return 0; > - if (attr == &dev_attr_preferred_path.attr && > - !sdev->handler) > - return 0; > -#endif > return attr->mode; > } > ]
On Fri, 2022-01-28 at 14:38 +0200, Julian Wiedmann wrote: > On 25.01.22 18:24, mwilck@suse.com wrote: > > From: Martin Wilck <mwilck@suse.com> > > > > If a SCSI device handler module is loaded after some SCSI devices > > have already been probed (e.g. via request_module() by dm- > > multipath), > > the "access_state" and "preferred_path" sysfs attributes remain > > invisible for > > these devices, although the handler is attached and live. The > > reason is > > that the visibility is only checked when the sysfs attribute group > > is > > first created. This results in an inconsistent user experience > > depending > > on the load order of SCSI low-level drivers vs. device handler > > modules. > > > > I suppose you looked at sysfs_update_group(), and it's not a good > fit? I admit I'm afraid of introducing race conditions when I update the visibility of attributes of live SCSI devices. I believe that'd be much harder to get right, and I don't see what's wrong with simply always making the attribute visible (other than a rather minimal user API change, which hardly any user will notice). Martin
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index f1e0c131b77c..226a50944c00 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -1228,14 +1228,6 @@ static umode_t scsi_sdev_attr_is_visible(struct kobject *kobj, !sdev->host->hostt->change_queue_depth) return 0; -#ifdef CONFIG_SCSI_DH - if (attr == &dev_attr_access_state.attr && - !sdev->handler) - return 0; - if (attr == &dev_attr_preferred_path.attr && - !sdev->handler) - return 0; -#endif return attr->mode; }