Minor all-round touch-ups #11859

nabijaczleweli · 2021-04-08T09:42:58Z

Motivation and Context

See individual commit messages.

Description

The first patch execs zstream dump from the zstreamdump wrapper instead of spawning it normally.

The second patch fixes zvol_wait when zvols with spaces in their names exist.

The third patch replaces a subprocess.run("cat", "known file").stdio.strip() in arc_summary3 with an open().read().strip()(!)

The fourth patch removes a useless cat in the bpftrace wrapper (and execs bpftrace).

The fifth and sixth patches fix zfs_ids_to_path usage and error messages.

How Has This Been Tested?

Ran it, mostly, I guess?

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. – none apply
I have run the ZFS Test Suite with this change applied. – CI take my hand
All commit messages are properly formatted and contain Signed-off-by.

behlendorf

These all looks pretty reasonable to me, though I didn't manually test them all.

ghost

Looks good to me, too.

pzakha · 2021-04-09T00:54:50Z

cmd/zvol_wait/zvol_wait

-		token=$(echo "$zvol_line" | awk '{print $3}')
-		redacted=$(echo "$zvol_line" | awk '{print $4}')
+	    name,volmode,receive_resume_token,redact_snaps |
+	    while IFS="	" read -r name volmode token redacted; do


nit: might want to add a comment saying that we are using a tab as a separator in case the name has a space in it. because of how things line up, IFS looks to be set to a space here.

fair enough, added a comment at EOL here

IFS=$'\t' instead?

bash extension

pzakha · 2021-04-09T01:00:58Z

cmd/zvol_wait/zvol_wait

+	echo "$zvols" | while read -r zvol; do
 		if [ ! -L "/dev/zvol/$zvol" ]; then


I've tested this and it still doesn't work with zvols that have spaces. This is because the 60-zvol udev rule replaces spaces by the + character, so if we have a zvol named "zvol 1", it will create link /dev/zvol/zvol+1. This is due to udev not accepting spaces (see 5f91bd3).

We should probably do the same replacement here when checking for the /dev link.

uhoh, good catch! updated to check for zvol | tr ' ' '+' instead

Signed-off-by: Ahelenia Ziemiańska <[email protected]>

list_zvols() would happily, for zvols with spaces in their names, assign the second half to volmode, &c., so use a normal read and set IFS to a tab instead of using 4 separate AWK processes(?) Similarly, in filter_out_deleted_zvols(), run zfs(8) once and use the output directly instead of spawning a zfs(8) process per zvol Signed-off-by: Ahelenia Ziemiańska <[email protected]>

Signed-off-by: Ahelenia Ziemiańska <[email protected]>

nabijaczleweli · 2021-04-09T16:13:57Z

Added fix for volmode=default+zvol_volmode=3 to be handled same as volmode=none

list_zvols() would happily, for zvols with spaces in their names, assign the second half to volmode, &c., so use a normal read and set IFS to a tab instead of using 4 separate AWK processes(?) Similarly, in filter_out_deleted_zvols(), run zfs(8) once and use the output directly instead of spawning a zfs(8) process per zvol Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes #11859

Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes #11859

Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11859

list_zvols() would happily, for zvols with spaces in their names, assign the second half to volmode, &c., so use a normal read and set IFS to a tab instead of using 4 separate AWK processes(?) Similarly, in filter_out_deleted_zvols(), run zfs(8) once and use the output directly instead of spawning a zfs(8) process per zvol Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11859

Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11859

list_zvols() would happily, for zvols with spaces in their names, assign the second half to volmode, &c., so use a normal read and set IFS to a tab instead of using 4 separate AWK processes(?) Similarly, in filter_out_deleted_zvols(), run zfs(8) once and use the output directly instead of spawning a zfs(8) process per zvol Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11859

Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11859

behlendorf added the Status: Code Review Needed Ready for review and testing label Apr 8, 2021

behlendorf approved these changes Apr 8, 2021

View reviewed changes

behlendorf requested review from a user and pzakha April 8, 2021 17:12

ghost approved these changes Apr 8, 2021

View reviewed changes

pzakha reviewed Apr 9, 2021

View reviewed changes

nabijaczleweli force-pushed the the-sins-of-the-mother-v1 branch 3 times, most recently from f13eb47 to f5a771c Compare April 9, 2021 02:06

nabijaczleweli added 7 commits April 9, 2021 18:12

zstreamdump: exec zstream dump

9fd63ae

Signed-off-by: Ahelenia Ziemiańska <[email protected]>

arc_summary3: just read /s/m/{mod}/version instead of spawning cat

15e0cc3

Signed-off-by: Ahelenia Ziemiańska <[email protected]>

contrib/bpftrace: exec bpftrace, remove useless cat

e1d37be

Signed-off-by: Ahelenia Ziemiańska <[email protected]>

zfs_ids_to_path: the -v comes after the executable name

f09d392

Signed-off-by: Ahelenia Ziemiańska <[email protected]>

zfs_ids_to_path: print correct wrong values

488e386

Signed-off-by: Ahelenia Ziemiańska <[email protected]>

zvol_wait: properly handle zvol_volmode sysctl being 3/none

bd7f1a8

Signed-off-by: Ahelenia Ziemiańska <[email protected]>

nabijaczleweli force-pushed the the-sins-of-the-mother-v1 branch from f5a771c to bd7f1a8 Compare April 9, 2021 16:13

pzakha approved these changes Apr 9, 2021

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Apr 11, 2021

behlendorf closed this in ea4541e Apr 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Minor all-round touch-ups #11859

Minor all-round touch-ups #11859

Uh oh!

nabijaczleweli commented Apr 8, 2021

Uh oh!

behlendorf left a comment

Uh oh!

ghost left a comment

Uh oh!

pzakha Apr 9, 2021

Uh oh!

nabijaczleweli Apr 9, 2021

Uh oh!

gdevenyi Apr 9, 2021

Uh oh!

nabijaczleweli Apr 10, 2021

Uh oh!

pzakha Apr 9, 2021

Uh oh!

nabijaczleweli Apr 9, 2021 •

edited

Loading

Uh oh!

nabijaczleweli commented Apr 9, 2021 •

edited

Loading

Uh oh!

Uh oh!

		echo "$zvols" \| while read -r zvol; do
		if [ ! -L "/dev/zvol/$zvol" ]; then

Minor all-round touch-ups #11859

Minor all-round touch-ups #11859

Uh oh!

Conversation

nabijaczleweli commented Apr 8, 2021

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

behlendorf left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

pzakha Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

nabijaczleweli Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

gdevenyi Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

nabijaczleweli Apr 10, 2021

Choose a reason for hiding this comment

Uh oh!

pzakha Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

nabijaczleweli Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nabijaczleweli commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nabijaczleweli Apr 9, 2021 •

edited

Loading

nabijaczleweli commented Apr 9, 2021 •

edited

Loading