Re: [Xen-devel] [PATCH 12/14] xen-blkback: safely unmap grants in case they are still in use

[David Vrabel <david.vrabel@xxxxxxxxxx>]

On 23/01/15 14:31, Roger Pau Monné wrote:
> El 19/01/15 a les 16.51, David Vrabel ha escrit:
>> From: Jenny Herbert <jennifer.herbert@xxxxxxxxxx>
>>
>> +static void xen_blkbk_unmap(struct xen_blkif *blkif,
>> +                            struct grant_page *pages[],
>> +                            int num)
>> +{
>> +    struct gnttab_unmap_grant_ref unmap[BLKIF_MAX_SEGMENTS_PER_REQUEST];
>> +    struct page *unmap_pages[BLKIF_MAX_SEGMENTS_PER_REQUEST];
>> +    unsigned int invcount = 0;
>> +    int ret;
>> +
>> +    while (num) {
>> +            unsigned int batch = min(num, BLKIF_MAX_SEGMENTS_PER_REQUEST);
>> +            
>> +            invcount = xen_blkbk_unmap_prepare(blkif, pages, batch,
>> +                                               unmap, unmap_pages);
> 
> I would add:
> 
> BUG_ON(invcount != batch);

I'm confused.  Surely invcount < batch is valid if one or more pages
within this batch are persistently mapped?

>> +            if (invcount) {
>> +                    ret = gnttab_unmap_refs(unmap, NULL, unmap_pages, 
>> invcount);
>>                      BUG_ON(ret);
>> -                    put_free_pages(blkif, unmap_pages, invcount);
>> -                    invcount = 0;
>> +                    xen_blkbk_unmap_done(blkif, unmap_pages, invcount);
>>              }
>> -    }
>> -    if (invcount) {
>> -            ret = gnttab_unmap_refs(unmap, NULL, unmap_pages, invcount);
>> -            BUG_ON(ret);
>> -            put_free_pages(blkif, unmap_pages, invcount);
>> +            pages += batch;
>> +            num -= batch;
> 
> This loop is sort of buggy, it should work, but it is sub-optimal.
> 
> The pages array can contain both persistent grants and normal grants
> (the ones that we should unmap). So blindly increasing the pages pointer
> with batch will mean that we could be iterating over grants that have
> already been freed. It is not really an issue because we set handle to
> BLKBACK_INVALID_HANDLE, but it's a waste.

Again, I don't follow you here.

David


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