staging/lustre: Move /proc/fs/lustre root level files to sysfs

PATCH In review [email protected] 2 participants 3 messages May 3, 2015

Subsystems (2)

Staging Kernel janitors

lore

Share & tools

lore

[email protected]

author

2015-05-03 03:10 UTC #0 lore raw

PATCH 2/3 staging/lustre: Move /proc/fs/lustre root level files to sysfs

From: Oleg Drokin <[email protected]>

except devices, for now.

Signed-off-by Oleg Drokin <[email protected]>

---
 .../lustre/lustre/obdclass/linux/linux-module.c    | 116 +++++++++++++--------
 1 file changed, 72 insertions(+), 44 deletions(-)

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +72 ~~-44~~

index 06944b8..abfb671 100644
@@ -64,6 +64,7 @@
 #include <linux/uaccess.h>
 #include <linux/miscdevice.h>
 #include <linux/seq_file.h>
+#include <linux/kobject.h>
 
 #include "../../../include/linux/libcfs/libcfs.h"
 #include "../../../include/linux/lnet/lnetctl.h"

@@ -216,29 +217,26 @@ struct miscdevice obd_psdev = {
 };
 
 
-#if defined (CONFIG_PROC_FS)
-static int obd_proc_version_seq_show(struct seq_file *m, void *v)
+static ssize_t version_show(struct kobject *kobj, char *buf)
 {
-	seq_printf(m, "lustre: %s\nkernel: %s\nbuild:  %s\n",
-		   LUSTRE_VERSION_STRING, "patchless_client", BUILD_VERSION);
-	return 0;
+	return snprintf(buf, PAGE_SIZE, "lustre: %s\nkernel: %s\nbuild:  %s\n",
+			LUSTRE_VERSION_STRING, "patchless_client",
+			BUILD_VERSION);
 }
-LPROC_SEQ_FOPS_RO(obd_proc_version);
 
-int obd_proc_pinger_seq_show(struct seq_file *m, void *v)
+static ssize_t pinger_show(struct kobject *kobj, char *buf)
 {
-	seq_printf(m, "%s\n", "on");
-	return 0;
+	return snprintf(buf, PAGE_SIZE, "%s\n", "on");
 }
-LPROC_SEQ_FOPS_RO(obd_proc_pinger);
 
-static int obd_proc_health_seq_show(struct seq_file *m, void *v)
+static ssize_t health_show(struct kobject *kobj, char *buf)
 {
 	bool healthy = true;
 	int i;
+	size_t offset = 0;
 
 	if (libcfs_catastrophe)
-		seq_printf(m, "LBUG\n");
+		offset = snprintf(buf, PAGE_SIZE, "LBUG\n");
 
 	read_lock(&obd_dev_lock);
 	for (i = 0; i < class_devno_max(); i++) {

@@ -256,8 +254,9 @@ static int obd_proc_health_seq_show(struct seq_file *m, void *v)
 		read_unlock(&obd_dev_lock);
 
 		if (obd_health_check(NULL, obd)) {
-			seq_printf(m, "device %s reported unhealthy\n",
-				   obd->obd_name);
+			offset += snprintf(buf + offset, PAGE_SIZE - offset,
+					   "device %s reported unhealthy\n",
+					   obd->obd_name);
 			healthy = false;
 		}
 		class_decref(obd, __func__, current);

@@ -266,32 +265,30 @@ static int obd_proc_health_seq_show(struct seq_file *m, void *v)
 	read_unlock(&obd_dev_lock);
 
 	if (healthy)
-		seq_puts(m, "healthy\n");
+		offset += snprintf(buf + offset, PAGE_SIZE - offset,
+				   "healthy\n");
 	else
-		seq_puts(m, "NOT HEALTHY\n");
+		offset += snprintf(buf + offset, PAGE_SIZE - offset,
+				   "NOT HEALTHY\n");
 
-	return 0;
+	return offset;
 }
-LPROC_SEQ_FOPS_RO(obd_proc_health);
 
-static int obd_proc_jobid_var_seq_show(struct seq_file *m, void *v)
+static ssize_t jobid_var_show(struct kobject *kobj, char *buf)
 {
-	seq_printf(m, "%s\n", obd_jobid_var);
-	return 0;
+	return snprintf(buf, PAGE_SIZE, "%s\n", obd_jobid_var);
 }
 
-static ssize_t obd_proc_jobid_var_seq_write(struct file *file,
-				const char __user *buffer,
-				size_t count, loff_t *off)
+static ssize_t jobid_var_store(struct kobject *kobj,
+			       const char *buffer,
+			       size_t count)
 {
 	if (!count || count > JOBSTATS_JOBID_VAR_MAX_LEN)
 		return -EINVAL;
 
 	memset(obd_jobid_var, 0, JOBSTATS_JOBID_VAR_MAX_LEN + 1);
 
-	/* This might leave the var invalid on error, which is probably fine.*/
-	if (copy_from_user(obd_jobid_var, buffer, count))
-		return -EFAULT;
+	memcpy(obd_jobid_var, buffer, count);
 
 	/* Trim the trailing '\n' if any */
 	if (obd_jobid_var[count - 1] = '\n')

@@ -299,23 +296,20 @@ static ssize_t obd_proc_jobid_var_seq_write(struct file *file,
 
 	return count;
 }
-LPROC_SEQ_FOPS(obd_proc_jobid_var);
 
-static int obd_proc_jobid_name_seq_show(struct seq_file *m, void *v)
+static ssize_t jobid_name_show(struct kobject *kobj, char *buf)
 {
-	seq_printf(m, "%s\n", obd_jobid_var);
-	return 0;
+	return snprintf(buf, PAGE_SIZE, "%s\n", obd_jobid_node);
 }
 
-static ssize_t obd_proc_jobid_name_seq_write(struct file *file,
-					     const char __user *buffer,
-					     size_t count, loff_t *off)
+static ssize_t jobid_name_store(struct kobject *kobj,
+				const char *buffer,
+				size_t count)
 {
 	if (!count || count > JOBSTATS_JOBID_SIZE)
 		return -EINVAL;
 
-	if (copy_from_user(obd_jobid_node, buffer, count))
-		return -EFAULT;
+	memcpy(obd_jobid_node, buffer, count);
 
 	obd_jobid_node[count] = 0;

@@ -325,22 +319,31 @@ static ssize_t obd_proc_jobid_name_seq_write(struct file *file,
 
 	return count;
 }
-LPROC_SEQ_FOPS(obd_proc_jobid_name);
 
+#if defined(CONFIG_PROC_FS)
 /* Root for /proc/fs/lustre */
 struct proc_dir_entry *proc_lustre_root = NULL;
 EXPORT_SYMBOL(proc_lustre_root);
 
 struct lprocfs_vars lprocfs_base[] = {
-	{ "version", &obd_proc_version_fops },
-	{ "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},
 	{ NULL }
 };
 
+LUSTRE_RO_ATTR(version);
+LUSTRE_RO_ATTR(pinger);
+LUSTRE_RO_ATTR(health);
+LUSTRE_RW_ATTR(jobid_var);
+LUSTRE_RW_ATTR(jobid_name);
+
+static struct attribute *lustre_attrs[] = {
+	LUSTRE_ATTR_LIST(version),
+	LUSTRE_ATTR_LIST(pinger),
+	LUSTRE_ATTR_LIST(health),
+	LUSTRE_ATTR_LIST(jobid_name),
+	LUSTRE_ATTR_LIST(jobid_var),
+	NULL,
+};
+
 static void *obd_device_list_seq_start(struct seq_file *p, loff_t *pos)
 {
 	if (*pos >= class_devno_max())

@@ -419,15 +422,36 @@ struct file_operations obd_device_list_fops = {
 	.release = seq_release,
 };
 
+struct kobject lustre_kobj;
+EXPORT_SYMBOL(lustre_kobj);
+
+static DECLARE_COMPLETION(lustre_kobj_unregister);
+static void lustre_sysfs_release(struct kobject *kobj)
+{
+	complete(&lustre_kobj_unregister);
+}
+
+static struct kobj_type lustre_fsroot_ktype = {
+	.default_attrs	= lustre_attrs,
+	.sysfs_ops	= &lustre_sysfs_ops,
+	.release	= lustre_sysfs_release,
+};
+
 int class_procfs_init(void)
 {
 	int rc = 0;
 
+	rc = kobject_init_and_add(&lustre_kobj, &lustre_fsroot_ktype,
+				  fs_kobj, "lustre");
+	if (rc)
+		goto out;
+
 	proc_lustre_root = lprocfs_register("fs/lustre", NULL,
-					    lprocfs_base, NULL);
+					    NULL, NULL);
 	if (IS_ERR(proc_lustre_root)) {
 		rc = PTR_ERR(proc_lustre_root);
 		proc_lustre_root = NULL;
+		kobject_put(&lustre_kobj);
 		goto out;
 	}

@@ -444,6 +468,10 @@ int class_procfs_clean(void)
 	if (proc_lustre_root) {
 		lprocfs_remove(&proc_lustre_root);
 	}
+
+	kobject_put(&lustre_kobj);
+	wait_for_completion(&lustre_kobj_unregister);
+
 	return 0;
 }
 #endif /* CONFIG_PROC_FS */

Signature · 1 line

2.1.0

Greg Kroah-Hartman

maintainer

2015-05-03 18:13 UTC #1 lore raw

Quoted 7 lines · On Sat, May 02, 2015 at 11:10:07PM -0400, [email protected] wrote:

From: Oleg Drokin <[email protected]>
except devices, for now.
Signed-off-by Oleg Drokin <[email protected]>
---
 .../lustre/lustre/obdclass/linux/linux-module.c    | 116 +++++++++++++--------

I need Documentation/ABI/ files for all of these sysfs files. Please put them in the drivers/staging/lustre/ directory for now.

Quoted 16 lines

 1 file changed, 72 insertions(+), 44 deletions(-)

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +1 -0

index 06944b8..abfb671 100644
@@ -64,6 +64,7 @@
 #include <linux/uaccess.h>
 #include <linux/miscdevice.h>
 #include <linux/seq_file.h>
+#include <linux/kobject.h>
 
 #include "../../../include/linux/libcfs/libcfs.h"
 #include "../../../include/linux/lnet/lnetctl.h"

@@ -216,29 +217,26 @@ struct miscdevice obd_psdev = {
 };

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c struct miscdevice obd_psdev = { · line 222

Quoted 10 lines

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +2 -5

 
 
-#if defined (CONFIG_PROC_FS)
-static int obd_proc_version_seq_show(struct seq_file *m, void *v)
+static ssize_t version_show(struct kobject *kobj, char *buf)
 {
-	seq_printf(m, "lustre: %s\nkernel: %s\nbuild:  %s\n",
-		   LUSTRE_VERSION_STRING, "patchless_client", BUILD_VERSION);
-	return 0;
+	return snprintf(buf, PAGE_SIZE, "lustre: %s\nkernel: %s\nbuild:  %s\n",

If you are using sysfs, you never need snprintf() as you had better only be sending something smaller than a page, or your code is wrong.

As it is here, again, please sysfs is one-value-per-file.

Yes, in some places we mess up and don't do that, but as long as I'm reviewing stuff, you better stick to that.

And you don't need the kernel version here, or the build version, as all of that is obvious from other api calls (i.e. uname).

So this file isn't needed at all.

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c struct miscdevice obd_psdev = { · line 230

Quoted 12 lines

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +4 -4

+			LUSTRE_VERSION_STRING, "patchless_client",
+			BUILD_VERSION);
 }
-LPROC_SEQ_FOPS_RO(obd_proc_version);
 
-int obd_proc_pinger_seq_show(struct seq_file *m, void *v)
+static ssize_t pinger_show(struct kobject *kobj, char *buf)
 {
-	seq_printf(m, "%s\n", "on");
-	return 0;
+	return snprintf(buf, PAGE_SIZE, "%s\n", "on");
 }

If it can never be a different value, why have this file at all?

Quoted 5 lines

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +1 -2

-LPROC_SEQ_FOPS_RO(obd_proc_pinger);
 
-static int obd_proc_health_seq_show(struct seq_file *m, void *v)
+static ssize_t health_show(struct kobject *kobj, char *buf)
 {

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c struct miscdevice obd_psdev = { · line 242

Quoted 10 lines

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +2 -1

 	bool healthy = true;
 	int i;
+	size_t offset = 0;
 
 	if (libcfs_catastrophe)
-		seq_printf(m, "LBUG\n");
+		offset = snprintf(buf, PAGE_SIZE, "LBUG\n");
 
 	read_lock(&obd_dev_lock);
 	for (i = 0; i < class_devno_max(); i++) {

This is not a single value per file.

Are you sure you shouldn't just be using debugfs for some of these? You can do whatever you want in debugfs. As long as you can still run if debugfs isn't enabled.

Quoted 102 lines

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +21 ~~-29~~

@@ -256,8 +254,9 @@ static int obd_proc_health_seq_show(struct seq_file *m, void *v)
 		read_unlock(&obd_dev_lock);
 
 		if (obd_health_check(NULL, obd)) {
-			seq_printf(m, "device %s reported unhealthy\n",
-				   obd->obd_name);
+			offset += snprintf(buf + offset, PAGE_SIZE - offset,
+					   "device %s reported unhealthy\n",
+					   obd->obd_name);
 			healthy = false;
 		}
 		class_decref(obd, __func__, current);

@@ -266,32 +265,30 @@ static int obd_proc_health_seq_show(struct seq_file *m, void *v)
 	read_unlock(&obd_dev_lock);
 
 	if (healthy)
-		seq_puts(m, "healthy\n");
+		offset += snprintf(buf + offset, PAGE_SIZE - offset,
+				   "healthy\n");
 	else
-		seq_puts(m, "NOT HEALTHY\n");
+		offset += snprintf(buf + offset, PAGE_SIZE - offset,
+				   "NOT HEALTHY\n");
 
-	return 0;
+	return offset;
 }
-LPROC_SEQ_FOPS_RO(obd_proc_health);
 
-static int obd_proc_jobid_var_seq_show(struct seq_file *m, void *v)
+static ssize_t jobid_var_show(struct kobject *kobj, char *buf)
 {
-	seq_printf(m, "%s\n", obd_jobid_var);
-	return 0;
+	return snprintf(buf, PAGE_SIZE, "%s\n", obd_jobid_var);
 }
 
-static ssize_t obd_proc_jobid_var_seq_write(struct file *file,
-				const char __user *buffer,
-				size_t count, loff_t *off)
+static ssize_t jobid_var_store(struct kobject *kobj,
+			       const char *buffer,
+			       size_t count)
 {
 	if (!count || count > JOBSTATS_JOBID_VAR_MAX_LEN)
 		return -EINVAL;
 
 	memset(obd_jobid_var, 0, JOBSTATS_JOBID_VAR_MAX_LEN + 1);
 
-	/* This might leave the var invalid on error, which is probably fine.*/
-	if (copy_from_user(obd_jobid_var, buffer, count))
-		return -EFAULT;
+	memcpy(obd_jobid_var, buffer, count);
 
 	/* Trim the trailing '\n' if any */
 	if (obd_jobid_var[count - 1] = '\n')

@@ -299,23 +296,20 @@ static ssize_t obd_proc_jobid_var_seq_write(struct file *file,
 
 	return count;
 }
-LPROC_SEQ_FOPS(obd_proc_jobid_var);
 
-static int obd_proc_jobid_name_seq_show(struct seq_file *m, void *v)
+static ssize_t jobid_name_show(struct kobject *kobj, char *buf)
 {
-	seq_printf(m, "%s\n", obd_jobid_var);
-	return 0;
+	return snprintf(buf, PAGE_SIZE, "%s\n", obd_jobid_node);
 }
 
-static ssize_t obd_proc_jobid_name_seq_write(struct file *file,
-					     const char __user *buffer,
-					     size_t count, loff_t *off)
+static ssize_t jobid_name_store(struct kobject *kobj,
+				const char *buffer,
+				size_t count)
 {
 	if (!count || count > JOBSTATS_JOBID_SIZE)
 		return -EINVAL;
 
-	if (copy_from_user(obd_jobid_node, buffer, count))
-		return -EFAULT;
+	memcpy(obd_jobid_node, buffer, count);
 
 	obd_jobid_node[count] = 0;

@@ -325,22 +319,31 @@ static ssize_t obd_proc_jobid_name_seq_write(struct file *file,
 
 	return count;
 }
-LPROC_SEQ_FOPS(obd_proc_jobid_name);
 
+#if defined(CONFIG_PROC_FS)
 /* Root for /proc/fs/lustre */
 struct proc_dir_entry *proc_lustre_root = NULL;
 EXPORT_SYMBOL(proc_lustre_root);
 
 struct lprocfs_vars lprocfs_base[] = {
-	{ "version", &obd_proc_version_fops },
-	{ "pinger", &obd_proc_pinger_fops },
-	{ "health_check", &obd_proc_health_fops },
-	{ "jobid_var", &obd_proc_jobid_var_fops },

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c static ssize_t obd_proc_jobid_name_seq_write(struct file *file, · line 336

Quoted 10 lines

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +5 -2

-	{ .name =	"jobid_name",
-	  .fops =	&obd_proc_jobid_name_fops},
 	{ NULL }
 };
 
+LUSTRE_RO_ATTR(version);
+LUSTRE_RO_ATTR(pinger);
+LUSTRE_RO_ATTR(health);
+LUSTRE_RW_ATTR(jobid_var);
+LUSTRE_RW_ATTR(jobid_name);

Are these static? If not, why not?

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c static ssize_t obd_proc_jobid_name_seq_write(struct file *file, · line 343

Quoted 7 lines

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +7 -0

+
+static struct attribute *lustre_attrs[] = {
+	LUSTRE_ATTR_LIST(version),
+	LUSTRE_ATTR_LIST(pinger),
+	LUSTRE_ATTR_LIST(health),
+	LUSTRE_ATTR_LIST(jobid_name),
+	LUSTRE_ATTR_LIST(jobid_var),

Do you really need this type of macro? Just spell the structures out.

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c struct file_operations obd_device_list_fops = { · line 426

Quoted 12 lines

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +5 -0

+	NULL,
+};
+
 static void *obd_device_list_seq_start(struct seq_file *p, loff_t *pos)
 {
 	if (*pos >= class_devno_max())

@@ -419,15 +422,36 @@ struct file_operations obd_device_list_fops = {
 	.release = seq_release,
 };
 
+struct kobject lustre_kobj;
+EXPORT_SYMBOL(lustre_kobj);

EXPORT_SYMBOL_GPL?

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c struct file_operations obd_device_list_fops = { · line 431

Quoted 5 lines

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +5 -0

+
+static DECLARE_COMPLETION(lustre_kobj_unregister);
+static void lustre_sysfs_release(struct kobject *kobj)
+{
+	complete(&lustre_kobj_unregister);

Not good, you should just clean up and go, this means something is really wrong, and you should NEVER have a static kobject. Which you do. Which is wrong.

Quoted 28 lines

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +15 -1

+}
+
+static struct kobj_type lustre_fsroot_ktype = {
+	.default_attrs	= lustre_attrs,
+	.sysfs_ops	= &lustre_sysfs_ops,
+	.release	= lustre_sysfs_release,
+};
+
 int class_procfs_init(void)
 {
 	int rc = 0;
 
+	rc = kobject_init_and_add(&lustre_kobj, &lustre_fsroot_ktype,
+				  fs_kobj, "lustre");
+	if (rc)
+		goto out;
+
 	proc_lustre_root = lprocfs_register("fs/lustre", NULL,
-					    lprocfs_base, NULL);
+					    NULL, NULL);
 	if (IS_ERR(proc_lustre_root)) {
 		rc = PTR_ERR(proc_lustre_root);
 		proc_lustre_root = NULL;
+		kobject_put(&lustre_kobj);
 		goto out;
 	}

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c int class_procfs_clean(void) · line 473

Quoted 6 lines

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +3 -0

@@ -444,6 +468,10 @@ int class_procfs_clean(void)
 	if (proc_lustre_root) {
 		lprocfs_remove(&proc_lustre_root);
 	}
+
+	kobject_put(&lustre_kobj);
+	wait_for_completion(&lustre_kobj_unregister);

Do you like to just sit and wait for forever? Because that is what will happen. Don't do that.

thanks,

greg k-h

Oleg Drokin

author

2015-05-03 20:03 UTC #2 lore raw

In reply to [email protected]

Quoted 12 lines · Greg Kroah-Hartman

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c struct miscdevice obd_psdev = { · line 222
Quoted 9 lines
drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +1 -2
-#if defined (CONFIG_PROC_FS)
-static int obd_proc_version_seq_show(struct seq_file *m, void *v)
+static ssize_t version_show(struct kobject *kobj, char *buf)
{
drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +1 -3
-	seq_printf(m, "lustre: %s\nkernel: %s\nbuild:  %s\n",
-		   LUSTRE_VERSION_STRING, "patchless_client", BUILD_VERSION);
-	return 0;
+	return snprintf(buf, PAGE_SIZE, "lustre: %s\nkernel: %s\nbuild:  %s\n",
If you are using sysfs, you never need snprintf() as you had better only be sending something smaller than a page, or your code is wrong.

Well, I am copying straight from ext4 here (and in other places), that I thought was a good model.
Is there a better model?
I guess I can convert to sprintfs in most of places, though.

Quoted 7 lines

As it is here, again, please sysfs is one-value-per-file.
Yes, in some places we mess up and don't do that, but as long as I'm reviewing stuff, you better stick to that.
And you don't need the kernel version here, or the build version, as all of that is obvious from other api calls (i.e. uname).

That would be true if only people did not apply patches to their kernels. I guess we don't care all that much about kernel version here as it could be obtained from uname,
but lustre version is kind of important for the case that people patch it up (and they do), so can I leave just that?

Quoted 30 lines

So this file isn't needed at all.
Quoted 11 lines
drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +2 -0
+			LUSTRE_VERSION_STRING, "patchless_client",
+			BUILD_VERSION);
}
drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +1 -2
-LPROC_SEQ_FOPS_RO(obd_proc_version);
 
-int obd_proc_pinger_seq_show(struct seq_file *m, void *v)
+static ssize_t pinger_show(struct kobject *kobj, char *buf)
{
drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +1 -2
-	seq_printf(m, "%s\n", "on");
-	return 0;
+	return snprintf(buf, PAGE_SIZE, "%s\n", "on");
Quoted 1 line
}
If it can never be a different value, why have this file at all?
Quoted 13 lines
drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +1 -2
-LPROC_SEQ_FOPS_RO(obd_proc_pinger);
 
-static int obd_proc_health_seq_show(struct seq_file *m, void *v)
+static ssize_t health_show(struct kobject *kobj, char *buf)
{
	bool healthy = true;
	int i;
drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +1 -0
+	size_t offset = 0;
if (libcfs_catastrophe)
drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +1 -1
-		seq_printf(m, "LBUG\n");
+		offset = snprintf(buf, PAGE_SIZE, "LBUG\n");

Quoted 8 lines

Quoted 2 lines
	read_lock(&obd_dev_lock);
	for (i = 0; i < class_devno_max(); i++) {
This is not a single value per file.
Are you sure you shouldn't just be using debugfs for some of these? You can do whatever you want in debugfs. As long as you can still run if debugfs isn't enabled.

I am definitely going to use debugfs in some cases.
This particular one I guess we'll need to split into per-obd health status and the overall "system health" that could be easily queried.

Quoted 15 lines

Quoted 15 lines
+#if defined(CONFIG_PROC_FS)
/* Root for /proc/fs/lustre */
struct proc_dir_entry *proc_lustre_root = NULL;
EXPORT_SYMBOL(proc_lustre_root);
struct lprocfs_vars lprocfs_base[] = {
drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +0 -6
-	{ "version", &obd_proc_version_fops },
-	{ "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},
	{ NULL }
};

Quoted 7 lines

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c static ssize_t obd_proc_jobid_name_seq_write(struct file *file, · line 336
Quoted 5 lines
drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +5 -0
+LUSTRE_RO_ATTR(version);
+LUSTRE_RO_ATTR(pinger);
+LUSTRE_RO_ATTR(health);
+LUSTRE_RW_ATTR(jobid_var);
+LUSTRE_RW_ATTR(jobid_name);
Are these static? If not, why not?

Yes, they are static.

Quoted 8 lines

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c static ssize_t obd_proc_jobid_name_seq_write(struct file *file, · line 343
Quoted 6 lines
drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +6 -0
+static struct attribute *lustre_attrs[] = {
+	LUSTRE_ATTR_LIST(version),
+	LUSTRE_ATTR_LIST(pinger),
+	LUSTRE_ATTR_LIST(health),
+	LUSTRE_ATTR_LIST(jobid_name),
+	LUSTRE_ATTR_LIST(jobid_var),
Do you really need this type of macro? Just spell the structures out.

Again, I am going by the fine example of ext4 here.
But I am happy to spell it out if you think it's better that way.

Quoted 16 lines

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c struct file_operations obd_device_list_fops = { · line 426
Quoted 12 lines
drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +3 -0
+	NULL,
+};
+
static void *obd_device_list_seq_start(struct seq_file *p, loff_t *pos)
{
	if (*pos >= class_devno_max())
drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +2 -0
@@ -419,15 +422,36 @@ struct file_operations obd_device_list_fops = {
 	.release = seq_release,
 };
 
+struct kobject lustre_kobj;
+EXPORT_SYMBOL(lustre_kobj);
EXPORT_SYMBOL_GPL?

Quoted 9 lines

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c struct file_operations obd_device_list_fops = { · line 431
Quoted 5 lines
drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +5 -0
+
+static DECLARE_COMPLETION(lustre_kobj_unregister);
+static void lustre_sysfs_release(struct kobject *kobj)
+{
+	complete(&lustre_kobj_unregister);
Not good, you should just clean up and go, this means something is really wrong, and you should NEVER have a static kobject. Which you do. Which is wrong.

This is how we tell the cleanup code that we are all done, though? Otherwise if I do kobject_put there and move on (to module unload), how all the other potential users of my kobject would be protected from the code disappearign from under them?

I can dynamically allocate lustre_kobj, but that does not really help this particular scenario.

Quoted 27 lines

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +15 -1

+}
+
+static struct kobj_type lustre_fsroot_ktype = {
+	.default_attrs	= lustre_attrs,
+	.sysfs_ops	= &lustre_sysfs_ops,
+	.release	= lustre_sysfs_release,
+};
+
 int class_procfs_init(void)
 {
 	int rc = 0;
 
+	rc = kobject_init_and_add(&lustre_kobj, &lustre_fsroot_ktype,
+				  fs_kobj, "lustre");
+	if (rc)
+		goto out;
+
 	proc_lustre_root = lprocfs_register("fs/lustre", NULL,
-					    lprocfs_base, NULL);
+					    NULL, NULL);
 	if (IS_ERR(proc_lustre_root)) {
 		rc = PTR_ERR(proc_lustre_root);
 		proc_lustre_root = NULL;
+		kobject_put(&lustre_kobj);
 		goto out;
 	}

Quoted 10 lines

drivers/staging/lustre/lustre/obdclass/linux/linux-module.c int class_procfs_clean(void) · line 473
Quoted 7 lines
drivers/staging/lustre/lustre/obdclass/linux/linux-module.c +3 -0
@@ -444,6 +468,10 @@ int class_procfs_clean(void)
 	if (proc_lustre_root) {
 		lprocfs_remove(&proc_lustre_root);
 	}
+
+	kobject_put(&lustre_kobj);
+	wait_for_completion(&lustre_kobj_unregister);
Do you like to just sit and wait for forever? Because that is what will happen. Don't do that.

ext4 does just that, so I imagined there was some significance here.

Also, before submitting these patches, I did test them. This does work (and why would it not, anyway? We call complete from lustre_sysfs_release above).

In fact cpufreq (the other code I am looking at for examples) does the same thing (Well, they do a bit more stuff in their .release method - they print a debug message ;).

Thanks!

Bye,
Oleg