From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCH 09/14] xen/grant-table: add a mechanism to safely unmap pages that are in use Date: Mon, 19 Jan 2015 15:27:02 +0000 Message-ID: <54BD2246.4050101@citrix.com> References: <1421077417-7162-1-git-send-email-david.vrabel@citrix.com> <1421077417-7162-10-git-send-email-david.vrabel@citrix.com> <54B5AAE0.1060503@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YDEEx-0004vC-FK for xen-devel@lists.xenproject.org; Mon, 19 Jan 2015 15:27:43 +0000 In-Reply-To: <54B5AAE0.1060503@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Boris Ostrovsky , David Vrabel , xen-devel@lists.xenproject.org Cc: Jenny Herbert List-Id: xen-devel@lists.xenproject.org On 13/01/15 23:31, Boris Ostrovsky wrote: > On 01/12/2015 10:43 AM, David Vrabel wrote: >> From: Jenny Herbert >> >> 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 >> #include >> #include >> +#include >> #include >> #include >> @@ -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