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

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 
+#include 
 
 #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  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@lists.xensource.com
http://lists.xensource.com/xen-devel



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

Chun Yan Liu writes ("Re: [Xen-devel][PATCH]improve suspend_evtchn lock
processing"):
> Hope there could be some hints. Thanks.
I think we should defer this patch to 4.2.  But here are some
comments in response to your 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.
The usual approach is to say that:
  * When locking you must
      open the file
      fstat
      fcntl F_SETLK[W]
      stat the file and check that the inode is the same
       as the one you have open; if not go back and try again
  * You may delete the lockfile if you have locked it first
> 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?
Perhaps this locking should be moved up into the toolstack, or done
via helper callbacks.
> 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?
If they do then they will have to be serialised.

Ian.

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