On Mon, 2017-05-15 at 08:04 +0200, Hannes Reinecke wrote:
> In general I like the idea; however, the 'filter' thingie is somewhat
> odd. I could somewhat buy into the idea of filtering for sense keys, but
> then I would have expected to use the 'sense_event_filter' as a bitmap
> of allowed sense keys.
> With your current design you can only filter for one specific sense key,
> _and_ you leave the remaining bits of the sense_event_filter variable
> unused.
> So either turn it into a bitmap and allow for several sense keys to be
> filtered, or treat it as mask for the _entire_ sense, but then you'd
> need for ASC and ASCQ, too.
>
> Cheers,
>
> Hannes
I think what would help here is understanding what you are trying to
accomplish with this change. Are you trying to allow generation of
a uevent on a particular sense KCQ combination you know in advance,
or a set of them, or do you want to be able to normally generate
uevents for every sense keys/codes (i.e. all the bitmap bits set)
and then perhaps narrow down to a specific one when you are looking
for a problem?
When I added the sense code uevent generation for Unit Attentions,
one big concern I had was how many events per second would be generated.
The userspace event handling is very slow, and can only handle a few
hundred events per second before it starts backing up, and then you
can lose events, which you don't want.
Generating a uevent on MEDIUM ERROR is somewhat worrying, when a drive
goes bad you could easily get these on every I/O.
Your actual uevent emit:
+#ifdef CONFIG_SCSI_SENSE_UEVENT
+ case SDEV_EVT_SCSI_SENSE:
+ envp[idx++] = "SDEV_UA=SCSI_SENSE";
+ for (i = idx; i < idx + 3; ++i) {
+ envp[i] = kzalloc(32, GFP_ATOMIC);
+ if (!envp[i])
+ break;
+ free_envp = i;
+ }
+ snprintf(envp[idx++], 32, "LBA=%lu", evt->sense_evt_data.lba);
+ snprintf(envp[idx++], 32, "SIZE=%d", evt->sense_evt_data.size);
+ snprintf(envp[idx++], 32, "SENSE_CODE=%1x/%02x/%02x",
+ evt->sense_evt_data.sshdr.sense_key,
+ evt->sense_evt_data.sshdr.asc,
+ evt->sense_evt_data.sshdr.ascq);
+ break;
+#endif
Is a little strange. The "SDEV_UA" property implies that it was a
UNIT ATTENTION, but you are filtering on other sense types, so you
would probably want a different property. Many SCSI commands do not
have a LOGICAL BLOCK ADDRESS field, or a TRANSFER LENGTH field, so
you might be reporting garbage if they don't. And, since this is a
SCSI command, you probably want to report the length from the command,
not the count in bytes from the request structure.
So I think a customizable sense reporting feature could be very useful,
but it needs a little more development.
-Ewan
