(was Re: [ofa-general] fmr pool free_list empty)

Ugh.

Quoted 7 lines

Looking at the FMR dirty list unmapping code in ib_fmr_batch_release(), there is a section that pulls all the dirty entries onto a list that it will later unmap and put back on the free list.

But it also plans to unmap all the free entries that have ever been remapped:

Yes, this came from a3cd7d90 ("IB/fmr_pool: ib_fmr_pool_flush() should flush all dirty FMRs"). That solved a real problem for Olaf, because otherwise dirty FMRs with not at the max map count might never get invalidated. It's not exactly an optimization but rather a correctness issue, because RDS relies on killing mapping eventually.

On the other hand, this behavior clearly does lead to the possibility of leaving the free list temporarily empty for stupid reasons.

I don't see a really good way to fix this at the momemnt, need to meditate a little.

This reverts commit a3cd7d9070be417a21905c997ee32d756d999b38.

The original commit breaks iSER reliably, making it complain:

iser: iser_reg_page_vec:ib_fmr_pool_map_phys failed: -11

The FMR cleanup thread runs ib_fmr_batch_release() as dirty entries build up. This commit causes clean but used FMR entries also to be purged. During that process, another thread can see that there are no free FMRs and fail, even though there should always have been enough available.

Signed-off-by Pete Wyckoff <[email protected]>

---
 drivers/infiniband/core/fmr_pool.c |   21 ++++++---------------
 1 files changed, 6 insertions(+), 15 deletions(-)

drivers/infiniband/core/fmr_pool.c +6 -15

index 7f00347..4044fdf 100644
@@ -139,7 +139,7 @@ static inline struct ib_pool_fmr *ib_fmr_cache_lookup(struct ib_fmr_pool *pool,
 static void ib_fmr_batch_release(struct ib_fmr_pool *pool)
 {
 	int                 ret;
-	struct ib_pool_fmr *fmr, *next;
+	struct ib_pool_fmr *fmr;
 	LIST_HEAD(unmap_list);
 	LIST_HEAD(fmr_list);
 

@@ -158,20 +158,6 @@ static void ib_fmr_batch_release(struct ib_fmr_pool *pool)
 #endif
 	}
 
-	/*
-	 * The free_list may hold FMRs that have been put there
-	 * because they haven't reached the max_remap count.
-	 * Invalidate their mapping as well.
-	 */
-	list_for_each_entry_safe(fmr, next, &pool->free_list, list) {
-		if (fmr->remap_count == 0)
-			continue;
-		hlist_del_init(&fmr->cache_node);
-		fmr->remap_count = 0;
-		list_add_tail(&fmr->fmr->list, &fmr_list);
-		list_move(&fmr->list, &unmap_list);
-	}
-
 	list_splice(&pool->dirty_list, &unmap_list);
 	INIT_LIST_HEAD(&pool->dirty_list);
 	pool->dirty_len = 0;

@@ -384,6 +370,11 @@ void ib_destroy_fmr_pool(struct ib_fmr_pool *pool)
 
 	i = 0;
 	list_for_each_entry_safe(fmr, tmp, &pool->free_list, list) {
+		if (fmr->remap_count) {
+			INIT_LIST_HEAD(&fmr_list);
+			list_add_tail(&fmr->fmr->list, &fmr_list);
+			ib_unmap_fmr(&fmr_list);
+		}
 		ib_dealloc_fmr(fmr->fmr);
 		list_del(&fmr->list);
 		kfree(fmr);

Pete, the subject says "PATCH 1/2" but I didn't see any follow-up message for PATCH 2/2. Just wondering :)

Quoted 2 lines · Benny Halevy

Pete, the subject says "PATCH 1/2" but I didn't see any follow-up message for PATCH 2/2. Just wondering :)

I think the problem's on your end ... I got it and so did marc:
http://marc.info/?l=linux-scsi&m=120405067313933&w=2

This looks like a really nice approach to me. Olaf?