| Message ID | 20190123191013.119684-2-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Fix handling of bidi commands | expand |
I disagree with investing further effort into BIDI support. It is dead for all practical purposes in standards and implementation, and the fact that we found all these bugs in it just further confirms that. The only answer to that is to bite the bullet and remove it.
On 2019-01-29 3:35 a.m., Christoph Hellwig wrote: > I disagree with investing further effort into BIDI support. It is > dead for all practical purposes in standards and implementation, > and the fact that we found all these bugs in it just further confirms > that. The only answer to that is to bite the bullet and remove it. The patchset was introduced by Bart as an expansion of my 1 line patch to get around an oops when a bidi command failed and returned a sense buffer. Boaz was able to replicate the oops in osd/exofs by injecting a SCSI error. So Bart's patchset is an expanded fix, or fix and improve. I'm guessing that bug was introduced by the blk-mq/scsi-mq work and was made more serious when the SCSI stack was forced to use blk-mq/scsi-mq exclusively. So are you arguing that an oops that you had a hand in generating *** should not fixed, and should be used as a further reason to remove bidi completely from the kernel? Bebugging? The T10 members that I have spoken to, have expressed surprise that Linux kernel developers are contemplating removing SCSI bidi support. True, XDWRITEREAD and bidi friends have been removed from draft SBC-4 but they are still used by RAID vendors and there is no move to deprecate them from SBC-3, the current standard. XCOPY(LID1) is an example of another command removed from draft SPC-5 but still much used as defined in SPC-4, the current standard. RESERVE and RELEASE are other examples. There also seems to be a consensus forming to add READ GATHERED (the bidi variant) to draft SBC-4. One member told me that their development lab maintains Linux kernels internally due to features they are experimenting with, being removed from the mainline. Perhaps they should be looking at using another open source OS. If the NVMe stack does bidi (and quad-di) using a different technique, couldn't the SCSI stack's bidi implementation (which is pretty ugly but is less ugly after this patchset) be changed to do bidi the same way as NVMe? Doug Gilbert *** 'git blame' credits you 20140117 with the memset() that causes the oops. BTW The pr_warn_once() warning people that bidi may be removed and to contact this list only seems to be in bsg code. exofs uses osd which is a SCSI ULD and osd does not use bsg to inject OSD commands. So users of exofs and the osd driver directly will not be warned and thus will be less likely to report to this list.
On Tue, 2019-01-29 at 15:03 -0500, Douglas Gilbert wrote: > One member told me that their development lab maintains > Linux kernels internally due to features they are experimenting with, > being removed from the mainline. That sounds unfortunate to me. Can you clarify which features they need and that are no longer available upstream? Thanks, Bart.
On Tue, 2019-01-29 at 09:35 +0100, Christoph Hellwig wrote: > I disagree with investing further effort into BIDI support. It is > dead for all practical purposes in standards and implementation, > and the fact that we found all these bugs in it just further confirms > that. The only answer to that is to bite the bullet and remove it. Hi Christoph, It would help if you could explain why you are so strongly opposed against keeping BIDI support in the kernel. If the reason is that removing BIDI support would allow to improve NVMe processing performance in the kernel by a small margin, that is not good enough as a reason. Users who care about that level of performance will access the NVMe queues directly from user space anyway to avoid the system call overhead. Bart.
Doug, > The T10 members that I have spoken to, have expressed surprise that > Linux kernel developers are contemplating removing SCSI bidi support. > True, XDWRITEREAD and bidi friends have been removed from draft SBC-4 > but they are still used by RAID vendors and there is no move to deprecate > them from SBC-3, the current standard. You can't deprecate something from an existing standard. > There also seems to be a consensus forming to add READ GATHERED (the > bidi variant) to draft SBC-4. That's news to me. I haven't heard anything happening on that front in T10 in a long time.
Bart, > It would help if you could explain why you are so strongly opposed > against keeping BIDI support in the kernel. My preference is to remove it because there is simply no good use case for it. The fact that this peculiar command was once defined and has since been obsoleted doesn't imply we should go out of our ways to support it. There's all sorts of other cruft defined by T10 that nobody uses and never will.
On Wed, 2019-01-30 at 22:06 -0500, Martin K. Petersen wrote: > > It would help if you could explain why you are so strongly opposed > > against keeping BIDI support in the kernel. > > My preference is to remove it because there is simply no good use case > for it. > > The fact that this peculiar command was once defined and has since been > obsoleted doesn't imply we should go out of our ways to support it. > There's all sorts of other cruft defined by T10 that nobody uses and > never will. Hi Martin, Removing bidi support will break bidi drivers. Does this mean that the osd driver should be removed before bidi support is removed from the SCSI core? Thanks, Bart.
Hi Bart, > Removing bidi support will break bidi drivers. Does this mean that the > osd driver should be removed before bidi support is removed from the > SCSI core? Yes. Christoph posted the patches a while back but I felt it was a bit too late in the cycle to merge them for 5.0.
On Tue, Jan 29, 2019 at 03:03:18PM -0500, Douglas Gilbert wrote: > So are you arguing that an oops that you had a hand in generating *** > should not fixed, and should be used as a further reason to remove bidi > completely from the kernel? Bebugging? It is not the reason, but the final nail in the coffin. As you might have noticed we've been trying to get rid of it for a while. > If the NVMe stack does bidi (and quad-di) using a different technique, > couldn't the SCSI stack's bidi implementation (which is pretty ugly but is > less ugly after this patchset) be changed to do bidi the same way as NVMe? NVMe does not do BIDI at all, and I'll use all my (little) klout in the committee to avoid having bidi support in NVMe.
On Wed, Jan 30, 2019 at 08:28:14AM -0800, Bart Van Assche wrote: > It would help if you could explain why you are so strongly opposed against > keeping BIDI support in the kernel. It creates a barely tested code path all over the SCSI and block layers, including a userspace attack vector. At the same time it sees zero real life usage - the only people touching it are less than a handful people just testing it. And yes, removing it also helps to shrink struct request, which is used by all block drivers, and removes additional code from the SCSI (not NVMe) as well as the block layer I/O path.
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c index cae6368ebb98..8c09e9e45a62 100644 --- a/drivers/scsi/iscsi_tcp.c +++ b/drivers/scsi/iscsi_tcp.c @@ -952,12 +952,6 @@ static umode_t iscsi_sw_tcp_attr_is_visible(int param_type, int param) return 0; } -static int iscsi_sw_tcp_slave_alloc(struct scsi_device *sdev) -{ - blk_queue_flag_set(QUEUE_FLAG_BIDI, sdev->request_queue); - return 0; -} - static int iscsi_sw_tcp_slave_configure(struct scsi_device *sdev) { struct iscsi_sw_tcp_host *tcp_sw_host = iscsi_host_priv(sdev->host); @@ -985,7 +979,7 @@ static struct scsi_host_template iscsi_sw_tcp_sht = { .eh_device_reset_handler= iscsi_eh_device_reset, .eh_target_reset_handler = iscsi_eh_recover_target, .dma_boundary = PAGE_SIZE - 1, - .slave_alloc = iscsi_sw_tcp_slave_alloc, + .bidi_supported = true, .slave_configure = iscsi_sw_tcp_slave_configure, .target_alloc = iscsi_target_alloc, .proc_name = "iscsi_tcp", diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index 661512bec3ac..e253c0129b40 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -3948,15 +3948,6 @@ static struct sdebug_dev_info *find_build_dev_info(struct scsi_device *sdev) return open_devip; } -static int scsi_debug_slave_alloc(struct scsi_device *sdp) -{ - if (sdebug_verbose) - pr_info("slave_alloc <%u %u %u %llu>\n", - sdp->host->host_no, sdp->channel, sdp->id, sdp->lun); - blk_queue_flag_set(QUEUE_FLAG_BIDI, sdp->request_queue); - return 0; -} - static int scsi_debug_slave_configure(struct scsi_device *sdp) { struct sdebug_dev_info *devip = @@ -5834,7 +5825,7 @@ static struct scsi_host_template sdebug_driver_template = { .proc_name = sdebug_proc_name, .name = "SCSI DEBUG", .info = scsi_debug_info, - .slave_alloc = scsi_debug_slave_alloc, + .bidi_supported = true, .slave_configure = scsi_debug_slave_configure, .slave_destroy = scsi_debug_slave_destroy, .ioctl = scsi_debug_ioctl, diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 00cd365fb7d2..94842b104bcc 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1881,6 +1881,8 @@ struct request_queue *scsi_mq_alloc_queue(struct scsi_device *sdev) sdev->request_queue->queuedata = sdev; __scsi_init_queue(sdev->host, sdev->request_queue); blk_queue_flag_set(QUEUE_FLAG_SCSI_PASSTHROUGH, sdev->request_queue); + if (sdev->host->hostt->bidi_supported) + blk_queue_flag_set(QUEUE_FLAG_BIDI, sdev->request_queue); return sdev->request_queue; } diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c index 7bd7c0c0db6f..e54a5c57a8bb 100644 --- a/drivers/target/loopback/tcm_loop.c +++ b/drivers/target/loopback/tcm_loop.c @@ -304,12 +304,6 @@ static int tcm_loop_target_reset(struct scsi_cmnd *sc) return FAILED; } -static int tcm_loop_slave_alloc(struct scsi_device *sd) -{ - blk_queue_flag_set(QUEUE_FLAG_BIDI, sd->request_queue); - return 0; -} - static struct scsi_host_template tcm_loop_driver_template = { .show_info = tcm_loop_show_info, .proc_name = "tcm_loopback", @@ -325,7 +319,7 @@ static struct scsi_host_template tcm_loop_driver_template = { .cmd_per_lun = 1024, .max_sectors = 0xFFFF, .dma_boundary = PAGE_SIZE - 1, - .slave_alloc = tcm_loop_slave_alloc, + .bidi_supported = true, .module = THIS_MODULE, .track_queue_depth = 1, }; diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index 6ca954e9f752..384b50992a56 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -430,6 +430,8 @@ struct scsi_host_template { /* True if the low-level driver supports blk-mq only */ unsigned force_blk_mq:1; + unsigned bidi_supported:1; + /* * Countdown for host blocking with no commands outstanding. */
This patch does not change any functionality but makes the drivers that support bidirectional commands more compact. Cc: Douglas Gilbert <dgilbert@interlog.com> Cc: Hannes Reinecke <hare@suse.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Lee Duncan <lduncan@suse.com> Cc: Chris Leech <cleech@redhat.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/iscsi_tcp.c | 8 +------- drivers/scsi/scsi_debug.c | 11 +---------- drivers/scsi/scsi_lib.c | 2 ++ drivers/target/loopback/tcm_loop.c | 8 +------- include/scsi/scsi_host.h | 2 ++ 5 files changed, 7 insertions(+), 24 deletions(-)