Fix accounting error for pending sync IO operations in zpool iostat

[MigeljanImeri]

Motivation and Context

Currently, when using zpool iostat, the pending sync IO operations are not shown properly, rather either a 1 or a 0 is displayed. This is due to vdev_queue_class_length checking whether the list is empty, rather then checking the length of the the list. This issue was noticed when doing performance evaluations with the O_DIRECT PR (#10018), as all IO's are issued with sync priority through the VDEV queues.

Description

I added a counter variable to vdev_queue_t to keep track of the pending sync IO operations and changed vdev_queue_class_length to now reference this variable.

How Has This Been Tested?

The changes were tested by using fio to generate sync IO operations. Without these changes the pending sync IO operations showed either 1 or 0, while with the changes the pending sync IO operations showed as expected.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[amotin]

While it is kind of nice from point of being consistent with AVL-trees, lists are used in tons of places, there it would be a waste of 8 bytes per list, plus a dirty cache line for most of modifications, and all of it just for (yes, useful, but) cosmetic issue. I wonder how often that stat is required and whether it would be cheaper to count the elements (may be not, just a thought).

I could feel some better if in process we'd also remove list_size from field there, used on Linux and FreeBSD only for a single assertion (I wonder if it was used on Illumos by debugger). And/or if we use it for something else, may be we could balance ARC and dbuf cache evictions using it or something like that.

[bwatkinson]

While it is kind of nice from point of being consistent with AVL-trees, lists are used in tons of places, there it would be a waste of 8 bytes per list, plus a dirty cache line for most of modifications, and all of it just for (yes, useful, but) cosmetic issue. I wonder how often that stat is required and whether it would be cheaper to count the elements (may be not, just a thought).

I could feel some better if in process we'd also remove list_size from field there, used on Linux and FreeBSD only for a single assertion (I wonder if it was used on Illumos by debugger). And/or if we use it for something else, may be we could balance ARC and dbuf cache evictions using it or something like that.

Part of the reason for this PR was doing investigations with small recordsize performance with the O_DIRECT PR (#10018). I agree that an unfortunate side effect of this would be inflating such a commonly used data structure by 8 bytes. It would make more sense if the length could also be leveraged in other parts of the code. Another alternative is this counter could just as easily be placed in the vdev_queue_t struct and handled entirely in vdev_queue.c. Do you see that as a preferred option?

[behlendorf]

Another alternative is this counter could just as easily be placed in the vdev_queue_t struct and handled entirely in vdev_queue.c. Do you see that as a preferred option?

Placing the length in the vdev_queue_t seems preferable to me in this particular instance to keep the list_t as lean and lightweight as possible.

[MigeljanImeri]

Another alternative is this counter could just as easily be placed in the vdev_queue_t struct and handled entirely in vdev_queue.c. Do you see that as a preferred option?

Placing the length in the vdev_queue_t seems preferable to me in this particular instance to keep the list_t as lean and lightweight as possible.

I have moved the counter to vdev_queue_t and have also changed the type to ulong_t.

[amotin]

I'd be OK about it, just we do not need a counter for every priority level, only for those that use lists. We could fit it into current size would we do:

diff --git a/sys/contrib/openzfs/include/sys/vdev_impl.h b/sys/contrib/openzfs/include/sys/vdev_impl.h
index ad9dc3aefd8e..f2cc4d7e53f2 100644
--- a/sys/contrib/openzfs/include/sys/vdev_impl.h
+++ b/sys/contrib/openzfs/include/sys/vdev_impl.h
@@ -131,7 +131,10 @@ typedef const struct vdev_ops {
  * Virtual device properties
  */
 typedef union vdev_queue_class {
-       list_t          vqc_list;
+       struct {
+               ulong_t         vqc_list_numnodes;
+               list_t          vqc_list;
+       };
        avl_tree_t      vqc_tree;
 } vdev_queue_class_t;
 

, combined with list_size removal I mentioned.

[MigeljanImeri]

I'd be OK about it, just we do not need a counter for every priority level, only for those that use lists. We could fit it into current size would we do:

diff --git a/sys/contrib/openzfs/include/sys/vdev_impl.h b/sys/contrib/openzfs/include/sys/vdev_impl.h
index ad9dc3aefd8e..f2cc4d7e53f2 100644
--- a/sys/contrib/openzfs/include/sys/vdev_impl.h
+++ b/sys/contrib/openzfs/include/sys/vdev_impl.h
@@ -131,7 +131,10 @@ typedef const struct vdev_ops {
  * Virtual device properties
  */
 typedef union vdev_queue_class {
-       list_t          vqc_list;
+       struct {
+               ulong_t         vqc_list_numnodes;
+               list_t          vqc_list;
+       };
        avl_tree_t      vqc_tree;
 } vdev_queue_class_t;
 

, combined with list_size removal I mentioned.

Okay, I see that makes sense. I have gone ahead and made those changes.

[amotin]

Thanks, but I didn't mean list_size removal in the same commit. It could be separate for readability.

Besides similar struct avl_tree we could possibly leave it in user-space for some libzfs ABI compatibility. I haven't checked, but I guess it may be used somewhere in public structures. Or we could remove them both at one time.

[MigeljanImeri]

Okay, I see I've removed the list_size removal from the commit.

[amotin]

Thanks. Will you open a separate PR for that, or you'd like me to?

[MigeljanImeri]

Thanks. Will you open a separate PR for that, or you'd like me to?

I can open a separate PR, should I keep list_size in the user space list or remove it from both?

[amotin]

I can open a separate PR, should I keep list_size in the user space list or remove it from both?

I've tried to search ZFS libraries for lists use and found only implementation in libspl, nothing else. But just to be safer I think we could do it as two commits in one PR, one only for kernel and one for user-space. This way the first could be merged into 2.2, while the second remain in master till 2.3. @behlendorf what do you think?

[behlendorf]

Agreed. Let's keep these changes in two separate commits to make sure we don't break the ABI. We might have code which embeds a list_t in some public structure. Although on initial grep I don't see anything obvious.