| Message ID | 20250120194925.44432-5-Kai.Makisara@kolumbus.fi (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: st: scsi_error: More reset patches | expand |
One suggestion here. On 1/20/25 2:49 PM, Kai Mäkisara wrote: > If the value read from the file is 1, reads and writes from/to the > device are blocked because the tape position may not match user's > expectation (tape rewound after device reset). > > Signed-off-by: Kai Mäkisara <Kai.Makisara@kolumbus.fi> > --- > Documentation/scsi/st.rst | 5 +++++ > drivers/scsi/st.c | 19 +++++++++++++++++++ > 2 files changed, 24 insertions(+) > > diff --git a/Documentation/scsi/st.rst b/Documentation/scsi/st.rst > index d3b28c28d74c..2209f03faad3 100644 > --- a/Documentation/scsi/st.rst > +++ b/Documentation/scsi/st.rst > @@ -157,6 +157,11 @@ enabled driver and mode options. The value in the file is a bit mask where the > bit definitions are the same as those used with MTSETDRVBUFFER in setting the > options. > > +Each directory contains the entry 'reset_blocked'. If this value is one, I suggest calling this 'position_lost' or 'position_unknown' rather than 'reset_blocked'. > +reading and writing to the device is blocked after device reset. Most > +devices rewind the tape after reset and the writes/read don't access the > +tape position the user expects. > + > A link named 'tape' is made from the SCSI device directory to the class > directory corresponding to the mode 0 auto-rewind device (e.g., st0). > > diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c > index 6ec1cfeb92ff..e4fda0954004 100644 > --- a/drivers/scsi/st.c > +++ b/drivers/scsi/st.c > @@ -4703,6 +4703,24 @@ options_show(struct device *dev, struct device_attribute *attr, char *buf) > } > static DEVICE_ATTR_RO(options); > > +/** > + * reset_blocked_show - Value 1 indicates that reads, writes, etc. are blocked position_lost_show > + * because a device reset has occurred and no operation positioning the tape > + * has been issued. > + * @dev: struct device > + * @attr: attribute structure > + * @buf: buffer to return formatted data in > + */ > +static ssize_t reset_blocked_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct st_modedef *STm = dev_get_drvdata(dev); > + struct scsi_tape *STp = STm->tape; > + > + return sprintf(buf, "%d", STp->pos_unknown); ^^^^^^^^^^^^^^ I'd like the name of this function/sysfs variable to more closely match the code that drives this state. > +} > +static DEVICE_ATTR_RO(reset_blocked); > + > /* Support for tape stats */ > > /** > @@ -4887,6 +4905,7 @@ static struct attribute *st_dev_attrs[] = { > &dev_attr_default_density.attr, > &dev_attr_default_compression.attr, > &dev_attr_options.attr, > + &dev_attr_reset_blocked.attr, > NULL, > }; > Otherwise, I've tested this out and everything works as expected! https://github.com/johnmeneghini/tape_tests/commit/9e04599605f4726050f9f2032b84df2daa6cd6ea Thanks!
> On 30. Jan 2025, at 20.27, John Meneghini <jmeneghi@redhat.com> wrote: > > One suggestion here. > > On 1/20/25 2:49 PM, Kai Mäkisara wrote: >> If the value read from the file is 1, reads and writes from/to the ... >> --- a/Documentation/scsi/st.rst >> +++ b/Documentation/scsi/st.rst >> @@ -157,6 +157,11 @@ enabled driver and mode options. The value in the file is a bit mask where the >> bit definitions are the same as those used with MTSETDRVBUFFER in setting the >> options. >> +Each directory contains the entry 'reset_blocked'. If this value is one, > > I suggest calling this 'position_lost' or 'position_unknown' rather than 'reset_blocked'. > >> +reading and writing to the device is blocked after device reset. Most >> +/** ... >> + * reset_blocked_show - Value 1 indicates that reads, writes, etc. are blocked > > position_lost_show Yes. The function name should be changed when the file name is changed. > >> + * because a device reset has occurred and no operation positioning the tape >> + * has been issued. >> + * @dev: struct device >> + * @attr: attribute structure >> + * @buf: buffer to return formatted data in >> + */ >> +static ssize_t reset_blocked_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct st_modedef *STm = dev_get_drvdata(dev); >> + struct scsi_tape *STp = STm->tape; >> + >> + return sprintf(buf, "%d", STp->pos_unknown); > ^^^^^^^^^^^^^^ > > I'd like the name of this function/sysfs variable to more closely match the code > that drives this state. I am not too fond of the name "reset_blocked", but I had to choose something :-) This name is based on the phenomenon causing the situation (device reset) and what it causes (blocks many st operations). I think "reset" should be part of the name. Device reset may mean that the tape position is changed (rewind), but it can also mean more: the device settings are reset to the saved values. Patch 1/4 restores the values set using st, but it does not restore the values set by other means (e.g., encryption). The user should remember to re-set also these before continuing. Combining the above and your suggestion, what about "position_lost_in_reset" or "pos_lost_in_reset"? (Whatever the name is, the user should check what has really happened. The name should point to the correct direction, but it should be short enough.) Thanks!
On 1/31/25 5:49 AM, "Kai Mäkisara (Kolumbus)" wrote: > Combining the above and your suggestion, what about "position_lost_in_reset" > or "pos_lost_in_reset"? (Whatever the name is, the user should check what > has really happened. The name should point to the correct direction, > but it should be short enough.) I like it! Reviewed-by: John Meneghini <jmeneghi@redhat.com> Tested-by: John Meneghini <jmeneghi@redhat.com>
diff --git a/Documentation/scsi/st.rst b/Documentation/scsi/st.rst index d3b28c28d74c..2209f03faad3 100644 --- a/Documentation/scsi/st.rst +++ b/Documentation/scsi/st.rst @@ -157,6 +157,11 @@ enabled driver and mode options. The value in the file is a bit mask where the bit definitions are the same as those used with MTSETDRVBUFFER in setting the options. +Each directory contains the entry 'reset_blocked'. If this value is one, +reading and writing to the device is blocked after device reset. Most +devices rewind the tape after reset and the writes/read don't access the +tape position the user expects. + A link named 'tape' is made from the SCSI device directory to the class directory corresponding to the mode 0 auto-rewind device (e.g., st0). diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c index 6ec1cfeb92ff..e4fda0954004 100644 --- a/drivers/scsi/st.c +++ b/drivers/scsi/st.c @@ -4703,6 +4703,24 @@ options_show(struct device *dev, struct device_attribute *attr, char *buf) } static DEVICE_ATTR_RO(options); +/** + * reset_blocked_show - Value 1 indicates that reads, writes, etc. are blocked + * because a device reset has occurred and no operation positioning the tape + * has been issued. + * @dev: struct device + * @attr: attribute structure + * @buf: buffer to return formatted data in + */ +static ssize_t reset_blocked_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct st_modedef *STm = dev_get_drvdata(dev); + struct scsi_tape *STp = STm->tape; + + return sprintf(buf, "%d", STp->pos_unknown); +} +static DEVICE_ATTR_RO(reset_blocked); + /* Support for tape stats */ /** @@ -4887,6 +4905,7 @@ static struct attribute *st_dev_attrs[] = { &dev_attr_default_density.attr, &dev_attr_default_compression.attr, &dev_attr_options.attr, + &dev_attr_reset_blocked.attr, NULL, };
If the value read from the file is 1, reads and writes from/to the device are blocked because the tape position may not match user's expectation (tape rewound after device reset). Signed-off-by: Kai Mäkisara <Kai.Makisara@kolumbus.fi> --- Documentation/scsi/st.rst | 5 +++++ drivers/scsi/st.c | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+)