libata: revert "libata: use blk taging" et al.

PATCH In review Tony Battersby 5 participants 12 messages Mar 11, 2015 – Mar 19, 2015

Subsystems (4)

SCSI libata SCSI libsas IBM Power Linux RAID adapter

lore

Share & tools

lore

Tony Battersby

author

2015-03-11 18:15 UTC #0 lore raw

PATCH 1/1 libata: revert "libata: use blk taging" et al.

This reverts commits 12cb5ce101abfaf74421f8cc9f196e708209eb79 and 98bd4be1ba95f2fe7f543910792b7163a5de06eb.

Commit 12cb5ce101ab ("libata: use blk taging") causes the following oops with scsi-mq enabled:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000058
IP: [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
PGD 32adf0067 PUD 32adf1067 PMD 0 
Oops: 0002 [#1] SMP DEBUG_PAGEALLOC 
Modules linked in: iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi igb
i2c_algo_bit ptp pps_core pm80xx libsas scsi_transport_sas sg coretemp
eeprom w83795 i2c_i801
CPU: 4 PID: 1450 Comm: cydiskbench Not tainted 4.0.0-rc3 #1
Hardware name: Supermicro X8DTH-i/6/iF/6F/X8DTH, BIOS 2.1b       05/04/12  
task: ffff8800ba86d500 ti: ffff88032a064000 task.ti: ffff88032a064000
RIP: 0010:[<ffffffff804fd46e>]  [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
RSP: 0018:ffff88032a067858  EFLAGS: 00010046
RAX: 0000000000000000 RBX: ffff8800ba0d2230 RCX: 000000000000002a
RDX: ffffffff80505ae0 RSI: 0000000000000020 RDI: ffff8800ba0d2230
RBP: ffff88032a067868 R08: 0000000000000201 R09: 0000000000000001
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8800ba0d0000
R13: ffff8800ba0d2230 R14: ffffffff80505ae0 R15: ffff8800ba0d0000
FS:  0000000041223950(0063) GS:ffff88033e480000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000058 CR3: 000000032a0a3000 CR4: 00000000000006e0
Stack:
 ffff880329eee758 ffff880329eee758 ffff88032a0678a8 ffffffff80502dad
 ffff8800ba167978 ffff880329eee758 ffff88032bf9c520 ffff8800ba167978
 ffff88032bf9c520 ffff88032bf9a290 ffff88032a0678b8 ffffffff80506909
Call Trace:
 [<ffffffff80502dad>] ata_scsi_translate+0x3d/0x1b0
 [<ffffffff80506909>] ata_sas_queuecmd+0x149/0x2a0
 [<ffffffffa0046650>] sas_queuecommand+0xa0/0x1f0 [libsas]
 [<ffffffff804ea544>] scsi_dispatch_cmd+0xd4/0x1a0
 [<ffffffff804eb50f>] scsi_queue_rq+0x66f/0x7f0
 [<ffffffff803e5098>] __blk_mq_run_hw_queue+0x208/0x3f0
 [<ffffffff803e54b8>] blk_mq_run_hw_queue+0x88/0xc0
 [<ffffffff803e5c74>] blk_mq_insert_request+0xc4/0x130
 [<ffffffff803e0b63>] blk_execute_rq_nowait+0x73/0x160
 [<ffffffffa0023fca>] sg_common_write+0x3da/0x720 [sg]
 [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
 [<ffffffffa0025100>] sg_new_write+0x250/0x360 [sg]
 [<ffffffff802a236c>] ? __lock_acquire+0x50c/0xc10
 [<ffffffff802a2b87>] ? lock_release_non_nested+0xa7/0x360
 [<ffffffff8068954b>] ? _raw_spin_unlock_irqrestore+0x3b/0x60
 [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
 [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
 [<ffffffffa0025feb>] sg_write+0x13b/0x450 [sg]
 [<ffffffff802a236c>] ? __lock_acquire+0x50c/0xc10
 [<ffffffff802cb1e9>] ? do_futex+0x109/0xbf0
 [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
 [<ffffffff8032ec91>] vfs_write+0xd1/0x1b0
 [<ffffffff8032ee54>] SyS_write+0x54/0xc0
 [<ffffffff80689932>] system_call_fastpath+0x12/0x17
Code: 24 20 04 0f 85 ec 00 00 00 49 83 3c 24 00 0f 84 cf 00 00 00 83 fe 1f

0f 87 dc 00 00 00 89 f0 48 69 c0 f0 00 00 00 49 8d 44 04 40 <89> 70 58 48 c7 40 10 00 00 00 00 4c 89 20 48 89 58 08 c7 40 64
RIP [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
RSP <ffff88032a067858>
CR2: 0000000000000058

---[ end trace 43f5eefb64627eff ]---

scsi-mq uses a host-wide tag map shared among all devices with some integer tag values >= ATA_MAX_QUEUE. These unexpectedly high tag values cause __ata_qc_from_tag() to return NULL, which is then dereferenced in ata_qc_new_init(), causing the oops above.

Due to conflicts, it is also necessary to revert commit 98bd4be1ba95 ("libata: move sas ata tag allocation to libata-scsi.c"), which appears to have been a follow-on cleanup to the commit that caused the problem.

Fixes 12cb5ce101ab ("libata: use blk taging")

Signed-off-by Tony Battersby <[email protected]>

---

Note: when reverting the two commits above, I had to fixup conflicts with the following commit:
72dd299d5039a336493993dcc63413cf31d0e662 ("libata: allow sata_sil24 to opt-out of tag ordered submission")

If anyone else can suggest a fix that does not involve reverting the commits, then I would be happy to test.

The oops output was generated using the SCSI generic driver (sg) to send commands to SATA disks connected to a pm80xx HBA.

Here is the code relevant to the oops:

enum {
	ATA_MAX_QUEUE = 32,
};

static inline unsigned int ata_tag_valid(unsigned int tag)

{
	return (tag < ATA_MAX_QUEUE) ? 1 : 0;
}

static inline struct ata_queued_cmd *__ata_qc_from_tag(struct ata_port *ap,
unsigned int tag)

{
	if (likely(ata_tag_valid(tag)))
		return &ap->qcmd[tag];
	return NULL;
}

struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)

{
	...
	qc = __ata_qc_from_tag(ap, tag); /* tag >= ATA_MAX_QUEUE */
	qc->tag = tag; /* qc is NULL, OOPS HERE */
	...
}

drivers/ata/libata-core.c +52 ~~-18~~

diff -urpN linux-4.0.0-rc3-a/drivers/ata/libata-core.c linux-4.0.0-rc3-b/drivers/ata/libata-core.c
@@ -1585,6 +1585,8 @@ unsigned ata_exec_internal_sg(struct ata
 	else
 		tag = 0;
 
+	if (test_and_set_bit(tag, &ap->qc_allocated))
+		BUG();
 	qc = __ata_qc_from_tag(ap, tag);
 
 	qc->tag = tag;

@@ -4720,36 +4722,69 @@ void swap_buf_le16(u16 *buf, unsigned in
 }
 
 /**
- *	ata_qc_new_init - Request an available ATA command, and initialize it
- *	@dev: Device from whom we request an available command structure
+ *	ata_qc_new - Request an available ATA command, for queueing
+ *	@ap: target port
+ *
+ *	Some ATA host controllers may implement a queue depth which is less
+ *	than ATA_MAX_QUEUE. So we shouldn't allocate a tag which is beyond
+ *	the hardware limitation.
  *
  *	LOCKING:
  *	None.
  */
 
-struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
+static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap)
 {
-	struct ata_port *ap = dev->link->ap;
-	struct ata_queued_cmd *qc;
+	struct ata_queued_cmd *qc = NULL;
+	unsigned int max_queue = ap->host->n_tags;
+	unsigned int i, tag;
 
 	/* no command while frozen */
 	if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
 		return NULL;
 
-	/* libsas case */
-	if (!ap->scsi_host) {
-		tag = ata_sas_allocate_tag(ap);
-		if (tag < 0)
-			return NULL;
+	for (i = 0, tag = ap->last_tag + 1; i < max_queue; i++, tag++) {
+		if (ap->flags & ATA_FLAG_LOWTAG)
+			tag = i;
+		else
+			tag = tag < max_queue ? tag : 0;
+
+		/* the last tag is reserved for internal command. */
+		if (tag == ATA_TAG_INTERNAL)
+			continue;
+
+		if (!test_and_set_bit(tag, &ap->qc_allocated)) {
+			qc = __ata_qc_from_tag(ap, tag);
+			qc->tag = tag;
+			ap->last_tag = tag;
+			break;
+		}
 	}
 
-	qc = __ata_qc_from_tag(ap, tag);
-	qc->tag = tag;
-	qc->scsicmd = NULL;
-	qc->ap = ap;
-	qc->dev = dev;
+	return qc;
+}
 
-	ata_qc_reinit(qc);
+/**
+ *	ata_qc_new_init - Request an available ATA command, and initialize it
+ *	@dev: Device from whom we request an available command structure
+ *
+ *	LOCKING:
+ *	None.
+ */
+
+struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev)
+{
+	struct ata_port *ap = dev->link->ap;
+	struct ata_queued_cmd *qc;
+
+	qc = ata_qc_new(ap);
+	if (qc) {
+		qc->scsicmd = NULL;
+		qc->ap = ap;
+		qc->dev = dev;
+
+		ata_qc_reinit(qc);
+	}
 
 	return qc;
 }

@@ -4776,8 +4811,7 @@ void ata_qc_free(struct ata_queued_cmd *
 	tag = qc->tag;
 	if (likely(ata_tag_valid(tag))) {
 		qc->tag = ATA_TAG_POISON;
-		if (!ap->scsi_host)
-			ata_sas_free_tag(tag, ap);
+		clear_bit(tag, &ap->qc_allocated);
 	}
 }

drivers/ata/libata.h +1 -3

diff -urpN linux-4.0.0-rc3-a/drivers/ata/libata.h linux-4.0.0-rc3-b/drivers/ata/libata.h
@@ -63,7 +63,7 @@ extern struct ata_link *ata_dev_phys_lin
 extern void ata_force_cbl(struct ata_port *ap);
 extern u64 ata_tf_to_lba(const struct ata_taskfile *tf);
 extern u64 ata_tf_to_lba48(const struct ata_taskfile *tf);
-extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag);
+extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev);
 extern int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
 			   u64 block, u32 n_block, unsigned int tf_flags,
 			   unsigned int tag);

@@ -144,8 +144,6 @@ extern void ata_scsi_dev_rescan(struct w
 extern int ata_bus_probe(struct ata_port *ap);
 extern int ata_scsi_user_scan(struct Scsi_Host *shost, unsigned int channel,
 			      unsigned int id, u64 lun);
-int ata_sas_allocate_tag(struct ata_port *ap);
-void ata_sas_free_tag(unsigned int tag, struct ata_port *ap);
 
 
 /* libata-eh.c */

drivers/ata/libata-scsi.c +1 ~~-32~~

diff -urpN linux-4.0.0-rc3-a/drivers/ata/libata-scsi.c linux-4.0.0-rc3-b/drivers/ata/libata-scsi.c
@@ -756,7 +756,7 @@ static struct ata_queued_cmd *ata_scsi_q
 {
 	struct ata_queued_cmd *qc;
 
-	qc = ata_qc_new_init(dev, cmd->request->tag);
+	qc = ata_qc_new_init(dev);
 	if (qc) {
 		qc->scsicmd = cmd;
 		qc->scsidone = cmd->scsi_done;

@@ -3668,9 +3668,6 @@ int ata_scsi_add_hosts(struct ata_host *
 		 */
 		shost->max_host_blocked = 1;
 
-		if (scsi_init_shared_tag_map(shost, host->n_tags))
-			goto err_add;
-
 		rc = scsi_add_host_with_dma(ap->scsi_host,
 						&ap->tdev, ap->host->dev);
 		if (rc)

@@ -4233,31 +4230,3 @@ int ata_sas_queuecmd(struct scsi_cmnd *c
 	return rc;
 }
 EXPORT_SYMBOL_GPL(ata_sas_queuecmd);
-
-int ata_sas_allocate_tag(struct ata_port *ap)
-{
-	unsigned int max_queue = ap->host->n_tags;
-	unsigned int i, tag;
-
-	for (i = 0, tag = ap->sas_last_tag + 1; i < max_queue; i++, tag++) {
-		if (ap->flags & ATA_FLAG_LOWTAG)
-			tag = 1;
-		else
-			tag = tag < max_queue ? tag : 0;
-
-		/* the last tag is reserved for internal command. */
-		if (tag == ATA_TAG_INTERNAL)
-			continue;
-
-		if (!test_and_set_bit(tag, &ap->sas_tag_allocated)) {
-			ap->sas_last_tag = tag;
-			return tag;
-		}
-	}
-	return -1;
-}
-
-void ata_sas_free_tag(unsigned int tag, struct ata_port *ap)
-{
-	clear_bit(tag, &ap->sas_tag_allocated);
-}

include/linux/libata.h +2 -3

diff -urpN linux-4.0.0-rc3-a/include/linux/libata.h linux-4.0.0-rc3-b/include/linux/libata.h
@@ -823,10 +823,10 @@ struct ata_port {
 	unsigned int		cbl;	/* cable type; ATA_CBL_xxx */
 
 	struct ata_queued_cmd	qcmd[ATA_MAX_QUEUE];
-	unsigned long		sas_tag_allocated; /* for sas tag allocation only */
+	unsigned long		qc_allocated;
 	unsigned int		qc_active;
 	int			nr_active_links; /* #links with active qcs */
-	unsigned int		sas_last_tag;	/* track next tag hw expects */
+	unsigned int		last_tag;	/* track next tag hw expects */
 
 	struct ata_link		link;		/* host default link */
 	struct ata_link		*slave_link;	/* see ata_slave_link_init() */

@@ -1352,7 +1352,6 @@ extern struct device_attribute *ata_comm
 	.ioctl			= ata_scsi_ioctl,		\
 	.queuecommand		= ata_scsi_queuecmd,		\
 	.can_queue		= ATA_DEF_QUEUE,		\
-	.tag_alloc_policy	= BLK_TAG_ALLOC_RR,		\
 	.this_id		= ATA_SHT_THIS_ID,		\
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,		\
 	.emulated		= ATA_SHT_EMULATED,		\

Jens Axboe

2015-03-11 21:46 UTC #1 lore raw

Quoted 61 lines · Tony Battersby

This reverts commits 12cb5ce101abfaf74421f8cc9f196e708209eb79 and 98bd4be1ba95f2fe7f543910792b7163a5de06eb.

Commit 12cb5ce101ab ("libata: use blk taging") causes the following oops with scsi-mq enabled:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000058
IP: [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
PGD 32adf0067 PUD 32adf1067 PMD 0
Oops: 0002 [#1] SMP DEBUG_PAGEALLOC
Modules linked in: iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi igb
i2c_algo_bit ptp pps_core pm80xx libsas scsi_transport_sas sg coretemp
eeprom w83795 i2c_i801
CPU: 4 PID: 1450 Comm: cydiskbench Not tainted 4.0.0-rc3 #1
Hardware name: Supermicro X8DTH-i/6/iF/6F/X8DTH, BIOS 2.1b       05/04/12
task: ffff8800ba86d500 ti: ffff88032a064000 task.ti: ffff88032a064000
RIP: 0010:[<ffffffff804fd46e>]  [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
RSP: 0018:ffff88032a067858  EFLAGS: 00010046
RAX: 0000000000000000 RBX: ffff8800ba0d2230 RCX: 000000000000002a
RDX: ffffffff80505ae0 RSI: 0000000000000020 RDI: ffff8800ba0d2230
RBP: ffff88032a067868 R08: 0000000000000201 R09: 0000000000000001
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8800ba0d0000
R13: ffff8800ba0d2230 R14: ffffffff80505ae0 R15: ffff8800ba0d0000
FS:  0000000041223950(0063) GS:ffff88033e480000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000058 CR3: 000000032a0a3000 CR4: 00000000000006e0
Stack:
  ffff880329eee758 ffff880329eee758 ffff88032a0678a8 ffffffff80502dad
  ffff8800ba167978 ffff880329eee758 ffff88032bf9c520 ffff8800ba167978
  ffff88032bf9c520 ffff88032bf9a290 ffff88032a0678b8 ffffffff80506909
Call Trace:
  [<ffffffff80502dad>] ata_scsi_translate+0x3d/0x1b0
  [<ffffffff80506909>] ata_sas_queuecmd+0x149/0x2a0
  [<ffffffffa0046650>] sas_queuecommand+0xa0/0x1f0 [libsas]
  [<ffffffff804ea544>] scsi_dispatch_cmd+0xd4/0x1a0
  [<ffffffff804eb50f>] scsi_queue_rq+0x66f/0x7f0
  [<ffffffff803e5098>] __blk_mq_run_hw_queue+0x208/0x3f0
  [<ffffffff803e54b8>] blk_mq_run_hw_queue+0x88/0xc0
  [<ffffffff803e5c74>] blk_mq_insert_request+0xc4/0x130
  [<ffffffff803e0b63>] blk_execute_rq_nowait+0x73/0x160
  [<ffffffffa0023fca>] sg_common_write+0x3da/0x720 [sg]
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffffa0025100>] sg_new_write+0x250/0x360 [sg]
  [<ffffffff802a236c>] ? __lock_acquire+0x50c/0xc10
  [<ffffffff802a2b87>] ? lock_release_non_nested+0xa7/0x360
  [<ffffffff8068954b>] ? _raw_spin_unlock_irqrestore+0x3b/0x60
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffffa0025feb>] sg_write+0x13b/0x450 [sg]
  [<ffffffff802a236c>] ? __lock_acquire+0x50c/0xc10
  [<ffffffff802cb1e9>] ? do_futex+0x109/0xbf0
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffff8032ec91>] vfs_write+0xd1/0x1b0
  [<ffffffff8032ee54>] SyS_write+0x54/0xc0
  [<ffffffff80689932>] system_call_fastpath+0x12/0x17
Code: 24 20 04 0f 85 ec 00 00 00 49 83 3c 24 00 0f 84 cf 00 00 00 83 fe 1f

Quoted 7 lines

---[ end trace 43f5eefb64627eff ]---
scsi-mq uses a host-wide tag map shared among all devices with some integer tag values >= ATA_MAX_QUEUE. These unexpectedly high tag values cause __ata_qc_from_tag() to return NULL, which is then dereferenced in ata_qc_new_init(), causing the oops above.

Wait, something is missing here. We should not be getting tag values that are >= ATA_MAX_QUEUE. Instead of reverting this, we need to figure out why this is happening, and fix it. That is correct way forward here. What setup is this being reproduced on?

Signature · 1 line

Jens Axboe

Tony Battersby

author

2015-03-11 22:19 UTC #2 lore raw

Quoted 65 lines · Jens Axboe

Quoted 64 lines · Tony Battersby

This reverts commits 12cb5ce101abfaf74421f8cc9f196e708209eb79 and 98bd4be1ba95f2fe7f543910792b7163a5de06eb.

Commit 12cb5ce101ab ("libata: use blk taging") causes the following oops with scsi-mq enabled:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000058
IP: [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
PGD 32adf0067 PUD 32adf1067 PMD 0
Oops: 0002 [#1] SMP DEBUG_PAGEALLOC
Modules linked in: iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi igb
i2c_algo_bit ptp pps_core pm80xx libsas scsi_transport_sas sg coretemp
eeprom w83795 i2c_i801
CPU: 4 PID: 1450 Comm: cydiskbench Not tainted 4.0.0-rc3 #1
Hardware name: Supermicro X8DTH-i/6/iF/6F/X8DTH, BIOS 2.1b       05/04/12
task: ffff8800ba86d500 ti: ffff88032a064000 task.ti: ffff88032a064000
RIP: 0010:[<ffffffff804fd46e>]  [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
RSP: 0018:ffff88032a067858  EFLAGS: 00010046
RAX: 0000000000000000 RBX: ffff8800ba0d2230 RCX: 000000000000002a
RDX: ffffffff80505ae0 RSI: 0000000000000020 RDI: ffff8800ba0d2230
RBP: ffff88032a067868 R08: 0000000000000201 R09: 0000000000000001
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8800ba0d0000
R13: ffff8800ba0d2230 R14: ffffffff80505ae0 R15: ffff8800ba0d0000
FS:  0000000041223950(0063) GS:ffff88033e480000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000058 CR3: 000000032a0a3000 CR4: 00000000000006e0
Stack:
  ffff880329eee758 ffff880329eee758 ffff88032a0678a8 ffffffff80502dad
  ffff8800ba167978 ffff880329eee758 ffff88032bf9c520 ffff8800ba167978
  ffff88032bf9c520 ffff88032bf9a290 ffff88032a0678b8 ffffffff80506909
Call Trace:
  [<ffffffff80502dad>] ata_scsi_translate+0x3d/0x1b0
  [<ffffffff80506909>] ata_sas_queuecmd+0x149/0x2a0
  [<ffffffffa0046650>] sas_queuecommand+0xa0/0x1f0 [libsas]
  [<ffffffff804ea544>] scsi_dispatch_cmd+0xd4/0x1a0
  [<ffffffff804eb50f>] scsi_queue_rq+0x66f/0x7f0
  [<ffffffff803e5098>] __blk_mq_run_hw_queue+0x208/0x3f0
  [<ffffffff803e54b8>] blk_mq_run_hw_queue+0x88/0xc0
  [<ffffffff803e5c74>] blk_mq_insert_request+0xc4/0x130
  [<ffffffff803e0b63>] blk_execute_rq_nowait+0x73/0x160
  [<ffffffffa0023fca>] sg_common_write+0x3da/0x720 [sg]
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffffa0025100>] sg_new_write+0x250/0x360 [sg]
  [<ffffffff802a236c>] ? __lock_acquire+0x50c/0xc10
  [<ffffffff802a2b87>] ? lock_release_non_nested+0xa7/0x360
  [<ffffffff8068954b>] ? _raw_spin_unlock_irqrestore+0x3b/0x60
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffffa0025feb>] sg_write+0x13b/0x450 [sg]
  [<ffffffff802a236c>] ? __lock_acquire+0x50c/0xc10
  [<ffffffff802cb1e9>] ? do_futex+0x109/0xbf0
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffff8032ec91>] vfs_write+0xd1/0x1b0
  [<ffffffff8032ee54>] SyS_write+0x54/0xc0
  [<ffffffff80689932>] system_call_fastpath+0x12/0x17
Code: 24 20 04 0f 85 ec 00 00 00 49 83 3c 24 00 0f 84 cf 00 00 00 83 fe 1f

---[ end trace 43f5eefb64627eff ]---

Quoted 9 lines

Quoted 4 lines
scsi-mq uses a host-wide tag map shared among all devices with some integer tag values >= ATA_MAX_QUEUE. These unexpectedly high tag values cause __ata_qc_from_tag() to return NULL, which is then dereferenced in ata_qc_new_init(), causing the oops above.
Wait, something is missing here. We should not be getting tag values that are >= ATA_MAX_QUEUE. Instead of reverting this, we need to figure out why this is happening, and fix it. That is correct way forward here. What setup is this being reproduced on?

Hardware:
PMC 8001 SAS HBA (PCI ID 117c:0042, PCI sub-ID 117c:0043) using pm80xx driver
4 SATA disks directly connected (no expander)

Test procedure:
Send disk read/write commands to SATA disks using the SCSI generic (sg) driver.

Analysis:

shost->can_queue is 508

With my patch applied to revert the problematic commits, I added the following code to ata_scsi_qc_new():

    int tag = cmd->request->tag;
    static int max_tag;
    if (tag > max_tag) {
        max_tag = tag;
        printk(KERN_DEBUG "max tag %d\n", tag);
    }

Testing one SATA disk at a time with scsi-mq enabled, I get a max tag of
64. Testing 4 disks at a time with scsi-mq enabled gives a max tag of
194. With scsi-mq disabled, I get a max tag of 30 no matter how many disks I test.

Tony

Shaohua Li

2015-03-12 01:55 UTC #3 lore raw

Quoted 99 lines · Tony Battersby

Quoted 65 lines · Jens Axboe
Quoted 64 lines · Tony Battersby
This reverts commits 12cb5ce101abfaf74421f8cc9f196e708209eb79 and 98bd4be1ba95f2fe7f543910792b7163a5de06eb.
Commit 12cb5ce101ab ("libata: use blk taging") causes the following oops with scsi-mq enabled:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000058
IP: [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
PGD 32adf0067 PUD 32adf1067 PMD 0
Oops: 0002 [#1] SMP DEBUG_PAGEALLOC
Modules linked in: iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi igb
i2c_algo_bit ptp pps_core pm80xx libsas scsi_transport_sas sg coretemp
eeprom w83795 i2c_i801
CPU: 4 PID: 1450 Comm: cydiskbench Not tainted 4.0.0-rc3 #1
Hardware name: Supermicro X8DTH-i/6/iF/6F/X8DTH, BIOS 2.1b       05/04/12
task: ffff8800ba86d500 ti: ffff88032a064000 task.ti: ffff88032a064000
RIP: 0010:[<ffffffff804fd46e>]  [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
RSP: 0018:ffff88032a067858  EFLAGS: 00010046
RAX: 0000000000000000 RBX: ffff8800ba0d2230 RCX: 000000000000002a
RDX: ffffffff80505ae0 RSI: 0000000000000020 RDI: ffff8800ba0d2230
RBP: ffff88032a067868 R08: 0000000000000201 R09: 0000000000000001
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8800ba0d0000
R13: ffff8800ba0d2230 R14: ffffffff80505ae0 R15: ffff8800ba0d0000
FS:  0000000041223950(0063) GS:ffff88033e480000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000058 CR3: 000000032a0a3000 CR4: 00000000000006e0
Stack:
  ffff880329eee758 ffff880329eee758 ffff88032a0678a8 ffffffff80502dad
  ffff8800ba167978 ffff880329eee758 ffff88032bf9c520 ffff8800ba167978
  ffff88032bf9c520 ffff88032bf9a290 ffff88032a0678b8 ffffffff80506909
Call Trace:
  [<ffffffff80502dad>] ata_scsi_translate+0x3d/0x1b0
  [<ffffffff80506909>] ata_sas_queuecmd+0x149/0x2a0
  [<ffffffffa0046650>] sas_queuecommand+0xa0/0x1f0 [libsas]
  [<ffffffff804ea544>] scsi_dispatch_cmd+0xd4/0x1a0
  [<ffffffff804eb50f>] scsi_queue_rq+0x66f/0x7f0
  [<ffffffff803e5098>] __blk_mq_run_hw_queue+0x208/0x3f0
  [<ffffffff803e54b8>] blk_mq_run_hw_queue+0x88/0xc0
  [<ffffffff803e5c74>] blk_mq_insert_request+0xc4/0x130
  [<ffffffff803e0b63>] blk_execute_rq_nowait+0x73/0x160
  [<ffffffffa0023fca>] sg_common_write+0x3da/0x720 [sg]
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffffa0025100>] sg_new_write+0x250/0x360 [sg]
  [<ffffffff802a236c>] ? __lock_acquire+0x50c/0xc10
  [<ffffffff802a2b87>] ? lock_release_non_nested+0xa7/0x360
  [<ffffffff8068954b>] ? _raw_spin_unlock_irqrestore+0x3b/0x60
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffffa0025feb>] sg_write+0x13b/0x450 [sg]
  [<ffffffff802a236c>] ? __lock_acquire+0x50c/0xc10
  [<ffffffff802cb1e9>] ? do_futex+0x109/0xbf0
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffff8032ec91>] vfs_write+0xd1/0x1b0
  [<ffffffff8032ee54>] SyS_write+0x54/0xc0
  [<ffffffff80689932>] system_call_fastpath+0x12/0x17
Code: 24 20 04 0f 85 ec 00 00 00 49 83 3c 24 00 0f 84 cf 00 00 00 83 fe 1f
0f 87 dc 00 00 00 89 f0 48 69 c0 f0 00 00 00 49 8d 44 04 40 <89> 70 58 48 c7 40 10 00 00 00 00 4c 89 20 48 89 58 08 c7 40 64
RIP [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
RSP <ffff88032a067858>
CR2: 0000000000000058
---[ end trace 43f5eefb64627eff ]---
Quoted 9 lines
Quoted 4 lines
scsi-mq uses a host-wide tag map shared among all devices with some integer tag values >= ATA_MAX_QUEUE. These unexpectedly high tag values cause __ata_qc_from_tag() to return NULL, which is then dereferenced in ata_qc_new_init(), causing the oops above.
Wait, something is missing here. We should not be getting tag values that are >= ATA_MAX_QUEUE. Instead of reverting this, we need to figure out why this is happening, and fix it. That is correct way forward here. What setup is this being reproduced on?
Hardware:
PMC 8001 SAS HBA (PCI ID 117c:0042, PCI sub-ID 117c:0043) using pm80xx driver
4 SATA disks directly connected (no expander)
Test procedure:
Send disk read/write commands to SATA disks using the SCSI generic (sg) driver.
Analysis:
shost->can_queue is 508
With my patch applied to revert the problematic commits, I added the following code to ata_scsi_qc_new():
    int tag = cmd->request->tag;
    static int max_tag;
    if (tag > max_tag) {
        max_tag = tag;
        printk(KERN_DEBUG "max tag %d\n", tag);
    }

Quoted 4 lines

Testing one SATA disk at a time with scsi-mq enabled, I get a max tag of
64. Testing 4 disks at a time with scsi-mq enabled gives a max tag of
194. With scsi-mq disabled, I get a max tag of 30 no matter how many disks I test.

ata_qc_new_init() has a check, if !ap->scsi_host, request tag will not be used and fallback to original tag allocation. you are using sas controller, so the problem is why ap->scsi_host is set in your case.

Thanks,
Shaohua

Shaohua Li

2015-03-12 02:32 UTC #4 lore raw

In reply to Tony Battersby

Quoted 99 lines · Tony Battersby

Quoted 65 lines · Jens Axboe
Quoted 64 lines · Tony Battersby
This reverts commits 12cb5ce101abfaf74421f8cc9f196e708209eb79 and 98bd4be1ba95f2fe7f543910792b7163a5de06eb.
Commit 12cb5ce101ab ("libata: use blk taging") causes the following oops with scsi-mq enabled:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000058
IP: [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
PGD 32adf0067 PUD 32adf1067 PMD 0
Oops: 0002 [#1] SMP DEBUG_PAGEALLOC
Modules linked in: iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi igb
i2c_algo_bit ptp pps_core pm80xx libsas scsi_transport_sas sg coretemp
eeprom w83795 i2c_i801
CPU: 4 PID: 1450 Comm: cydiskbench Not tainted 4.0.0-rc3 #1
Hardware name: Supermicro X8DTH-i/6/iF/6F/X8DTH, BIOS 2.1b       05/04/12
task: ffff8800ba86d500 ti: ffff88032a064000 task.ti: ffff88032a064000
RIP: 0010:[<ffffffff804fd46e>]  [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
RSP: 0018:ffff88032a067858  EFLAGS: 00010046
RAX: 0000000000000000 RBX: ffff8800ba0d2230 RCX: 000000000000002a
RDX: ffffffff80505ae0 RSI: 0000000000000020 RDI: ffff8800ba0d2230
RBP: ffff88032a067868 R08: 0000000000000201 R09: 0000000000000001
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8800ba0d0000
R13: ffff8800ba0d2230 R14: ffffffff80505ae0 R15: ffff8800ba0d0000
FS:  0000000041223950(0063) GS:ffff88033e480000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000058 CR3: 000000032a0a3000 CR4: 00000000000006e0
Stack:
  ffff880329eee758 ffff880329eee758 ffff88032a0678a8 ffffffff80502dad
  ffff8800ba167978 ffff880329eee758 ffff88032bf9c520 ffff8800ba167978
  ffff88032bf9c520 ffff88032bf9a290 ffff88032a0678b8 ffffffff80506909
Call Trace:
  [<ffffffff80502dad>] ata_scsi_translate+0x3d/0x1b0
  [<ffffffff80506909>] ata_sas_queuecmd+0x149/0x2a0
  [<ffffffffa0046650>] sas_queuecommand+0xa0/0x1f0 [libsas]
  [<ffffffff804ea544>] scsi_dispatch_cmd+0xd4/0x1a0
  [<ffffffff804eb50f>] scsi_queue_rq+0x66f/0x7f0
  [<ffffffff803e5098>] __blk_mq_run_hw_queue+0x208/0x3f0
  [<ffffffff803e54b8>] blk_mq_run_hw_queue+0x88/0xc0
  [<ffffffff803e5c74>] blk_mq_insert_request+0xc4/0x130
  [<ffffffff803e0b63>] blk_execute_rq_nowait+0x73/0x160
  [<ffffffffa0023fca>] sg_common_write+0x3da/0x720 [sg]
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffffa0025100>] sg_new_write+0x250/0x360 [sg]
  [<ffffffff802a236c>] ? __lock_acquire+0x50c/0xc10
  [<ffffffff802a2b87>] ? lock_release_non_nested+0xa7/0x360
  [<ffffffff8068954b>] ? _raw_spin_unlock_irqrestore+0x3b/0x60
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffffa0025feb>] sg_write+0x13b/0x450 [sg]
  [<ffffffff802a236c>] ? __lock_acquire+0x50c/0xc10
  [<ffffffff802cb1e9>] ? do_futex+0x109/0xbf0
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffff8032ec91>] vfs_write+0xd1/0x1b0
  [<ffffffff8032ee54>] SyS_write+0x54/0xc0
  [<ffffffff80689932>] system_call_fastpath+0x12/0x17
Code: 24 20 04 0f 85 ec 00 00 00 49 83 3c 24 00 0f 84 cf 00 00 00 83 fe 1f
0f 87 dc 00 00 00 89 f0 48 69 c0 f0 00 00 00 49 8d 44 04 40 <89> 70 58 48 c7 40 10 00 00 00 00 4c 89 20 48 89 58 08 c7 40 64
RIP [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
RSP <ffff88032a067858>
CR2: 0000000000000058
---[ end trace 43f5eefb64627eff ]---
Quoted 9 lines
Quoted 4 lines
scsi-mq uses a host-wide tag map shared among all devices with some integer tag values >= ATA_MAX_QUEUE. These unexpectedly high tag values cause __ata_qc_from_tag() to return NULL, which is then dereferenced in ata_qc_new_init(), causing the oops above.
Wait, something is missing here. We should not be getting tag values that are >= ATA_MAX_QUEUE. Instead of reverting this, we need to figure out why this is happening, and fix it. That is correct way forward here. What setup is this being reproduced on?
Hardware:
PMC 8001 SAS HBA (PCI ID 117c:0042, PCI sub-ID 117c:0043) using pm80xx driver
4 SATA disks directly connected (no expander)
Test procedure:
Send disk read/write commands to SATA disks using the SCSI generic (sg) driver.
Analysis:
shost->can_queue is 508
With my patch applied to revert the problematic commits, I added the following code to ata_scsi_qc_new():
    int tag = cmd->request->tag;
    static int max_tag;
    if (tag > max_tag) {
        max_tag = tag;
        printk(KERN_DEBUG "max tag %d\n", tag);
    }

Quoted 4 lines

Testing one SATA disk at a time with scsi-mq enabled, I get a max tag of
64. Testing 4 disks at a time with scsi-mq enabled gives a max tag of
194. With scsi-mq disabled, I get a max tag of 30 no matter how many disks I test.

Can you please try this debug patch:

drivers/scsi/libsas/sas_ata.c +0 -1

index 932d9cc..46f153f 100644
@@ -572,7 +572,6 @@ int sas_ata_init(struct domain_device *found_dev)
 
 	ap->private_data = found_dev;
 	ap->cbl = ATA_CBL_SATA;
-	ap->scsi_host = shost;
 	rc = ata_sas_port_init(ap);
 	if (rc) {
 		ata_sas_port_destroy(ap);

Dan Williams

2015-03-12 10:14 UTC #5 lore raw

Quoted 112 lines · Shaohua Li

Quoted 99 lines · Tony Battersby
Quoted 65 lines · Jens Axboe
Quoted 64 lines · Tony Battersby
This reverts commits 12cb5ce101abfaf74421f8cc9f196e708209eb79 and 98bd4be1ba95f2fe7f543910792b7163a5de06eb.
Commit 12cb5ce101ab ("libata: use blk taging") causes the following oops with scsi-mq enabled:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000058
IP: [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
PGD 32adf0067 PUD 32adf1067 PMD 0
Oops: 0002 [#1] SMP DEBUG_PAGEALLOC
Modules linked in: iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi igb
i2c_algo_bit ptp pps_core pm80xx libsas scsi_transport_sas sg coretemp
eeprom w83795 i2c_i801
CPU: 4 PID: 1450 Comm: cydiskbench Not tainted 4.0.0-rc3 #1
Hardware name: Supermicro X8DTH-i/6/iF/6F/X8DTH, BIOS 2.1b       05/04/12
task: ffff8800ba86d500 ti: ffff88032a064000 task.ti: ffff88032a064000
RIP: 0010:[<ffffffff804fd46e>]  [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
RSP: 0018:ffff88032a067858  EFLAGS: 00010046
RAX: 0000000000000000 RBX: ffff8800ba0d2230 RCX: 000000000000002a
RDX: ffffffff80505ae0 RSI: 0000000000000020 RDI: ffff8800ba0d2230
RBP: ffff88032a067868 R08: 0000000000000201 R09: 0000000000000001
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8800ba0d0000
R13: ffff8800ba0d2230 R14: ffffffff80505ae0 R15: ffff8800ba0d0000
FS:  0000000041223950(0063) GS:ffff88033e480000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000058 CR3: 000000032a0a3000 CR4: 00000000000006e0
Stack:
  ffff880329eee758 ffff880329eee758 ffff88032a0678a8 ffffffff80502dad
  ffff8800ba167978 ffff880329eee758 ffff88032bf9c520 ffff8800ba167978
  ffff88032bf9c520 ffff88032bf9a290 ffff88032a0678b8 ffffffff80506909
Call Trace:
  [<ffffffff80502dad>] ata_scsi_translate+0x3d/0x1b0
  [<ffffffff80506909>] ata_sas_queuecmd+0x149/0x2a0
  [<ffffffffa0046650>] sas_queuecommand+0xa0/0x1f0 [libsas]
  [<ffffffff804ea544>] scsi_dispatch_cmd+0xd4/0x1a0
  [<ffffffff804eb50f>] scsi_queue_rq+0x66f/0x7f0
  [<ffffffff803e5098>] __blk_mq_run_hw_queue+0x208/0x3f0
  [<ffffffff803e54b8>] blk_mq_run_hw_queue+0x88/0xc0
  [<ffffffff803e5c74>] blk_mq_insert_request+0xc4/0x130
  [<ffffffff803e0b63>] blk_execute_rq_nowait+0x73/0x160
  [<ffffffffa0023fca>] sg_common_write+0x3da/0x720 [sg]
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffffa0025100>] sg_new_write+0x250/0x360 [sg]
  [<ffffffff802a236c>] ? __lock_acquire+0x50c/0xc10
  [<ffffffff802a2b87>] ? lock_release_non_nested+0xa7/0x360
  [<ffffffff8068954b>] ? _raw_spin_unlock_irqrestore+0x3b/0x60
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffffa0025feb>] sg_write+0x13b/0x450 [sg]
  [<ffffffff802a236c>] ? __lock_acquire+0x50c/0xc10
  [<ffffffff802cb1e9>] ? do_futex+0x109/0xbf0
  [<ffffffff80308b8e>] ? might_fault+0x5e/0xb0
  [<ffffffff8032ec91>] vfs_write+0xd1/0x1b0
  [<ffffffff8032ee54>] SyS_write+0x54/0xc0
  [<ffffffff80689932>] system_call_fastpath+0x12/0x17
Code: 24 20 04 0f 85 ec 00 00 00 49 83 3c 24 00 0f 84 cf 00 00 00 83 fe 1f
0f 87 dc 00 00 00 89 f0 48 69 c0 f0 00 00 00 49 8d 44 04 40 <89> 70 58 48 c7 40 10 00 00 00 00 4c 89 20 48 89 58 08 c7 40 64
RIP [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
RSP <ffff88032a067858>
CR2: 0000000000000058
---[ end trace 43f5eefb64627eff ]---
Quoted 9 lines
Quoted 4 lines
scsi-mq uses a host-wide tag map shared among all devices with some integer tag values >= ATA_MAX_QUEUE. These unexpectedly high tag values cause __ata_qc_from_tag() to return NULL, which is then dereferenced in ata_qc_new_init(), causing the oops above.
Wait, something is missing here. We should not be getting tag values that are >= ATA_MAX_QUEUE. Instead of reverting this, we need to figure out why this is happening, and fix it. That is correct way forward here. What setup is this being reproduced on?
Hardware:
PMC 8001 SAS HBA (PCI ID 117c:0042, PCI sub-ID 117c:0043) using pm80xx driver
4 SATA disks directly connected (no expander)
Test procedure:
Send disk read/write commands to SATA disks using the SCSI generic (sg) driver.
Analysis:
shost->can_queue is 508
With my patch applied to revert the problematic commits, I added the following code to ata_scsi_qc_new():
    int tag = cmd->request->tag;
    static int max_tag;
    if (tag > max_tag) {
        max_tag = tag;
        printk(KERN_DEBUG "max tag %d\n", tag);
    }
Quoted 4 lines
Testing one SATA disk at a time with scsi-mq enabled, I get a max tag of
64. Testing 4 disks at a time with scsi-mq enabled gives a max tag of
194. With scsi-mq disabled, I get a max tag of 30 no matter how many disks I test.
Can you please try this debug patch:
drivers/scsi/libsas/sas_ata.c +0 -0
index 932d9cc..46f153f 100644

drivers/scsi/libsas/sas_ata.c int sas_ata_init(struct domain_device *found_dev) · line 577

Quoted 7 lines

drivers/scsi/libsas/sas_ata.c +0 -1

@@ -572,7 +572,6 @@ int sas_ata_init(struct domain_device *found_dev)
 
        ap->private_data = found_dev;
        ap->cbl = ATA_CBL_SATA;
-       ap->scsi_host = shost;
        rc = ata_sas_port_init(ap);
        if (rc) {
                ata_sas_port_destroy(ap);

We need a scsi_host for error recovery, see:

ata_std_sched_eh()

Tejun Heo

2015-03-12 12:11 UTC #6 lore raw

Quoted 11 lines · Dan Williams

drivers/scsi/libsas/sas_ata.c int sas_ata_init(struct domain_device *found_dev) · line 577
Quoted 8 lines
drivers/scsi/libsas/sas_ata.c +0 -1
@@ -572,7 +572,6 @@ int sas_ata_init(struct domain_device *found_dev)
 
        ap->private_data = found_dev;
        ap->cbl = ATA_CBL_SATA;
-       ap->scsi_host = shost;
        rc = ata_sas_port_init(ap);
        if (rc) {
                ata_sas_port_destroy(ap);
We need a scsi_host for error recovery, see:

Quoted 1 line

ata_std_sched_eh()

IOW, let's just add a flag bit to identify SAS hosts.

Thanks.

Signature · 1 line

tejun

Shaohua Li

2015-03-12 12:46 UTC #7 lore raw

Quoted 12 lines · Tejun Heo

Quoted 11 lines · Dan Williams

Review int sas_ata_init(struct domain_device *found_dev) · line 577

Quoted 8 lines

(quoted diff) +0 -1

@@ -572,7 +572,6 @@ int sas_ata_init(struct domain_device *found_dev)
 
        ap->private_data = found_dev;
        ap->cbl = ATA_CBL_SATA;
-       ap->scsi_host = shost;
        rc = ata_sas_port_init(ap);
        if (rc) {
                ata_sas_port_destroy(ap);

We need a scsi_host for error recovery, see:

Quoted 3 lines

Quoted 1 line
ata_std_sched_eh()
IOW, let's just add a flag bit to identify SAS hosts.

Yep. Tony, can you please try below patch please?

ata: Add a new flag for sas controller

Add a new flag to destinguish sas controller. sas controller has its own tag allocation, which doesn't directly match to ata tag. We use the new flag for sas ata tag allocation.

Reported-by Tony Battersby <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

drivers/ata/libata-core.c +2 -2

index 4c35f08..ef150eb 100644
@@ -4737,7 +4737,7 @@ struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
 		return NULL;
 
 	/* libsas case */
-	if (!ap->scsi_host) {
+	if (ap->flags & ATA_FLAG_SAS_HOST) {
 		tag = ata_sas_allocate_tag(ap);
 		if (tag < 0)
 			return NULL;

@@ -4776,7 +4776,7 @@ void ata_qc_free(struct ata_queued_cmd *qc)
 	tag = qc->tag;
 	if (likely(ata_tag_valid(tag))) {
 		qc->tag = ATA_TAG_POISON;
-		if (!ap->scsi_host)
+		if (ap->flags & ATA_FLAG_SAS_HOST)
 			ata_sas_free_tag(tag, ap);
 	}
 }

drivers/scsi/ipr.c +2 -1

index 9219953..d9afc51 100644
@@ -6815,7 +6815,8 @@ static struct ata_port_operations ipr_sata_ops = {
 };
 
 static struct ata_port_info sata_port_info = {
-	.flags		= ATA_FLAG_SATA | ATA_FLAG_PIO_DMA,
+	.flags		= ATA_FLAG_SATA | ATA_FLAG_PIO_DMA |
+			  ATA_FLAG_SAS_HOST,
 	.pio_mask	= ATA_PIO4_ONLY,
 	.mwdma_mask	= ATA_MWDMA2,
 	.udma_mask	= ATA_UDMA6,

drivers/scsi/libsas/sas_ata.c +2 -1

index 932d9cc..9c706d8 100644
@@ -547,7 +547,8 @@ static struct ata_port_operations sas_sata_ops = {
 };
 
 static struct ata_port_info sata_port_info = {
-	.flags = ATA_FLAG_SATA | ATA_FLAG_PIO_DMA | ATA_FLAG_NCQ,
+	.flags = ATA_FLAG_SATA | ATA_FLAG_PIO_DMA | ATA_FLAG_NCQ |
+		 ATA_FLAG_SAS_HOST,
 	.pio_mask = ATA_PIO4,
 	.mwdma_mask = ATA_MWDMA2,
 	.udma_mask = ATA_UDMA6,

include/linux/libata.h +1 -0

index fc03efa..6b08cc1 100644
@@ -232,6 +232,7 @@ enum {
 					      * led */
 	ATA_FLAG_NO_DIPM	= (1 << 23), /* host not happy with DIPM */
 	ATA_FLAG_LOWTAG		= (1 << 24), /* host wants lowest available tag */
+	ATA_FLAG_SAS_HOST	= (1 << 25), /* SAS host */
 
 	/* bits 24:31 of ap->flags are reserved for LLD specific flags */

Tony Battersby

author Tested-by

2015-03-12 13:51 UTC #8 lore raw

Quoted 12 lines · Shaohua Li

Yep. Tony, can you please try below patch please?
ata: Add a new flag for sas controller
Add a new flag to destinguish sas controller. sas controller has its own tag allocation, which doesn't directly match to ata tag. We use the new flag for sas ata tag allocation.
Reported-by Tony Battersby <[email protected]>
Signed-off-by Shaohua Li <[email protected]>

Yes, that fixes it. Thanks!

Tested-by Tony Battersby <[email protected]>

Quoted 63 lines

drivers/ata/libata-core.c +2 -2

index 4c35f08..ef150eb 100644
@@ -4737,7 +4737,7 @@ struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
 		return NULL;
 
 	/* libsas case */
-	if (!ap->scsi_host) {
+	if (ap->flags & ATA_FLAG_SAS_HOST) {
 		tag = ata_sas_allocate_tag(ap);
 		if (tag < 0)
 			return NULL;

@@ -4776,7 +4776,7 @@ void ata_qc_free(struct ata_queued_cmd *qc)
 	tag = qc->tag;
 	if (likely(ata_tag_valid(tag))) {
 		qc->tag = ATA_TAG_POISON;
-		if (!ap->scsi_host)
+		if (ap->flags & ATA_FLAG_SAS_HOST)
 			ata_sas_free_tag(tag, ap);
 	}
 }

drivers/scsi/ipr.c +2 -1

index 9219953..d9afc51 100644
@@ -6815,7 +6815,8 @@ static struct ata_port_operations ipr_sata_ops = {
 };
 
 static struct ata_port_info sata_port_info = {
-	.flags		= ATA_FLAG_SATA | ATA_FLAG_PIO_DMA,
+	.flags		= ATA_FLAG_SATA | ATA_FLAG_PIO_DMA |
+			  ATA_FLAG_SAS_HOST,
 	.pio_mask	= ATA_PIO4_ONLY,
 	.mwdma_mask	= ATA_MWDMA2,
 	.udma_mask	= ATA_UDMA6,

drivers/scsi/libsas/sas_ata.c +2 -1

index 932d9cc..9c706d8 100644
@@ -547,7 +547,8 @@ static struct ata_port_operations sas_sata_ops = {
 };
 
 static struct ata_port_info sata_port_info = {
-	.flags = ATA_FLAG_SATA | ATA_FLAG_PIO_DMA | ATA_FLAG_NCQ,
+	.flags = ATA_FLAG_SATA | ATA_FLAG_PIO_DMA | ATA_FLAG_NCQ |
+		 ATA_FLAG_SAS_HOST,
 	.pio_mask = ATA_PIO4,
 	.mwdma_mask = ATA_MWDMA2,
 	.udma_mask = ATA_UDMA6,

include/linux/libata.h +1 -0

index fc03efa..6b08cc1 100644
@@ -232,6 +232,7 @@ enum {
 					      * led */
 	ATA_FLAG_NO_DIPM	= (1 << 23), /* host not happy with DIPM */
 	ATA_FLAG_LOWTAG		= (1 << 24), /* host wants lowest available tag */
+	ATA_FLAG_SAS_HOST	= (1 << 25), /* SAS host */
 
 	/* bits 24:31 of ap->flags are reserved for LLD specific flags */

Tejun Heo

2015-03-12 13:59 UTC #9 lore raw

In reply to Shaohua Li

Quoted 5 lines · Shaohua Li

ata: Add a new flag for sas controller
Add a new flag to destinguish sas controller. sas controller has its own tag allocation, which doesn't directly match to ata tag. We use the new flag for sas ata tag allocation.

This doesn't really explain why the patch came to be. Can you please update it to include what broke how and got fixed? A Fixes: tag would be nice too.

Thanks.

Signature · 1 line

tejun

Shaohua Li

2015-03-12 17:33 UTC #10 lore raw

Quoted 10 lines · Tejun Heo

Quoted 5 lines · Shaohua Li
ata: Add a new flag for sas controller
Add a new flag to destinguish sas controller. sas controller has its own tag allocation, which doesn't directly match to ata tag. We use the new flag for sas ata tag allocation.
This doesn't really explain why the patch came to be. Can you please update it to include what broke how and got fixed? A Fixes: tag would be nice too.

Is this better?

ata: Add a new flag to destinguish sas controller

SAS controller has its own tag allocation, which doesn't directly match to ATA tag, so SAS and SATA have different code path for ata tags. Originally we use port->scsi_host (98bd4be1) to destinguish SAS controller, but libsas set ->scsi_host too, so we can't use it for the destinguish, we add a new flag for this purpose.

Reported-and-tested-by Tony Battersby <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

drivers/ata/libata-core.c +2 -2

index 4c35f08..ef150eb 100644
@@ -4737,7 +4737,7 @@ struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
 		return NULL;
 
 	/* libsas case */
-	if (!ap->scsi_host) {
+	if (ap->flags & ATA_FLAG_SAS_HOST) {
 		tag = ata_sas_allocate_tag(ap);
 		if (tag < 0)
 			return NULL;

@@ -4776,7 +4776,7 @@ void ata_qc_free(struct ata_queued_cmd *qc)
 	tag = qc->tag;
 	if (likely(ata_tag_valid(tag))) {
 		qc->tag = ATA_TAG_POISON;
-		if (!ap->scsi_host)
+		if (ap->flags & ATA_FLAG_SAS_HOST)
 			ata_sas_free_tag(tag, ap);
 	}
 }

drivers/scsi/ipr.c +2 -1

index 9219953..d9afc51 100644
@@ -6815,7 +6815,8 @@ static struct ata_port_operations ipr_sata_ops = {
 };
 
 static struct ata_port_info sata_port_info = {
-	.flags		= ATA_FLAG_SATA | ATA_FLAG_PIO_DMA,
+	.flags		= ATA_FLAG_SATA | ATA_FLAG_PIO_DMA |
+			  ATA_FLAG_SAS_HOST,
 	.pio_mask	= ATA_PIO4_ONLY,
 	.mwdma_mask	= ATA_MWDMA2,
 	.udma_mask	= ATA_UDMA6,

drivers/scsi/libsas/sas_ata.c +2 -1

index 932d9cc..9c706d8 100644
@@ -547,7 +547,8 @@ static struct ata_port_operations sas_sata_ops = {
 };
 
 static struct ata_port_info sata_port_info = {
-	.flags = ATA_FLAG_SATA | ATA_FLAG_PIO_DMA | ATA_FLAG_NCQ,
+	.flags = ATA_FLAG_SATA | ATA_FLAG_PIO_DMA | ATA_FLAG_NCQ |
+		 ATA_FLAG_SAS_HOST,
 	.pio_mask = ATA_PIO4,
 	.mwdma_mask = ATA_MWDMA2,
 	.udma_mask = ATA_UDMA6,

include/linux/libata.h +1 -0

index fc03efa..6b08cc1 100644
@@ -232,6 +232,7 @@ enum {
 					      * led */
 	ATA_FLAG_NO_DIPM	= (1 << 23), /* host not happy with DIPM */
 	ATA_FLAG_LOWTAG		= (1 << 24), /* host wants lowest available tag */
+	ATA_FLAG_SAS_HOST	= (1 << 25), /* SAS host */
 
 	/* bits 24:31 of ap->flags are reserved for LLD specific flags */

Tejun Heo

2015-03-19 18:17 UTC #11 lore raw

PATCH 1/1 ata: Add a new flag to destinguish sas controller

Quoted 1 line

From 5067c0469c643512f24786990e315f9c15cc7d24 Mon Sep 17 00:00:00 2001

From: Shaohua Li <[email protected]>
Date: Thu, 12 Mar 2015 10:32:18 -0700

Without this patch, the following oops can happen because scsi-mq uses a host-wide tag map shared among all devices with some integer tag values >= ATA_MAX_QUEUE. These unexpectedly high tag values cause __ata_qc_from_tag() to return NULL, which is then dereferenced in ata_qc_new_init().

  BUG: unable to handle kernel NULL pointer dereference at 0000000000000058
  IP: [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
  PGD 32adf0067 PUD 32adf1067 PMD 0
  Oops: 0002 [#1] SMP DEBUG_PAGEALLOC
  Modules linked in: iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi igb
  i2c_algo_bit ptp pps_core pm80xx libsas scsi_transport_sas sg coretemp
  eeprom w83795 i2c_i801
  CPU: 4 PID: 1450 Comm: cydiskbench Not tainted 4.0.0-rc3 #1
  Hardware name: Supermicro X8DTH-i/6/iF/6F/X8DTH, BIOS 2.1b       05/04/12
  task: ffff8800ba86d500 ti: ffff88032a064000 task.ti: ffff88032a064000
  RIP: 0010:[<ffffffff804fd46e>]  [<ffffffff804fd46e>] ata_qc_new_init+0x3e/0x120
  RSP: 0018:ffff88032a067858  EFLAGS: 00010046
  RAX: 0000000000000000 RBX: ffff8800ba0d2230 RCX: 000000000000002a
  RDX: ffffffff80505ae0 RSI: 0000000000000020 RDI: ffff8800ba0d2230
  RBP: ffff88032a067868 R08: 0000000000000201 R09: 0000000000000001
  R10: 0000000000000000 R11: 0000000000000000 R12: ffff8800ba0d0000
  R13: ffff8800ba0d2230 R14: ffffffff80505ae0 R15: ffff8800ba0d0000
  FS:  0000000041223950(0063) GS:ffff88033e480000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
  CR2: 0000000000000058 CR3: 000000032a0a3000 CR4: 00000000000006e0
  Stack:
   ffff880329eee758 ffff880329eee758 ffff88032a0678a8 ffffffff80502dad
   ffff8800ba167978 ffff880329eee758 ffff88032bf9c520 ffff8800ba167978
   ffff88032bf9c520 ffff88032bf9a290 ffff88032a0678b8 ffffffff80506909
  Call Trace:
   [<ffffffff80502dad>] ata_scsi_translate+0x3d/0x1b0
   [<ffffffff80506909>] ata_sas_queuecmd+0x149/0x2a0
   [<ffffffffa0046650>] sas_queuecommand+0xa0/0x1f0 [libsas]
   [<ffffffff804ea544>] scsi_dispatch_cmd+0xd4/0x1a0
   [<ffffffff804eb50f>] scsi_queue_rq+0x66f/0x7f0
   [<ffffffff803e5098>] __blk_mq_run_hw_queue+0x208/0x3f0
   [<ffffffff803e54b8>] blk_mq_run_hw_queue+0x88/0xc0
   [<ffffffff803e5c74>] blk_mq_insert_request+0xc4/0x130
   [<ffffffff803e0b63>] blk_execute_rq_nowait+0x73/0x160
   [<ffffffffa0023fca>] sg_common_write+0x3da/0x720 [sg]
   [<ffffffffa0025100>] sg_new_write+0x250/0x360 [sg]
   [<ffffffffa0025feb>] sg_write+0x13b/0x450 [sg]
   [<ffffffff8032ec91>] vfs_write+0xd1/0x1b0
   [<ffffffff8032ee54>] SyS_write+0x54/0xc0
   [<ffffffff80689932>] system_call_fastpath+0x12/0x17

tj: updated description.

Fixes 12cb5ce101ab ("libata: use blk taging")

Reported-and-tested-by Tony Battersby <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

Signed-off-by Tejun Heo <[email protected]>

---
Applied with description updated.

Thanks.

 drivers/ata/libata-core.c     | 4 ++--
 drivers/scsi/ipr.c            | 3 ++-
 drivers/scsi/libsas/sas_ata.c | 3 ++-
 include/linux/libata.h        | 1 +
 4 files changed, 7 insertions(+), 4 deletions(-)

drivers/ata/libata-core.c +2 -2

index 4c35f08..ef150eb 100644
@@ -4737,7 +4737,7 @@ struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
 		return NULL;
 
 	/* libsas case */
-	if (!ap->scsi_host) {
+	if (ap->flags & ATA_FLAG_SAS_HOST) {
 		tag = ata_sas_allocate_tag(ap);
 		if (tag < 0)
 			return NULL;

@@ -4776,7 +4776,7 @@ void ata_qc_free(struct ata_queued_cmd *qc)
 	tag = qc->tag;
 	if (likely(ata_tag_valid(tag))) {
 		qc->tag = ATA_TAG_POISON;
-		if (!ap->scsi_host)
+		if (ap->flags & ATA_FLAG_SAS_HOST)
 			ata_sas_free_tag(tag, ap);
 	}
 }

drivers/scsi/ipr.c +2 -1

index 9219953..d9afc51 100644
@@ -6815,7 +6815,8 @@ static struct ata_port_operations ipr_sata_ops = {
 };
 
 static struct ata_port_info sata_port_info = {
-	.flags		= ATA_FLAG_SATA | ATA_FLAG_PIO_DMA,
+	.flags		= ATA_FLAG_SATA | ATA_FLAG_PIO_DMA |
+			  ATA_FLAG_SAS_HOST,
 	.pio_mask	= ATA_PIO4_ONLY,
 	.mwdma_mask	= ATA_MWDMA2,
 	.udma_mask	= ATA_UDMA6,

drivers/scsi/libsas/sas_ata.c +2 -1

index 932d9cc..9c706d8 100644
@@ -547,7 +547,8 @@ static struct ata_port_operations sas_sata_ops = {
 };
 
 static struct ata_port_info sata_port_info = {
-	.flags = ATA_FLAG_SATA | ATA_FLAG_PIO_DMA | ATA_FLAG_NCQ,
+	.flags = ATA_FLAG_SATA | ATA_FLAG_PIO_DMA | ATA_FLAG_NCQ |
+		 ATA_FLAG_SAS_HOST,
 	.pio_mask = ATA_PIO4,
 	.mwdma_mask = ATA_MWDMA2,
 	.udma_mask = ATA_UDMA6,

include/linux/libata.h +1 -0

index fc03efa..6b08cc1 100644
@@ -232,6 +232,7 @@ enum {
 					      * led */
 	ATA_FLAG_NO_DIPM	= (1 << 23), /* host not happy with DIPM */
 	ATA_FLAG_LOWTAG		= (1 << 24), /* host wants lowest available tag */
+	ATA_FLAG_SAS_HOST	= (1 << 25), /* SAS host */
 
 	/* bits 24:31 of ap->flags are reserved for LLD specific flags */

Signature · 1 line

2.1.0