From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753441AbcADLYw (ORCPT ); Mon, 4 Jan 2016 06:24:52 -0500 Received: from mail-wm0-f48.google.com ([74.125.82.48]:35486 "EHLO mail-wm0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753153AbcADLYu (ORCPT ); Mon, 4 Jan 2016 06:24:50 -0500 Subject: Re: [PATCH] lightnvm: add full block direct to the gc list To: Wenwei Tao References: <1451901289-27149-1-git-send-email-ww.tao0320@gmail.com> Cc: linux-kernel@vger.kernel.org, linux-block@vger.kernel.org From: =?UTF-8?Q?Matias_Bj=c3=b8rling?= Message-ID: <568A567F.5020108@lightnvm.io> Date: Mon, 4 Jan 2016 12:24:47 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <1451901289-27149-1-git-send-email-ww.tao0320@gmail.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/04/2016 10:54 AM, Wenwei Tao wrote: > We allocate gcb to queue full block to the gc list, > but gcb allocation may fail, if that happens, the > block will not get reclaimed. So add the full block > direct to the gc list, omit the queuing step. > > Signed-off-by: Wenwei Tao > --- > drivers/lightnvm/rrpc.c | 47 ++++++++++------------------------------------- > 1 file changed, 10 insertions(+), 37 deletions(-) > > diff --git a/drivers/lightnvm/rrpc.c b/drivers/lightnvm/rrpc.c > index 40b0309..27fb98d 100644 > --- a/drivers/lightnvm/rrpc.c > +++ b/drivers/lightnvm/rrpc.c > @@ -475,24 +475,6 @@ static void rrpc_lun_gc(struct work_struct *work) > /* TODO: Hint that request queue can be started again */ > } > > -static void rrpc_gc_queue(struct work_struct *work) > -{ > - struct rrpc_block_gc *gcb = container_of(work, struct rrpc_block_gc, > - ws_gc); > - struct rrpc *rrpc = gcb->rrpc; > - struct rrpc_block *rblk = gcb->rblk; > - struct nvm_lun *lun = rblk->parent->lun; > - struct rrpc_lun *rlun = &rrpc->luns[lun->id - rrpc->lun_offset]; > - > - spin_lock(&rlun->lock); > - list_add_tail(&rblk->prio, &rlun->prio_list); > - spin_unlock(&rlun->lock); > - > - mempool_free(gcb, rrpc->gcb_pool); > - pr_debug("nvm: block '%lu' is full, allow GC (sched)\n", > - rblk->parent->id); > -} > - > static const struct block_device_operations rrpc_fops = { > .owner = THIS_MODULE, > }; > @@ -620,39 +602,30 @@ err: > return NULL; > } > > -static void rrpc_run_gc(struct rrpc *rrpc, struct rrpc_block *rblk) > -{ > - struct rrpc_block_gc *gcb; > - > - gcb = mempool_alloc(rrpc->gcb_pool, GFP_ATOMIC); > - if (!gcb) { > - pr_err("rrpc: unable to queue block for gc."); > - return; > - } > - > - gcb->rrpc = rrpc; > - gcb->rblk = rblk; > - > - INIT_WORK(&gcb->ws_gc, rrpc_gc_queue); > - queue_work(rrpc->kgc_wq, &gcb->ws_gc); > -} > - > static void rrpc_end_io_write(struct rrpc *rrpc, struct rrpc_rq *rrqd, > sector_t laddr, uint8_t npages) > { > struct rrpc_addr *p; > struct rrpc_block *rblk; > struct nvm_lun *lun; > + struct rrpc_lun *rlun; > int cmnt_size, i; > > for (i = 0; i < npages; i++) { > p = &rrpc->trans_map[laddr + i]; > rblk = p->rblk; > lun = rblk->parent->lun; > + rlun = &rrpc->luns[lun->id - rrpc->lun_offset]; > > cmnt_size = atomic_inc_return(&rblk->data_cmnt_size); > - if (unlikely(cmnt_size == rrpc->dev->pgs_per_blk)) > - rrpc_run_gc(rrpc, rblk); > + if (unlikely(cmnt_size == rrpc->dev->pgs_per_blk)) { > + pr_debug("nvm: block '%lu' is full, allow GC (sched)\n", > + rblk->parent->id); > + spin_lock(&rlun->lock); A deadlock might occur, as the lock can be called from interrupt context. The other ->rlun usages will have to be converted to spinlock_irqsave/spinlock_irqrestore to be valid. The reason for the queueing is that the ->rlun lock is held for a while in rrpc_lun_gc. Therefore, it rather takes the queueing overhead, than disable interrupts on the CPU for the duration of the ->prio sorting and selection of victim block. My assumptions about this optimization might be premature. So I like to be proved wrong. > + list_add_tail(&rblk->prio, &rlun->prio_list); > + spin_unlock(&rlun->lock); > + > + } > } > } > >