ivampiresp/Spdk
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

lib/iscsi: Factor out freeing iSCSI tasks to a helper function

Browse Source 
iSCSI target frees iSCSI tasks when exiting connection or removing
LUN.  The difference is only that the passed LUN is NULL or not.
To make the code clearer, this patch factors out freeing iSCSI
tasks from iscsi_conn_free_tasks() and _iscsi_conn_remove_lun()
into _iscsi_conn_free_tasks().

The refactoring has subtle cases and so add UT code together.

The next patch will fix the issue that secondary tasks are left
even after primary tasks are freed when exiting connection or
removing LUN, and this patch clarifies the next patch.

Signed-off-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Change-Id: I18aaed6fe18a1c561ac88a0e5dc1296f9941d0e8
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/473154
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
This commit is contained in:
Shuhei Matsumoto 2019-11-04 09:50:42 +09:00 committed by Tomasz Zawadzki
parent ac417aa9d6
commit cdb7398dca
2 changed files with 199 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

60
lib/iscsi/conn.c
View File

@ -338,27 +338,35 @@ spdk_iscsi_conn_free_pdu(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pd
spdk_put_pdu(pdu);
}
static int
iscsi_conn_free_tasks(struct spdk_iscsi_conn *conn)
static void
_iscsi_conn_free_tasks(struct spdk_iscsi_conn *conn, struct spdk_scsi_lun *lun)
{
struct spdk_iscsi_pdu *pdu, *tmp_pdu;
struct spdk_iscsi_task *iscsi_task, *tmp_iscsi_task;
TAILQ_FOREACH_SAFE(pdu, &conn->write_pdu_list, tailq, tmp_pdu) {
TAILQ_REMOVE(&conn->write_pdu_list, pdu, tailq);
spdk_iscsi_conn_free_pdu(conn, pdu);
/* If connection is exited (no LUN is specified) or the PDU's LUN matches
* the LUN that was removed, free this PDU immediately. If the pdu's LUN
* is NULL, then we know the datain handling code already detected the hot
* removal, so we can free that PDU as well.
*/
if ((lun == NULL) ||
(pdu->task && (lun == pdu->task->scsi.lun || NULL == pdu->task->scsi.lun))) {
TAILQ_REMOVE(&conn->write_pdu_list, pdu, tailq);
spdk_iscsi_conn_free_pdu(conn, pdu);
}
}
TAILQ_FOREACH_SAFE(pdu, &conn->snack_pdu_list, tailq, tmp_pdu) {
TAILQ_REMOVE(&conn->snack_pdu_list, pdu, tailq);
if (pdu->task) {
if (pdu->task && (lun == NULL || lun == pdu->task->scsi.lun)) {
spdk_iscsi_task_put(pdu->task);
}
spdk_put_pdu(pdu);
}
TAILQ_FOREACH_SAFE(iscsi_task, &conn->queued_datain_tasks, link, tmp_iscsi_task) {
if (!iscsi_task->is_queued) {
if ((!iscsi_task->is_queued) && (lun == NULL || lun == iscsi_task->scsi.lun)) {
TAILQ_REMOVE(&conn->queued_datain_tasks, iscsi_task, link);
if (iscsi_task->current_datain_offset > 0) {
assert(conn->data_in_cnt > 0);
@ -367,6 +375,12 @@ iscsi_conn_free_tasks(struct spdk_iscsi_conn *conn)
spdk_iscsi_task_put(iscsi_task);
}
}
}
static int
iscsi_conn_free_tasks(struct spdk_iscsi_conn *conn)
{
_iscsi_conn_free_tasks(conn, NULL);
if (conn->pending_task_cnt) {
return -1;
@ -482,8 +496,6 @@ _iscsi_conn_remove_lun(void *_ctx)
struct spdk_iscsi_conn *conn = ctx->conn;
struct spdk_scsi_lun *lun = ctx->lun;
int lun_id = spdk_scsi_lun_get_id(lun);
struct spdk_iscsi_pdu *pdu, *tmp_pdu;
struct spdk_iscsi_task *iscsi_task, *tmp_iscsi_task;
free(ctx);
@ -496,37 +508,7 @@ _iscsi_conn_remove_lun(void *_ctx)
}
spdk_clear_all_transfer_task(conn, lun, NULL);
TAILQ_FOREACH_SAFE(pdu, &conn->write_pdu_list, tailq, tmp_pdu) {
/* If the pdu's LUN matches the LUN that was removed, free this
* PDU immediately. If the pdu's LUN is NULL, then we know
* the datain handling code already detected the hot removal,
* so we can free that PDU as well.
*/
if (pdu->task &&
(lun == pdu->task->scsi.lun || NULL == pdu->task->scsi.lun)) {
TAILQ_REMOVE(&conn->write_pdu_list, pdu, tailq);
spdk_iscsi_conn_free_pdu(conn, pdu);
}
}
TAILQ_FOREACH_SAFE(pdu, &conn->snack_pdu_list, tailq, tmp_pdu) {
if (pdu->task && (lun == pdu->task->scsi.lun)) {
TAILQ_REMOVE(&conn->snack_pdu_list, pdu, tailq);
spdk_iscsi_task_put(pdu->task);
spdk_put_pdu(pdu);
}
}
TAILQ_FOREACH_SAFE(iscsi_task, &conn->queued_datain_tasks, link, tmp_iscsi_task) {
if ((!iscsi_task->is_queued) && (lun == iscsi_task->scsi.lun)) {
TAILQ_REMOVE(&conn->queued_datain_tasks, iscsi_task, link);
if (iscsi_task->current_datain_offset > 0) {
assert(conn->data_in_cnt > 0);
conn->data_in_cnt--;
}
spdk_iscsi_task_put(iscsi_task);
}
}
_iscsi_conn_free_tasks(conn, lun);
iscsi_conn_close_lun(conn, lun_id);
}

180
test/unit/lib/iscsi/conn.c/conn_ut.c
View File

@ -44,6 +44,10 @@ SPDK_LOG_REGISTER_COMPONENT("iscsi", SPDK_LOG_ISCSI)
#define DMIN32(A,B) ((uint32_t) ((uint32_t)(A) > (uint32_t)(B) ? (uint32_t)(B) : (uint32_t)(A)))
struct spdk_scsi_lun {
uint8_t reserved;
};
struct spdk_iscsi_globals g_spdk_iscsi;
static TAILQ_HEAD(read_tasks_head, spdk_iscsi_task) g_ut_read_tasks =
TAILQ_HEAD_INITIALIZER(g_ut_read_tasks);
@ -89,7 +93,11 @@ DEFINE_STUB(spdk_sock_group_add_sock, int,
DEFINE_STUB(spdk_sock_group_remove_sock, int,
(struct spdk_sock_group *group, struct spdk_sock *sock), 0);
DEFINE_STUB_V(spdk_scsi_task_put, (struct spdk_scsi_task *task));
void
spdk_scsi_task_put(struct spdk_scsi_task *task)
{
task->ref--;
}
DEFINE_STUB(spdk_scsi_dev_get_lun, struct spdk_scsi_lun *,
(struct spdk_scsi_dev *dev, int lun_id), NULL);
@ -387,6 +395,173 @@ recursive_flush_pdus_calls(void)
CU_ASSERT(rc == 0);
}
static bool
dequeue_pdu(void *_head, struct spdk_iscsi_pdu *pdu)
{
TAILQ_HEAD(queued_pdus, spdk_iscsi_pdu) *head = _head;
struct spdk_iscsi_pdu *tmp;
TAILQ_FOREACH(tmp, head, tailq) {
if (tmp == pdu) {
TAILQ_REMOVE(head, tmp, tailq);
return true;
}
}
return false;
}
static bool
dequeue_task(void *_head, struct spdk_iscsi_task *task)
{
TAILQ_HEAD(queued_tasks, spdk_iscsi_task) *head = _head;
struct spdk_iscsi_task *tmp;
TAILQ_FOREACH(tmp, head, link) {
if (tmp == task) {
TAILQ_REMOVE(head, tmp, link);
return true;
}
}
return false;
}
static void
free_tasks_on_connection(void)
{
struct spdk_iscsi_conn conn = {};
struct spdk_iscsi_pdu pdu1 = {}, pdu2 = {}, pdu3 = {}, pdu4 = {};
struct spdk_iscsi_task task1 = {}, task2 = {}, task3 = {};
struct spdk_scsi_lun lun1 = {}, lun2 = {};
TAILQ_INIT(&conn.write_pdu_list);
TAILQ_INIT(&conn.snack_pdu_list);
TAILQ_INIT(&conn.queued_datain_tasks);
pdu1.task = &task1;
pdu2.task = &task2;
pdu3.task = &task3;
task1.scsi.lun = &lun1;
task2.scsi.lun = &lun2;
task1.is_queued = false;
task2.is_queued = false;
task3.is_queued = true;
/* Test conn->write_pdu_list. */
task1.scsi.ref = 1;
task2.scsi.ref = 1;
task3.scsi.ref = 1;
TAILQ_INSERT_TAIL(&conn.write_pdu_list, &pdu1, tailq);
TAILQ_INSERT_TAIL(&conn.write_pdu_list, &pdu2, tailq);
TAILQ_INSERT_TAIL(&conn.write_pdu_list, &pdu3, tailq);
TAILQ_INSERT_TAIL(&conn.write_pdu_list, &pdu4, tailq);
/* Free all PDUs when exiting connection. */
_iscsi_conn_free_tasks(&conn, NULL);
CU_ASSERT(TAILQ_EMPTY(&conn.write_pdu_list));
CU_ASSERT(task1.scsi.ref == 0);
CU_ASSERT(task2.scsi.ref == 0);
CU_ASSERT(task3.scsi.ref == 0);
task1.scsi.ref = 1;
task2.scsi.ref = 1;
task3.scsi.ref = 1;
TAILQ_INSERT_TAIL(&conn.write_pdu_list, &pdu1, tailq);
TAILQ_INSERT_TAIL(&conn.write_pdu_list, &pdu2, tailq);
TAILQ_INSERT_TAIL(&conn.write_pdu_list, &pdu3, tailq);
TAILQ_INSERT_TAIL(&conn.write_pdu_list, &pdu4, tailq);
/* Free PDUs whose LUN matches the passed LUN or is NULL. */
_iscsi_conn_free_tasks(&conn, &lun1);
CU_ASSERT(!dequeue_pdu(&conn.write_pdu_list, &pdu1));
CU_ASSERT(dequeue_pdu(&conn.write_pdu_list, &pdu2));
CU_ASSERT(!dequeue_pdu(&conn.write_pdu_list, &pdu3));
CU_ASSERT(dequeue_pdu(&conn.write_pdu_list, &pdu4));
CU_ASSERT(task1.scsi.ref == 0);
CU_ASSERT(task2.scsi.ref == 1);
CU_ASSERT(task3.scsi.ref == 0);
/* Test conn->snack_pdu_list */
task1.scsi.ref = 1;
task2.scsi.ref = 1;
task3.scsi.ref = 1;
TAILQ_INSERT_TAIL(&conn.snack_pdu_list, &pdu1, tailq);
TAILQ_INSERT_TAIL(&conn.snack_pdu_list, &pdu2, tailq);
TAILQ_INSERT_TAIL(&conn.snack_pdu_list, &pdu3, tailq);
TAILQ_INSERT_TAIL(&conn.snack_pdu_list, &pdu4, tailq);
/* Free all PDUs and associated tasks when exiting connection. */
_iscsi_conn_free_tasks(&conn, NULL);
CU_ASSERT(!dequeue_pdu(&conn.snack_pdu_list, &pdu1));
CU_ASSERT(!dequeue_pdu(&conn.snack_pdu_list, &pdu2));
CU_ASSERT(!dequeue_pdu(&conn.snack_pdu_list, &pdu3));
CU_ASSERT(!dequeue_pdu(&conn.snack_pdu_list, &pdu4));
CU_ASSERT(task1.scsi.ref == 0);
CU_ASSERT(task2.scsi.ref == 0);
CU_ASSERT(task3.scsi.ref == 0);
task1.scsi.ref = 1;
task2.scsi.ref = 1;
task3.scsi.ref = 1;
TAILQ_INSERT_TAIL(&conn.snack_pdu_list, &pdu1, tailq);
TAILQ_INSERT_TAIL(&conn.snack_pdu_list, &pdu2, tailq);
TAILQ_INSERT_TAIL(&conn.snack_pdu_list, &pdu3, tailq);
TAILQ_INSERT_TAIL(&conn.snack_pdu_list, &pdu4, tailq);
/* Free all PDUs and free associated tasks whose lun matches the passed LUN. */
_iscsi_conn_free_tasks(&conn, &lun1);
CU_ASSERT(!dequeue_pdu(&conn.snack_pdu_list, &pdu1));
CU_ASSERT(!dequeue_pdu(&conn.snack_pdu_list, &pdu2));
CU_ASSERT(!dequeue_pdu(&conn.snack_pdu_list, &pdu3));
CU_ASSERT(!dequeue_pdu(&conn.snack_pdu_list, &pdu4));
CU_ASSERT(task1.scsi.ref == 0);
CU_ASSERT(task2.scsi.ref == 1);
CU_ASSERT(task3.scsi.ref == 1);
/* Test conn->queued_datain_tasks */
task1.scsi.ref = 1;
task2.scsi.ref = 1;
task3.scsi.ref = 1;
TAILQ_INSERT_TAIL(&conn.queued_datain_tasks, &task1, link);
TAILQ_INSERT_TAIL(&conn.queued_datain_tasks, &task2, link);
TAILQ_INSERT_TAIL(&conn.queued_datain_tasks, &task3, link);
/* Free all tasks which is not queued when exiting connection. */
_iscsi_conn_free_tasks(&conn, NULL);
CU_ASSERT(!dequeue_task(&conn.queued_datain_tasks, &task1));
CU_ASSERT(!dequeue_task(&conn.queued_datain_tasks, &task2));
CU_ASSERT(dequeue_task(&conn.queued_datain_tasks, &task3));
CU_ASSERT(task1.scsi.ref == 0);
CU_ASSERT(task2.scsi.ref == 0);
CU_ASSERT(task3.scsi.ref == 1);
task1.scsi.ref = 1;
task2.scsi.ref = 1;
task3.scsi.ref = 1;
TAILQ_INSERT_TAIL(&conn.queued_datain_tasks, &task1, link);
TAILQ_INSERT_TAIL(&conn.queued_datain_tasks, &task2, link);
TAILQ_INSERT_TAIL(&conn.queued_datain_tasks, &task3, link);
/* Free all tasks which is not queued and.whose LUN matches the passed LUN. */
_iscsi_conn_free_tasks(&conn, &lun1);
CU_ASSERT(!dequeue_task(&conn.queued_datain_tasks, &task1));
CU_ASSERT(dequeue_task(&conn.queued_datain_tasks, &task2));
CU_ASSERT(dequeue_task(&conn.queued_datain_tasks, &task3));
CU_ASSERT(task1.scsi.ref == 0);
CU_ASSERT(task2.scsi.ref == 1);
CU_ASSERT(task3.scsi.ref == 1);
}
int
main(int argc, char **argv)
{
@ -409,7 +584,8 @@ main(int argc, char **argv)
read_task_split_reverse_order_case) == NULL ||
CU_add_test(suite, "propagate_scsi_error_status_for_split_read_tasks",
propagate_scsi_error_status_for_split_read_tasks) == NULL ||
CU_add_test(suite, "recursive_flush_pdus_calls", recursive_flush_pdus_calls) == NULL
CU_add_test(suite, "recursive_flush_pdus_calls", recursive_flush_pdus_calls) == NULL ||
CU_add_test(suite, "free_tasks_on_connection", free_tasks_on_connection) == NULL
) {
CU_cleanup_registry();
return CU_get_error();