Re: [PATCH] lightnvm: pblk: take bitmap alloc. out of critical section

["Javier González" <javier@javigon.com>]

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

> On 17 Apr 2018, at 04.48, Matias Bjørling <mb@lightnvm.io> wrote:
> 
> On 4/16/18 12:21 PM, Javier González wrote:
>> Allocate line bitmaps outside of the line lock on line preparation.
>> Signed-off-by: Javier González <javier@cnexlabs.com>
> 
> 
> The patch description tells what the patch does, it should tell why
> the change the done.
> 

Taking an allocation out of a critical zone should be a reason on itself.

>> ---
>>  drivers/lightnvm/pblk-core.c | 96 +++++++++++++++++++++++++-------------------
>>  1 file changed, 55 insertions(+), 41 deletions(-)
>> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
>> index 5f960a6609c8..7762e89984ee 100644
>> --- a/drivers/lightnvm/pblk-core.c
>> +++ b/drivers/lightnvm/pblk-core.c
>> @@ -1058,6 +1058,25 @@ static int pblk_line_init_metadata(struct pblk *pblk, struct pblk_line *line,
>>  	return 1;
>>  }
>>  +static int pblk_line_alloc_bitmaps(struct pblk *pblk, struct pblk_line *line)
>> +{
>> +	struct pblk_line_meta *lm = &pblk->lm;
>> +
>> +	line->map_bitmap = kzalloc(lm->sec_bitmap_len, GFP_KERNEL);
>> +	if (!line->map_bitmap)
>> +		return -ENOMEM;
>> +
>> +	/* will be initialized using bb info from map_bitmap */
>> +	line->invalid_bitmap = kmalloc(lm->sec_bitmap_len, GFP_KERNEL);
>> +	if (!line->invalid_bitmap) {
>> +		kfree(line->map_bitmap);
>> +		line->map_bitmap = NULL;
>> +		return -ENOMEM;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  /* For now lines are always assumed full lines. Thus, smeta former and current
>>   * lun bitmaps are omitted.
>>   */
>> @@ -1162,18 +1181,7 @@ static int pblk_line_prepare(struct pblk *pblk, struct pblk_line *line)
>>  {
>>  	struct pblk_line_meta *lm = &pblk->lm;
>>  	int blk_in_line = atomic_read(&line->blk_in_line);
>> -	int blk_to_erase, ret;
>> -
>> -	line->map_bitmap = kzalloc(lm->sec_bitmap_len, GFP_ATOMIC);
>> -	if (!line->map_bitmap)
>> -		return -ENOMEM;
>> -
>> -	/* will be initialized using bb info from map_bitmap */
>> -	line->invalid_bitmap = kmalloc(lm->sec_bitmap_len, GFP_ATOMIC);
>> -	if (!line->invalid_bitmap) {
>> -		ret = -ENOMEM;
>> -		goto fail_free_map_bitmap;
>> -	}
>> +	int blk_to_erase;
>>    	/* Bad blocks do not need to be erased */
>>  	bitmap_copy(line->erase_bitmap, line->blk_bitmap, lm->blk_per_line);
>> @@ -1191,15 +1199,15 @@ static int pblk_line_prepare(struct pblk *pblk, struct pblk_line *line)
>>  	}
>>    	if (blk_in_line < lm->min_blk_line) {
>> -		ret = -EAGAIN;
>> -		goto fail_free_invalid_bitmap;
>> +		spin_unlock(&line->lock);
>> +		return -EAGAIN;
>>  	}
>>    	if (line->state != PBLK_LINESTATE_FREE) {
>>  		WARN(1, "pblk: corrupted line %d, state %d\n",
>>  							line->id, line->state);
>> -		ret = -EINTR;
>> -		goto fail_free_invalid_bitmap;
>> +		spin_unlock(&line->lock);
>> +		return -EINTR;
>>  	}
>>    	line->state = PBLK_LINESTATE_OPEN;
>> @@ -1213,16 +1221,6 @@ static int pblk_line_prepare(struct pblk *pblk, struct pblk_line *line)
>>  	kref_init(&line->ref);
>>    	return 0;
>> -
>> -fail_free_invalid_bitmap:
>> -	spin_unlock(&line->lock);
>> -	kfree(line->invalid_bitmap);
>> -	line->invalid_bitmap = NULL;
>> -fail_free_map_bitmap:
>> -	kfree(line->map_bitmap);
>> -	line->map_bitmap = NULL;
>> -
>> -	return ret;
>>  }
>>    int pblk_line_recov_alloc(struct pblk *pblk, struct pblk_line *line)
>> @@ -1242,13 +1240,15 @@ int pblk_line_recov_alloc(struct pblk *pblk, struct pblk_line *line)
>>  	}
>>  	spin_unlock(&l_mg->free_lock);
>>  -	pblk_rl_free_lines_dec(&pblk->rl, line, true);
>> +	if (pblk_line_alloc_bitmaps(pblk, line))
>> +		return -EINTR;
> 
> Why return -EINTR, instead of the return value from (0, -ENOMEM) from pblk_line_alloc_bitmap?

Sure.

> 
> 
>>    	if (!pblk_line_init_bb(pblk, line, 0)) {
>>  		list_add(&line->list, &l_mg->free_list);
>>  		return -EINTR;
>>  	}
>>  +	pblk_rl_free_lines_dec(&pblk->rl, line, true);
>>  	return 0;
>>  }
>>  @@ -1260,6 +1260,24 @@ void pblk_line_recov_close(struct pblk *pblk, struct pblk_line *line)
>>  	line->emeta = NULL;
>>  }
>>  +static void pblk_line_clear(struct pblk_line *line)
>> +{
>> +	*line->vsc = cpu_to_le32(EMPTY_ENTRY);
>> +
>> +	line->map_bitmap = NULL;
>> +	line->invalid_bitmap = NULL;
>> +	line->smeta = NULL;
>> +	line->emeta = NULL;
>> +}
> 
> Instead of pblk_line_clear, how about pblk_line_reinit?

Emmmmm... yes, why not.

I'll resend.

Javier

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