答复： [Ocfs2-devel] [patch 2/3]OCFS2: allow the ocfs2 heartbeat thread to prioritize I/O --revision 4
Zhen Wei
zwei at novell.com
Sat Dec 9 22:45:31 PST 2006
hi, Mark:
according your suggestion, I modified the patch:
- define the "/usr/bin/ionice" path
- verify the input args like other option
- remove the printing errors code
zhen wei
zwei at novell.com
+86 10 65339225
>>> Mark Fasheh <mark.fasheh at oracle.com> 06年12月09日 上午 9:20 >>>
Hi Zwei,
On Fri, Dec 08, 2006 at 12:48:26AM -0700, Zhen Wei wrote:
> - replace calling syscall interface with running ionice
> - fix the running ionice return code
> - use device name instead of uuid for mount.ocfs2
The kernel patch and the mount.ocfs2 patches look good. I only have a couple
of comments for this one below.
> @@ -289,6 +292,41 @@ static errcode_t start_heartbeat(struct
> return err;
> }
>
> +static errcode_t adjust_priority(struct hb_ctl_options *hbo)
> +{
> + int ret, child_status;
> + pid_t hb_pid, child_pid;
> + char level_arg[16], pid_arg[16];
> +
> + if (access ("/usr/bin/ionice", X_OK) != 0)
You might want to make "/usr/bin/ionice" a #define at the top of this file,
since we use it at least twice.
> + return OCFS2_ET_NO_IONICE;
> +
> + get_uuid (hbo->dev_str, hbo->uuid_str);
You shouldn't have to call get_uuid() here -- see my comments below.
> @@ -385,6 +432,13 @@ static int process_options(struct hb_ctl
> ret = -EINVAL;
> break;
>
> + case HB_ACTION_IONICE:
> + /* ionice needs uuid and priority */
> + if (!hbo->dev_str || hbo->io_prio < 0 ||
> + hbo->io_prio > 7)
> + ret = -EINVAL;
> + break;
The ionice option should be just like the other options and allow the user
to specify an uuid _or_ a device. The main function will call get_uuid() if
the device was specified.
This will also help avoid the manual call to get_uuid() you have up in
adjust_priority().
> @@ -483,6 +538,14 @@ int main(int argc, char **argv)
> }
> break;
>
> + case HB_ACTION_IONICE:
> + err = adjust_priority(&hbo);
> + if (err) {
> + com_err(progname, err, "while adjusting heartbeat thread I/O priority");
> + ret = err;
> + }
You're still printing errors unconditionally.
To break it down, the two errors we don't want to print for are:
* Not finding that the heartbeat region 'pid' attribute exists in configfs.
(Should be an O2CB_ET_SERVICE_UNAVAILABLE return)
* Not being able to run /usr/bin/ionice because it doesn't exist.
(Should be an OCFS2_ET_NO_IONICE return)
Neither of those two errors should be fatal, or should print to the user.
You'll have to filter them from the com_err.
Right now the rule is that all versions of ocfs2-tools must be compatible
with old versions of the file system module.
Thanks for the patches so far,
--Mark
--
Mark Fasheh
Senior Software Developer, Oracle
mark.fasheh at oracle.com
-------------- next part --------------
--- trunk.orig/libocfs2/ocfs2_err.et 2006-12-08 15:09:42.000000000 +0800
+++ trunk/libocfs2/ocfs2_err.et 2006-12-08 15:14:49.000000000 +0800
@@ -159,4 +159,7 @@ ec OCFS2_ET_BLOCK_SIZE_TOO_SMALL_FOR_HAR
ec OCFS2_ET_INVALID_LOCKRES,
"The lock name is invalid"
+ec OCFS2_ET_NO_IONICE,
+ "Can't find ionice"
+
end
--- trunk.orig/libo2cb/include/o2cb.h 2006-12-08 15:09:42.000000000 +0800
+++ trunk/libo2cb/include/o2cb.h 2006-12-08 15:09:55.000000000 +0800
@@ -98,6 +98,10 @@ errcode_t o2cb_start_heartbeat_region_pe
errcode_t o2cb_stop_heartbeat_region_perm(const char *cluster_name,
const char *region_name);
+errcode_t o2cb_get_hb_thread_pid (const char *cluster_name,
+ const char *region_name,
+ pid_t *pid);
+
errcode_t o2cb_get_region_ref(const char *region_name,
int undo);
errcode_t o2cb_put_region_ref(const char *region_name,
--- trunk.orig/libo2cb/o2cb_abi.c 2006-12-08 15:09:42.000000000 +0800
+++ trunk/libo2cb/o2cb_abi.c 2006-12-08 15:09:55.000000000 +0800
@@ -1257,6 +1257,36 @@ void o2cb_free_nodes_list(char **nodes)
o2cb_free_dir_list(nodes);
}
+errcode_t o2cb_get_hb_thread_pid (const char *cluster_name, const char *region_name,
+ pid_t *pid)
+{
+ char attr_path[PATH_MAX];
+ char _fake_cluster_name[NAME_MAX];
+ char attr_value[16];
+ errcode_t ret;
+
+ if (!cluster_name) {
+ ret = _fake_default_cluster(_fake_cluster_name);
+ if (ret)
+ return ret;
+ cluster_name = _fake_cluster_name;
+ }
+
+ ret = snprintf(attr_path, PATH_MAX - 1,
+ O2CB_FORMAT_HEARTBEAT_REGION_ATTR,
+ configfs_path, cluster_name, region_name,
+ "pid");
+
+ if ((ret <= 0) || (ret == (PATH_MAX - 1)))
+ return O2CB_ET_INTERNAL_FAILURE;
+
+ ret = o2cb_get_attribute(attr_path, attr_value, sizeof(attr_value) - 1);
+ if (ret == 0)
+ *pid = atoi (attr_value);
+
+ return ret;
+}
+
errcode_t o2cb_get_node_num(const char *cluster_name, const char *node_name,
uint16_t *node_num)
{
--- trunk.orig/ocfs2_hb_ctl/ocfs2_hb_ctl.c 2006-12-08 15:09:42.000000000 +0800
+++ trunk/ocfs2_hb_ctl/ocfs2_hb_ctl.c 2006-12-10 14:24:41.000000000 +0800
@@ -29,6 +29,7 @@
#define _GNU_SOURCE /* Because libc really doesn't want us using O_DIRECT? */
#include <sys/types.h>
+#include <sys/wait.h>
#include <inttypes.h>
#include <stdio.h>
@@ -46,6 +47,7 @@
#define DEV_PREFIX "/dev/"
#define PROC_IDE_FORMAT "/proc/ide/%s/media"
+#define IONICE_PATH "/usr/bin/ionice"
enum hb_ctl_action {
HB_ACTION_UNKNOWN,
@@ -53,12 +55,14 @@ enum hb_ctl_action {
HB_ACTION_START,
HB_ACTION_STOP,
HB_ACTION_REFINFO,
+ HB_ACTION_IONICE,
};
struct hb_ctl_options {
enum hb_ctl_action action;
char *dev_str;
char *uuid_str;
+ int io_prio;
};
@@ -289,6 +293,40 @@ static errcode_t start_heartbeat(struct
return err;
}
+static errcode_t adjust_priority(struct hb_ctl_options *hbo)
+{
+ int ret, child_status;
+ pid_t hb_pid, child_pid;
+ char level_arg[16], pid_arg[16];
+
+ if (access (IONICE_PATH, X_OK) != 0)
+ return OCFS2_ET_NO_IONICE;
+
+ ret = o2cb_get_hb_thread_pid (NULL, hbo->uuid_str, &hb_pid);
+ if (ret != 0)
+ return ret;
+
+ child_pid = fork ();
+ if (child_pid == 0) {
+ sprintf (level_arg, "-n%d", hbo->io_prio);
+ sprintf (pid_arg, "-p%d", hb_pid);
+ execlp (IONICE_PATH, "ionice", "-c1", level_arg, pid_arg, NULL);
+
+ ret = errno;
+ exit (ret);
+ } else if (child_pid > 0) {
+ ret = waitpid (child_pid, &child_status, 0);
+ if (ret == 0)
+ ret = WEXITSTATUS(child_status);
+ else
+ ret = errno;
+ } else {
+ ret = errno;
+ }
+
+ return ret;
+}
+
static errcode_t stop_heartbeat(struct hb_ctl_options *hbo)
{
errcode_t err;
@@ -317,7 +355,7 @@ static int read_options(int argc, char *
ret = 0;
while(1) {
- c = getopt(argc, argv, "ISKd:u:h");
+ c = getopt(argc, argv, "ISKPd:u:n:h");
if (c == -1)
break;
@@ -334,6 +372,10 @@ static int read_options(int argc, char *
hbo->action = HB_ACTION_START;
break;
+ case 'P':
+ hbo->action = HB_ACTION_IONICE;
+ break;
+
case 'd':
if (optarg)
hbo->dev_str = strdup(optarg);
@@ -344,6 +386,11 @@ static int read_options(int argc, char *
hbo->uuid_str = strdup(optarg);
break;
+ case 'n':
+ if (optarg)
+ hbo->io_prio = atoi(optarg);
+ break;
+
case 'I':
hbo->action = HB_ACTION_REFINFO;
break;
@@ -385,6 +432,14 @@ static int process_options(struct hb_ctl
ret = -EINVAL;
break;
+ case HB_ACTION_IONICE:
+ /* ionice needs uuid and priority */
+ if ((hbo->uuid_str && hbo->dev_str) ||
+ (!hbo->uuid_str && !hbo->dev_str) ||
+ hbo->io_prio < 0 || hbo->io_prio > 7)
+ ret = -EINVAL;
+ break;
+
case HB_ACTION_UNKNOWN:
ret = -EINVAL;
break;
@@ -407,6 +462,8 @@ static void print_usage(int err)
fprintf(output, " %s -K -u <uuid>\n", progname);
fprintf(output, " %s -I -d <device>\n", progname);
fprintf(output, " %s -I -u <uuid>\n", progname);
+ fprintf(output, " %s -P -d <device> [-n <io_priority>]\n", progname);
+ fprintf(output, " %s -P -u <uuid> [-n <io_priority>]\n", progname);
fprintf(output, " %s -h\n", progname);
}
@@ -414,7 +471,7 @@ int main(int argc, char **argv)
{
errcode_t err = 0;
int ret = 0;
- struct hb_ctl_options hbo = { HB_ACTION_UNKNOWN, NULL, NULL };
+ struct hb_ctl_options hbo = { HB_ACTION_UNKNOWN, NULL, NULL, 0 };
char hbuuid[33];
setbuf(stdout, NULL);
@@ -483,6 +540,12 @@ int main(int argc, char **argv)
}
break;
+ case HB_ACTION_IONICE:
+ err = adjust_priority(&hbo);
+ if (err)
+ ret = err;
+ break;
+
case HB_ACTION_REFINFO:
err = print_hb_ref_info(&hbo);
if (err) {
More information about the Ocfs2-devel
mailing list