hv: hv_fcopy: drop the obsolete message on transfer failure

PATCH Superseded Dexuan Cui 4 participants 17 messages Nov 27, 2014 – Jan 14, 2015 Hyper-V/Azure core and drivers

Share & tools

Dexuan Cui

author maintainer

2014-11-27 11:59 UTC #0 lore raw

PATCH v3 1/1 hv: hv_fcopy: drop the obsolete message on transfer failure

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 <[email protected]>

Cc Vitaly Kuznetsov <[email protected]>

Cc K. Y. Srinivasan <[email protected]>

Signed-off-by Dexuan Cui <[email protected]>

---

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(+)

drivers/hv/hv_fcopy.c +19 -0

index 23b2ce2..faa6ba6 100644
@@ -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))
+		;
+
 }
 
 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))
+			;
+		fcopy_respond_to_host(HV_E_FAIL);
+	}
 	return 0;
 }

Signature · 1 line

1.9.1

Jason Wang

2014-11-28 06:47 UTC #1 lore raw

Quoted 42 lines · Dexuan Cui

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 <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
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(+)
drivers/hv/hv_fcopy.c +3 -0
index 23b2ce2..faa6ba6 100644
@@ -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
drivers/hv/hv_fcopy.c +4 -0
+	 * 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

drivers/hv/hv_fcopy.c static void fcopy_work_func(struct work_struct *dummy) · line 99

Quoted 4 lines

drivers/hv/hv_fcopy.c +4 -0

+	 * 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?

Quoted 6 lines

drivers/hv/hv_fcopy.c +1 -0

+
 }
 
 static int fcopy_handle_handshake(u32 version)

drivers/hv/hv_fcopy.c static int fcopy_release(struct inode *inode, · line 371

Quoted 9 lines

drivers/hv/hv_fcopy.c +5 -0

@@ -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.

Quoted 14 lines

drivers/hv/hv_fcopy.c +2 -0

+		fcopy_respond_to_host(HV_E_FAIL);
+	}
 	return 0;
 }

Signature · 8 lines

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Dexuan Cui

author maintainer

2014-11-28 08:36 UTC #2 lore raw

Quoted 59 lines

-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Friday, November 28, 2014 14:47 PM
To: Dexuan Cui
Cc: [email protected]; [email protected]; driverdev- [email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; Haiyang Zhang
Subject: Re: [PATCH v3] hv: hv_fcopy: drop the obsolete message on transfer failure
Quoted 42 lines · Dexuan Cui
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 <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
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(+)
drivers/hv/hv_fcopy.c +3 -0
index 23b2ce2..faa6ba6 100644
@@ -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
drivers/hv/hv_fcopy.c +4 -0
+	 * 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
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct work_struct *dummy) · line 99
Quoted 4 lines
drivers/hv/hv_fcopy.c +4 -0
+	 * 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.

Quoted 4 lines

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.

Quoted 4 lines

Quoted 4 lines
+
}

Quoted 13 lines

drivers/hv/hv_fcopy.c static int fcopy_release(struct inode *inode, · line 371
Quoted 11 lines
static int fcopy_handle_handshake(u32 version)
drivers/hv/hv_fcopy.c +5 -0
@@ -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.

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?

Please share your idea.

Quoted 9 lines

Quoted 9 lines
drivers/hv/hv_fcopy.c +2 -1
+		fcopy_respond_to_host(HV_E_FAIL);
+	}
 	return 0;
 }
 
--
1.9.1

-- Dexuan

��{.n�+��+%��ݶ��w��{.n�+��{��G��{ay�ʇڙ�,j��f��h��z_�(�階�ݢj"��m��G��?��&��~��iO��z��v�^�m��?�I�

Jason Wang

2014-11-28 10:13 UTC #3 lore raw

Quoted 83 lines · Dexuan Cui

Quoted 65 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Friday, November 28, 2014 14:47 PM
To: Dexuan Cui
Cc: [email protected]; [email protected];
driverdev-
[email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; Haiyang Zhang
Subject: Re: [PATCH v3] hv: hv_fcopy: drop the obsolete message on
transfer
failure
Quoted 2 lines · Dexuan Cui
In the case the user-space daemon crashes, hangs or is killed, we need to down the semaphore, otherwise, after the daemon starts
next
Quoted 27 lines
time, the obsolete data in fcopy_transaction.message or fcopy_transaction.fcopy_msg will be used immediately.
Cc Jason Wang <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
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(+)
drivers/hv/hv_fcopy.c +0 -0
index 23b2ce2..faa6ba6 100644
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct work_struct · line 91
Quoted 6 lines
drivers/hv/hv_fcopy.c +2 -0
@@ -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
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct work_struct · line 92
Quoted 1 line
drivers/hv/hv_fcopy.c +1 -0
+	 * need to down the semaphore, otherwise, after the daemon 
starts
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct work_struct *dummy) · line 99
Quoted 10 lines
next
drivers/hv/hv_fcopy.c +4 -0
+	 * 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
drivers/hv/hv_fcopy.c +4 -0
+	 * 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.
Quoted 5 lines
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.

Quoted 8 lines

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?

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;
       }

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?

Quoted 17 lines

Quoted 16 lines

drivers/hv/hv_fcopy.c +6 -0

+
 }
 
 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))
+			;

Quoted 7 lines

Quoted 1 line
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

Quoted 8 lines

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();

Quoted 7 lines

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?

Thanks

Quoted 17 lines

Please share your idea.
Quoted 9 lines
Quoted 9 lines
drivers/hv/hv_fcopy.c +2 -1
+		fcopy_respond_to_host(HV_E_FAIL);
+	}
 	return 0;
 }
 
--
1.9.1
-- Dexuan
��NrybXǧv^)޺{.n+{zXܨ}Ơz&j:+vzZ++zfh~izw?&)ߢf^jǫym@Aa0hi

Dexuan Cui

author maintainer

2014-11-28 11:55 UTC #4 lore raw

Quoted 109 lines

-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Friday, November 28, 2014 18:13 PM
To: Dexuan Cui
Cc: [email protected]; [email protected]; driverdev- [email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on transfer failure
Quoted 86 lines · Dexuan Cui
Quoted 65 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Friday, November 28, 2014 14:47 PM
To: Dexuan Cui
Cc: [email protected]; [email protected];
driverdev-
[email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; Haiyang Zhang
Subject: Re: [PATCH v3] hv: hv_fcopy: drop the obsolete message on
transfer
failure
Quoted 2 lines · Dexuan Cui
In the case the user-space daemon crashes, hangs or is killed, we need to down the semaphore, otherwise, after the daemon starts
next
Quoted 27 lines
time, the obsolete data in fcopy_transaction.message or fcopy_transaction.fcopy_msg will be used immediately.
Cc Jason Wang <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
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(+)
drivers/hv/hv_fcopy.c +0 -0
index 23b2ce2..faa6ba6 100644
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct work_struct · line 91
Quoted 6 lines
drivers/hv/hv_fcopy.c +2 -0
@@ -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
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct work_struct · line 92
Quoted 1 line
drivers/hv/hv_fcopy.c +1 -0
+	 * need to down the semaphore, otherwise, after the daemon
starts
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct work_struct *dummy) · line 99
Quoted 10 lines
next
drivers/hv/hv_fcopy.c +4 -0
+	 * 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
drivers/hv/hv_fcopy.c +4 -0
+	 * 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.
Quoted 5 lines
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.
Quoted 12 lines
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.

Quoted 1 line

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.

Quoted 7 lines

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()?

Quoted 50 lines

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?
Quoted 17 lines
Quoted 17 lines
Quoted 16 lines
drivers/hv/hv_fcopy.c +6 -0
+
 }
 
 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))
+			;
Quoted 8 lines
Quoted 1 line
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
Quoted 8 lines
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();
Quoted 7 lines
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?

Quoted 8 lines

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.

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

Quoted 10 lines

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
drivers/hv/hv_fcopy.c +2 -1
+		fcopy_respond_to_host(HV_E_FAIL);
+	}
 	return 0;
 }
 
--
1.9.1

-- Dexuan

Jason Wang

2014-12-01 08:23 UTC #5 lore raw

Quoted 123 lines · Dexuan Cui

Quoted 123 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Friday, November 28, 2014 18:13 PM
To: Dexuan Cui
Cc: [email protected]; [email protected];
driverdev-
[email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on
transfer
failure
Quoted 7 lines · Dexuan Cui
Quoted 7 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Friday, November 28, 2014 14:47 PM
To: Dexuan Cui
Cc: [email protected]; [email protected];
driverdev-
[email protected]; [email protected];
[email protected]; KY
Quoted 2 lines
Quoted 2 lines
Srinivasan; [email protected]; Haiyang Zhang
Subject: Re: [PATCH v3] hv: hv_fcopy: drop the obsolete message
on
Quoted 3 lines
Quoted 3 lines
transfer
failure
On Thu, Nov 27, 2014 at 9:09 PM, Dexuan Cui
<[email protected]>
Quoted 2 lines
Quoted 2 lines
Quoted 1 line · wrote:
In the case the user-space daemon crashes, hangs or is
killed, we
Quoted 11 lines
Quoted 11 lines
Quoted 1 line
need to down the semaphore, otherwise, after the daemon starts
next
Quoted 9 lines
time, the obsolete data in fcopy_transaction.message or fcopy_transaction.fcopy_msg will be used immediately.
Cc Jason Wang <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
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.
Quoted 11 lines
Quoted 11 lines
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct
Quoted 11 lines
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(+)
drivers/hv/hv_fcopy.c +0 -0
index 23b2ce2..faa6ba6 100644
@@ -86,6 +86,18 @@ static void fcopy_work_func(struct 
work_struct
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/hv/hv_fcopy.c +2 -0
 *dummy)
 	 * process the pending transaction.
 	 */
 	fcopy_respond_to_host(HV_E_FAIL);
+
+	/* In the case the user-space daemon crashes, hangs or is
Quoted 8 lines
Quoted 8 lines
killed, we
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct · line 92
Quoted 1 line
drivers/hv/hv_fcopy.c +1 -0
+	 * need to down the semaphore, otherwise, after the daemon
starts
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct work_struct *dummy)
Quoted 5 lines
next
drivers/hv/hv_fcopy.c +4 -0
+	 * 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)?
Quoted 8 lines
Quoted 8 lines
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct work_struct *dummy) · line 99
Quoted 5 lines
We're
drivers/hv/hv_fcopy.c +4 -0
+	 * 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.
Quoted 7 lines
Quoted 5 lines
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
Quoted 4 lines
semaphore, it's OK anyway:
Scenario 1):
1.1: when the daemon is blocked on the pread(), the daemon
receives
Quoted 5 lines
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
Quoted 7 lines
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
Quoted 10 lines
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.

Quoted 5 lines

Quoted 2 lines
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.

Quoted 9 lines

Quoted 9 lines
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;
         }

Quoted 5 lines

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.

Quoted 63 lines

Quoted 63 lines
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?
Quoted 7 lines
Quoted 7 lines
drivers/hv/hv_fcopy.c static int fcopy_release(struct inode
Quoted 7 lines
drivers/hv/hv_fcopy.c +1 -0
+
 }
 
 static int fcopy_handle_handshake(u32 version)
@@ -351,6 +363,13 @@ static int fcopy_release(struct inode 
*inode,
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
drivers/hv/hv_fcopy.c +5 -0
 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))
+			;
Quoted 8 lines
Quoted 1 line
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
Quoted 11 lines
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
Quoted 4 lines
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.

Quoted 4 lines

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

Yes, it needs.

Quoted 3 lines

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))

down_trylock(&fcopy_transaction.read_sema);

Thanks

Quoted 14 lines

Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
drivers/hv/hv_fcopy.c +2 -1
+		fcopy_respond_to_host(HV_E_FAIL);
+	}
 	return 0;
 }
 
--
1.9.1
-- Dexuan

Dexuan Cui

author maintainer

2014-12-01 09:48 UTC #6 lore raw

Quoted 225 lines

-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, December 1, 2014 16:23 PM
To: Dexuan Cui
Cc: [email protected]; [email protected]; driverdev- [email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on transfer failure
Quoted 123 lines · Dexuan Cui
Quoted 123 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Friday, November 28, 2014 18:13 PM
To: Dexuan Cui
Cc: [email protected]; [email protected];
driverdev-
[email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on
transfer
failure
Quoted 7 lines · Dexuan Cui
Quoted 7 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Friday, November 28, 2014 14:47 PM
To: Dexuan Cui
Cc: [email protected]; [email protected];
driverdev-
[email protected]; [email protected];
[email protected]; KY
Quoted 2 lines
Quoted 2 lines
Srinivasan; [email protected]; Haiyang Zhang
Subject: Re: [PATCH v3] hv: hv_fcopy: drop the obsolete message
on
Quoted 3 lines
Quoted 3 lines
transfer
failure
On Thu, Nov 27, 2014 at 9:09 PM, Dexuan Cui
<[email protected]>
Quoted 2 lines
Quoted 2 lines
Quoted 1 line · wrote:
In the case the user-space daemon crashes, hangs or is
killed, we
Quoted 11 lines
Quoted 11 lines
Quoted 1 line
need to down the semaphore, otherwise, after the daemon starts
next
Quoted 9 lines
time, the obsolete data in fcopy_transaction.message or fcopy_transaction.fcopy_msg will be used immediately.
Cc Jason Wang <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
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.
Quoted 11 lines
Quoted 11 lines
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct
Quoted 11 lines
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(+)
drivers/hv/hv_fcopy.c +0 -0
index 23b2ce2..faa6ba6 100644
@@ -86,6 +86,18 @@ static void fcopy_work_func(struct
work_struct
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/hv/hv_fcopy.c +2 -0
 *dummy)
 	 * process the pending transaction.
 	 */
 	fcopy_respond_to_host(HV_E_FAIL);
+
+	/* In the case the user-space daemon crashes, hangs or is
Quoted 8 lines
Quoted 8 lines
killed, we
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct · line 92
Quoted 1 line
drivers/hv/hv_fcopy.c +1 -0
+	 * need to down the semaphore, otherwise, after the daemon
starts
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct work_struct *dummy)
Quoted 5 lines
next
drivers/hv/hv_fcopy.c +4 -0
+	 * 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)?
Quoted 8 lines
Quoted 8 lines
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct work_struct *dummy) · line 99
Quoted 5 lines
We're
drivers/hv/hv_fcopy.c +4 -0
+	 * 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.
Quoted 7 lines
Quoted 5 lines
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
Quoted 4 lines
semaphore, it's OK anyway:
Scenario 1):
1.1: when the daemon is blocked on the pread(), the daemon
receives
Quoted 5 lines
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
Quoted 7 lines
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
Quoted 10 lines
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.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 9 lines
Quoted 9 lines
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;
         }
Quoted 8 lines
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.
Quoted 53 lines
Quoted 53 lines
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?
Quoted 7 lines
Quoted 7 lines
drivers/hv/hv_fcopy.c static int fcopy_release(struct inode
Quoted 7 lines
drivers/hv/hv_fcopy.c +1 -0
+
 }
 
 static int fcopy_handle_handshake(u32 version)
@@ -351,6 +363,13 @@ static int fcopy_release(struct inode
*inode,
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
drivers/hv/hv_fcopy.c +5 -0
 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))
+			;
Quoted 8 lines
Quoted 1 line
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
Quoted 11 lines
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
Quoted 3 lines
buffer an obsolete message in the driver, and the message will be fetched by the next instance of the daemon...
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
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.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
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.

I agree.
Obviously we can do something to make the daemon/driver work better in the corner cases.

Quoted 4 lines

Quoted 2 lines
The daemon/driver communication has so many corner cases...
Looks so, let's first address the issue mentioned in this patch.

OK.

Quoted 8 lines

I don't have any more comments other than changing
if(down_trylock(&fcopy_transaction.read_sema))
     ;
to
down_trylock(&fcopy_transaction.read_sema);

Hi Jason,
This is to address Vitaly's comment in the bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=1162100#c5

down_trylock(&fcopy_transaction.read_sema) will

"
produces the following compile warning:
drivers/hv/hv_fcopy.c: In function ‘fcopy_work_func’:
drivers/hv/hv_fcopy.c:95:2: warning: ignoring return value of ‘down_trylock’, declared with attribute warn_unused_result [-Wunused-result]
(void)down_trylock(&fcopy_transaction.read_sema);
"

Actually I personally don't care about the warning, because we only see it when we run some kind of code checker program. :-)

I can change my v3 to the "normal" style you prefer, if there is no strong objection from Vitaly?

Thanks,
-- Dexuan

Vitaly Kuznetsov

2014-12-01 09:55 UTC #7 lore raw

Quoted 264 lines · Dexuan Cui

Quoted 225 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, December 1, 2014 16:23 PM
To: Dexuan Cui
Cc: [email protected]; [email protected]; driverdev- [email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on transfer failure
Quoted 123 lines · Dexuan Cui
Quoted 123 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Friday, November 28, 2014 18:13 PM
To: Dexuan Cui
Cc: [email protected]; [email protected];
driverdev-
[email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on
transfer
failure
Quoted 7 lines · Dexuan Cui
Quoted 7 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Friday, November 28, 2014 14:47 PM
To: Dexuan Cui
Cc: [email protected]; [email protected];
driverdev-
[email protected]; [email protected];
[email protected]; KY
Quoted 2 lines
Quoted 2 lines
Srinivasan; [email protected]; Haiyang Zhang
Subject: Re: [PATCH v3] hv: hv_fcopy: drop the obsolete message
on
Quoted 3 lines
Quoted 3 lines
transfer
failure
On Thu, Nov 27, 2014 at 9:09 PM, Dexuan Cui
<[email protected]>
Quoted 2 lines
Quoted 2 lines
Quoted 1 line · wrote:
In the case the user-space daemon crashes, hangs or is
killed, we
Quoted 11 lines
Quoted 11 lines
Quoted 1 line
need to down the semaphore, otherwise, after the daemon starts
next
Quoted 9 lines
time, the obsolete data in fcopy_transaction.message or fcopy_transaction.fcopy_msg will be used immediately.
Cc Jason Wang <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
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.
Quoted 11 lines
Quoted 11 lines
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct
Quoted 11 lines
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(+)
drivers/hv/hv_fcopy.c +0 -0
index 23b2ce2..faa6ba6 100644
@@ -86,6 +86,18 @@ static void fcopy_work_func(struct
work_struct
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/hv/hv_fcopy.c +2 -0
 *dummy)
 	 * process the pending transaction.
 	 */
 	fcopy_respond_to_host(HV_E_FAIL);
+
+	/* In the case the user-space daemon crashes, hangs or is
Quoted 8 lines
Quoted 8 lines
killed, we
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct · line 92
Quoted 1 line
drivers/hv/hv_fcopy.c +1 -0
+	 * need to down the semaphore, otherwise, after the daemon
starts
drivers/hv/hv_fcopy.c
Quoted 5 lines
next
drivers/hv/hv_fcopy.c +4 -0
+	 * 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)?
Quoted 8 lines
Quoted 8 lines
drivers/hv/hv_fcopy.c
Quoted 5 lines
We're
drivers/hv/hv_fcopy.c +4 -0
+	 * 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.
Quoted 7 lines
Quoted 5 lines
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
Quoted 4 lines
semaphore, it's OK anyway:
Scenario 1):
1.1: when the daemon is blocked on the pread(), the daemon
receives
Quoted 5 lines
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
Quoted 7 lines
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
Quoted 10 lines
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.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 9 lines
Quoted 9 lines
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;
         }
Quoted 8 lines
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.
Quoted 53 lines
Quoted 53 lines
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?
Quoted 7 lines
Quoted 7 lines
drivers/hv/hv_fcopy.c static int fcopy_release(struct inode
Quoted 7 lines
drivers/hv/hv_fcopy.c +1 -0
+
 }
 
 static int fcopy_handle_handshake(u32 version)
@@ -351,6 +363,13 @@ static int fcopy_release(struct inode
*inode,
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
drivers/hv/hv_fcopy.c +5 -0
 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))
+			;
Quoted 8 lines
Quoted 1 line
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
Quoted 11 lines
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
Quoted 3 lines
buffer an obsolete message in the driver, and the message will be fetched by the next instance of the daemon...
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
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.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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.
I agree.
Obviously we can do something to make the daemon/driver work better in the corner cases.
Quoted 4 lines
Quoted 2 lines
The daemon/driver communication has so many corner cases...
Looks so, let's first address the issue mentioned in this patch.
OK.
Quoted 8 lines
I don't have any more comments other than changing
if(down_trylock(&fcopy_transaction.read_sema))
     ;
to
down_trylock(&fcopy_transaction.read_sema);
Hi Jason,
This is to address Vitaly's comment in the bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=1162100#c5
down_trylock(&fcopy_transaction.read_sema) will
"
produces the following compile warning:
drivers/hv/hv_fcopy.c: In function ‘fcopy_work_func’:
drivers/hv/hv_fcopy.c:95:2: warning: ignoring return value of ‘down_trylock’, declared with attribute warn_unused_result [-Wunused-result]
(void)down_trylock(&fcopy_transaction.read_sema);
"

Quoted 5 lines

Actually I personally don't care about the warning, because we only see it when we run some kind of code checker program. :-)
I can change my v3 to the "normal" style you prefer, if there is no strong objection from Vitaly?

Well, I still prefer pr_debug() there: these are being compiled out on non-debug builds. I'm not objecting to any solution here though, really a minor issue.

Quoted 3 lines

Thanks,
-- Dexuan

Signature · 1 line

  Vitaly

Jason Wang

Acked-by

2014-12-01 10:18 UTC #8 lore raw

In reply to Dexuan Cui

Quoted 297 lines · Dexuan Cui

Quoted 259 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, December 1, 2014 16:23 PM
To: Dexuan Cui
Cc: [email protected]; [email protected];
driverdev-
[email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on
transfer
failure
Quoted 7 lines · Dexuan Cui
Quoted 7 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Friday, November 28, 2014 18:13 PM
To: Dexuan Cui
Cc: [email protected]; [email protected];
driverdev-
[email protected]; [email protected];
[email protected]; KY
Quoted 2 lines
Quoted 2 lines
Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message
on
Quoted 3 lines
Quoted 3 lines
transfer
failure
On Fri, Nov 28, 2014 at 4:36 PM, Dexuan Cui
<[email protected]>
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines · wrote:
Quoted 5 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Friday, November 28, 2014 14:47 PM
To: Dexuan Cui
Cc: [email protected];
[email protected];
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
driverdev-
[email protected]; [email protected];
[email protected]; KY
Quoted 2 lines
Quoted 2 lines
Srinivasan; [email protected]; Haiyang Zhang
Subject: Re: [PATCH v3] hv: hv_fcopy: drop the obsolete
message
Quoted 9 lines
Quoted 9 lines
on
Quoted 3 lines
Quoted 3 lines
transfer
failure
On Thu, Nov 27, 2014 at 9:09 PM, Dexuan Cui
<[email protected]>
Quoted 2 lines
Quoted 2 lines
Quoted 1 line · wrote:
In the case the user-space daemon crashes, hangs or is
killed, we
Quoted 1 line
Quoted 1 line
Quoted 1 line
need to down the semaphore, otherwise, after the daemon
starts
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
next
Quoted 11 lines
time, the obsolete data in fcopy_transaction.message or fcopy_transaction.fcopy_msg will be used immediately.
Cc Jason Wang <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
v2: I removed the "FCP" prefix as Greg asked.
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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.
Quoted 1 line
Quoted 1 line
Quoted 1 line
I removed the pr_debug (it isn't so meaningful)and
added a
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
comment instead.
 drivers/hv/hv_fcopy.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
Quoted 12 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct
Quoted 5 lines
drivers/hv/hv_fcopy.c +0 -0
index 23b2ce2..faa6ba6 100644
@@ -86,6 +86,18 @@ static void fcopy_work_func(struct
Quoted 7 lines
work_struct
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/hv/hv_fcopy.c +2 -0
 *dummy)
 	 * process the pending transaction.
 	 */
 	fcopy_respond_to_host(HV_E_FAIL);
+
+	/* In the case the user-space daemon crashes, hangs or 
is
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
killed, we
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct · line 92
Quoted 1 line
drivers/hv/hv_fcopy.c +1 -0
+	 * need to down the semaphore, otherwise, after the 
daemon
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
starts
drivers/hv/hv_fcopy.c
Quoted 2 lines
next
drivers/hv/hv_fcopy.c +1 -0
+	 * time, the obsolete data in fcopy_transaction.message 
or
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
drivers/hv/hv_fcopy.c
Quoted 3 lines
drivers/hv/hv_fcopy.c +3 -0
+	 * fcopy_transaction.fcopy_msg will be used immediately.
+	 *
+	 * NOTE: fcopy_read() happens to get the semaphore (very
rare)?
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
We're
drivers/hv/hv_fcopy.c +1 -0
+	 * still OK, because we've reported the failure to the 
host.
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
drivers/hv/hv_fcopy.c
Quoted 3 lines
drivers/hv/hv_fcopy.c +3 -0
+	 */
+	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
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
check
here?
Hi Jason,
My idea is: here we need down_trylock(), but in case we can't
get
Quoted 9 lines
Quoted 9 lines
the
Quoted 4 lines
semaphore, it's OK anyway:
Scenario 1):
1.1: when the daemon is blocked on the pread(), the daemon
receives
Quoted 3 lines
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
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
1.3.2: the driver down()-es the semaphore;
1.4: the daemon receives SIGCONT and it will be still blocked
on
Quoted 8 lines
Quoted 8 lines
the
Quoted 7 lines
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
Quoted 3 lines
Quoted 3 lines
1.3.2
Quoted 2 lines
and
do down() in fcopy_read(), it will receive the message but:
the
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
driver has
reported the failure to the host user and the driver's 1.3.2
can't
Quoted 11 lines
Quoted 9 lines
Quoted 6 lines
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()
Quoted 1 line
will try to cancel fcopy_work.
Yes.
Quoted 3 lines
Quoted 3 lines
Looks like in this case userspace() may wait in
down_interruptible()
Quoted 10 lines
Quoted 6 lines
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
Quoted 4 lines
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.
Quoted 27 lines
Quoted 27 lines
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?
Quoted 7 lines
Quoted 7 lines
drivers/hv/hv_fcopy.c static int fcopy_release(struct inode
Quoted 7 lines
drivers/hv/hv_fcopy.c +1 -0
+
 }
 
 static int fcopy_handle_handshake(u32 version)
@@ -351,6 +363,13 @@ static int fcopy_release(struct inode
*inode,
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
drivers/hv/hv_fcopy.c +5 -0
 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))
+			;
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
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
Quoted 1 line
Quoted 1 line
Quoted 1 line
work (as you suggested) and down() the semaphore, otherwise,
the
Quoted 9 lines
Quoted 9 lines
Quoted 1 line
obsolete message will be received by the next instance of the
daemon.
Yes
Quoted 5 lines
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
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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
Quoted 2 lines
Quoted 2 lines
we'll
Quoted 1 line
buffer an obsolete message in the driver, and the message
will be
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
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
Quoted 3 lines
Quoted 3 lines
to be received. Spinlock won't help.
But an idea is let the daemon the handle such cases. E.g make
sure
Quoted 2 lines
Quoted 2 lines
the
processing begins with START_COPY and end with
COMPLETE/CANCEL_COPY.
Quoted 2 lines
Quoted 2 lines
Drop all requests that does not start with START_COPY.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Thought?
Good idea.
I also think we should reinforce the concept of state machine in
the
Quoted 1 line
daemon code.
Yes, it needs.
I agree.
Obviously we can do something to make the daemon/driver work better in the corner cases.
Quoted 4 lines
Quoted 2 lines
The daemon/driver communication has so many corner cases...
Looks so, let's first address the issue mentioned in this patch.
OK.
Quoted 8 lines
I don't have any more comments other than changing
if(down_trylock(&fcopy_transaction.read_sema))
      ;
to
down_trylock(&fcopy_transaction.read_sema);
Hi Jason,
This is to address Vitaly's comment in the bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=1162100#c5
down_trylock(&fcopy_transaction.read_sema) will
"
produces the following compile warning:
drivers/hv/hv_fcopy.c: In function ‘fcopy_work_func’:
drivers/hv/hv_fcopy.c:95:2: warning: ignoring return value of ‘down_trylock’, declared with attribute warn_unused_result [-Wunused-result]
(void)down_trylock(&fcopy_transaction.read_sema);
"

Quoted 5 lines

Actually I personally don't care about the warning, because we only see it when we run some kind of code checker program. :-)
I can change my v3 to the "normal" style you prefer, if there is no strong objection from Vitaly?

Ah, I see the point. Then no objection for this patch.

Since Vitaly said he does not has objection.

Acked-by Jason Wang <[email protected]>

Dexuan Cui

author maintainer

2014-12-01 11:01 UTC #9 lore raw

Quoted 315 lines

-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, December 1, 2014 18:18 PM
To: Dexuan Cui
Cc: [email protected]; [email protected]; driverdev- [email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on transfer failure
Quoted 302 lines · Dexuan Cui
Quoted 259 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, December 1, 2014 16:23 PM
To: Dexuan Cui
Cc: [email protected]; [email protected];
driverdev-
[email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on
transfer
failure
Quoted 7 lines · Dexuan Cui
Quoted 7 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Friday, November 28, 2014 18:13 PM
To: Dexuan Cui
Cc: [email protected]; [email protected];
driverdev-
[email protected]; [email protected];
[email protected]; KY
Quoted 2 lines
Quoted 2 lines
Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message
on
Quoted 3 lines
Quoted 3 lines
transfer
failure
On Fri, Nov 28, 2014 at 4:36 PM, Dexuan Cui
<[email protected]>
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines · wrote:
Quoted 5 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Friday, November 28, 2014 14:47 PM
To: Dexuan Cui
Cc: [email protected];
[email protected];
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
driverdev-
[email protected]; [email protected];
[email protected]; KY
Quoted 2 lines
Quoted 2 lines
Srinivasan; [email protected]; Haiyang Zhang
Subject: Re: [PATCH v3] hv: hv_fcopy: drop the obsolete
message
Quoted 9 lines
Quoted 9 lines
on
Quoted 3 lines
Quoted 3 lines
transfer
failure
On Thu, Nov 27, 2014 at 9:09 PM, Dexuan Cui
<[email protected]>
Quoted 2 lines
Quoted 2 lines
Quoted 1 line · wrote:
In the case the user-space daemon crashes, hangs or is
killed, we
Quoted 1 line
Quoted 1 line
Quoted 1 line
need to down the semaphore, otherwise, after the daemon
starts
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
next
Quoted 11 lines
time, the obsolete data in fcopy_transaction.message or fcopy_transaction.fcopy_msg will be used immediately.
Cc Jason Wang <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
v2: I removed the "FCP" prefix as Greg asked.
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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.
Quoted 1 line
Quoted 1 line
Quoted 1 line
I removed the pr_debug (it isn't so meaningful)and
added a
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
comment instead.
 drivers/hv/hv_fcopy.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
Quoted 12 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct
Quoted 5 lines
drivers/hv/hv_fcopy.c +0 -0
index 23b2ce2..faa6ba6 100644
@@ -86,6 +86,18 @@ static void fcopy_work_func(struct
Quoted 7 lines
work_struct
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/hv/hv_fcopy.c +2 -0
 *dummy)
 	 * process the pending transaction.
 	 */
 	fcopy_respond_to_host(HV_E_FAIL);
+
+	/* In the case the user-space daemon crashes, hangs or
is
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
killed, we
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct · line 92
Quoted 1 line
drivers/hv/hv_fcopy.c +1 -0
+	 * need to down the semaphore, otherwise, after the
daemon
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
starts
drivers/hv/hv_fcopy.c
Quoted 2 lines
next
drivers/hv/hv_fcopy.c +1 -0
+	 * time, the obsolete data in fcopy_transaction.message
or
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
drivers/hv/hv_fcopy.c
Quoted 3 lines
drivers/hv/hv_fcopy.c +3 -0
+	 * fcopy_transaction.fcopy_msg will be used immediately.
+	 *
+	 * NOTE: fcopy_read() happens to get the semaphore (very
rare)?
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
We're
drivers/hv/hv_fcopy.c +1 -0
+	 * still OK, because we've reported the failure to the
host.
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
drivers/hv/hv_fcopy.c
Quoted 3 lines
drivers/hv/hv_fcopy.c +3 -0
+	 */
+	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
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
check
here?
Hi Jason,
My idea is: here we need down_trylock(), but in case we can't
get
Quoted 9 lines
Quoted 9 lines
the
Quoted 4 lines
semaphore, it's OK anyway:
Scenario 1):
1.1: when the daemon is blocked on the pread(), the daemon
receives
Quoted 3 lines
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
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
1.3.2: the driver down()-es the semaphore;
1.4: the daemon receives SIGCONT and it will be still blocked
on
Quoted 8 lines
Quoted 8 lines
the
Quoted 7 lines
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
Quoted 3 lines
Quoted 3 lines
1.3.2
Quoted 2 lines
and
do down() in fcopy_read(), it will receive the message but:
the
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
driver has
reported the failure to the host user and the driver's 1.3.2
can't
Quoted 11 lines
Quoted 9 lines
Quoted 6 lines
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()
Quoted 1 line
will try to cancel fcopy_work.
Yes.
Quoted 3 lines
Quoted 3 lines
Looks like in this case userspace() may wait in
down_interruptible()
Quoted 10 lines
Quoted 6 lines
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
Quoted 4 lines
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.
Quoted 27 lines
Quoted 27 lines
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?
Quoted 7 lines
Quoted 7 lines
drivers/hv/hv_fcopy.c static int fcopy_release(struct inode
Quoted 7 lines
drivers/hv/hv_fcopy.c +1 -0
+
 }
 
 static int fcopy_handle_handshake(u32 version)
@@ -351,6 +363,13 @@ static int fcopy_release(struct inode
*inode,
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
drivers/hv/hv_fcopy.c +5 -0
 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))
+			;
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
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
Quoted 1 line
Quoted 1 line
Quoted 1 line
work (as you suggested) and down() the semaphore, otherwise,
the
Quoted 9 lines
Quoted 9 lines
Quoted 1 line
obsolete message will be received by the next instance of the
daemon.
Yes
Quoted 5 lines
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
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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
Quoted 2 lines
Quoted 2 lines
we'll
Quoted 1 line
buffer an obsolete message in the driver, and the message
will be
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
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
Quoted 3 lines
Quoted 3 lines
to be received. Spinlock won't help.
But an idea is let the daemon the handle such cases. E.g make
sure
Quoted 2 lines
Quoted 2 lines
the
processing begins with START_COPY and end with
COMPLETE/CANCEL_COPY.
Quoted 2 lines
Quoted 2 lines
Drop all requests that does not start with START_COPY.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Thought?
Good idea.
I also think we should reinforce the concept of state machine in
the
Quoted 1 line
daemon code.
Yes, it needs.
I agree.
Obviously we can do something to make the daemon/driver work better in the corner cases.
Quoted 4 lines
Quoted 2 lines
The daemon/driver communication has so many corner cases...
Looks so, let's first address the issue mentioned in this patch.
OK.
Quoted 8 lines
I don't have any more comments other than changing
if(down_trylock(&fcopy_transaction.read_sema))
      ;
to
down_trylock(&fcopy_transaction.read_sema);
Hi Jason,
This is to address Vitaly's comment in the bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=1162100#c5
down_trylock(&fcopy_transaction.read_sema) will
"
produces the following compile warning:
drivers/hv/hv_fcopy.c: In function ‘fcopy_work_func’:
drivers/hv/hv_fcopy.c:95:2: warning: ignoring return value of ‘down_trylock’, declared with attribute warn_unused_result [-Wunused-result]
(void)down_trylock(&fcopy_transaction.read_sema);
"
Actually I personally don't care about the warning, because we only see it when we run some kind of code checker program. :-)

Quoted 8 lines

Quoted 2 lines
I can change my v3 to the "normal" style you prefer, if there is no strong objection from Vitaly?
Ah, I see the point. Then no objection for this patch.
Since Vitaly said he does not has objection.
Acked-by Jason Wang <[email protected]>

I realized non-trivial effort might be needed to better handle corner cases. This is only the first step.

E.g., I found a new corner case:
after we unload the hv_utils/fcopy driver, the fcopy daemon can exit and fcopy_release() will be invoked -- at this time fcopy_release() has become obsolete and invalid, and as a result, a kernel panic can happen: I did see the panic once.

-- Dexuan ��{.n�+��+%��ݶ��w��{.n�+��{��G��{ay�ʇڙ�,j��f��h��z_�(�階�ݢj"��m��G��?��&��~��iO��z��v�^�m��?�I�

KY Srinivasan

2014-12-01 15:55 UTC #10 lore raw

Quoted 296 lines

-----Original Message-----
From: Dexuan Cui
Sent: Monday, December 1, 2014 3:01 AM
To: Jason Wang
Cc: [email protected]; [email protected]; driverdev- [email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on transfer failure
Quoted 13 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, December 1, 2014 18:18 PM
To: Dexuan Cui
Cc: [email protected]; [email protected]; driverdev- [email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on transfer failure
On Mon, Dec 1, 2014 at 5:47 PM, Dexuan Cui <[email protected]>
Quoted 58 lines · wrote:
Quoted 58 lines
Quoted 58 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, December 1, 2014 16:23 PM
To: Dexuan Cui
Cc: [email protected]; [email protected];
driverdev-
[email protected]; [email protected]; [email protected];
KY Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on
transfer failure On Fri, Nov 28, 2014 at 7:54 PM, Dexuan Cui <[email protected]>
Quoted 2 lines · wrote:
Quoted 2 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]] >> Sent:
Friday, November 28, 2014 18:13 PM >> To: Dexuan Cui >> Cc:
[email protected]; [email protected]; >> driverdev- >> [email protected]; [email protected]; [email protected]; KY >> Srinivasan; [email protected]; Haiyang Zhang >> Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on >> transfer >> failure >> On Fri, Nov 28, 2014 at
Quoted 1 line · Dexuan Cui
Quoted 1 line
Quoted 1 line
Quoted 1 line
-----Original Message----- >> >> From: Jason Wang
[mailto:[email protected]] >> >> Sent: Friday, November 28, 2014 14:47 PM >> >> To: Dexuan Cui >> >> Cc:
[email protected]; [email protected]; >> >> driverdev- >> >> [email protected]; [email protected];
Quoted 1 line
Quoted 1 line
[email protected]; KY >> >> Srinivasan; [email protected];
Haiyang Zhang >> >> Subject: Re: [PATCH v3] hv: hv_fcopy: drop the obsolete message >> on >> >> transfer >> >> failure >>
Quoted 1 line
Quoted 1 line
On Thu, Nov 27, 2014 at 9:09 PM, Dexuan Cui >>
Quoted 2 lines · <[email protected]> >> >> wrote:
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
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.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Cc Jason Wang <[email protected]> >> >> > Cc:
Vitaly Kuznetsov <[email protected]> >> >> > Cc: K. Y. Srinivasan <[email protected]> >> >> > Signed-off-by: Dexuan Cui <[email protected]> >> >> > --- >> >> > >> >> > v2: I removed the "FCP" prefix as Greg asked.
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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.
Quoted 1 line
Quoted 1 line
Quoted 1 line
I removed the pr_debug (it isn't so meaningful)and
added a
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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 >>
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 1 line
Quoted 1 line
Review static void fcopy_work_func(struct >>
Quoted 1 line
(quoted diff) +0 -0
@@ -86,6 +86,18 @@ static void fcopy_work_func(struct  >>
work_struct >> >> > *dummy)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
(quoted diff) +2 -0
 	 * process the pending transaction.
 	 */
 	fcopy_respond_to_host(HV_E_FAIL);
+
+	/* In the case the user-space daemon crashes, hangs
or
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
is
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
killed, we
Quoted 1 line
(quoted diff) +1 -0
+	 * need to down the semaphore, otherwise, after
the
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
daemon
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
starts
Quoted 2 lines
next
(quoted diff) +1 -0
+	 * time, the obsolete data in
fcopy_transaction.message
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
or
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +1 -0
+	 * fcopy_transaction.fcopy_msg will be used
immediately.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+	 *
+	 * NOTE: fcopy_read() happens to get the
semaphore (very
Quoted 88 lines
Quoted 88 lines
Quoted 88 lines
Quoted 3 lines
Quoted 3 lines
rare)?
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
We're
(quoted diff) +1 -0
+	 * still OK, because we've reported the failure to the
host.
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Review
Quoted 3 lines
(quoted diff) +3 -0
+	 */
+	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?
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Hi Jason,
My idea is: here we need down_trylock(), but in case we
can't get >> the >> > semaphore, it's OK anyway:
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
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().
Quoted 1 line
Quoted 1 line
Quoted 1 line
Without the down_trylock(), in 1.4, the daemon can receive
an >> > obsolete message.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
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
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
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.
Quoted 1 line
Quoted 1 line
Quoted 1 line
BTW, I think in the daemon's hv_start_fcopy() we should add
a >> > close(target_fd) before open()-ing a new one.
Quoted 2 lines
Quoted 2 lines
Right, but how about the case when resuming from SIGSTOP but
no >> timeout?
Quoted 2 lines
Sorry, I don't understand this:
if no timeout, fcopy_read() will get the semaphore and
fcopy_write()
Quoted 1 line
will try to cancel fcopy_work.
Yes.
Quoted 3 lines
Quoted 3 lines
Looks like in this case userspace() may wait in
down_interruptible()
Quoted 9 lines
Quoted 6 lines
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).
Quoted 1 line
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.
Quoted 6 lines
Quoted 6 lines
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?
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
+
}
static int fcopy_handle_handshake(u32 version) >> >>
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Review static int fcopy_release(struct inode >>
Quoted 1 line
(quoted diff) +0 -0
@@ -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)? */
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +1 -0
+		if
(down_trylock(&fcopy_transaction.read_sema))
Quoted 81 lines
Quoted 81 lines
Quoted 37 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Review
Quoted 1 line
(quoted diff) +1 -0
+			;
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.
Quoted 7 lines
Quoted 7 lines
Yes
Quoted 5 lines
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
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
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.
Quoted 1 line
Quoted 1 line
Hmm, in this case, fcopy_send_data() will do up() later,
and
Quoted 2 lines
Quoted 2 lines
we'll
Quoted 1 line
buffer an obsolete message in the driver, and the message
will be >> > fetched by the next instance of the daemon...
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Looks we need a spinlock here?
Unless fcopy_release() can wait for all data for current
transation >> to be received. Spinlock won't help.
Quoted 2 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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.
I agree.
Obviously we can do something to make the daemon/driver work better in the corner cases.
Quoted 4 lines
Quoted 2 lines
The daemon/driver communication has so many corner cases...
Looks so, let's first address the issue mentioned in this patch.
OK.
Quoted 8 lines
I don't have any more comments other than changing
if(down_trylock(&fcopy_transaction.read_sema))
      ;
to
down_trylock(&fcopy_transaction.read_sema);
Hi Jason,
This is to address Vitaly's comment in the bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=1162100#c5
down_trylock(&fcopy_transaction.read_sema) will
"
produces the following compile warning:
drivers/hv/hv_fcopy.c: In function ‘fcopy_work_func’:
drivers/hv/hv_fcopy.c:95:2: warning: ignoring return value of ‘down_trylock’, declared with attribute warn_unused_result [-Wunused-result]
(void)down_trylock(&fcopy_transaction.read_sema);
"
Actually I personally don't care about the warning, because we only see it when we run some kind of code checker program. :-)
Quoted 8 lines
Quoted 2 lines
I can change my v3 to the "normal" style you prefer, if there is no strong objection from Vitaly?
Ah, I see the point. Then no objection for this patch.
Since Vitaly said he does not has objection.
Acked-by Jason Wang <[email protected]>

Quoted 8 lines

I realized non-trivial effort might be needed to better handle corner cases. This is only the first step.
E.g., I found a new corner case:
after we unload the hv_utils/fcopy driver, the fcopy daemon can exit and fcopy_release() will be invoked -- at this time fcopy_release() has become obsolete and invalid, and as a result, a kernel panic can happen: I did see the panic once.

You should not be able to unload the driver when there is still an open reference from the Daemon.

K. Y

Quoted 2 lines

-- Dexuan

Dexuan Cui

author maintainer

2014-12-02 05:34 UTC #11 lore raw

Quoted 315 lines

-----Original Message-----
From: KY Srinivasan
Sent: Monday, December 1, 2014 23:55 PM
To: Dexuan Cui; Jason Wang
Cc: [email protected]; [email protected]; driverdev- [email protected]; [email protected]; [email protected]; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on transfer failure
Quoted 17 lines
-----Original Message-----
From: Dexuan Cui
Sent: Monday, December 1, 2014 3:01 AM
To: Jason Wang
Cc: [email protected]; [email protected]; driverdev- [email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on transfer failure
Quoted 7 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, December 1, 2014 18:18 PM
To: Dexuan Cui
Cc: [email protected]; [email protected]; driverdev- [email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; Haiyang
Zhang
Quoted 17 lines
Quoted 6 lines
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on transfer failure
On Mon, Dec 1, 2014 at 5:47 PM, Dexuan Cui <[email protected]>
Quoted 10 lines · wrote:
Quoted 10 lines
Quoted 10 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, December 1, 2014 16:23 PM
To: Dexuan Cui
Cc: [email protected]; [email protected];
driverdev-
[email protected]; [email protected]; [email protected];
KY Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on
transfer failure On Fri, Nov 28, 2014 at 7:54 PM, Dexuan Cui
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
<[email protected]>
Quoted 2 lines · wrote:
Quoted 2 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]] >> Sent:
Friday, November 28, 2014 18:13 PM >> To: Dexuan Cui >> Cc:
[email protected]; [email protected]; >> driverdev- >> [email protected]; [email protected]; [email protected]; KY >> Srinivasan; [email protected];
Haiyang
Quoted 252 lines
Quoted 40 lines
Quoted 40 lines
Quoted 40 lines
Zhang >> Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on >> transfer >> failure >> On Fri, Nov 28, 2014 at
Quoted 1 line · Dexuan Cui
Quoted 1 line
Quoted 1 line
Quoted 1 line
-----Original Message----- >> >> From: Jason Wang
[mailto:[email protected]] >> >> Sent: Friday, November 28, 2014 14:47 PM >> >> To: Dexuan Cui >> >> Cc:
[email protected]; [email protected]; >> >> driverdev- >> >> [email protected]; [email protected];
Quoted 1 line
Quoted 1 line
[email protected]; KY >> >> Srinivasan; [email protected];
Haiyang Zhang >> >> Subject: Re: [PATCH v3] hv: hv_fcopy: drop the obsolete message >> on >> >> transfer >> >> failure >>
Quoted 1 line
Quoted 1 line
On Thu, Nov 27, 2014 at 9:09 PM, Dexuan Cui >>
Quoted 2 lines · <[email protected]> >> >> wrote:
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
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.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Cc Jason Wang <[email protected]> >> >> > Cc:
Vitaly Kuznetsov <[email protected]> >> >> > Cc: K. Y. Srinivasan <[email protected]> >> >> > Signed-off-by: Dexuan Cui <[email protected]> >> >> > --- >> >> > >> >> > v2: I removed the "FCP" prefix as Greg asked.
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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.
Quoted 1 line
Quoted 1 line
Quoted 1 line
I removed the pr_debug (it isn't so meaningful)and
added a
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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 >>
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 1 line
Quoted 1 line
Review static void fcopy_work_func(struct >>
Quoted 1 line
(quoted diff) +0 -0
@@ -86,6 +86,18 @@ static void fcopy_work_func(struct  >>
work_struct  >>  >>  > *dummy)
 >>  >>  >   * process the pending transaction.
 >>  >>  >   */
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
(quoted diff) +0 -0
 >  >  fcopy_respond_to_host(HV_E_FAIL);
Quoted 2 lines
Quoted 2 lines
(quoted diff) +2 -0
+
+/* In the case the user-space daemon crashes, hangs
or
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
is
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
killed, we
Quoted 1 line
+ * need to down the semaphore, otherwise, after
the
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
daemon
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
starts
Quoted 2 lines
next
+ * time, the obsolete data in
fcopy_transaction.message
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
or
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
+ * fcopy_transaction.fcopy_msg will be used
immediately.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
+ *
+ * NOTE: fcopy_read() happens to get the
semaphore (very
Quoted 88 lines
Quoted 88 lines
Quoted 88 lines
Quoted 3 lines
Quoted 3 lines
rare)?
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
We're
+ * still OK, because we've reported the failure to the
host.
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Review
Quoted 3 lines
+ */
(quoted diff) +2 -0
+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?
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Hi Jason,
My idea is: here we need down_trylock(), but in case we
can't get >> the >> > semaphore, it's OK anyway:
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
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().
Quoted 1 line
Quoted 1 line
Quoted 1 line
Without the down_trylock(), in 1.4, the daemon can receive
an >> > obsolete message.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
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
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
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.
Quoted 1 line
Quoted 1 line
Quoted 1 line
BTW, I think in the daemon's hv_start_fcopy() we should add
a >> > close(target_fd) before open()-ing a new one.
Quoted 2 lines
Quoted 2 lines
Right, but how about the case when resuming from SIGSTOP but
no >> timeout?
Quoted 2 lines
Sorry, I don't understand this:
if no timeout, fcopy_read() will get the semaphore and
fcopy_write()
Quoted 1 line
will try to cancel fcopy_work.
Yes.
Quoted 3 lines
Quoted 3 lines
Looks like in this case userspace() may wait in
down_interruptible()
Quoted 9 lines
Quoted 6 lines
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).
Quoted 1 line
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.
Quoted 6 lines
Quoted 6 lines
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?
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
+
}
static int fcopy_handle_handshake(u32 version) >> >>
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Review static int fcopy_release(struct inode >>
Quoted 1 line
(quoted diff) +0 -0
@@ -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)? */
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
+if
(down_trylock(&fcopy_transaction.read_sema))
Quoted 81 lines
Quoted 81 lines
Quoted 37 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
+;
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.
Quoted 7 lines
Quoted 7 lines
Yes
Quoted 5 lines
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
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
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.
Quoted 1 line
Quoted 1 line
Hmm, in this case, fcopy_send_data() will do up() later,
and
Quoted 2 lines
Quoted 2 lines
we'll
Quoted 1 line
buffer an obsolete message in the driver, and the message
will be >> > fetched by the next instance of the daemon...
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Looks we need a spinlock here?
Unless fcopy_release() can wait for all data for current
transation >> to be received. Spinlock won't help.
Quoted 2 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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.
I agree.
Obviously we can do something to make the daemon/driver work better in the corner cases.
Quoted 4 lines
Quoted 2 lines
The daemon/driver communication has so many corner cases...
Looks so, let's first address the issue mentioned in this patch.
OK.
Quoted 8 lines
I don't have any more comments other than changing
if(down_trylock(&fcopy_transaction.read_sema))
      ;
to
down_trylock(&fcopy_transaction.read_sema);
Hi Jason,
This is to address Vitaly's comment in the bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=1162100#c5
down_trylock(&fcopy_transaction.read_sema) will
"
produces the following compile warning:
drivers/hv/hv_fcopy.c: In function ‘fcopy_work_func’:
drivers/hv/hv_fcopy.c:95:2: warning: ignoring return value of ‘down_trylock’, declared with attribute warn_unused_result [-Wunused-result]
(void)down_trylock(&fcopy_transaction.read_sema);
"
Actually I personally don't care about the warning, because we only see it when we run some kind of code checker program. :-)
Quoted 8 lines
Quoted 2 lines
I can change my v3 to the "normal" style you prefer, if there is no strong objection from Vitaly?
Ah, I see the point. Then no objection for this patch.
Since Vitaly said he does not has objection.
Acked-by Jason Wang <[email protected]>
Quoted 7 lines
I realized non-trivial effort might be needed to better handle corner cases. This is only the first step.
E.g., I found a new corner case:
after we unload the hv_utils/fcopy driver, the fcopy daemon can exit and fcopy_release() will be invoked -- at this time fcopy_release() has become obsolete and invalid, and as a result, a kernel panic can happen: I did see

Quoted 8 lines

the
Quoted 1 line
panic once.
You should not be able to unload the driver when there is still an open reference from the
Daemon.
K. Y

Hi KY, I'm able to reproduce the below issue every time:
(I ran the below in Ubuntu 14.10 but I think we're able to repro it in any distro):

1) start hv_fcopy_daemon and use ps to make sure it's running in the background;
2) kill -STOP pidof hv_fcopy_daemon, and use ps to make sure the daemon is in the state of "Ts";
3) rmmod hv_utils # dexuan: this can runs fine.
4) kill -CONT pidof hv_fcopy_daemon and we see an oops at once:

[ 9699.421564] BUG: unable to handle kernel paging request at ffffffffc019c230
[ 9699.422701] IP: [<ffffffff811de294>] filp_close+0x24/0x70
[ 9699.423154] PGD 1c16067 PUD 1c18067 PMD 366d8067 PTE 0
[ 9699.423154] Oops: 0000 [#1] SMP
[ 9699.423154] Modules linked in: hv_utils rfcomm bnep bluetooth 6lowpan_iphc joydev hid_generic crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel aes_x86_64 lrw gf128mul glue_helper ablk_helper cryptd hid_hyperv serio_raw hid hyperv_keyboard hv_balloon mac_hid hyperv_fb i2c_piix4 parport_pc ppdev lp parport hv_storvsc psmouse floppy hv_vmbus tulip pata_acpi [last unloaded: hv_utils]
[ 9699.423154] CPU: 0 PID: 4659 Comm: hv_fcopy_daemon Not tainted 3.16.0-23-generic #31-Ubuntu
[ 9699.423154] Hardware name: Microsoft Corporation Virtual Machine/Virtual Machine, BIOS 090006  05/23/2012
[ 9699.423154] task: ffff880019ce5bb0 ti: ffff8800c7be0000 task.ti: ffff8800c7be0000
[ 9699.423154] RIP: 0010:[<ffffffff811de294>]  [<ffffffff811de294>] filp_close+0x24/0x70
[ 9699.423154] RSP: 0018:ffff8800c7be3e48  EFLAGS: 00010246
[ 9699.423154] RAX: ffffffffc019c1c0 RBX: ffff8800c7b9c200 RCX: ffff8800396548d8
[ 9699.423154] RDX: 0000000080000000 RSI: ffff880039654880 RDI: ffff8800c7b9c200
[ 9699.423154] RBP: ffff8800c7be3e60 R08: 0000000000000001 R09: 00000001802a0017
[ 9699.423154] R10: ffff88003de17640 R11: ffff880087963140 R12: 0000000000000000
[ 9699.423154] R13: ffff880039654880 R14: ffff880039654890 R15: 0000000000000004
[ 9699.423154] FS:  0000000000000000(0000) GS:ffff88003de00000(0000) knlGS:0000000000000000
[ 9699.423154] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 9699.423154] CR2: ffffffffc019c230 CR3: 0000000001c13000 CR4: 00000000001406f0
[ 9699.423154] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 9699.423154] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 9699.423154] Stack:
[ 9699.423154]  0000000000000001 0000000000000000 ffff880039654880 ffff8800c7be3ea0
[ 9699.423154]  ffffffff811ff208 00000001034a4c80 ffff880019ce5bb0 ffff880039654880
[ 9699.423154]  ffff880019ce6308 ffff8800126b8e00 ffff8800126b8e60 ffff8800c7be3ec8
[ 9699.423154] Call Trace:
[ 9699.423154]  [<ffffffff811ff208>] put_files_struct+0x78/0xd0
[ 9699.423154]  [<ffffffff811ff307>] exit_files+0x47/0x50
[ 9699.423154]  [<ffffffff810725a7>] do_exit+0x357/0xab0
[ 9699.423154]  [<ffffffff81072d83>] do_group_exit+0x43/0xc0
[ 9699.423154]  [<ffffffff81072e14>] SyS_exit_group+0x14/0x20
[ 9699.423154]  [<ffffffff81787ced>] system_call_fastpath+0x1a/0x1f
[ 9699.423154] Code: ff ff 0f 1f 44 00 00 0f 1f 44 00 00 55 48 89 e5 41 55 41 54 53 48 8b 47 38 48 89 fb 48 85 c0 74 46 48 8b 47 28 45 31 e4 49 89 f5 <48> 8b 40 70 48 85 c0 74 05 ff d0 41 89 c4 f6 43 45 40 75 16 4c
[ 9699.423154] RIP  [<ffffffff811de294>] filp_close+0x24/0x70
[ 9699.423154]  RSP <ffff8800c7be3e48>
[ 9699.423154] CR2: ffffffffc019c230
[ 9699.423154] ---[ end trace 0278723740833669 ]---
[ 9699.423154] Fixing recursive fault but reboot is needed!

I understand an administrator should not unload the driver before stopping the daemon, but this could happen by accident.

-- Dexuan

Jason Wang

2014-12-02 06:58 UTC #12 lore raw

Quoted 403 lines · Dexuan Cui

Quoted 394 lines
-----Original Message-----
From: KY Srinivasan
Sent: Monday, December 1, 2014 23:55 PM
To: Dexuan Cui; Jason Wang
Cc: [email protected]; [email protected];
driverdev-
[email protected]; [email protected]; [email protected]; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on
transfer
failure
Quoted 5 lines
-----Original Message-----
From: Dexuan Cui
Sent: Monday, December 1, 2014 3:01 AM
To: Jason Wang
Cc: [email protected]; [email protected];
driverdev-
Quoted 1 line
[email protected]; [email protected]; [email protected];
KY
Quoted 2 lines
Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message
on transfer
Quoted 9 lines
failure
Quoted 7 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, December 1, 2014 18:18 PM
To: Dexuan Cui
Cc: [email protected]; [email protected]; driverdev- [email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; Haiyang
Zhang
Quoted 1 line
Quoted 1 line
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message
on
Quoted 11 lines
Quoted 5 lines
transfer failure
On Mon, Dec 1, 2014 at 5:47 PM, Dexuan Cui <[email protected]>
Quoted 5 lines · wrote:
Quoted 5 lines
Quoted 5 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, December 1, 2014 16:23 PM
To: Dexuan Cui
Cc: [email protected];
[email protected];
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
driverdev-
[email protected]; [email protected];
[email protected];
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
KY Srinivasan; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete
message on
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
transfer failure On Fri, Nov 28, 2014 at 7:54 PM, Dexuan
Cui
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
<[email protected]>
Quoted 2 lines · wrote:
Quoted 2 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]] >> Sent:
Friday, November 28, 2014 18:13 PM >> To: Dexuan Cui >>
Cc:
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
[email protected]; [email protected]; >> driverdev- >> [email protected]; [email protected]; [email protected]; KY >> Srinivasan; [email protected];
Haiyang
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Zhang >> Subject: RE: [PATCH v3] hv: hv_fcopy: drop the
obsolete
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
message on >> transfer >> failure >> On Fri, Nov 28,
2014 at
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line · Dexuan Cui
Quoted 1 line
Quoted 1 line
Quoted 1 line
-----Original Message----- >> >> From: Jason Wang
[mailto:[email protected]] >> >> Sent: Friday, November
28,
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
2014 14:47 PM >> >> To: Dexuan Cui >> >> Cc:
[email protected]; [email protected];
driverdev- >> >> [email protected];
[email protected];
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
[email protected]; KY >> >> Srinivasan;
[email protected];
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Haiyang Zhang >> >> Subject: Re: [PATCH v3] hv: hv_fcopy:
drop
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
the obsolete message >> on >> >> transfer >> >>
failure >>
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
On Thu, Nov 27, 2014 at 9:09 PM, Dexuan Cui >>
Quoted 1 line · <[email protected]> >> >> wrote:
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
In the case the user-space daemon crashes, hangs
or is
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
killed, we >> >> > need to down the semaphore,
otherwise,
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
after the daemon starts >> >> next >> >> > time, the
obsolete
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
data in fcopy_transaction.message or >> >> > fcopy_transaction.fcopy_msg will be used immediately.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Cc Jason Wang <[email protected]> >> >> >
Cc:
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Vitaly Kuznetsov <[email protected]> >> >> > Cc: K. Y. Srinivasan <[email protected]> >> >> > Signed-off-by:
Dexuan Cui
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
<[email protected]> >> >> > --- >> >> > >> >> >
v2: I
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
removed the "FCP" prefix as Greg asked.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
suggested.
Quoted 1 line
Quoted 1 line
Quoted 1 line
I removed the pr_debug (it isn't so
meaningful)and
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
added a
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
comment instead.
 drivers/hv/hv_fcopy.c | 19 +++++++++++++++++++  
 1 file changed, 19 insertions(+)  >>  >>  >  >>  >>  > 
diff
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
--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 >>
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Quoted 1 line
Review static void fcopy_work_func(struct
Quoted 1 line
(quoted diff) +0 -0
@@ -86,6 +86,18 @@ static void fcopy_work_func(struct  
work_struct  >>  >>  > *dummy)
 >>  >>  >   * process the pending transaction.
 >>  >>  >   */
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
(quoted diff) +0 -0
 >  >  fcopy_respond_to_host(HV_E_FAIL);
Quoted 2 lines
Quoted 2 lines
(quoted diff) +2 -0
+
+/* In the case the user-space daemon crashes, 
hangs
Quoted 9 lines
or
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
is
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
killed, we
Quoted 1 line
+ * need to down the semaphore, otherwise, after
the
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
daemon
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
starts
Quoted 2 lines
next
+ * time, the obsolete data in
Quoted 10 lines
fcopy_transaction.message
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
or
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
+ * fcopy_transaction.fcopy_msg will be used
immediately.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
+ *
+ * NOTE: fcopy_read() happens to get the
semaphore (very
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
rare)?
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
We're
+ * still OK, because we've reported the failure
to the
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
host.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Review
Quoted 3 lines
+ */
(quoted diff) +2 -0
+if (down_trylock(&fcopy_transaction.read_sema))
+;
Sorry, I'm not quite understand how if () ; can
help here.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Btw, a question not relate to this patch.
What happens if a daemon is resume from SIGSTOP and
expires the >> >> check >> >> here?
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Hi Jason,
My idea is: here we need down_trylock(), but in case
we
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
can't get >> the >> > semaphore, it's OK anyway:
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Scenario 1):
1.1: when the daemon is blocked on the pread(), the
daemon
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
receives >> > SIGSTOP; >> > 1.2: the host user runs
the
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
PowerShell Copy-VMFile command; >> > 1.3.1: the driver
reports
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
the failure to the host user in 5s and >> > 1.3.2: the
driver
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
down()-es the semaphore; >> > 1.4: the daemon receives
SIGCONT
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
and it will be still blocked on >> the >> > pread().
Quoted 1 line
Quoted 1 line
Quoted 1 line
Without the down_trylock(), in 1.4, the daemon can
receive
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
an >> > obsolete message.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
NOTE: in this scenario, the daemon is not killed.
Scenario 2):
In senario 1), if the daemon receives SIGCONT between
1.3.1
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
and >> 1.3.2 >> > and >> > do down() in fcopy_read(),
it will
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
receive the message but:
the
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
driver has
reported the failure to the host user and the
driver's 1.3.2
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
can't >> > get the >> > semaphore -- IMO this is
acceptably OK,
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
though in the VM, an >> > incomplete >> > file will be
left
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
there.
Quoted 1 line
Quoted 1 line
Quoted 1 line
BTW, I think in the daemon's hv_start_fcopy() we
should add
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
a >> > close(target_fd) before open()-ing a new one.
Quoted 2 lines
Quoted 2 lines
Right, but how about the case when resuming from
SIGSTOP but
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
no >> timeout?
Quoted 2 lines
Sorry, I don't understand this:
if no timeout, fcopy_read() will get the semaphore and
fcopy_write()
Quoted 1 line
will try to cancel fcopy_work.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Yes.
Quoted 3 lines
Quoted 3 lines
Looks like in this case userspace() may wait in
down_interruptible()
Quoted 3 lines
Quoted 3 lines
until timeout. We probably need something like this:
if
(down_interruptible(&fcopy_transaction.read_sema)) {
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
                 up(&fcopy_transaction.read_sema);
                 return -EINTR;
         }
until "timeout"?
if the daemon can't get the semaphore, it can only be
wake by a
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
signal(the > daemon doesn't install handler, so by
default most
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
signals will kill > the daemon).
Quoted 1 line
In case a signal waking up the daemon doesn't kill the
daemon,
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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,
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
may add such one which should be useful for something like debugging.
Quoted 6 lines
Quoted 6 lines
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 >>
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
since the file may not even opened and it may take time >>
to
Quoted 18 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
handle signals?
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
+
}
static int fcopy_handle_handshake(u32 version)
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Review static int fcopy_release(struct inode
Quoted 1 line
(quoted diff) +0 -0
@@ -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
Quoted 9 lines
rare)? */
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
+if
(down_trylock(&fcopy_transaction.read_sema))
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
+;
And this.
Scenario 3):
When the daemon exits(e.g., SIGKILL received), if
there is a
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
fcopy_work >> > pending (scheduled but not start to
run
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
yet), we should cancel the >> > work (as you suggested) and down() the semaphore, otherwise, the >> > obsolete message
will
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
be received by the next instance of the >> daemon.
Quoted 5 lines
Quoted 5 lines
Yes
Quoted 3 lines
Scenario 4): in the driver's
hv_fcopy_onchannelcallback():
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
        schedule_delayed_work(&fcopy_work, 5*HZ);
        ----> if fcopy_release() is running on 
another vcpu,
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
just
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
before the next line?
        fcopy_send_data();
In this case, fcopy_release() can cancel fcopy_work,
but >>
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
can't get the semaphore since it hasn't been up()-ed.
Quoted 1 line
Quoted 1 line
Hmm, in this case, fcopy_send_data() will do up()
later,
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
and
Quoted 2 lines
Quoted 2 lines
we'll
Quoted 1 line
buffer an obsolete message in the driver, and the
message
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
will be >> > fetched by the next instance of the daemon...
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Looks we need a spinlock here?
Unless fcopy_release() can wait for all data for current
transation >> to be received. Spinlock won't help.
Quoted 2 lines
Quoted 2 lines
But an idea is let the daemon the handle such cases.
E.g make
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
sure >> the >> processing begins with START_COPY and end
with
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
COMPLETE/CANCEL_COPY.
Quoted 5 lines
Quoted 3 lines
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
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
the > daemon code.
Yes, it needs.
I agree.
Obviously we can do something to make the daemon/driver work
better
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
in the corner cases.
Quoted 2 lines
Quoted 2 lines
The daemon/driver communication has so many corner
cases...
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Looks so, let's first address the issue mentioned in this
patch.
Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
OK.
Quoted 8 lines
I don't have any more comments other than changing
if(down_trylock(&fcopy_transaction.read_sema))
      ;
to
down_trylock(&fcopy_transaction.read_sema);
Hi Jason,
This is to address Vitaly's comment in the bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=1162100#c5
down_trylock(&fcopy_transaction.read_sema) will
"
produces the following compile warning:
drivers/hv/hv_fcopy.c: In function ‘fcopy_work_func’:
drivers/hv/hv_fcopy.c:95:2: warning: ignoring return value of ‘down_trylock’, declared with attribute warn_unused_result [-Wunused-result]
(void)down_trylock(&fcopy_transaction.read_sema);
"
Quoted 1 line
Quoted 1 line
Quoted 1 line
Actually I personally don't care about the warning, because
we only
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
see it when we run some kind of code checker program. :-)
I can change my v3 to the "normal" style you prefer, if
there is no
Quoted 9 lines
Quoted 7 lines
Quoted 1 line
strong objection from Vitaly?
Ah, I see the point. Then no objection for this patch.
Since Vitaly said he does not has objection.
Acked-by Jason Wang <[email protected]>
I realized non-trivial effort might be needed to better handle
corner cases.
Quoted 4 lines
This is only the first step.
E.g., I found a new corner case:
after we unload the hv_utils/fcopy driver, the fcopy daemon can
exit and
Quoted 1 line
fcopy_release() will be invoked -- at this time fcopy_release()
has become
Quoted 1 line
obsolete and invalid, and as a result, a kernel panic can happen:
I did see
the
Quoted 8 lines
Quoted 1 line
panic once.
You should not be able to unload the driver when there is still an
open
reference from the
Daemon.
K. Y

Quoted 7 lines

Hi KY, I'm able to reproduce the below issue every time:
(I ran the below in Ubuntu 14.10 but I think we're able to repro it in any distro):
1) start hv_fcopy_daemon and use ps to make sure it's running in the background;
2) kill -STOP pidof hv_fcopy_daemon, and use ps to make sure the daemon is in the state of "Ts";
3) rmmod hv_utils # dexuan: this can runs fine.

Probably the opened file should hold a refcnt of the module.

Quoted 47 lines

4) kill -CONT pidof hv_fcopy_daemon and we see an oops at once:

[ 9699.421564] BUG: unable to handle kernel paging request at ffffffffc019c230
[ 9699.422701] IP: [<ffffffff811de294>] filp_close+0x24/0x70
[ 9699.423154] PGD 1c16067 PUD 1c18067 PMD 366d8067 PTE 0
[ 9699.423154] Oops: 0000 [#1] SMP
[ 9699.423154] Modules linked in: hv_utils rfcomm bnep bluetooth 6lowpan_iphc joydev hid_generic crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel aes_x86_64 lrw gf128mul glue_helper ablk_helper cryptd hid_hyperv serio_raw hid hyperv_keyboard hv_balloon mac_hid hyperv_fb i2c_piix4 parport_pc ppdev lp parport hv_storvsc psmouse floppy hv_vmbus tulip pata_acpi [last unloaded: hv_utils]
[ 9699.423154] CPU: 0 PID: 4659 Comm: hv_fcopy_daemon Not tainted 3.16.0-23-generic #31-Ubuntu
[ 9699.423154] Hardware name: Microsoft Corporation Virtual Machine/Virtual Machine, BIOS 090006  05/23/2012
[ 9699.423154] task: ffff880019ce5bb0 ti: ffff8800c7be0000 task.ti: ffff8800c7be0000
[ 9699.423154] RIP: 0010:[<ffffffff811de294>]  [<ffffffff811de294>] filp_close+0x24/0x70
[ 9699.423154] RSP: 0018:ffff8800c7be3e48  EFLAGS: 00010246
[ 9699.423154] RAX: ffffffffc019c1c0 RBX: ffff8800c7b9c200 RCX: ffff8800396548d8
[ 9699.423154] RDX: 0000000080000000 RSI: ffff880039654880 RDI: ffff8800c7b9c200
[ 9699.423154] RBP: ffff8800c7be3e60 R08: 0000000000000001 R09: 00000001802a0017
[ 9699.423154] R10: ffff88003de17640 R11: ffff880087963140 R12: 0000000000000000
[ 9699.423154] R13: ffff880039654880 R14: ffff880039654890 R15: 0000000000000004
[ 9699.423154] FS:  0000000000000000(0000) GS:ffff88003de00000(0000) knlGS:0000000000000000
[ 9699.423154] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 9699.423154] CR2: ffffffffc019c230 CR3: 0000000001c13000 CR4: 00000000001406f0
[ 9699.423154] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 9699.423154] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 9699.423154] Stack:
[ 9699.423154]  0000000000000001 0000000000000000 ffff880039654880 ffff8800c7be3ea0
[ 9699.423154]  ffffffff811ff208 00000001034a4c80 ffff880019ce5bb0 ffff880039654880
[ 9699.423154]  ffff880019ce6308 ffff8800126b8e00 ffff8800126b8e60 ffff8800c7be3ec8
[ 9699.423154] Call Trace:
[ 9699.423154]  [<ffffffff811ff208>] put_files_struct+0x78/0xd0
[ 9699.423154]  [<ffffffff811ff307>] exit_files+0x47/0x50
[ 9699.423154]  [<ffffffff810725a7>] do_exit+0x357/0xab0
[ 9699.423154]  [<ffffffff81072d83>] do_group_exit+0x43/0xc0
[ 9699.423154]  [<ffffffff81072e14>] SyS_exit_group+0x14/0x20
[ 9699.423154]  [<ffffffff81787ced>] system_call_fastpath+0x1a/0x1f
[ 9699.423154] Code: ff ff 0f 1f 44 00 00 0f 1f 44 00 00 55 48 89 e5 41 55 41 54 53 48 8b 47 38 48 89 fb 48 85 c0 74 46 48 8b 47 28 45 31 e4 49 89 f5 <48> 8b 40 70 48 85 c0 74 05 ff d0 41 89 c4 f6 43 45 40 75 16 4c
[ 9699.423154] RIP  [<ffffffff811de294>] filp_close+0x24/0x70
[ 9699.423154]  RSP <ffff8800c7be3e48>
[ 9699.423154] CR2: ffffffffc019c230
[ 9699.423154] ---[ end trace 0278723740833669 ]---
[ 9699.423154] Fixing recursive fault but reboot is needed!

I understand an administrator should not unload the driver before stopping the daemon, but this could happen by accident.

-- Dexuan

�NrybXǧv^)޺{.n+{zXܨ}Ơz&j:+vzZ++zfh~izw?&)ߢf^jǫym@Aa0hi

Vitaly Kuznetsov

2015-01-13 13:52 UTC #13 lore raw

In reply to Dexuan Cui

Quoted 4 lines · Dexuan Cui

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.

It seems this patch got lost and I don't see it in recent 'resend' series. K.Y., Dexuan, can you please take a look?

Quoted 55 lines

Cc Jason Wang <[email protected]>

Cc Vitaly Kuznetsov <[email protected]>

Cc K. Y. Srinivasan <[email protected]>

Signed-off-by Dexuan Cui <[email protected]>

---

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(+)

drivers/hv/hv_fcopy.c +19 -0

index 23b2ce2..faa6ba6 100644
@@ -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))
+		;
+
 }
 
 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))
+			;
+		fcopy_respond_to_host(HV_E_FAIL);
+	}
 	return 0;
 }

Signature · 1 line

  Vitaly

Dexuan Cui

author maintainer

2015-01-14 01:44 UTC #14 lore raw

Quoted 12 lines

-----Original Message-----
From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Tuesday, January 13, 2015 21:52 PM
To: Dexuan Cui; KY Srinivasan
Cc: [email protected]; [email protected]; driverdev- [email protected]; [email protected]; [email protected]; [email protected]; Haiyang Zhang
Subject: Re: [PATCH v3] hv: hv_fcopy: drop the obsolete message on transfer failure
Dexuan Cui <[email protected]> writes:

Quoted 7 lines

Quoted 4 lines
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.
It seems this patch got lost and I don't see it in recent 'resend' series. K.Y., Dexuan, can you please take a look?

Hi Vitaly, Jason,
The patch can't fix all the corner cases (it would need non-trivial efforts for that) as we discussed, but I think it would be better for us to have it as it can indeed fix an obvious issue and doesn't introduce new issues.

I think I can document the known discussed corner cases in the patch as TODOs and resend the patch.

Please let me know if you have different opinions. :-)

Thanks,
-- Dexuan

Quoted 63 lines

Quoted 8 lines
Cc Jason Wang <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
Quoted 8 lines
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 static void fcopy_work_func(struct work_struct
Quoted 9 lines
 drivers/hv/hv_fcopy.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
drivers/hv/hv_fcopy.c +0 -0
index 23b2ce2..faa6ba6 100644
@@ -86,6 +86,18 @@ static void fcopy_work_func(struct work_struct
*dummy)
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct work_struct *dummy)
Quoted 6 lines
drivers/hv/hv_fcopy.c +3 -0
 	 * 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
drivers/hv/hv_fcopy.c static int fcopy_release(struct inode *inode, struct
Quoted 13 lines
drivers/hv/hv_fcopy.c +9 -0
+	 * 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))
+		;
+
 }
 
 static int fcopy_handle_handshake(u32 version)
@@ -351,6 +363,13 @@ static int fcopy_release(struct inode *inode, struct
file *f)
drivers/hv/hv_fcopy.c static int fcopy_release(struct inode *inode, struct file *f) · line 374
Quoted 12 lines
drivers/hv/hv_fcopy.c +7 -0
 	 */
 	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))
+			;
+		fcopy_respond_to_host(HV_E_FAIL);
+	}
 	return 0;
 }
--
Vitaly

Jason Wang

2015-01-14 02:28 UTC #15 lore raw

Quoted 27 lines · Dexuan Cui

Quoted 14 lines
-----Original Message-----
From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Tuesday, January 13, 2015 21:52 PM
To: Dexuan Cui; KY Srinivasan
Cc: [email protected]; [email protected];
driverdev-
[email protected]; [email protected]; [email protected]; [email protected]; Haiyang Zhang
Subject: Re: [PATCH v3] hv: hv_fcopy: drop the obsolete message on
transfer
failure
Dexuan Cui <[email protected]> writes:
Quoted 8 lines
Quoted 2 lines
In the case the user-space daemon crashes, hangs or is killed, we need to down the semaphore, otherwise, after the daemon starts
next
Quoted 2 lines
time, the obsolete data in fcopy_transaction.message or fcopy_transaction.fcopy_msg will be used immediately.
It seems this patch got lost and I don't see it in recent 'resend' series. K.Y., Dexuan, can you please take a look?
Hi Vitaly, Jason,
The patch can't fix all the corner cases (it would need non-trivial efforts for that) as we discussed, but I think it would be better for us to have it as it can indeed fix an obvious issue and doesn't introduce new issues.

Quoted 7 lines

I think I can document the known discussed corner cases in the patch as TODOs and resend the patch.
Please let me know if you have different opinions. :-)
Thanks,
-- Dexuan

Yes, I think it's ok.
We can do other fixes on top.

Thanks

KY Srinivasan

2015-01-14 03:21 UTC #16 lore raw

In reply to Dexuan Cui

Quoted 36 lines

-----Original Message-----
From: Dexuan Cui
Sent: Tuesday, January 13, 2015 5:44 PM
To: Vitaly Kuznetsov; KY Srinivasan
Cc: [email protected]; [email protected]; driverdev- [email protected]; [email protected]; [email protected]; [email protected]; Haiyang Zhang
Subject: RE: [PATCH v3] hv: hv_fcopy: drop the obsolete message on transfer failure
Quoted 12 lines
-----Original Message-----
From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Tuesday, January 13, 2015 21:52 PM
To: Dexuan Cui; KY Srinivasan
Cc: [email protected]; [email protected]; driverdev- [email protected]; [email protected]; [email protected]; [email protected]; Haiyang Zhang
Subject: Re: [PATCH v3] hv: hv_fcopy: drop the obsolete message on transfer failure
Dexuan Cui <[email protected]> writes:
Quoted 7 lines
Quoted 4 lines
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.
It seems this patch got lost and I don't see it in recent 'resend' series. K.Y., Dexuan, can you please take a look?
Hi Vitaly, Jason,
The patch can't fix all the corner cases (it would need non-trivial efforts for that) as we discussed, but I think it would be better for us to have it as it can indeed fix an obvious issue and doesn't introduce new issues.

Quoted 4 lines

I think I can document the known discussed corner cases in the patch as TODOs and resend the patch.
Please let me know if you have different opinions. :-)

Resend the patch.

K. Y

Quoted 69 lines

Thanks,
-- Dexuan
Quoted 35 lines
Quoted 10 lines
Cc Jason Wang <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
v2: I removed the "FCP" prefix as Greg asked.
Quoted 7 lines
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 static void fcopy_work_func(struct work_struct
Quoted 9 lines
 drivers/hv/hv_fcopy.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
drivers/hv/hv_fcopy.c index renamed from drivers/hv/hv_fcopy.c +0 -0
23b2ce2..faa6ba6 100644
drivers/hv/hv_fcopy.c +0 -0
@@ -86,6 +86,18 @@ static void fcopy_work_func(struct work_struct
*dummy)
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct work_struct *dummy)
Quoted 6 lines
drivers/hv/hv_fcopy.c +3 -0
 	 * 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
Quoted 4 lines
drivers/hv/hv_fcopy.c static void fcopy_work_func(struct work_struct *dummy)
Quoted 4 lines
drivers/hv/hv_fcopy.c +4 -0
+	 * 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
Quoted 25 lines
Quoted 9 lines
drivers/hv/hv_fcopy.c +5 -0
+	 * still OK, because we've reported the failure to the host.
+	 */
+	if (down_trylock(&fcopy_transaction.read_sema))
+		;
+
 }
 
 static int fcopy_handle_handshake(u32 version) @@ -351,6 +363,13 @@
static int fcopy_release(struct inode *inode, struct
file *f)
drivers/hv/hv_fcopy.c static int fcopy_release(struct inode *inode, struct file *f) · line 374
Quoted 12 lines
drivers/hv/hv_fcopy.c +7 -0
 	 */
 	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))
+			;
+		fcopy_respond_to_host(HV_E_FAIL);
+	}
 	return 0;
 }
--
Vitaly