diff mbox

[V2] Add -f option to qemu-nbd

Message ID 1321529765-14863-1-git-send-email-cyliu@suse.com
State New
Headers show

Commit Message

Chunyan Liu Nov. 17, 2011, 11:36 a.m. UTC
Adding -f option to qemu-nbd to support finding a free nbd device and connect
disk image to that device. Usage of this option is similar to "losetup -f".
#qemu-nbd -f
show the first free nbd device found at the very moment.
#qemu-nbd -f disk.img
find a free nbd device and connect disk.img to that device.

Adding lock to the nbd device before connecting disk image to that device to
handling race conditions.

Signed-off-by: Chunyan Liu <cyliu@suse.com>
---
 qemu-nbd.c |   90 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 89 insertions(+), 1 deletions(-)

Comments

Paolo Bonzini Nov. 17, 2011, 1:41 p.m. UTC | #1
On 11/17/2011 12:36 PM, Chunyan Liu wrote:
>
> Adding lock to the nbd device before connecting disk image to that device to
> handling race conditions.

This removes the possibility for other programs to lock.  Have you 
checked what happens if you use the same device twice and whether you 
can piggyback on e.g. an EBUSY from the NBD_SET_SOCK ioctl?

Paolo
Stefan Hajnoczi Nov. 17, 2011, 1:52 p.m. UTC | #2
On Thu, Nov 17, 2011 at 1:41 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 11/17/2011 12:36 PM, Chunyan Liu wrote:
>>
>> Adding lock to the nbd device before connecting disk image to that device
>> to
>> handling race conditions.
>
> This removes the possibility for other programs to lock.  Have you checked
> what happens if you use the same device twice and whether you can piggyback
> on e.g. an EBUSY from the NBD_SET_SOCK ioctl?

Yes, I just sent an email showing how we can use NBD_SET_LOCK for -EBUSY.

Stefan
Chunyan Liu Nov. 18, 2011, 1:25 a.m. UTC | #3
Yes. I have tested using same device twice as described in my previous mail. Without lock: 
If issuing "qemu-nbd -c /dev/nbd0 disk.img" and "qemu-nbd -c  /dev/nbd0 disk1.img" almost at the same time, both can pass nbd_init() and get to nbd_client(), then the latter one will fail and exit, but the first one does not work well either (fail to show partitions.) That's why I think we should add a lock in an earlier time. 
If issuing two commands in a long enough inetval, the first one can work well, the second one will fail at nbd_init(). 

Thanks, 
Chunyan 

>>> Paolo Bonzini <pbonzini@redhat.com> 11/17/2011 9:41 PM >>>
On 11/17/2011 12:36 PM, Chunyan Liu wrote:
>
> Adding lock to the nbd device before connecting disk image to that device to
> handling race conditions.
This removes the possibility for other programs to lock.  Have you
checked what happens if you use the same device twice and whether you
can piggyback on e.g. an EBUSY from the NBD_SET_SOCK ioctl?

Paolo
Paolo Bonzini Nov. 18, 2011, 8:59 a.m. UTC | #4
On 11/18/2011 02:25 AM, Chun Yan Liu wrote:
> Yes. I have tested using same device twice as described in my previous
> mail. Without lock:
>
> If issuing "qemu-nbd -c /dev/nbd0 disk.img" and "qemu-nbd -c  /dev/nbd0
> disk1.img" almost at the same time, both can pass nbd_init() and get to
> nbd_client(), then the latter one will fail and exit, but the first one
> does not work well either (fail to show partitions.) That's why I think
> we should add a lock in an earlier time.

This is an initialization problem.  As Stefan wrote, functionality for 
atomic acquisition of NBD devices is provided by the kernel; the problem 
is simply that QEMU does not use it. :)

Paolo
diff mbox

Patch

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 291cba2..64892a8 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -32,6 +32,7 @@ 
 #include <signal.h>
 #include <libgen.h>
 #include <pthread.h>
+#include <fcntl.h>
 
 #define SOCKET_PATH    "/var/lock/qemu-nbd-%s"
 
@@ -244,6 +245,60 @@  out:
     return (void *) EXIT_FAILURE;
 }
 
+static int is_nbd_used(int minor)
+{
+    FILE *proc;
+    int NBDMAJOR = 43;
+    char buf[BUFSIZ];
+    int find = 0;
+
+    proc = fopen("/proc/partitions", "r");
+    if (proc != NULL) {
+        while (fgets(buf, sizeof(buf), proc)) {
+            int m, n;
+            unsigned long long sz;
+            char name[16];
+            char *pname = name;
+            char *end;
+
+            if (sscanf(buf, " %d %d %llu %128[^\n ]",
+                    &m, &n, &sz, name) != 4)
+                continue;
+            if (m != NBDMAJOR)
+                continue;
+            if (strncmp(name, "nbd", 3))
+                continue;
+            pname += 3;
+            n = strtol(pname, &end, 10);
+            if (end && end != pname && *end == '\0' && n == minor) {
+                find = 1;
+                break;
+            }
+        }
+        fclose(proc);
+    }
+
+    return find;
+}
+
+static char *find_free_nbd_device(void)
+{
+    int i;
+    int nbds_max = 16;
+    char name[16];
+    char *devname = NULL;
+
+    for (i = 0; i < nbds_max; i++) {
+        if (!is_nbd_used(i)) {
+            snprintf(name, sizeof(name), "/dev/nbd%d", i);
+            devname = strdup(name);
+            break;
+        }
+    }
+
+    return devname;
+}
+
 int main(int argc, char **argv)
 {
     BlockDriverState *bs;
@@ -256,7 +311,7 @@  int main(int argc, char **argv)
     struct sockaddr_in addr;
     socklen_t addr_len = sizeof(addr);
     off_t fd_size;
-    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:t";
+    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:tf";
     struct option lopt[] = {
         { "help", 0, NULL, 'h' },
         { "version", 0, NULL, 'V' },
@@ -273,6 +328,7 @@  int main(int argc, char **argv)
         { "shared", 1, NULL, 'e' },
         { "persistent", 0, NULL, 't' },
         { "verbose", 0, NULL, 'v' },
+        { "find", 0, NULL, 'f' },
         { NULL, 0, NULL, 0 }
     };
     int ch;
@@ -292,6 +348,9 @@  int main(int argc, char **argv)
     int max_fd;
     int persistent = 0;
     pthread_t client_thread;
+    char *devname = NULL;
+    int find = 0;
+    struct flock lock;
 
     /* The client thread uses SIGTERM to interrupt the server.  A signal
      * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -374,6 +433,18 @@  int main(int argc, char **argv)
         case 'v':
             verbose = 1;
             break;
+        case 'f':
+            find = 1;
+            devname = find_free_nbd_device();
+            if (devname == NULL)
+                exit(1);
+            if (argc == optind) {
+                printf("%s\n", devname);
+                free(devname);
+                exit(0);
+            }
+            device = devname;
+            break;
         case 'V':
             version(argv[0]);
             exit(0);
@@ -464,11 +535,28 @@  int main(int argc, char **argv)
         /* Open before spawning new threads.  In the future, we may
          * drop privileges after opening.
          */
+retry:
         fd = open(device, O_RDWR);
         if (fd == -1) {
             err(EXIT_FAILURE, "Failed to open %s", device);
         }
 
+        memset(&lock, 0, sizeof(lock));
+        lock.l_type = F_WRLCK;
+        lock.l_whence = SEEK_SET;
+        if (fcntl(fd, F_SETLK, &lock) != 0) {
+            if (find) {
+                close(fd);
+                free(device);
+                device = find_free_nbd_device();
+                if (!device)
+                    err(EXIT_FAILURE, "Could not find free nbd device");
+                goto retry;
+            } else {
+                err(EXIT_FAILURE, "Could not lock %s", device);
+            }
+        }
+
         if (sockpath == NULL) {
             sockpath = g_malloc(128);
             snprintf(sockpath, 128, SOCKET_PATH, basename(device));