| Message ID | 1321426669-7337-1-git-send-email-cyliu@suse.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 16, 2011 at 6:57 AM, Chunyan Liu <cyliu@suse.com> wrote: > Currently qemu-nbd does not support finding free nbd device for users like > "losetup -f" and issuing "qemu-nbd -c /dev/nbdX disk.img" won't report error > message when /dev/nbd is already in use. It makes things a little confusing. > This patch adds "-f" option to qemu-nbd to support finding a free nbd device > for users. Please review and share your comments. Thanks. > > Signed-off-by: Chunyan Liu <cyliu@suse.com> > --- > qemu-nbd.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 64 insertions(+), 1 deletions(-) This patch finds a free device but does not immediately attach to it and use it. Interfaces like this are prone to race conditions, I think it would make more sense to combine the -f option with running the actual NBD server. I suggest: qemu-nbd -f disk.img That way it is safe to execute multiple qemu-nbd -f at the same time without race conditions. Plus it probably makes the user's life easier than having to say qemu-nbd -c $(qemu-nbd -f) disk.img. Stefan
On Wed, Nov 16, 2011 at 6:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Wed, Nov 16, 2011 at 6:57 AM, Chunyan Liu <cyliu@suse.com> wrote: >> Currently qemu-nbd does not support finding free nbd device for users like >> "losetup -f" and issuing "qemu-nbd -c /dev/nbdX disk.img" won't report error >> message when /dev/nbd is already in use. It makes things a little confusing. >> This patch adds "-f" option to qemu-nbd to support finding a free nbd device >> for users. Please review and share your comments. Thanks. >> >> Signed-off-by: Chunyan Liu <cyliu@suse.com> >> --- >> qemu-nbd.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 files changed, 64 insertions(+), 1 deletions(-) > > This patch finds a free device but does not immediately attach to it > and use it. Interfaces like this are prone to race conditions, I > think it would make more sense to combine the -f option with running > the actual NBD server. > > I suggest: > qemu-nbd -f disk.img Why must we add one new option? I prefer to not adding new option, only enhance existing function of qemu-nbd -c disk.img. > > That way it is safe to execute multiple qemu-nbd -f at the same time > without race conditions. Plus it probably makes the user's life > easier than having to say qemu-nbd -c $(qemu-nbd -f) disk.img. > > Stefan > >
On Wed, Nov 16, 2011 at 11:49 AM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote: > On Wed, Nov 16, 2011 at 6:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: >> On Wed, Nov 16, 2011 at 6:57 AM, Chunyan Liu <cyliu@suse.com> wrote: >>> Currently qemu-nbd does not support finding free nbd device for users like >>> "losetup -f" and issuing "qemu-nbd -c /dev/nbdX disk.img" won't report error >>> message when /dev/nbd is already in use. It makes things a little confusing. >>> This patch adds "-f" option to qemu-nbd to support finding a free nbd device >>> for users. Please review and share your comments. Thanks. >>> >>> Signed-off-by: Chunyan Liu <cyliu@suse.com> >>> --- >>> qemu-nbd.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >>> 1 files changed, 64 insertions(+), 1 deletions(-) >> >> This patch finds a free device but does not immediately attach to it >> and use it. Interfaces like this are prone to race conditions, I >> think it would make more sense to combine the -f option with running >> the actual NBD server. >> >> I suggest: >> qemu-nbd -f disk.img > Why must we add one new option? I prefer to not adding new option, > only enhance existing function of qemu-nbd -c disk.img. That would change the command-line interface and break existing users. There might be scripts or tools that call out to qemu-nbd, they would break if we changed the meaning of -c. Stefan
On Wed, 2011-11-16 at 10:34 +0000, Stefan Hajnoczi wrote: > On Wed, Nov 16, 2011 at 6:57 AM, Chunyan Liu <cyliu@suse.com> wrote: > > Currently qemu-nbd does not support finding free nbd device for users like > > "losetup -f" and issuing "qemu-nbd -c /dev/nbdX disk.img" won't report error > > message when /dev/nbd is already in use. It makes things a little confusing. > > This patch adds "-f" option to qemu-nbd to support finding a free nbd device > > for users. Please review and share your comments. Thanks. > > > > Signed-off-by: Chunyan Liu <cyliu@suse.com> > > --- > > qemu-nbd.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > > 1 files changed, 64 insertions(+), 1 deletions(-) > > This patch finds a free device but does not immediately attach to it > and use it. Interfaces like this are prone to race conditions, I > think it would make more sense to combine the -f option with running > the actual NBD server. > > I suggest: > qemu-nbd -f disk.img > > That way it is safe to execute multiple qemu-nbd -f at the same time > without race conditions. I agree, but you'd also need some locking inside qemu-nbd wouldn't you? Or have it just keep trying devices until one works perhaps. > Plus it probably makes the user's life > easier than having to say qemu-nbd -c $(qemu-nbd -f) disk.img. Absolutely. Ian.
On Wed, Nov 16, 2011 at 10:27 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Wed, Nov 16, 2011 at 11:49 AM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote: >> On Wed, Nov 16, 2011 at 6:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: >>> On Wed, Nov 16, 2011 at 6:57 AM, Chunyan Liu <cyliu@suse.com> wrote: >>>> Currently qemu-nbd does not support finding free nbd device for users like >>>> "losetup -f" and issuing "qemu-nbd -c /dev/nbdX disk.img" won't report error >>>> message when /dev/nbd is already in use. It makes things a little confusing. >>>> This patch adds "-f" option to qemu-nbd to support finding a free nbd device >>>> for users. Please review and share your comments. Thanks. >>>> >>>> Signed-off-by: Chunyan Liu <cyliu@suse.com> >>>> --- >>>> qemu-nbd.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >>>> 1 files changed, 64 insertions(+), 1 deletions(-) >>> >>> This patch finds a free device but does not immediately attach to it >>> and use it. Interfaces like this are prone to race conditions, I >>> think it would make more sense to combine the -f option with running >>> the actual NBD server. >>> >>> I suggest: >>> qemu-nbd -f disk.img >> Why must we add one new option? I prefer to not adding new option, >> only enhance existing function of qemu-nbd -c disk.img. > > That would change the command-line interface and break existing users. > There might be scripts or tools that call out to qemu-nbd, they would > break if we changed the meaning of -c. Sound reasonable, thanks > > Stefan >
On Wed, Nov 16, 2011 at 5:23 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Wed, 2011-11-16 at 10:34 +0000, Stefan Hajnoczi wrote: >> On Wed, Nov 16, 2011 at 6:57 AM, Chunyan Liu <cyliu@suse.com> wrote: >> > Currently qemu-nbd does not support finding free nbd device for users like >> > "losetup -f" and issuing "qemu-nbd -c /dev/nbdX disk.img" won't report error >> > message when /dev/nbd is already in use. It makes things a little confusing. >> > This patch adds "-f" option to qemu-nbd to support finding a free nbd device >> > for users. Please review and share your comments. Thanks. >> > >> > Signed-off-by: Chunyan Liu <cyliu@suse.com> >> > --- >> > qemu-nbd.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> > 1 files changed, 64 insertions(+), 1 deletions(-) >> >> This patch finds a free device but does not immediately attach to it >> and use it. Interfaces like this are prone to race conditions, I >> think it would make more sense to combine the -f option with running >> the actual NBD server. >> >> I suggest: >> qemu-nbd -f disk.img >> >> That way it is safe to execute multiple qemu-nbd -f at the same time >> without race conditions. > > I agree, but you'd also need some locking inside qemu-nbd wouldn't you? > Or have it just keep trying devices until one works perhaps. Right, I haven't checked the nbd driver iterface but you'd have to actually open the device and claim it in order to atomically find out if it is free. But that's probably not much more work than what this patch does, it just has the advantage of being safer. Stefan
Thanks for your suggestions. For the usage "qemu-nbd -f disk.img", adding some code could implement it. I think it could be like "losetup -f" usage. #qemu-nbd -f show the first free nbd device at this moment. user can choose to issue "qemu-nbd -c THAT_DEVICE disk.img" or not. #qemu-nbd -f disk.img find a free nbd device and connect disk.img to that device. How do you think? For the race conditions caused by executing multiple qemu-nbd -f at the same time, I've tried both ways (1. lock; 2. if one device not work, trying other devices until one works). In my testing, the 2nd way has problem. When issuing "qemu-nbd -c /dev/nbd0 disk.img -v" and "qemu-nbd -c /dev/nbd0 disk1.img -v" at the same time, the latter one will eventually exit with EXIT_FAILURE, but the first one cannot work normally as well, it cannot show disk partitions. Executing multiple "qemu-nbd -f" has same problem. So, it seems using lock from a earlier time is more proper. In my testing, I'm using file lock (fcntl). For "qemu-nbd -c" case, if lock failed, qemu-nbd exits directly. For "qemu-nbd -f" case, if lock failed, redo find_free_nbd_device (there might be updates) and then try to connectdisk.img to the new free device. Will post V2 soon. >>> Ian Campbell <Ian.Campbell@citrix.com> 11/17/2011 1:23 AM >>> On Wed, 2011-11-16 at 10:34 +0000, Stefan Hajnoczi wrote: > On Wed, Nov 16, 2011 at 6:57 AM, Chunyan Liu <cyliu@suse.com> wrote: > > Currently qemu-nbd does not support finding free nbd device for users like > > "losetup -f" and issuing "qemu-nbd -c /dev/nbdX disk.img" won't report error > > message when /dev/nbd is already in use. It makes things a little confusing. > > This patch adds "-f" option to qemu-nbd to support finding a free nbd device > > for users. Please review and share your comments. Thanks. > > > > Signed-off-by: Chunyan Liu <cyliu@suse.com> > > --- > > qemu-nbd.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > > 1 files changed, 64 insertions(+), 1 deletions(-) > > This patch finds a free device but does not immediately attach to it > and use it. Interfaces like this are prone to race conditions, I > think it would make more sense to combine the -f option with running > the actual NBD server. > > I suggest: > qemu-nbd -f disk.img > > That way it is safe to execute multiple qemu-nbd -f at the same time > without race conditions. I agree, but you'd also need some locking inside qemu-nbd wouldn't you? Or have it just keep trying devices until one works perhaps. > Plus it probably makes the user's life > easier than having to say qemu-nbd -c $(qemu-nbd -f) disk.img. Absolutely. Ian.
On Thu, Nov 17, 2011 at 11:34 AM, Chun Yan Liu <cyliu@suse.com> wrote: > Thanks for your suggestions. > > For the usage "qemu-nbd -f disk.img", adding some code could implement it. I > think it could be like "losetup -f" usage. > > #qemu-nbd -f > > show the first free nbd device at this moment. > > user can choose to issue "qemu-nbd -c THAT_DEVICE disk.img" or not. > > #qemu-nbd -f disk.img > > find a free nbd device and connect disk.img to that device. > > How do you think? > > For the race conditions caused by executing multiple qemu-nbd -f at the same > time, I've tried both ways (1. lock; 2. if one device not work, trying other > devices until one works). > > In my testing, the 2nd way has problem. When issuing "qemu-nbd -c /dev/nbd0 > disk.img -v" and "qemu-nbd -c /dev/nbd0 disk1.img -v" at the same time, the > latter one will eventually exit with EXIT_FAILURE, but the first one cannot > work normally as well, it cannot show disk partitions. Executing multiple > "qemu-nbd -f" has same problem. The problem is that qemu/nbd.c is doing it wrong. The linux/drivers/block/nbd.c driver returns -EBUSY from ioctl(NBD_SET_SOCK) if the device already has a socket configured. However, qemu/nbd.c issues NBD_SET_BLKSIZE and other settings ioctls *before* NBD_SET_SOCK. This clobbers the existing nbd connection settings. Then we take over the device using NBD_CLEAR_SOCK and NBD_SET_SOCK instead of noticing there is already a socket configured. I think qemu/nbd.c:nbd_init() should be fixed to: 1. NBD_SET_SOCK. This fails with -EBUSY if an existing socket is configured. This is the atomic acquire/test operation. 2. NBD_SET_BLKSIZE and other settings ioctls. The only user-visible change is that qemu-nbd -c /dev/nbd0 no longer hijacks the nbd device. Instead it properly detects -EBUSY. This seems like a reasonable change to make although we could restrict it to only qemu-nbd -f disk.img in order to preserve the current behavior for qemu-nbd -c /dev/nbd0. I don't think using file locks is necessary since it can be done properly through the nbd ioctl interface. Stefan
diff --git a/qemu-nbd.c b/qemu-nbd.c index 291cba2..5c43a68 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -244,6 +244,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 +310,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 +327,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 +347,7 @@ int main(int argc, char **argv) int max_fd; int persistent = 0; pthread_t client_thread; + char *devname = NULL; /* 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 +430,13 @@ int main(int argc, char **argv) case 'v': verbose = 1; break; + case 'f': + devname = find_free_nbd_device(); + if (devname) { + printf("%s\n", devname); + } + exit(0); + break; case 'V': version(argv[0]); exit(0);
Currently qemu-nbd does not support finding free nbd device for users like "losetup -f" and issuing "qemu-nbd -c /dev/nbdX disk.img" won't report error message when /dev/nbd is already in use. It makes things a little confusing. This patch adds "-f" option to qemu-nbd to support finding a free nbd device for users. Please review and share your comments. Thanks. Signed-off-by: Chunyan Liu <cyliu@suse.com> --- qemu-nbd.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 64 insertions(+), 1 deletions(-)