[developer] Re: [openzfs/openzfs] 9647 Introduce ZFS Read/Write kstats (#664)
This is not ready to go yet. I still haven't updated the code with the restructuring from ZoL. There is a kstat memory leak that I need to narrow down in the new code before updating this. Sorry for the delay. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/664#issuecomment-426351265 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/Ta167a88bbab5c009-Me25b2d91d7d659ccb8d2c920 Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
[developer] Re: [openzfs/openzfs] 9647 Introduce ZFS Read/Write kstats (#664)
@sdimitro can you rebase this and then we'll get it integrated. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/664#issuecomment-425963430 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/Ta167a88bbab5c009-M982734f14b31fdc74a07403e Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
[developer] Re: [openzfs/openzfs] 9647 Introduce ZFS Read/Write kstats (#664)
Looks like the ZoL commit was merged. Let's move forward with this. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/664#issuecomment-416244239 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/Ta167a88bbab5c009-M4f1fa242275bbdf73ed9b2b3 Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
[developer] Re: [openzfs/openzfs] 9647 Introduce ZFS Read/Write kstats (#664)
Let's keep a hold on this until the ZoL PR makes it in: https://github.com/zfsonlinux/zfs/pull/7705 -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/664#issuecomment-414166806 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/Ta167a88bbab5c009-M30c7648d65ba650c25fb1b05 Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
[developer] Re: [openzfs/openzfs] 9647 Introduce ZFS Read/Write kstats (#664)
sdimitro commented on this pull request.
> @@ -1217,18 +1352,10 @@ zvol_dumpio(zvol_state_t *zv, void *addr, uint64_t
> offset, uint64_t size,
int
zvol_strategy(buf_t *bp)
{
- zfs_soft_state_t *zs = NULL;
see my other comment (e.g. cleanup).
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/664#discussion_r202529019
--
openzfs: openzfs-developer
Permalink:
https://openzfs.topicbox.com/groups/developer/Ta167a88bbab5c009-M7f0463273753eeeadeb73439
Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9647 Introduce ZFS Read/Write kstats (#664)
sdimitro commented on this pull request.
> @@ -653,43 +654,28 @@ zfs_read(vnode_t *vp, uio_t *uio, int ioflag, cred_t
> *cr, caller_context_t *ct)
static int
zfs_write(vnode_t *vp, uio_t *uio, int ioflag, cred_t *cr, caller_context_t
*ct)
{
- znode_t *zp = VTOZ(vp);
Since I was adding the kstat_update calls in that functions, I figured I'd
refactor the styling here so variables are declared closer to their usage now
that C99 is enabled.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/664#discussion_r202529013
--
openzfs: openzfs-developer
Permalink:
https://openzfs.topicbox.com/groups/developer/Ta167a88bbab5c009-M4a92b85f6ad15d00e281b387
Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9647 Introduce ZFS Read/Write kstats (#664)
allanjude commented on this pull request.
> @@ -653,43 +654,28 @@ zfs_read(vnode_t *vp, uio_t *uio, int ioflag, cred_t
> *cr, caller_context_t *ct)
static int
zfs_write(vnode_t *vp, uio_t *uio, int ioflag, cred_t *cr, caller_context_t
*ct)
{
- znode_t *zp = VTOZ(vp);
Why are you changing the style of the variable declarations here?
> @@ -1217,18 +1352,10 @@ zvol_dumpio(zvol_state_t *zv, void *addr, uint64_t
> offset, uint64_t size,
int
zvol_strategy(buf_t *bp)
{
- zfs_soft_state_t *zs = NULL;
Again the style of the variable declarations seems to change for no reason.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/664#pullrequestreview-137245466
--
openzfs: openzfs-developer
Permalink:
https://openzfs.topicbox.com/groups/developer/Ta167a88bbab5c009-M54f1a841d1fc828b6010c049
Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9647 Introduce ZFS Read/Write kstats (#664)
Ramzec commented on this pull request. > + * There should not be anything wrong with having kstats for +* snapshots. Since we are not sure how useful they would be +* though nor how much their memory overhead would matter in +* a filesystem with many snapshots, we skip them for now. +*/ + if (zfsvfs->z_issnap) + return; + + /* +* We are limited by KSTAT_STRLEN for the kstat's name here +* which is a lot less that the string length of a dataset's +* path. Thus, we distinguish handles by their objset IDs +* prepended by the pool's load_guid. Note that both numbers +* are formatted in hex. See breakdown in comment below. +* +* We use the pool's load_guid because it is guaranteed to Yep, that what I supposed. Would be nice if you put that into the comment that will make it more clear. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/664#discussion_r201450327 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/Ta167a88bbab5c009-Mf5096c9d878b3fd558456f5a Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9647 Introduce ZFS Read/Write kstats (#664)
sdimitro commented on this pull request. > + * There should not be anything wrong with having kstats for +* snapshots. Since we are not sure how useful they would be +* though nor how much their memory overhead would matter in +* a filesystem with many snapshots, we skip them for now. +*/ + if (zfsvfs->z_issnap) + return; + + /* +* We are limited by KSTAT_STRLEN for the kstat's name here +* which is a lot less that the string length of a dataset's +* path. Thus, we distinguish handles by their objset IDs +* prepended by the pool's load_guid. Note that both numbers +* are formatted in hex. See breakdown in comment below. +* +* We use the pool's load_guid because it is guaranteed to Thank you for looking at the review @Ramzec. Yes. The pool-guid can change without re-import (e.g. we do a `zpool reguid `). Thus, if we were using the pool-guild as part of the kstat name, if the pool's guid changed, all these kstats would be left off with the old guid. Those kstats are created when a dataset is mounted and destroyed when the dataset is unmounted. So it makes more sense to use the pool's load_guid as this would keep things consistent. Given the above, recreating all these structures every time the guid changes would be very complicated and unnecessary. Does this make sense? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/664#discussion_r201429988 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/Ta167a88bbab5c009-M9b1057e3bdb1b52c87b97469 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9647 Introduce ZFS Read/Write kstats (#664)
Ramzec commented on this pull request. > + * There should not be anything wrong with having kstats for +* snapshots. Since we are not sure how useful they would be +* though nor how much their memory overhead would matter in +* a filesystem with many snapshots, we skip them for now. +*/ + if (zfsvfs->z_issnap) + return; + + /* +* We are limited by KSTAT_STRLEN for the kstat's name here +* which is a lot less that the string length of a dataset's +* path. Thus, we distinguish handles by their objset IDs +* prepended by the pool's load_guid. Note that both numbers +* are formatted in hex. See breakdown in comment below. +* +* We use the pool's load_guid because it is guaranteed to Do you mean that the pool-guid can be changed without re-import of the corresponding pool, that will require recreate of kstat-structures? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/664#pullrequestreview-135926959 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/Ta167a88bbab5c009-Mfcc83c377b1d5401ea1c9dfd Delivery options: https://openzfs.topicbox.com/groups
