LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jens Axboe <axboe@kernel.dk>
To: "Matias Bjørling" <mb@lightnvm.io>,
	"Javier González" <javier@javigon.com>
Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
	"Javier González" <javier@cnexlabs.com>
Subject: Re: [PATCH] lightnvm: pblk: remove unnecessary bio_get/put
Date: Fri, 1 Jun 2018 07:47:16 -0600	[thread overview]
Message-ID: <162341d7-83b1-6bb3-9e00-f7bbc14f99f0@kernel.dk> (raw)
In-Reply-To: <f1579a62-0e85-b58d-ca33-a9635dd0ebaa@lightnvm.io>

On 6/1/18 6:05 AM, Matias Bjørling wrote:
> On 06/01/2018 02:01 PM, Javier González wrote:
>> In the read path, pblk gets a reference to the incoming bio and puts it
>> after ending the bio. Though this behavior is correct, it is unnecessary
>> since pblk is the one putting the bio, therefore, it cannot disappear
>> underneath it.
>>
>> Removing this reference, allows to clean up rqd->bio and avoids pointer
>> bouncing for the different read paths. Now, the incoming bio always
>> resides in the read context and pblk's internal bios (if any) reside in
>> rqd->bio.
>>
>> Signed-off-by: Javier González <javier@cnexlabs.com>
>> ---
>>   drivers/lightnvm/pblk-read.c | 65 +++++++++++++++++++-------------------------
>>   1 file changed, 28 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
>> index fa7b60f852d9..38360de23d4e 100644
>> --- a/drivers/lightnvm/pblk-read.c
>> +++ b/drivers/lightnvm/pblk-read.c
>> @@ -39,10 +39,10 @@ static int pblk_read_from_cache(struct pblk *pblk, struct bio *bio,
>>   }
>>   
>>   static void pblk_read_ppalist_rq(struct pblk *pblk, struct nvm_rq *rqd,
>> -				 sector_t blba, unsigned long *read_bitmap)
>> +				 struct bio *bio, sector_t blba,
>> +				 unsigned long *read_bitmap)
>>   {
>>   	struct pblk_sec_meta *meta_list = rqd->meta_list;
>> -	struct bio *bio = rqd->bio;
>>   	struct ppa_addr ppas[PBLK_MAX_REQ_ADDRS];
>>   	int nr_secs = rqd->nr_ppas;
>>   	bool advanced_bio = false;
>> @@ -189,7 +189,6 @@ static void pblk_end_user_read(struct bio *bio)
>>   	WARN_ONCE(bio->bi_status, "pblk: corrupted read bio\n");
>>   #endif
>>   	bio_endio(bio);
>> -	bio_put(bio);
>>   }
>>   
>>   static void __pblk_end_io_read(struct pblk *pblk, struct nvm_rq *rqd,
>> @@ -197,23 +196,18 @@ static void __pblk_end_io_read(struct pblk *pblk, struct nvm_rq *rqd,
>>   {
>>   	struct nvm_tgt_dev *dev = pblk->dev;
>>   	struct pblk_g_ctx *r_ctx = nvm_rq_to_pdu(rqd);
>> -	struct bio *bio = rqd->bio;
>> +	struct bio *int_bio = rqd->bio;
>>   	unsigned long start_time = r_ctx->start_time;
>>   
>>   	generic_end_io_acct(dev->q, READ, &pblk->disk->part0, start_time);
>>   
>>   	if (rqd->error)
>>   		pblk_log_read_err(pblk, rqd);
>> -#ifdef CONFIG_NVM_DEBUG
>> -	else
>> -		WARN_ONCE(bio->bi_status, "pblk: corrupted read error\n");
>> -#endif
>>   
>>   	pblk_read_check_seq(pblk, rqd, r_ctx->lba);
>>   
>> -	bio_put(bio);
>> -	if (r_ctx->private)
>> -		pblk_end_user_read((struct bio *)r_ctx->private);
>> +	if (int_bio)
>> +		bio_put(int_bio);
>>   
>>   	if (put_line)
>>   		pblk_read_put_rqd_kref(pblk, rqd);
>> @@ -230,16 +224,19 @@ static void __pblk_end_io_read(struct pblk *pblk, struct nvm_rq *rqd,
>>   static void pblk_end_io_read(struct nvm_rq *rqd)
>>   {
>>   	struct pblk *pblk = rqd->private;
>> +	struct pblk_g_ctx *r_ctx = nvm_rq_to_pdu(rqd);
>> +	struct bio *bio = (struct bio *)r_ctx->private;
>>   
>> +	pblk_end_user_read(bio);
>>   	__pblk_end_io_read(pblk, rqd, true);
>>   }
>>   
>> -static int pblk_partial_read_bio(struct pblk *pblk, struct nvm_rq *rqd,
>> -				 unsigned int bio_init_idx,
>> -				 unsigned long *read_bitmap)
>> +static int pblk_partial_read(struct pblk *pblk, struct nvm_rq *rqd,
>> +			     struct bio *orig_bio, unsigned int bio_init_idx,
>> +			     unsigned long *read_bitmap)
>>   {
>> -	struct bio *new_bio, *bio = rqd->bio;
>>   	struct pblk_sec_meta *meta_list = rqd->meta_list;
>> +	struct bio *new_bio;
>>   	struct bio_vec src_bv, dst_bv;
>>   	void *ppa_ptr = NULL;
>>   	void *src_p, *dst_p;
>> @@ -256,11 +253,11 @@ static int pblk_partial_read_bio(struct pblk *pblk, struct nvm_rq *rqd,
>>   	new_bio = bio_alloc(GFP_KERNEL, nr_holes);
>>   
>>   	if (pblk_bio_add_pages(pblk, new_bio, GFP_KERNEL, nr_holes))
>> -		goto err_add_pages;
>> +		goto fail_add_pages;
>>   
>>   	if (nr_holes != new_bio->bi_vcnt) {
>>   		pr_err("pblk: malformed bio\n");
>> -		goto err;
>> +		goto fail;
>>   	}
>>   
>>   	for (i = 0; i < nr_secs; i++)
>> @@ -283,7 +280,7 @@ static int pblk_partial_read_bio(struct pblk *pblk, struct nvm_rq *rqd,
>>   	if (ret) {
>>   		bio_put(rqd->bio);
>>   		pr_err("pblk: sync read IO submission failed\n");
>> -		goto err;
>> +		goto fail;
>>   	}
>>   
>>   	if (rqd->error) {
>> @@ -319,7 +316,7 @@ static int pblk_partial_read_bio(struct pblk *pblk, struct nvm_rq *rqd,
>>   		meta_list[hole].lba = lba_list_media[i];
>>   
>>   		src_bv = new_bio->bi_io_vec[i++];
>> -		dst_bv = bio->bi_io_vec[bio_init_idx + hole];
>> +		dst_bv = orig_bio->bi_io_vec[bio_init_idx + hole];
>>   
>>   		src_p = kmap_atomic(src_bv.bv_page);
>>   		dst_p = kmap_atomic(dst_bv.bv_page);
>> @@ -338,28 +335,26 @@ static int pblk_partial_read_bio(struct pblk *pblk, struct nvm_rq *rqd,
>>   
>>   	bio_put(new_bio);
>>   
>> -	/* Complete the original bio and associated request */
>> -	bio_endio(bio);
>> -	rqd->bio = bio;
>> +	/* restore original request */
>> +	rqd->bio = NULL;
>>   	rqd->nr_ppas = nr_secs;
>>   
>>   	__pblk_end_io_read(pblk, rqd, false);
>> -	return NVM_IO_OK;
>> +	return NVM_IO_DONE;
>>   
>> -err:
>> +fail:
>>   	/* Free allocated pages in new bio */
>>   	pblk_bio_free_pages(pblk, new_bio, 0, new_bio->bi_vcnt);
>> -err_add_pages:
>> +fail_add_pages:
>>   	pr_err("pblk: failed to perform partial read\n");
>>   	__pblk_end_io_read(pblk, rqd, false);
>>   	return NVM_IO_ERR;
>>   }
>>   
>> -static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd,
>> +static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd, struct bio *bio,
>>   			 sector_t lba, unsigned long *read_bitmap)
>>   {
>>   	struct pblk_sec_meta *meta_list = rqd->meta_list;
>> -	struct bio *bio = rqd->bio;
>>   	struct ppa_addr ppa;
>>   
>>   	pblk_lookup_l2p_seq(pblk, &ppa, lba, 1);
>> @@ -423,14 +418,15 @@ int pblk_submit_read(struct pblk *pblk, struct bio *bio)
>>   	rqd = pblk_alloc_rqd(pblk, PBLK_READ);
>>   
>>   	rqd->opcode = NVM_OP_PREAD;
>> -	rqd->bio = bio;
>>   	rqd->nr_ppas = nr_secs;
>> +	rqd->bio = NULL; /* cloned bio if needed */
>>   	rqd->private = pblk;
>>   	rqd->end_io = pblk_end_io_read;
>>   
>>   	r_ctx = nvm_rq_to_pdu(rqd);
>>   	r_ctx->start_time = jiffies;
>>   	r_ctx->lba = blba;
>> +	r_ctx->private = bio; /* original bio */
>>   
>>   	/* Save the index for this bio's start. This is needed in case
>>   	 * we need to fill a partial read.
>> @@ -448,17 +444,15 @@ int pblk_submit_read(struct pblk *pblk, struct bio *bio)
>>   		rqd->ppa_list = rqd->meta_list + pblk_dma_meta_size;
>>   		rqd->dma_ppa_list = rqd->dma_meta_list + pblk_dma_meta_size;
>>   
>> -		pblk_read_ppalist_rq(pblk, rqd, blba, &read_bitmap);
>> +		pblk_read_ppalist_rq(pblk, rqd, bio, blba, &read_bitmap);
>>   	} else {
>> -		pblk_read_rq(pblk, rqd, blba, &read_bitmap);
>> +		pblk_read_rq(pblk, rqd, bio, blba, &read_bitmap);
>>   	}
>>   
>> -	bio_get(bio);
>>   	if (bitmap_full(&read_bitmap, nr_secs)) {
>> -		bio_endio(bio);
>>   		atomic_inc(&pblk->inflight_io);
>>   		__pblk_end_io_read(pblk, rqd, false);
>> -		return NVM_IO_OK;
>> +		return NVM_IO_DONE;
>>   	}
>>   
>>   	/* All sectors are to be read from the device */
>> @@ -473,13 +467,10 @@ int pblk_submit_read(struct pblk *pblk, struct bio *bio)
>>   		}
>>   
>>   		rqd->bio = int_bio;
>> -		r_ctx->private = bio;
>>   
>>   		if (pblk_submit_io(pblk, rqd)) {
>>   			pr_err("pblk: read IO submission failed\n");
>>   			ret = NVM_IO_ERR;
>> -			if (int_bio)
>> -				bio_put(int_bio);
>>   			goto fail_end_io;
>>   		}
>>   
>> @@ -489,7 +480,7 @@ int pblk_submit_read(struct pblk *pblk, struct bio *bio)
>>   	/* The read bio request could be partially filled by the write buffer,
>>   	 * but there are some holes that need to be read from the drive.
>>   	 */
>> -	return pblk_partial_read_bio(pblk, rqd, bio_init_idx, &read_bitmap);
>> +	return pblk_partial_read(pblk, rqd, bio, bio_init_idx, &read_bitmap);
>>   
>>   fail_rqd_free:
>>   	pblk_free_rqd(pblk, rqd, PBLK_READ);
>>
> 
> Hi Jens,
> 
> Would you please pick this up as well?

Please just make it part of your updated series, it doesn't apply
to what I currently have.

-- 
Jens Axboe

  reply	other threads:[~2018-06-01 13:47 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-01 12:01 [PATCH V2] " Javier González
2018-06-01 12:01 ` [PATCH] " Javier González
2018-06-01 12:05   ` Matias Bjørling
2018-06-01 13:47     ` Jens Axboe [this message]
  -- strict thread matches above, loose matches on Subject: below --
2018-04-16 10:22 Javier González
2018-06-01 10:22 ` Javier González
2018-06-01 10:42   ` Matias Bjørling
2018-06-01 10:48     ` Javier González

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=162341d7-83b1-6bb3-9e00-f7bbc14f99f0@kernel.dk \
    --to=axboe@kernel.dk \
    --cc=javier@cnexlabs.com \
    --cc=javier@javigon.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mb@lightnvm.io \
    --subject='Re: [PATCH] lightnvm: pblk: remove unnecessary bio_get/put' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).