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

Jason Wang <jasowang@xxxxxxxxxx> · Mon, 01 Dec 2014 08:30:43 +0008

On Fri, Nov 28, 2014 at 7:54 PM, Dexuan Cui <decui@xxxxxxxxxxxxx> wrote:
 -----Original Message-----
 From: Jason Wang [mailto:jasowang@xxxxxxxxxx]
 Sent: Friday, November 28, 2014 18:13 PM
 To: Dexuan Cui
 Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; 
driverdev-
 devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx; KY
 Srinivasan; vkuznets@xxxxxxxxxx; Haiyang Zhang
 Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on 
transfer
 failure
 On Fri, Nov 28, 2014 at 4:36 PM, Dexuan Cui <decui@xxxxxxxxxxxxx> 
wrote:
 >>  -----Original Message-----
 >>  From: Jason Wang [mailto:jasowang@xxxxxxxxxx]
 >>  Sent: Friday, November 28, 2014 14:47 PM
 >>  To: Dexuan Cui
 >>  Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
 >> driverdev-
 >>  devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; 
apw@xxxxxxxxxxxxx; KY
 >>  Srinivasan; vkuznets@xxxxxxxxxx; Haiyang Zhang
 >>  Subject: Re: [PATCH v3] hv: hv_fcopy: drop the obsolete message 
on
 >> transfer
 >>  failure
 >>  On Thu, Nov 27, 2014 at 9:09 PM, Dexuan Cui 
<decui@xxxxxxxxxxxxx>
 >> wrote:
 >>  > 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.
 >>  >
 >>  > Cc: Jason Wang <jasowang@xxxxxxxxxx>
 >>  > Cc: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
 >>  > Cc: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
 >>  > Signed-off-by: Dexuan Cui <decui@xxxxxxxxxxxxx>
 >>  > ---
 >>  >
 >>  > 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"
 >>  >
 >>  > v3: I added the code in fcopy_release() as Jason Wang 
suggested.
 >>  >     I removed the pr_debug (it isn't so meaningful)and added a
 >>  > comment instead.
 >>  >
 >>  >  drivers/hv/hv_fcopy.c | 19 +++++++++++++++++++
 >>  >  1 file changed, 19 insertions(+)
 >>  >
 >>  > diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
 >>  > index 23b2ce2..faa6ba6 100644
 >>  > --- a/drivers/hv/hv_fcopy.c
 >>  > +++ b/drivers/hv/hv_fcopy.c
 >>  > @@ -86,6 +86,18 @@ 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.
 >>  > +	 *
 >>  > +	 * NOTE: fcopy_read() happens to get the semaphore (very 
rare)?
 >>  > We're
 >>  > +	 * still OK, because we've reported the failure to the host.
 >>  > +	 */
 >>  > +	if (down_trylock(&fcopy_transaction.read_sema))
 >>  > +		;
 >>
 >>  Sorry, I'm not quite understand how if () ; can help here.
 >>
 >>  Btw, a question not relate to this patch.
 >>
 >>  What happens if a daemon is resume from SIGSTOP and expires the
 >> check
 >>  here?
 > Hi Jason,
 > My idea is: here we need down_trylock(), but in case we can't get 
the
 > semaphore, it's OK anyway:
 >
 > Scenario 1):
 > 1.1: when the daemon is blocked on the pread(), the daemon 
receives
 > SIGSTOP;
 > 1.2: the host user runs the PowerShell Copy-VMFile command;
 > 1.3.1: the driver reports the failure to the host user in 5s and
 > 1.3.2: the driver down()-es the semaphore;
 > 1.4: the daemon receives SIGCONT and it will be still blocked on 
the
 > pread().
 > Without the down_trylock(), in 1.4, the daemon can receive an
 > obsolete message.
 > NOTE: in this scenario, the daemon is not killed.
 >
 > Scenario 2):
 > In senario 1), if the daemon receives SIGCONT between 1.3.1 and 
1.3.2
 > and
 > do down() in fcopy_read(), it will receive the message but: the
 > driver has
 > reported the failure to the host user and the driver's 1.3.2 can't
 > get the
 > semaphore -- IMO this is acceptably OK, though in the VM, an
 > incomplete
 > file will be left there.
 > BTW, I think in the daemon's hv_start_fcopy() we should add a
 > close(target_fd) before open()-ing a new one.

 Right, but how about the case when resuming from SIGSTOP but no 
timeout?
Sorry, I don't understand this:
if no timeout, fcopy_read() will get the semaphore and fcopy_write()
will try to cancel fcopy_work.

Yes.

 Looks like in this case userspace() may wait in down_interruptible()
 until timeout. We probably need something like this:

         if (down_interruptible(&fcopy_transaction.read_sema)) {
                 up(&fcopy_transaction.read_sema);
                 return -EINTR;
         }
until "timeout"?
if the daemon can't get the semaphore, it can only be wake by a 
signal(the
daemon doesn't install handler, so by default most signals will kill 
the daemon).
In case a signal waking up the daemon doesn't kill the daemon, why 
should
we do up()?

True, no need since we do down_trylock() in release().

Btw, there's no EINTR handling in handling pread() return value,
may add such one which should be useful for something like debugging.

 This should synchronize with the timeout work for sure.
 But how about only schedule it after this?
 It does not may sense to start the timer during interrupt
 since the file may not even opened and it may take time
 to handle signals?

 >
 >>  >
 >>  > +
 >>  >  }
 >>  >
 >>  >  static int fcopy_handle_handshake(u32 version)
 >>  > @@ -351,6 +363,13 @@ static int fcopy_release(struct inode 
*inode,
 >>  > struct file *f)
 >>  >  	 */
 >>  >  	in_hand_shake = true;
 >>  >  	opened = false;
 >>  > +
 >>  > +	if (cancel_delayed_work_sync(&fcopy_work)) {
 >>  > +		/* We haven't up()-ed the semaphore(very rare)? */
 >>  > +		if (down_trylock(&fcopy_transaction.read_sema))
 >>  > +			;
 >>
 >>  And this.
 >
 > Scenario 3):
 > When the daemon exits(e.g., SIGKILL received), if there is a
 > fcopy_work
 > pending (scheduled but not start to run yet), we should cancel the
 > work (as you suggested) and down() the semaphore, otherwise, the
 > obsolete message will be received by the next instance of the 
daemon.

 Yes
 >
 >
 > Scenario 4):  in the driver's hv_fcopy_onchannelcallback():
 >         schedule_delayed_work(&fcopy_work, 5*HZ);
 >         ----> if fcopy_release() is running on another vcpu, just
 > before the next line?
 >         fcopy_send_data();
 >
 > In this case, fcopy_release() can cancel fcopy_work, but
 > can't get the semaphore since it hasn't been up()-ed.
 > Hmm, in this case,   fcopy_send_data() will do up()  later, and 
we'll
 > buffer an obsolete message in the driver, and the message will be
 > fetched by the next instance of the daemon...
 >
 > Looks we need a spinlock here?

 Unless fcopy_release() can wait for all data for current transation
 to be received. Spinlock won't help.

 But an idea is let the daemon the handle such cases. E.g make sure 
the
 processing begins with START_COPY and end with COMPLETE/CANCEL_COPY.
 Drop all requests that does not start with START_COPY.

 Thought?
Good idea.
I also think we should reinforce the concept of state machine in the
daemon code.

Yes, it needs.

The daemon/driver communication has so many corner cases...

Looks so, let's first address the issue mentioned in this patch.

I don't have any more comments other than changing

if(down_trylock(&fcopy_transaction.read_sema))
    ;

to 

down_trylock(&fcopy_transaction.read_sema);

Thanks

 >
 >>  >
 >>  > +		fcopy_respond_to_host(HV_E_FAIL);
 >>  > +	}
 >>  >  	return 0;
 >>  >  }
 >>  >
 >>  > --
 >>  > 1.9.1
 >>  >

 -- Dexuan

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