| Message ID | 1531823419-8665-1-git-send-email-perfn@axis.com |
|---|---|
| State | Superseded |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | UBI: Add volume read and write statistics | expand |
Am Dienstag, 17. Juli 2018, 12:30:19 CEST schrieb Per Forlin: > Simple read and write statistics. > * Bytes read > * Bytes written > * Number of reads > * Number of writes > > This is useful to find out how the storage is being utilized. > For block devices this already exists via /proc/diskstats. > The intention of this patch is to add similar stats > for UBI as well. Why on UBI level and not MTD? Thanks, //richard
> To: Per Förlin > Cc: linux-mtd@lists.infradead.org; Artem Bityutskiy; Per Förlin > Subject: Re: [PATCH] UBI: Add volume read and write statistics > > Am Dienstag, 17. Juli 2018, 12:30:19 CEST schrieb Per Forlin: > > Simple read and write statistics. > > * Bytes read > > * Bytes written > > * Number of reads > > * Number of writes > > > > This is useful to find out how the storage is being utilized. > > For block devices this already exists via /proc/diskstats. > > The intention of this patch is to add similar stats > > for UBI as well. > > Why on UBI level and not MTD? In my case I wanted to evaluate the performance per volume. I have one MTD device with several UBI volumes. It would be sufficient to have it on an MTD level to see the overall storage usage. This would still be very helpful for me. Having it on an MTD level is of course more general. I wouldn't mind changing the patch to add the stats in mtdcore for mtd_read() and mtd_write() In case of MTD block devices the stats will be somewhat redundant with /proc/diskstats. Do you think I should update the patch to add MTD stats instead?
Per, Am Dienstag, 17. Juli 2018, 14:08:51 CEST schrieb Per Förlin: > > To: Per Förlin > > Cc: linux-mtd@lists.infradead.org; Artem Bityutskiy; Per Förlin > > Subject: Re: [PATCH] UBI: Add volume read and write statistics > > > > Am Dienstag, 17. Juli 2018, 12:30:19 CEST schrieb Per Forlin: > > > Simple read and write statistics. > > > * Bytes read > > > * Bytes written > > > * Number of reads > > > * Number of writes > > > > > > This is useful to find out how the storage is being utilized. > > > For block devices this already exists via /proc/diskstats. > > > The intention of this patch is to add similar stats > > > for UBI as well. > > > > Why on UBI level and not MTD? > In my case I wanted to evaluate the performance per volume. I have one MTD > device with several UBI volumes. It would be sufficient to have it on > an MTD level to see the overall storage usage. > This would still be very helpful for me. > > Having it on an MTD level is of course more general. > I wouldn't mind changing the patch to add the stats in mtdcore > for mtd_read() and mtd_write() > > In case of MTD block devices the stats will be somewhat redundant with > /proc/diskstats. > > Do you think I should update the patch to add MTD stats instead? While having a cup of coffee I thought more about this. Actually both, MTD and UBI makes sense. The most important issue is that you integrate it with the existing diskstats. So instead of having our own interface feeding MTD/UBI stats into diskstats would be nice. Did you look into that? I'm not sure how much work this would be. That way users can use existing tools such as iostat... Thanks, //richard
Thanks Richard for your feedback, > ________________________________________ > From: Richard Weinberger <richard@nod.at> > Sent: Tuesday, July 17, 2018 4:34 PM > To: Per Förlin > Cc: linux-mtd@lists.infradead.org; Artem Bityutskiy > Subject: Re: [PATCH] UBI: Add volume read and write statistics > > Per, > > Am Dienstag, 17. Juli 2018, 14:08:51 CEST schrieb Per Förlin: > > > To: Per Förlin > > > Cc: linux-mtd@lists.infradead.org; Artem Bityutskiy; Per Förlin > > > Subject: Re: [PATCH] UBI: Add volume read and write statistics > > > > > > Am Dienstag, 17. Juli 2018, 12:30:19 CEST schrieb Per Forlin: > > > > Simple read and write statistics. > > > > * Bytes read > > > > * Bytes written > > > > * Number of reads > > > > * Number of writes > > > > > > > > This is useful to find out how the storage is being utilized. > > > > For block devices this already exists via /proc/diskstats. > > > > The intention of this patch is to add similar stats > > > > for UBI as well. > > > > > > Why on UBI level and not MTD? > > In my case I wanted to evaluate the performance per volume. I have one MTD > > device with several UBI volumes. It would be sufficient to have it on > > an MTD level to see the overall storage usage. > > This would still be very helpful for me. > > > > Having it on an MTD level is of course more general. > > I wouldn't mind changing the patch to add the stats in mtdcore > > for mtd_read() and mtd_write() > > > > In case of MTD block devices the stats will be somewhat redundant with > > /proc/diskstats. > > > > Do you think I should update the patch to add MTD stats instead? > > While having a cup of coffee I thought more about this. > Actually both, MTD and UBI makes sense. > The most important issue is that you integrate it with the existing diskstats. > So instead of having our own interface feeding MTD/UBI stats into diskstats > would be nice. Did you look into that? I'm not sure how much work this would be. > That way users can use existing tools such as iostat... I actually started out looking for the information under diskstats, then I learned it's only for block devices. I took a quick glance at it before I went for the sys implementation instead. diskstats is separated from the MTD and UBI stuff and I don't know if one can make a connection to MTD/UBI somehow. I will take a closer look at this.
On Tue, Jul 17, 2018 at 8:01 AM Per Förlin <per.forlin@axis.com> wrote: > > Thanks Richard for your feedback, > > > ________________________________________ > > From: Richard Weinberger <richard@nod.at> > > Sent: Tuesday, July 17, 2018 4:34 PM > > To: Per Förlin > > Cc: linux-mtd@lists.infradead.org; Artem Bityutskiy > > Subject: Re: [PATCH] UBI: Add volume read and write statistics > > > > Per, > > > > Am Dienstag, 17. Juli 2018, 14:08:51 CEST schrieb Per Förlin: > > > > To: Per Förlin > > > > Cc: linux-mtd@lists.infradead.org; Artem Bityutskiy; Per Förlin > > > > Subject: Re: [PATCH] UBI: Add volume read and write statistics > > > > > > > > Am Dienstag, 17. Juli 2018, 12:30:19 CEST schrieb Per Forlin: > > > > > Simple read and write statistics. > > > > > * Bytes read > > > > > * Bytes written > > > > > * Number of reads > > > > > * Number of writes > > > > > > > > > > This is useful to find out how the storage is being utilized. > > > > > For block devices this already exists via /proc/diskstats. > > > > > The intention of this patch is to add similar stats > > > > > for UBI as well. > > > > > > > > Why on UBI level and not MTD? > > > In my case I wanted to evaluate the performance per volume. I have one MTD > > > device with several UBI volumes. It would be sufficient to have it on > > > an MTD level to see the overall storage usage. > > > This would still be very helpful for me. > > > > > > Having it on an MTD level is of course more general. > > > I wouldn't mind changing the patch to add the stats in mtdcore > > > for mtd_read() and mtd_write() > > > > > > In case of MTD block devices the stats will be somewhat redundant with > > > /proc/diskstats. > > > > > > Do you think I should update the patch to add MTD stats instead? > > > > While having a cup of coffee I thought more about this. > > Actually both, MTD and UBI makes sense. > > The most important issue is that you integrate it with the existing diskstats. > > So instead of having our own interface feeding MTD/UBI stats into diskstats > > would be nice. Did you look into that? I'm not sure how much work this would be. > > That way users can use existing tools such as iostat... > I actually started out looking for the information under diskstats, > then I learned it's only for block devices. I took a quick glance at > it before I went for the sys implementation instead. diskstats is > separated from the MTD and UBI stuff and I don't know if one can make a > connection to MTD/UBI somehow. I will take a closer look at this. Perhaps it was "only for block devices" because no one ever implemented the necessary hooks in MTD or UBI? I don't know the history, nor the information you found, just making a stab in the dark. If UBI and/or MTD can provide the statistics that diskstats needs in a interpretation that makes sense, why not go that way? - Steve
Am Dienstag, 17. Juli 2018, 17:06:12 CEST schrieb Steve deRosier: > > > While having a cup of coffee I thought more about this. > > > Actually both, MTD and UBI makes sense. > > > The most important issue is that you integrate it with the existing diskstats. > > > So instead of having our own interface feeding MTD/UBI stats into diskstats > > > would be nice. Did you look into that? I'm not sure how much work this would be. > > > That way users can use existing tools such as iostat... > > I actually started out looking for the information under diskstats, > > then I learned it's only for block devices. I took a quick glance at > > it before I went for the sys implementation instead. diskstats is > > separated from the MTD and UBI stuff and I don't know if one can make a > > connection to MTD/UBI somehow. I will take a closer look at this. > > Perhaps it was "only for block devices" because no one ever > implemented the necessary hooks in MTD or UBI? I don't know the > history, nor the information you found, just making a stab in the > dark. > > If UBI and/or MTD can provide the statistics that diskstats needs in a > interpretation that makes sense, why not go that way? Yeah, that's what I have in mind. Maybe we can easily teach diskstats about MTD. Thanks, //richard
> ________________________________________ > From: Richard Weinberger <richard@nod.at> > Sent: Tuesday, July 17, 2018 5:10 PM > To: Steve deRosier > Cc: Per Förlin; linux-mtd@lists.infradead.org; Artem Bityutskiy > Subject: Re: [PATCH] UBI: Add volume read and write statistics > > Am Dienstag, 17. Juli 2018, 17:06:12 CEST schrieb Steve deRosier: > > > > While having a cup of coffee I thought more about this. > > > > Actually both, MTD and UBI makes sense. > > > > The most important issue is that you integrate it with the existing diskstats. > > > > So instead of having our own interface feeding MTD/UBI stats into diskstats > > > > would be nice. Did you look into that? I'm not sure how much work this would be. > > > > That way users can use existing tools such as iostat... > > > I actually started out looking for the information under diskstats, > > > then I learned it's only for block devices. I took a quick glance at > > > it before I went for the sys implementation instead. diskstats is > > > separated from the MTD and UBI stuff and I don't know if one can make a > > > connection to MTD/UBI somehow. I will take a closer look at this. > > > > Perhaps it was "only for block devices" because no one ever > > implemented the necessary hooks in MTD or UBI? I don't know the > > history, nor the information you found, just making a stab in the > > dark. > > > > If UBI and/or MTD can provide the statistics that diskstats needs in a > > interpretation that makes sense, why not go that way? > > Yeah, that's what I have in mind. Maybe we can easily teach diskstats > about MTD. Now I got the chance to look at this again. It's not as bad as I thought. The diskstats simply takes the block class and iterate over all devices. For each device statistics are printed out. It should be possible to do the same for the MTD class and the UBI class. So far I haven't tested anything just reading code. Then it's a matter of taste. Is MTD and UBI stats welcome in genhd.c? I will try to find time the coming days or so to make a prove of concept implementation to get a better idea of how big the change would be.
> > Am Dienstag, 17. Juli 2018, 17:06:12 CEST schrieb Steve deRosier: > > > > > While having a cup of coffee I thought more about this. > > > > > Actually both, MTD and UBI makes sense. > > > > > The most important issue is that you integrate it with the existing diskstats. > > > > > So instead of having our own interface feeding MTD/UBI stats into diskstats > > > > > would be nice. Did you look into that? I'm not sure how much work this would be. > > > > > That way users can use existing tools such as iostat... > > > > I actually started out looking for the information under diskstats, > > > > then I learned it's only for block devices. I took a quick glance at > > > > it before I went for the sys implementation instead. diskstats is > > > > separated from the MTD and UBI stuff and I don't know if one can make a > > > > connection to MTD/UBI somehow. I will take a closer look at this. > > > > > > Perhaps it was "only for block devices" because no one ever > > > implemented the necessary hooks in MTD or UBI? I don't know the > > > history, nor the information you found, just making a stab in the > > > dark. > > > > > > If UBI and/or MTD can provide the statistics that diskstats needs in a > > > interpretation that makes sense, why not go that way? > > > > Yeah, that's what I have in mind. Maybe we can easily teach diskstats > > about MTD. > Now I got the chance to look at this again. It's not as bad as I thought. > The diskstats simply takes the block class and iterate over all devices. > For each device statistics are printed out. > It should be possible to do the same for the MTD class and the UBI class. > So far I haven't tested anything just reading code. > Then it's a matter of taste. Is MTD and UBI stats welcome in genhd.c? > > I will try to find time the coming days or so to make a prove of concept > implementation to get a better idea of how big the change would be. I made one observation. The stats in disktats also exists under /sys/class/block for every device. Actually it's the stat under /sys that is also available in diskstats. For instance: cat /sys/class/block/sda/stat 220 86 10682 830 14 4 128 110 0 680 940 This format is documented in iostats.txt The first step could be to only add it under /sys/class/ubi and /sys/class/mtd. I'm not sure how to add all these metrics my patch only adds 4 of them. "Unsupported values" could be set to 0 for now. I will continue to think about how to integrate with diskstats but I'm also in favor of adding this support in incremental steps.
diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c index b98481b..8d708c4 100644 --- a/drivers/mtd/ubi/eba.c +++ b/drivers/mtd/ubi/eba.c @@ -618,6 +618,9 @@ int ubi_eba_read_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum, if (err) return err; + vol->stats.rcount++; + vol->stats.rbytes += len; + pnum = vol->eba_tbl->entries[lnum].pnum; if (pnum >= 0) { err = check_mapping(ubi, vol, lnum, &pnum); @@ -1032,6 +1035,9 @@ int ubi_eba_write_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum, if (err) return err; + vol->stats.wcount++; + vol->stats.wbytes += len; + pnum = vol->eba_tbl->entries[lnum].pnum; if (pnum >= 0) { err = check_mapping(ubi, vol, lnum, &pnum); diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h index f5ba97c..08a313fe 100644 --- a/drivers/mtd/ubi/ubi.h +++ b/drivers/mtd/ubi/ubi.h @@ -293,6 +293,23 @@ struct ubi_eba_leb_desc { }; /** + * struct ubi_volume_stats - Volume statistics + * @rbytes: the number of bytes read + * @wbytes: the number of bytes written + * @rcount: the number of read requests + * @wcount: the number of write requests + * + * This structure contains read and write statistics. + * + */ +struct ubi_volume_stats { + u64 rbytes; + u64 wbytes; + u32 rcount; + u32 wcount; +}; + +/** * struct ubi_volume - UBI volume description data structure. * @dev: device object to make use of the the Linux device model * @cdev: character device object to create character device @@ -384,6 +401,8 @@ struct ubi_volume { #ifdef CONFIG_MTD_UBI_FASTMAP unsigned long *checkmap; #endif + + struct ubi_volume_stats stats; }; /** diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c index 0be5167..021de10 100644 --- a/drivers/mtd/ubi/vmt.c +++ b/drivers/mtd/ubi/vmt.c @@ -51,6 +51,8 @@ static struct device_attribute attr_vol_data_bytes = __ATTR(data_bytes, S_IRUGO, vol_attribute_show, NULL); static struct device_attribute attr_vol_upd_marker = __ATTR(upd_marker, S_IRUGO, vol_attribute_show, NULL); +static struct device_attribute attr_vol_stats = + __ATTR(stats, S_IRUGO, vol_attribute_show, NULL); /* * "Show" method for files in '/<sysfs>/class/ubi/ubiX_Y/'. @@ -107,6 +109,11 @@ static ssize_t vol_attribute_show(struct device *dev, ret = sprintf(buf, "%lld\n", vol->used_bytes); else if (attr == &attr_vol_upd_marker) ret = sprintf(buf, "%d\n", vol->upd_marker); + else if (attr == &attr_vol_stats) + ret = sprintf(buf, "rbytes wbytes rcount wcount:\n" + "%llu %llu %u %u\n", + vol->stats.rbytes, vol->stats.wbytes, + vol->stats.rcount, vol->stats.wcount); else /* This must be a bug */ ret = -EINVAL; @@ -129,6 +136,7 @@ static struct attribute *volume_dev_attrs[] = { &attr_vol_usable_eb_size.attr, &attr_vol_data_bytes.attr, &attr_vol_upd_marker.attr, + &attr_vol_stats.attr, NULL }; ATTRIBUTE_GROUPS(volume_dev);