> On 22 Mar 2019, at 22.48, Igor Konopko <igor.j.konopko@intel.com> wrote:
> 
> This patch is made in order to prepare read path for new approach to
> partial read handling (which is needed for multi-page bvec handling)
> The most important change is to move the handling of completed and
> failed bio from the pblk_make_rq() to particular read and write
> functions. This is needed, since after partial read path changes,
> sometimes completed/failed bio will be different from original one, so
> we cannot do this any longer in pblk_make_rq(). Other changes are
> small read path refactor in order to reduce the size of another patch
> with partial read changes. Generally the goal of this patch is not to
> change the functionality, but just to prepare the code for the
> following changes.
> 
> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
> ---
> drivers/lightnvm/pblk-cache.c |  7 ++--
> drivers/lightnvm/pblk-init.c  | 14 ++------
> drivers/lightnvm/pblk-read.c  | 83 ++++++++++++++++++++-----------------------
> drivers/lightnvm/pblk.h       |  4 +--
> 4 files changed, 47 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/lightnvm/pblk-cache.c b/drivers/lightnvm/pblk-cache.c
> index c9fa26f..d2a3683 100644
> --- a/drivers/lightnvm/pblk-cache.c
> +++ b/drivers/lightnvm/pblk-cache.c
> @@ -18,7 +18,7 @@
> 
> #include "pblk.h"
> 
> -int pblk_write_to_cache(struct pblk *pblk, struct bio *bio, unsigned long flags)
> +void pblk_write_to_cache(struct pblk *pblk, struct bio *bio, unsigned long flags)
> {
> 	struct request_queue *q = pblk->dev->q;
> 	struct pblk_w_ctx w_ctx;
> @@ -43,6 +43,7 @@ int pblk_write_to_cache(struct pblk *pblk, struct bio *bio, unsigned long flags)
> 		goto retry;
> 	case NVM_IO_ERR:
> 		pblk_pipeline_stop(pblk);
> +		bio_io_error(bio);
> 		goto out;
> 	}
> 
> @@ -79,7 +80,9 @@ int pblk_write_to_cache(struct pblk *pblk, struct bio *bio, unsigned long flags)
> out:
> 	generic_end_io_acct(q, REQ_OP_WRITE, &pblk->disk->part0, start_time);
> 	pblk_write_should_kick(pblk);
> -	return ret;
> +
> +	if (ret == NVM_IO_DONE)
> +		bio_endio(bio);
> }
> 
> /*
> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
> index e2e1193..4211cd1 100644
> --- a/drivers/lightnvm/pblk-init.c
> +++ b/drivers/lightnvm/pblk-init.c
> @@ -50,7 +50,6 @@ struct bio_set pblk_bio_set;
> static blk_qc_t pblk_make_rq(struct request_queue *q, struct bio *bio)
> {
> 	struct pblk *pblk = q->queuedata;
> -	int ret;
> 
> 	if (bio_op(bio) == REQ_OP_DISCARD) {
> 		pblk_discard(pblk, bio);
> @@ -65,7 +64,7 @@ static blk_qc_t pblk_make_rq(struct request_queue *q, struct bio *bio)
> 	 */
> 	if (bio_data_dir(bio) == READ) {
> 		blk_queue_split(q, &bio);
> -		ret = pblk_submit_read(pblk, bio);
> +		pblk_submit_read(pblk, bio);
> 	} else {
> 		/* Prevent deadlock in the case of a modest LUN configuration
> 		 * and large user I/Os. Unless stalled, the rate limiter
> @@ -74,16 +73,7 @@ static blk_qc_t pblk_make_rq(struct request_queue *q, struct bio *bio)
> 		if (pblk_get_secs(bio) > pblk_rl_max_io(&pblk->rl))
> 			blk_queue_split(q, &bio);
> 
> -		ret = pblk_write_to_cache(pblk, bio, PBLK_IOTYPE_USER);
> -	}
> -
> -	switch (ret) {
> -	case NVM_IO_ERR:
> -		bio_io_error(bio);
> -		break;
> -	case NVM_IO_DONE:
> -		bio_endio(bio);
> -		break;
> +		pblk_write_to_cache(pblk, bio, PBLK_IOTYPE_USER);
> 	}
> 
> 	return BLK_QC_T_NONE;
> diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
> index 6569746..597fe6d 100644
> --- a/drivers/lightnvm/pblk-read.c
> +++ b/drivers/lightnvm/pblk-read.c
> @@ -179,7 +179,8 @@ static void pblk_end_user_read(struct bio *bio, int error)
> {
> 	if (error && error != NVM_RSP_WARN_HIGHECC)
> 		bio_io_error(bio);
> -	bio_endio(bio);
> +	else
> +		bio_endio(bio);
> }
> 
> static void __pblk_end_io_read(struct pblk *pblk, struct nvm_rq *rqd,
> @@ -383,7 +384,6 @@ static int pblk_partial_read_bio(struct pblk *pblk, struct nvm_rq *rqd,
> 
> 	/* Free allocated pages in new bio */
> 	pblk_bio_free_pages(pblk, rqd->bio, 0, rqd->bio->bi_vcnt);
> -	__pblk_end_io_read(pblk, rqd, false);
> 	return NVM_IO_ERR;
> }
> 
> @@ -428,7 +428,7 @@ static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd, struct bio *bio,
> 	}
> }
> 
> -int pblk_submit_read(struct pblk *pblk, struct bio *bio)
> +void pblk_submit_read(struct pblk *pblk, struct bio *bio)
> {
> 	struct nvm_tgt_dev *dev = pblk->dev;
> 	struct request_queue *q = dev->q;
> @@ -436,9 +436,9 @@ int pblk_submit_read(struct pblk *pblk, struct bio *bio)
> 	unsigned int nr_secs = pblk_get_secs(bio);
> 	struct pblk_g_ctx *r_ctx;
> 	struct nvm_rq *rqd;
> +	struct bio *int_bio;
> 	unsigned int bio_init_idx;
> 	DECLARE_BITMAP(read_bitmap, NVM_MAX_VLBA);
> -	int ret = NVM_IO_ERR;
> 
> 	generic_start_io_acct(q, REQ_OP_READ, bio_sectors(bio),
> 			      &pblk->disk->part0);
> @@ -449,74 +449,67 @@ int pblk_submit_read(struct pblk *pblk, struct bio *bio)
> 
> 	rqd->opcode = NVM_OP_PREAD;
> 	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.
> 	 */
> 	bio_init_idx = pblk_get_bi_idx(bio);
> 
> -	if (pblk_alloc_rqd_meta(pblk, rqd))
> -		goto fail_rqd_free;
> +	if (pblk_alloc_rqd_meta(pblk, rqd)) {
> +		bio_io_error(bio);
> +		pblk_free_rqd(pblk, rqd, PBLK_READ);
> +		return;
> +	}
> +
> +	/* Clone read bio to deal internally with:
> +	 * -read errors when reading from drive
> +	 * -bio_advance() calls during l2p lookup and cache reads
> +	 */
> +	int_bio = bio_clone_fast(bio, GFP_KERNEL, &pblk_bio_set);
> 
> 	if (nr_secs > 1)
> 		pblk_read_ppalist_rq(pblk, rqd, bio, blba, read_bitmap);
> 	else
> 		pblk_read_rq(pblk, rqd, bio, blba, read_bitmap);
> 
> +	r_ctx->private = bio; /* original bio */
> +	rqd->bio = int_bio; /* internal bio */
> +
> 	if (bitmap_full(read_bitmap, nr_secs)) {
> +		pblk_end_user_read(bio, 0);
> 		atomic_inc(&pblk->inflight_io);
> 		__pblk_end_io_read(pblk, rqd, false);
> -		return NVM_IO_DONE;
> +		return;
> 	}
> 
> -	/* All sectors are to be read from the device */
> -	if (bitmap_empty(read_bitmap, rqd->nr_ppas)) {
> -		struct bio *int_bio = NULL;
> -
> -		/* Clone read bio to deal with read errors internally */
> -		int_bio = bio_clone_fast(bio, GFP_KERNEL, &pblk_bio_set);
> -		if (!int_bio) {
> -			pblk_err(pblk, "could not clone read bio\n");
> -			goto fail_end_io;
> -		}
> -
> -		rqd->bio = int_bio;
> -
> -		if (pblk_submit_io(pblk, rqd)) {
> +	if (!bitmap_empty(read_bitmap, rqd->nr_ppas)) {
> +		/* 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.
> +		 */
> +		bio_put(int_bio);
> +		rqd->bio = NULL;
> +		if (pblk_partial_read_bio(pblk, rqd, bio_init_idx, read_bitmap,
> +					    nr_secs)) {
> 			pblk_err(pblk, "read IO submission failed\n");
> -			ret = NVM_IO_ERR;
> -			goto fail_end_io;
> +			bio_io_error(bio);
> +			__pblk_end_io_read(pblk, rqd, false);
> 		}
> -
> -		return NVM_IO_OK;
> +		return;
> 	}
> 
> -	/* 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.
> -	 */
> -	ret = pblk_partial_read_bio(pblk, rqd, bio_init_idx, read_bitmap,
> -				    nr_secs);
> -	if (ret)
> -		goto fail_meta_free;
> -
> -	return NVM_IO_OK;
> -
> -fail_meta_free:
> -	nvm_dev_dma_free(dev->parent, rqd->meta_list, rqd->dma_meta_list);
> -fail_rqd_free:
> -	pblk_free_rqd(pblk, rqd, PBLK_READ);
> -	return ret;
> -fail_end_io:
> -	__pblk_end_io_read(pblk, rqd, false);
> -	return ret;
> +	/* All sectors are to be read from the device */
> +	if (pblk_submit_io(pblk, rqd)) {
> +		pblk_err(pblk, "read IO submission failed\n");
> +		bio_io_error(bio);
> +		__pblk_end_io_read(pblk, rqd, false);
> +	}
> }
> 
> static int read_ppalist_rq_gc(struct pblk *pblk, struct nvm_rq *rqd,
> diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
> index 381f074..a3c925d 100644
> --- a/drivers/lightnvm/pblk.h
> +++ b/drivers/lightnvm/pblk.h
> @@ -868,7 +868,7 @@ void pblk_get_packed_meta(struct pblk *pblk, struct nvm_rq *rqd);
> /*
>  * pblk user I/O write path
>  */
> -int pblk_write_to_cache(struct pblk *pblk, struct bio *bio,
> +void pblk_write_to_cache(struct pblk *pblk, struct bio *bio,
> 			unsigned long flags);
> int pblk_write_gc_to_cache(struct pblk *pblk, struct pblk_gc_rq *gc_rq);
> 
> @@ -894,7 +894,7 @@ void pblk_write_kick(struct pblk *pblk);
>  * pblk read path
>  */
> extern struct bio_set pblk_bio_set;
> -int pblk_submit_read(struct pblk *pblk, struct bio *bio);
> +void pblk_submit_read(struct pblk *pblk, struct bio *bio);
> int pblk_submit_read_gc(struct pblk *pblk, struct pblk_gc_rq *gc_rq);
> /*
>  * pblk recovery
> --
> 2.9.5

The patch looks sane.


Reviewed-by: Javier González <javier@javigon.com>