| Message ID | 3fe54b69910e811ad63b2f0e37bd806e28752e8a.1708772619.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: functional test cases for tempfsid | expand |
On Sat, Feb 24, 2024 at 4:43 PM Anand Jain <anand.jain@oracle.com> wrote: > > check_fsid() provides a method to verify if the given device is mounted > with the tempfsid in the kernel. Function sb() is an internal only > function. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > Reviewed-by: Filipe Manana <fdmanana@suse.com> > --- > v3: > add rb > add _require_btrfs_command inspect-internal dump-super > v2: > Fix typo in the commit log. > Fix array SCRATCH_DEV_POOL_SAVED handling. > > common/btrfs | 37 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/common/btrfs b/common/btrfs > index e1b29c613767..406be9574e32 100644 > --- a/common/btrfs > +++ b/common/btrfs > @@ -792,3 +792,40 @@ _has_btrfs_sysfs_feature_attr() > > test -e /sys/fs/btrfs/features/$feature_attr > } > + > +# Print the fsid and metadata uuid replaced with constant strings FSID and > +# METADATA_UUID. Compare temp_fsid with fsid and metadata_uuid, then echo what > +# it matches to or TEMP_FSID. This helps in comparing with the golden output. > +check_fsid() > +{ > + local dev1=$1 > + local fsid > + local metadata_uuid > + > + _require_btrfs_command inspect-internal dump-super So, as pointed out in the previous version of the patchset, the function should do the require for everything it needs that may not be available. It's doing for the inspect-internal command, but it's missing a: _require_btrfs_sysfs_fsid Instead this is being called for every test case that calls this new helper function, when those requirements should be hidden from the tests themselves. Thanks. > + > + # on disk fsid > + fsid=$($BTRFS_UTIL_PROG inspect-internal dump-super $dev1 | \ > + grep ^fsid | $AWK_PROG -d" " '{print $2}') > + echo -e "On disk fsid:\t\t$fsid" | sed -e "s/$fsid/FSID/g" > + > + # Print FSID even if it is not the same as metadata_uuid because it has > + # to match in the golden output. > + metadata_uuid=$(cat /sys/fs/btrfs/$fsid/metadata_uuid) > + echo -e "Metadata uuid:\t\tFSID" > + > + # This returns the temp_fsid if set > + tempfsid=$(_btrfs_get_fsid $dev1) > + if [[ $tempfsid == $fsid ]]; then > + echo -e "Temp fsid:\t\tFSID" > + elif [[ $tempfsid == $metadata_uuid ]]; then > + # If we are here, it means there is a bug; let it not match with > + # the golden output. > + echo -e "Temp fsid:\t\t$metadata_uuid" > + else > + echo -e "Temp fsid:\t\tTEMPFSID" > + fi > + > + echo -e -n "Tempfsid status:\t" > + cat /sys/fs/btrfs/$tempfsid/temp_fsid > +} > -- > 2.39.3 >
> function should do the require for everything it needs that may not be > available. > It's doing for the inspect-internal command, but it's missing a: > > _require_btrfs_sysfs_fsid Yes, it did. Actually, check_fsid() would need the following to cover all the prerequisites. _require_btrfs_fs_sysfs _require_btrfs_fs_feature temp_fsid _require_btrfs_fs_feature metadata_uuid _require_btrfs_command inspect-internal dump-super I already have v4 with what you just suggested, I am going to send it. > Instead this is being called for every test case that calls this new > helper function, when those requirements should be hidden from the > tests themselves. However, I am a bit skeptical if we should move all prerequisites to the helpers or only some major prerequisites. Because returning _notrun() in the middle of the testcase is something I am not sure is better than at the beginning of the testcase (I do not have a specific example where it is not a good idea, though). And, theoretically, figuring out if the test case would run/_notrun() will be complicated. Next, we shall end up checking the _require..() multiple times in a test case, though one time is enough (the test cases 311, 312, 313 call check_fsid() two times). Furthermore, it will inconsistent, as a lot of command wraps are already missing such a requirement; I'm not sure if we shall ever achieve consistency across fstests (For example: _cp_reflink() missing _require_cp_reflink). Lastly, if there are duplicating prerequisites across the helper functions, then we call _require..() many more times (for example: 313 will call mkfs_clone() and check_fsid() two times, which means we would verify the following three times in a testcase. _require_btrfs_fs_feature metadata_uuid _require_btrfs_command inspect-internal dump-super So, how about prerequisites of the newer functions as comments above the function to be copied into the test case? Thanks, Anand
On Wed, Feb 28, 2024 at 9:36 AM Anand Jain <anand.jain@oracle.com> wrote: > > > > > function should do the require for everything it needs that may not be > > available. > > It's doing for the inspect-internal command, but it's missing a: > > > > _require_btrfs_sysfs_fsid > > > Yes, it did. Actually, check_fsid() would need the following to > cover all the prerequisites. > > _require_btrfs_fs_sysfs > _require_btrfs_fs_feature temp_fsid > _require_btrfs_fs_feature metadata_uuid > _require_btrfs_command inspect-internal dump-super > > > I already have v4 with what you just suggested, I am going to send it. > > > > Instead this is being called for every test case that calls this new > > helper function, when those requirements should be hidden from the > > tests themselves. > > However, I am a bit skeptical if we should move all prerequisites to > the helpers or only some major prerequisites. > > Because returning _notrun() in the middle of the testcase is something > I am not sure is better than at the beginning of the testcase (I do not > have a specific example where it is not a good idea, though). > > And, theoretically, figuring out if the test case would run/_notrun() > will be complicated. > > Next, we shall end up checking the _require..() multiple times in > a test case, though one time is enough (the test cases 311, 312, > 313 call check_fsid() two times). > > Furthermore, it will inconsistent, as a lot of command wraps are > already missing such a requirement; I'm not sure if we shall ever > achieve consistency across fstests (For example: _cp_reflink() > missing _require_cp_reflink). > > Lastly, if there are duplicating prerequisites across the helper > functions, then we call _require..() many more times (for example: > 313 will call mkfs_clone() and check_fsid() two times, which > means we would verify the following three times in a testcase. > > _require_btrfs_fs_feature metadata_uuid > _require_btrfs_command inspect-internal dump-super > > > So, how about prerequisites of the newer functions as comments > above the function to be copied into the test case? Calling the require functions doesn't take that much time, I'm not worried about more 1, 2, 3 or 10 milliseconds of test run time. Now having each test that uses a common function to call all the require functions is hard to maintain and messy. Commenting the requirements on top of each function is not bullet proof - test authors will have to do it and reviewers as well all the time. Not to mention that if a function's implementation changes and now it has different requirements, we'll have to change every single test that uses it. Thanks. > > Thanks, Anand
On 2/28/24 15:58, Filipe Manana wrote: > On Wed, Feb 28, 2024 at 9:36 AM Anand Jain <anand.jain@oracle.com> wrote: >> >> >> >>> function should do the require for everything it needs that may not be >>> available. >>> It's doing for the inspect-internal command, but it's missing a: >>> >>> _require_btrfs_sysfs_fsid >> >> >> Yes, it did. Actually, check_fsid() would need the following to >> cover all the prerequisites. >> >> _require_btrfs_fs_sysfs >> _require_btrfs_fs_feature temp_fsid >> _require_btrfs_fs_feature metadata_uuid >> _require_btrfs_command inspect-internal dump-super >> >> >> I already have v4 with what you just suggested, I am going to send it. >> >> >> > Instead this is being called for every test case that calls this new >> > helper function, when those requirements should be hidden from the >> > tests themselves. >> >> However, I am a bit skeptical if we should move all prerequisites to >> the helpers or only some major prerequisites. >> >> Because returning _notrun() in the middle of the testcase is something >> I am not sure is better than at the beginning of the testcase (I do not >> have a specific example where it is not a good idea, though). >> >> And, theoretically, figuring out if the test case would run/_notrun() >> will be complicated. >> >> Next, we shall end up checking the _require..() multiple times in >> a test case, though one time is enough (the test cases 311, 312, >> 313 call check_fsid() two times). >> >> Furthermore, it will inconsistent, as a lot of command wraps are >> already missing such a requirement; I'm not sure if we shall ever >> achieve consistency across fstests (For example: _cp_reflink() >> missing _require_cp_reflink). >> >> Lastly, if there are duplicating prerequisites across the helper >> functions, then we call _require..() many more times (for example: >> 313 will call mkfs_clone() and check_fsid() two times, which >> means we would verify the following three times in a testcase. >> >> _require_btrfs_fs_feature metadata_uuid >> _require_btrfs_command inspect-internal dump-super >> >> >> So, how about prerequisites of the newer functions as comments >> above the function to be copied into the test case? > > Calling the require functions doesn't take that much time, I'm not > worried about more 1, 2, 3 or 10 milliseconds of test run time. > > Now having each test that uses a common function to call all the > require functions is hard to maintain and messy. > > Commenting the requirements on top of each function is not bullet > proof - test authors will have to do it and reviewers as well all the > time. > Not to mention that if a function's implementation changes and now it > has different requirements, we'll have to change every single test > that uses it. I was trying to keep the code optimized and avoid duplicate '_require..' statements as much as possible. Also, I aimed to avoid '_notrun' in the middle of the testcase, which keeps it inline with the rest of the older testcases. However, it seems not to be a big deal, so let the '_require..' statements be in the helpers. This makes the test case look more concise and further makes it easy to make changes. For example, if a helper is deleted, the testcase will still be fine without bugs. I have updated the rest of the test cases with this idea in v4. Thanks, Anand
diff --git a/common/btrfs b/common/btrfs index e1b29c613767..406be9574e32 100644 --- a/common/btrfs +++ b/common/btrfs @@ -792,3 +792,40 @@ _has_btrfs_sysfs_feature_attr() test -e /sys/fs/btrfs/features/$feature_attr } + +# Print the fsid and metadata uuid replaced with constant strings FSID and +# METADATA_UUID. Compare temp_fsid with fsid and metadata_uuid, then echo what +# it matches to or TEMP_FSID. This helps in comparing with the golden output. +check_fsid() +{ + local dev1=$1 + local fsid + local metadata_uuid + + _require_btrfs_command inspect-internal dump-super + + # on disk fsid + fsid=$($BTRFS_UTIL_PROG inspect-internal dump-super $dev1 | \ + grep ^fsid | $AWK_PROG -d" " '{print $2}') + echo -e "On disk fsid:\t\t$fsid" | sed -e "s/$fsid/FSID/g" + + # Print FSID even if it is not the same as metadata_uuid because it has + # to match in the golden output. + metadata_uuid=$(cat /sys/fs/btrfs/$fsid/metadata_uuid) + echo -e "Metadata uuid:\t\tFSID" + + # This returns the temp_fsid if set + tempfsid=$(_btrfs_get_fsid $dev1) + if [[ $tempfsid == $fsid ]]; then + echo -e "Temp fsid:\t\tFSID" + elif [[ $tempfsid == $metadata_uuid ]]; then + # If we are here, it means there is a bug; let it not match with + # the golden output. + echo -e "Temp fsid:\t\t$metadata_uuid" + else + echo -e "Temp fsid:\t\tTEMPFSID" + fi + + echo -e -n "Tempfsid status:\t" + cat /sys/fs/btrfs/$tempfsid/temp_fsid +}