| Message ID | 20190314160428.3559-5-igor.j.konopko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lightnvm: next set of improvements for 5.2 | expand |
> On 14 Mar 2019, at 09.04, Igor Konopko <igor.j.konopko@intel.com> wrote: > > In case of OOB recovery, when some of the chunks are in closed state, > we are calculating number of written sectors in line incorrectly, > because we are always counting chunk WP, which for closed chunks > does not longer reflects written sectors in particular chunks. This > patch for such a chunks takes clba field instead. > > Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> > --- > drivers/lightnvm/pblk-recovery.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/lightnvm/pblk-recovery.c b/drivers/lightnvm/pblk-recovery.c > index 83b467b..bcd3633 100644 > --- a/drivers/lightnvm/pblk-recovery.c > +++ b/drivers/lightnvm/pblk-recovery.c > @@ -101,6 +101,8 @@ static void pblk_update_line_wp(struct pblk *pblk, struct pblk_line *line, > > static u64 pblk_sec_in_open_line(struct pblk *pblk, struct pblk_line *line) > { > + struct nvm_tgt_dev *dev = pblk->dev; > + struct nvm_geo *geo = &dev->geo; > struct pblk_line_meta *lm = &pblk->lm; > int nr_bb = bitmap_weight(line->blk_bitmap, lm->blk_per_line); > u64 written_secs = 0; > @@ -113,7 +115,11 @@ static u64 pblk_sec_in_open_line(struct pblk *pblk, struct pblk_line *line) > if (chunk->state & NVM_CHK_ST_OFFLINE) > continue; > > - written_secs += chunk->wp; > + if (chunk->state & NVM_CHK_ST_OPEN) > + written_secs += chunk->wp; > + else if (chunk->state & NVM_CHK_ST_CLOSED) > + written_secs += geo->clba; > + > valid_chunks++; > } > > -- > 2.9.5 Mmmm. The change is correct, but can you develop on why the WP does not reflect the written sectors in a closed chunk? As I understand it, the WP reflects the last written sector; if it happens to be WP == clba, then the chunk state machine transitions to closed, but the WP remains untouched. It is only when we reset the chunk that the WP comes back to 0. Am I missing something?
On 3/16/19 3:43 PM, Javier González wrote: >> On 14 Mar 2019, at 09.04, Igor Konopko <igor.j.konopko@intel.com> wrote: >> >> In case of OOB recovery, when some of the chunks are in closed state, >> we are calculating number of written sectors in line incorrectly, >> because we are always counting chunk WP, which for closed chunks >> does not longer reflects written sectors in particular chunks. This >> patch for such a chunks takes clba field instead. >> >> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> >> --- >> drivers/lightnvm/pblk-recovery.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/lightnvm/pblk-recovery.c b/drivers/lightnvm/pblk-recovery.c >> index 83b467b..bcd3633 100644 >> --- a/drivers/lightnvm/pblk-recovery.c >> +++ b/drivers/lightnvm/pblk-recovery.c >> @@ -101,6 +101,8 @@ static void pblk_update_line_wp(struct pblk *pblk, struct pblk_line *line, >> >> static u64 pblk_sec_in_open_line(struct pblk *pblk, struct pblk_line *line) >> { >> + struct nvm_tgt_dev *dev = pblk->dev; >> + struct nvm_geo *geo = &dev->geo; >> struct pblk_line_meta *lm = &pblk->lm; >> int nr_bb = bitmap_weight(line->blk_bitmap, lm->blk_per_line); >> u64 written_secs = 0; >> @@ -113,7 +115,11 @@ static u64 pblk_sec_in_open_line(struct pblk *pblk, struct pblk_line *line) >> if (chunk->state & NVM_CHK_ST_OFFLINE) >> continue; >> >> - written_secs += chunk->wp; >> + if (chunk->state & NVM_CHK_ST_OPEN) >> + written_secs += chunk->wp; >> + else if (chunk->state & NVM_CHK_ST_CLOSED) >> + written_secs += geo->clba; >> + >> valid_chunks++; >> } >> >> -- >> 2.9.5 > > Mmmm. The change is correct, but can you develop on why the WP does not > reflect the written sectors in a closed chunk? As I understand it, the > WP reflects the last written sector; if it happens to be WP == clba, then > the chunk state machine transitions to closed, but the WP remains > untouched. It is only when we reset the chunk that the WP comes back to > 0. Am I missing something? > I agree with Javier. In OCSSD, the write pointer shall always be valid.
On 17.03.2019 20:24, Matias Bjørling wrote: > On 3/16/19 3:43 PM, Javier González wrote: >>> On 14 Mar 2019, at 09.04, Igor Konopko <igor.j.konopko@intel.com> wrote: >>> >>> In case of OOB recovery, when some of the chunks are in closed state, >>> we are calculating number of written sectors in line incorrectly, >>> because we are always counting chunk WP, which for closed chunks >>> does not longer reflects written sectors in particular chunks. This >>> patch for such a chunks takes clba field instead. >>> >>> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> >>> --- >>> drivers/lightnvm/pblk-recovery.c | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/lightnvm/pblk-recovery.c >>> b/drivers/lightnvm/pblk-recovery.c >>> index 83b467b..bcd3633 100644 >>> --- a/drivers/lightnvm/pblk-recovery.c >>> +++ b/drivers/lightnvm/pblk-recovery.c >>> @@ -101,6 +101,8 @@ static void pblk_update_line_wp(struct pblk >>> *pblk, struct pblk_line *line, >>> >>> static u64 pblk_sec_in_open_line(struct pblk *pblk, struct pblk_line >>> *line) >>> { >>> + struct nvm_tgt_dev *dev = pblk->dev; >>> + struct nvm_geo *geo = &dev->geo; >>> struct pblk_line_meta *lm = &pblk->lm; >>> int nr_bb = bitmap_weight(line->blk_bitmap, lm->blk_per_line); >>> u64 written_secs = 0; >>> @@ -113,7 +115,11 @@ static u64 pblk_sec_in_open_line(struct pblk >>> *pblk, struct pblk_line *line) >>> if (chunk->state & NVM_CHK_ST_OFFLINE) >>> continue; >>> >>> - written_secs += chunk->wp; >>> + if (chunk->state & NVM_CHK_ST_OPEN) >>> + written_secs += chunk->wp; >>> + else if (chunk->state & NVM_CHK_ST_CLOSED) >>> + written_secs += geo->clba; >>> + >>> valid_chunks++; >>> } >>> >>> -- >>> 2.9.5 >> >> Mmmm. The change is correct, but can you develop on why the WP does not >> reflect the written sectors in a closed chunk? As I understand it, the >> WP reflects the last written sector; if it happens to be WP == clba, then >> the chunk state machine transitions to closed, but the WP remains >> untouched. It is only when we reset the chunk that the WP comes back to >> 0. Am I missing something? >> > > I agree with Javier. In OCSSD, the write pointer shall always be valid. So based on OCSSD 2.0 spec "If WP = SLBA + NLB, the chunk is closed" (also on "Figure 5: Chunk State Diagram in spec": WP = SLBA + NLB for closed chunk), my understanding it that the WP is not valid for that particular use case, since in written_secs we want to obtain NLB field (relative within chunk, without starting LBA value). So that's why I used fixed CLBA here, since it WP pointer for closed chunk is absolute value instead of relative one. Did I misunderstood sth in the spec?
> On 18 Mar 2019, at 13.50, Igor Konopko <igor.j.konopko@intel.com> wrote: > > > > On 17.03.2019 20:24, Matias Bjørling wrote: >> On 3/16/19 3:43 PM, Javier González wrote: >>>> On 14 Mar 2019, at 09.04, Igor Konopko <igor.j.konopko@intel.com> wrote: >>>> >>>> In case of OOB recovery, when some of the chunks are in closed state, >>>> we are calculating number of written sectors in line incorrectly, >>>> because we are always counting chunk WP, which for closed chunks >>>> does not longer reflects written sectors in particular chunks. This >>>> patch for such a chunks takes clba field instead. >>>> >>>> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> >>>> --- >>>> drivers/lightnvm/pblk-recovery.c | 8 +++++++- >>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/lightnvm/pblk-recovery.c b/drivers/lightnvm/pblk-recovery.c >>>> index 83b467b..bcd3633 100644 >>>> --- a/drivers/lightnvm/pblk-recovery.c >>>> +++ b/drivers/lightnvm/pblk-recovery.c >>>> @@ -101,6 +101,8 @@ static void pblk_update_line_wp(struct pblk *pblk, struct pblk_line *line, >>>> >>>> static u64 pblk_sec_in_open_line(struct pblk *pblk, struct pblk_line *line) >>>> { >>>> + struct nvm_tgt_dev *dev = pblk->dev; >>>> + struct nvm_geo *geo = &dev->geo; >>>> struct pblk_line_meta *lm = &pblk->lm; >>>> int nr_bb = bitmap_weight(line->blk_bitmap, lm->blk_per_line); >>>> u64 written_secs = 0; >>>> @@ -113,7 +115,11 @@ static u64 pblk_sec_in_open_line(struct pblk *pblk, struct pblk_line *line) >>>> if (chunk->state & NVM_CHK_ST_OFFLINE) >>>> continue; >>>> >>>> - written_secs += chunk->wp; >>>> + if (chunk->state & NVM_CHK_ST_OPEN) >>>> + written_secs += chunk->wp; >>>> + else if (chunk->state & NVM_CHK_ST_CLOSED) >>>> + written_secs += geo->clba; >>>> + >>>> valid_chunks++; >>>> } >>>> >>>> -- >>>> 2.9.5 >>> >>> Mmmm. The change is correct, but can you develop on why the WP does not >>> reflect the written sectors in a closed chunk? As I understand it, the >>> WP reflects the last written sector; if it happens to be WP == clba, then >>> the chunk state machine transitions to closed, but the WP remains >>> untouched. It is only when we reset the chunk that the WP comes back to >>> 0. Am I missing something? >> I agree with Javier. In OCSSD, the write pointer shall always be valid. > > So based on OCSSD 2.0 spec "If WP = SLBA + NLB, the chunk is closed" (also on "Figure 5: Chunk State Diagram in spec": WP = SLBA + NLB for closed chunk), my understanding it that the WP is not valid for that particular use case, since in written_secs we want to obtain NLB field (relative within chunk, without starting LBA value). So that's why I used fixed CLBA here, since it WP pointer for closed chunk is absolute value instead of relative one. > Did I misunderstood sth in the spec? I see where the confusion comes from. I have always understood it in the way that WP points to the next available sector in the chunk. When WP = SLBA + NLB, then the WP remains in the last valid sector (as there are no more available sectors). Both polk and QEMU are implemented this way, but it might be worth a clarification in the spec (to be one way or the other)? Matias?
diff --git a/drivers/lightnvm/pblk-recovery.c b/drivers/lightnvm/pblk-recovery.c index 83b467b..bcd3633 100644 --- a/drivers/lightnvm/pblk-recovery.c +++ b/drivers/lightnvm/pblk-recovery.c @@ -101,6 +101,8 @@ static void pblk_update_line_wp(struct pblk *pblk, struct pblk_line *line, static u64 pblk_sec_in_open_line(struct pblk *pblk, struct pblk_line *line) { + struct nvm_tgt_dev *dev = pblk->dev; + struct nvm_geo *geo = &dev->geo; struct pblk_line_meta *lm = &pblk->lm; int nr_bb = bitmap_weight(line->blk_bitmap, lm->blk_per_line); u64 written_secs = 0; @@ -113,7 +115,11 @@ static u64 pblk_sec_in_open_line(struct pblk *pblk, struct pblk_line *line) if (chunk->state & NVM_CHK_ST_OFFLINE) continue; - written_secs += chunk->wp; + if (chunk->state & NVM_CHK_ST_OPEN) + written_secs += chunk->wp; + else if (chunk->state & NVM_CHK_ST_CLOSED) + written_secs += geo->clba; + valid_chunks++; }
In case of OOB recovery, when some of the chunks are in closed state, we are calculating number of written sectors in line incorrectly, because we are always counting chunk WP, which for closed chunks does not longer reflects written sectors in particular chunks. This patch for such a chunks takes clba field instead. Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> --- drivers/lightnvm/pblk-recovery.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)