lightnvm: pblk: fix changing GC group list for a line

PATCH In review Rakesh Pandit 2 participants 4 messages Sep 28, 2017 – Oct 2, 2017 Block layer

Share & tools

Rakesh Pandit

author

2017-09-28 14:41 UTC #0 lore raw

PATCH 1/1 lightnvm: pblk: fix changing GC group list for a line

pblk_line_gc_list seems to had a bug since the introduction of pblk in getting GC list for a line. In b20ba1bc7 while redesigning GC algorithm it was not fixed correctly. The problem is that even if valid sector count (vsc) is less that mid_thrs (threshold for GC mid list) it would always satisfy 'vsc < high_thrs' as high_thrs > mid_thrs always.

Fixes a4bd217b4("lightnvm: physical block device (pblk) target")

Signed-off-by Rakesh Pandit <[email protected]>

---
 drivers/lightnvm/pblk-core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/lightnvm/pblk-core.c +5 -5

index 8150164..93a58ed 100644
@@ -295,16 +295,16 @@ struct list_head *pblk_line_gc_list(struct pblk *pblk, struct pblk_line *line)
 			line->gc_group = PBLK_LINEGC_FULL;
 			move_list = &l_mg->gc_full_list;
 		}
-	} else if (vsc < lm->high_thrs) {
-		if (line->gc_group != PBLK_LINEGC_HIGH) {
-			line->gc_group = PBLK_LINEGC_HIGH;
-			move_list = &l_mg->gc_high_list;
-		}
 	} else if (vsc < lm->mid_thrs) {
 		if (line->gc_group != PBLK_LINEGC_MID) {
 			line->gc_group = PBLK_LINEGC_MID;
 			move_list = &l_mg->gc_mid_list;
 		}
+	} else if (vsc < lm->high_thrs) {
+		if (line->gc_group != PBLK_LINEGC_HIGH) {
+			line->gc_group = PBLK_LINEGC_HIGH;
+			move_list = &l_mg->gc_high_list;
+		}
 	} else if (vsc < line->sec_in_line) {
 		if (line->gc_group != PBLK_LINEGC_LOW) {
 			line->gc_group = PBLK_LINEGC_LOW;

Signature · 1 line

2.9.3

Javier González

2017-10-02 11:27 UTC #1 lore raw

Quoted 42 lines · Rakesh Pandit

pblk_line_gc_list seems to had a bug since the introduction of pblk in getting GC list for a line. In b20ba1bc7 while redesigning GC algorithm it was not fixed correctly. The problem is that even if valid sector count (vsc) is less that mid_thrs (threshold for GC mid list) it would always satisfy 'vsc < high_thrs' as high_thrs > mid_thrs always.
Fixes a4bd217b4("lightnvm: physical block device (pblk) target")
Signed-off-by Rakesh Pandit <[email protected]>
---
drivers/lightnvm/pblk-core.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
drivers/lightnvm/pblk-core.c +5 -6
index 8150164..93a58ed 100644
@@ -295,16 +295,16 @@ struct list_head *pblk_line_gc_list(struct pblk *pblk, struct pblk_line *line)
 			line->gc_group = PBLK_LINEGC_FULL;
 			move_list = &l_mg->gc_full_list;
 		}
-	} else if (vsc < lm->high_thrs) {
-		if (line->gc_group != PBLK_LINEGC_HIGH) {
-			line->gc_group = PBLK_LINEGC_HIGH;
-			move_list = &l_mg->gc_high_list;
-		}
 	} else if (vsc < lm->mid_thrs) {
 		if (line->gc_group != PBLK_LINEGC_MID) {
 			line->gc_group = PBLK_LINEGC_MID;
 			move_list = &l_mg->gc_mid_list;
 		}
+	} else if (vsc < lm->high_thrs) {
+		if (line->gc_group != PBLK_LINEGC_HIGH) {
+			line->gc_group = PBLK_LINEGC_HIGH;
+			move_list = &l_mg->gc_high_list;
+		}
 	} else if (vsc < line->sec_in_line) {
 		if (line->gc_group != PBLK_LINEGC_LOW) {
 			line->gc_group = PBLK_LINEGC_LOW;
--

Quoted 1 line

2.9.3

You're right that the naming for high/mid/low was not updated when aligning vsc with GC thresholds. But the fix should be making high, being high, instead of reordering when moving into the GC bucket.

Does it make sense to you?

drivers/lightnvm/pblk-init.c +2 -2

index 7cf4b536d899..bc5c6cc12ad5 100644
@@ -675,8 +675,8 @@ static int pblk_lines_init(struct pblk *pblk)
        lm->blk_bitmap_len = BITS_TO_LONGS(geo->nr_luns) * sizeof(long);
        lm->sec_bitmap_len = BITS_TO_LONGS(lm->sec_per_line) * sizeof(long);
        lm->lun_bitmap_len = BITS_TO_LONGS(geo->nr_luns) * sizeof(long);
-       lm->high_thrs = lm->sec_per_line / 2;
-       lm->mid_thrs = lm->sec_per_line / 4;
+       lm->high_thrs = lm->sec_per_line / 4;
+       lm->mid_thrs = lm->sec_per_line / 2;
        lm->meta_distance = (geo->nr_luns / 2) * pblk->min_write_pgs;
 
        /* Calculate necessary pages for smeta. See comment over struct

Rakesh Pandit

author

2017-10-02 11:43 UTC #2 lore raw

Quoted 48 lines · Javier Gonzï¿½lez

Quoted 42 lines · Rakesh Pandit
pblk_line_gc_list seems to had a bug since the introduction of pblk in getting GC list for a line. In b20ba1bc7 while redesigning GC algorithm it was not fixed correctly. The problem is that even if valid sector count (vsc) is less that mid_thrs (threshold for GC mid list) it would always satisfy 'vsc < high_thrs' as high_thrs > mid_thrs always.
Fixes a4bd217b4("lightnvm: physical block device (pblk) target")
Signed-off-by Rakesh Pandit <[email protected]>
---
drivers/lightnvm/pblk-core.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
drivers/lightnvm/pblk-core.c +5 -6
index 8150164..93a58ed 100644
@@ -295,16 +295,16 @@ struct list_head *pblk_line_gc_list(struct pblk *pblk, struct pblk_line *line)
 			line->gc_group = PBLK_LINEGC_FULL;
 			move_list = &l_mg->gc_full_list;
 		}
-	} else if (vsc < lm->high_thrs) {
-		if (line->gc_group != PBLK_LINEGC_HIGH) {
-			line->gc_group = PBLK_LINEGC_HIGH;
-			move_list = &l_mg->gc_high_list;
-		}
 	} else if (vsc < lm->mid_thrs) {
 		if (line->gc_group != PBLK_LINEGC_MID) {
 			line->gc_group = PBLK_LINEGC_MID;
 			move_list = &l_mg->gc_mid_list;
 		}
+	} else if (vsc < lm->high_thrs) {
+		if (line->gc_group != PBLK_LINEGC_HIGH) {
+			line->gc_group = PBLK_LINEGC_HIGH;
+			move_list = &l_mg->gc_high_list;
+		}
 	} else if (vsc < line->sec_in_line) {
 		if (line->gc_group != PBLK_LINEGC_LOW) {
 			line->gc_group = PBLK_LINEGC_LOW;
--
Quoted 1 line
2.9.3
You're right that the naming for high/mid/low was not updated when aligning vsc with GC thresholds. But the fix should be making high, being high, instead of reordering when moving into the GC bucket.

Quoted 1 line

Does it make sense to you?

Yes.

Quoted 6 lines

drivers/lightnvm/pblk-init.c +0 -0
index 7cf4b536d899..bc5c6cc12ad5 100644

drivers/lightnvm/pblk-init.c static int pblk_lines_init(struct pblk *pblk) · line 682

Quoted 10 lines

drivers/lightnvm/pblk-init.c +2 -2

@@ -675,8 +675,8 @@ static int pblk_lines_init(struct pblk *pblk)
        lm->blk_bitmap_len = BITS_TO_LONGS(geo->nr_luns) * sizeof(long);
        lm->sec_bitmap_len = BITS_TO_LONGS(lm->sec_per_line) * sizeof(long);
        lm->lun_bitmap_len = BITS_TO_LONGS(geo->nr_luns) * sizeof(long);
-       lm->high_thrs = lm->sec_per_line / 2;
-       lm->mid_thrs = lm->sec_per_line / 4;
+       lm->high_thrs = lm->sec_per_line / 4;
+       lm->mid_thrs = lm->sec_per_line / 2;
        lm->meta_distance = (geo->nr_luns / 2) * pblk->min_write_pgs;
 
        /* Calculate necessary pages for smeta. See comment over struct

Regards,

Javier González

2017-10-02 11:44 UTC #3 lore raw

Quoted 51 lines · Rakesh Pandit

Quoted 48 lines · Javier González
Quoted 42 lines · Rakesh Pandit
pblk_line_gc_list seems to had a bug since the introduction of pblk in getting GC list for a line. In b20ba1bc7 while redesigning GC algorithm it was not fixed correctly. The problem is that even if valid sector count (vsc) is less that mid_thrs (threshold for GC mid list) it would always satisfy 'vsc < high_thrs' as high_thrs > mid_thrs always.
Fixes a4bd217b4("lightnvm: physical block device (pblk) target")
Signed-off-by Rakesh Pandit <[email protected]>
---
drivers/lightnvm/pblk-core.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
drivers/lightnvm/pblk-core.c +5 -6
index 8150164..93a58ed 100644
@@ -295,16 +295,16 @@ struct list_head *pblk_line_gc_list(struct pblk *pblk, struct pblk_line *line)
 			line->gc_group = PBLK_LINEGC_FULL;
 			move_list = &l_mg->gc_full_list;
 		}
-	} else if (vsc < lm->high_thrs) {
-		if (line->gc_group != PBLK_LINEGC_HIGH) {
-			line->gc_group = PBLK_LINEGC_HIGH;
-			move_list = &l_mg->gc_high_list;
-		}
 	} else if (vsc < lm->mid_thrs) {
 		if (line->gc_group != PBLK_LINEGC_MID) {
 			line->gc_group = PBLK_LINEGC_MID;
 			move_list = &l_mg->gc_mid_list;
 		}
+	} else if (vsc < lm->high_thrs) {
+		if (line->gc_group != PBLK_LINEGC_HIGH) {
+			line->gc_group = PBLK_LINEGC_HIGH;
+			move_list = &l_mg->gc_high_list;
+		}
 	} else if (vsc < line->sec_in_line) {
 		if (line->gc_group != PBLK_LINEGC_LOW) {
 			line->gc_group = PBLK_LINEGC_LOW;
--
Quoted 1 line
2.9.3
You're right that the naming for high/mid/low was not updated when aligning vsc with GC thresholds. But the fix should be making high, being high, instead of reordering when moving into the GC bucket.

Quoted 4 lines

Quoted 1 line
Does it make sense to you?
Yes.

Cool. I'll fix it when picking it up.

Javier