On Dec 23, 2010, at 4:05 PM, Trond Myklebust wrote:
> On Thu, 2010-12-23 at 15:08 -0500, Fred Isaman wrote:
>> On Dec 23, 2010, at 2:25 PM, Trond Myklebust wrote:
>>
>>> On Thu, 2010-12-23 at 12:29 -0500, Fred Isaman wrote:
>>>> +
>>>> + if (args->cbl_recall_type == RETURN_FILE) {
>>>> + LIST_HEAD(free_me_list);
>>>> +
>>>> + args->cbl_inode = NULL;
>>>> + spin_lock(&clp->cl_lock);
>>>> + list_for_each_entry(lo, &clp->cl_layouts, plh_layouts) {
>>>> + if (nfs_compare_fh(&args->cbl_fh,
>>>> + &NFS_I(lo->plh_inode)->fh))
>>>> + continue;
>>>> + if (test_bit(NFS_LAYOUT_BULK_RECALL, &lo->plh_flags))
>>>> + rv = NFS4ERR_DELAY;
>>>> + else {
>>>> + /* Without this, layout can be freed as soon
>>>> + * as we release cl_lock. Matched in
>>>> + * do_callback_layoutrecall.
>>>> + */
>>>> + get_layout_hdr(lo);
>>>> + args->cbl_inode = lo->plh_inode;
>>>> + rv = NFS4_OK;
>>>> + }
>>>> + break;
>>>> + }
>>>> + spin_unlock(&clp->cl_lock);
>>>> +
>>>> + spin_lock(&lo->plh_inode->i_lock);
>>>
>>> What guarantees do you have that the inode still exists here? Does the
>>> layout segment hold a reference to it?
>>>
>>
>> This section is why I once asked about whether the layout needs to reference the inode. I believe the answer is nothing. The lseg/layout definitely does not hold direct references. I assume doing grab/put of inode here and below is acceptable?
>
> igrab/iput is fine in this context.
>
> There is also the question: what if we don't find a match for the layout
> recall arguments? It looks to me as if we end up with 'lo ==
> clp->cl_layouts', with disastrous consequences when we then do
> spin_lock(lo->plh_inode->i_lock)...
>
gah, you're right. I'll get that fixed too.
Fred
>>> Also, How do you know that (!test_bit(NFS_LAYOUT_BULK_RECALL,
>>> &lo->plh_flags)) is still valid when you get here?
>>>
>>
>> Because it can not be set elsewhere while the NFS4CLNT_LAYOUTRECALL bit is set.
>
> OK. One race averted :-)
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> Trond.Myklebust@xxxxxxxxxx
> www.netapp.com
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
