On Fri, Feb 06, 2015 at 04:59:16AM -0800, Greg KH wrote: > On Fri, Feb 06, 2015 at 12:20:53AM +0000, Seymour, Shane M wrote: > > The current patch that implements tape statistics is here: > > > > http://marc.info/?l=linux-scsi&m=142112067313723&w=2 > > Aside from the "do we want to do this all in a single file" issue that I > will say more on below, this patch has issues. Please don't use a > kobject for _ANYTHING_ in sysfs that has a struct device as a parent. > If you do that, it can't be seen by userspace tools very well, if at > all. I can't speak for Shane but wouldn't spend too much time looking at the current v2 patch: it's the result of a pretty ugly compromise suggested on linux-scsi. This thread was really to try to settle the discussion on the structure of the stats files. > > Recently there was was another discussion here about one file vs a collection of files for tape statistics: > > > > http://marc.info/?l=linux-scsi&m=142316255501550&w=2 > > > > The result was that I should ask here what method I should use. I > > would like to get feedback in relation to tape statistics and one file > > vs multi-file in sysfs. I'm happy to keep the existing code or change > > to a single file approach. > > One of the primary reasons we created sysfs and the "one value per file" > rule is that multi-value files just do not work well. Yes, you get an > atomic snapshot, and you save some open/read/close syscall roundtrips, > but you do so at the expense of forcing userspace to "know" what the > format of the file is. And once you create it, you can NEVER CHANGE IT > AGAIN. I am not convinced this is a concern for tape statistics: they are pretty much a solved problem. The commercial *nixes have had this for decades. Likewise for disk stats: although fluff like maj:min/name etc. has been shuffled a few times the basic fields have remained unchanged for a very long time and sysfs already removes the need to include an identity field. > Yes, that's right, if you come up with some new statistic in the future, > or realize that one of the ones you have now is wrong, you can't change > it, you have to make a whole new file, otherwise you could break > userspace tools. I understand the fact that you can't change them; I just don't think it's a big problem in this specific case (and much less than some of the more imaginative sysfs content - 2d int arrays with column headers anyone?). > And yes, open/read/close does take take a few extra cycles, but you > can't really measure it for a virtual filesystem like this on any modern > system. I'll try to get some numbers when I get back home next week - Shane is talking about use cases involving tens of thousands of tape devices. I am not certain that the overhead would be unmeasurable in that case: the additional context switching & TLB flushes alone seem like they would add up. > Hope this helps explain why we have the sysfs rule, and why you should > continue to follow it as well. > > Yes, it's not always followed, but that's usually because people forgot > why we had this rule, and no one noticed or pointed it out to me that it > was wrong. Perhaps sysfs.txt should be updated to make the position more clear? The current wording seems rather more liberal than this thread would suggest. Maybe something like the patch below? This would help people who are trying to dtrt by reading the documentation. Regards, Bryn. From 3081aad4cc4d19b68f39499dbeb3837f0642f70e Mon Sep 17 00:00:00 2001 From: "Bryn M. Reeves" <bmr@xxxxxxxxxx> Date: Fri, 6 Feb 2015 15:19:39 +0000 Subject: [PATCH] docs/sysfs: Specify array valued attribute review requirements Although the linux-api position that one-value-per-file is a strong rule is very clear in mailing list discussions the sysfs.txt documentation suggests a rather more liberal stance: "... it is socially acceptable to express an array of values of the same type." Fix the documentation to make it clear that such uses should be discussed on linux-api first. Signed-off-by: Bryn M. Reeves <bmr@xxxxxxxxxx> --- Documentation/filesystems/sysfs.txt | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/Documentation/filesystems/sysfs.txt b/Documentation/filesystems/sysfs.txt index b35a64b..494fa78 100644 --- a/Documentation/filesystems/sysfs.txt +++ b/Documentation/filesystems/sysfs.txt @@ -57,8 +57,15 @@ attributes. Attributes should be ASCII text files, preferably with only one value per file. It is noted that it may not be efficient to contain only one -value per file, so it is socially acceptable to express an array of -values of the same type. +value per file, so it may be socially acceptable to express an array of +values of the same type. + +If you are considering adding such an array attribute to sysfs please +discuss it via the linux-api mailing list first to ensure that your +proposed use is acceptable: + + https://www.kernel.org/doc/man-pages/linux-api-ml.html + linux-api@xxxxxxxxxxxxxxx Mixing types, expressing multiple lines of data, and doing fancy formatting of data is heavily frowned upon. Doing these things may get -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
