From: Paolo 'B. G. <bla...@ya...> - 2006-10-29 19:20:30
|
Many cleanups for the UBD driver; these are mostly microfixes, I was waiting to finish and reorder also locking fixes (the code works, it is only to resplit, reproof-read and changelogs must be written) but I decided to send these ones for now. The rest will maybe be merged for 2.6.20. The only locking change is a conversion of a spinlock to a mutex, but it is correct anyway, and it has been tested (which is enough since it relates just to the setup/teardown path) with all debugging options active; I did boot-test and hotplug/hotunplug test. -- Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!". Paolo Giarrusso, aka Blaisorblade http://www.user-mode-linux.org/~blaisorblade Chiacchiera con i tuoi amici in tempo reale! http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com |
From: Paolo 'B. G. <bla...@ya...> - 2006-10-29 19:20:30
|
From: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> With 256 minors and 16 minors used per each UBD device, we can allow the use of up to 16 UBD devices per UML. Also chnage parse_unit and leave to the caller (which already do it) the check for excess numbers, since this is just supposed to do raw parsing. Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> --- arch/um/drivers/ubd_kern.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c index 784c74c..984b5da 100644 --- a/arch/um/drivers/ubd_kern.c +++ b/arch/um/drivers/ubd_kern.c @@ -117,7 +117,7 @@ static int ubd_ioctl(struct inode * inod unsigned int cmd, unsigned long arg); static int ubd_getgeo(struct block_device *bdev, struct hd_geometry *geo); -#define MAX_DEV (8) +#define MAX_DEV (16) static struct block_device_operations ubd_blops = { .owner = THIS_MODULE, @@ -277,7 +277,7 @@ static int parse_unit(char **ptr) return(-1); *ptr = end; } - else if (('a' <= *str) && (*str <= 'h')) { + else if (('a' <= *str) && (*str <= 'z')) { n = *str - 'a'; str++; *ptr = str; Chiacchiera con i tuoi amici in tempo reale! http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com |
From: Paolo 'B. G. <bla...@ya...> - 2006-10-29 19:20:30
|
From: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> Add documentation about some fields in struct ubd, whose meaning is non-obvious due to struct names (should change names altogether, I agree). Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> --- arch/um/drivers/ubd_kern.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c index 984b5da..e034ca4 100644 --- a/arch/um/drivers/ubd_kern.c +++ b/arch/um/drivers/ubd_kern.c @@ -150,8 +150,9 @@ #endif static struct openflags global_openflags = OPEN_FLAGS; struct cow { - /* This is the backing file, actually */ + /* backing file name */ char *file; + /* backing file fd */ int fd; unsigned long *bitmap; unsigned long bitmap_len; @@ -160,6 +161,8 @@ struct cow { }; struct ubd { + /* name (and fd, below) of the file opened for writing, either the + * backing or the cow file. */ char *file; int count; int fd; Chiacchiera con i tuoi amici in tempo reale! http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com |
From: Paolo 'B. G. <bla...@ya...> - 2006-10-29 19:20:36
|
From: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> Rename the ubd_dev array to ubd_devs and then call any "struct ubd" ubd_dev instead of dev, which doesn't make clear what we're treating (and no, it's not hungarian notation - not any more than calling all vm_area_struct vma or all inodes inode). Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> --- arch/um/drivers/ubd_kern.c | 198 ++++++++++++++++++++++---------------------- 1 files changed, 99 insertions(+), 99 deletions(-) diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c index e034ca4..50385cc 100644 --- a/arch/um/drivers/ubd_kern.c +++ b/arch/um/drivers/ubd_kern.c @@ -195,14 +195,14 @@ #define DEFAULT_UBD { \ .cow = DEFAULT_COW, \ } -struct ubd ubd_dev[MAX_DEV] = { [ 0 ... MAX_DEV - 1 ] = DEFAULT_UBD }; +struct ubd ubd_devs[MAX_DEV] = { [ 0 ... MAX_DEV - 1 ] = DEFAULT_UBD }; static int ubd0_init(void) { - struct ubd *dev = &ubd_dev[0]; + struct ubd *ubd_dev = &ubd_devs[0]; - if(dev->file == NULL) - dev->file = "root_fs"; + if(ubd_dev->file == NULL) + ubd_dev->file = "root_fs"; return(0); } @@ -290,7 +290,7 @@ static int parse_unit(char **ptr) static int ubd_setup_common(char *str, int *index_out) { - struct ubd *dev; + struct ubd *ubd_dev; struct openflags flags = global_openflags; char *backing_file; int n, err, i; @@ -345,8 +345,8 @@ static int ubd_setup_common(char *str, i err = 1; spin_lock(&ubd_lock); - dev = &ubd_dev[n]; - if(dev->file != NULL){ + ubd_dev = &ubd_devs[n]; + if(ubd_dev->file != NULL){ printk(KERN_ERR "ubd_setup : device already configured\n"); goto out; } @@ -363,10 +363,10 @@ static int ubd_setup_common(char *str, i flags.s = 1; break; case 'd': - dev->no_cow = 1; + ubd_dev->no_cow = 1; break; case 'c': - dev->shared = 1; + ubd_dev->shared = 1; break; case '=': str++; @@ -393,7 +393,7 @@ break_loop: } if(backing_file){ - if(dev->no_cow) + if(ubd_dev->no_cow) printk(KERN_ERR "Can't specify both 'd' and a " "cow file\n"); else { @@ -401,9 +401,9 @@ break_loop: backing_file++; } } - dev->file = str; - dev->cow.file = backing_file; - dev->boot_openflags = flags; + ubd_dev->file = str; + ubd_dev->cow.file = backing_file; + ubd_dev->boot_openflags = flags; out: spin_unlock(&ubd_lock); return(err); @@ -544,83 +544,83 @@ void kill_io_thread(void) __uml_exitcall(kill_io_thread); -static int ubd_file_size(struct ubd *dev, __u64 *size_out) +static int ubd_file_size(struct ubd *ubd_dev, __u64 *size_out) { char *file; - file = dev->cow.file ? dev->cow.file : dev->file; + file = ubd_dev->cow.file ? ubd_dev->cow.file : ubd_dev->file; return(os_file_size(file, size_out)); } -static void ubd_close(struct ubd *dev) +static void ubd_close(struct ubd *ubd_dev) { - os_close_file(dev->fd); - if(dev->cow.file == NULL) + os_close_file(ubd_dev->fd); + if(ubd_dev->cow.file == NULL) return; - os_close_file(dev->cow.fd); - vfree(dev->cow.bitmap); - dev->cow.bitmap = NULL; + os_close_file(ubd_dev->cow.fd); + vfree(ubd_dev->cow.bitmap); + ubd_dev->cow.bitmap = NULL; } -static int ubd_open_dev(struct ubd *dev) +static int ubd_open_dev(struct ubd *ubd_dev) { struct openflags flags; char **back_ptr; int err, create_cow, *create_ptr; - dev->openflags = dev->boot_openflags; + ubd_dev->openflags = ubd_dev->boot_openflags; create_cow = 0; - create_ptr = (dev->cow.file != NULL) ? &create_cow : NULL; - back_ptr = dev->no_cow ? NULL : &dev->cow.file; - dev->fd = open_ubd_file(dev->file, &dev->openflags, dev->shared, - back_ptr, &dev->cow.bitmap_offset, - &dev->cow.bitmap_len, &dev->cow.data_offset, + create_ptr = (ubd_dev->cow.file != NULL) ? &create_cow : NULL; + back_ptr = ubd_dev->no_cow ? NULL : &ubd_dev->cow.file; + ubd_dev->fd = open_ubd_file(ubd_dev->file, &ubd_dev->openflags, ubd_dev->shared, + back_ptr, &ubd_dev->cow.bitmap_offset, + &ubd_dev->cow.bitmap_len, &ubd_dev->cow.data_offset, create_ptr); - if((dev->fd == -ENOENT) && create_cow){ - dev->fd = create_cow_file(dev->file, dev->cow.file, - dev->openflags, 1 << 9, PAGE_SIZE, - &dev->cow.bitmap_offset, - &dev->cow.bitmap_len, - &dev->cow.data_offset); - if(dev->fd >= 0){ + if((ubd_dev->fd == -ENOENT) && create_cow){ + ubd_dev->fd = create_cow_file(ubd_dev->file, ubd_dev->cow.file, + ubd_dev->openflags, 1 << 9, PAGE_SIZE, + &ubd_dev->cow.bitmap_offset, + &ubd_dev->cow.bitmap_len, + &ubd_dev->cow.data_offset); + if(ubd_dev->fd >= 0){ printk(KERN_INFO "Creating \"%s\" as COW file for " - "\"%s\"\n", dev->file, dev->cow.file); + "\"%s\"\n", ubd_dev->file, ubd_dev->cow.file); } } - if(dev->fd < 0){ - printk("Failed to open '%s', errno = %d\n", dev->file, - -dev->fd); - return(dev->fd); + if(ubd_dev->fd < 0){ + printk("Failed to open '%s', errno = %d\n", ubd_dev->file, + -ubd_dev->fd); + return(ubd_dev->fd); } - if(dev->cow.file != NULL){ + if(ubd_dev->cow.file != NULL){ err = -ENOMEM; - dev->cow.bitmap = (void *) vmalloc(dev->cow.bitmap_len); - if(dev->cow.bitmap == NULL){ + ubd_dev->cow.bitmap = (void *) vmalloc(ubd_dev->cow.bitmap_len); + if(ubd_dev->cow.bitmap == NULL){ printk(KERN_ERR "Failed to vmalloc COW bitmap\n"); goto error; } flush_tlb_kernel_vm(); - err = read_cow_bitmap(dev->fd, dev->cow.bitmap, - dev->cow.bitmap_offset, - dev->cow.bitmap_len); + err = read_cow_bitmap(ubd_dev->fd, ubd_dev->cow.bitmap, + ubd_dev->cow.bitmap_offset, + ubd_dev->cow.bitmap_len); if(err < 0) goto error; - flags = dev->openflags; + flags = ubd_dev->openflags; flags.w = 0; - err = open_ubd_file(dev->cow.file, &flags, dev->shared, NULL, + err = open_ubd_file(ubd_dev->cow.file, &flags, ubd_dev->shared, NULL, NULL, NULL, NULL, NULL); if(err < 0) goto error; - dev->cow.fd = err; + ubd_dev->cow.fd = err; } return(0); error: - os_close_file(dev->fd); + os_close_file(ubd_dev->fd); return(err); } @@ -650,14 +650,14 @@ static int ubd_new_disk(int major, u64 s /* sysfs register (not for ide fake devices) */ if (major == MAJOR_NR) { - ubd_dev[unit].pdev.id = unit; - ubd_dev[unit].pdev.name = DRIVER_NAME; - ubd_dev[unit].pdev.dev.release = dummy_device_release; - platform_device_register(&ubd_dev[unit].pdev); - disk->driverfs_dev = &ubd_dev[unit].pdev.dev; + ubd_devs[unit].pdev.id = unit; + ubd_devs[unit].pdev.name = DRIVER_NAME; + ubd_devs[unit].pdev.dev.release = dummy_device_release; + platform_device_register(&ubd_devs[unit].pdev); + disk->driverfs_dev = &ubd_devs[unit].pdev.dev; } - disk->private_data = &ubd_dev[unit]; + disk->private_data = &ubd_devs[unit]; disk->queue = ubd_queue; add_disk(disk); @@ -669,25 +669,25 @@ #define ROUND_BLOCK(n) ((n + ((1 << 9) - static int ubd_add(int n) { - struct ubd *dev = &ubd_dev[n]; + struct ubd *ubd_dev = &ubd_devs[n]; int err; err = -ENODEV; - if(dev->file == NULL) + if(ubd_dev->file == NULL) goto out; - err = ubd_file_size(dev, &dev->size); + err = ubd_file_size(ubd_dev, &ubd_dev->size); if(err < 0) goto out; - dev->size = ROUND_BLOCK(dev->size); + ubd_dev->size = ROUND_BLOCK(ubd_dev->size); - err = ubd_new_disk(MAJOR_NR, dev->size, n, &ubd_gendisk[n]); + err = ubd_new_disk(MAJOR_NR, ubd_dev->size, n, &ubd_gendisk[n]); if(err) goto out; if(fake_major != MAJOR_NR) - ubd_new_disk(fake_major, dev->size, n, + ubd_new_disk(fake_major, ubd_dev->size, n, &fake_gendisk[n]); /* perhaps this should also be under the "if (fake_major)" above */ @@ -719,7 +719,7 @@ static int ubd_config(char *str) spin_lock(&ubd_lock); err = ubd_add(n); if(err) - ubd_dev[n].file = NULL; + ubd_devs[n].file = NULL; spin_unlock(&ubd_lock); return(err); @@ -727,7 +727,7 @@ static int ubd_config(char *str) static int ubd_get_config(char *name, char *str, int size, char **error_out) { - struct ubd *dev; + struct ubd *ubd_dev; int n, len = 0; n = parse_unit(&name); @@ -736,19 +736,19 @@ static int ubd_get_config(char *name, ch return(-1); } - dev = &ubd_dev[n]; + ubd_dev = &ubd_devs[n]; spin_lock(&ubd_lock); - if(dev->file == NULL){ + if(ubd_dev->file == NULL){ CONFIG_CHUNK(str, size, len, "", 1); goto out; } - CONFIG_CHUNK(str, size, len, dev->file, 0); + CONFIG_CHUNK(str, size, len, ubd_dev->file, 0); - if(dev->cow.file != NULL){ + if(ubd_dev->cow.file != NULL){ CONFIG_CHUNK(str, size, len, ",", 0); - CONFIG_CHUNK(str, size, len, dev->cow.file, 1); + CONFIG_CHUNK(str, size, len, ubd_dev->cow.file, 1); } else CONFIG_CHUNK(str, size, len, "", 1); @@ -769,7 +769,7 @@ static int ubd_id(char **str, int *start static int ubd_remove(int n) { - struct ubd *dev; + struct ubd *ubd_dev; int err = -ENODEV; spin_lock(&ubd_lock); @@ -777,14 +777,14 @@ static int ubd_remove(int n) if(ubd_gendisk[n] == NULL) goto out; - dev = &ubd_dev[n]; + ubd_dev = &ubd_devs[n]; - if(dev->file == NULL) + if(ubd_dev->file == NULL) goto out; /* you cannot remove a open disk */ err = -EBUSY; - if(dev->count > 0) + if(ubd_dev->count > 0) goto out; del_gendisk(ubd_gendisk[n]); @@ -797,8 +797,8 @@ static int ubd_remove(int n) fake_gendisk[n] = NULL; } - platform_device_unregister(&dev->pdev); - *dev = ((struct ubd) DEFAULT_UBD); + platform_device_unregister(&ubd_dev->pdev); + *ubd_dev = ((struct ubd) DEFAULT_UBD); err = 0; out: spin_unlock(&ubd_lock); @@ -876,7 +876,7 @@ int ubd_driver_init(void){ return(0); } err = um_request_irq(UBD_IRQ, thread_fd, IRQ_READ, ubd_intr, - IRQF_DISABLED, "ubd", ubd_dev); + IRQF_DISABLED, "ubd", ubd_devs); if(err != 0) printk(KERN_ERR "um_request_irq failed - errno = %d\n", -err); return 0; @@ -887,24 +887,24 @@ device_initcall(ubd_driver_init); static int ubd_open(struct inode *inode, struct file *filp) { struct gendisk *disk = inode->i_bdev->bd_disk; - struct ubd *dev = disk->private_data; + struct ubd *ubd_dev = disk->private_data; int err = 0; - if(dev->count == 0){ - err = ubd_open_dev(dev); + if(ubd_dev->count == 0){ + err = ubd_open_dev(ubd_dev); if(err){ printk(KERN_ERR "%s: Can't open \"%s\": errno = %d\n", - disk->disk_name, dev->file, -err); + disk->disk_name, ubd_dev->file, -err); goto out; } } - dev->count++; - set_disk_ro(disk, !dev->openflags.w); + ubd_dev->count++; + set_disk_ro(disk, !ubd_dev->openflags.w); /* This should no more be needed. And it didn't work anyway to exclude * read-write remounting of filesystems.*/ - /*if((filp->f_mode & FMODE_WRITE) && !dev->openflags.w){ - if(--dev->count == 0) ubd_close(dev); + /*if((filp->f_mode & FMODE_WRITE) && !ubd_dev->openflags.w){ + if(--ubd_dev->count == 0) ubd_close(ubd_dev); err = -EROFS; }*/ out: @@ -914,10 +914,10 @@ static int ubd_open(struct inode *inode, static int ubd_release(struct inode * inode, struct file * file) { struct gendisk *disk = inode->i_bdev->bd_disk; - struct ubd *dev = disk->private_data; + struct ubd *ubd_dev = disk->private_data; - if(--dev->count == 0) - ubd_close(dev); + if(--ubd_dev->count == 0) + ubd_close(ubd_dev); return(0); } @@ -985,12 +985,12 @@ static void cowify_req(struct io_thread_ static int prepare_request(struct request *req, struct io_thread_req *io_req) { struct gendisk *disk = req->rq_disk; - struct ubd *dev = disk->private_data; + struct ubd *ubd_dev = disk->private_data; __u64 offset; int len; /* This should be impossible now */ - if((rq_data_dir(req) == WRITE) && !dev->openflags.w){ + if((rq_data_dir(req) == WRITE) && !ubd_dev->openflags.w){ printk("Write attempted on readonly ubd device %s\n", disk->disk_name); end_request(req, 0); @@ -1000,8 +1000,8 @@ static int prepare_request(struct reques offset = ((__u64) req->sector) << 9; len = req->current_nr_sectors << 9; - io_req->fds[0] = (dev->cow.file != NULL) ? dev->cow.fd : dev->fd; - io_req->fds[1] = dev->fd; + io_req->fds[0] = (ubd_dev->cow.file != NULL) ? ubd_dev->cow.fd : ubd_dev->fd; + io_req->fds[1] = ubd_dev->fd; io_req->cow_offset = -1; io_req->offset = offset; io_req->length = len; @@ -1010,13 +1010,13 @@ static int prepare_request(struct reques io_req->op = (rq_data_dir(req) == READ) ? UBD_READ : UBD_WRITE; io_req->offsets[0] = 0; - io_req->offsets[1] = dev->cow.data_offset; + io_req->offsets[1] = ubd_dev->cow.data_offset; io_req->buffer = req->buffer; io_req->sectorsize = 1 << 9; - if(dev->cow.file != NULL) - cowify_req(io_req, dev->cow.bitmap, dev->cow.bitmap_offset, - dev->cow.bitmap_len); + if(ubd_dev->cow.file != NULL) + cowify_req(io_req, ubd_dev->cow.bitmap, ubd_dev->cow.bitmap_offset, + ubd_dev->cow.bitmap_len); return(0); } @@ -1054,18 +1054,18 @@ static void do_ubd_request(request_queue static int ubd_getgeo(struct block_device *bdev, struct hd_geometry *geo) { - struct ubd *dev = bdev->bd_disk->private_data; + struct ubd *ubd_dev = bdev->bd_disk->private_data; geo->heads = 128; geo->sectors = 32; - geo->cylinders = dev->size / (128 * 32 * 512); + geo->cylinders = ubd_dev->size / (128 * 32 * 512); return 0; } static int ubd_ioctl(struct inode * inode, struct file * file, unsigned int cmd, unsigned long arg) { - struct ubd *dev = inode->i_bdev->bd_disk->private_data; + struct ubd *ubd_dev = inode->i_bdev->bd_disk->private_data; struct hd_driveid ubd_id = { .cyls = 0, .heads = 128, @@ -1075,7 +1075,7 @@ static int ubd_ioctl(struct inode * inod switch (cmd) { struct cdrom_volctrl volume; case HDIO_GET_IDENTITY: - ubd_id.cyls = dev->size / (128 * 32 * 512); + ubd_id.cyls = ubd_dev->size / (128 * 32 * 512); if(copy_to_user((char __user *) arg, (char *) &ubd_id, sizeof(ubd_id))) return(-EFAULT); Chiacchiera con i tuoi amici in tempo reale! http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com |
From: Jeff D. <jd...@ad...> - 2006-10-30 19:16:56
|
On Sun, Oct 29, 2006 at 08:20:29PM +0100, Paolo 'Blaisorblade' Giarrusso wrote: > From: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> > > and then call any "struct ubd" ubd_dev instead of dev, which doesn't > make clear what we're treating (and no, it's not hungarian notation - > not any more than calling all vm_area_struct vma or all inodes > inode). I can't say that I like this part of it. I don't see any alternate interpretation of a variable called 'dev', and renaming it to 'ubd_dev' seems redundant, given that we are in the ubd driver. Plus, this change sent a couple of lines over the 80-character boundary. Jeff Work email - jdike at linux dot intel dot com |
From: Paolo 'B. G. <bla...@ya...> - 2006-10-29 19:20:37
|
From: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> This lock protects ubd setup and teardown, so is only used in process context; beyond that, during such setup memory allocations must be performed and some generic functions which can sleep must be called (such as add_disk()). So the only correct solution is to make it a mutex instead of a spin_lock. No other change is done - this lock must be acquired in different places but it's done afterwards. Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> --- arch/um/drivers/ubd_kern.c | 23 ++++++++++++----------- 1 files changed, 12 insertions(+), 11 deletions(-) diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c index 23663b7..e4fd29a 100644 --- a/arch/um/drivers/ubd_kern.c +++ b/arch/um/drivers/ubd_kern.c @@ -107,7 +107,8 @@ static inline void ubd_set_bit(__u64 bit #define DRIVER_NAME "uml-blkdev" static DEFINE_SPINLOCK(ubd_io_lock); -static DEFINE_SPINLOCK(ubd_lock); + +static DEFINE_MUTEX(ubd_lock); static void (*do_ubd)(void); @@ -314,7 +315,7 @@ static int ubd_setup_common(char *str, i } err = 1; - spin_lock(&ubd_lock); + mutex_lock(&ubd_lock); if(fake_major != MAJOR_NR){ printk(KERN_ERR "Can't assign a fake major twice\n"); goto out1; @@ -326,7 +327,7 @@ static int ubd_setup_common(char *str, i major); err = 0; out1: - spin_unlock(&ubd_lock); + mutex_unlock(&ubd_lock); return(err); } @@ -343,7 +344,7 @@ static int ubd_setup_common(char *str, i } err = 1; - spin_lock(&ubd_lock); + mutex_lock(&ubd_lock); ubd_dev = &ubd_devs[n]; if(ubd_dev->file != NULL){ @@ -405,7 +406,7 @@ break_loop: ubd_dev->cow.file = backing_file; ubd_dev->boot_openflags = flags; out: - spin_unlock(&ubd_lock); + mutex_unlock(&ubd_lock); return(err); } @@ -716,11 +717,11 @@ static int ubd_config(char *str) } if(n == -1) return(0); - spin_lock(&ubd_lock); + mutex_lock(&ubd_lock); err = ubd_add(n); if(err) ubd_devs[n].file = NULL; - spin_unlock(&ubd_lock); + mutex_unlock(&ubd_lock); return(err); } @@ -737,7 +738,7 @@ static int ubd_get_config(char *name, ch } ubd_dev = &ubd_devs[n]; - spin_lock(&ubd_lock); + mutex_lock(&ubd_lock); if(ubd_dev->file == NULL){ CONFIG_CHUNK(str, size, len, "", 1); @@ -753,7 +754,7 @@ static int ubd_get_config(char *name, ch else CONFIG_CHUNK(str, size, len, "", 1); out: - spin_unlock(&ubd_lock); + mutex_unlock(&ubd_lock); return(len); } @@ -772,7 +773,7 @@ static int ubd_remove(int n) struct ubd *ubd_dev; int err = -ENODEV; - spin_lock(&ubd_lock); + mutex_lock(&ubd_lock); if(ubd_gendisk[n] == NULL) goto out; @@ -801,7 +802,7 @@ static int ubd_remove(int n) *ubd_dev = ((struct ubd) DEFAULT_UBD); err = 0; out: - spin_unlock(&ubd_lock); + mutex_unlock(&ubd_lock); return err; } Chiacchiera con i tuoi amici in tempo reale! http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com |
From: Paolo 'B. G. <bla...@ya...> - 2006-10-29 19:20:38
|
From: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> To rethink locking, I needed to understand well what each function does. While doing this I renamed some: * ubd_close -> ubd_close_dev (since it pairs with ubd_open_dev) * ubd_new_disk -> ubd_disk_register (it handles registration with the block layer - one hopes this makes clearer the difference with ubd_add()) Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> --- arch/um/drivers/ubd_kern.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c index 50385cc..23663b7 100644 --- a/arch/um/drivers/ubd_kern.c +++ b/arch/um/drivers/ubd_kern.c @@ -552,7 +552,7 @@ static int ubd_file_size(struct ubd *ubd return(os_file_size(file, size_out)); } -static void ubd_close(struct ubd *ubd_dev) +static void ubd_close_dev(struct ubd *ubd_dev) { os_close_file(ubd_dev->fd); if(ubd_dev->cow.file == NULL) @@ -629,7 +629,7 @@ static void dummy_device_release(struct { } -static int ubd_new_disk(int major, u64 size, int unit, +static int ubd_disk_register(int major, u64 size, int unit, struct gendisk **disk_out) { @@ -682,12 +682,12 @@ static int ubd_add(int n) ubd_dev->size = ROUND_BLOCK(ubd_dev->size); - err = ubd_new_disk(MAJOR_NR, ubd_dev->size, n, &ubd_gendisk[n]); + err = ubd_disk_register(MAJOR_NR, ubd_dev->size, n, &ubd_gendisk[n]); if(err) goto out; if(fake_major != MAJOR_NR) - ubd_new_disk(fake_major, ubd_dev->size, n, + ubd_disk_register(fake_major, ubd_dev->size, n, &fake_gendisk[n]); /* perhaps this should also be under the "if (fake_major)" above */ @@ -904,7 +904,7 @@ static int ubd_open(struct inode *inode, /* This should no more be needed. And it didn't work anyway to exclude * read-write remounting of filesystems.*/ /*if((filp->f_mode & FMODE_WRITE) && !ubd_dev->openflags.w){ - if(--ubd_dev->count == 0) ubd_close(ubd_dev); + if(--ubd_dev->count == 0) ubd_close_dev(ubd_dev); err = -EROFS; }*/ out: @@ -917,7 +917,7 @@ static int ubd_release(struct inode * in struct ubd *ubd_dev = disk->private_data; if(--ubd_dev->count == 0) - ubd_close(ubd_dev); + ubd_close_dev(ubd_dev); return(0); } Chiacchiera con i tuoi amici in tempo reale! http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com |
From: Paolo 'B. G. <bla...@ya...> - 2006-10-29 19:20:40
|
From: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> Add some comments about requirements for ubd_io_lock and expand its use. When an irq signals that the "controller" (i.e. another thread on the host, which does the actual requests and is the only one blocked on I/O on the host) has done some work, we call again the request function ourselves (do_ubd_request). We now do that with ubd_io_lock held - that's useful to protect against concurrent calls to elv_next_request and so on. XXX: Maybe we shouldn't call at all the request function. Input needed on this. Are we supposed to plug and unplug the queue? That code "indirectly" does that by setting a flag, called do_ubd, which makes the request function return (it's a residual of 2.4 block layer interface). Meanwhile, however, merge this patch, which improves things. Cc: Jens Axboe <ax...@su...> Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> --- arch/um/drivers/ubd_kern.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-) diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c index e4fd29a..1202592 100644 --- a/arch/um/drivers/ubd_kern.c +++ b/arch/um/drivers/ubd_kern.c @@ -106,6 +106,8 @@ static inline void ubd_set_bit(__u64 bit #define DRIVER_NAME "uml-blkdev" +/* Can be taken in interrupt context, and is passed to the block layer to lock + * the request queue. Kernel side code knows that. */ static DEFINE_SPINLOCK(ubd_io_lock); static DEFINE_MUTEX(ubd_lock); @@ -497,6 +499,8 @@ static void __ubd_finish(struct request end_request(req, 1); } +/* Callable only from interrupt context - otherwise you need to do + * spin_lock_irq()/spin_lock_irqsave() */ static inline void ubd_finish(struct request *req, int error) { spin_lock(&ubd_io_lock); @@ -504,7 +508,7 @@ static inline void ubd_finish(struct req spin_unlock(&ubd_io_lock); } -/* Called without ubd_io_lock held */ +/* Called without ubd_io_lock held, and only in interrupt context. */ static void ubd_handler(void) { struct io_thread_req req; @@ -525,7 +529,9 @@ static void ubd_handler(void) ubd_finish(rq, req.error); reactivate_fd(thread_fd, UBD_IRQ); + spin_lock(&ubd_io_lock); do_ubd_request(ubd_queue); + spin_unlock(&ubd_io_lock); } static irqreturn_t ubd_intr(int irq, void *dev) Chiacchiera con i tuoi amici in tempo reale! http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com |
From: Paolo 'B. G. <bla...@ya...> - 2006-10-29 19:20:45
|
From: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> Pure whitespace and style fixes split out from subsequent patch. Some changes (err -> ret) don't make sense now, only later, but I split them out anyway since they cluttered the patch. Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> --- arch/um/drivers/ubd_kern.c | 31 ++++++++++++++++++++----------- 1 files changed, 20 insertions(+), 11 deletions(-) diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c index 1202592..48b2a4f 100644 --- a/arch/um/drivers/ubd_kern.c +++ b/arch/um/drivers/ubd_kern.c @@ -709,27 +709,36 @@ out: static int ubd_config(char *str) { - int n, err; + int n, ret; str = kstrdup(str, GFP_KERNEL); - if(str == NULL){ + if (str == NULL) { printk(KERN_ERR "ubd_config failed to strdup string\n"); - return(1); + ret = 1; + goto out; } - err = ubd_setup_common(str, &n); - if(err){ - kfree(str); - return(-1); + ret = ubd_setup_common(str, &n); + if (ret) { + ret = -1; + goto err_free; + } + if (n == -1) { + ret = 0; + goto out; } - if(n == -1) return(0); mutex_lock(&ubd_lock); - err = ubd_add(n); - if(err) + ret = ubd_add(n); + if (ret) ubd_devs[n].file = NULL; mutex_unlock(&ubd_lock); - return(err); +out: + return ret; + +err_free: + kfree(str); + goto out; } static int ubd_get_config(char *name, char *str, int size, char **error_out) Chiacchiera con i tuoi amici in tempo reale! http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com |
From: Jeff D. <jd...@ad...> - 2006-10-30 19:28:53
|
On Sun, Oct 29, 2006 at 08:20:41PM +0100, Paolo 'Blaisorblade' Giarrusso wrote: > From: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> > - return(1); > + ret = 1; > + goto out; > +out: > + return ret; I think the original form should stay, except for the CodingStyle fix. As Al once pointed out, 'goto out; ... out: return' is spelled 'return'. If you have no cleanup to do before returning, you might as well just return. > + if (n == -1) { > + ret = 0; > + goto out; > } > - if(n == -1) return(0); The comment should have noted the bug fix present here. Jeff |
From: Jeff D. <jd...@ad...> - 2006-10-30 19:32:01
|
On Sun, Oct 29, 2006 at 08:20:41PM +0100, Paolo 'Blaisorblade' Giarrusso wrote: > From: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> Forget my comment about documenting the bug fix - I was looking at the driver with all patches applied and missed that this patch didn't close the leak, and that a later one did, and documented it. Jeff -- Work email - jdike at linux dot intel dot com |
From: Paolo 'B. G. <bla...@ya...> - 2006-10-29 19:20:47
|
From: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> do_ubd is actually just a boolean variable - the way it is used currently is a leftover from the old 2.4 block layer, but it is still used; its use is suspicious, but removing it would be too intrusive for now and needs more thinking. Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> --- arch/um/drivers/ubd_kern.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-) diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c index 48b2a4f..1a85d39 100644 --- a/arch/um/drivers/ubd_kern.c +++ b/arch/um/drivers/ubd_kern.c @@ -112,7 +112,9 @@ static DEFINE_SPINLOCK(ubd_io_lock); static DEFINE_MUTEX(ubd_lock); -static void (*do_ubd)(void); +/* XXX - this made sense in 2.4 days, now it's only used as a boolean, and + * probably it doesn't make sense even for that. */ +static int do_ubd; static int ubd_open(struct inode * inode, struct file * filp); static int ubd_release(struct inode * inode, struct file * file); @@ -508,6 +510,7 @@ static inline void ubd_finish(struct req spin_unlock(&ubd_io_lock); } +/* XXX - move this inside ubd_intr. */ /* Called without ubd_io_lock held, and only in interrupt context. */ static void ubd_handler(void) { @@ -515,7 +518,7 @@ static void ubd_handler(void) struct request *rq = elv_next_request(ubd_queue); int n; - do_ubd = NULL; + do_ubd = 0; intr_count++; n = os_read_file(thread_fd, &req, sizeof(req)); if(n != sizeof(req)){ @@ -1058,7 +1061,7 @@ static void do_ubd_request(request_queue return; err = prepare_request(req, &io_req); if(!err){ - do_ubd = ubd_handler; + do_ubd = 1; n = os_write_file(thread_fd, (char *) &io_req, sizeof(io_req)); if(n != sizeof(io_req)) Chiacchiera con i tuoi amici in tempo reale! http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com |
From: Paolo 'B. G. <bla...@ya...> - 2006-10-29 19:20:50
|
From: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> Use bitfields for boolean fields in ubd data structure. Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> --- arch/um/drivers/ubd_kern.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c index 1a85d39..66dc23d 100644 --- a/arch/um/drivers/ubd_kern.c +++ b/arch/um/drivers/ubd_kern.c @@ -174,8 +174,8 @@ struct ubd { __u64 size; struct openflags boot_openflags; struct openflags openflags; - int shared; - int no_cow; + unsigned shared:1; + unsigned no_cow:1; struct cow cow; struct platform_device pdev; }; Chiacchiera con i tuoi amici in tempo reale! http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com |
From: Paolo 'B. G. <bla...@ya...> - 2006-10-29 19:20:55
|
From: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> To simplify error handling, make sure fd is saved into ubd_dev->fd only when we are sure it is an fd and not an error code. Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> --- arch/um/drivers/ubd_kern.c | 17 ++++++++++------- 1 files changed, 10 insertions(+), 7 deletions(-) diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c index 66dc23d..641782e 100644 --- a/arch/um/drivers/ubd_kern.c +++ b/arch/um/drivers/ubd_kern.c @@ -578,33 +578,36 @@ static int ubd_open_dev(struct ubd *ubd_ struct openflags flags; char **back_ptr; int err, create_cow, *create_ptr; + int fd; ubd_dev->openflags = ubd_dev->boot_openflags; create_cow = 0; create_ptr = (ubd_dev->cow.file != NULL) ? &create_cow : NULL; back_ptr = ubd_dev->no_cow ? NULL : &ubd_dev->cow.file; - ubd_dev->fd = open_ubd_file(ubd_dev->file, &ubd_dev->openflags, ubd_dev->shared, + + fd = open_ubd_file(ubd_dev->file, &ubd_dev->openflags, ubd_dev->shared, back_ptr, &ubd_dev->cow.bitmap_offset, &ubd_dev->cow.bitmap_len, &ubd_dev->cow.data_offset, create_ptr); - if((ubd_dev->fd == -ENOENT) && create_cow){ - ubd_dev->fd = create_cow_file(ubd_dev->file, ubd_dev->cow.file, + if((fd == -ENOENT) && create_cow){ + fd = create_cow_file(ubd_dev->file, ubd_dev->cow.file, ubd_dev->openflags, 1 << 9, PAGE_SIZE, &ubd_dev->cow.bitmap_offset, &ubd_dev->cow.bitmap_len, &ubd_dev->cow.data_offset); - if(ubd_dev->fd >= 0){ + if(fd >= 0){ printk(KERN_INFO "Creating \"%s\" as COW file for " "\"%s\"\n", ubd_dev->file, ubd_dev->cow.file); } } - if(ubd_dev->fd < 0){ + if(fd < 0){ printk("Failed to open '%s', errno = %d\n", ubd_dev->file, - -ubd_dev->fd); - return(ubd_dev->fd); + -fd); + return fd; } + ubd_dev->fd = fd; if(ubd_dev->cow.file != NULL){ err = -ENOMEM; Chiacchiera con i tuoi amici in tempo reale! http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com |
From: Paolo 'B. G. <bla...@ya...> - 2006-10-29 19:20:58
|
From: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> Fix a small memory leak in ubd_config, and clearify the confusion which lead to it. Then, some little changes not affecting operations - * move init functions together, * add a comment about a potential problem in case of some evolution in the block layer, * mark all initcalls as static __init functions * mark an used once little function as inline * document that mconsole methods are all called in process context (was triggered when checking ubd mconsole methods). Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> --- arch/um/drivers/ubd_kern.c | 44 ++++++++++++++++++++++----------------- arch/um/include/mconsole_kern.h | 1 + arch/um/kernel/tt/tracer.c | 1 - 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c index 641782e..8323af0 100644 --- a/arch/um/drivers/ubd_kern.c +++ b/arch/um/drivers/ubd_kern.c @@ -202,17 +202,6 @@ #define DEFAULT_UBD { \ struct ubd ubd_devs[MAX_DEV] = { [ 0 ... MAX_DEV - 1 ] = DEFAULT_UBD }; -static int ubd0_init(void) -{ - struct ubd *ubd_dev = &ubd_devs[0]; - - if(ubd_dev->file == NULL) - ubd_dev->file = "root_fs"; - return(0); -} - -__initcall(ubd0_init); - /* Only changed by fake_ide_setup which is a setup */ static int fake_ide = 0; static struct proc_dir_entry *proc_ide_root = NULL; @@ -293,6 +282,10 @@ static int parse_unit(char **ptr) return(n); } +/* If *index_out == -1 at exit, the passed option was a general one; + * otherwise, the str pointer is used (and owned) inside ubd_devs array, so it + * should not be freed on exit. + */ static int ubd_setup_common(char *str, int *index_out) { struct ubd *ubd_dev; @@ -480,8 +473,9 @@ int thread_fd = -1; /* Changed by ubd_handler, which is serialized because interrupts only * happen on CPU 0. + * XXX: currently unused. */ -int intr_count = 0; +static int intr_count = 0; /* call ubd_finish if you need to serialize */ static void __ubd_finish(struct request *req, int error) @@ -554,7 +548,7 @@ void kill_io_thread(void) __uml_exitcall(kill_io_thread); -static int ubd_file_size(struct ubd *ubd_dev, __u64 *size_out) +static inline int ubd_file_size(struct ubd *ubd_dev, __u64 *size_out) { char *file; @@ -730,7 +724,7 @@ static int ubd_config(char *str) } if (n == -1) { ret = 0; - goto out; + goto err_free; } mutex_lock(&ubd_lock); @@ -827,6 +821,7 @@ out: return err; } +/* All these are called by mconsole in process context and without ubd-specific locks. */ static struct mc_device ubd_mc = { .name = "ubd", .config = ubd_config, @@ -835,7 +830,7 @@ static struct mc_device ubd_mc = { .remove = ubd_remove, }; -static int ubd_mc_init(void) +static int __init ubd_mc_init(void) { mconsole_register_dev(&ubd_mc); return 0; @@ -843,13 +838,24 @@ static int ubd_mc_init(void) __initcall(ubd_mc_init); +static int __init ubd0_init(void) +{ + struct ubd *ubd_dev = &ubd_devs[0]; + + if(ubd_dev->file == NULL) + ubd_dev->file = "root_fs"; + return(0); +} + +__initcall(ubd0_init); + static struct platform_driver ubd_driver = { .driver = { .name = DRIVER_NAME, }, }; -int ubd_init(void) +static int __init ubd_init(void) { int i; @@ -877,7 +883,7 @@ int ubd_init(void) late_initcall(ubd_init); -int ubd_driver_init(void){ +static int __init ubd_driver_init(void){ unsigned long stack; int err; @@ -1384,8 +1390,8 @@ void do_io(struct io_thread_req *req) */ int kernel_fd = -1; -/* Only changed by the io thread */ -int io_count = 0; +/* Only changed by the io thread. XXX: currently unused. */ +static int io_count = 0; int io_thread(void *arg) { diff --git a/arch/um/include/mconsole_kern.h b/arch/um/include/mconsole_kern.h index d0b6901..1ea6d92 100644 --- a/arch/um/include/mconsole_kern.h +++ b/arch/um/include/mconsole_kern.h @@ -14,6 +14,7 @@ struct mconsole_entry { struct mc_request request; }; +/* All these methods are called in process context. */ struct mc_device { struct list_head list; char *name; diff --git a/arch/um/kernel/tt/tracer.c b/arch/um/kernel/tt/tracer.c index 9882342..b919535 100644 --- a/arch/um/kernel/tt/tracer.c +++ b/arch/um/kernel/tt/tracer.c @@ -176,7 +176,6 @@ struct { int signal_index[32]; int nsignals = 0; int debug_trace = 0; -extern int io_nsignals, io_count, intr_count; extern void signal_usr1(int sig); Chiacchiera con i tuoi amici in tempo reale! http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com |
From: Andrew M. <ak...@os...> - 2006-10-29 20:02:36
|
On Sun, 29 Oct 2006 20:17:23 +0100 "Paolo 'Blaisorblade' Giarrusso" <bla...@ya...> wrote: > Many cleanups for the UBD driver; these are mostly microfixes, I was waiting to > finish and reorder also locking fixes (the code works, it is only to resplit, > reproof-read and changelogs must be written) but I decided to send these ones > for now. The rest will maybe be merged for 2.6.20. None of this really looks like -rc3 material. Why do you think it's serious enough to justify late inclusion? I'm not particularly fussed about UBD though - if you and Jeff particularly want this lot in 2.6.19 then the world won't end. "[PATCH 03/11] uml ubd driver: var renames" didn't apply due to dummy_device_release not being present, which doesn't inspire confidence. What tree are you patching? |
From: Blaisorblade <bla...@ya...> - 2006-10-29 20:23:57
|
On Sunday 29 October 2006 21:02, Andrew Morton wrote: > On Sun, 29 Oct 2006 20:17:23 +0100 > > "Paolo 'Blaisorblade' Giarrusso" <bla...@ya...> wrote: > > Many cleanups for the UBD driver; these are mostly microfixes, I was > > waiting to finish and reorder also locking fixes (the code works, it is > > only to resplit, reproof-read and changelogs must be written) but I > > decided to send these ones for now. The rest will maybe be merged for > > 2.6.20. > None of this really looks like -rc3 material. Why do you think it's > serious enough to justify late inclusion? > I'm not particularly fussed about UBD though - if you and Jeff particularly > want this lot in 2.6.19 then the world won't end. If Jeff is worried about these patches destabilizing UML you can held them out for now (but keep in -mm for 2.6.20), that's absolutely fine for me; however there are a few real bug fixes for error paths and IMHO they are safe enough to merge. > "[PATCH 03/11] uml ubd driver: var renames" didn't apply due to > dummy_device_release not being present, which doesn't inspire confidence. > What tree are you patching? It's just git HEAD, but earlier little patches cause that conflict. I am sure that the obvious fixup is correct. I just gave a look to that hunk in the addition log and it looks ok. -- Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!". Paolo Giarrusso, aka Blaisorblade http://www.user-mode-linux.org/~blaisorblade Chiacchiera con i tuoi amici in tempo reale! http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com |
From: Jeff D. <jd...@ad...> - 2006-10-30 19:37:55
|
On Sun, Oct 29, 2006 at 12:02:24PM -0800, Andrew Morton wrote: > I'm not particularly fussed about UBD though - if you and Jeff particularly > want this lot in 2.6.19 then the world won't end. I'm fine with these - I have a stylistic quibble here and there, but the changes are either no-ops or small bug fixes. Jeff -- Work email - jdike at linux dot intel dot com |