Re: [Xen-devel][PATCH]improve suspend_evtchn lock processing

["Chun Yan Liu" <cyliu@xxxxxxxxxx>]

Dear Ian,
Still about the stale suspend_evtchn_lock problem, I made a mistake in previous flock impl, misunstood the flock usage, sorry for that. But I still have some questions:

First, following is a change to use fcntl instead of flock (not available in newlibc when "make stubdom") and O_EXCL file, but the lock file could not be unlinked safely. There will be many lock files left in system.
To use fcntl properly, the lock file should be created when VM started and deleted when VM destroyed. But I'm not sure if that change is acceptable or not. And I noticed in another patch, you mentioned fcntl (SETLK) is not very portable. Does that mean we'd better not use fcntl + SETLK to implement lock?

Second, in the lastest code, the suspend_evtchn lock is set to each VM. Is there any user case that two processes contend for the suspend event channel of the same VM?

Hope there could be some hints. Thanks.


diff -r 3c4c3d48a835 tools/libxc/xc_suspend.c
--- a/tools/libxc/xc_suspend.c    Thu Aug 26 11:16:56 2010 +0100
+++ b/tools/libxc/xc_suspend.c    Tue Feb 22 23:55:19 2011 +0800
@@ -16,19 +16,22 @@
 
 #include "xc_private.h"
 #include "xenguest.h"
+#include <unistd.h>
+#include <fcntl.h>
 
 #define SUSPEND_LOCK_FILE "/var/lib/xen/suspend_evtchn"
+static int lock_fd = -1;
 static int lock_suspend_event(xc_interface *xch, int domid)
 {
     int fd, rc;
     mode_t mask;
-    char buf[128];
     char suspend_file[256];
+    struct flock fl;
 
     snprintf(suspend_file, sizeof(suspend_file), "%s_%d_lock.d",
         SUSPEND_LOCK_FILE, domid);
     mask = umask(022);
-    fd = open(suspend_file, O_CREAT | O_EXCL | O_RDWR, 0666);
+    fd = open(suspend_file, O_CREAT | O_RDWR, 0666);
     if (fd < 0)
     {
         ERROR("Can't create lock file for suspend event channel %s\n",
@@ -36,43 +39,29 @@
         return -EINVAL;
     }
     umask(mask);
-    snprintf(buf, sizeof(buf), "%10ld", (long)getpid());
 
-    rc = write_exact(fd, buf, strlen(buf));
-    close(fd);
+    memset(&fl,0,sizeof(fl));
+    fl.l_type = F_WRLCK;
+    rc = fcntl(fd, F_SETLK, &fl);
 
+    if (rc != 0){
+        ERROR("Fail to lock suspend event channel\n");
+    close(fd);
+    return rc;
+    }
+
+    lock_fd = fd;
     return rc;
 }
 
 static int unlock_suspend_event(xc_interface *xch, int domid)
 {
-    int fd, pid, n;
-    char buf[128];
-    char suspend_file[256];
-
-    snprintf(suspend_file, sizeof(suspend_file), "%s_%d_lock.d",
-        SUSPEND_LOCK_FILE, domid);
-    fd = open(suspend_file, O_RDWR);
-
-    if (fd < 0)
+    if (lock_fd < 0)
         return -EINVAL;
 
-    n = read(fd, buf, 127);
-
-    close(fd);
-
-    if (n > 0)
-    {
-        sscanf(buf, "%d", &pid);
-        /* We are the owner, so we can simply delete the file */
-        if (pid == getpid())
-        {
-            unlink(suspend_file);
-            return 0;
-        }
-    }
-
-    return -EPERM;
+    close(lock_fd);
+    lock_fd = -1;
+    return 0;
 }
 
 int xc_await_suspend(xc_interface *xch, int xce, int suspend_evtchn)
@@ -127,8 +116,7 @@
     return suspend_evtchn;
 
 cleanup:
-    if (suspend_evtchn != -1)
-        xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn);
+    xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn);
 
     return -1;
 }




>>> Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> 12/14/10 1:46 PM >>>
Chun Yan Liu writes ("Re: [Xen-devel][PATCH]improve suspend_evtchn lock processing"):
> Yes. Using fctrl or flock instead can avoid the problem, better than an O_EXCL lock file.
> I made a patch to use flock instead, please have a review:

Thanks for this patch and sorry for the delay replying. I have just a
few comments:

> diff -r 3c4c3d48a835 tools/libxc/xc_suspend.c
> --- a/tools/libxc/xc_suspend.c Thu Aug 26 11:16:56 2010 +0100
> +++ b/tools/libxc/xc_suspend.c Fri Nov 26 19:41:23 2010 +0800
> @@ -16,19 +16,19 @@
...
> +#include

That looks like a mistake.

> @@ -127,8 +115,7 @@
> return suspend_evtchn;
>
> cleanup:
> - if (suspend_evtchn != -1)
> - xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn);
> + xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn);
>
> return -1;
> }

What is this change for ?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel