Re: [PATCH 1/4] mtd: Add sysfs attr to expose ECC stats

[Brian Norris]

+ Greg KH, LKML

For context:

http://thread.gmane.org/gmane.linux.drivers.mtd/52517/focus=52518

On Tue, Apr 15, 2014 at 11:13:52AM +0000, Pekon Gupta wrote:
> >From: Ezequiel Garcia
> >>On Mar 27, Gupta, Pekon wrote:
> >> >
> >> >+static ssize_t mtd_ecc_stats_show(struct device *dev,
> >> >+		struct device_attribute *attr, char *buf)
> >> >+{
> >> >+	struct mtd_info *mtd = dev_get_drvdata(dev);
> >> >+	struct mtd_ecc_stats *ecc_stats = &mtd->ecc_stats;
> >> >+
> >> >+	return snprintf(buf, PAGE_SIZE, "%8u %8u %8u %8u\n", ecc_stats->corrected,
> >> >+						ecc_stats->failed,
> >> >+						ecc_stats->badblocks,
> >> >+						ecc_stats->bbtblocks);
> >> Though I would have liked to see each field of ecc_stats as a separate sysfs entry
> >
> >I tried to match the ecc_stats structure exactly in the sysfs entry. Keep
> >in mind creating/keeping a sysfs file has its cost. Since it's trivial
> >to get one of the fields with any text parsing tool I didn't think having
> >a file per stat was worth it.

I'm not sure what kind of cost this has these days. I thought there was
some work to reduce the required kobject memory usage for each new sysfs
file.

> There are some guidelines about attributes in 'Documentation/filesystems/sysfs.txt'
> Though it's acceptable to put array of values of the "same type" in single sysfs file,
> But I'm still not confident on having all members of 'struct ecc_stats' being
> represented by single sysfs file
[...]

I agree, it looks like the sysfs policy would recommend against putting
distinct properties in the same file.

I'm not sure if /sys/block/<disk>/stat is a good example, as it does
violate this policy. It also seems to have some historical baggage.

But there is potentially one good reason for putting this distinct
information in a single file: if the information must be returned
atomically. For disk stats, it might be important to get a consistent
snapshot of the disk stats (or nearly so, with minimal locking
overhead), which might change significantly between file accesses if
we're doing half a dozen file queries instead.

This same reason may not apply to these ECC stats, since none of these
ECC stats are likely to be changing concurrently.

So I personally might lean toward "one file per attribute" here.

> >It won't impact as they are two completely different things. The ecc_stats
> >output is only "vaguely inspired" in block stats; but these are two completely
> >different stats. It's not about tool reusability, but about consistency.
> >
> >> Also, How about printing what these numbers mean ?

This sounds like an argument from the /proc days. Not sure if we want to
repeat that in /sys.

> >> I hope this will still keep it machine readable.
> >Well, this is not a debugfs entry, so I'm not sure we want to add such debug
> >information. Anyone can take a look at the code and see what ecc_stats mean.

The code or (as suggested by Pekon) the documentation
(Documentation/ABI/). Either way -- with a single 'ecc_stats' table, or
with 4 separate files -- they need to be documented.

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/