From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Grover Subject: Re: [PATCH 5/6] target: Allocate se_luns only when used Date: Fri, 07 Mar 2014 10:12:09 -0800 Message-ID: <531A0BF9.9040504@redhat.com> References: <1394144131-31499-1-git-send-email-agrover@redhat.com> <1394144131-31499-6-git-send-email-agrover@redhat.com> <20140307104114.GC14195@infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140307104114.GC14195@infradead.org> Sender: target-devel-owner@vger.kernel.org To: Christoph Hellwig Cc: target-devel@vger.kernel.org, linux-scsi@vger.kernel.org List-Id: linux-scsi@vger.kernel.org On 03/07/2014 02:41 AM, Christoph Hellwig wrote: >> - se_tpg->tpg_lun_list = array_zalloc(TRANSPORT_MAX_LUNS_PER_TPG, >> - sizeof(struct se_lun), GFP_KERNEL); >> + se_tpg->tpg_lun_list = kzalloc(TRANSPORT_MAX_LUNS_PER_TPG * sizeof(void*), >> + GFP_KERNEL); > > Again seems like the pointer array should just be embedded instead of > dynamically allocated. Also given that it's properly typdef the kcalloc > should have used the type instead of void *. And it should have been > kcalloc to start with.. Yup. I posted a "7 of 6" patch that folds the arrays into their parent structures. >> +void core_tpg_free_lun( >> + struct se_portal_group *tpg, >> + struct se_lun *lun) >> +{ >> spin_lock(&tpg->tpg_lun_lock); >> - lun->lun_status = TRANSPORT_LUN_STATUS_FREE; >> + tpg->tpg_lun_list[lun->unpacked_lun] = NULL; >> spin_unlock(&tpg->tpg_lun_lock); >> + >> + kfree(lun); > > I can't see how the synchronization can work without refcounting the lun > structure. The lock just protectes the assignment, but you free it > right after. What happens to how jsut dereferenced it under the lock > but then uses it outside (e.g. core_dev_add_initiator_node_lun_acl). Well you're right, but this is one instance of a larger lio locking/refcounting hairball. This will be addressed in a separate patch series. Regards -- Andy