Re: [PATCH] lightnvm: consider max hw sectors supported for max_write_pgs

From: "Javier González" <javier@javigon.com>
To: Zhoujie Wu <zjwu@marvell.com>
Cc: "Matias Bjørling" <mb@lightnvm.io>,
	linux-block@vger.kernel.org, hongd@marvell.com
Subject: Re: [PATCH] lightnvm: consider max hw sectors supported for max_write_pgs
Date: Fri, 5 Oct 2018 10:05:21 +0200	[thread overview]
Message-ID: <93D74649-8C00-479C-9273-C19969F44C8F@javigon.com> (raw)
In-Reply-To: <1538699190-4857-1-git-send-email-zjwu@marvell.com>

[-- Attachment #1: Type: text/plain, Size: 1980 bytes --]

> On 5 Oct 2018, at 02.26, Zhoujie Wu <zjwu@marvell.com> wrote:
> 
> When do GC, the number of read/write sectors are determined
> by max_write_pgs(see gc_rq preparation in pblk_gc_line_prepare_ws).
> 
> Due to max_write_pgs doesn't consider max hw sectors
> supported by nvme controller(128K), which leads to GC
> tries to read 64 * 4K in one command, and see below error
> caused by pblk_bio_map_addr in function pblk_submit_read_gc.
> 
> [ 2923.005376] pblk: could not add page to bio
> [ 2923.005377] pblk: could not allocate GC bio (18446744073709551604)
> 
> Signed-off-by: Zhoujie Wu <zjwu@marvell.com>
> ---
> drivers/lightnvm/pblk-init.c | 4 ++++
> 1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
> index e357388..2e51875 100644
> --- a/drivers/lightnvm/pblk-init.c
> +++ b/drivers/lightnvm/pblk-init.c
> @@ -393,6 +393,7 @@ static int pblk_core_init(struct pblk *pblk)
> 	struct nvm_tgt_dev *dev = pblk->dev;
> 	struct nvm_geo *geo = &dev->geo;
> 	int ret, max_write_ppas;
> +	struct request_queue *bqueue = dev->q;
> 

Detail: Can you move this under struct nvm_tgt_dev *dev = pblk->dev;? So
that we maintain ordering?

> 	atomic64_set(&pblk->user_wa, 0);
> 	atomic64_set(&pblk->pad_wa, 0);
> @@ -407,6 +408,9 @@ static int pblk_core_init(struct pblk *pblk)
> 	pblk->min_write_pgs = geo->ws_opt;
> 	max_write_ppas = pblk->min_write_pgs * geo->all_luns;
> 	pblk->max_write_pgs = min_t(int, max_write_ppas, NVM_MAX_VLBA);
> +	/* consider the max hw sector as well */

No need for this comment.

> +	pblk->max_write_pgs =  min_t(int, pblk->max_write_pgs,
> +		queue_max_hw_sectors(bqueue) / (geo->csecs >> 9));
> 	pblk_set_sec_per_write(pblk, pblk->min_write_pgs);
> 
> 	pblk->pad_dist = kcalloc(pblk->min_write_pgs - 1, sizeof(atomic64_t),
> --
> 1.9.1

Besides the comment above, it looks good to me.

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

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]