[bug report] lightnvm: pblk: return NVM_ error on failed submission

[bug report] lightnvm: pblk: return NVM_ error on failed submission

[Dan Carpenter]

Hello Javier González,

The patch b6730dd4a954: "lightnvm: pblk: return NVM_ error on failed
submission" from Jun 1, 2018, leads to the following static checker
warning:

	drivers/lightnvm/pblk-recovery.c:473 pblk_recov_scan_oob()
	warn: 'pblk->inflight_io.counter' not decremented on lines: 426.

drivers/lightnvm/pblk-recovery.c
   417  
   418                  for (j = 0; j < pblk->min_write_pgs; j++, i++)
   419                          ppa_list[i] =
   420                                  addr_to_gen_ppa(pblk, paddr + j, line->id);
   421          }
   422  
   423          ret = pblk_submit_io_sync(pblk, rqd, data);
   424          if (ret) {
   425                  pblk_err(pblk, "I/O submission failed: %d\n", ret);
   426                  return ret;

The pblk_submit_io_sync() increments the pblk->inflight_io counter but
doesn't decrement it on all error paths.  It looks like something a
little bit subtle is going no but I'm not sure how it works exactly.

   427          }
   428  
   429          atomic_dec(&pblk->inflight_io);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

   430  
   431          /* If a read fails, do a best effort by padding the line and retrying */
   432          if (rqd->error && rqd->error != NVM_RSP_WARN_HIGHECC) {
   433                  int pad_distance, ret;
   434  
   435                  if (padded) {
   436                          pblk_log_read_err(pblk, rqd);
   437                          return -EINTR;
   438                  }
   439  

regards,
dan carpenter

Re: [bug report] lightnvm: pblk: return NVM_ error on failed submission

[Javier González]

On 11.05.2020 16:50, Dan Carpenter wrote:
>Hello Javier González,
>
>The patch b6730dd4a954: "lightnvm: pblk: return NVM_ error on failed
>submission" from Jun 1, 2018, leads to the following static checker
>warning:
>
>	drivers/lightnvm/pblk-recovery.c:473 pblk_recov_scan_oob()
>	warn: 'pblk->inflight_io.counter' not decremented on lines: 426.
>
>drivers/lightnvm/pblk-recovery.c
>   417
>   418                  for (j = 0; j < pblk->min_write_pgs; j++, i++)
>   419                          ppa_list[i] =
>   420                                  addr_to_gen_ppa(pblk, paddr + j, line->id);
>   421          }
>   422
>   423          ret = pblk_submit_io_sync(pblk, rqd, data);
>   424          if (ret) {
>   425                  pblk_err(pblk, "I/O submission failed: %d\n", ret);
>   426                  return ret;
>
>The pblk_submit_io_sync() increments the pblk->inflight_io counter but
>doesn't decrement it on all error paths.  It looks like something a
>little bit subtle is going no but I'm not sure how it works exactly.
>
>   427          }
>   428
>   429          atomic_dec(&pblk->inflight_io);
>                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>   430
>   431          /* If a read fails, do a best effort by padding the line and retrying */
>   432          if (rqd->error && rqd->error != NVM_RSP_WARN_HIGHECC) {
>   433                  int pad_distance, ret;
>   434
>   435                  if (padded) {
>   436                          pblk_log_read_err(pblk, rqd);
>   437                          return -EINTR;
>   438                  }
>   439
>
>regards,
>dan carpenter

Hi Dan,

As I recall, we used this counter to see the total I/Os that were
submitted through pblk and wanted to keep track of how many of them
completed - so the error path did not decrement as it was I/O that had
made it to the FTL but had not reached the device.

I can see how this is confusing, as through time we introduced dedicated
counters to the failed I/Os and inflight became a counter on I/Os going
to the device.

I believe we can fix this by checking the return value on the submit
functions and decrementing in case of error.

Do you want me to send a patch or you want to fix it yourself?

Thanks,
Javier

Re: [bug report] lightnvm: pblk: return NVM_ error on failed submission

[Dan Carpenter]

On Tue, May 12, 2020 at 11:03:18AM +0200, Javier González wrote:
> On 11.05.2020 16:50, Dan Carpenter wrote:
> > Hello Javier González,
> > 
> > The patch b6730dd4a954: "lightnvm: pblk: return NVM_ error on failed
> > submission" from Jun 1, 2018, leads to the following static checker
> > warning:
> > 
> > 	drivers/lightnvm/pblk-recovery.c:473 pblk_recov_scan_oob()
> > 	warn: 'pblk->inflight_io.counter' not decremented on lines: 426.
> > 
> > drivers/lightnvm/pblk-recovery.c
> >   417
> >   418                  for (j = 0; j < pblk->min_write_pgs; j++, i++)
> >   419                          ppa_list[i] =
> >   420                                  addr_to_gen_ppa(pblk, paddr + j, line->id);
> >   421          }
> >   422
> >   423          ret = pblk_submit_io_sync(pblk, rqd, data);
> >   424          if (ret) {
> >   425                  pblk_err(pblk, "I/O submission failed: %d\n", ret);
> >   426                  return ret;
> > 
> > The pblk_submit_io_sync() increments the pblk->inflight_io counter but
> > doesn't decrement it on all error paths.  It looks like something a
> > little bit subtle is going no but I'm not sure how it works exactly.
> > 
> >   427          }
> >   428
> >   429          atomic_dec(&pblk->inflight_io);
> >                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> >   430
> >   431          /* If a read fails, do a best effort by padding the line and retrying */
> >   432          if (rqd->error && rqd->error != NVM_RSP_WARN_HIGHECC) {
> >   433                  int pad_distance, ret;
> >   434
> >   435                  if (padded) {
> >   436                          pblk_log_read_err(pblk, rqd);
> >   437                          return -EINTR;
> >   438                  }
> >   439
> > 
> > regards,
> > dan carpenter
> 
> Hi Dan,
> 
> As I recall, we used this counter to see the total I/Os that were
> submitted through pblk and wanted to keep track of how many of them
> completed - so the error path did not decrement as it was I/O that had
> made it to the FTL but had not reached the device.
> 
> I can see how this is confusing, as through time we introduced dedicated
> counters to the failed I/Os and inflight became a counter on I/Os going
> to the device.
> 
> I believe we can fix this by checking the return value on the submit
> functions and decrementing in case of error.
> 
> Do you want me to send a patch or you want to fix it yourself?

Could you send the patch?

regards,
dan carpenter