[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b19e0b0f-6b1c-4126-931b-d7b0d01fb3da@suse.de>
Date: Tue, 6 Feb 2024 14:51:24 +0900
From: Hannes Reinecke <hare@...e.de>
To: Daniel Wagner <dwagner@...e.de>, James Smart <james.smart@...adcom.com>
Cc: Keith Busch <kbusch@...nel.org>, Christoph Hellwig <hch@....de>,
linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 12/12] nvmet-fc: use RCU list iterator for assoc_list
On 1/31/24 16:51, Daniel Wagner wrote:
> The assoc_list is a RCU protected list, thus use the RCU flavor of list
> functions.
>
> Let's use this opportunity and refactor this code and move the lookup
> into a helper and give it a descriptive name.
>
> Signed-off-by: Daniel Wagner <dwagner@...e.de>
> ---
> drivers/nvme/target/fc.c | 34 ++++++++++++++++++++++------------
> 1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
> index 671d096745a5..fd229f310c93 100644
> --- a/drivers/nvme/target/fc.c
> +++ b/drivers/nvme/target/fc.c
> @@ -1114,14 +1114,27 @@ nvmet_fc_schedule_delete_assoc(struct nvmet_fc_tgt_assoc *assoc)
> queue_work(nvmet_wq, &assoc->del_work);
> }
>
> +static bool
> +nvmet_fc_assoc_exits(struct nvmet_fc_tgtport *tgtport, u64 association_id)
> +{
> + struct nvmet_fc_tgt_assoc *a;
> +
> + list_for_each_entry_rcu(a, &tgtport->assoc_list, a_list) {
> + if (association_id == a->association_id)
> + return true;
> + }
> +
> + return false;
> +}
> +
> static struct nvmet_fc_tgt_assoc *
> nvmet_fc_alloc_target_assoc(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
> {
> - struct nvmet_fc_tgt_assoc *assoc, *tmpassoc;
> + struct nvmet_fc_tgt_assoc *assoc;
> unsigned long flags;
> + bool done;
> u64 ran;
> int idx;
> - bool needrandom = true;
>
> if (!tgtport->pe)
> return NULL;
> @@ -1145,24 +1158,21 @@ nvmet_fc_alloc_target_assoc(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
> INIT_WORK(&assoc->del_work, nvmet_fc_delete_assoc_work);
> atomic_set(&assoc->terminating, 0);
>
> - while (needrandom) {
> + done = false;
> + do {
> get_random_bytes(&ran, sizeof(ran) - BYTES_FOR_QID);
> ran = ran << BYTES_FOR_QID_SHIFT;
>
> spin_lock_irqsave(&tgtport->lock, flags);
> - needrandom = false;
> - list_for_each_entry(tmpassoc, &tgtport->assoc_list, a_list) {
> - if (ran == tmpassoc->association_id) {
> - needrandom = true;
> - break;
> - }
> - }
> - if (!needrandom) {
> + rcu_read_lock();
> + if (!nvmet_fc_assoc_exits(tgtport, ran)) {
> assoc->association_id = ran;
> list_add_tail_rcu(&assoc->a_list, &tgtport->assoc_list);
> + done = true;
> }
> + rcu_read_unlock();
> spin_unlock_irqrestore(&tgtport->lock, flags);
Odd. You already take the tgtport lock, so there really is no point in
using rcu_read_lock() here.
Or does the tgtport lock not protect the assoc_list?
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@...e.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Ivo Totev, Andrew McDonald,
Werner Knoblich
Powered by blists - more mailing lists