Re: [PATCH 1/3] scsi: myrs: Fix a logical vs bitwise bug
Dan, > The || was supposed to be |. The original code just sets ->result to 1. Applied to 4.20/scsi-queue, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 2/3] scsi: myrs: Fix the processor absent message in processor_show()
Dan, > If both processors are absent then it's supposed to print that, but > instead we print that just the second processor is absent. Applied to 4.20/scsi-queue. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 3/3] scsi: myrs: prevent negatives in disable_enclosure_messages_store()
Dan, > We only want the value to be zero or one. > > It's not a big deal, but say we passed set value to INT_MIN, then > disable_enclosure_messages_show() would return that 12 bytes of "buf" > are initialized but actually only 3 are. I think there are tools like > KASAN which will trigger an info leak warning when that happens. s/kstrtoint/kstrtouint/? -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH RESEND] scsi: qla2xxx: I/Os timing out on surprise removal of
Bill, > When doing a surprise removal of an adapter, some in flight I/Os can > get stuck and take a while to complete (they actually timeout and are > retried). We are not handling an early error exit from > qla2xxx_eh_abort properly. This doesn't apply to 4.20/scsi-queue and the surrounding code has changed significantly. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: myrs: fix build failure on 32 bit
James, > For 32 bit versions we have to be careful about divisions of 64 bit > quantities so use do_div() instead of a direct division. This fixes a > warning about _uldivmod being undefined in certain configurations Applied to 4.20/scsi-queue. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 5/8] sg: add free list, rework locking
Hi linux-scsi-owner,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on v4.19-rc8 next-20181019]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/linux-scsi-owner-vger-kernel-org/sg-major-cleanup-remove-max_queue-limit/20181019-183809
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: i386-randconfig-i1-201841 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/scsi/sg.o: In function `sg_rq_end_io_usercontext':
>> drivers/scsi/sg.c:1494: undefined reference to `sg_rq_state_str'
>> drivers/scsi/sg.c:1494: undefined reference to `sg_rq_state_str'
vim +1494 drivers/scsi/sg.c
1470
1471 /*
1472 * This user context function is needed to clean up a request that has
been
1473 * interrupted (e.g. by control-C at keyboard). That leads to a request
1474 * being an 'orphan' and will be cleared here unless the 'keep_orphan'
flag
1475 * has been set on the owning file descriptor. In that case the user is
1476 * expected to call read() or ioctl(SG_IORECEIVE) to receive the
response
1477 * and free resources held by the interrupted request.
1478 */
1479 static void
1480 sg_rq_end_io_usercontext(struct work_struct *work)
1481 {
1482 struct sg_request *srp = container_of(work, struct sg_request,
ew.work);
1483 struct sg_fd *sfp;
1484
1485 if (!srp) {
1486 WARN_ONCE("s: srp unexpectedly NULL\n", __func__);
1487 return;
1488 }
1489 sfp = srp->parentfp;
1490 if (!sfp) {
1491 WARN_ONCE(1, "%s: sfp unexpectedly NULL\n", __func__);
1492 return;
1493 }
> 1494 SG_LOG(3, sfp->parentdp, "%s: clean srp=0x%p, rq_state: %s\n",
1495 __func__, srp, sg_rq_state_str(srp->rq_state, true));
1496 sg_finish_scsi_blk_rq(srp);
1497 sg_remove_request(sfp, srp);
1498 kref_put(>f_ref, sg_remove_sfp);
1499 }
1500
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH 5/8] sg: add free list, rework locking
On Fri, 2018-10-19 at 15:55 -0400, Douglas Gilbert wrote:
> On 2018-10-19 11:22 a.m., Bart Van Assche wrote:
> > On Fri, 2018-10-19 at 02:24 -0400, Douglas Gilbert wrote:
> > > static void
> > > -sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo)
> > > +sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo,
> > > + int max_num)
> > > {
> > > struct sg_request *srp;
> > > int val;
> > > - unsigned int ms;
> > >
> > > val = 0;
> > > - list_for_each_entry(srp, >rq_list, entry) {
> > > - if (val >= SG_MAX_QUEUE)
> > > - break;
> > > - rinfo[val].req_state = srp->done + 1;
> > > + list_for_each_entry(srp, >rq_list, rq_entry) {
> > > + if (val >= max_num)
> > > + return;
> >
> > What protects the sfp->rq_list against concurrent changes? It seems to me
> > like all other code that iterates over or modifies that list protects that
> > list with rq_list_lock?
>
> Bart,
> The function is called from sg_ioctl() case SG_GET_REQUEST_TABLE and at the
> call point the read_lock is held on rq_list_lock. Maybe I can add a comment
> above the function about the lock being held. [At least it is as the end
> of the patch series, and that is all I care about :-)]
Hi Doug,
Thanks for the clarification. How about adding a __must_hold() annotation?
Bart.
[bug report] scsi: myrb: Add Mylex RAID controller (block interface)
Hello Hannes Reinecke,
The patch 081ff398c56c: "scsi: myrb: Add Mylex RAID controller (block
interface)" from Oct 17, 2018, leads to the following static checker
warning:
drivers/scsi/myrb.c:1614 myrb_ldev_queuecommand()
warn: assigning signed to unsigned: 'mbox->type5.sg_count = nsge'
'(-12),0,2-s32max'
drivers/scsi/myrb.c
1579 if (scmd->sc_data_direction == DMA_NONE)
1580 goto submit;
1581 nsge = scsi_dma_map(scmd);
^
nsge can be -ENOMEM;
1582 if (nsge == 1) {
1583 sgl = scsi_sglist(scmd);
1584 if (scmd->sc_data_direction == DMA_FROM_DEVICE)
1585 mbox->type5.opcode = MYRB_CMD_READ;
1586 else
1587 mbox->type5.opcode = MYRB_CMD_WRITE;
1588
1589 mbox->type5.ld.xfer_len = block_cnt;
1590 mbox->type5.ld.ldev_num = sdev->id;
1591 mbox->type5.lba = lba;
1592 mbox->type5.addr = (u32)sg_dma_address(sgl);
1593 } else {
1594 struct myrb_sge *hw_sgl;
1595 dma_addr_t hw_sgl_addr;
1596 int i;
1597
1598 hw_sgl = dma_pool_alloc(cb->sg_pool, GFP_ATOMIC,
_sgl_addr);
1599 if (!hw_sgl)
1600 return SCSI_MLQUEUE_HOST_BUSY;
1601
1602 cmd_blk->sgl = hw_sgl;
1603 cmd_blk->sgl_addr = hw_sgl_addr;
1604
1605 if (scmd->sc_data_direction == DMA_FROM_DEVICE)
1606 mbox->type5.opcode = MYRB_CMD_READ_SG;
1607 else
1608 mbox->type5.opcode = MYRB_CMD_WRITE_SG;
1609
1610 mbox->type5.ld.xfer_len = block_cnt;
1611 mbox->type5.ld.ldev_num = sdev->id;
1612 mbox->type5.lba = lba;
1613 mbox->type5.addr = hw_sgl_addr;
1614 mbox->type5.sg_count = nsge;
1615
1616 scsi_for_each_sg(scmd, sgl, nsge, i) {
1617 hw_sgl->sge_addr = (u32)sg_dma_address(sgl);
1618 hw_sgl->sge_count = (u32)sg_dma_len(sgl);
1619 hw_sgl++;
1620 }
1621 }
1622 submit:
1623 spin_lock_irqsave(>queue_lock, flags);
regards,
dan carpenter
Re: dma related cleanups for wd719x
On Thursday 18 October 2018 15:01:14 Christoph Hellwig wrote:
> Hi Ondrej,
>
> can you look over this series, which cleans up a few dma-related
> bits in the wd719x driver?
>
This makes it work a bit (can detect drive and read partitions) but it hangs
completely after hdparm -t.
diff --git a/drivers/scsi/wd719x.c b/drivers/scsi/wd719x.c
index d47190f08ed6..55a7a542e653 100644
--- a/drivers/scsi/wd719x.c
+++ b/drivers/scsi/wd719x.c
@@ -183,7 +183,7 @@ static void wd719x_finish_cmd(struct wd719x_scb *scb, int
result)
list_del(>list);
dma_unmap_single(>pdev->dev, scb->phys,
- sizeof(struct wd719x_scb), DMA_TO_DEVICE);
+ sizeof(struct wd719x_scb), DMA_BIDIRECTIONAL);
scsi_dma_unmap(cmd);
dma_unmap_single(>pdev->dev, cmd->SCp.dma_handle,
SCSI_SENSE_BUFFERSIZE, DMA_FROM_DEVICE);
@@ -236,6 +236,12 @@ static int wd719x_queuecommand(struct Scsi_Host *sh,
struct scsi_cmnd *cmd)
if (count_sg) {
struct scatterlist *sg;
+ scb->phys = dma_map_single(>pdev->dev, scb, sizeof(*scb),
+ DMA_BIDIRECTIONAL);
+
+ if (dma_mapping_error(>pdev->dev, scb->phys))
+ goto out_unmap_scsi_cmd;
+
scb->data_length = cpu_to_le32(count_sg *
sizeof(struct wd719x_sglist));
scb->data_p = cpu_to_le32(scb->phys +
@@ -246,11 +252,6 @@ static int wd719x_queuecommand(struct Scsi_Host *sh,
struct scsi_cmnd *cmd)
scb->sg_list[i].length = cpu_to_le32(sg_dma_len(sg));
}
scb->SCB_options |= WD719X_SCB_FLAGS_DO_SCATTER_GATHER;
-
- scb->phys = dma_map_single(>pdev->dev, scb, sizeof(*scb),
- DMA_TO_DEVICE);
- if (dma_mapping_error(>pdev->dev, scb->phys))
- goto out_unmap_scsi_cmd;
} else { /* zero length */
scb->data_length = 0;
scb->data_p = 0;
--
Ondrej Zary
Re: [PATCH 5/8] sg: add free list, rework locking
On 2018-10-19 11:22 a.m., Bart Van Assche wrote:
On Fri, 2018-10-19 at 02:24 -0400, Douglas Gilbert wrote:
static void
-sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo)
+sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo,
+ int max_num)
{
struct sg_request *srp;
int val;
- unsigned int ms;
val = 0;
- list_for_each_entry(srp, >rq_list, entry) {
- if (val >= SG_MAX_QUEUE)
- break;
- rinfo[val].req_state = srp->done + 1;
+ list_for_each_entry(srp, >rq_list, rq_entry) {
+ if (val >= max_num)
+ return;
What protects the sfp->rq_list against concurrent changes? It seems to me
like all other code that iterates over or modifies that list protects that
list with rq_list_lock?
Bart,
The function is called from sg_ioctl() case SG_GET_REQUEST_TABLE and at the
call point the read_lock is held on rq_list_lock. Maybe I can add a comment
above the function about the lock being held. [At least it is as the end
of the patch series, and that is all I care about :-)]
Doug Gilbert
P.S. Best to look at sg.c after each patch is applied, not the !@#$ing
stupid patches.
Re: [PATCH 3/8] sg: split header, expand and correct descriptions
On 2018-10-19 4:31 a.m., Johannes Thumshirn wrote:
On 19/10/18 08:24, Douglas Gilbert wrote:
+/* Alternate style type names, "..._t" variants preferred */
+typedef struct sg_io_hdr Sg_io_hdr;
+typedef struct sg_io_vec Sg_io_vec;
+typedef struct sg_scsi_id Sg_scsi_id;
+typedef struct sg_req_info Sg_req_info;
There are no _t variants for the above, or am I missing something?
I've expanded the comment to make it clearer I'm referring to the
definitions above in that header ***.
For example: Referring to
typedef struct sg_io_hdr {
... /* the definition of its fields */
} sg_io_hdr_t;
So the suggestion is to prefer sg_io_hdr_t to Sg_io_hdr and if you
are using C rather that C++ (in the user space) and you have very
backward looking conventions then prefer:
struct sg_io_hdr
Is that clearer?
Doug Gilbert
*** If the unified diff doesn't show that, then that is one of the
many weakness of using unified diffs for code reviews. One
of the things I'm trying to clean up is 15 years of
"laparoscopic" patches (and diff and patch are fighting back).
Re: [PATCH 1/5] ips: use lower_32_bits and upper_32_bits instead of reinventing them
On Thu, 2018-10-18 at 15:03 +0200, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig
> ---
> drivers/scsi/ips.c | 6 +++---
> drivers/scsi/ips.h | 3 ---
> 2 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
> index ee8a1ecd58fd..679321e96a86 100644
> --- a/drivers/scsi/ips.c
> +++ b/drivers/scsi/ips.c
> @@ -1801,13 +1801,13 @@ ips_fill_scb_sg_single(ips_ha_t * ha, dma_addr_t
> busaddr,
> }
> if (IPS_USE_ENH_SGLIST(ha)) {
> scb->sg_list.enh_list[indx].address_lo =
> - cpu_to_le32(pci_dma_lo32(busaddr));
> + cpu_to_le32(lower_32_bits(busaddr));
> scb->sg_list.enh_list[indx].address_hi =
> - cpu_to_le32(pci_dma_hi32(busaddr));
> + cpu_to_le32(upper_32_bits(busaddr));
> scb->sg_list.enh_list[indx].length = cpu_to_le32(e_len);
> } else {
> scb->sg_list.std_list[indx].address =
> - cpu_to_le32(pci_dma_lo32(busaddr));
> + cpu_to_le32(lower_32_bits(busaddr));
> scb->sg_list.std_list[indx].length = cpu_to_le32(e_len);
> }
>
> diff --git a/drivers/scsi/ips.h b/drivers/scsi/ips.h
> index db546171e97f..42c180e3938b 100644
> --- a/drivers/scsi/ips.h
> +++ b/drivers/scsi/ips.h
> @@ -96,9 +96,6 @@
>#define __iomem
> #endif
>
> - #define pci_dma_hi32(a) ((a >> 16) >> 16)
> - #define pci_dma_lo32(a) (a & 0x)
> -
> #if (BITS_PER_LONG > 32) || defined(CONFIG_HIGHMEM64G)
>#define IPS_ENABLE_DMA64(1)
> #else
Reviewed-by: Bart Van Assche
Re: [PATCH 2/8] sg: introduce sg_log macro
On 2018-10-19 3:45 a.m., Johannes Thumshirn wrote:
On 19/10/18 08:24, Douglas Gilbert wrote:
[..]
+/*
+ * Kernel needs to be built with CONFIG_SCSI_LOGGING to see log messages.
+ * 'depth' is a number between 1 (most severe) and 7 (most noisy, most
+ * information). All messages are logged as informational (KERN_INFO). In
+ * the unexpected situation where sdp is NULL the macro reverts to a pr_info
+ * and ignores CONFIG_SCSI_LOGGING and always prints to the log.
+ */
+#define SG_LOG(depth, sdp, fmt, a...) \
+ do {\
+ if (IS_ERR_OR_NULL(sdp)) { \
+ pr_info("sg: sdp=NULL_or_ERR, " fmt, ##a);\
+ } else {\
+ SCSI_LOG_TIMEOUT(depth, sdev_prefix_printk( \
+KERN_INFO, (sdp)->device, \
+(sdp)->disk->disk_name, fmt, \
+##a)); \
+ } \
+ } while (0)
Hi Doug,
have you considered using the kernel's dynamic debug infrastructure instead?
Hi,
I'll follow what the scsi mid-level and the other ULDs do. IOW, no
change. The debug messages they produce are quite helpful (to me, I
use them a lot, and Tony B. has asked for more precision) and well-tuned
to the SCSI subsystem (e.g. telling us what sdp represents in useful
terms).
And they can be compiled out (but not my pr_info above, probably
should be a pr_warn).
Doug Gilbert
Re: [PATCH] scsi: myrs: fix build failure on 32 bit
On 10/18/18 4:50 PM, James Bottomley wrote:
> For 32 bit versions we have to be careful about divisions of 64 bit
> quantities so use do_div() instead of a direct division. This fixes a
> warning about _uldivmod being undefined in certain configurations
on i386 it was:
ERROR: "__udivdi3" [drivers/scsi/myrs.ko] undefined!
and this patch fixes that build error.
Tested-by: Randy Dunlap # build-tested
thanks.
>
> Fixes: 77266186397c ("scsi: myrs: Add Mylex RAID controller")
> Reported-by: kbuild test robot
> Signed-off-by: James Bottomley
> ---
> drivers/scsi/myrs.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/myrs.c b/drivers/scsi/myrs.c
> index b02ee0b0dd55..a9f9c77e889f 100644
> --- a/drivers/scsi/myrs.c
> +++ b/drivers/scsi/myrs.c
> @@ -1978,7 +1978,8 @@ myrs_get_resync(struct device *dev)
> struct scsi_device *sdev = to_scsi_device(dev);
> struct myrs_hba *cs = shost_priv(sdev->host);
> struct myrs_ldev_info *ldev_info = sdev->hostdata;
> - u8 percent_complete = 0, status;
> + u64 percent_complete = 0;
> + u8 status;
>
> if (sdev->channel < cs->ctlr_info->physchan_present || !ldev_info)
> return;
> @@ -1986,8 +1987,8 @@ myrs_get_resync(struct device *dev)
> unsigned short ldev_num = ldev_info->ldev_num;
>
> status = myrs_get_ldev_info(cs, ldev_num, ldev_info);
> - percent_complete = ldev_info->rbld_lba * 100 /
> - ldev_info->cfg_devsize;
> + percent_complete = ldev_info->rbld_lba * 100;
> + do_div(percent_complete, ldev_info->cfg_devsize);
> }
> raid_set_resync(myrs_raid_template, dev, percent_complete);
> }
>
--
~Randy
Re: [PATCH -next] scsi: qedf: remove set but not used variable 'fcport'
On Fri, 2018-10-19 at 12:12 +, YueHaibing wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/scsi/qedf/qedf_main.c: In function 'qedf_eh_abort':
> drivers/scsi/qedf/qedf_main.c:619:21: warning:
> variable 'fcport' set but not used [-Wunused-but-set-variable]
> struct qedf_rport *fcport;
>
> It never used since introduction in
> commit 61d8658b4a43 ("scsi: qedf: Add QLogic FastLinQ offload FCoE
> driver framework.")
>
> Signed-off-by: YueHaibing
> ---
> drivers/scsi/qedf/qedf_main.c | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/scsi/qedf/qedf_main.c
> b/drivers/scsi/qedf/qedf_main.c
> index d5a4f17..04f50a3 100644
> --- a/drivers/scsi/qedf/qedf_main.c
> +++ b/drivers/scsi/qedf/qedf_main.c
> @@ -615,8 +615,6 @@ static u32 qedf_get_login_failures(void *cookie)
> static int qedf_eh_abort(struct scsi_cmnd *sc_cmd)
> {
> struct fc_rport *rport =
> starget_to_rport(scsi_target(sc_cmd->device));
> - struct fc_rport_libfc_priv *rp = rport->dd_data;
> - struct qedf_rport *fcport;
> struct fc_lport *lport;
> struct qedf_ctx *qedf;
> struct qedf_ioreq *io_req;
> @@ -636,8 +634,6 @@ static int qedf_eh_abort(struct scsi_cmnd
> *sc_cmd)
> goto out;
> }
>
> - fcport = (struct qedf_rport *)[1];
> -
> io_req = (struct qedf_ioreq *)sc_cmd->SCp.ptr;
> if (!io_req) {
> QEDF_ERR(&(qedf->dbg_ctx), "io_req is NULL.\n");
>
>
>
Yes, but qedf_eh_host_reset() checks if (fcport == NULL) and
returns FAILED and it's not clear that qedf_eh_abort() shouldn't
do the same thing, consider that qedf_process_cqe() checks
whether io_req->fcport == NULL so it does not seem clear that
the io_req->fcport from sc_cmd->SCp.ptr will always be valid later...
Can someone from Cavium (Chad) comment?
-Ewan
Re: [PATCH 6/8] sg: complete locking changes on ioctl+debug
Hi linux-scsi-owner, Thank you for the patch! Yet something to improve: [auto build test ERROR on mkp-scsi/for-next] [also build test ERROR on v4.19-rc8 next-20181019] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/linux-scsi-owner-vger-kernel-org/sg-major-cleanup-remove-max_queue-limit/20181019-183809 base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next config: arm-multi_v5_defconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=arm All errors (new ones prefixed by >>): >> ERROR: "sg_rq_state_str" [drivers/scsi/sg.ko] undefined! --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
RE: Looking for some help understanding error handling
> -Original Message- > From: linux-scsi-ow...@vger.kernel.org ow...@vger.kernel.org> On Behalf Of John Garry > Sent: Friday, October 19, 2018 2:19 AM > To: Chris Moore - C33997 ; h...@suse.de; > linux-scsi@vger.kernel.org; Jason Yan > Subject: Re: Looking for some help understanding error handling > > On 05/10/2018 16:51, chris.mo...@microchip.com wrote: > > Thanks Hannes, > > > > After some pointers from Shane Seymour I found that the FC and SRP > > transport layers have a devloss timer, so that when a device > > disappears they hold on to the target information for a time waiting > > to see if it comes back. The SAS transport layer doesn't have that feature. > > > > The options for me then would be to modify scsi_transport_sas.c to > > implement the devloss timeout, or to put that functionality into my LLDD. > > > > I'm willing to put the work into the SAS transport and libsas, but I > > suspect there's not a universal need for it. And since my LLDD is for > > internal use at our company and won't be upstreamed, I'll probably > > just do the work there. If anyone feels that this is a feature that more > people would want then I'll look into doing that. > > Hello, > > This feature sounds interesting for libsas. I however have a question on > feasibility of devloss here (note: I'm not familiar with the > concept/realization > for other standards): if a device is deattached and re-attached, how can we > confirm the same device? For SAS device it's ok as a disk has the WWN, but > what about SATA? > > Thanks, > John Would the serial number work? I haven't worked a lot with SATA drives, but ATA8-ACS says the IDENTIFY DEVICE response must contain a unique serial number. Chris > > > > > Thanks, > > Chris > > > >> -Original Message- > >> From: Hannes Reinecke [mailto:h...@suse.de] > >> Sent: Friday, October 5, 2018 8:01 AM > >> To: Chris Moore - C33997 ; linux- > >> s...@vger.kernel.org > >> Subject: Re: Looking for some help understanding error handling > >> > >> On 10/2/18 11:04 PM, chris.mo...@microchip.com wrote: > >>> I'm working on LLDD for a SAS/SATA host adapter, and trying to > >>> understand > >> how the system handles link loss and recovery. > >>> > >>> Say I have a device that gets recognized and attached as sd > >>> 12:0:4:0, at > >> /dev/sdb. > >>> The drive goes offline temporarily, then comes back online. > >>> When it does, it comes back as sd 12:0:5:0, and maybe /dev/sdb, > >>> maybe > >> /dev/sdc. > >>> > >>> I'm not sure how the Id gets assigned. Since this is the same > >>> drive, is there some way my driver can tell libsas and/or SCSI core > >>> that it's the > >> same drive coming back? > >>> Or is there no way to control that? > >>> > >> Not really. The target device is getting destroyed once the device > >> disconnects, and when it reconnects a new structure is allocated. But > >> as the target number is a simple counter it gets increased up each > allocation. > >> > >>> I looked into /dev/disk/by-id, but that also didn't quite do what I > >>> expected. If I open /dev/disk/by-id/some_identifier, that's a > >>> symlink to, > >> say, /dev/sdb. > >> > >> Yes. > >> > >>> /dev/sdb goes away, comes back as /dev/sdc, but my process doesn't > >>> know that, it still has /dev/disk/by-id/some_identifier opened and > >>> so it will > >> never recover without closing and reopening the file. > >>> > >> Simply don't keep hold of the symlink; once you have opened you'll > >> miss any updates to the symlink itself. > >> So better to open the symlink, check the device, do whatever needs to > >> be done, and _close the symlink_ again. > >> Then you can listen for udev events telling you when a device appears > >> or vanishes. > >> > >> Cheers, > >> > >> Hannes > >> -- > >> Dr. Hannes Reinecke Teamlead Storage & Networking > >> h...@suse.de +49 911 74053 688 > >> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg > >> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB > >> 21284 (AG Nürnberg) >
Re: [PATCH] bsg: convert to use blk-mq
On Fri, Oct 19, 2018 at 09:50:53AM -0600, Jens Axboe wrote: > On 10/19/18 9:01 AM, Benjamin Block wrote: > > On Wed, Oct 17, 2018 at 10:01:16AM -0600, Jens Axboe wrote: > >> On 10/17/18 9:55 AM, Benjamin Block wrote: > >>> On Tue, Oct 16, 2018 at 08:43:01AM -0600, Jens Axboe wrote: > Requires a few changes to the FC transport class as well. > > Cc: Johannes Thumshirn > Cc: Benjamin Block > Cc: linux-scsi@vger.kernel.org > Signed-off-by: Jens Axboe > --- > block/bsg-lib.c | 102 +-- > drivers/scsi/scsi_transport_fc.c | 61 ++ > 2 files changed, 91 insertions(+), 72 deletions(-) > > >>> > >>> Hey Jens, > >>> > >>> I haven't had time to look into this in any deep way - but I did plan to > >>> -, but just to see whether it starts and runs some I/O I tried giving it > >>> a spin and came up with nothing (see line 3 and 5): > >> > >> I'm an idiot, can you try this on top? > >> > >> > >> diff --git a/block/bsg-lib.c b/block/bsg-lib.c > >> index 1aa0ed3fc339..95e12b635225 100644 > >> --- a/block/bsg-lib.c > >> +++ b/block/bsg-lib.c > >> @@ -311,7 +311,7 @@ struct request_queue *bsg_setup_queue(struct device > >> *dev, const char *name, > >>int ret = -ENOMEM; > >> > >>set = kzalloc(sizeof(*set), GFP_KERNEL); > >> - if (set) > >> + if (!set) > >>return ERR_PTR(-ENOMEM); > >> > >>set->ops = _mq_ops; > >> > > > > Well, yes, that would be wrong. But it still doesn't fly (s390x stack > > trace): > > > > [ 36.271953] scsi host0: scsi_eh_0: sleeping > > [ 36.272571] scsi host0: zfcp > > [ 36.298065] WARNING: CPU: 7 PID: 856 at block/blk-settings.c:71 > > blk_queue_rq_timed_out+0x44/0x60 > > [ 36.298315] Modules linked in: zfcp scsi_transport_fc dm_multipath > > [ 36.299015] CPU: 7 PID: 856 Comm: systemd-udevd Tainted: GW > >4.19.0-rc8-bb-next+ #1 > > [ 36.299059] Hardware name: IBM 3906 M03 704 (LPAR) > > [ 36.299101] Krnl PSW : 0704e0018000 00ced494 > > (blk_queue_rq_timed_out+0x44/0x60) > > [ 36.299192]R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 > > PM:0 RI:0 EA:3 > > [ 36.299259] Krnl GPRS: 015cee60 > > a0ce0180 0300 > > [ 36.299304]0300 00ced486 > > a5738000 03ff8069eba0 > > [ 36.299349]a11ec6a8 a0ce > > a11ec400 03ff805ee438 > > [ 36.299394]a0ce 03ff805f6b00 > > 00ced486 a28af0b0 > > [ 36.299460] Krnl Code: 00ced486: e310c182ltg > > %r1,384(%r12) > > 00ced48c: a7840004brc > > 8,ced494 > > #00ced490: a7f40001brc > > 15,ced492 > > >00ced494: 4120c150la > > %r2,336(%r12) > > 00ced498: c0e5ffc76290brasl > > %r14,5d99b8 > > 00ced49e: e3b0c1500024stg > > %r11,336(%r12) > > 00ced4a4: ebbff0a4lmg > > %r11,%r15,160(%r15) > > 00ced4aa: 07febcr > > 15,%r14 > > [ 36.299879] Call Trace: > > [ 36.299922] ([] 0xa11ec6a8) > > [ 36.299979] [<03ff805ee3fa>] fc_host_setup+0x622/0x660 > > [scsi_transport_fc] > > [ 36.300029] [<00f0baca>] transport_setup_classdev+0x62/0x70 > > [ 36.300075] [<00f0b592>] > > attribute_container_add_device+0x182/0x1d0 > > [ 36.300163] [<03ff80503cae>] scsi_sysfs_add_host+0x5e/0x100 > > [scsi_mod] > > [ 36.300245] [<03ff804e6d7c>] scsi_add_host_with_dma+0x2dc/0x468 > > [scsi_mod] > > [ 36.300310] [<03ff806835f2>] zfcp_scsi_adapter_register+0x222/0x260 > > [zfcp] > > [ 36.300373] [<03ff8066a49a>] zfcp_adapter_enqueue+0xae2/0xb20 > > [zfcp] > > [ 36.300435] [<03ff8066b436>] zfcp_ccw_set_online+0xb6/0x208 [zfcp] > > [ 36.300482] [<00f8ad14>] ccw_device_set_online+0x41c/0x878 > > [ 36.300527] [<00f8b374>] > > online_store_recog_and_online+0x204/0x230 > > [ 36.300572] [<00f8de20>] online_store+0x290/0x3e8 > > [ 36.300619] [<007842c0>] kernfs_fop_write+0x1b0/0x288 > > [ 36.300663] [<0064217e>] __vfs_write+0xee/0x398 > > [ 36.300705] [<006426b4>] vfs_write+0xec/0x238 > > [ 36.300754] [<00642abe>] ksys_write+0xd6/0x148 > > [ 36.300800] [<0137b668>] system_call+0x2b0/0x2d0 > > [ 36.300843] 5 locks held by systemd-udevd/856: > > [ 36.300882] #0: da3c74e2 (sb_writers#4){.+.+}, at: > > vfs_write+0xd6/0x238 > > [ 36.301053] #1: 2a1f461f (>mutex){+.+.}, at: > > kernfs_fop_write+0x258/0x288 > > [ 36.301202] #2: deb28615
Re: [PATCH] bsg: convert to use blk-mq
On 10/19/18 9:01 AM, Benjamin Block wrote: > On Wed, Oct 17, 2018 at 10:01:16AM -0600, Jens Axboe wrote: >> On 10/17/18 9:55 AM, Benjamin Block wrote: >>> On Tue, Oct 16, 2018 at 08:43:01AM -0600, Jens Axboe wrote: Requires a few changes to the FC transport class as well. Cc: Johannes Thumshirn Cc: Benjamin Block Cc: linux-scsi@vger.kernel.org Signed-off-by: Jens Axboe --- block/bsg-lib.c | 102 +-- drivers/scsi/scsi_transport_fc.c | 61 ++ 2 files changed, 91 insertions(+), 72 deletions(-) >>> >>> Hey Jens, >>> >>> I haven't had time to look into this in any deep way - but I did plan to >>> -, but just to see whether it starts and runs some I/O I tried giving it >>> a spin and came up with nothing (see line 3 and 5): >> >> I'm an idiot, can you try this on top? >> >> >> diff --git a/block/bsg-lib.c b/block/bsg-lib.c >> index 1aa0ed3fc339..95e12b635225 100644 >> --- a/block/bsg-lib.c >> +++ b/block/bsg-lib.c >> @@ -311,7 +311,7 @@ struct request_queue *bsg_setup_queue(struct device >> *dev, const char *name, >> int ret = -ENOMEM; >> >> set = kzalloc(sizeof(*set), GFP_KERNEL); >> -if (set) >> +if (!set) >> return ERR_PTR(-ENOMEM); >> >> set->ops = _mq_ops; >> > > Well, yes, that would be wrong. But it still doesn't fly (s390x stack > trace): > > [ 36.271953] scsi host0: scsi_eh_0: sleeping > [ 36.272571] scsi host0: zfcp > [ 36.298065] WARNING: CPU: 7 PID: 856 at block/blk-settings.c:71 > blk_queue_rq_timed_out+0x44/0x60 > [ 36.298315] Modules linked in: zfcp scsi_transport_fc dm_multipath > [ 36.299015] CPU: 7 PID: 856 Comm: systemd-udevd Tainted: GW > 4.19.0-rc8-bb-next+ #1 > [ 36.299059] Hardware name: IBM 3906 M03 704 (LPAR) > [ 36.299101] Krnl PSW : 0704e0018000 00ced494 > (blk_queue_rq_timed_out+0x44/0x60) > [ 36.299192]R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 > RI:0 EA:3 > [ 36.299259] Krnl GPRS: 015cee60 a0ce0180 > 0300 > [ 36.299304]0300 00ced486 a5738000 > 03ff8069eba0 > [ 36.299349]a11ec6a8 a0ce a11ec400 > 03ff805ee438 > [ 36.299394]a0ce 03ff805f6b00 00ced486 > a28af0b0 > [ 36.299460] Krnl Code: 00ced486: e310c182ltg > %r1,384(%r12) > 00ced48c: a7840004brc > 8,ced494 > #00ced490: a7f40001brc > 15,ced492 > >00ced494: 4120c150la > %r2,336(%r12) > 00ced498: c0e5ffc76290brasl > %r14,5d99b8 > 00ced49e: e3b0c1500024stg > %r11,336(%r12) > 00ced4a4: ebbff0a4lmg > %r11,%r15,160(%r15) > 00ced4aa: 07febcr > 15,%r14 > [ 36.299879] Call Trace: > [ 36.299922] ([] 0xa11ec6a8) > [ 36.299979] [<03ff805ee3fa>] fc_host_setup+0x622/0x660 > [scsi_transport_fc] > [ 36.300029] [<00f0baca>] transport_setup_classdev+0x62/0x70 > [ 36.300075] [<00f0b592>] > attribute_container_add_device+0x182/0x1d0 > [ 36.300163] [<03ff80503cae>] scsi_sysfs_add_host+0x5e/0x100 > [scsi_mod] > [ 36.300245] [<03ff804e6d7c>] scsi_add_host_with_dma+0x2dc/0x468 > [scsi_mod] > [ 36.300310] [<03ff806835f2>] zfcp_scsi_adapter_register+0x222/0x260 > [zfcp] > [ 36.300373] [<03ff8066a49a>] zfcp_adapter_enqueue+0xae2/0xb20 [zfcp] > [ 36.300435] [<03ff8066b436>] zfcp_ccw_set_online+0xb6/0x208 [zfcp] > [ 36.300482] [<00f8ad14>] ccw_device_set_online+0x41c/0x878 > [ 36.300527] [<00f8b374>] > online_store_recog_and_online+0x204/0x230 > [ 36.300572] [<00f8de20>] online_store+0x290/0x3e8 > [ 36.300619] [<007842c0>] kernfs_fop_write+0x1b0/0x288 > [ 36.300663] [<0064217e>] __vfs_write+0xee/0x398 > [ 36.300705] [<006426b4>] vfs_write+0xec/0x238 > [ 36.300754] [<00642abe>] ksys_write+0xd6/0x148 > [ 36.300800] [<0137b668>] system_call+0x2b0/0x2d0 > [ 36.300843] 5 locks held by systemd-udevd/856: > [ 36.300882] #0: da3c74e2 (sb_writers#4){.+.+}, at: > vfs_write+0xd6/0x238 > [ 36.301053] #1: 2a1f461f (>mutex){+.+.}, at: > kernfs_fop_write+0x258/0x288 > [ 36.301202] #2: deb28615 (kn->count#52){.+.+}, at: > kernfs_fop_write+0x26e/0x288 > [ 36.301374] #3: 2ddb9663 (>mutex){}, at: > online_store+0x19c/0x3e8 > [ 36.301523] #4: 982b5ed9 (attribute_container_mutex){+.+.}, at: > attribute_container_add_device+0x3c/0x1d0 > [
Re: [PATCH 5/8] sg: add free list, rework locking
On Fri, 2018-10-19 at 10:38 -0400, Tony Battersby wrote: > Incidentally, I have been using my own home-grown target-mode SCSI > system for the past 16 years, but now I am starting to look into > switching to SCST. I was just reading about their "SGV cache": > > http://scst.sourceforge.net/scst_pg.html > > It looks like it serves a similar purpose to what you are trying to > accomplish with recycling the indirect I/O buffers between different > requests. Perhaps you can borrow some inspiration from them (or even > some code). Although the current SCST SGV cache implementation is more complicated than necessary, I agree that it would be useful to have such a cache implementation in the Linux kernel. The NVMe target driver and the SCSI target core would also benefit from caching SGV allocations. Bart.
Re: [PATCH 5/8] sg: add free list, rework locking
On Fri, 2018-10-19 at 02:24 -0400, Douglas Gilbert wrote:
> static void
> -sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo)
> +sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo,
> + int max_num)
> {
> struct sg_request *srp;
> int val;
> - unsigned int ms;
>
> val = 0;
> - list_for_each_entry(srp, >rq_list, entry) {
> - if (val >= SG_MAX_QUEUE)
> - break;
> - rinfo[val].req_state = srp->done + 1;
> + list_for_each_entry(srp, >rq_list, rq_entry) {
> + if (val >= max_num)
> + return;
What protects the sfp->rq_list against concurrent changes? It seems to me
like all other code that iterates over or modifies that list protects that
list with rq_list_lock?
Thanks,
Bart.
Re: [PATCH 4/8] sg: expand request states
On Fri, 2018-10-19 at 02:24 -0400, Douglas Gilbert wrote: > +/* Following defines are states of sg_request::rq_state */ > +#define SG_RQ_INACTIVE 0/* request not in use (e.g. on fl) */ > +#define SG_RQ_INFLIGHT 1/* SCSI request issued, no response yet */ > +#define SG_RQ_AWAIT_READ 2 /* response received, awaiting read */ > +#define SG_RQ_DONE_READ 3 /* read is ongoing or done */ > +#define SG_RQ_BUSY 4/* example: reserve request changing size */ Please introduce an enumeration type instead of #defining these constants to allow the compiler to verify assignments to sg_request::rq_state. Thanks, Bart.
Re: [PATCH] bsg: convert to use blk-mq
On Wed, Oct 17, 2018 at 10:01:16AM -0600, Jens Axboe wrote: > On 10/17/18 9:55 AM, Benjamin Block wrote: > > On Tue, Oct 16, 2018 at 08:43:01AM -0600, Jens Axboe wrote: > >> Requires a few changes to the FC transport class as well. > >> > >> Cc: Johannes Thumshirn > >> Cc: Benjamin Block > >> Cc: linux-scsi@vger.kernel.org > >> Signed-off-by: Jens Axboe > >> --- > >> block/bsg-lib.c | 102 +-- > >> drivers/scsi/scsi_transport_fc.c | 61 ++ > >> 2 files changed, 91 insertions(+), 72 deletions(-) > >> > > > > Hey Jens, > > > > I haven't had time to look into this in any deep way - but I did plan to > > -, but just to see whether it starts and runs some I/O I tried giving it > > a spin and came up with nothing (see line 3 and 5): > > I'm an idiot, can you try this on top? > > > diff --git a/block/bsg-lib.c b/block/bsg-lib.c > index 1aa0ed3fc339..95e12b635225 100644 > --- a/block/bsg-lib.c > +++ b/block/bsg-lib.c > @@ -311,7 +311,7 @@ struct request_queue *bsg_setup_queue(struct device *dev, > const char *name, > int ret = -ENOMEM; > > set = kzalloc(sizeof(*set), GFP_KERNEL); > - if (set) > + if (!set) > return ERR_PTR(-ENOMEM); > > set->ops = _mq_ops; > Well, yes, that would be wrong. But it still doesn't fly (s390x stack trace): [ 36.271953] scsi host0: scsi_eh_0: sleeping [ 36.272571] scsi host0: zfcp [ 36.298065] WARNING: CPU: 7 PID: 856 at block/blk-settings.c:71 blk_queue_rq_timed_out+0x44/0x60 [ 36.298315] Modules linked in: zfcp scsi_transport_fc dm_multipath [ 36.299015] CPU: 7 PID: 856 Comm: systemd-udevd Tainted: GW 4.19.0-rc8-bb-next+ #1 [ 36.299059] Hardware name: IBM 3906 M03 704 (LPAR) [ 36.299101] Krnl PSW : 0704e0018000 00ced494 (blk_queue_rq_timed_out+0x44/0x60) [ 36.299192]R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 RI:0 EA:3 [ 36.299259] Krnl GPRS: 015cee60 a0ce0180 0300 [ 36.299304]0300 00ced486 a5738000 03ff8069eba0 [ 36.299349]a11ec6a8 a0ce a11ec400 03ff805ee438 [ 36.299394]a0ce 03ff805f6b00 00ced486 a28af0b0 [ 36.299460] Krnl Code: 00ced486: e310c182ltg %r1,384(%r12) 00ced48c: a7840004brc 8,ced494 #00ced490: a7f40001brc 15,ced492 >00ced494: 4120c150la %r2,336(%r12) 00ced498: c0e5ffc76290brasl %r14,5d99b8 00ced49e: e3b0c1500024stg %r11,336(%r12) 00ced4a4: ebbff0a4lmg %r11,%r15,160(%r15) 00ced4aa: 07febcr 15,%r14 [ 36.299879] Call Trace: [ 36.299922] ([] 0xa11ec6a8) [ 36.299979] [<03ff805ee3fa>] fc_host_setup+0x622/0x660 [scsi_transport_fc] [ 36.300029] [<00f0baca>] transport_setup_classdev+0x62/0x70 [ 36.300075] [<00f0b592>] attribute_container_add_device+0x182/0x1d0 [ 36.300163] [<03ff80503cae>] scsi_sysfs_add_host+0x5e/0x100 [scsi_mod] [ 36.300245] [<03ff804e6d7c>] scsi_add_host_with_dma+0x2dc/0x468 [scsi_mod] [ 36.300310] [<03ff806835f2>] zfcp_scsi_adapter_register+0x222/0x260 [zfcp] [ 36.300373] [<03ff8066a49a>] zfcp_adapter_enqueue+0xae2/0xb20 [zfcp] [ 36.300435] [<03ff8066b436>] zfcp_ccw_set_online+0xb6/0x208 [zfcp] [ 36.300482] [<00f8ad14>] ccw_device_set_online+0x41c/0x878 [ 36.300527] [<00f8b374>] online_store_recog_and_online+0x204/0x230 [ 36.300572] [<00f8de20>] online_store+0x290/0x3e8 [ 36.300619] [<007842c0>] kernfs_fop_write+0x1b0/0x288 [ 36.300663] [<0064217e>] __vfs_write+0xee/0x398 [ 36.300705] [<006426b4>] vfs_write+0xec/0x238 [ 36.300754] [<00642abe>] ksys_write+0xd6/0x148 [ 36.300800] [<0137b668>] system_call+0x2b0/0x2d0 [ 36.300843] 5 locks held by systemd-udevd/856: [ 36.300882] #0: da3c74e2 (sb_writers#4){.+.+}, at: vfs_write+0xd6/0x238 [ 36.301053] #1: 2a1f461f (>mutex){+.+.}, at: kernfs_fop_write+0x258/0x288 [ 36.301202] #2: deb28615 (kn->count#52){.+.+}, at: kernfs_fop_write+0x26e/0x288 [ 36.301374] #3: 2ddb9663 (>mutex){}, at: online_store+0x19c/0x3e8 [ 36.301523] #4: 982b5ed9 (attribute_container_mutex){+.+.}, at: attribute_container_add_device+0x3c/0x1d0 [ 36.301675] Last Breaking-Event-Address: [ 36.301717] [<00ced490>] blk_queue_rq_timed_out+0x40/0x60 [ 36.301758] irq event stamp: 9073 [ 36.301804] hardirqs last enabled at (9081):
Re: [PATCH 5/8] sg: add free list, rework locking
On 10/19/18 2:24 AM, Douglas Gilbert wrote:
> + if (sfp->tot_fd_thresh)
> + sfp->sum_fd_dlens += align_sz;
>
What lock protects sfp->sum_fd_dlens?
> /*
> @@ -2216,38 +2401,85 @@ sg_add_request(struct sg_fd *sfp)
> * data length exceeds rem_sgat_thresh then the data (or sgat) is
> * cleared and the request is appended to the tail of the free list.
> */
> -static int
> +static void
> sg_remove_request(struct sg_fd *sfp, struct sg_request *srp)
> {
> + bool reserve;
> unsigned long iflags;
> - int res = 0;
> + const char *cp = "head";
> + char b[64];
>
> - if (!sfp || !srp || list_empty(>rq_list))
> - return res;
> + if (WARN_ON(!sfp || !srp))
> + return;
> write_lock_irqsave(>rq_list_lock, iflags);
> - if (!list_empty(>entry)) {
> - list_del(>entry);
> - srp->parentfp = NULL;
> - res = 1;
> - }
> + spin_lock(>rq_entry_lck);
> + /*
> + * N.B. sg_request object not de-allocated (freed). The contents of
> + * rq_list and rq_free_list lists are de-allocated (freed) when the
> + * owning file descriptor is closed. The free list acts as a LIFO.
> + * This can improve the chance of a cache hit when request is re-used.
> + */
> + reserve = (sfp->reserve_srp == srp);
> + if (reserve || srp->data.dlen <= sfp->rem_sgat_thresh) {
> + list_del(>rq_entry);
> + if (srp->data.dlen > 0)
> + list_add(>free_entry, >rq_free_list);
> + else {
> + list_add_tail(>free_entry, >rq_free_list);
> + cp = "tail";
> + }
> + snprintf(b, sizeof(b), "%ssrp=0x%p move to fl %s",
> + (reserve ? "reserve " : ""), srp, cp);
> + } else {
> + srp->rq_state = SG_RQ_BUSY;
> + list_del(>rq_entry);
> + spin_unlock(>rq_entry_lck);
> + write_unlock_irqrestore(>rq_list_lock, iflags);
> + if (sfp->sum_fd_dlens) {
> + u32 uv = srp->data.dlen;
> +
> + if (uv <= sfp->sum_fd_dlens)
> + sfp->sum_fd_dlens -= uv;
> + else {
> + SG_LOG(2, sfp->parentdp,
> +"%s: logic error this dlen > %s\n",
> +__func__, "sum_fd_dlens");
> + sfp->sum_fd_dlens = 0;
> + }
> + }
> + sg_remove_sgat(srp);
> + /* don't kfree(srp), move clear request to tail of fl */
> + write_lock_irqsave(>rq_list_lock, iflags);
> + spin_lock(>rq_entry_lck);
> + list_add_tail(>free_entry, >rq_free_list);
> + snprintf(b, sizeof(b), "clear sgat srp=0x%p move to fl tail",
> + srp);
> + }
>
>
Here again, no lock protecting sfp->sum_fd_dlens.
Incidentally, I have been using my own home-grown target-mode SCSI
system for the past 16 years, but now I am starting to look into
switching to SCST. I was just reading about their "SGV cache":
http://scst.sourceforge.net/scst_pg.html
It looks like it serves a similar purpose to what you are trying to
accomplish with recycling the indirect I/O buffers between different
requests. Perhaps you can borrow some inspiration from them (or even
some code).
Tony Battersby
Cybernetics
[PATCH -next] scsi: qedf: remove set but not used variable 'fcport'
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/scsi/qedf/qedf_main.c: In function 'qedf_eh_abort':
drivers/scsi/qedf/qedf_main.c:619:21: warning:
variable 'fcport' set but not used [-Wunused-but-set-variable]
struct qedf_rport *fcport;
It never used since introduction in
commit 61d8658b4a43 ("scsi: qedf: Add QLogic FastLinQ offload FCoE driver
framework.")
Signed-off-by: YueHaibing
---
drivers/scsi/qedf/qedf_main.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
index d5a4f17..04f50a3 100644
--- a/drivers/scsi/qedf/qedf_main.c
+++ b/drivers/scsi/qedf/qedf_main.c
@@ -615,8 +615,6 @@ static u32 qedf_get_login_failures(void *cookie)
static int qedf_eh_abort(struct scsi_cmnd *sc_cmd)
{
struct fc_rport *rport = starget_to_rport(scsi_target(sc_cmd->device));
- struct fc_rport_libfc_priv *rp = rport->dd_data;
- struct qedf_rport *fcport;
struct fc_lport *lport;
struct qedf_ctx *qedf;
struct qedf_ioreq *io_req;
@@ -636,8 +634,6 @@ static int qedf_eh_abort(struct scsi_cmnd *sc_cmd)
goto out;
}
- fcport = (struct qedf_rport *)[1];
-
io_req = (struct qedf_ioreq *)sc_cmd->SCp.ptr;
if (!io_req) {
QEDF_ERR(&(qedf->dbg_ctx), "io_req is NULL.\n");
Re: [PATCH 5/8] sg: add free list, rework locking
Hi linux-scsi-owner, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on mkp-scsi/for-next] [also build test WARNING on v4.19-rc8 next-20181019] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/linux-scsi-owner-vger-kernel-org/sg-major-cleanup-remove-max_queue-limit/20181019-183809 base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next config: i386-randconfig-x078-201841 (attached as .config) compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 All warnings (new ones prefixed by >>): >> drivers//scsi/sg.c:240:20: warning: 'sg_rq_state_str' used but never defined static const char *sg_rq_state_str(u8 rq_state, bool long_str); ^~~ drivers//scsi/sg.c:933:1: warning: 'sg_fill_request_table' defined but not used [-Wunused-function] sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo, ^ drivers//scsi/sg.c:19:12: warning: 'sg_version_num' defined but not used [-Wunused-variable] static int sg_version_num = 30901; /* 2 digits for each component */ ^~ vim +/sg_rq_state_str +240 drivers//scsi/sg.c 212 213 /* tasklet or soft irq callback */ 214 static void sg_rq_end_io(struct request *rq, blk_status_t status); 215 static int sg_start_req(struct sg_request *srp, u8 *cmd); 216 static void sg_finish_scsi_blk_rq(struct sg_request *srp); 217 static int sg_mk_sgat_dlen(struct sg_request *srp, struct sg_fd *sfp, 218 int dlen); 219 static ssize_t sg_new_read(struct sg_fd *sfp, char __user *buf, size_t count, 220 struct sg_request *srp); 221 static ssize_t sg_v3_write(struct sg_fd *sfp, struct file *file, 222 const char __user *buf, size_t count, 223 bool read_only, bool sync, 224 struct sg_request **o_srp); 225 static struct sg_request *sg_common_write(struct sg_fd *sfp, 226const struct sg_io_hdr *hp, 227struct sg_io_v4 *h4p, u8 *cmnd, 228bool sync, int timeout); 229 static int sg_read_oxfer(struct sg_request *srp, char __user *outp, 230 int num_xfer); 231 static void sg_remove_sgat(struct sg_request *srp); 232 static struct sg_fd *sg_add_sfp(struct sg_device *sdp); 233 static void sg_remove_sfp(struct kref *); 234 static struct sg_request *sg_get_rq_pack_id(struct sg_fd *sfp, int pack_id); 235 static struct sg_request *sg_add_request(struct sg_fd *sfp, int dxfr_len, 236 bool sync); 237 static void sg_remove_request(struct sg_fd *sfp, struct sg_request *srp); 238 static struct sg_device *sg_get_dev(int min_dev); 239 static void sg_device_destroy(struct kref *kref); > 240 static const char *sg_rq_state_str(u8 rq_state, bool long_str); 241 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
[PATCH 3/3] scsi: myrs: prevent negatives in disable_enclosure_messages_store()
We only want the value to be zero or one.
It's not a big deal, but say we passed set value to INT_MIN, then
disable_enclosure_messages_show() would return that 12 bytes of "buf"
are initialized but actually only 3 are. I think there are tools like
KASAN which will trigger an info leak warning when that happens.
Fixes: 77266186397c ("scsi: myrs: Add Mylex RAID controller (SCSI interface)")
Signed-off-by: Dan Carpenter
---
drivers/scsi/myrs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/myrs.c b/drivers/scsi/myrs.c
index 07e5a3f54e31..55842ed54231 100644
--- a/drivers/scsi/myrs.c
+++ b/drivers/scsi/myrs.c
@@ -1501,7 +1501,7 @@ static ssize_t disable_enclosure_messages_store(struct
device *dev,
if (ret)
return ret;
- if (value > 2)
+ if (value < 0 || value > 2)
return -EINVAL;
cs->disable_enc_msg = value;
--
2.11.0
Re: Looking for some help understanding error handling
On 05/10/2018 16:51, chris.mo...@microchip.com wrote: Thanks Hannes, After some pointers from Shane Seymour I found that the FC and SRP transport layers have a devloss timer, so that when a device disappears they hold on to the target information for a time waiting to see if it comes back. The SAS transport layer doesn't have that feature. The options for me then would be to modify scsi_transport_sas.c to implement the devloss timeout, or to put that functionality into my LLDD. I'm willing to put the work into the SAS transport and libsas, but I suspect there's not a universal need for it. And since my LLDD is for internal use at our company and won't be upstreamed, I'll probably just do the work there. If anyone feels that this is a feature that more people would want then I'll look into doing that. Hello, This feature sounds interesting for libsas. I however have a question on feasibility of devloss here (note: I'm not familiar with the concept/realization for other standards): if a device is deattached and re-attached, how can we confirm the same device? For SAS device it's ok as a disk has the WWN, but what about SATA? Thanks, John Thanks, Chris -Original Message- From: Hannes Reinecke [mailto:h...@suse.de] Sent: Friday, October 5, 2018 8:01 AM To: Chris Moore - C33997 ; linux- s...@vger.kernel.org Subject: Re: Looking for some help understanding error handling On 10/2/18 11:04 PM, chris.mo...@microchip.com wrote: I'm working on LLDD for a SAS/SATA host adapter, and trying to understand how the system handles link loss and recovery. Say I have a device that gets recognized and attached as sd 12:0:4:0, at /dev/sdb. The drive goes offline temporarily, then comes back online. When it does, it comes back as sd 12:0:5:0, and maybe /dev/sdb, maybe /dev/sdc. I'm not sure how the Id gets assigned. Since this is the same drive, is there some way my driver can tell libsas and/or SCSI core that it's the same drive coming back? Or is there no way to control that? Not really. The target device is getting destroyed once the device disconnects, and when it reconnects a new structure is allocated. But as the target number is a simple counter it gets increased up each allocation. I looked into /dev/disk/by-id, but that also didn't quite do what I expected. If I open /dev/disk/by-id/some_identifier, that's a symlink to, say, /dev/sdb. Yes. /dev/sdb goes away, comes back as /dev/sdc, but my process doesn't know that, it still has /dev/disk/by-id/some_identifier opened and so it will never recover without closing and reopening the file. Simply don't keep hold of the symlink; once you have opened you'll miss any updates to the symlink itself. So better to open the symlink, check the device, do whatever needs to be done, and _close the symlink_ again. Then you can listen for udev events telling you when a device appears or vanishes. Cheers, Hannes -- Dr. Hannes ReineckeTeamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
[PATCH 2/3] scsi: myrs: Fix the processor absent message in processor_show()
If both processors are absent then it's supposed to print that, but
instead we print that just the second processor is absent.
Fixes: 77266186397c ("scsi: myrs: Add Mylex RAID controller (SCSI interface)")
Signed-off-by: Dan Carpenter
---
drivers/scsi/myrs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/myrs.c b/drivers/scsi/myrs.c
index b02ee0b0dd55..07e5a3f54e31 100644
--- a/drivers/scsi/myrs.c
+++ b/drivers/scsi/myrs.c
@@ -1366,11 +1366,11 @@ static ssize_t processor_show(struct device *dev,
first_processor, info->cpu[0].cpu_count,
info->cpu[1].cpu_name,
second_processor, info->cpu[1].cpu_count);
- else if (!second_processor)
+ else if (first_processor && !second_processor)
ret = snprintf(buf, 64, "1: %s (%s, %d cpus)\n2: absent\n",
info->cpu[0].cpu_name,
first_processor, info->cpu[0].cpu_count);
- else if (!first_processor)
+ else if (!first_processor && second_processor)
ret = snprintf(buf, 64, "1: absent\n2: %s (%s, %d cpus)\n",
info->cpu[1].cpu_name,
second_processor, info->cpu[1].cpu_count);
--
2.11.0
[PATCH 1/3] scsi: myrs: Fix a logical vs bitwise bug
The || was supposed to be |. The original code just sets ->result to 1.
Fixes: 77266186397c ("scsi: myrs: Add Mylex RAID controller (SCSI interface)")
Signed-off-by: Dan Carpenter
---
drivers/scsi/myrs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/myrs.c b/drivers/scsi/myrs.c
index b02ee0b0dd55..17e691bde485 100644
--- a/drivers/scsi/myrs.c
+++ b/drivers/scsi/myrs.c
@@ -2085,7 +2085,7 @@ static void myrs_handle_scsi(struct myrs_hba *cs, struct
myrs_cmdblk *cmd_blk,
status == MYRS_STATUS_DEVICE_NON_RESPONSIVE2)
scmd->result = (DID_BAD_TARGET << 16);
else
- scmd->result = (DID_OK << 16) || status;
+ scmd->result = (DID_OK << 16) | status;
scmd->scsi_done(scmd);
}
--
2.11.0
Re: [PATCH 3/8] sg: split header, expand and correct descriptions
On 19/10/18 08:24, Douglas Gilbert wrote: > +/* Alternate style type names, "..._t" variants preferred */ > +typedef struct sg_io_hdr Sg_io_hdr; > +typedef struct sg_io_vec Sg_io_vec; > +typedef struct sg_scsi_id Sg_scsi_id; > +typedef struct sg_req_info Sg_req_info; There are no _t variants for the above, or am I missing something? -- Johannes ThumshirnSUSE Labs 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 4/8] sg: expand request states
Looks good (but the ifdefs are creepy), Reviewed-by: Johannes Thumshirn -- Johannes ThumshirnSUSE Labs 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 2/8] sg: introduce sg_log macro
On 19/10/18 08:24, Douglas Gilbert wrote:
[..]
> +/*
> + * Kernel needs to be built with CONFIG_SCSI_LOGGING to see log messages.
> + * 'depth' is a number between 1 (most severe) and 7 (most noisy, most
> + * information). All messages are logged as informational (KERN_INFO). In
> + * the unexpected situation where sdp is NULL the macro reverts to a pr_info
> + * and ignores CONFIG_SCSI_LOGGING and always prints to the log.
> + */
> +#define SG_LOG(depth, sdp, fmt, a...)\
> + do {\
> + if (IS_ERR_OR_NULL(sdp)) { \
> + pr_info("sg: sdp=NULL_or_ERR, " fmt, ##a); \
> + } else {\
> + SCSI_LOG_TIMEOUT(depth, sdev_prefix_printk( \
> + KERN_INFO, (sdp)->device, \
> + (sdp)->disk->disk_name, fmt, \
> + ##a)); \
> + } \
> + } while (0)
Hi Doug,
have you considered using the kernel's dynamic debug infrastructure instead?
--
Johannes ThumshirnSUSE Labs
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 1/8] sg: types and naming cleanup
On 19/10/18 08:24, Douglas Gilbert wrote: > Remove typedefs and use better type names like bool and u8 where > appropriate. Rename some variables and functions for clarity. > Adjust formatting (e.g. function definitions) to be more > consistent across the driver. Thanks a lot Doug, this is highly appreciated. I have one minor Nit if you need to resend the series, please remove the casts form void* (mostly filp->private_data). Otherwise: Reviewed-by: Johannes Thumshirn -- Johannes ThumshirnSUSE Labs 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] scsi: myrs: fix build failure on 32 bit
On 10/19/18 1:50 AM, James Bottomley wrote:
For 32 bit versions we have to be careful about divisions of 64 bit
quantities so use do_div() instead of a direct division. This fixes a
warning about _uldivmod being undefined in certain configurations
Fixes: 77266186397c ("scsi: myrs: Add Mylex RAID controller")
Reported-by: kbuild test robot
Signed-off-by: James Bottomley
---
drivers/scsi/myrs.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/myrs.c b/drivers/scsi/myrs.c
index b02ee0b0dd55..a9f9c77e889f 100644
--- a/drivers/scsi/myrs.c
+++ b/drivers/scsi/myrs.c
@@ -1978,7 +1978,8 @@ myrs_get_resync(struct device *dev)
struct scsi_device *sdev = to_scsi_device(dev);
struct myrs_hba *cs = shost_priv(sdev->host);
struct myrs_ldev_info *ldev_info = sdev->hostdata;
- u8 percent_complete = 0, status;
+ u64 percent_complete = 0;
+ u8 status;
if (sdev->channel < cs->ctlr_info->physchan_present || !ldev_info)
return;
@@ -1986,8 +1987,8 @@ myrs_get_resync(struct device *dev)
unsigned short ldev_num = ldev_info->ldev_num;
status = myrs_get_ldev_info(cs, ldev_num, ldev_info);
- percent_complete = ldev_info->rbld_lba * 100 /
- ldev_info->cfg_devsize;
+ percent_complete = ldev_info->rbld_lba * 100;
+ do_div(percent_complete, ldev_info->cfg_devsize);
}
raid_set_resync(myrs_raid_template, dev, percent_complete);
}
Thanks James.
Reviewed-by: Hannes Reinecke
Cheers,
Hannes
[PATCH 3/8] sg: split header, expand and correct descriptions
Split scsi/sg.h into a smaller scsi/sg.h which includes a new header:
uapi/scsi/sg.h . Overall expand the twin header files with new
functionality in this patchset and functionality to be added in the
next patchset to implement SG_IOSUBMIT and friends. Adjust format to
modern kernel conventions. Correct and expand some comments.
Signed-off-by: Douglas Gilbert
---
The new header file: uapi/scsi/sg.h is expected to be in the
kernel's public interface. This takes time (i.e. months or years)
to find its way into glibc and Linux distributions. So the sooner
it is presented and accepted the better.
>From the C perspective, nothing is removed or changed (or shouldn't
be), only additions. This means that the original typedefs stay (but
they are not used in the driver source file: sg.c) since user space
programs may be using them.
include/scsi/sg.h | 252 ++---
include/uapi/scsi/sg.h | 415 +
2 files changed, 426 insertions(+), 241 deletions(-)
create mode 100644 include/uapi/scsi/sg.h
diff --git a/include/scsi/sg.h b/include/scsi/sg.h
index f91bcca604e4..596f68746f66 100644
--- a/include/scsi/sg.h
+++ b/include/scsi/sg.h
@@ -7,23 +7,23 @@
/*
* History:
* Started: Aug 9 by Lawrence Foard (entr...@world.std.com), to allow user
- * process control of SCSI devices.
+ * process control of SCSI devices.
* Development Sponsored by Killy Corp. NY NY
*
* Original driver (sg.h):
- * Copyright (C) 1992 Lawrence Foard
+ * Copyright (C) 1992 Lawrence Foard
* Version 2 and 3 extensions to driver:
- * Copyright (C) 1998 - 2014 Douglas Gilbert
+ * Copyright (C) 1998 - 2018 Douglas Gilbert
*
- * Version: 3.5.36 (20140603)
- * This version is for 2.6 and 3 series kernels.
+ * Version: 3.9.01 (20181016)
+ * This version is for 2.6, 3 and 4 series kernels.
*
* Documentation
* =
* A web site for the SG device driver can be found at:
- * http://sg.danny.cz/sg [alternatively check the MAINTAINERS file]
+ * http://sg.danny.cz/sg [alternatively check the MAINTAINERS file]
* The documentation for the sg version 3 driver can be found at:
- * http://sg.danny.cz/sg/p/sg_v3_ho.html
+ * http://sg.danny.cz/sg/p/sg_v3_ho.html
* Also see: /Documentation/scsi/scsi-generic.txt
*
* For utility and test programs see: http://sg.danny.cz/sg/sg3_utils.html
@@ -33,242 +33,12 @@
extern int sg_big_buff; /* for sysctl */
#endif
+#include
-typedef struct sg_iovec /* same structure as used by readv() Linux system */
-{ /* call. It defines one scatter-gather element. */
-void __user *iov_base; /* Starting address */
-size_t iov_len; /* Length in bytes */
-} sg_iovec_t;
-
-
-typedef struct sg_io_hdr
-{
-int interface_id; /* [i] 'S' for SCSI generic (required) */
-int dxfer_direction;/* [i] data transfer direction */
-unsigned char cmd_len; /* [i] SCSI command length */
-unsigned char mx_sb_len;/* [i] max length to write to sbp */
-unsigned short iovec_count; /* [i] 0 implies no scatter gather */
-unsigned int dxfer_len; /* [i] byte count of data transfer */
-void __user *dxferp; /* [i], [*io] points to data transfer memory
- or scatter gather list */
-unsigned char __user *cmdp; /* [i], [*i] points to command to perform */
-void __user *sbp; /* [i], [*o] points to sense_buffer memory */
-unsigned int timeout; /* [i] MAX_UINT->no timeout (unit: millisec) */
-unsigned int flags; /* [i] 0 -> default, see SG_FLAG... */
-int pack_id;/* [i->o] unused internally (normally) */
-void __user * usr_ptr; /* [i->o] unused internally */
-unsigned char status; /* [o] scsi status */
-unsigned char masked_status;/* [o] shifted, masked scsi status */
-unsigned char msg_status; /* [o] messaging level data (optional) */
-unsigned char sb_len_wr;/* [o] byte count actually written to sbp */
-unsigned short host_status; /* [o] errors from host adapter */
-unsigned short driver_status;/* [o] errors from software driver */
-int resid; /* [o] dxfer_len - actual_transferred */
-unsigned int duration; /* [o] time taken by cmd (unit: millisec) */
-unsigned int info; /* [o] auxiliary information */
-} sg_io_hdr_t; /* 64 bytes long (on i386) */
-
-#define SG_INTERFACE_ID_ORIG 'S'
-
-/* Use negative values to flag difference from original sg_header structure */
-#define SG_DXFER_NONE (-1) /* e.g. a SCSI Test Unit Ready command */
-#define SG_DXFER_TO_DEV (-2)/* e.g. a SCSI WRITE command */
-#define SG_DXFER_FROM_DEV (-3) /* e.g. a SCSI READ command */
-#define SG_DXFER_TO_FROM_DEV (-4) /* treated like SG_DXFER_FROM_DEV with the
- additional property than during indirect
-
[PATCH 7/8] sg: rework ioctl handling
Rework ioctl handling, report clearly to the log which ioctl has
been invoked. Add a new "IOWR" ioctl: SG_SET_GET_EXTENDED which
permits several integer and boolean values to be "_SET_" (i.e.
passed into the driver, potentially changing its actions) and/or
read from the driver (the "_GET_" part) in a single operation.
Signed-off-by: Douglas Gilbert
---
One feature of the new SG_SET_GET_EXTENDED ioctl is ability to
fetch the sg device minor number (e.g. the "3" in /dev/sg3)
associated with the current file descriptor. A boolean addition
is the ability to change command timekeeping on the current file
descriptor from milliseconds (the default) to nanoseconds.
drivers/scsi/sg.c | 546 +++---
1 file changed, 414 insertions(+), 132 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 9b30d6667cc9..583846ebc5e0 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -76,11 +76,11 @@ static int sg_proc_init(void);
#define SG_MAX_CDB_SIZE 252
/* Following defines are states of sg_request::rq_state */
-#define SG_RQ_INACTIVE 0/* request not in use (e.g. on fl) */
-#define SG_RQ_INFLIGHT 1/* SCSI request issued, no response yet */
-#define SG_RQ_AWAIT_READ 2 /* response received, awaiting read */
-#define SG_RQ_DONE_READ 3 /* read is ongoing or done */
-#define SG_RQ_BUSY 4/* example: reserve request changing size */
+#define SG_RQ_INACTIVE 0 /* request not in use (e.g. on fl) */
+#define SG_RQ_INFLIGHT 1 /* SCSI request issued, no response yet */
+#define SG_RQ_AWAIT_READ 2 /* response received, awaiting read */
+#define SG_RQ_DONE_READ 3 /* read is ongoing or done */
+#define SG_RQ_BUSY 4 /* example: reserve request changing size */
/* free up requests larger than this dlen size after use */
#define SG_RQ_DATA_THRESHOLD (128 * 1024)
@@ -88,8 +88,8 @@ static int sg_proc_init(void);
/* If sum_of(dlen) of a fd exceeds this, write() will yield E2BIG */
#define SG_TOT_FD_THRESHOLD (16 * 1024 * 1024)
-#define SG_TIME_UNIT_MS 0 /* milliseconds */
-#define SG_TIME_UNIT_NS 1 /* nanoseconds */
+#define SG_TIME_UNIT_MS 0 /* milliseconds */
+#define SG_TIME_UNIT_NS 1 /* nanoseconds */
#define SG_DEF_TIME_UNIT SG_TIME_UNIT_MS
#define SG_DEFAULT_TIMEOUT mult_frac(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
@@ -184,7 +184,6 @@ struct sg_fd { /* holds the state of a file
descriptor */
bool cmd_q; /* true -> allow command queuing, false -> don't */
bool keep_orphan;/* false -> drop (def), true -> keep for read() */
bool mmap_called; /* false -> mmap() never called on this fd */
- bool sse_seen; /* SG_SET_EXTENDED ioctl seen */
bool time_in_ns;/* report times in nanoseconds */
u8 next_cmd_len;/* 0: automatic, >0: use on next write() */
struct sg_request *reserve_srp; /* allocate on open(), starts on fl */
@@ -238,6 +237,8 @@ static void sg_remove_request(struct sg_fd *sfp, struct
sg_request *srp);
static struct sg_device *sg_get_dev(int min_dev);
static void sg_device_destroy(struct kref *kref);
static const char *sg_rq_state_str(u8 rq_state, bool long_str);
+static struct sg_request *sg_mk_srp(struct sg_fd *sfp, bool first,
+ rwlock_t *rwlp, unsigned long *iflagsp);
#define SZ_SG_HEADER sizeof(struct sg_header) /* v1 and v2 header */
#define SZ_SG_IO_HDR sizeof(struct sg_io_hdr) /* v3 header */
@@ -505,9 +506,9 @@ sg_read(struct file *filp, char __user *buf, size_t count,
loff_t * ppos)
return IS_ERR(sfp) ? PTR_ERR(sfp) : -ENXIO;
}
sdp = sfp->parentdp;
+ SG_LOG(3, sdp, "%s: read() count=%d\n", __func__, (int)count);
if (IS_ERR_OR_NULL(sdp))
return IS_ERR(sdp) ? PTR_ERR(sdp) : -ENXIO;
- SG_LOG(3, sdp, "%s: read() count=%d\n", __func__, (int)count);
if (!access_ok(VERIFY_WRITE, buf, count))
return -EFAULT;
@@ -812,11 +813,11 @@ sg_v3_write(struct sg_fd *sfp, struct file *file, const
char __user *buf,
return -ENOSYS;
if (hp->flags & SG_FLAG_MMAP_IO) {
if (!list_empty(>rq_list))
- return -EBUSY; /* already active requests on fd */
+ return -EBUSY; /* already active requests on fd */
if (hp->dxfer_len > sfp->reserve_srp->data.dlen)
- return -ENOMEM; /* MMAP_IO size must fit in reserve */
+ return -ENOMEM; /* MMAP_IO size must fit in reserve */
if (hp->flags & SG_FLAG_DIRECT_IO)
- return -EINVAL; /* not both MMAP_IO and DIRECT_IO */
+ return -EINVAL; /* not both MMAP_IO and DIRECT_IO */
}
sfp->cmd_q = true; /* when sg_io_hdr seen, set command queuing on */
ul_timeout = msecs_to_jiffies(hp->timeout);
@@
[PATCH 5/8] sg: add free list, rework locking
Remove fixed 16 sg_request object array and replace with an active
rq_list plus a request free list. Add finer grained spin lock to
sg_request and do a major rework on locking. sg_request objects now
are only de-allocated when the owning file descriptor is closed.
This simplifies locking issues.
Signed-off-by: Douglas Gilbert
---
This patch is big and complex. Towards the end the diff program
completely loses the plot. Better to use difftool on a two pane
window.
drivers/scsi/sg.c | 1225 +++--
1 file changed, 739 insertions(+), 486 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index c77701c0b939..45bad8159e51 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -139,44 +139,56 @@ struct sg_scatter_hold { /* holding area for scsi
scatter gather info */
struct sg_device; /* forward declarations */
struct sg_fd;
-struct sg_request {/* SG_MAX_QUEUE requests outstanding per file */
- struct list_head entry; /* list entry */
- struct sg_fd *parentfp; /* NULL -> not in use */
+/*
+ * For any file descriptor: at any time a sg_request object must be a member
+ * of sg_fd::rq_list or rq_free_list::rq_free_list. The only exception is
+ * within a rq_list_lock write lock when it is moving between those two lists.
+ */
+
+struct sg_request {/* active SCSI command or inactive on free list (fl) */
+ struct list_head rq_entry; /* member of rq_list (active cmd) */
+ struct list_head free_entry;/* member of rq_free_list */
+ spinlock_t rq_entry_lck;
struct sg_scatter_hold data;/* hold buffer, perhaps scatter list */
union {
struct sg_io_hdr header; /* see */
- struct sg_io_v4 hdr_v4; /* see */
+ struct sg_v4_hold v4_hold;/* related to */
};
- u8 sense_b[SCSI_SENSE_BUFFERSIZE];
- bool hdr_v4_active; /* selector for anonymous union above */
- bool res_used; /* true -> use reserve buffer, false -> don't */
+ struct execute_work ew;
+ ktime_t start_ts; /* used when sg_fd::time_in_ns is true */
+ bool v4_active; /* selector for autonomous union above */
bool orphan;/* true -> drop on sight, false -> normal */
- bool sg_io_owned; /* true -> packet belongs to SG_IO */
- /* done protected by rq_list_lock */
- char done; /* 0->before bh, 1->before read, 2->read */
+ bool sync_invoc;/* true -> synchronous (e.g. from ioctl(SG_IO)) */
+ u8 rq_state;/* one of 5 states, see SG_RQ_* defines */
+ u8 sense_b[SCSI_SENSE_BUFFERSIZE];
+ struct sg_fd *parentfp; /* pointer to owning fd, even when on fl */
+ struct sg_scatter_hold *d2p; /* optional 2nd data buffer for bidi */
struct request *rq;
struct bio *bio;
- struct execute_work ew;
};
-struct sg_fd { /* holds the state of a file descriptor */
- struct list_head sfd_siblings; /* protected by device's sfd_lock */
+struct sg_fd { /* holds the state of a file descriptor */
+ struct list_head sfd_entry; /* member sg_device::sfds list */
struct sg_device *parentdp; /* owning device */
wait_queue_head_t read_wait;/* queue read until command done */
- rwlock_t rq_list_lock; /* protect access to list in req_arr */
struct mutex f_mutex; /* protect against changes in this fd */
+ rwlock_t rq_list_lock; /* protect access to sg_request lists */
+ struct list_head rq_list; /* head of inflight sg_request list */
+ struct list_head rq_free_list; /* head of sg_request free list */
int timeout;/* defaults to SG_DEFAULT_TIMEOUT */
int timeout_user; /* defaults to SG_DEFAULT_TIMEOUT_USER */
- struct sg_scatter_hold reserve; /* one held for this file descriptor */
- struct list_head rq_list; /* head of request list */
- struct fasync_struct *async_qp; /* used by asynchronous notification */
- struct sg_request req_arr[SG_MAX_QUEUE];/* used as singly-linked list */
+ int rem_sgat_thresh;/* > this, request's sgat cleared after use */
+ u32 tot_fd_thresh; /* E2BIG if sum_of(dlen) > this, 0: ignore */
+ u32 sum_fd_dlens; /* when tot_fd_thresh>0 this is sum_of(dlen) */
bool force_packid; /* true -> pack_id input to read() */
bool cmd_q; /* true -> allow command queuing, false -> don't */
- u8 next_cmd_len;/* 0: automatic, >0: use on next write() */
bool keep_orphan;/* false -> drop (def), true -> keep for read() */
bool mmap_called; /* false -> mmap() never called on this fd */
- bool res_in_use;/* true -> 'reserve' array in use */
+ bool sse_seen; /* SG_SET_EXTENDED ioctl seen */
+ bool time_in_ns;/* report times in nanoseconds */
+ u8
[PATCH 6/8] sg: complete locking changes on ioctl+debug
Complete the locking and structure changes of ioctl and debug
('cat /proc/scsi/sg/debug') handling.
Signed-off-by: Douglas Gilbert
---
This was the code that was "#if 0'-ed out 2 patches ago. It
also shuts checkpatch.pl up as it doesn't like that technique
but offers no viable substitute.
drivers/scsi/sg.c | 204 +-
1 file changed, 130 insertions(+), 74 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 45bad8159e51..9b30d6667cc9 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1004,7 +1004,6 @@ srp_state_or_detaching(struct sg_device *sdp, struct
sg_request *srp)
return ret;
}
-#if 0 /* temporary to shorten big patch */
static long
sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
{
@@ -1036,24 +1035,37 @@ sg_ioctl(struct file *filp, unsigned int cmd_in,
unsigned long arg)
return -ENXIO;
if (!access_ok(VERIFY_WRITE, p, SZ_SG_IO_HDR))
return -EFAULT;
- result = sg_new_write(sfp, filp, p, SZ_SG_IO_HDR,
-1, read_only, 1, );
+ result = sg_v3_write(sfp, filp, p, SZ_SG_IO_HDR, read_only,
+true, );
if (result < 0)
return result;
result = wait_event_interruptible(sfp->read_wait,
- (srp_done(sfp, srp) || atomic_read(>detaching)));
- if (atomic_read(>detaching))
+ srp_state_or_detaching(sdp, srp));
+
+ spin_lock_irqsave(>rq_entry_lck, iflags);
+ if (unlikely(result)) { /* -ERESTARTSYS because signal hit */
+ srp->orphan = true;
+ srp->rq_state = SG_RQ_INFLIGHT;
+ spin_unlock_irqrestore(>rq_entry_lck, iflags);
+ SG_LOG(1, sdp, "%s: wait_event_interruptible-->%d\n",
+ __func__, result);
+ return result;
+ }
+ if (unlikely(atomic_read(>detaching))) {
+ srp->rq_state = SG_RQ_INACTIVE;
+ spin_unlock_irqrestore(>rq_entry_lck, iflags);
return -ENODEV;
- write_lock_irq(>rq_list_lock);
- if (srp->done) {
- srp->done = 2;
- write_unlock_irq(>rq_list_lock);
+ } else if (likely(srp->rq_state == SG_RQ_AWAIT_READ)) {
+ srp->rq_state = SG_RQ_DONE_READ;
+ spin_unlock_irqrestore(>rq_entry_lck, iflags);
result = sg_new_read(sfp, p, SZ_SG_IO_HDR, srp);
return (result < 0) ? result : 0;
}
- srp->orphan = true;
- write_unlock_irq(>rq_list_lock);
- return result; /* -ERESTARTSYS because signal hit process */
+ cp = sg_rq_state_str(srp->rq_state, true);
+ SG_LOG(1, sdp, "%s: unexpected srp=0x%p state: %s\n", __func__,
+ srp, cp);
+ spin_unlock_irqrestore(>rq_entry_lck, iflags);
+ return -EPROTO; /* Logic error */
case SG_SET_TIMEOUT:
result = get_user(val, ip);
if (result)
@@ -1112,25 +1124,36 @@ sg_ioctl(struct file *filp, unsigned int cmd_in,
unsigned long arg)
if (!access_ok(VERIFY_WRITE, ip, sizeof(int)))
return -EFAULT;
read_lock_irqsave(>rq_list_lock, iflags);
- list_for_each_entry(srp, >rq_list, entry) {
- if ((1 == srp->done) && (!srp->sg_io_owned)) {
- read_unlock_irqrestore(>rq_list_lock,
- iflags);
- __put_user(srp->header.pack_id, ip);
- return 0;
+ leave = false;
+ val = -1;
+ list_for_each_entry(srp, >rq_list, rq_entry) {
+ spin_lock(>rq_entry_lck);
+ if (srp->rq_state == SG_RQ_AWAIT_READ &&
+ !srp->sync_invoc) {
+ val = srp->header.pack_id;
+ leave = true;
}
+ spin_unlock(>rq_entry_lck);
+ if (leave)
+ break;
}
read_unlock_irqrestore(>rq_list_lock, iflags);
- __put_user(-1, ip);
+ __put_user(val, ip);
+ SG_LOG(3, sdp, "%s:SG_GET_PACK_ID=%d\n", __func__, val);
return 0;
case SG_GET_NUM_WAITING:
read_lock_irqsave(>rq_list_lock, iflags);
val = 0;
- list_for_each_entry(srp,
[PATCH 8/8] sg: user control for q_at_head or tail
Add a SG_SET_GET_EXTENDED ioctl control for whether commands
will be queued_at_head or queued_at_tail by the block layer
(together with the scsi mid-level). It has file scope.
Signed-off-by: Douglas Gilbert
---
The user can still override this setting on a per command
basis with the SG_FLAG_Q_AT_HEAD and SG_FLAG_Q_AT_TAIL
in the sg v3 and v4 structures.
drivers/scsi/sg.c | 35 +++
include/uapi/scsi/sg.h | 3 ++-
2 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 583846ebc5e0..258923aac50d 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -93,6 +93,10 @@ static int sg_proc_init(void);
#define SG_DEF_TIME_UNIT SG_TIME_UNIT_MS
#define SG_DEFAULT_TIMEOUT mult_frac(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
+#define SG_FD_Q_AT_TAIL true
+#define SG_FD_Q_AT_HEAD false
+#define SG_DEFAULT_Q_AT SG_FD_Q_AT_HEAD/* for backward compatibility */
+
int sg_big_buff = SG_DEF_RESERVED_SIZE;
/* N.B. This variable is readable and writeable via
/proc/scsi/sg/def_reserved_size . Each time sg_open() is called a buffer
@@ -185,6 +189,7 @@ struct sg_fd { /* holds the state of a file
descriptor */
bool keep_orphan;/* false -> drop (def), true -> keep for read() */
bool mmap_called; /* false -> mmap() never called on this fd */
bool time_in_ns;/* report times in nanoseconds */
+ bool q_at_tail; /* queue at tail if true, head when false */
u8 next_cmd_len;/* 0: automatic, >0: use on next write() */
struct sg_request *reserve_srp; /* allocate on open(), starts on fl */
struct fasync_struct *async_qp; /* used by asynchronous notification */
@@ -894,9 +899,13 @@ sg_common_write(struct sg_fd *sfp, const struct sg_io_hdr
*hi_p,
srp->start_ts = ktime_get_with_offset(TK_OFFS_BOOT);
else
hp->duration = jiffies_to_msecs(jiffies);
- /* at tail if v3 or later interface and tail flag set */
- at_head = !(hp->interface_id != '\0' &&
- (SG_FLAG_Q_AT_TAIL & hp->flags));
+
+ if (hp->interface_id == '\0') /* v1 and v2 interface */
+ at_head = true; /* backward compatibility */
+ else if (sfp->q_at_tail) /* cmd flags can override sfd setting */
+ at_head = (SG_FLAG_Q_AT_HEAD & hp->flags);
+ else/* this sfd is defaulting to head */
+ at_head = !(SG_FLAG_Q_AT_TAIL & hp->flags);
srp->rq->timeout = timeout;
kref_get(>f_ref); /* sg_rq_end_io() does kref_put(). */
@@ -1186,8 +1195,10 @@ sg_set_get_extended(struct sg_fd *sfp, void __user *p)
}
SG_LOG(3, sdp, "%s: wr_mask=0x%x rd_mask=0x%x\n", __func__,
seip->valid_wr_mask, seip->valid_rd_mask);
+ /* reserved_sz (u32), read-write */
if (or_masks & SG_SEIM_RESERVED_SIZE)
result = sg_reserved_sz(sfp, seip);
+ /* rq_rem_sgat_threshold (u32), read-write [impacts re-use only] */
if (or_masks & SG_SEIM_RQ_REM_THRESH) {
if (seip->valid_wr_mask & SG_SEIM_RQ_REM_THRESH) {
uv = seip->rq_rem_sgat_thresh;
@@ -1198,6 +1209,7 @@ sg_set_get_extended(struct sg_fd *sfp, void __user *p)
if (seip->valid_rd_mask & SG_SEIM_RQ_REM_THRESH)
seip->rq_rem_sgat_thresh = sfp->rem_sgat_thresh;
}
+ /* tot_fd_thresh (u32), read-write [sum of active cmd dlen_s] */
if (or_masks & SG_SEIM_TOT_FD_THRESH) {
if (seip->valid_wr_mask & SG_SEIM_TOT_FD_THRESH) {
uv = seip->tot_fd_thresh;
@@ -1208,8 +1220,9 @@ sg_set_get_extended(struct sg_fd *sfp, void __user *p)
if (seip->valid_rd_mask & SG_SEIM_TOT_FD_THRESH)
seip->tot_fd_thresh = sfp->tot_fd_thresh;
}
+ /* check all boolean flags if either wr or rd mask set in or_mask */
if (or_masks & SG_SEIM_CTL_FLAGS) {
- /* don't care whether wr or rd mask set in or_mask */
+ /* TIME_IN_NS boolean, read-write */
if (seip->ctl_flags_wr_mask & SG_CTL_FLAGM_TIME_IN_NS)
sfp->time_in_ns =
!!(seip->ctl_flags & SG_CTL_FLAGM_TIME_IN_NS);
@@ -1219,19 +1232,32 @@ sg_set_get_extended(struct sg_fd *sfp, void __user *p)
else
seip->ctl_flags &= ~SG_CTL_FLAGM_TIME_IN_NS;
}
+ /* ORPHANS boolean, read-only */
if (seip->ctl_flags_rd_mask & SG_CTL_FLAGM_ORPHANS) {
if (sg_any_persistent_orphans(sfp))
seip->ctl_flags |= SG_CTL_FLAGM_ORPHANS;
else
seip->ctl_flags &= ~SG_CTL_FLAGM_ORPHANS;
}
+ /* OTHER_OPENS boolean, read-only */
[PATCH 1/8] sg: types and naming cleanup
Remove typedefs and use better type names like bool and u8 where
appropriate. Rename some variables and functions for clarity.
Adjust formatting (e.g. function definitions) to be more
consistent across the driver.
Signed-off-by: Douglas Gilbert
---
The intention is to move to sg_version_num 40001 when a second
patchset implementing SG_IOSUBMIT and friends is applied. In
the meantime, move from the latest version number in the kernel
(30536) to 30901 to indicate a significant change but not yet
implementing the sg v4 interface.
drivers/scsi/sg.c | 827 ++
1 file changed, 467 insertions(+), 360 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index c6ad00703c5b..7e723f37a269 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -7,7 +7,7 @@
* Original driver (sg.c):
*Copyright (C) 1992 Lawrence Foard
* Version 2 and 3 extensions to driver:
- *Copyright (C) 1998 - 2014 Douglas Gilbert
+ *Copyright (C) 1998 - 2018 Douglas Gilbert
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -16,16 +16,9 @@
*
*/
-static int sg_version_num = 30536; /* 2 digits for each component */
-#define SG_VERSION_STR "3.5.36"
+static int sg_version_num = 30901; /* 2 digits for each component */
+#define SG_VERSION_STR "3.9.01"
-/*
- * D. P. Gilbert (dgilb...@interlog.com), notes:
- * - scsi logging is available via SCSI_LOG_TIMEOUT macros. First
- *the kernel/module needs to be built with CONFIG_SCSI_LOGGING
- *(otherwise the macros compile to empty statements).
- *
- */
#include
#include
@@ -52,6 +45,8 @@ static int sg_version_num = 30536;/* 2 digits for each
component */
#include
#include
#include /* for sg_check_file_access() */
+#include
+#include
#include "scsi.h"
#include
@@ -64,7 +59,7 @@ static int sg_version_num = 30536;/* 2 digits for each
component */
#ifdef CONFIG_SCSI_PROC_FS
#include
-static char *sg_version_date = "20140603";
+static char *sg_version_date = "20181018";
static int sg_proc_init(void);
#endif
@@ -73,7 +68,8 @@ static int sg_proc_init(void);
#define SG_MAX_DEVS 32768
-/* SG_MAX_CDB_SIZE should be 260 (spc4r37 section 3.1.30) however the type
+/*
+ * SG_MAX_CDB_SIZE should be 260 (spc4r37 section 3.1.30) however the type
* of sg_io_hdr::cmd_len can only represent 255. All SCSI commands greater
* than 16 bytes are "variable length" whose length is a multiple of 4
*/
@@ -100,44 +96,54 @@ static int sg_add_device(struct device *, struct
class_interface *);
static void sg_remove_device(struct device *, struct class_interface *);
static DEFINE_IDR(sg_index_idr);
-static DEFINE_RWLOCK(sg_index_lock); /* Also used to lock
- file descriptor list
for device */
+static DEFINE_RWLOCK(sg_index_lock); /* Also used to lock fd list for device */
static struct class_interface sg_interface = {
.add_dev= sg_add_device,
.remove_dev = sg_remove_device,
};
-typedef struct sg_scatter_hold { /* holding area for scsi scatter gather info
*/
- unsigned short k_use_sg; /* Count of kernel scatter-gather pieces */
- unsigned sglist_len; /* size of malloc'd scatter-gather list ++ */
- unsigned bufflen; /* Size of (aggregate) data buffer */
- struct page **pages;
- int page_order;
- char dio_in_use;/* 0->indirect IO (or mmap), 1->dio */
- unsigned char cmd_opcode; /* first byte of command */
-} Sg_scatter_hold;
+struct sg_v4_hold {/* parts of sg_io_v4 object needed in async usage */
+ __user u8 *usr_ptr; /* derived from sg_io_v4::usr_ptr */
+ __user u8 *sbp; /* derived from sg_io_v4::response */
+ u16 cmd_len;/* truncated of sg_io_v4::request_len */
+ u16 max_sb_len; /* truncated of sg_io_v4::max_response_len */
+ u32 flags; /* copy of sg_io_v4::flags */
+};
+
+struct sg_scatter_hold { /* holding area for scsi scatter gather info */
+ struct page **pages;/* num_sgat element array of struct page* */
+ int page_order; /* byte_len = (page_size * (2**page_order)) */
+ int dlen; /* Byte length of data buffer */
+ unsigned short num_sgat;/* actual number of scatter-gather segments */
+ bool dio_in_use;/* false->indirect IO (or mmap), true->dio */
+ u8 cmd_opcode; /* first byte of command */
+};
struct sg_device; /* forward declarations */
struct sg_fd;
-typedef struct sg_request {/* SG_MAX_QUEUE requests outstanding per file */
+struct sg_request {/* SG_MAX_QUEUE requests outstanding per file */
struct list_head entry; /* list entry */
struct sg_fd *parentfp; /* NULL -> not in use */
- Sg_scatter_hold
[PATCH 4/8] sg: expand request states
Introduce the new rq_state defines. SG_RQ_DATA_THRESHOLD is a
default value that if the data length of a request exceeds
then, after that request is completed, the data buffer will
be freed up as the sg_request object is placed on the free
list. SG_TOT_FD_THRESHOLD is a default, per file descriptor
value that the sum of outstanding command data lengths is not
allowed to exceed.
Signed-off-by: Douglas Gilbert
---
The '#if 0's are temporary and removed in a later patch.
They allow the following large and complex patch to be
a bit shorter and still compile.
drivers/scsi/sg.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 71623685abfe..c77701c0b939 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -75,6 +75,22 @@ static int sg_proc_init(void);
*/
#define SG_MAX_CDB_SIZE 252
+/* Following defines are states of sg_request::rq_state */
+#define SG_RQ_INACTIVE 0/* request not in use (e.g. on fl) */
+#define SG_RQ_INFLIGHT 1/* SCSI request issued, no response yet */
+#define SG_RQ_AWAIT_READ 2 /* response received, awaiting read */
+#define SG_RQ_DONE_READ 3 /* read is ongoing or done */
+#define SG_RQ_BUSY 4/* example: reserve request changing size */
+
+/* free up requests larger than this dlen size after use */
+#define SG_RQ_DATA_THRESHOLD (128 * 1024)
+
+/* If sum_of(dlen) of a fd exceeds this, write() will yield E2BIG */
+#define SG_TOT_FD_THRESHOLD (16 * 1024 * 1024)
+
+#define SG_TIME_UNIT_MS 0 /* milliseconds */
+#define SG_TIME_UNIT_NS 1 /* nanoseconds */
+#define SG_DEF_TIME_UNIT SG_TIME_UNIT_MS
#define SG_DEFAULT_TIMEOUT mult_frac(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
int sg_big_buff = SG_DEF_RESERVED_SIZE;
@@ -950,6 +966,7 @@ sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info
*rinfo)
}
}
+#if 0 /* temporary to shorten big patch */
static long
sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
{
@@ -1227,6 +1244,7 @@ sg_compat_ioctl(struct file *filp, unsigned int cmd_in,
unsigned long arg)
return -ENOIOCTLCMD;
}
#endif
+#endif /* temporary to shorten big patch */
static __poll_t
sg_poll(struct file *filp, poll_table * wait)
@@ -1496,10 +1514,12 @@ static const struct file_operations sg_fops = {
.read = sg_read,
.write = sg_write,
.poll = sg_poll,
+#if 0 /* temporary to shorten big patch */
.unlocked_ioctl = sg_ioctl,
#ifdef CONFIG_COMPAT
.compat_ioctl = sg_compat_ioctl,
#endif
+#endif /* temporary to shorten big patch */
.open = sg_open,
.mmap = sg_mmap,
.release = sg_release,
@@ -2422,12 +2442,16 @@ static const struct seq_operations devstrs_seq_ops = {
.show = sg_proc_seq_show_devstrs,
};
+#if 0 /* temporary to shorten big patch */
static int sg_proc_seq_show_debug(struct seq_file *s, void *v);
+#endif /* temporary to shorten big patch */
static const struct seq_operations debug_seq_ops = {
.start = dev_seq_start,
.next = dev_seq_next,
.stop = dev_seq_stop,
+#if 0 /* temporary to shorten big patch */
.show = sg_proc_seq_show_debug,
+#endif /* temporary to shorten big patch */
};
static int
@@ -2601,6 +2625,8 @@ sg_proc_seq_show_devstrs(struct seq_file *s, void *v)
return 0;
}
+#if 0 /* temporary to shorten big patch */
+
/* must be called while holding sg_index_lock */
static void
sg_proc_debug_helper(struct seq_file *s, struct sg_device *sdp)
@@ -2704,6 +2730,7 @@ sg_proc_seq_show_debug(struct seq_file *s, void *v)
read_unlock_irqrestore(_index_lock, iflags);
return 0;
}
+#endif /* temporary to shorten big patch */
#endif /* CONFIG_SCSI_PROC_FS */
--
2.17.1
[PATCH 0/8] sg: major cleanup, remove max_queue limit
The intention is to add two new ioctls as proposed by Linus Torvalds: SG_IOSUBMIT and SG_IORECEIVE to replace the write()/read() async interface. But first, clean up the driver and remove the SG_MAX_QUEUE limit of no more than 16 queued commands on a file descriptor at a time. A free list has been added and the de-allocation of sg_request objects is deferred until the close() of a file. Locking is extensively reworked, especially at the struct sg_fd and sg_request level. A new SG_SET_GET_EXTENDED ioctl has been added. It allows multiple integer values and booleans to be written to and/or read from the driver. An example is changing and/or reading the reserved request data length (there is one of these per fd). An example of a new feature is changing and/or reading the per-fd upper limit on the sum of outstanding data buffer sizes (default is 16 MB). An example of a boolean is a bit to do all following command timekeeping in nanoseconds rather that the default millseconds. A later patchset will add implementations for the SG_IOSUBMIT and SG_IORECEIVE plus handling of the sg v4 interface with the existing SG_IO ioctl. This patchset is against Martin Petersen 4.20/scsi-queue branch. Douglas Gilbert (8): sg: types and naming cleanup sg: introduce sg_log macro sg: split header, expand and correct descriptions sg: expand request states sg: add free list, rework locking sg: complete locking changes on ioctl+debug sg: rework ioctl handling sg: user control for q_at_head or tail drivers/scsi/sg.c | 2484 ++-- include/scsi/sg.h | 252 +--- include/uapi/scsi/sg.h | 416 +++ 3 files changed, 2036 insertions(+), 1116 deletions(-) create mode 100644 include/uapi/scsi/sg.h -- 2.17.1
[PATCH 2/8] sg: introduce sg_log macro
Introduce the SG_LOG macro to replace long-winded
'SCSI_LOG_TIMEOUT(3, sg_printk ...' debug messages. Use __func__
wherever appropriate to make the debug messages more portable.
Signed-off-by: Douglas Gilbert
---
drivers/scsi/sg.c | 162 +-
1 file changed, 72 insertions(+), 90 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 7e723f37a269..71623685abfe 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -216,9 +216,24 @@ static void sg_device_destroy(struct kref *kref);
/* #define SZ_SG_IOVEC sizeof(struct sg_iovec) synonym for 'struct iovec' */
#define SZ_SG_REQ_INFO sizeof(struct sg_req_info)
-#define sg_printk(prefix, sdp, fmt, a...) \
- sdev_prefix_printk(prefix, (sdp)->device, \
- (sdp)->disk->disk_name, fmt, ##a)
+/*
+ * Kernel needs to be built with CONFIG_SCSI_LOGGING to see log messages.
+ * 'depth' is a number between 1 (most severe) and 7 (most noisy, most
+ * information). All messages are logged as informational (KERN_INFO). In
+ * the unexpected situation where sdp is NULL the macro reverts to a pr_info
+ * and ignores CONFIG_SCSI_LOGGING and always prints to the log.
+ */
+#define SG_LOG(depth, sdp, fmt, a...) \
+ do {\
+ if (IS_ERR_OR_NULL(sdp)) { \
+ pr_info("sg: sdp=NULL_or_ERR, " fmt, ##a); \
+ } else {\
+ SCSI_LOG_TIMEOUT(depth, sdev_prefix_printk( \
+KERN_INFO, (sdp)->device, \
+(sdp)->disk->disk_name, fmt, \
+##a)); \
+ } \
+ } while (0)
/*
* The SCSI interfaces that use read() and write() as an asynchronous variant
of
@@ -316,9 +331,8 @@ sg_open(struct inode *inode, struct file *filp)
sdp = sg_get_dev(min_dev);
if (IS_ERR(sdp))
return PTR_ERR(sdp);
-
- SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sdp,
- "sg_open: flags=0x%x\n", flags));
+ SG_LOG(3, sdp, "%s: flags=0x%x; device open count prior=%d\n",
+ __func__, flags, sdp->open_cnt);
/* This driver's module count bumped by fops_get in */
/* Prevent the device driver from vanishing while we sleep */
@@ -414,9 +428,10 @@ sg_release(struct inode *inode, struct file *filp)
return IS_ERR(sfp) ? PTR_ERR(sfp) : -ENXIO;
}
sdp = sfp->parentdp;
- SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sdp, "sg_release\n"));
if (IS_ERR_OR_NULL(sdp))
return IS_ERR(sdp) ? PTR_ERR(sdp) : -ENXIO;
+ SG_LOG(3, sdp, "%s: device open count prior=%d\n", __func__,
+ sdp->open_cnt);
mutex_lock(>open_rel_lock);
scsi_autopm_put_device(sdp->device);
@@ -462,8 +477,7 @@ sg_read(struct file *filp, char __user *buf, size_t count,
loff_t * ppos)
sdp = sfp->parentdp;
if (IS_ERR_OR_NULL(sdp))
return IS_ERR(sdp) ? PTR_ERR(sdp) : -ENXIO;
- SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sdp, "%s: count=%d\n",
-__func__, (int)count));
+ SG_LOG(3, sdp, "%s: read() count=%d\n", __func__, (int)count);
if (!access_ok(VERIFY_WRITE, buf, count))
return -EFAULT;
@@ -663,10 +677,9 @@ sg_write(struct file *filp, const char __user *buf, size_t
count, loff_t * ppos)
return IS_ERR(sfp) ? PTR_ERR(sfp) : -ENXIO;
}
sdp = sfp->parentdp;
+ SG_LOG(3, sdp, "%s: write(3rd arg) count=%d\n", __func__, (int)count);
if (IS_ERR_OR_NULL(sdp))
return IS_ERR(sdp) ? PTR_ERR(sdp) : -ENXIO;
- SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sdp, "%s: count=%d\n",
-__func__, (int)count));
if (atomic_read(>detaching))
return -ENODEV;
if (!((filp->f_flags & O_NONBLOCK) ||
@@ -687,8 +700,7 @@ sg_write(struct file *filp, const char __user *buf, size_t
count, loff_t * ppos)
return -EIO;/* minimum scsi command length is 6 bytes */
if (!(srp = sg_add_request(sfp))) {
- SCSI_LOG_TIMEOUT(1, sg_printk(KERN_INFO, sdp,
- "sg_write: queue full\n"));
+ SG_LOG(1, sdp, "%s: queue full\n", __func__);
return -EDOM;
}
buf += SZ_SG_HEADER;
@@ -703,9 +715,8 @@ sg_write(struct file *filp, const char __user *buf, size_t
count, loff_t * ppos)
cmd_size = 12;
}
mutex_unlock(>f_mutex);
- SCSI_LOG_TIMEOUT(4, sg_printk(KERN_INFO, sdp,
- "%s: scsi
