On Nov 4, 2008, at 3:26 PM, Tom Haynes wrote:
> Sam Falkner wrote:
>> On Nov 4, 2008, at 12:22 PM, Tom Haynes wrote:
>>
>>
>>> ----------------
>>>
>>>
>>> 584 if (found) {
>>> 585 /*
>>> 586 * Found it. Since we're holding the bucket
>>> lock,
>>> 587 * we know that any garbage-collection thread
>>> cannot
>>> 588 * free the nnode. Increment its refcount and
>>> we're
>>> 589 * done.
>>> 590 */
>>> 591 mutex_enter(&found->nn_lock);
>>> 592 found->nn_refcount++;
>>>
>>>
>>> You could get integer wrap at 592 and mark the node as okay to reap.
>>
>> Or I could put in a 1-second sleep, stretching it out to 136 years or
>> so. Just kidding; ASSERT() added.
>>
> We seem to spend time adding this type of code to the ref count
> implementations after we ship them
> and customers start to hit edge cases. And they are typically P1
> bugs at
> that point.
>
> Is it defensive programming, sure. But better to do it earlier...
I agree; I was just trying to be funny.
* * *
Thanks for the review! I will post a revised webrev once all the
reviews trickle in.
- Sam
>
>
>
>>
>>
>>> -----------
>>>
>>> 2676 resp->mblk = mp;
>>>
>>> Are all the consumers protected?
>>
>> xdr_READ4res() does the right thing if resp->mblk is NULL. Is that
>> what you meant?
>>
>
> Yes.
>
>>
>>>
>>> usr/src/uts/common/fs/nfs/nfs_nnode_vn.c
>>>
>>> 659 data->nvd_vp = vp;
>>> 660 /* no need to VN_HOLD; we steal the reference */
>>> 661 md = nnode_vn_md_alloc();
>>> 662 md->nvm_vp = vp;
>>> 663 VN_HOLD(md->nvm_vp);
>>> 664 state = nnode_vn_state_alloc();
>>> 665 state->nvs_vp = vp;
>>> 666 VN_HOLD(state->nvs_vp);
>>>
>>>
>>> You might mention what we don't have to VN_HOLD, i.e., data->nvd_vp,
>>> because at first blush,
>>> we do hold 3 lines down....
>>>
>>> ------------
>>>
>>> 753 /* no need to VN_HOLD, we steal the reference */
>>>
>>> 803 /* no need for VN_HOLD, we steal the reference */
>>>
>>> And instead of being buried down inside the functions, perhaps this
>>> can be documented at the
>>> top of the functions as to which *vp get held (and thus need to be
>>> released) and which
>>> ones are stolen...
>>
>> It is a bit confusing, but the end result is each instance of
>> nnode_vn_*_t will have one vnode_t pointer, held one time, so that
>> one
>> VN_RELE() will release it. What if I added a block comment above the
>> nnode_build_* function to that effect?
>>
>
> Yes - one block comment would do.
>
>>
>>> --------
>>>
>>> =================
>>>
>>> usr/src/uts/common/nfs/nnode_vn.h
>>>
>>>
>>> 64 /*
>>> 65 * NB: The firsts fields of nnode_fid_key_v41_t must be the same
>>> as all
>>> 66 * of the fields in nnode_fid_key_t.
>>> 67 */
>>> 68
>>> 69 typedef struct {
>>> 70 fsid_t *nfk_fsid;
>>> 71 fid_t *nfk_fid;
>>> 72 uint32_t *nfk_other;
>>> 73 fid_t *nfk_xfid;
>>>
>>> #define NNODE_FID_FIELDS \
>>> fsid_t *nfk_fsid;
>>> fid_t *nfk_fid;
>>> uint32_t *nfk_other;
>>>
>>> And:
>>>
>>> 69 typedef struct {
>>> 70 NNODE_FID_FIELDS
>>> 73 fid_t *nfk_xfid;
>>>
>>> I.e., get the compiler to enforce your comments...
>>
>> Interesting! I'll think about this. I'm a little reluctant to do it
>> right away, because this could all be simplified when and if we go to
>> the "hang the nnodes off of the exportinfo" thing. In that scenario,
>> our compare functions don't need to be nearly as generic. Instead,
>> they would be comparing a contiguous chunk of opaque bytes.
>>
>> - Sam
>
> It isn't a must, so no need to do ...
> _______________________________________________
> nfsv41-discuss mailing list
> nfsv41-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/nfsv41-discuss
