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)... > > 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