[Qemu-devel] [PATCH V2] Add -f option to qemu-nbd

[Qemu-devel] [PATCH V2] Add -f option to qemu-nbd

[Chunyan Liu]

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

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

Re: [Qemu-devel] [PATCH V2] Add -f option to qemu-nbd

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH V2] Add -f option to qemu-nbd

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [PATCH V2] Add -f option to qemu-nbd

[Chun Yan Liu]

[-- Attachment #1.1: Type: text/plain, Size: 1002 bytes --]


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


[-- Attachment #1.2: Type: text/html, Size: 2036 bytes --]

Re: [Qemu-devel] [PATCH V2] Add -f option to qemu-nbd

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH V2] Add -f option to qemu-nbd

[Chun Yan Liu]

[-- Attachment #1.1: Type: text/plain, Size: 2297 bytes --]


I've had a look at nbd driver code and viewed the trace log, and get clear about why the previously mentioned problem happens: 
1st time: qemu-nbd -c /dev/nbd0 disk.img 
nbd_init: send these ioctl(s) in order, SET_BLKSIZE, SET_SIZE, CLEAR_SOCK, SET_SOCK 
nbd_clinet: NBD_DO_IT (it will then handle request(s) in which it should use nbd_device->sock.) 
2st time: qemu-nbd -c /dev/nbd0 disk1.img 
nbd_init: send same ioctl(s) to the same nbd device, it will reset nbd_device->sock 
nbd_client: still send NBD_DO_IT, it find there is on client connected, then return EBUSY and send CLEAR_SOCK, the result is: it will clear ndb_device->sock, which will cause the 1st time "qemu-nbd -c" fail to handle request any longer, including unable to read partition table. 

According to above code logic, if lock in an early place is not accepted, then removing CLEAR_SOCK in nbd_init phase can also solve problem. In fact, if cleanup work done well, I think that ioctl is not needed. Any comments? 
  
diff --git a/nbd.c b/nbd.c 
index e6c931c..067a57b 100644 
--- a/nbd.c 
+++ b/nbd.c 
@@ -386,15 +386,6 @@ int nbd_init(int fd, int csock, uint32_t flags, off_t size, size_t blocksize) 
         return -1; 
     } 

-    TRACE("Clearing NBD socket"); 
- 
-    if (ioctl(fd, NBD_CLEAR_SOCK) == -1) { 
-        int serrno = errno; 
-        LOG("Failed clearing NBD socket"); 
-        errno = serrno; 
-        return -1; 
-    } 
- 
     TRACE("Setting NBD socket"); 

     if (ioctl(fd, NBD_SET_SOCK, csock) == -1) { 

>>> Paolo Bonzini <pbonzini@redhat.com> 11/18/2011 4:59 PM >>>
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



[-- Attachment #1.2: Type: text/html, Size: 6087 bytes --]

Re: [Qemu-devel] [PATCH V2] Add -f option to qemu-nbd

[Paolo Bonzini]

On 11/23/2011 07:51 AM, Chun Yan Liu wrote:
>
> According to above code logic, if lock in an early place is not
> accepted, then removing CLEAR_SOCK in nbd_init phase can also solve
> problem. In fact, if cleanup work done well, I think that ioctl is not
> needed. Any comments?

I think you're right.  In addition, SET_BLKSIZE and SET_SIZE should not 
be sent unless SET_SOCK succeeds.

Paolo