diff mbox series

[v5,12/12] nvmet-fc: use RCU list iterator for assoc_list

Message ID 20240131085112.21668-13-dwagner@suse.de
State New
Headers
Series enable nvmet-fc for blktests |

Commit Message

Daniel Wagner Jan. 31, 2024, 8:51 a.m. UTC 
  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@suse.de>
---
 drivers/nvme/target/fc.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

Comments

Christoph Hellwig Jan. 31, 2024, 5:31 p.m. UTC | #1 
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Hannes Reinecke Feb. 6, 2024, 5:51 a.m. UTC | #2 
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@suse.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
Keith Busch Feb. 6, 2024, 7:22 p.m. UTC | #3 
On Tue, Feb 06, 2024 at 02:51:24PM +0900, Hannes Reinecke wrote:
> On 1/31/24 16:51, Daniel Wagner wrote:
> > +		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.

That's a interesting point, but I think it's harmless since there's no
rcu sync within the read section.

  https://www.kernel.org/doc/Documentation/RCU/Design/Requirements/Requirements.html#Guaranteed%20Read-to-Write%20Upgrade

That said, the rcu_read_lock() seems like it should be moved to
nvmet_fc_assoc_exits() since that's the only part reading rcu protected
structures.
Daniel Wagner Feb. 12, 2024, 10:31 a.m. UTC | #4 
On Tue, Feb 06, 2024 at 11:22:49AM -0800, Keith Busch wrote:
> That said, the rcu_read_lock() seems like it should be moved to
> nvmet_fc_assoc_exits() since that's the only part reading rcu protected
> structures.

FTR, here is a fix:

https://lore.kernel.org/linux-nvme/20240212102940.11137-1-dwagner@suse.de/
diff mbox series

Patch

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);
-	}
+	} while (!done);
 
 	return assoc;