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

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

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

[Ezequiel Garcia]

On 12 May 05:50 PM, Brian Norris wrote:
> > 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.
> 

OK...

> 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.
> 

Right.

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

Yup, no problem. Greg, if you can confirm this it'd be great.

> > >> 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.
> 

Sure, will document in next round.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Greg Kroah-Hartman]

On Mon, May 12, 2014 at 11:26:14PM -0300, Ezequiel Garcia wrote:
> On 12 May 05:50 PM, Brian Norris wrote:
> > > 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.
> > 
> 
> OK...
> 
> > 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.
> > 
> 
> Right.
> 
> > So I personally might lean toward "one file per attribute" here.
> > 
> 
> Yup, no problem. Greg, if you can confirm this it'd be great.

Yes, that is the rule for sysfs, please do not put multiple values in a
single sysfs file for a variety of good reasons.

> > > >> 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.
> > 
> 
> Sure, will document in next round.

This documentation is required, thanks.

greg k-h

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

[Ezequiel Garcia]

On 13 May 01:15 PM, Greg Kroah-Hartman wrote:
> On Mon, May 12, 2014 at 11:26:14PM -0300, Ezequiel Garcia wrote:
> > On 12 May 05:50 PM, Brian Norris wrote:
> > > > 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.
> > > 
> > 
> > OK...
> > 
> > > 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.
> > > 
> > 
> > Right.
> > 
> > > So I personally might lean toward "one file per attribute" here.
> > > 
> > 
> > Yup, no problem. Greg, if you can confirm this it'd be great.
> 
> Yes, that is the rule for sysfs, please do not put multiple values in a
> single sysfs file for a variety of good reasons.
> 

Will do!

> > > > >> 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.
> > > 
> > 
> > Sure, will document in next round.
> 
> This documentation is required, thanks.
> 

Sure, no problem.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Ezequiel Garcia]

On 12 May 11:26 PM, Ezequiel Garcia wrote:
> On 12 May 05:50 PM, Brian Norris wrote:
> > > 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.
> > 
[..]
> > So I personally might lean toward "one file per attribute" here.
> > 

Brian,

Having agreed on doing one file per attribute, I'm now not sure how to
name them. Maybe you can give me a hand?

Let me add some context: these are the per-MTD partition fields of the
mtd_ecc_stats struct, although two of them aren't related to ECC, but to
the bad blocks management. This is the struct:

struct mtd_ecc_stats {
        __u32 corrected;
        __u32 failed;
        __u32 badblocks;
        __u32 bbtblocks;
};

How about the following?

* corrected_bits
* uncorrectable_errors
* badblocks
* bbtblocks
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com