Re: [PATCH 09/14] xen/grant-table: add a mechanism to safely unmap pages that are in use

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

On 13/01/15 23:31, Boris Ostrovsky wrote:
> On 01/12/2015 10:43 AM, David Vrabel wrote:
>> From: Jenny Herbert <jennifer.herbert@citrix.com>
>>
>> Introduce gnttab_unmap_refs_async() that can be used to safely unmap
>> pages that may be in use (ref count > 1).  If the pages are in use the
>> unmap is deferred and retried later.  This polling is not very clever
>> but it should be good enough if the cases where the delay is necessary
>> are rare.
>>
>> The initial delay is 5 ms and is increased linearly on each subsequent
>> retry (to reduce load if the page is in use for a long time).
>>
>> This is needed to allow block backends using grant mapping to safely
>> use network storage (block or filesystem based such as iSCSI or NFS).
>>
>> The network storage driver may complete a block request whilst there
>> is a queued network packet retry (because the ack from the remote end
>> races with deciding to queue the retry).  The pages for the retried
>> packet would be grant unmapped and the network driver (or hardware)
>> would access the unmapped page.
[...]
>> --- a/drivers/xen/grant-table.c
>> +++ b/drivers/xen/grant-table.c
>> @@ -42,6 +42,7 @@
>>   #include <linux/io.h>
>>   #include <linux/delay.h>
>>   #include <linux/hardirq.h>
>> +#include <linux/workqueue.h>
>>     #include <xen/xen.h>
>>   #include <xen/interface/xen.h>
>> @@ -817,6 +818,49 @@ int gnttab_unmap_refs(struct
>> gnttab_unmap_grant_ref *unmap_ops,
>>   }
>>   EXPORT_SYMBOL_GPL(gnttab_unmap_refs);
>>   +#define GNTTAB_UNMAP_REFS_DELAY 5
>> +
>> +static void __gnttab_unmap_refs_async(struct gntab_unmap_queue_data*
>> item);
>> +
>> +static void gnttab_unmap_work(struct work_struct *work)
>> +{
>> +    struct gntab_unmap_queue_data
>> +        *unmap_data = container_of(work,
>> +                       struct gntab_unmap_queue_data,
>> +                       gnttab_work.work);
>> +    if (unmap_data->age != UINT_MAX)
>> +        unmap_data->age++;
>> +    __gnttab_unmap_refs_async(unmap_data);
> 
> Should there be a termination condition if pages are never (for some
> definition of "never") released? Return -ETIMEDOUT in done()? Or at
> least print a warning once?

There's not really any sensible error handling that can be done on such
a timeout.  We can't complete the original request and we can't just
forget about it.  Also, any such timeout would be to aid debugging a bug
elsewhere in the system so I'd prefer not to add it until it's needed.

David