Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] lightnvm: pblk: remove debug from pblk_[down/up]_page
To:     javier@cnexlabs.com
Cc:     igor.j.konopko@intel.com, marcin.dziegielewski@intel.com,
        hans.holmberg@cnexlabs.com, hlitz@ucsc.edu,
        youngtack.jin@circuitblvd.com, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20180829140832.29341-1-mb@lightnvm.io>
 <AAA74C68-4C22-48C8-B138-1C8EE72A875C@cnexlabs.com>
From:   =?UTF-8?Q?Matias_Bj=c3=b8rling?= <mb@lightnvm.io>
Message-ID: <e0076c19-7e7e-b51a-1f25-25250ed88b5c@lightnvm.io>
Date:   Fri, 31 Aug 2018 16:22:56 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <AAA74C68-4C22-48C8-B138-1C8EE72A875C@cnexlabs.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-GB
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 08/31/2018 02:11 PM, Javier Gonzalez wrote:
>> On 29 Aug 2018, at 16.08, Matias Bjørling <mb@lightnvm.io> wrote:
>>
>> Remove the debug only iteration within __pblk_down_page, which
>> then allows us to reduce the number of arguments down to pblk and
>> the parallel unit from the functions that calls it. Simplifying the
>> callers logic considerably.
>>
>> Also, rename the functions pblk_[down/up]_page to pblk_[down/up]_pu,
>> to communicate that it takes a lock on the parallel unit.
>>
>> Signed-off-by: Matias Bjørling <mb@lightnvm.io>
>> ---
>> drivers/lightnvm/pblk-core.c     | 34 +++++++++-------------------------
>> drivers/lightnvm/pblk-map.c      |  2 +-
>> drivers/lightnvm/pblk-recovery.c |  6 +++---
>> drivers/lightnvm/pblk-write.c    |  6 +++---
>> drivers/lightnvm/pblk.h          |  6 +++---
>> 5 files changed, 19 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
>> index bb1a7cc24cbb..08df6bcd2f81 100644
>> --- a/drivers/lightnvm/pblk-core.c
>> +++ b/drivers/lightnvm/pblk-core.c
>> @@ -1861,8 +1861,7 @@ void pblk_gen_run_ws(struct pblk *pblk, struct pblk_line *line, void *priv,
>> 	queue_work(wq, &line_ws->ws);
>> }
>>
>> -static void __pblk_down_page(struct pblk *pblk, struct ppa_addr *ppa_list,
>> -			     int nr_ppas, int pos)
>> +static void __pblk_down_pu(struct pblk *pblk, int pos)
>> {
>> 	struct pblk_lun *rlun = &pblk->luns[pos];
>> 	int ret;
>> @@ -1871,13 +1870,6 @@ static void __pblk_down_page(struct pblk *pblk, struct ppa_addr *ppa_list,
>> 	 * Only send one inflight I/O per LUN. Since we map at a page
>> 	 * granurality, all ppas in the I/O will map to the same LUN
>> 	 */
>> -#ifdef CONFIG_NVM_PBLK_DEBUG
>> -	int i;
>> -
>> -	for (i = 1; i < nr_ppas; i++)
>> -		WARN_ON(ppa_list[0].a.lun != ppa_list[i].a.lun ||
>> -				ppa_list[0].a.ch != ppa_list[i].a.ch);
>> -#endif
>>
>> 	ret = down_timeout(&rlun->wr_sem, msecs_to_jiffies(30000));
>> 	if (ret == -ETIME || ret == -EINTR)
>> @@ -1885,21 +1877,21 @@ static void __pblk_down_page(struct pblk *pblk, struct ppa_addr *ppa_list,
>> 				-ret);
>> }
>>
>> -void pblk_down_page(struct pblk *pblk, struct ppa_addr *ppa_list, int nr_ppas)
>> +void pblk_down_pu(struct pblk *pblk, struct ppa_addr ppa)
>> {
>> 	struct nvm_tgt_dev *dev = pblk->dev;
>> 	struct nvm_geo *geo = &dev->geo;
>> -	int pos = pblk_ppa_to_pos(geo, ppa_list[0]);
>> +	int pos = pblk_ppa_to_pos(geo, ppa);
>>
>> -	__pblk_down_page(pblk, ppa_list, nr_ppas, pos);
>> +	__pblk_down_pu(pblk, pos);
>> }
>>
>> -void pblk_down_rq(struct pblk *pblk, struct ppa_addr *ppa_list, int nr_ppas,
>> +void pblk_down_rq(struct pblk *pblk, struct ppa_addr ppa,
>> 		  unsigned long *lun_bitmap)
>> {
>> 	struct nvm_tgt_dev *dev = pblk->dev;
>> 	struct nvm_geo *geo = &dev->geo;
>> -	int pos = pblk_ppa_to_pos(geo, ppa_list[0]);
>> +	int pos = pblk_ppa_to_pos(geo, ppa);
>>
>> 	/* If the LUN has been locked for this same request, do no attempt to
>> 	 * lock it again
>> @@ -1907,23 +1899,15 @@ void pblk_down_rq(struct pblk *pblk, struct ppa_addr *ppa_list, int nr_ppas,
>> 	if (test_and_set_bit(pos, lun_bitmap))
>> 		return;
>>
>> -	__pblk_down_page(pblk, ppa_list, nr_ppas, pos);
>> +	__pblk_down_pu(pblk, pos);
>> }
>>
>> -void pblk_up_page(struct pblk *pblk, struct ppa_addr *ppa_list, int nr_ppas)
>> +void pblk_up_pu(struct pblk *pblk, struct ppa_addr ppa)
>> {
>> 	struct nvm_tgt_dev *dev = pblk->dev;
>> 	struct nvm_geo *geo = &dev->geo;
>> 	struct pblk_lun *rlun;
>> -	int pos = pblk_ppa_to_pos(geo, ppa_list[0]);
>> -
>> -#ifdef CONFIG_NVM_PBLK_DEBUG
>> -	int i;
>> -
>> -	for (i = 1; i < nr_ppas; i++)
>> -		WARN_ON(ppa_list[0].a.lun != ppa_list[i].a.lun ||
>> -				ppa_list[0].a.ch != ppa_list[i].a.ch);
>> -#endif
>> +	int pos = pblk_ppa_to_pos(geo, ppa);
>>
>> 	rlun = &pblk->luns[pos];
>> 	up(&rlun->wr_sem);
>> diff --git a/drivers/lightnvm/pblk-map.c b/drivers/lightnvm/pblk-map.c
>> index dc0efb852475..ff677ca6e4e1 100644
>> --- a/drivers/lightnvm/pblk-map.c
>> +++ b/drivers/lightnvm/pblk-map.c
>> @@ -79,7 +79,7 @@ static int pblk_map_page_data(struct pblk *pblk, unsigned int sentry,
>> 		}
>> 	}
>>
>> -	pblk_down_rq(pblk, ppa_list, nr_secs, lun_bitmap);
>> +	pblk_down_rq(pblk, ppa_list[0], lun_bitmap);
>> 	return 0;
>> }
>>
>> diff --git a/drivers/lightnvm/pblk-recovery.c b/drivers/lightnvm/pblk-recovery.c
>> index eea901d7cebc..df624d504d24 100644
>> --- a/drivers/lightnvm/pblk-recovery.c
>> +++ b/drivers/lightnvm/pblk-recovery.c
>> @@ -227,7 +227,7 @@ static void pblk_end_io_recov(struct nvm_rq *rqd)
>> 	struct pblk_pad_rq *pad_rq = rqd->private;
>> 	struct pblk *pblk = pad_rq->pblk;
>>
>> -	pblk_up_page(pblk, ppa_list, rqd->nr_ppas);
>> +	pblk_up_pu(pblk, ppa_list[0]);
>>
>> 	pblk_free_rqd(pblk, rqd, PBLK_WRITE_INT);
>>
>> @@ -339,12 +339,12 @@ static int pblk_recov_pad_oob(struct pblk *pblk, struct pblk_line *line,
>> 	}
>>
>> 	kref_get(&pad_rq->ref);
>> -	pblk_down_page(pblk, rqd->ppa_list, rqd->nr_ppas);
>> +	pblk_down_pu(pblk, rqd->ppa_list[0]);
>>
>> 	ret = pblk_submit_io(pblk, rqd);
>> 	if (ret) {
>> 		pblk_err(pblk, "I/O submission failed: %d\n", ret);
>> -		pblk_up_page(pblk, rqd->ppa_list, rqd->nr_ppas);
>> +		pblk_up_pu(pblk, rqd->ppa_list[0]);
>> 		goto fail_free_bio;
>> 	}
>>
>> diff --git a/drivers/lightnvm/pblk-write.c b/drivers/lightnvm/pblk-write.c
>> index cd579b440b56..55d198edd70a 100644
>> --- a/drivers/lightnvm/pblk-write.c
>> +++ b/drivers/lightnvm/pblk-write.c
>> @@ -270,7 +270,7 @@ static void pblk_end_io_write_meta(struct nvm_rq *rqd)
>> 	struct ppa_addr *ppa_list = nvm_rq_to_ppa_list(rqd);
>> 	int sync;
>>
>> -	pblk_up_page(pblk, ppa_list, rqd->nr_ppas);
>> +	pblk_up_pu(pblk, ppa_list[0]);
>>
>> 	if (rqd->error) {
>> 		pblk_log_write_err(pblk, rqd);
>> @@ -420,7 +420,7 @@ int pblk_submit_meta_io(struct pblk *pblk, struct pblk_line *meta_line)
>> 		list_del(&meta_line->list);
>> 	spin_unlock(&l_mg->close_lock);
>>
>> -	pblk_down_page(pblk, ppa_list, rqd->nr_ppas);
>> +	pblk_down_pu(pblk, ppa_list[0]);
>>
>> 	ret = pblk_submit_io(pblk, rqd);
>> 	if (ret) {
>> @@ -431,7 +431,7 @@ int pblk_submit_meta_io(struct pblk *pblk, struct pblk_line *meta_line)
>> 	return NVM_IO_OK;
>>
>> fail_rollback:
>> -	pblk_up_page(pblk, ppa_list, rqd->nr_ppas);
>> +	pblk_up_pu(pblk, ppa_list[0]);
>> 	spin_lock(&l_mg->close_lock);
>> 	pblk_dealloc_page(pblk, meta_line, rq_ppas);
>> 	list_add(&meta_line->list, &meta_line->list);
>> diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
>> index 2f61f4428fcb..390855375b1e 100644
>> --- a/drivers/lightnvm/pblk.h
>> +++ b/drivers/lightnvm/pblk.h
>> @@ -823,10 +823,10 @@ u64 pblk_alloc_page(struct pblk *pblk, struct pblk_line *line, int nr_secs);
>> u64 __pblk_alloc_page(struct pblk *pblk, struct pblk_line *line, int nr_secs);
>> int pblk_calc_secs(struct pblk *pblk, unsigned long secs_avail,
>> 		   unsigned long secs_to_flush);
>> -void pblk_up_page(struct pblk *pblk, struct ppa_addr *ppa_list, int nr_ppas);
>> -void pblk_down_rq(struct pblk *pblk, struct ppa_addr *ppa_list, int nr_ppas,
>> +void pblk_up_pu(struct pblk *pblk, struct ppa_addr ppa);
>> +void pblk_down_rq(struct pblk *pblk, struct ppa_addr ppa,
>> 		  unsigned long *lun_bitmap);
>> -void pblk_down_page(struct pblk *pblk, struct ppa_addr *ppa_list, int nr_ppas);
>> +void pblk_down_pu(struct pblk *pblk, struct ppa_addr ppa);
>> void pblk_up_rq(struct pblk *pblk, unsigned long *lun_bitmap);
>> int pblk_bio_add_pages(struct pblk *pblk, struct bio *bio, gfp_t flags,
>> 		       int nr_pages);
>> --
>> 2.11.0
> 
> 
> The patch looks good to me. The renaming to PU though is confusing. The
> underlying reason to take the semaphore is to avoid write pointer
> violations. At the moment, we only have an open line in pblk, which
> makes that the semaphore effectively protects a PU, but when we have
> several open lines this is no longer true.
> 

I don't understand this, how will it change when multiple lines are active?

My guess is that one has to keep track of the number of outstanding I/Os 
for each PU. Regarding naming, pblk_[down/up]_line doesn't quite cut it, 
and _page doesn't mean anything to me. Can we find a better name?