Commit ab808d2b authored by Shuhei Matsumoto's avatar Shuhei Matsumoto Committed by Tomasz Zawadzki
Browse files

lib/scsi: Pass bdev_name instead of bdev to scsi_lun_construct() 



Pass not bdev but bdev_name to scsi_lun_construct() to fix the
race condition due to the time gap between spdk_bdev_get_by_name()
and spdk_bdev_open(). A pointer to a bdev is valid only while the
bdev is opened.

spdk_bdev_open() has been replaced recently by spdk_bdev_open_ext(),
but the issue still existed.

Signed-off-by: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Change-Id: Ic462422dbc2501c24907f56a36570fbb54fef65b
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/4482


Reviewed-by: default avatarAleksey Marchuk <alexeymar@mellanox.com>
Reviewed-by: default avatarTomasz Zawadzki <tomasz.zawadzki@intel.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
parent 357c9580

lib/scsi/dev.c

+1 −9
Original line number Diff line number Diff line
@@ -151,16 +151,8 @@ spdk_scsi_dev_add_lun_ext(struct spdk_scsi_dev *dev, const char *bdev_name, int 
			  void (*hotremove_cb)(const struct spdk_scsi_lun *, void *),

			  void *hotremove_ctx)

{

	struct spdk_bdev *bdev;

	struct spdk_scsi_lun *lun;



	bdev = spdk_bdev_get_by_name(bdev_name);

	if (bdev == NULL) {

		SPDK_ERRLOG("device %s: cannot find bdev '%s' (target %d)\n",

			    dev->name, bdev_name, lun_id);

		return -1;

	}



	/* Search the lowest free LUN ID if LUN ID is default */

	if (lun_id == -1) {

		lun_id = scsi_dev_find_lowest_free_lun_id(dev);
@@ -170,7 +162,7 @@ spdk_scsi_dev_add_lun_ext(struct spdk_scsi_dev *dev, const char *bdev_name, int 
		}

	}



	lun = scsi_lun_construct(bdev, resize_cb, resize_ctx, hotremove_cb, hotremove_ctx);

	lun = scsi_lun_construct(bdev_name, resize_cb, resize_ctx, hotremove_cb, hotremove_ctx);

	if (lun == NULL) {

		return -1;

	}

lib/scsi/lun.c

+9 −8
Original line number Diff line number Diff line
@@ -418,12 +418,12 @@ bdev_event_cb(enum spdk_bdev_event_type type, struct spdk_bdev *bdev, 
/**

 * \brief Constructs a new spdk_scsi_lun object based on the provided parameters.

 *

 * \param bdev  bdev associated with this LUN

 * \param bdev_name Bdev name to open and associate with this LUN

 *

 * \return NULL if bdev == NULL

 * \return NULL if bdev whose name matches is not found

 * \return pointer to the new spdk_scsi_lun object otherwise

 */

struct spdk_scsi_lun *scsi_lun_construct(struct spdk_bdev *bdev,

struct spdk_scsi_lun *scsi_lun_construct(const char *bdev_name,

		void (*resize_cb)(const struct spdk_scsi_lun *, void *),

		void *resize_ctx,

		void (*hotremove_cb)(const struct spdk_scsi_lun *, void *),
@@ -432,8 +432,8 @@ struct spdk_scsi_lun *scsi_lun_construct(struct spdk_bdev *bdev, 
	struct spdk_scsi_lun *lun;

	int rc;



	if (bdev == NULL) {

		SPDK_ERRLOG("bdev must be non-NULL\n");

	if (bdev_name == NULL) {

		SPDK_ERRLOG("bdev_name must be non-NULL\n");

		return NULL;

	}
@@ -443,10 +443,10 @@ struct spdk_scsi_lun *scsi_lun_construct(struct spdk_bdev *bdev, 
		return NULL;

	}



	rc = spdk_bdev_open_ext(spdk_bdev_get_name(bdev), true, bdev_event_cb, lun, &lun->bdev_desc);

	rc = spdk_bdev_open_ext(bdev_name, true, bdev_event_cb, lun, &lun->bdev_desc);



	if (rc != 0) {

		SPDK_ERRLOG("bdev %s cannot be opened, error=%d\n", spdk_bdev_get_name(bdev), rc);

		SPDK_ERRLOG("bdev %s cannot be opened, error=%d\n", bdev_name, rc);

		free(lun);

		return NULL;

	}
@@ -458,7 +458,8 @@ struct spdk_scsi_lun *scsi_lun_construct(struct spdk_bdev *bdev, 
	TAILQ_INIT(&lun->mgmt_tasks);

	TAILQ_INIT(&lun->pending_mgmt_tasks);



	lun->bdev = bdev;

	/* Bdev is not removed while it is opened. */

	lun->bdev = spdk_bdev_desc_get_bdev(lun->bdev_desc);

	lun->io_channel = NULL;

	lun->hotremove_cb = hotremove_cb;

	lun->hotremove_ctx = hotremove_ctx;

lib/scsi/scsi_internal.h

+1 −1
Original line number Diff line number Diff line
@@ -175,7 +175,7 @@ struct spdk_scsi_lun { 
	struct spdk_poller *reset_poller;

};



struct spdk_scsi_lun *scsi_lun_construct(struct spdk_bdev *bdev,

struct spdk_scsi_lun *scsi_lun_construct(const char *bdev_name,

		void (*resize_cb)(const struct spdk_scsi_lun *, void *),

		void *resize_ctx,

		void (*hotremove_cb)(const struct spdk_scsi_lun *, void *),

test/unit/lib/scsi/dev.c/dev_ut.c

+13 −33
Original line number Diff line number Diff line
@@ -43,25 +43,14 @@ 


#include "spdk_internal/mock.h"



/* Unit test bdev mockup */

struct spdk_bdev {

	char name[100];

};



static struct spdk_bdev g_bdevs[] = {

	{"malloc0"},

	{"malloc1"},

static char *g_bdev_names[] = {

	"malloc0",

	"malloc1",

};



static struct spdk_scsi_port *g_initiator_port_with_pending_tasks = NULL;

static struct spdk_scsi_port *g_initiator_port_with_pending_mgmt_tasks = NULL;



const char *

spdk_bdev_get_name(const struct spdk_bdev *bdev)

{

	return bdev->name;

}



static struct spdk_scsi_task *

spdk_get_task(uint32_t *owner_task_ctr)

{
@@ -81,21 +70,26 @@ spdk_scsi_task_put(struct spdk_scsi_task *task) 
	free(task);

}



struct spdk_scsi_lun *scsi_lun_construct(struct spdk_bdev *bdev,

struct spdk_scsi_lun *scsi_lun_construct(const char *bdev_name,

		void (*resize_cb)(const struct spdk_scsi_lun *, void *),

		void *resize_ctx,

		void (*hotremove_cb)(const struct spdk_scsi_lun *, void *),

		void *hotremove_ctx)

{

	struct spdk_scsi_lun *lun;

	size_t i;



	for (i = 0; i < SPDK_COUNTOF(g_bdev_names); i++) {

		if (strcmp(bdev_name, g_bdev_names[i]) == 0) {

			lun = calloc(1, sizeof(struct spdk_scsi_lun));

			SPDK_CU_ASSERT_FATAL(lun != NULL);



	lun->bdev = bdev;



			return lun;

		}

	}



	return NULL;

}



void

scsi_lun_destruct(struct spdk_scsi_lun *lun)
@@ -103,20 +97,6 @@ scsi_lun_destruct(struct spdk_scsi_lun *lun) 
	free(lun);

}



struct spdk_bdev *

spdk_bdev_get_by_name(const char *bdev_name)

{

	size_t i;



	for (i = 0; i < SPDK_COUNTOF(g_bdevs); i++) {

		if (strcmp(bdev_name, g_bdevs[i].name) == 0) {

			return &g_bdevs[i];

		}

	}



	return NULL;

}



DEFINE_STUB_V(scsi_lun_execute_mgmt_task,

	      (struct spdk_scsi_lun *lun, struct spdk_scsi_task *task));

test/unit/lib/scsi/lun.c/lun_ut.c

+6 −6
Original line number Diff line number Diff line
@@ -105,6 +105,9 @@ DEFINE_STUB_V(spdk_bdev_close, (struct spdk_bdev_desc *desc)); 
DEFINE_STUB(spdk_bdev_get_name, const char *,

	    (const struct spdk_bdev *bdev), "test");



DEFINE_STUB(spdk_bdev_desc_get_bdev, struct spdk_bdev *,

	    (struct spdk_bdev_desc *bdev_desc), NULL);



DEFINE_STUB_V(spdk_scsi_dev_queue_mgmt_task,

	      (struct spdk_scsi_dev *dev, struct spdk_scsi_task *task));
@@ -147,9 +150,8 @@ DEFINE_STUB(spdk_bdev_get_io_channel, struct spdk_io_channel *, 
static struct spdk_scsi_lun *lun_construct(void)

{

	struct spdk_scsi_lun		*lun;

	struct spdk_bdev		bdev;



	lun = scsi_lun_construct(&bdev, NULL, NULL, NULL, NULL);

	lun = scsi_lun_construct("ut_bdev", NULL, NULL, NULL, NULL);



	SPDK_CU_ASSERT_FATAL(lun != NULL);

	return lun;
@@ -582,7 +584,6 @@ lun_reset_task_suspend_scsi_task(void) 
static void

lun_check_pending_tasks_only_for_specific_initiator(void)

{

	struct spdk_bdev bdev = {};

	struct spdk_scsi_lun *lun;

	struct spdk_scsi_task task1 = {};

	struct spdk_scsi_task task2 = {};
@@ -590,7 +591,7 @@ lun_check_pending_tasks_only_for_specific_initiator(void) 
	struct spdk_scsi_port initiator_port2 = {};

	struct spdk_scsi_port initiator_port3 = {};



	lun = scsi_lun_construct(&bdev, NULL, NULL, NULL, NULL);

	lun = scsi_lun_construct("ut_bdev", NULL, NULL, NULL, NULL);



	task1.initiator_port = &initiator_port1;

	task2.initiator_port = &initiator_port2;
@@ -652,11 +653,10 @@ lun_check_pending_tasks_only_for_specific_initiator(void) 
static void

abort_pending_mgmt_tasks_when_lun_is_removed(void)

{

	struct spdk_bdev bdev = {};

	struct spdk_scsi_lun *lun;

	struct spdk_scsi_task task1, task2, task3;



	lun = scsi_lun_construct(&bdev, NULL, NULL, NULL, NULL);

	lun = scsi_lun_construct("ut_bdev", NULL, NULL, NULL, NULL);



	/* Normal case */

	ut_init_task(&task1);