From: Andy Grover <agrover@redhat.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: target-devel@vger.kernel.org, linux-scsi@vger.kernel.org
Subject: Re: [PATCH 5/6] target: Allocate se_luns only when used
Date: Fri, 07 Mar 2014 10:12:09 -0800 [thread overview]
Message-ID: <531A0BF9.9040504@redhat.com> (raw)
In-Reply-To: <20140307104114.GC14195@infradead.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
next prev parent reply other threads:[~2014-03-07 18:12 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-06 22:15 [PATCH 0/6] target: Save memory on unused se_dev_entrys and se_luns Andy Grover
2014-03-06 22:15 ` [PATCH 1/6] target: Allocate se_dev_entrys in device list only when used Andy Grover
2014-03-07 10:33 ` Christoph Hellwig
2014-03-06 22:15 ` [PATCH 2/6] target: core_tpg_post_dellun can return void Andy Grover
2014-03-06 22:15 ` [PATCH 3/6] target: Change core_dev_del_lun to take a se_lun instead of unpacked_lun Andy Grover
2014-03-07 10:35 ` Christoph Hellwig
2014-03-07 18:20 ` Andy Grover
2014-03-06 22:15 ` [PATCH 4/6] target: Rename core_tpg_post_dellun to remove_lun Andy Grover
2014-03-06 22:15 ` [PATCH 5/6] target: Allocate se_luns only when used Andy Grover
2014-03-07 10:41 ` Christoph Hellwig
2014-03-07 18:12 ` Andy Grover [this message]
2014-03-07 18:22 ` Christoph Hellwig
2014-03-07 18:31 ` Andy Grover
2014-03-12 19:04 ` Nicholas A. Bellinger
2014-03-06 22:15 ` [PATCH 6/6] target: Remove core_tpg_release_virtual_lun0 function Andy Grover
2014-03-07 17:50 ` [PATCH 7/6] target: Do not allocate device_list and tpg_lun_list arrays separately Andy Grover
2014-03-10 22:45 ` [PATCHv2 0/9] target: Save memory on unused se_dev_entrys and se_luns Andy Grover
2014-03-10 22:45 ` [PATCHv2 1/9] target: Add locking to some accesses to nacl.device_list Andy Grover
2014-03-10 22:45 ` [PATCHv2 2/9] target: Don't unlock/relock tpg_lun_lock in loop in add_node_to_devs Andy Grover
2014-03-10 22:45 ` [PATCHv2 3/9] target: Allocate se_dev_entrys in device list only when used Andy Grover
2014-03-10 22:45 ` [PATCHv2 4/9] target: core_tpg_post_dellun can return void Andy Grover
2014-03-10 22:45 ` [PATCHv2 5/9] target: Change core_dev_del_lun to take a se_lun instead of unpacked_lun Andy Grover
2014-03-10 22:45 ` [PATCHv2 6/9] target: Rename core_tpg_post_dellun to remove_lun Andy Grover
2014-03-10 22:45 ` [PATCHv2 7/9] target: Allocate se_luns only when used Andy Grover
2014-03-10 22:45 ` [PATCHv2 8/9] target: Remove core_tpg_release_virtual_lun0 function Andy Grover
2014-03-10 22:45 ` [PATCHv2 9/9] target: Refactor core_enable_device_list_for_node Andy Grover
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=531A0BF9.9040504@redhat.com \
--to=agrover@redhat.com \
--cc=hch@infradead.org \
--cc=linux-scsi@vger.kernel.org \
--cc=target-devel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).