by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH 2/4] target: Drop lun_sep_lock for se_lun->lun_se_dev RCU usage

> @@ -683,7 +679,7 @@ void core_tpg_remove_lun(
> dev->export_count--;
> spin_unlock(&dev->se_port_lock);
>
> - lun->lun_se_dev = NULL;
> + rcu_assign_pointer(lun->lun_se_dev, NULL);
> }

What guarantees that the se_device stays around for at least a RCU
grace period after this assignment?

2015-05-25 22:59:27

by Nicholas A. Bellinger

[permalink] [raw]

Subject: Re: [PATCH 2/4] target: Drop lun_sep_lock for se_lun->lun_se_dev RCU usage

On Fri, 2015-05-22 at 13:56 +0200, Christoph Hellwig wrote:
> > @@ -683,7 +679,7 @@ void core_tpg_remove_lun(
> > dev->export_count--;
> > spin_unlock(&dev->se_port_lock);
> >
> > - lun->lun_se_dev = NULL;
> > + rcu_assign_pointer(lun->lun_se_dev, NULL);
> > }
>
> What guarantees that the se_device stays around for at least a RCU
> grace period after this assignment?

Nothing.

Since se_device is embedded in the backend driver device structure, this
currently requires that each driver do it's own call_rcu().

>From c933729d347fcb3d226500346ed06deb011e73bb Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger <[email protected]>
Date: Sat, 23 May 2015 23:35:56 -0700
Subject: [PATCH] target: Convert to backend driver call_rcu release

Reported-by: Christoph Hellwig <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
drivers/target/target_core_device.c | 1 +
drivers/target/target_core_file.c | 11 +++++++++--
drivers/target/target_core_iblock.c | 10 +++++++++-
drivers/target/target_core_pscsi.c | 11 +++++++++--
drivers/target/target_core_rd.c | 10 +++++++++-
drivers/target/target_core_stat.c | 6 +++---
drivers/target/target_core_user.c | 11 +++++++++--
include/target/target_core_base.h | 3 +++
8 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 50314b6..75c6324 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -733,6 +733,7 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
dev->se_hba = hba;
dev->transport = hba->backend->ops;
dev->prot_length = sizeof(struct se_dif_v1_tuple);
+ dev->hba_index = hba->hba_index;

INIT_LIST_HEAD(&dev->dev_list);
INIT_LIST_HEAD(&dev->dev_sep_list);
diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 90e09ba..ac9cbf1 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -241,6 +241,14 @@ fail:
return ret;
}

+static void fd_dev_call_rcu(struct rcu_head *p)
+{
+ struct se_device *dev = container_of(p, struct se_device, rcu_head);
+ struct fd_dev *fd_dev = FD_DEV(dev);
+
+ kfree(fd_dev);
+}
+
static void fd_free_device(struct se_device *dev)
{
struct fd_dev *fd_dev = FD_DEV(dev);
@@ -249,8 +257,7 @@ static void fd_free_device(struct se_device *dev)
filp_close(fd_dev->fd_file, NULL);
fd_dev->fd_file = NULL;
}
-
- kfree(fd_dev);
+ call_rcu(&dev->rcu_head, fd_dev_call_rcu);
}

static int fd_do_rw(struct se_cmd *cmd, struct file *fd,
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index bd9dcd8..1a78e31 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -191,6 +191,14 @@ out:
return ret;
}

+static void iblock_dev_call_rcu(struct rcu_head *p)
+{
+ struct se_device *dev = container_of(p, struct se_device, rcu_head);
+ struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
+
+ kfree(ib_dev);
+}
+
static void iblock_free_device(struct se_device *dev)
{
struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
@@ -200,7 +208,7 @@ static void iblock_free_device(struct se_device *dev)
if (ib_dev->ibd_bio_set != NULL)
bioset_free(ib_dev->ibd_bio_set);

- kfree(ib_dev);
+ call_rcu(&dev->rcu_head, iblock_dev_call_rcu);
}

static unsigned long long iblock_emulate_read_cap_with_block_size(
diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index 5bc458e..c710ff0 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -578,6 +578,14 @@ static int pscsi_configure_device(struct se_device *dev)
return -ENODEV;
}

+static void pscsi_dev_call_rcu(struct rcu_head *p)
+{
+ struct se_device *dev = container_of(p, struct se_device, rcu_head);
+ struct pscsi_dev_virt *pdv = PSCSI_DEV(dev);
+
+ kfree(pdv);
+}
+
static void pscsi_free_device(struct se_device *dev)
{
struct pscsi_dev_virt *pdv = PSCSI_DEV(dev);
@@ -607,8 +615,7 @@ static void pscsi_free_device(struct se_device *dev)

pdv->pdv_sd = NULL;
}
-
- kfree(pdv);
+ call_rcu(&dev->rcu_head, pscsi_dev_call_rcu);
}

static void pscsi_transport_complete(struct se_cmd *cmd, struct scatterlist *sg,
diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
index 5f84144..dd495b6 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -350,12 +350,20 @@ fail:
return ret;
}

+static void rd_dev_call_rcu(struct rcu_head *p)
+{
+ struct se_device *dev = container_of(p, struct se_device, rcu_head);
+ struct rd_dev *rd_dev = RD_DEV(dev);
+
+ kfree(rd_dev);
+}
+
static void rd_free_device(struct se_device *dev)
{
struct rd_dev *rd_dev = RD_DEV(dev);

rd_release_device_space(rd_dev);
- kfree(rd_dev);
+ call_rcu(&dev->rcu_head, rd_dev_call_rcu);
}

static struct rd_dev_sg_table *rd_get_sg_table(struct rd_dev *rd_dev, u32 page)
diff --git a/drivers/target/target_core_stat.c b/drivers/target/target_core_stat.c
index d38a18e..6d675b5 100644
--- a/drivers/target/target_core_stat.c
+++ b/drivers/target/target_core_stat.c
@@ -548,7 +548,7 @@ static ssize_t target_stat_scsi_port_show_attr_inst(
rcu_read_lock();
dev = lun->lun_se_dev;
if (dev)
- ret = snprintf(page, PAGE_SIZE, "%u\n", dev->se_hba->hba_index);
+ ret = snprintf(page, PAGE_SIZE, "%u\n", dev->hba_index);
rcu_read_unlock();
return ret;
}
@@ -667,7 +667,7 @@ static ssize_t target_stat_scsi_tgt_port_show_attr_inst(
rcu_read_lock();
dev = lun->lun_se_dev;
if (dev)
- ret = snprintf(page, PAGE_SIZE, "%u\n", dev->se_hba->hba_index);
+ ret = snprintf(page, PAGE_SIZE, "%u\n", dev->hba_index);
rcu_read_unlock();
return ret;
}
@@ -838,7 +838,7 @@ static ssize_t target_stat_scsi_transport_show_attr_inst(
rcu_read_lock();
dev = lun->lun_se_dev;
if (dev)
- ret = snprintf(page, PAGE_SIZE, "%u\n", dev->se_hba->hba_index);
+ ret = snprintf(page, PAGE_SIZE, "%u\n", dev->hba_index);
rcu_read_unlock();
return ret;
}
diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
index d59df02..6742e53 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -960,6 +960,14 @@ static int tcmu_check_pending_cmd(int id, void *p, void *data)
return -EINVAL;
}

+static void tcmu_dev_call_rcu(struct rcu_head *p)
+{
+ struct se_device *dev = container_of(p, struct se_device, rcu_head);
+ struct tcmu_dev *udev = TCMU_DEV(dev);
+
+ kfree(udev);
+}
+
static void tcmu_free_device(struct se_device *dev)
{
struct tcmu_dev *udev = TCMU_DEV(dev);
@@ -985,8 +993,7 @@ static void tcmu_free_device(struct se_device *dev)
kfree(udev->uio_info.name);
kfree(udev->name);
}
-
- kfree(udev);
+ call_rcu(&dev->rcu_head, tcmu_dev_call_rcu);
}

enum {
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 2b90b7f..382e591 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -822,6 +822,9 @@ struct se_device {
struct se_lun xcopy_lun;
/* Protection Information */
int prot_length;
+ /* For se_lun->lun_se_dev RCU read-side critical access */
+ u32 hba_index;
+ struct rcu_head rcu_head;
};

struct se_hba {