Re: [PATCH v3 0/3] Report all request failures again to user space
On Wed, 2018-04-11 at 17:45 -0400, Douglas Gilbert wrote: > It is a good bet that some sg based apps in the wild will be using the > old (shifted+masked) SCSI status defines. The basis of the sg_v3 interface > is 'struct sg_io_hdr' and it has two SCSI status fields: 'status' and > 'masked_status'. The latter one matches the old SCSI status defines > that you want to get rid of. I think that it is fine to get rid of > them in the kernel, but not in the user space. > > A suggestion: you create a new header called scsi_legacy.h in the > uapi/linux directory that is only included at the start of scsi.h > when __KERNEL__ is not defined. And in scsi_legacy.h prefix those old > defines with a comment like: ... please use don't use the following > defines, use the ones that start with SAM_STAT_ found in instead. Hello Doug, A change I made in the include/scsi/scsi.h header file in 2015 has not yet made it into the latest glibc /usr/include/scsi/scsi.h header file. So I think we don't have to worry about backwards compatibility in the header files under include/scsi. The glibc maintainers however might appreciate it if we would introduce files under include/uapi/scsi that they can copy instead of having to maintain the glibc SCSI header files themselves. Bart.
Re: [PATCH v3 0/3] Report all request failures again to user space
On 2018-04-11 12:25 PM, Johannes Thumshirn wrote: On Wed, Apr 11, 2018 at 12:20:49PM -0400, Martin K. Petersen wrote: Johannes, While we're at it, anyone still in love with the non SAM_ status bytes? If not they'll be gone afterwards as well. /me is up for some spring cleaning. Just be careful not to break any sg/bsg apps that rely on them. Good point. In _theory_ all should be fine if I'm not removing the actual defines if they're exported. Will be checking. It is a good bet that some sg based apps in the wild will be using the old (shifted+masked) SCSI status defines. The basis of the sg_v3 interface is 'struct sg_io_hdr' and it has two SCSI status fields: 'status' and 'masked_status'. The latter one matches the old SCSI status defines that you want to get rid of. I think that it is fine to get rid of them in the kernel, but not in the user space. A suggestion: you create a new header called scsi_legacy.h in the uapi/linux directory that is only included at the start of scsi.h when __KERNEL__ is not defined. And in scsi_legacy.h prefix those old defines with a comment like: ... please use don't use the following defines, use the ones that start with SAM_STAT_ found in instead. Doug Gilbert
Re: [PATCH v3 0/3] Report all request failures again to user space
On Wed, Apr 11, 2018 at 12:20:49PM -0400, Martin K. Petersen wrote: > > Johannes, > > > While we're at it, anyone still in love with the non SAM_ status > > bytes? If not they'll be gone afterwards as well. > > > > /me is up for some spring cleaning. > > Just be careful not to break any sg/bsg apps that rely on them. Good point. In _theory_ all should be fine if I'm not removing the actual defines if they're exported. Will be checking. -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH v3 0/3] Report all request failures again to user space
Johannes, > While we're at it, anyone still in love with the non SAM_ status > bytes? If not they'll be gone afterwards as well. > > /me is up for some spring cleaning. Just be careful not to break any sg/bsg apps that rely on them. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v3 0/3] Report all request failures again to user space
On Wed, Apr 11, 2018 at 12:05:18PM -0400, Martin K. Petersen wrote: > > Johannes, > > > Well it's defined in "drivers/scsi/scsi_typedefs.h" and only used by > > some (old) drivers inside drivers/scsi so it can't be a UAPI => we're > > not breaking any existing user-space with it. > > I'm all for nuking it. :-) While we're at it, anyone still in love with the non SAM_ status bytes? If not they'll be gone afterwards as well. /me is up for some spring cleaning. -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH v3 0/3] Report all request failures again to user space
Johannes, > Well it's defined in "drivers/scsi/scsi_typedefs.h" and only used by > some (old) drivers inside drivers/scsi so it can't be a UAPI => we're > not breaking any existing user-space with it. I'm all for nuking it. > Anyways that's still WIP but I plan to have it done before LSF/MM so > we can have a hallway track if needed. Sounds good! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v3 0/3] Report all request failures again to user space
On Tue, Apr 10, 2018 at 02:53:40PM +, Bart Van Assche wrote: > I'm in favor of the removal. But maybe there is something I'm overlooking > and that means that the typedef should not be removed? Well it's defined in "drivers/scsi/scsi_typedefs.h" and only used by some (old) drivers inside drivers/scsi so it can't be a UAPI => we're not breaking any existing user-space with it. I already did the conversion locally in my revamped scsi result handling series branch and if someone steps up I can easily revert it again but I don't see a compelling reason to do so. Anyways that's still WIP but I plan to have it done before LSF/MM so we can have a hallway track if needed. Byte, Johannes -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH v3 0/3] Report all request failures again to user space
On Tue, 2018-04-10 at 15:48 +0200, Johannes Thumshirn wrote: > On Mon, Apr 09, 2018 at 09:35:21PM -0400, Martin K. Petersen wrote: > > > > Johannes, > > > > > I did start a series [1] for this but than got distracted by more urgent > > > things. I can pick it up again I think. > > > > > > [1] > > > https://git.kernel.org/pub/scm/linux/kernel/git/jth/linux.git/log/?h=iscsi_DID_REQUEUE > > > > Definitely a step in the right direction. > > Does anyone object getting rid of the Scsi_Cmnd typedef while I'm at > it? > > This would ease my refactoring a lot, asI don't have to teach > coccinelle yet another case that multiplies the rules. I'm in favor of the removal. But maybe there is something I'm overlooking and that means that the typedef should not be removed? Bart.
Re: [PATCH v3 0/3] Report all request failures again to user space
Hi,
[This is an automated email]
This commit has been processed by the -stable helper bot and determined
to be a high probability candidate for -stable trees. (score: 19.8603)
The bot has tested the following trees: v4.16.1, v4.15.16, v4.14.33, v4.9.93,
v4.4.127.
v4.16.1: Failed to apply! Possible dependencies:
Unable to calculate
v4.15.16: Build OK!
v4.14.33: Build OK!
v4.9.93: Failed to apply! Possible dependencies:
2a842acab109 ("block: introduce new block status code type")
b6800ec54c74 ("Make scsi_result_to_blk_status() recognize CONDITION MET")
v4.4.127: Failed to apply! Possible dependencies:
2a842acab109 ("block: introduce new block status code type")
b6800ec54c74 ("Make scsi_result_to_blk_status() recognize CONDITION MET")
Please let us know if you'd like to have this patch included in a stable tree.
--
Thanks,
Sasha
Re: [PATCH v3 0/3] Report all request failures again to user space
On Mon, Apr 09, 2018 at 09:35:21PM -0400, Martin K. Petersen wrote: > > Johannes, > > > I did start a series [1] for this but than got distracted by more urgent > > things. I can pick it up again I think. > > > > [1] > > https://git.kernel.org/pub/scm/linux/kernel/git/jth/linux.git/log/?h=iscsi_DID_REQUEUE > > Definitely a step in the right direction. Does anyone object getting rid of the Scsi_Cmnd typedef while I'm at it? This would ease my refactoring a lot, asI don't have to teach coccinelle yet another case that multiplies the rules. Thanks, Johannes -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH v3 0/3] Report all request failures again to user space
Johannes, > I did start a series [1] for this but than got distracted by more urgent > things. I can pick it up again I think. > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/jth/linux.git/log/?h=iscsi_DID_REQUEUE Definitely a step in the right direction. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v3 0/3] Report all request failures again to user space
Bart, > A recent change in the SCSI core caused certain request failures no > longer to be reported to user space. Damien noticed this by sending a > write request that is not aligned to the write pointer to an SMR drive > from user space. Such non-aligned write requests are failed by the > drive and such failures should be reported to user space. This patch > series makes sure that all SCSI request failures are again reported to > user space. This patch series also makes the SCSI core recognize > status codes like CONDITION MET as not being an error. Please > consider at least the first patch in this series for the rc stage of > kernel v4.17. Applied to 4.17/scsi-fixes. Thank you! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v3 0/3] Report all request failures again to user space
On Fri, Apr 06, 2018 at 02:59:16PM +, Bart Van Assche wrote:
> On Fri, 2018-04-06 at 09:45 +0200, Johannes Thumshirn wrote:
> > On 05/04/18 19:49, Martin K. Petersen wrote:
> > > Longer term I'd really like to see the command result integer
> > > host/driver/msg/status stuff cleaned up. It's super arcane and the
> > > associated naming schemes make it a very error-prone interface.
> > >
> >
> > I did start a series [1] for this but than got distracted by more urgent
> > things. I can pick it up again I think.
> >
> > [1]
> > https://git.kernel.org/pub/scm/linux/kernel/git/jth/linux.git/log/?h=iscsi_DID_REQUEUE
>
> Hello Johannes,
>
> Since modifying how SCSI results are communicated from SCSI LLDs to the SCSI
> core requires to touch all SCSI LLDs, please consider to include the following
> changes in your patch series:
> - Remove the deprecated SCSI status codes (GOOD, CHECK_CONDITION, etc.) and
> use the SAM_STAT_* symbolic names instead.
I'll have a look, thanks for the heads up.
> - Ensure that assigning an int to scsi_cmnd.result triggers a compiler error.
> There is code in the Linux kernel that mixes traditional error codes (-EIO)
> with the SCSI result codes (DID_ERROR << 16). I think most of that code is
> buggy and that it should be fixed. Changing the type of scsi_cmnd.result is
> one way to find such code. How about something like the following (untested)
> patch?
I'll give it a shot.
>
> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
> index cb85eddb47ea..27f1c347f0ea 100644
> --- a/include/scsi/scsi.h
> +++ b/include/scsi/scsi.h
> @@ -18,6 +18,27 @@ enum scsi_timeouts {
> SCSI_DEFAULT_EH_TIMEOUT = 10 * HZ,
> };
>
> +/*
> + * Note: .result is signed because SCSI LLDs store a SCSI result in
> + * .result while the bsg driver stores a negative error code in .result.
> + */
> +typedef union {
> + s32 result;
> + struct {
> +#if defined(__BIG_ENDIAN)
> + u8 driver_byte;
> + u8 host_byte;
> + u8 msg_byte;
> + u8 status_byte;
> +#elif defined(__LITTLE_ENDIAN)
> + u8 status_byte;
> + u8 msg_byte;
> + u8 host_byte;
> + u8 driver_byte;
> +#endif
> + };
> +} scsi_result;
> +
> /*
> * DIX-capable adapters effectively support infinite chaining for the
> * protection information scatterlist
> @@ -38,8 +59,10 @@ enum scsi_timeouts {
> * This returns true for known good conditions that may be treated as
> * command completed normally
> */
> -static inline int scsi_status_is_good(int status)
> +static inline int scsi_status_is_good(scsi_result result)
> {
> + u8 status = result.status_byte;
> +
> /*
>* FIXME: bit0 is listed as reserved in SCSI-2, but is
>* significant in SCSI-3. For now, we follow the SCSI-2
> @@ -205,10 +228,10 @@ static inline int scsi_is_wlun(u64 lun)
> * host_byte = set by low-level driver to indicate status.
> * driver_byte = set by mid-level.
> */
> -#define status_byte(result) (((result) >> 1) & 0x7f)
> -#define msg_byte(result)(((result) >> 8) & 0xff)
> -#define host_byte(result) (((result) >> 16) & 0xff)
> -#define driver_byte(result) (((result) >> 24) & 0xff)
> +#define status_byte(result) ((result).status_byte >> 1)
> +#define msg_byte(result)((result).msg_byte)
> +#define host_byte(result) ((result).host_byte)
> +#define driver_byte(result) ((result).driver_byte)
>
> #define sense_class(sense) (((sense) >> 4) & 0x7)
> #define sense_error(sense) ((sense) & 0xf)
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 2280b2351739..cbc5df948dd3 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -144,7 +144,7 @@ struct scsi_cmnd {
>* obtained by scsi_malloc is guaranteed
>* to be at an address < 16Mb). */
>
> - int result; /* Status code from lower level driver */
> + scsi_result result; /* Status code from lower level driver */
> int flags; /* Command flags */
>
> unsigned char tag; /* SCSI-II queued command tag */
>
> Bart.
--
Johannes Thumshirn Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH v3 0/3] Report all request failures again to user space
On Fri, 2018-04-06 at 09:45 +0200, Johannes Thumshirn wrote:
> On 05/04/18 19:49, Martin K. Petersen wrote:
> > Longer term I'd really like to see the command result integer
> > host/driver/msg/status stuff cleaned up. It's super arcane and the
> > associated naming schemes make it a very error-prone interface.
> >
>
> I did start a series [1] for this but than got distracted by more urgent
> things. I can pick it up again I think.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/jth/linux.git/log/?h=iscsi_DID_REQUEUE
Hello Johannes,
Since modifying how SCSI results are communicated from SCSI LLDs to the SCSI
core requires to touch all SCSI LLDs, please consider to include the following
changes in your patch series:
- Remove the deprecated SCSI status codes (GOOD, CHECK_CONDITION, etc.) and
use the SAM_STAT_* symbolic names instead.
- Ensure that assigning an int to scsi_cmnd.result triggers a compiler error.
There is code in the Linux kernel that mixes traditional error codes (-EIO)
with the SCSI result codes (DID_ERROR << 16). I think most of that code is
buggy and that it should be fixed. Changing the type of scsi_cmnd.result is
one way to find such code. How about something like the following (untested)
patch?
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index cb85eddb47ea..27f1c347f0ea 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -18,6 +18,27 @@ enum scsi_timeouts {
SCSI_DEFAULT_EH_TIMEOUT = 10 * HZ,
};
+/*
+ * Note: .result is signed because SCSI LLDs store a SCSI result in
+ * .result while the bsg driver stores a negative error code in .result.
+ */
+typedef union {
+ s32 result;
+ struct {
+#if defined(__BIG_ENDIAN)
+ u8 driver_byte;
+ u8 host_byte;
+ u8 msg_byte;
+ u8 status_byte;
+#elif defined(__LITTLE_ENDIAN)
+ u8 status_byte;
+ u8 msg_byte;
+ u8 host_byte;
+ u8 driver_byte;
+#endif
+ };
+} scsi_result;
+
/*
* DIX-capable adapters effectively support infinite chaining for the
* protection information scatterlist
@@ -38,8 +59,10 @@ enum scsi_timeouts {
* This returns true for known good conditions that may be treated as
* command completed normally
*/
-static inline int scsi_status_is_good(int status)
+static inline int scsi_status_is_good(scsi_result result)
{
+ u8 status = result.status_byte;
+
/*
* FIXME: bit0 is listed as reserved in SCSI-2, but is
* significant in SCSI-3. For now, we follow the SCSI-2
@@ -205,10 +228,10 @@ static inline int scsi_is_wlun(u64 lun)
* host_byte = set by low-level driver to indicate status.
* driver_byte = set by mid-level.
*/
-#define status_byte(result) (((result) >> 1) & 0x7f)
-#define msg_byte(result)(((result) >> 8) & 0xff)
-#define host_byte(result) (((result) >> 16) & 0xff)
-#define driver_byte(result) (((result) >> 24) & 0xff)
+#define status_byte(result) ((result).status_byte >> 1)
+#define msg_byte(result)((result).msg_byte)
+#define host_byte(result) ((result).host_byte)
+#define driver_byte(result) ((result).driver_byte)
#define sense_class(sense) (((sense) >> 4) & 0x7)
#define sense_error(sense) ((sense) & 0xf)
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 2280b2351739..cbc5df948dd3 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -144,7 +144,7 @@ struct scsi_cmnd {
* obtained by scsi_malloc is guaranteed
* to be at an address < 16Mb). */
- int result; /* Status code from lower level driver */
+ scsi_result result; /* Status code from lower level driver */
int flags; /* Command flags */
unsigned char tag; /* SCSI-II queued command tag */
Bart.
Re: [PATCH v3 0/3] Report all request failures again to user space
On 05/04/18 19:49, Martin K. Petersen wrote: > > Bart, > >> A recent change in the SCSI core caused certain request failures no >> longer to be reported to user space. Damien noticed this by sending a >> write request that is not aligned to the write pointer to an SMR drive >> from user space. Such non-aligned write requests are failed by the >> drive and such failures should be reported to user space. This patch >> series makes sure that all SCSI request failures are again reported to >> user space. This patch series also makes the SCSI core recognize >> status codes like CONDITION MET as not being an error. Please >> consider at least the first patch in this series for the rc stage of >> kernel v4.17. > > Looks good to me. > > Longer term I'd really like to see the command result integer > host/driver/msg/status stuff cleaned up. It's super arcane and the > associated naming schemes make it a very error-prone interface. > I did start a series [1] for this but than got distracted by more urgent things. I can pick it up again I think. [1] https://git.kernel.org/pub/scm/linux/kernel/git/jth/linux.git/log/?h=iscsi_DID_REQUEUE Johannes -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH v3 0/3] Report all request failures again to user space
Bart, > A recent change in the SCSI core caused certain request failures no > longer to be reported to user space. Damien noticed this by sending a > write request that is not aligned to the write pointer to an SMR drive > from user space. Such non-aligned write requests are failed by the > drive and such failures should be reported to user space. This patch > series makes sure that all SCSI request failures are again reported to > user space. This patch series also makes the SCSI core recognize > status codes like CONDITION MET as not being an error. Please > consider at least the first patch in this series for the rc stage of > kernel v4.17. Looks good to me. Longer term I'd really like to see the command result integer host/driver/msg/status stuff cleaned up. It's super arcane and the associated naming schemes make it a very error-prone interface. -- Martin K. Petersen Oracle Linux Engineering
