Re: [Xen-devel] [PATCH v1 2/7] xen-blkback: use balloon pages for all mappings

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

>> Perhaps you could update the comment from saying 'map this grant' (which
>> implies doing it NOW as opposed to have done it already), and say:
>>
>> /*
>> .. continue using the grant non-persistently. Note that
>> we mapped it in the earlier loop and the earlier if conditional
>> sets pending_handle(pending_req, i) = map[j].handle.
>> */
>>
>>
>>
>>>                       }
>>> -                     pending_handle(pending_req, i) =
>>> -                             persistent_gnts[i]->handle;
>>> -
>>> -                     if (ret)
>>> -                             continue;
>>> -             } else {
>>> -                     pending_handle(pending_req, i) = map[j++].handle;
>>> -                     bitmap_set(pending_req->unmap_seg, i, 1);
>>> -
>>> -                     if (ret)
>>> -                             continue;
>>> +                     persistent_gnt->gnt = map[j].ref;
>>> +                     persistent_gnt->handle = map[j].handle;
>>> +                     persistent_gnt->page = pages[i];
>>
>> Oh boy, that is a confusing. i and j. Keep loosing track which one is which.
>> It lookis right.
>>
>>> +                     if (add_persistent_gnt(&blkif->persistent_gnts,
>>> +                                            persistent_gnt)) {
>>> +                             kfree(persistent_gnt);
>>
>> I would also say 'persisten_gnt = NULL' for extra measure of safety

This loop was not completely right, I failed to set the unmap_seg bit if
we tried to map the grant persistently and failed, we would just jump to
next without doing:

bitmap_set(pending_req->unmap_seg, seg_idx, 1);

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel