On Wed, Mar 30, 2022 at 2:38 PM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > On Wed, Mar 30, 2022 at 02:10:11PM +0300, Amir Goldstein wrote: > > On Wed, Mar 30, 2022 at 1:26 PM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > > > > > In addition to the generic and filesystem-specific idmapped mount > > > testsuites that already exist upstream today add simple infrastructure > > > so any test can be run on idmapped mounts simply by setting > > > IDMAPPED_MOUNTS=true in the config file or section. The main user for > > > now will be overlay to verify it works correctly on idmapped mounts. > > > > > > > It is not clear from this message how IDMAPPED_MOUNTS=true affects > > non -overlay runs and the answer is complicated: > > > > You handled the majority of cases using _test_mount and _scratch_mount > > but there are still many tests that open code _mount and some helpers > > like _dmthin_mount that are used quite often. > > I verified that actually for testing overlay. _dmthin_mount is used in 8 > tests (btrfs/196, generic/347, generic/405, generic/455, generic/457, > generic/470, generic/482, generic/500). None of them were run in a > regular testrun on the 5.17 base kernel. > Direct _mount calls in the generic/ testsuite are 8 (generic/042, > generic/081, generic/108, generic/361, generic/459, generic/563, > generic/620, geneirc/649). > Again, none of them were run in a regular testrun on the 5.17 base > kernel. Let alone that for most of them conversion to be tested on > idmapped mounts didn't make sense. > Yes of course, overlayfs tests cannot call _mount directly so your tests with check -overlay should be perfectly fine. My comment is only addressing the *generic* implementation of IDMAPPED_MOUNTS. > > > > So either document that IDMAPPED_MOUNTS=true will idmapp the > > fs mounts whenever it can as best effort, or restrict the use of > > IDMAPPED_MOUNTS=true with OVERLAY=true for now. > > Just to clarify, the test patch will be upstreamed independently. It is > included here so people can run xfstests right away if they are > interested. > > The infrastructure is completely generic so every filesystem that > supports idmapped mounts can simply run all of their tests idmapped. But > there will be corner-cases. For example, xfs doesn't allow bulkstat on > idmapped mounts because it is a filesystem wide operation, i.e. you can > retrieve information for any inode in the filesystem so the operation > cannot be scoped reasonably under a single mount. So xfstests testing > bulkstat will fail as it's blocked. Similar for some btrfs ioctl()s. > > We could of course restrict this testmode to -overlay for which we know > things work correctly but that I don't like that as this means that > people won't start using it and so we won't see issues unless someone > sits down and goes through all 750+ tests and figures out for each > individual one why it fails the way it fails. > > I'd rather not restrict the generic infrastructure so people can run and > report failures and we can fix them up or block them as we detect them. > So I'd rather just point that out in the commit message, I think. > Yes this makes perfect sense to me. The failing bulkstat tests are exactly why we would want to let people start testing their fs, it's the false negatives that I am worried about. all the tests that use _dmthin_mount() will succeed without the tester knows that idmalleped was not tested and its going to be hard to audit all the callers of _mount, so either you make _mount a wrapper to __mount or I don't know what. Maybe you'll think of something wiser to do. Thanks, Amir.
