From fd3a9025c0349bc9b01d627529f54e6e1e389015 Mon Sep 17 00:00:00 2001 From: Nicholas Bellinger Date: Wed, 27 Feb 2013 17:53:52 -0800 Subject: [PATCH 1/5] iscsi-target: Fix immediate queue starvation regression with DATAIN This patch addresses a v3.5+ regression in iscsi-target where TX thread process context -> handle_response_queue() execution is allowed to run unbounded while servicing constant outgoing flow of ISTATE_SEND_DATAIN response state. This ends up preventing memory release of StatSN acknowledged commands in a timely manner when under heavy large block streaming DATAIN workloads. The regression bug was initially introduced with: commit 6f3c0e69a9c20441bdc6d3b2d18b83b244384ec6 Author: Andy Grover Date: Tue Apr 3 15:51:09 2012 -0700 target/iscsi: Refactor target_tx_thread immediate+response queue loops Go ahead and follow original iscsi_target_tx_thread() logic and check to break for immediate queue processing after each DataIN Sequence and/or Response PDU has been sent. Reported-by: Benjamin ESTRABAUD Cc: Andy Grover Cc: Signed-off-by: Nicholas Bellinger --- drivers/target/iscsi/iscsi_target.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index 23a98e658306..af77396234a2 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -3583,6 +3583,10 @@ static int handle_response_queue(struct iscsi_conn *conn) spin_lock_bh(&cmd->istate_lock); cmd->i_state = ISTATE_SENT_STATUS; spin_unlock_bh(&cmd->istate_lock); + + if (atomic_read(&conn->check_immediate_queue)) + return 1; + continue; } else if (ret == 2) { /* Still must send status, @@ -3672,7 +3676,7 @@ static int handle_response_queue(struct iscsi_conn *conn) } if (atomic_read(&conn->check_immediate_queue)) - break; + return 1; } return 0; @@ -3716,12 +3720,15 @@ int iscsi_target_tx_thread(void *arg) signal_pending(current)) goto transport_err; +get_immediate: ret = handle_immediate_queue(conn); if (ret < 0) goto transport_err; ret = handle_response_queue(conn); - if (ret == -EAGAIN) + if (ret == 1) + goto get_immediate; + else if (ret == -EAGAIN) goto restart; else if (ret < 0) goto transport_err; From 63b91d5a492ae1cdc1ba0a0a45024718f6d1437f Mon Sep 17 00:00:00 2001 From: Asias He Date: Wed, 27 Feb 2013 12:50:56 +0800 Subject: [PATCH 2/5] target: Add __exit annotation for module_exit functions Inclues sbp_exit, fileio_module_exit, iblock_module_exit and pscsi_module_exit. Note: rd_module_exit() can not be annotated by __exit, becasue it is called by target_core_init_configfs() which is annotated by __init. Signed-off-by: Asias He Signed-off-by: Nicholas Bellinger --- drivers/target/sbp/sbp_target.c | 2 +- drivers/target/target_core_file.c | 2 +- drivers/target/target_core_iblock.c | 2 +- drivers/target/target_core_pscsi.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c index 2e8d06f198ae..9d3dadeed82f 100644 --- a/drivers/target/sbp/sbp_target.c +++ b/drivers/target/sbp/sbp_target.c @@ -2598,7 +2598,7 @@ static int __init sbp_init(void) return 0; }; -static void sbp_exit(void) +static void __exit sbp_exit(void) { sbp_deregister_configfs(); }; diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c index ca36a38eb274..2936cfe41dc1 100644 --- a/drivers/target/target_core_file.c +++ b/drivers/target/target_core_file.c @@ -631,7 +631,7 @@ static int __init fileio_module_init(void) return transport_subsystem_register(&fileio_template); } -static void fileio_module_exit(void) +static void __exit fileio_module_exit(void) { transport_subsystem_release(&fileio_template); } diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c index c73f4a950e23..8bcc514ec8b6 100644 --- a/drivers/target/target_core_iblock.c +++ b/drivers/target/target_core_iblock.c @@ -821,7 +821,7 @@ static int __init iblock_module_init(void) return transport_subsystem_register(&iblock_template); } -static void iblock_module_exit(void) +static void __exit iblock_module_exit(void) { transport_subsystem_release(&iblock_template); } diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c index 2bcfd79cf595..37575a1f038b 100644 --- a/drivers/target/target_core_pscsi.c +++ b/drivers/target/target_core_pscsi.c @@ -1178,7 +1178,7 @@ static int __init pscsi_module_init(void) return transport_subsystem_register(&pscsi_template); } -static void pscsi_module_exit(void) +static void __exit pscsi_module_exit(void) { transport_subsystem_release(&pscsi_template); } From b07da9fb527e547e2c6198f5594f523bcc11433c Mon Sep 17 00:00:00 2001 From: Asias He Date: Wed, 27 Feb 2013 13:29:28 +0800 Subject: [PATCH 3/5] target/pscsi: Drop unnecessary NULL assignment to bio->bi_next Signed-off-by: Asias He Signed-off-by: Nicholas Bellinger --- drivers/target/target_core_pscsi.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c index 37575a1f038b..e005f9f4e4a6 100644 --- a/drivers/target/target_core_pscsi.c +++ b/drivers/target/target_core_pscsi.c @@ -952,7 +952,6 @@ pscsi_map_sg(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, while (*hbio) { bio = *hbio; *hbio = (*hbio)->bi_next; - bio->bi_next = NULL; bio_endio(bio, 0); /* XXX: should be error */ } return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; @@ -1092,7 +1091,6 @@ pscsi_execute_cmd(struct se_cmd *cmd) while (hbio) { struct bio *bio = hbio; hbio = hbio->bi_next; - bio->bi_next = NULL; bio_endio(bio, 0); /* XXX: should be error */ } ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; From 472b72f2db7831d7dbe22ffdff4adee3bd49b05d Mon Sep 17 00:00:00 2001 From: Asias He Date: Wed, 27 Feb 2013 13:29:29 +0800 Subject: [PATCH 4/5] target/pscsi: Fix page increment The page++ is wrong. It makes bio_add_pc_page() pointing to a wrong page address if the 'while (len > 0 && data_len > 0) { ... }' loop is executed more than one once. Signed-off-by: Asias He Cc: Signed-off-by: Nicholas Bellinger --- drivers/target/target_core_pscsi.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c index e005f9f4e4a6..f6921bf99113 100644 --- a/drivers/target/target_core_pscsi.c +++ b/drivers/target/target_core_pscsi.c @@ -940,7 +940,6 @@ pscsi_map_sg(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, bio = NULL; } - page++; len -= bytes; data_len -= bytes; off = 0; From 2dbe10a202d2743582b5fb7c9864455ef6ecf9a6 Mon Sep 17 00:00:00 2001 From: Asias He Date: Wed, 27 Feb 2013 13:29:30 +0800 Subject: [PATCH 5/5] target/pscsi: Rename sg_num to nr_vecs in pscsi_get_bio() It is actually a vector not a sg, so nr_vecs is better than sg_num. Signed-off-by: Asias He Signed-off-by: Nicholas Bellinger --- drivers/target/target_core_pscsi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c index f6921bf99113..82e78d72fdb6 100644 --- a/drivers/target/target_core_pscsi.c +++ b/drivers/target/target_core_pscsi.c @@ -840,14 +840,14 @@ static void pscsi_bi_endio(struct bio *bio, int error) bio_put(bio); } -static inline struct bio *pscsi_get_bio(int sg_num) +static inline struct bio *pscsi_get_bio(int nr_vecs) { struct bio *bio; /* * Use bio_malloc() following the comment in for bio -> struct request * in block/blk-core.c:blk_make_request() */ - bio = bio_kmalloc(GFP_KERNEL, sg_num); + bio = bio_kmalloc(GFP_KERNEL, nr_vecs); if (!bio) { pr_err("PSCSI: bio_kmalloc() failed\n"); return NULL;