| Message ID | 20220410173652.313016-1-dgilbert@interlog.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | scsi: fix scsi_cmd::cmd_len | expand |
This still misses any good explanation of why we want all this.
On 2022-04-11 01:03, Christoph Hellwig wrote:
> This still misses any good explanation of why we want all this.
Advantages:
- undoes regression in ce70fd9a551af, that is:
- cdb_len > 32 no longer allowed (visible to the user space), undone
- but we still have this one:
- prior to lk5.18 sizeof(scsi_cmnd::cmnd) is that of a
pointer but >= lk5.18 sizeof(scsi_cmnd::cmnd) is 32 (or 16)
- makes all scsi_cmnd objects 16 bytes smaller
- hides the poorly named dtor for scsi_cmnd objects (blk_mq_free_request)
within a more intuitively named inline: scsi_free_cmnd
- scsi_free_cmnd() allows other cleanups to be hooked, like the one
proposed to free the long CDB heap, if used
- supplies three access functions for manipulating CDBs.
scsi_cmnd_set_cdb() removes the need for memset()s and cdb[n]=0 code,
and setting scsi_cmnd::cmd_len when ULDs and LLDs are building CDBs
- allows scsi_cmnd::cmnd to be renamed scsi_cmnd::__cdb in the future
to encourage the use of those access functions
- patches to code accessing scsi_cmnd::cmnd change the name of a SCSI
CDB (a byte array) to 'cdb' rather than the confusing terms: 'cmnd'
or 'cmd'
Disadvantages:
- burdens each access to a CDB with (scsi_cmnd::flags & SCMD_LONG_CDB)
check
- LLDs that want to fetch 32 byte CDBs (or longer) need to use the
scsi_cmnd_get_cdb() access function. For CDB lengths <= 16 bytes
they can continue to access scsi_cmnd::cmnd directly
On Mon, Apr 11, 2022 at 11:06:17AM -0400, Douglas Gilbert wrote: > On 2022-04-11 01:03, Christoph Hellwig wrote: >> This still misses any good explanation of why we want all this. > > Advantages: > - undoes regression in ce70fd9a551af, that is: > - cdb_len > 32 no longer allowed (visible to the user space), undone What exact regression causes this for real users and no just people playing around with scsi_debug? > - but we still have this one: > - prior to lk5.18 sizeof(scsi_cmnd::cmnd) is that of a > pointer but >= lk5.18 sizeof(scsi_cmnd::cmnd) is 32 (or 16) Please check the total size of struct scsi_cmnd, which is what really matters. > - makes all scsi_cmnd objects 16 bytes smaller Do we have data why this matters? > - hides the poorly named dtor for scsi_cmnd objects (blk_mq_free_request) > within a more intuitively named inline: scsi_free_cmnd I don't think this is in any way poorly named. > Disadvantages: > - burdens each access to a CDB with (scsi_cmnd::flags & SCMD_LONG_CDB) > check > - LLDs that want to fetch 32 byte CDBs (or longer) need to use the > scsi_cmnd_get_cdb() access function. For CDB lengths <= 16 bytes > they can continue to access scsi_cmnd::cmnd directly It adds back the dynamic allocation for 32-byte CDBs that we got rid of. It also breaks all LLDS actually using 32-byte CDBS currently as far as I can tell.
On 2022-04-11 11:52, Christoph Hellwig wrote: > On Mon, Apr 11, 2022 at 11:06:17AM -0400, Douglas Gilbert wrote: >> On 2022-04-11 01:03, Christoph Hellwig wrote: >>> This still misses any good explanation of why we want all this. >> >> Advantages: >> - undoes regression in ce70fd9a551af, that is: >> - cdb_len > 32 no longer allowed (visible to the user space), undone > > What exact regression causes this for real users and no just people > playing around with scsi_debug? Sorry, you are regressing something that has been in place for over 20 years and required by SPC (1 through 5) standards. The onus should not be on me to prove that regression is not safe. It should be the other way around (i.e. for you to prove that it is safe). I admit that working with scsi_debug can be fun, but it seems to me a few other people have found it a useful tool. Some football advice might apply here: play the ball, not the man. >> - but we still have this one: >> - prior to lk5.18 sizeof(scsi_cmnd::cmnd) is that of a >> pointer but >= lk5.18 sizeof(scsi_cmnd::cmnd) is 32 (or 16) > > Please check the total size of struct scsi_cmnd, which is what really > matters. From my laptop (64 bit) where scsi_cmnd1 is the original struct scsi_cmnd: xtwo70 kernel: sizeof(struct scsi_cmnd)=376, sizeof(struct scsi_cmnd1)=392 So is slightly > 4% (higher on 32 bit machines) insignificant? Since I have that measurement code in place, try a few other things .... Changing scsi_cmnd::flags to be a u8 and putting sense_buffer and host_scribble next to one another (they are pointers) gives: xtwo70 kernel: sizeof(struct scsi_cmnd)=360, sizeof(struct scsi_cmnd1)=392 Now we are at a 8% reduction. > >> - makes all scsi_cmnd objects 16 bytes smaller > > Do we have data why this matters? In commit ce70fd9a551af7424a7dace2a1ba05a7de8eae27 you wrote: "Now that each scsi_request is backed by a scsi_cmnd, there is no need to indirect the CDB storage. Change all submitters of SCSI passthrough requests to store the CDB information directly in the scsi_cmnd, and while doing so allocate the full 32 bytes that cover all Linux supported SCSI hosts instead of requiring dynamic allocation for > 16 byte CDBs. On 64-bit systems this does not change the size of the scsi_cmnd at all, while on 32-bit systems it slightly increases it for now, but that increase will be made up by the removal of the remaining scsi_request fields." $ cd drivers/scsi $ find . -name '*.c' -exec grep "SCSI_MAX_VARLEN_CDB_SIZE" {} \; -print shost->max_cmd_len = SCSI_MAX_VARLEN_CDB_SIZE; ./iscsi_tcp.c shost->max_cmd_len = SCSI_MAX_VARLEN_CDB_SIZE; ./cxgbi/libcxgbi.c include/scsi/scsi_proto.h:#define SCSI_MAX_VARLEN_CDB_SIZE 260 Two examples that make your "the full 32 bytes that cover all ..." assertion false. Also those quoted comments seem to give weight to the argument that writer believes that the size of scsi_cmnd matters. If so, I agree, smaller is better. >> - hides the poorly named dtor for scsi_cmnd objects (blk_mq_free_request) >> within a more intuitively named inline: scsi_free_cmnd > > I don't think this is in any way poorly named. Seriously? As well, having a scsi_cmnd destructor opens up the possibility of deferring kmem_cache_free(scsi_sense_cache, cmd->sense_buffer) from scsi_mq_exit_request() to that destructor. Then if scsi_cmnd objects are re-used, scsi_mq_init_request() need only get a cmd->sense_buffer if one has not yet been allocated. Again, I present no data, but pretty obviously a performance win. Another advantage of that patchset: - in scsi_initialize_rq() the patch initializes CBD to Test Unit Ready (6 zeros), previously it did a memset(scmd->cmnd, 0, 32), so that is another small win. That initialization could be further optimized with: scmd->l_cdb.dummy_tur = 0; /* clears first 8 zeros */ scmd->cmd_len = SCSI_TEST_UNIT_READY_CDB_LEN; to bypass memset() completely. >> Disadvantages: >> - burdens each access to a CDB with (scsi_cmnd::flags & SCMD_LONG_CDB) >> check >> - LLDs that want to fetch 32 byte CDBs (or longer) need to use the >> scsi_cmnd_get_cdb() access function. For CDB lengths <= 16 bytes >> they can continue to access scsi_cmnd::cmnd directly > > It adds back the dynamic allocation for 32-byte CDBs that we got rid of. > It also breaks all LLDS actually using 32-byte CDBS currently as far as > I can tell. As Bart pointed out, the dynamic allocation for 32-byte CDBs is relatively rare, more than made up for by the 4% reduction in struct scsi_cmnd's size. As for the second sentence, if this patchset is accepted, I will find and fix those. The ones I did check limited cdb_s to 16 bytes. Doug Gilbert
Douglas, >> What exact regression causes this for real users and no just people >> playing around with scsi_debug? > > Sorry, you are regressing something that has been in place for over 20 > years and required by SPC (1 through 5) standards. The onus should > not be on me to prove that regression is not safe. It should be the > other way around (i.e. for you to prove that it is safe). Christoph's question is valid: Ignoring obsolete command sets that we no longer support, what is the real world use case for variable length CDBs larger than 32 bytes? Which devices require them, and do we want to support those devices? That fact that a SCSI spec permits something is not the same as saying we must support it. "Required by SPC" is news to me.
As described in the first patch of this set, commit ce70fd9a551af reduced the maximum size of any SCSI commands (CDB length) to 32 bytes. It was previously larger in struct scsi_request which has now (lk 5.18.0-rc1) been removed. Use a slightly different scheme than before to support CDB lengths greater than 16 bytes. Three access function are added, one for read access, one for writable access that doesn't increase the cdb length, and the third for create/write access to the CDB held inside a scsi_cmnd object. This reduces the size of struct scsi_cmnd by 16 bytes. A scsi_cmnd object is always paired with a struct request object that immediately precedes it. To note this pairing the new comments refer to a scsi_cmnd "sub-object". Prior to this patch the constructor/destructor naming was confusing: - scsi_alloc_request() to create a pair - blk_mq_free_request() to destruct a pair Add a new destructor function: scsi_free_cmnd(struct scsi_cmnd *scmd) to make this a bit clearer. Also scsi_free_cmnd() will free up a pointer to a long cdb buffer on the heap, if one is present. These changes have been applied to SCSI mid-level and the upper level drivers (ULDs, e.g. sd). Only one low level driver (LLD) has been updated: scsi_debug. The rest of the LLDs can continue to use scsi_cmnd::cmnd directly _unless_ they wish to support CDB lengths > 16 bytes; in that case they should use scsi_cmnd_get_cdb(). This patchset is against lk 5.18.0-rc1 and also applies cleanly to MKP's 5.19/scsi-queue branch. Changes since the first revision: - introduce a new SCSI_MAX_RUN_TIME_8BIT_CDB_LEN define for the case where hp->cmd_len is an 8 bit quantity. This addresses a kernel test robot report on patch 1 sent 20220408 - take up Bart's suggestion to reduce SCSI_MAX_COMPILE_TIME_CDB_LEN to 16, hence shaving 16 bytes off the size of struct scsi_cmnd - introduce scsi_cmnd_get_changeable_cdb() access function for the case when the code wants to change some bytes in an existing cdb but not increase its size - add a broad description of how struct scsi_cmnd fits into overall architecture, tweak some other comments - further work on the sr and stex ULDs Douglas Gilbert (6): scsi_cmnd: reinstate support for cmd_len > 32 sd, sd_zbc: use scsi_cmnd cdb access functions sg: reinstate cmd_len > 32 bsg: allow cmd_len > 32 scsi_debug: reinstate cmd_len > 32 st,sr,stex: use scsi_cmnd cdb access functions drivers/scsi/scsi_bsg.c | 22 +- drivers/scsi/scsi_debug.c | 410 +++++++++++++++++++----------------- drivers/scsi/scsi_debugfs.c | 3 +- drivers/scsi/scsi_error.c | 76 ++++--- drivers/scsi/scsi_ioctl.c | 21 +- drivers/scsi/scsi_lib.c | 75 ++++++- drivers/scsi/scsi_logging.c | 11 +- drivers/scsi/sd.c | 176 +++++++++------- drivers/scsi/sd_zbc.c | 12 +- drivers/scsi/sg.c | 23 +- drivers/scsi/sr.c | 40 ++-- drivers/scsi/st.c | 12 +- drivers/scsi/stex.c | 22 +- include/scsi/scsi_cmnd.h | 116 ++++++++-- include/scsi/scsi_eh.h | 6 +- 15 files changed, 623 insertions(+), 402 deletions(-)