RE: [PATCH v2] hv: hv_fcopy: drop the obsolete message on transfer failure

[Jason Wang]

On Thu, Nov 27, 2014 at 4:50 PM, Dexuan Cui <de...@microsoft.com> wrote:

 -----Original Message-----
 From: Jason Wang [mailto:jasow...@redhat.com]
 Sent: Thursday, November 27, 2014 15:15 PM
 To: Dexuan Cui

Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; driverdev-

 de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com; KY
 Srinivasan; vkuzn...@redhat.com; Haiyang Zhang

Subject: Re: [PATCH v2] hv: hv_fcopy: drop the obsolete message on transfer

 failure
 ----- Original Message -----
 > In the case the user-space daemon crashes, hangs or is killed, we

> need to down the semaphore, otherwise, after the daemon starts next

 > time, the obsolete data in fcopy_transaction.message or
 > fcopy_transaction.fcopy_msg will be used immediately.
 >
 > Reviewed-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
 > Cc: K. Y. Srinivasan <k...@microsoft.com>
 > Signed-off-by: Dexuan Cui <de...@microsoft.com>
 > ---
 >
 > v2: I removed the "FCP" prefix as Greg asked.
 >
 >     I also updated the output message a little:
 >     "FCP: failed to acquire the semaphore" -->
 >     "can not acquire the semaphore: it is benign"
 >
 >  drivers/hv/hv_fcopy.c | 9 +++++++++
 >  1 file changed, 9 insertions(+)
 >
 > diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
 > index 23b2ce2..c518ad9 100644
 > --- a/drivers/hv/hv_fcopy.c
 > +++ b/drivers/hv/hv_fcopy.c
 > @@ -86,6 +86,15 @@ static void fcopy_work_func(struct work_struct
 *dummy)
 >    * process the pending transaction.
 >    */
 >   fcopy_respond_to_host(HV_E_FAIL);
 > +

> + /* In the case the user-space daemon crashes, hangs or is killed, we > + * need to down the semaphore, otherwise, after the daemon starts

 next
 > +  * time, the obsolete data in fcopy_transaction.message or
 > +  * fcopy_transaction.fcopy_msg will be used immediately.
 > +  */

Looks still racy, what happens if the daemon start before down_trylock()

 but after fcopy_respont_to_host() here?

Jason,
Thanks for pointing this out!
IMO we can resolve this by adding down_trylock() in fcopy_release().
What's your opinion?

Looks better and need to cancel the timeout also here?

> + if (down_trylock(&fcopy_transaction.read_sema))

 > +         pr_debug("can not acquire the semaphore: it is benign\n");

typo

 > +
 >  }

Sorry -- what typo do you mean?

s/benign/begin/ ?

Thanks,
-- Dexuan
�NrybXǧv^)޺{.n+{zXܨ}Ơz&j:+vzZ++zfh~izw?&)ߢf^jǫym@Aa0hi

_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel