Skip to contentSkip to conversation

staging/lustre: Replace jobid acquiring with per node setting

PATCH In review Oleg Drokin 2 participants 6 messages Apr 28, 2014 – May 4, 2014 Staging

lore
Share & tools
lore

Oleg Drokin

author
#0 lore raw

PATCH 1/1 staging/lustre: Replace jobid acquiring with per node setting

Insted of meddling directly in process environment variables (which is also not possible on certain platforms due to not exported symbols), create jobid_name proc file to represent this info (to be filled by job scheduler epilogue).

Signed-off-by Oleg Drokin <[email protected]>
CC Andreas Dilger <[email protected]>
---
 .../staging/lustre/include/linux/libcfs/curproc.h  |   1 -
 .../staging/lustre/lustre/include/lprocfs_status.h |   1 +
 drivers/staging/lustre/lustre/include/obd_class.h  |   3 +
 .../lustre/lustre/libcfs/linux/linux-curproc.c     | 152 ---------------------
 drivers/staging/lustre/lustre/obdclass/class_obd.c |  50 ++-----
 .../lustre/lustre/obdclass/linux/linux-module.c    |  27 ++++
 6 files changed, 44 insertions(+), 190 deletions(-)
drivers/staging/lustre/include/linux/libcfs/curproc.h +0 -1
index 8fd47c9..b314f34 100644
@@ -56,7 +56,6 @@
 /* check if task is running in compat mode.*/
 #define current_pid()		(current->pid)
 #define current_comm()		(current->comm)
-int cfs_get_environ(const char *key, char *value, int *val_len);
 
 typedef __u32 cfs_cap_t;
drivers/staging/lustre/lustre/include/lprocfs_status.h +1 -0
index 9ce12c7..1b7f6a9 100644
@@ -369,6 +369,7 @@ static inline void s2dhms(struct dhms *ts, time_t secs)
 #define JOBSTATS_JOBID_VAR_MAX_LEN	20
 #define JOBSTATS_DISABLE		"disable"
 #define JOBSTATS_PROCNAME_UID		"procname_uid"
+#define JOBSTATS_NODELOCAL		"nodelocal"
 
 extern int lprocfs_write_frac_helper(const char *buffer, unsigned long count,
 				     int *val, int mult);
drivers/staging/lustre/lustre/include/obd_class.h +3 -0
index 61ba370..e265820 100644
@@ -2182,6 +2182,9 @@ void class_exit_uuidlist(void);
 int mea_name2idx(struct lmv_stripe_md *mea, const char *name, int namelen);
 int raw_name2idx(int hashtype, int count, const char *name, int namelen);
 
+/* class_obd.c */
+extern char obd_jobid_node[];
+
 /* prng.c */
 #define ll_generate_random_uuid(uuid_out) cfs_get_random_bytes(uuid_out, sizeof(class_uuid_t))
drivers/staging/lustre/lustre/libcfs/linux/linux-curproc.c +0 -152

158 lines not on this page · raw message

drivers/staging/lustre/lustre/obdclass/class_obd.c +13 -37

67 lines not on this page · raw message

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +27 -0
index 0334882..bdf2eed 100644
@@ -292,6 +292,31 @@ static ssize_t obd_proc_jobid_var_seq_write(struct file *file, const char *buffe
 }
 LPROC_SEQ_FOPS(obd_proc_jobid_var);
 
+static int obd_proc_jobid_name_seq_show(struct seq_file *m, void *v)
+{
+	return seq_printf(m, "%s\n", obd_jobid_var);
+}
+
+static ssize_t obd_proc_jobid_name_seq_write(struct file *file,
+					     const char __user *buffer,
+					     size_t count, loff_t *off)
+{
+	if (!count || count > JOBSTATS_JOBID_SIZE)
+		return -EINVAL;
+
+	if (copy_from_user(obd_jobid_node, buffer, count))
+		return -EFAULT;
+
+	obd_jobid_node[count] = 0;
+
+	/* Trim the trailing '\n' if any */
+	if (obd_jobid_node[count - 1] == '\n')
+		obd_jobid_node[count - 1] = 0;
+
+	return count;
+}
+LPROC_SEQ_FOPS(obd_proc_jobid_name);
+
 /* Root for /proc/fs/lustre */
 struct proc_dir_entry *proc_lustre_root = NULL;
 EXPORT_SYMBOL(proc_lustre_root);

@@ -301,6 +326,8 @@ struct lprocfs_vars lprocfs_base[] = {
 	{ "pinger", &obd_proc_pinger_fops },
 	{ "health_check", &obd_proc_health_fops },
 	{ "jobid_var", &obd_proc_jobid_var_fops },
+	{ .name =	"jobid_name",
+	  .fops =	&obd_proc_jobid_name_fops},
 	{ 0 }
 };
Signature · 1 line
1.9.0
maintainer
#1 lore raw
Quoted 4 lines · Oleg Drokin

Insted of meddling directly in process environment variables (which is also not possible on certain platforms due to not exported symbols), create jobid_name proc file to represent this info (to be filled by job scheduler epilogue).

That looks better, but what are you going to do about getting rid of all of these proc files? Just move them all at once? I really don't like the idea of adding new ones, and want a plan for what you all are going to do here moving forward.

I'll take this for now, as it is better than what you are doing with the environment variables...

thanks,

greg k-h

author
#2 lore raw
Quoted 9 lines · Greg Kroah-Hartman
Quoted 4 lines · Oleg Drokin

Insted of meddling directly in process environment variables (which is also not possible on certain platforms due to not exported symbols), create jobid_name proc file to represent this info (to be filled by job scheduler epilogue).

That looks better, but what are you going to do about getting rid of all of these proc files? Just move them all at once? I really don't like the idea of adding new ones, and want a plan for what you all are going to do here moving forward.

What proc files are to referring to, if I may ask?
I don't think I saw complaints about proc files, the complaints I saw were mostly about reading env variables directly and the like so that was the focus of this patch. Did I miss some side discussion? Any pointers?

Thanks!

Bye,
Oleg

maintainer
#3 lore raw
Quoted 8 lines · Oleg Drokin
Quoted 5 lines · Greg Kroah-Hartman
Quoted 4 lines · Oleg Drokin

Insted of meddling directly in process environment variables (which is also not possible on certain platforms due to not exported symbols), create jobid_name proc file to represent this info (to be filled by job scheduler epilogue).

Quoted 6 lines
Quoted 4 lines

That looks better, but what are you going to do about getting rid of all of these proc files? Just move them all at once? I really don't like the idea of adding new ones, and want a plan for what you all are going to do here moving forward.

What proc files are to referring to, if I may ask?

All of them :)

Quoted 3 lines

I don't think I saw complaints about proc files, the complaints I saw were mostly about reading env variables directly and the like so that was the focus of this patch. Did I miss some side discussion? Any pointers?

No, no side discussion, the proc files need to be removed / fixed before the code can be merged to the "proper" part of the kernel tree.

thanks,

greg k-h

author
#4 lore raw

Hello!

Quoted 5 lines · Greg Kroah-Hartman
Quoted 3 lines

I don't think I saw complaints about proc files, the complaints I saw were mostly about reading env variables directly and the like so that was the focus of this patch. Did I miss some side discussion? Any pointers?

No, no side discussion, the proc files need to be removed / fixed before the code can be merged to the "proper" part of the kernel tree.

So, what's broken about them then?
It's not like there are no files in proc or that lustre-proc files are causing some sort of breakage (at least not anymore after that recent patch).

So some guidance would be really appreciated.

Bye,
Oleg

maintainer
#5 lore raw
Quoted 6 lines · Oleg Drokin

Hello!

Quoted 3 lines · Greg Kroah-Hartman
Quoted 3 lines

I don't think I saw complaints about proc files, the complaints I saw were mostly about reading env variables directly and the like so that was the focus of this patch. Did I miss some side discussion? Any pointers?

Quoted 6 lines
Quoted 2 lines

No, no side discussion, the proc files need to be removed / fixed before the code can be merged to the "proper" part of the kernel tree.

So, what's broken about them then?
It's not like there are no files in proc or that lustre-proc files are causing some sort of breakage (at least not anymore after that recent patch).

The rule of not adding new proc files that do not relate to processes, has been around for over a decade now, since 2.6.0 came out.

Please use sysfs instead, you can put your files in /sys/fs/lustre/ and keep them to "one-value-per-file" files and document them in Documentation/ABI/

thanks,

greg k-h