From: Christoph Hellwig <hch@lst.de>
To: "Nicholas A. Bellinger" <nab@daterainc.com>
Cc: target-devel <target-devel@vger.kernel.org>,
linux-scsi <linux-scsi@vger.kernel.org>,
linux-kernel <linux-kernel@vger.kernel.org>,
Hannes Reinecke <hare@suse.de>, Christoph Hellwig <hch@lst.de>,
Sagi Grimberg <sagig@mellanox.com>,
Nicholas Bellinger <nab@linux-iscsi.org>
Subject: Re: [PATCH 07/12] target/pr: Convert registration check to RCU pointer
Date: Wed, 13 May 2015 08:13:39 +0200 [thread overview]
Message-ID: <20150513061339.GA21390@lst.de> (raw)
In-Reply-To: <1431422736-29125-8-git-send-email-nab@daterainc.com>
On Tue, May 12, 2015 at 09:25:31AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch converts core_scsi3_pr_seq_non_holder() check for non
> reservation holding registrations to use se_deve->pr_reg as an RCU
> protected pointer.
->pr_reg is only used a boolean flag:
drivers/target/target_core_device.c: rcu_assign_pointer(orig->pr_reg, NULL);
drivers/target/target_core_pr.c: registered = (se_deve->pr_reg
!= NULL);
drivers/target/target_core_pr.c:
rcu_assign_pointer(deve->pr_reg, pr_reg);
drivers/target/target_core_pr.c: * conditional checks for deve->pr_reg
pointer access complete.
drivers/target/target_core_pr.c:
rcu_assign_pointer(deve->pr_reg, pr_reg_tmp);
drivers/target/target_core_pr.c: * conditional checks for
deve->pr_reg pointer access complete.
drivers/target/target_core_pr.c:
rcu_assign_pointer(deve->pr_reg, NULL);
drivers/target/target_core_pr.c: * conditional checks for deve->pr_reg
pointer access complete.
so no need to any RCU magic here, it's never dereferences and can be
replace with a scalar, or probably better an atomic bitop.
>
> It also includes associated rcu_assign_pointer() + synchronize_rcu()
> in __core_scsi3_add_registration() and __core_scsi3_free_registration()
> for the RCU updater path.
>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
> drivers/target/target_core_device.c | 1 +
> drivers/target/target_core_pr.c | 75 +++++++++++++++++++++++++++----------
> include/target/target_core_base.h | 4 +-
> 3 files changed, 59 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> index 911758b..430d3d6 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -448,6 +448,7 @@ int core_disable_device_list_for_node(
> * Disable struct se_dev_entry LUN ACL mapping
> */
> core_scsi3_ua_release_all(orig);
> + rcu_assign_pointer(orig->pr_reg, NULL);
> rcu_assign_pointer(orig->se_lun, NULL);
> rcu_assign_pointer(orig->se_lun_acl, NULL);
> orig->lun_flags = 0;
> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index c0b593a..491043d 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -327,9 +327,13 @@ static int core_scsi3_pr_seq_non_holder(
> int we = 0; /* Write Exclusive */
> int legacy = 0; /* Act like a legacy device and return
> * RESERVATION CONFLICT on some CDBs */
> + bool registered = false;
>
> rcu_read_lock();
> se_deve = target_nacl_find_deve(nacl, cmd->orig_fe_lun);
> + if (se_deve)
> + registered = (se_deve->pr_reg != NULL);
> + rcu_read_unlock();
> /*
> * Determine if the registration should be ignored due to
> * non-matching ISIDs in target_scsi3_pr_reservation_check().
> @@ -346,7 +350,7 @@ static int core_scsi3_pr_seq_non_holder(
> * Some commands are only allowed for the persistent reservation
> * holder.
> */
> - if ((se_deve->def_pr_registered) && !(ignore_reg))
> + if ((registered) && !(ignore_reg))
> registered_nexus = 1;
> break;
> case PR_TYPE_WRITE_EXCLUSIVE_REGONLY:
> @@ -356,7 +360,7 @@ static int core_scsi3_pr_seq_non_holder(
> * Some commands are only allowed for registered I_T Nexuses.
> */
> reg_only = 1;
> - if ((se_deve->def_pr_registered) && !(ignore_reg))
> + if ((registered) && !(ignore_reg))
> registered_nexus = 1;
> break;
> case PR_TYPE_WRITE_EXCLUSIVE_ALLREG:
> @@ -366,14 +370,12 @@ static int core_scsi3_pr_seq_non_holder(
> * Each registered I_T Nexus is a reservation holder.
> */
> all_reg = 1;
> - if ((se_deve->def_pr_registered) && !(ignore_reg))
> + if ((registered) && !(ignore_reg))
> registered_nexus = 1;
> break;
> default:
> - rcu_read_unlock();
> return -EINVAL;
> }
> - rcu_read_unlock();
> /*
> * Referenced from spc4r17 table 45 for *NON* PR holder access
> */
> @@ -1009,10 +1011,6 @@ static void __core_scsi3_dump_registration(
> pr_reg->pr_reg_aptpl);
> }
>
> -/*
> - * this function can be called with struct se_device->dev_reservation_lock
> - * when register_move = 1
> - */
> static void __core_scsi3_add_registration(
> struct se_device *dev,
> struct se_node_acl *nacl,
> @@ -1023,6 +1021,7 @@ static void __core_scsi3_add_registration(
> const struct target_core_fabric_ops *tfo = nacl->se_tpg->se_tpg_tfo;
> struct t10_pr_registration *pr_reg_tmp, *pr_reg_tmp_safe;
> struct t10_reservation *pr_tmpl = &dev->t10_pr;
> + struct se_dev_entry *deve;
>
> /*
> * Increment PRgeneration counter for struct se_device upon a successful
> @@ -1039,10 +1038,22 @@ static void __core_scsi3_add_registration(
>
> spin_lock(&pr_tmpl->registration_lock);
> list_add_tail(&pr_reg->pr_reg_list, &pr_tmpl->registration_list);
> - pr_reg->pr_reg_deve->def_pr_registered = 1;
>
> __core_scsi3_dump_registration(tfo, dev, nacl, pr_reg, register_type);
> spin_unlock(&pr_tmpl->registration_lock);
> +
> + mutex_lock(&nacl->lun_entry_mutex);
> + deve = target_nacl_find_deve(nacl, pr_reg->pr_res_mapped_lun);
> + if (deve)
> + rcu_assign_pointer(deve->pr_reg, pr_reg);
> + mutex_unlock(&nacl->lun_entry_mutex);
> +
> + /*
> + * Wait for read path critical RCU in core_scsi3_pr_seq_non_holder()
> + * conditional checks for deve->pr_reg pointer access complete.
> + */
> + synchronize_rcu();
> +
> /*
> * Skip extra processing for ALL_TG_PT=0 or REGISTER_AND_MOVE.
> */
> @@ -1054,6 +1065,8 @@ static void __core_scsi3_add_registration(
> */
> list_for_each_entry_safe(pr_reg_tmp, pr_reg_tmp_safe,
> &pr_reg->pr_reg_atp_list, pr_reg_atp_mem_list) {
> + struct se_node_acl *nacl_tmp = pr_reg_tmp->pr_reg_nacl;
> +
> list_del(&pr_reg_tmp->pr_reg_atp_mem_list);
>
> pr_reg_tmp->pr_res_generation = core_scsi3_pr_generation(dev);
> @@ -1061,13 +1074,23 @@ static void __core_scsi3_add_registration(
> spin_lock(&pr_tmpl->registration_lock);
> list_add_tail(&pr_reg_tmp->pr_reg_list,
> &pr_tmpl->registration_list);
> - pr_reg_tmp->pr_reg_deve->def_pr_registered = 1;
>
> - __core_scsi3_dump_registration(tfo, dev,
> - pr_reg_tmp->pr_reg_nacl, pr_reg_tmp,
> - register_type);
> + __core_scsi3_dump_registration(tfo, dev, nacl_tmp, pr_reg_tmp,
> + register_type);
> spin_unlock(&pr_tmpl->registration_lock);
>
> + mutex_lock(&nacl->lun_entry_mutex);
> + deve = target_nacl_find_deve(nacl_tmp, pr_reg_tmp->pr_res_mapped_lun);
> + if (deve)
> + rcu_assign_pointer(deve->pr_reg, pr_reg_tmp);
> + mutex_unlock(&nacl->lun_entry_mutex);
> +
> + /*
> + * Wait for read path critical RCU in core_scsi3_pr_seq_non_holder()
> + * conditional checks for deve->pr_reg pointer access complete.
> + */
> + synchronize_rcu();
> +
> /*
> * Drop configfs group dependency reference from
> * __core_scsi3_alloc_registration()
> @@ -1243,13 +1266,13 @@ static void __core_scsi3_free_registration(
> const struct target_core_fabric_ops *tfo =
> pr_reg->pr_reg_nacl->se_tpg->se_tpg_tfo;
> struct t10_reservation *pr_tmpl = &dev->t10_pr;
> + struct se_node_acl *nacl = pr_reg->pr_reg_nacl;
> + struct se_dev_entry *deve;
> char i_buf[PR_REG_ISID_ID_LEN];
>
> memset(i_buf, 0, PR_REG_ISID_ID_LEN);
> core_pr_dump_initiator_port(pr_reg, i_buf, PR_REG_ISID_ID_LEN);
>
> - pr_reg->pr_reg_deve->def_pr_registered = 0;
> - pr_reg->pr_reg_deve->pr_res_key = 0;
> if (!list_empty(&pr_reg->pr_reg_list))
> list_del(&pr_reg->pr_reg_list);
> /*
> @@ -1258,6 +1281,8 @@ static void __core_scsi3_free_registration(
> */
> if (dec_holders)
> core_scsi3_put_pr_reg(pr_reg);
> +
> + spin_unlock(&pr_tmpl->registration_lock);
> /*
> * Wait until all reference from any other I_T nexuses for this
> * *pr_reg have been released. Because list_del() is called above,
> @@ -1265,13 +1290,24 @@ static void __core_scsi3_free_registration(
> * count back to zero, and we release *pr_reg.
> */
> while (atomic_read(&pr_reg->pr_res_holders) != 0) {
> - spin_unlock(&pr_tmpl->registration_lock);
> pr_debug("SPC-3 PR [%s] waiting for pr_res_holders\n",
> tfo->get_fabric_name());
> cpu_relax();
> - spin_lock(&pr_tmpl->registration_lock);
> }
>
> + mutex_lock(&nacl->lun_entry_mutex);
> + deve = target_nacl_find_deve(nacl, pr_reg->pr_res_mapped_lun);
> + if (deve)
> + rcu_assign_pointer(deve->pr_reg, NULL);
> + mutex_unlock(&nacl->lun_entry_mutex);
> +
> + /*
> + * Wait for read path critical RCU in core_scsi3_pr_seq_non_holder()
> + * conditional checks for deve->pr_reg pointer access complete.
> + */
> + synchronize_rcu();
> +
> + spin_lock(&pr_tmpl->registration_lock);
> pr_debug("SPC-3 PR [%s] Service Action: UNREGISTER Initiator"
> " Node: %s%s\n", tfo->get_fabric_name(),
> pr_reg->pr_reg_nacl->initiatorname,
> @@ -3428,13 +3464,14 @@ after_iport_check:
> dest_pr_reg = __core_scsi3_locate_pr_reg(dev, dest_node_acl,
> iport_ptr);
> if (!dest_pr_reg) {
> + spin_unlock(&dev->dev_reservation_lock);
> if (core_scsi3_alloc_registration(cmd->se_dev,
> dest_node_acl, dest_se_deve, iport_ptr,
> sa_res_key, 0, aptpl, 2, 1)) {
> - spin_unlock(&dev->dev_reservation_lock);
> ret = TCM_INVALID_PARAMETER_LIST;
> goto out;
> }
> + spin_lock(&dev->dev_reservation_lock);
> dest_pr_reg = __core_scsi3_locate_pr_reg(dev, dest_node_acl,
> iport_ptr);
> new_reg = 1;
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index 1b06d27..2f0c830 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -640,7 +640,6 @@ struct se_lun_acl {
> };
>
> struct se_dev_entry {
> - bool def_pr_registered;
> /* See transport_lunflags_table */
> u32 lun_flags;
> u32 mapped_lun;
> @@ -657,7 +656,8 @@ struct se_dev_entry {
> struct se_node_acl *se_node_acl;
> struct se_lun_acl __rcu *se_lun_acl;
> spinlock_t ua_lock;
> - struct se_lun *se_lun;
> + struct se_lun __rcu *se_lun;
> + struct t10_pr_registration __rcu *pr_reg;
> struct list_head alua_port_list;
> struct list_head ua_list;
> struct hlist_node link;
> --
> 1.9.1
---end quoted text---
next prev parent reply other threads:[~2015-05-13 6:13 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-12 9:25 [PATCH 00/12] target: TPG/NodeACL LUN table conversion to RCU hlist Nicholas A. Bellinger
2015-05-12 9:25 ` [PATCH 01/12] target: Convert se_node_acl->device_list[] " Nicholas A. Bellinger
2015-05-12 20:58 ` Andy Grover
2015-05-13 5:08 ` Nicholas A. Bellinger
2015-05-13 5:32 ` Christoph Hellwig
2015-05-13 5:41 ` Nicholas A. Bellinger
2015-05-13 5:46 ` Christoph Hellwig
2015-05-13 6:20 ` Nicholas A. Bellinger
2015-05-13 6:48 ` Christoph Hellwig
2015-05-13 6:35 ` Christoph Hellwig
2015-05-13 8:46 ` Nicholas A. Bellinger
2015-05-17 16:51 ` Christoph Hellwig
2015-05-18 7:17 ` Nicholas A. Bellinger
2015-05-18 7:41 ` Christoph Hellwig
2015-05-18 8:01 ` Christoph Hellwig
2015-05-19 6:05 ` Nicholas A. Bellinger
2015-05-19 6:22 ` Christoph Hellwig
2015-05-21 17:03 ` Christoph Hellwig
2015-05-21 18:10 ` Nicholas A. Bellinger
2015-05-12 9:25 ` [PATCH 02/12] target: Convert REPORT_LUN + MODE_SENSE to RCU reader Nicholas A. Bellinger
2015-05-13 5:47 ` Christoph Hellwig
2015-05-13 8:10 ` Nicholas A. Bellinger
2015-05-12 9:25 ` [PATCH 03/12] target/configfs: Convert mappedlun + SCSI MIBs " Nicholas A. Bellinger
2015-05-12 20:58 ` Andy Grover
2015-05-13 5:09 ` Nicholas A. Bellinger
2015-05-13 5:49 ` Christoph Hellwig
2015-05-12 9:25 ` [PATCH 04/12] target: Convert UNIT_ATTENTION logic " Nicholas A. Bellinger
2015-05-12 9:25 ` [PATCH 05/12] target: Convert transport_lookup_*_lun " Nicholas A. Bellinger
2015-05-13 5:55 ` Christoph Hellwig
2015-05-13 7:42 ` Nicholas A. Bellinger
2015-05-12 9:25 ` [PATCH 06/12] target/pr: Convert se_dev_entry to kref for RCU Nicholas A. Bellinger
2015-05-13 5:59 ` Christoph Hellwig
2015-05-12 9:25 ` [PATCH 07/12] target/pr: Convert registration check to RCU pointer Nicholas A. Bellinger
2015-05-13 6:13 ` Christoph Hellwig [this message]
2015-05-12 9:25 ` [PATCH 08/12] target/pr: Change alloc_registration to avoid pr_reg_tg_pt_lun Nicholas A. Bellinger
2015-05-12 9:25 ` [PATCH 09/12] target: Convert se_portal_group->tpg_lun_list[] to RCU hlist Nicholas A. Bellinger
2015-05-13 6:24 ` Christoph Hellwig
2015-05-13 7:22 ` Juergen Gross
2015-05-13 7:53 ` Christoph Hellwig
2015-05-19 6:46 ` Christoph Hellwig
2015-05-12 9:25 ` [PATCH 10/12] target: Convert se_tpg->acl_node_lock to ->acl_node_mutex Nicholas A. Bellinger
2015-05-12 9:25 ` [PATCH 11/12] target: Convert core_tpg_deregister to use list splice Nicholas A. Bellinger
2015-05-12 9:25 ` [PATCH 12/12] target: Drop unused se_lun->lun_acl_list Nicholas A. Bellinger
2015-05-13 6:29 ` [PATCH 00/12] target: TPG/NodeACL LUN table conversion to RCU hlist Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150513061339.GA21390@lst.de \
--to=hch@lst.de \
--cc=hare@suse.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=nab@daterainc.com \
--cc=nab@linux-iscsi.org \
--cc=sagig@mellanox.com \
--cc=target-devel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).