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

[David Vrabel <david.vrabel@citrix.com>]

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@citrix.com>
>>
>> +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