Re: [PATCH 08/20] lightnvm: pblk: sched. metadata on write thread

[Rakesh Pandit]

Hi Javier,

one small issue I found for error path while going through changes:

On Mon, Jun 26, 2017 at 11:57:17AM +0200, Javier González wrote:
..
> +static int pblk_lines_alloc_metadata(struct pblk *pblk)
> +{
> +	struct pblk_line_mgmt *l_mg = &pblk->l_mg;
> +	struct pblk_line_meta *lm = &pblk->lm;
> +	int i;
> +
> +	/* smeta is always small enough to fit on a kmalloc memory allocation,
> +	 * emeta depends on the number of LUNs allocated to the pblk instance
> +	 */
> +	l_mg->smeta_alloc_type = PBLK_KMALLOC_META;
> +	for (i = 0; i < PBLK_DATA_LINES; i++) {
> +		l_mg->sline_meta[i] = kmalloc(lm->smeta_len, GFP_KERNEL);
> +		if (!l_mg->sline_meta[i])
> +			goto fail_free_smeta;

For this error path the the goto label at end doesn't free up
resources correctly.  It needs a

while (--index >= 0)...

logic with appropriate adjustment.

> +	}
> +
> +	/* emeta allocates three different buffers for managing metadata with
> +	 * in-memory and in-media layouts
> +	 */
> +	for (i = 0; i < PBLK_DATA_LINES; i++) {
> +		struct pblk_emeta *emeta;
> +
> +		emeta = kmalloc(sizeof(struct pblk_emeta), GFP_KERNEL);
> +		if (!emeta)
> +			goto fail_free_emeta;
> +
> +		if (lm->emeta_len[0] > KMALLOC_MAX_CACHE_SIZE) {
> +			l_mg->emeta_alloc_type = PBLK_VMALLOC_META;
> +
> +			emeta->buf = vmalloc(lm->emeta_len[0]);
> +			if (!emeta->buf) {
> +				kfree(emeta);
> +				goto fail_free_emeta;
> +			}
> +
> +			emeta->nr_entries = lm->emeta_sec[0];
> +			l_mg->eline_meta[i] = emeta;
> +		} else {
> +			l_mg->emeta_alloc_type = PBLK_KMALLOC_META;
> +
> +			emeta->buf = kmalloc(lm->emeta_len[0], GFP_KERNEL);
> +			if (!emeta->buf) {
> +				kfree(emeta);
> +				goto fail_free_emeta;
> +			}
> +
> +			emeta->nr_entries = lm->emeta_sec[0];
> +			l_mg->eline_meta[i] = emeta;
> +		}
> +	}
> +
> +	l_mg->vsc_list = kcalloc(l_mg->nr_lines, sizeof(__le32), GFP_KERNEL);
> +	if (!l_mg->vsc_list)
> +		goto fail_free_emeta;
> +
> +	for (i = 0; i < l_mg->nr_lines; i++)
> +		l_mg->vsc_list[i] = cpu_to_le32(EMPTY_ENTRY);
> +
> +	return 0;
> +
> +fail_free_emeta:
> +	while (--i >= 0) {
> +		vfree(l_mg->eline_meta[i]->buf);

This would need l_mg->emeta_alloc_type check to decide whether
allocation was done with kmalloc or vmalloc.

> +		kfree(&l_mg->eline_meta[i]);
> +	}
> +
> +fail_free_smeta:
> +	for (i = 0; i < PBLK_DATA_LINES; i++)
> +		pblk_mfree(&l_mg->sline_meta[i], l_mg->smeta_alloc_type);
> +
> +	return -ENOMEM;
> +}
> +

Thanks,