EVMS core 1/4: evms.c
A message that is not in the archive (sent off-list or not yet received): 02100307355501.05904@boiler
PATCH 1/1 EVMS core 1/4: evms.c
I have tried twice to send the first part of this patch, but it does not seem to have gone through. I'm guessing it's stuck on a mail filter somewhere. For the time being, please see:
http://marc.theaimsgroup.com/?l=evms-devel&m=103365150530085&w=2
for the contents of evms.c.
Quoted 8 lines · Kevin Corry
Greetings,
Here is part 1 of the EVMS core. This provides the plugin framework and common services.
Quoted 2 lines
+#include <
net/checksum.h>
Networking headers in volume managment code?
Review
Quoted 7 lines
(quoted diff) +7
-0+/* + * string used when validating & logging redundant metadata + */ +u8 *evms_primary_string = "primary"; +EXPORT_SYMBOL(evms_primary_string); +u8 *evms_secondary_string = "secondary"; +EXPORT_SYMBOL(evms_secondary_string);
Why do you export strings? Wouldn't a simple cpp symbol do it? Also the symbol names are bigger than the actual string they represent.. Looks a little pointless :)
Review
Quoted 4 lines
(quoted diff) +4
-0+/** + * SYSCTL - EVMS folder definitions/variables + **/ +#ifdef CONFIG_PROC_FS
Needs to be checked for CONFIG_SYSCTL instead.
Review
Quoted 3 lines
(quoted diff) +3
-0+/**********************************************************/ +/* START -- arch ioctl32 support */ +/**********************************************************/
IMHO this is the wrong place. What about an conditionally compiled evms_ioctl32.c file?
Quoted 6 lines
+/**
+ *find_next_volume- locates first or next logical volume
+ * @lv: current logical volume
+ *
+ * returns the next logical volume or NULL
+ **/
All user of this look like they better used list_for_each?
Review
Quoted 7 lines
(quoted diff) +7
-0+ +/** + * find_next_volume_safe - locates first or next logical volume (safe for removes) + * @next_lv: ptr to next logical volume + * + * returns the next logical volume or NULL + **/
Dito with list_for_each_safe
Review
Quoted 8 lines
(quoted diff) +8
-0+/** + * lookup_volume - finds a logical volume by minor number + * @minor: minor number of logical volume to be found + * + * returns the logical volume of the specified minor or NULL. + **/ +static struct evms_logical_volume * +lookup_volume(int minor)
Very bad if you ever want to be able to use more than one major number.
Quoted 7 lines
(quoted diff) +7
-0+/**********************************************************/ +/* START -- exported functions/Common Services */ +/**********************************************************/ + +/** + * evms_cs_get_version - returns the current EVMS version + * @major: returned major value
Review
Quoted 10 lines
(quoted diff) +10
-0+ * @minor: returned minor value + **/ +void +evms_cs_get_version(int *major, int *minor) +{ + *major = EVMS_MAJOR_VERSION; + *minor = EVMS_MINOR_VERSION; +} + +EXPORT_SYMBOL(evms_cs_get_version);
Scap this. Modules under linux aren't binary compatible.
Review
Quoted 13 lines
(quoted diff) +13
-0+int +evms_cs_check_version(struct evms_version *required, + struct evms_version *actual) +{ + if ((required->major != actual->major) || + (required->minor > actual->minor) || + ((required->minor == actual->minor) && + (required->patchlevel > actual->patchlevel))) + return (-EINVAL); + return 0; +} + +EXPORT_SYMBOL(evms_cs_check_version);
Dito.
Quoted 9 lines
(quoted diff) +9
-0+ +/** + * evms_cs_allocate_logical_node - allocates an evms logical node structure + * @pp: address of the pointer which will contain the address of newly allocated node + * + * allocates and zeros an evms_logical_node structure. + * + * returns: 0 if sucessful + * -ENOMEM if unsuccessful
Review
Quoted 10 lines
(quoted diff) +10
-0+ **/ +int +evms_cs_allocate_logical_node(struct evms_logical_node **pp) +{ + *pp = kmalloc(sizeof (struct evms_logical_node), GFP_KERNEL); + if (*pp == NULL) { + return -ENOMEM; + } + memset(*pp, 0, sizeof (struct evms_logical_node)); + return 0;
A helper for kmalloc + memset looks rather pointles..
Quoted 16 lines
(quoted diff) +16
-0+#define CRC_POLYNOMIAL 0xEDB88320L +static u32 crc_table[256]; +static u32 crc_table_built = FALSE; + +/** + * build_crc_table + * + * initialzes the internal crc table + **/ +static void +build_crc_table(void) +{ + u32 i, j, crc; + + for (i = 0; i <= 255; i++) { + crc = i;
Review
Quoted 10 lines
(quoted diff) +10
-0+ for (j = 8; j > 0; j--) { + if (crc & 1) + crc = (crc >> 1) ^ CRC_POLYNOMIAL; + else + crc >>= 1; + } + crc_table[i] = crc; + } + crc_table_built = TRUE; +}
Is this a different crc from the lib/crc32.c one?
Review
Quoted 7 lines
(quoted diff) +7
-0+ done = FALSE; + while (!done) { + new_entry = mempool_alloc(evms_io_notify_pool, GFP_NOIO); + if (!new_entry) { + schedule(); + continue; + }
Umm..
Quoted 7 lines
(quoted diff) +7
-0+int +evms_cs_volume_request_in_progress(kdev_t dev, + int operation, int *current_count) +{ + struct evms_logical_volume *volume = lookup_volume(minor(dev)); + if (!volume || !volume->node) { + return -ENODEV;
Review
Quoted 10 lines
(quoted diff) +10
-0+ } + if (operation > 0) { + atomic_inc(&volume->requests_in_progress); + } else if (operation < 0) { + atomic_dec(&volume->requests_in_progress); + } + if (current_count) { + *current_count = atomic_read(&volume->requests_in_progress); + } + return 0;
This function should be three ones for the different functionality. Also kdev_t won't last long for block devices..
Quoted 12 lines
(quoted diff) +12
-0+/** + * is_busy - determines if a block_devices is currently in use + * @dev: device to check + * + * determines if a block_device is in use or not + * + * returns: 0 = device is not in use + * -EBUSY if device is in use + * -ENOMEM if unable to get a bdev + **/ +static int +is_busy(kdev_t dev)
Review
Quoted 10 lines
(quoted diff) +10
-0+{ + struct block_device *bdev; + + bdev = bdget(kdev_t_to_nr(dev)); + if (!bdev) + return (-ENOMEM); + if (bd_claim(bdev, is_busy)) + return (-EBUSY); + bd_release(bdev); + return (0);
I don't think this is_busy check is a good idea. Anyways it should be better something like this (then in block_dev.c):
int bd_busy(struct block_device *bdev)
{
int res = 0;
spin_lock(&bdev_lock);
if (bdev->bd_holder)
res = -EBUSY;
spin_unlock(&bdev_lock);
return res;
}Quoted 23 lines
(quoted diff) +21
-0+static int +evms_ioctl_cmd_get_info_level(void *arg) +{ + /* copy info to userspace */ + if (copy_to_user(arg, &evms_info_level, sizeof (evms_info_level))) + return -EFAULT; + + return 0; +} + +/** + * evms_ioctl_cmd_set_info_level + * @arg: int value + * + * sets the evms info (syslog logging) level + * + * returns: 0 = success + * otherwise error code + **/ +static int +evms_ioctl_cmd_set_info_level(void *arg)
Review
Quoted 10 lines
(quoted diff) +10
-0+{ + int temp; + + /* copy info from userspace */ + if (copy_from_user(&temp, arg, sizeof (temp))) + return -EFAULT; + evms_info_level = temp; + + return 0; +}
Didn't you already export these two through /proc?
Quoted 9 lines
(quoted diff) +9
-0+ if (qv->command) { + /* After setting the volume to + * a quiesced state, there could + * be threads (on SMP systems) + * that are executing in the + * function, evms_handle_request, + * between the "wait_event" and the + * "atomic_inc" lines. We need to + * provide a "delay" sufficient
Review
Quoted 10 lines
(quoted diff) +10
-0+ * to allow those threads to + * to reach the atomic_inc's + * before executing the while loop + * below. The "schedule" call should + * provide this. + */ + schedule(); + /* wait for outstanding requests to complete */ + while (atomic_read(&volume->requests_in_progress) > 0) + schedule();
ever heard of waitqueues and wait_event?
Quoted 5 lines
+/**
+ *evms_ioctl_cmd_rediscover_volumes
+ * @inode: vfs ioctl parameter
+ * @file: vfs ioctl parameter
+ * @cmd: vfs ioctl parame
Looks like even the EVMS list snipped the rest of the mail :)
Quoted 4 lines · Christoph Hellwig
I don't think this
is_busycheck is a good idea. Anyways it should be better something like this (then inblock_dev.c):int
bd_busy(struct block_device *bdev)
Quoted 8 lines
{
int res = 0;
spin_lock(&bdev_lock);
if (bdev->bd_holder)
res = -EBUSY;
spin_unlock(&bdev_lock);
return res;
}It's completely useless - any code that actually relies on its value is racy, since there's nothing to prevent bd_claim() from being called just as we drop bdev_lock.
The same applies to original version - if you want to protect some area, use bd_claim() and don't release it until you are out of critical area, damnit.
Quoted 3 lines · Christoph Hellwig
Quoted 1 line
+#include <
net/checksum.h>Networking headers in volume managment code?
Should be <asm/checksum.h>
Quoted 11 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+/* + * string used when validating & logging redundant metadata + */ +u8 *evms_primary_string = "primary"; +EXPORT_SYMBOL(evms_primary_string); +u8 *evms_secondary_string = "secondary"; +EXPORT_SYMBOL(evms_secondary_string);Why do you export strings? Wouldn't a simple cpp symbol do it? Also the symbol names are bigger than the actual string they represent.. Looks a little pointless :)
These can probably be turned into #define's, but I'll have to look into it further.
Quoted 6 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+/** + * SYSCTL - EVMS folder definitions/variables + **/ +#ifdef CONFIG_PROC_FSNeeds to be checked for
CONFIG_SYSCTLinstead.
Yep.
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+/**********************************************************/ +/* START -- arch ioctl32 support */ +/**********************************************************/IMHO this is the wrong place. What about an conditionally compiled
evms_ioctl32.cfile?
Yeah. We've already discussed what to do with this code. It was originally in arch/ppc64/ioctl32.c, but we moved it out of there in order to use common code between ppc64, sparc64, etc, and put it into evms.c for the time being. This may wind up in a separate file, along with the corresponding calls in evms_init_module and evms_exit_module.
Quoted 9 lines
Quoted 6 lines
+/**
+ *find_next_volume- locates first or next logical volume
+ * @lv: current logical volume
+ *
+ * returns the next logical volume or NULL
+ **/All user of this look like they better used
list_for_each?
Quoted 9 lines
Quoted 7 lines
(quoted diff) +3
-0+ +/** + * find_next_volume_safe - locates first or next logical volume (safefor removes) + * @next_lv: ptr to next logical volume
+ *
+ * returns the next logical volume or NULL
+ **/Dito with
list_for_each_safe
Quite possibly. Will look into this as well.
Quoted 11 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+/** + * lookup_volume - finds a logical volume by minor number + * @minor: minor number of logical volume to be found + * + * returns the logical volume of the specified minor or NULL. + **/ +static struct evms_logical_volume * +lookup_volume(int minor)Very bad if you ever want to be able to use more than one major number.
Well, we've been under the impression that linux be going to 12-bit majors and 20-bit minors at some point in the future. That would allow for 1 million volumes under the EVMS driver. If this is so, would there be any reason to use more than one major?
Quoted 11 lines
Quoted 11 lines
(quoted diff) +11
-0+/**********************************************************/ +/* START -- exported functions/Common Services */ +/**********************************************************/ + +/** + * evms_cs_get_version - returns the current EVMS version + * @major: returned major value + * @minor: returned minor value + **/ +void +evms_cs_get_version(int *major, int *minor)
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+{ + *major = EVMS_MAJOR_VERSION; + *minor = EVMS_MINOR_VERSION; +} + +EXPORT_SYMBOL(evms_cs_get_version);Scap this. Modules under linux aren't binary compatible.
This can go. I don't think anyone calls it anymore.
Quoted 7 lines
Quoted 7 lines
(quoted diff) +7
-0+int +evms_cs_check_version(struct evms_version *required, + struct evms_version *actual) +{ + if ((required->major != actual->major) || + (required->minor > actual->minor) || + ((required->minor == actual->minor) &&
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ (required->patchlevel > actual->patchlevel))) + return (-EINVAL); + return 0; +} + +EXPORT_SYMBOL(evms_cs_check_version);Dito.
This function is also used to check volume metadata versions, not just module versions.
Quoted 20 lines
Quoted 20 lines
(quoted diff) +20
-0+#define CRC_POLYNOMIAL 0xEDB88320L +static u32 crc_table[256]; +static u32 crc_table_built = FALSE; + +/** + * build_crc_table + * + * initialzes the internal crc table + **/ +static void +build_crc_table(void) +{ + u32 i, j, crc; + + for (i = 0; i <= 255; i++) { + crc = i; + for (j = 8; j > 0; j--) { + if (crc & 1) + crc = (crc >> 1) ^ CRC_POLYNOMIAL; + else
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ crc >>= 1; + } + crc_table[i] = crc; + } + crc_table_built = TRUE; +}Is this a different crc from the
lib/crc32.cone?
I'll have to check. If the common library code can be used, then we can remove this.
Quoted 11 lines
Quoted 11 lines
(quoted diff) +11
-0+int +evms_cs_volume_request_in_progress(kdev_t dev, + int operation, int *current_count) +{ + struct evms_logical_volume *volume = lookup_volume(minor(dev)); + if (!volume || !volume->node) { + return -ENODEV; + } + if (operation > 0) { + atomic_inc(&volume->requests_in_progress); + } else if (operation < 0) {
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ atomic_dec(&volume->requests_in_progress); + } + if (current_count) { + *current_count = atomic_read(&volume->requests_in_progress); + } + return 0;This function should be three ones for the different functionality.
I suppose. Seems like a matter of personal preference.
Quoted 1 line
Also
kdev_twon't last long for block devices..
True. The calls to this function from the plugins look kind of kludgy due to the kdev_t, so we'll try to discuss different ways to achieve this functionality.
Quoted 27 lines
Quoted 27 lines
(quoted diff) +25
-0+static int +evms_ioctl_cmd_get_info_level(void *arg) +{ + /* copy info to userspace */ + if (copy_to_user(arg, &evms_info_level, sizeof (evms_info_level))) + return -EFAULT; + + return 0; +} + +/** + * evms_ioctl_cmd_set_info_level + * @arg: int value + * + * sets the evms info (syslog logging) level + * + * returns: 0 = success + * otherwise error code + **/ +static int +evms_ioctl_cmd_set_info_level(void *arg) +{ + int temp; + + /* copy info from userspace */
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ if (copy_from_user(&temp, arg, sizeof (temp))) + return -EFAULT; + evms_info_level = temp; + + return 0; +}Didn't you already export these two through /proc?
I don't see any reason why both interfaces can't exist. What if SYSCTL isn't enabled (as hard as that might be to imagine)?
Quoted 13 lines
Quoted 13 lines
(quoted diff) +13
-0+ if (qv->command) { + /* After setting the volume to + * a quiesced state, there could + * be threads (on SMP systems) + * that are executing in the + * function, evms_handle_request, + * between the "wait_event" and the + * "atomic_inc" lines. We need to + * provide a "delay" sufficient + * to allow those threads to + * to reach the atomic_inc's + * before executing the while loop + * below. The "schedule" call should
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ * provide this. + */ + schedule(); + /* wait for outstanding requests to complete */ + while (atomic_read(&volume->requests_in_progress) > 0) + schedule();ever heard of waitqueues and
wait_event?
This can probably be changed.
Thanks for the input!
Signature · 3 lines
Kevin Corry [email protected] http://evms.sourceforge.net/
Quoted 7 lines · Christoph Hellwig
Quoted 5 lines
+/**
+ *evms_ioctl_cmd_rediscover_volumes
+ * @inode: vfs ioctl parameter
+ * @file: vfs ioctl parameter
+ * @cmd: vfs ioctl parameLooks like even the EVMS list snipped the rest of the mail :)
Actually, looks like MARC snipped the end. The version I sent out to evms-devel is complete. Unfortunately, it looks like SF's archives also snipped it.
Try this URL for the full file with latest changes:
Signature · 3 lines
Kevin Corry [email protected] http://evms.sourceforge.net/
Quoted 8 lines · Christoph Hellwig
Quoted 8 lines
+ * allocates and zeros an
evms_logical_nodestructure.
+ *
+ * returns: 0 if sucessful
+ *-ENOMEMif unsuccessful
+ **/(quoted diff) +3
-0+int +evms_cs_allocate_logical_node(struct evms_logical_node **pp) +{
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ *pp = kmalloc(sizeof (struct evms_logical_node), GFP_KERNEL); + if (*pp == NULL) { + return -ENOMEM; + } + memset(*pp, 0, sizeof (struct evms_logical_node)); + return 0;A helper for kmalloc + memset looks rather pointles..
This looks, like they want to slabify it later. But a define might be better here, indeed.
Regards
Ingo Oeser
Signature · 1 line
Science is what we can tell a computer. Art is everything else. --- D.E.Knuth