generic/492: bypass the blkid cache when probing the label
PATCH 1/1 generic/492: bypass the blkid cache when probing the label
blkid may serve a stale label from its cache instead of probing the device, so the label just set via xfs_io can be missed and the output mismatch is non-deterministic.
Pass "-c /dev/null" so that blkid always probes the on-disk superblock directly. Unlike the low-level probing mode (-p), which requires util-linux 2.16 or later, this is also supported by the legacy blkid shipped with e2fsprogs.
--- tests/generic/492 | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
tests/generic/492 +4 -3
@@ -28,14 +28,15 @@ $XFS_IO_PROG -c "label -c" $SCRATCH_MNT $XFS_IO_PROG -c "label" $SCRATCH_MNT # And that userspace can see it now, while mounted -# NB: some blkid has trailing whitespace, filter it out here +# NB: bypass the blkid cache which may hold a stale label, and filter +# out the trailing whitespace some blkid versions emit $XFS_IO_PROG -c "label -s label.$seq" $SCRATCH_MNT $XFS_IO_PROG -c "label" $SCRATCH_MNT -blkid -s LABEL $SCRATCH_DEV | _filter_scratch | sed -e "s/ $//g" +blkid -c /dev/null -s LABEL $SCRATCH_DEV | _filter_scratch | sed -e "s/ $//g" # And that the it is still there when it's unmounted _scratch_unmount -blkid -s LABEL $SCRATCH_DEV | _filter_scratch | sed -e "s/ $//g" +blkid -c /dev/null -s LABEL $SCRATCH_DEV | _filter_scratch | sed -e "s/ $//g" # And that it persists after a remount _scratch_mount
Signature · 1 line
2.43.7
Quoted 11 lines · Baokun Li
blkid may serve a stale label from its cache instead of probing the device, so the label just set via
xfs_iocan be missed and the output mismatch is non-deterministic.Pass "-c
/dev/null" so that blkid always probes the on-disk superblock directly. Unlike the low-level probing mode (-p), which requires util-linux 2.16 or later, this is also supported by the legacy blkid shipped with e2fsprogs.Fixes c3c9630968a6 ("generic: test online label ioctl")Signed-off-by Baokun Li <[email protected]>
Seems fine to me; I guess blkid could be racing with whatever updates the cache...udev?
--D
Quoted 31 lines
--- tests/generic/492 | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)tests/generic/492 +4
-3@@ -28,14 +28,15 @@ $XFS_IO_PROG -c "label -c" $SCRATCH_MNT $XFS_IO_PROG -c "label" $SCRATCH_MNT # And that userspace can see it now, while mounted -# NB: some blkid has trailing whitespace, filter it out here +# NB: bypass the blkid cache which may hold a stale label, and filter +# out the trailing whitespace some blkid versions emit $XFS_IO_PROG -c "label -s label.$seq" $SCRATCH_MNT $XFS_IO_PROG -c "label" $SCRATCH_MNT -blkid -s LABEL $SCRATCH_DEV | _filter_scratch | sed -e "s/ $//g" +blkid -c /dev/null -s LABEL $SCRATCH_DEV | _filter_scratch | sed -e "s/ $//g" # And that the it is still there when it's unmounted _scratch_unmount -blkid -s LABEL $SCRATCH_DEV | _filter_scratch | sed -e "s/ $//g" +blkid -c /dev/null -s LABEL $SCRATCH_DEV | _filter_scratch | sed -e "s/ $//g" # And that it persists after a remount _scratch_mountSignature · 1 line
2.43.7
Hi Darrick,
Quoted 10 lines · Darrick J. Wong
Quoted 9 lines · Baokun Li
blkid may serve a stale label from its cache instead of probing the device, so the label just set via
xfs_iocan be missed and the output mismatch is non-deterministic.Pass "-c
/dev/null" so that blkid always probes the on-disk superblock directly. Unlike the low-level probing mode (-p), which requires util-linux 2.16 or later, this is also supported by the legacy blkid shipped with e2fsprogs.
Quoted 4 lines
Quoted 2 lines
Fixes c3c9630968a6 ("generic: test online label ioctl")Signed-off-by Baokun Li <[email protected]>Seems fine to me; I guess blkid could be racing with whatever updates the cache...udev?
Not a race, and not related to udev. It's a long story; details below.
In the xfstests-bld appliance blkid is e2fsprogs' legacy blkid and its cache is /etc/blkid.tab; the only writers are blkid itself and e2fsprogs' fsck (both probe with the cache enabled). systemd's udev uses libblkid's low-level probe API and never touches this file.
What we hit is a strictly sequential sequence that depends on two "same second" coincidences. The cache validity check (blkid_verify() in lib/blkid/probe.c) is:
now >= TIME && mtime(seconds) <= TIME && age < 2s
Both sides are in seconds, and only the 2s window applies to a fresh blkid process (BLKID_BID_FL_VERIFIED is not persistent).
mkfs only advances the device mtime and never updates the cache; the fs label ioctl updates neither, so right after setting a new label a cached lookup can still return the old one.
The trap is set by _check_scratch_fs: it runs "fsck -t ext4 ...", which probes the device with the cache enabled and writes the entry back with TIME=now and the current label state.
Here is a sequential 490/491/492 run that trips it (timestamps from a CI log, ext4/bigalloc_1k):
M-2 09:09:44 end of 490: fsck finds the previous entry stale (age >= 2s) -> probes -> tab = {no LABEL, TIME=44}
M-1 09:09:45 start of 491: mkfs -> device mtime = 45 (mkfs doesn't update the cache; tab stays TIME=44) M 09:09:46 end of 491: unmount + _check_scratch_fs -> fsck sees mtime(45) > TIME(44) -> probes -> tab = {no LABEL, TIME=46} <- trap
M 09:09:46 start of 492: mkfs -> device mtime = 46 M 09:09:46 mount; ioctl label="label.492" (mtime unchanged) M 09:09:46 blkid(35): mtime(46) <= TIME(46), age < 2s -> cache deemed valid, no probe -> prints no LABEL M+1 09:09:47 umount; blkid(39): still < 2s -> again M+1 09:09:47 check reports output mismatch (two lines missing)
The difference between the e2fsprogs and util-linux libblkid is the time granularity: the former stores seconds only (TIME="%ld", bid_time = time(0), compares st_mtime <= bid_time), while the latter stores usec (TIME="sec.usec") and has an nsec tie-break in the compare, so the same-second case is much easier to hit with the e2fsprogs blkid.
The patch avoids this by probing with -c /dev/null (the legacy blkid has no -p). Reproduced on ext4/bigalloc_1k: unpatched 10 of 65 runs fail, patched 120/120 pass.
Quoted 4 lines
Reviewed-by "Darrick J. Wong" <[email protected]>--D
Thanks for the review!
Cheers,
Baokun
Quoted 62 lines · Baokun Li
Hi Darrick,
Quoted 10 lines · Darrick J. Wong
Quoted 9 lines · Baokun Li
blkid may serve a stale label from its cache instead of probing the device, so the label just set via
xfs_iocan be missed and the output mismatch is non-deterministic.Pass "-c
/dev/null" so that blkid always probes the on-disk superblock directly. Unlike the low-level probing mode (-p), which requires util-linux 2.16 or later, this is also supported by the legacy blkid shipped with e2fsprogs.Quoted 4 lines
Quoted 2 lines
Fixes c3c9630968a6 ("generic: test online label ioctl")Signed-off-by Baokun Li <[email protected]>Seems fine to me; I guess blkid could be racing with whatever updates the cache...udev?
Not a race, and not related to udev. It's a long story; details below.
In the xfstests-bld appliance blkid is e2fsprogs' legacy blkid and its cache is
/etc/blkid.tab; the only writers are blkid itself and e2fsprogs' fsck (both probe with the cache enabled). systemd's udev uses libblkid's low-level probe API and never touches this file.What we hit is a strictly sequential sequence that depends on two "same second" coincidences. The cache validity check (
blkid_verify()inlib/blkid/probe.c) is:now >= TIME && mtime(seconds) <= TIME && age < 2s
Both sides are in seconds, and only the 2s window applies to a fresh blkid process (
BLKID_BID_FL_VERIFIEDis not persistent).mkfs only advances the device mtime and never updates the cache; the fs label ioctl updates neither, so right after setting a new label a cached lookup can still return the old one.
The trap is set by
_check_scratch_fs: it runs "fsck -t ext4 ...", which probes the device with the cache enabled and writes the entry back with TIME=now and the current label state.Here is a sequential 490/491/492 run that trips it (timestamps from a CI log, ext4/bigalloc_1k):
M-2 09:09:44 end of 490: fsck finds the previous entry stale (age >= 2s) -> probes -> tab = {no LABEL, TIME=44}
M-1 09:09:45 start of 491: mkfs -> device mtime = 45 (mkfs doesn't update the cache; tab stays TIME=44) M 09:09:46 end of 491: unmount +
_check_scratch_fs-> fsck sees mtime(45) > TIME(44) -> probes -> tab = {no LABEL, TIME=46} <- trapM 09:09:46 start of 492: mkfs -> device mtime = 46 M 09:09:46 mount; ioctl label="label.492" (mtime unchanged) M 09:09:46 blkid(35): mtime(46) <= TIME(46), age < 2s -> cache deemed valid, no probe -> prints no LABEL M+1 09:09:47 umount; blkid(39): still < 2s -> again M+1 09:09:47 check reports output mismatch (two lines missing)
Quoted 5 lines
The difference between the e2fsprogs and util-linux libblkid is the time granularity: the former stores seconds only (TIME="%ld",
bid_time= time(0), comparesst_mtime<=bid_time), while the latter stores usec (TIME="sec.usec") and has an nsec tie-break in the compare, so the same-second case is much easier to hit with the e2fsprogs blkid.
[add ext4 list to cc]
Perhaps xfstests-bld should use the system blkid then? IIRC the one in e2fsprogs is very old and out of date.
--D
Quoted 18 lines
The patch avoids this by probing with -c
/dev/null(the legacy blkid has no -p). Reproduced on ext4/bigalloc_1k: unpatched 10 of 65 runs fail, patched 120/120 pass.Quoted 4 lines
Reviewed-by "Darrick J. Wong" <[email protected]>--D
Thanks for the review!
Cheers,
Baokun
Quoted 8 lines · Darrick J. Wong
Quoted 5 lines
The difference between the e2fsprogs and util-linux libblkid is the time granularity: the former stores seconds only (TIME="%ld",
bid_time= time(0), comparesst_mtime<=bid_time), while the latter stores usec (TIME="sec.usec") and has an nsec tie-break in the compare, so the same-second case is much easier to hit with the e2fsprogs blkid.Perhaps xfstests-bld should use the system blkid then? IIRC the one in e2fsprogs is very old and out of date.
Historically, xfstests-bld was built in environments where libblkid wasn't available. This included Android's runtime (which uses the bionic libc), and grte (Google Runtime Environment)[1].
[1] https://github.com/bazelment/lrte/blob/master/grte/grte-build
However, as more and more tests require utilities from util-linux, xfstests-bld was taught to build util-linux. And if util-linux is being built, then we don't use e2fsprogs-libs any more, which is where libblkid is built. From build-all:
if test -z "$USE_LOCAL_E2FSLIBS" -o -d util-linux; then
SKIP_E2FSLIBS=yes
fi
I've always built xfstesets-bld with util-linux, even though it is listed as an optional repository. This is because (a) util-linux is needed for Android and other non-standard Linux environments, and (b) the version of util-linux used by earlier Debian Stable was too old for some of the newer tests in fstests and blktests.
If you aren't building xfstests-bld with util-linux, it falls back to building libblkid from e2fsprogs-libs. HOWEVER, we don't actually blkid, and libblkid is always built statically. So what seems to be going on is that one of the programs built by xfstests-bld is using the older libblkid, and that results in the blkid cache being writen with a second granularity.
I've never noticed this issue because I've always built with util-linux. The way to solve this is to require building with the system libblkid if util-linux is not being built, and to drop e2fsprogs-libs from xfstests-bld. Or just build xfstests-bld with util-linux. :-)
Cheers,
- Ted
Quoted 36 lines · Theodore Tso
Quoted 7 lines · Darrick J. Wong
Quoted 5 lines
The difference between the e2fsprogs and util-linux libblkid is the time granularity: the former stores seconds only (TIME="%ld",
bid_time= time(0), comparesst_mtime<=bid_time), while the latter stores usec (TIME="sec.usec") and has an nsec tie-break in the compare, so the same-second case is much easier to hit with the e2fsprogs blkid.Perhaps xfstests-bld should use the system blkid then? IIRC the one in e2fsprogs is very old and out of date.
Historically, xfstests-bld was built in environments where libblkid wasn't available. This included Android's runtime (which uses the bionic libc), and grte (Google Runtime Environment)[1].
[1] https://github.com/bazelment/lrte/blob/master/grte/grte-build
However, as more and more tests require utilities from util-linux, xfstests-bld was taught to build util-linux. And if util-linux is being built, then we don't use e2fsprogs-libs any more, which is where libblkid is built. From build-all:
if test -z "$USE_LOCAL_E2FSLIBS" -o -d util-linux; then
SKIP_E2FSLIBS=yes
fiI've always built xfstesets-bld with util-linux, even though it is listed as an optional repository. This is because (a) util-linux is needed for Android and other non-standard Linux environments, and (b) the version of util-linux used by earlier Debian Stable was too old for some of the newer tests in fstests and blktests.
If you aren't building xfstests-bld with util-linux, it falls back to building libblkid from e2fsprogs-libs. HOWEVER, we don't actually blkid, and libblkid is always built statically. So what seems to be going on is that one of the programs built by xfstests-bld is using the older libblkid, and that results in the blkid cache being writen with a second granularity.
Quoted 5 lines
I've never noticed this issue because I've always built with util-linux. The way to solve this is to require building with the system libblkid if util-linux is not being built, and to drop e2fsprogs-libs from xfstests-bld. Or just build xfstests-bld with util-linux. :-)
Thanks for the detailed explanation.
I checked my environment again though -- it turned out not to be xfstests-bld at all. xfstests-bld never touches e2fsprogs.
What happened is that I had manually run "make install" of a locally patched e2fsprogs in the test VM, which overwrote util-linux's blkid and fsck.
Rebuilding e2fsprogs with these options disabled made the test pass:
./configure --disable-libblkid --disable-fsck \
--disable-libuuid --disable-uuidd
I ran unpatched generic/492 60 times on each side in a VM: 12/60 failures with the hand-installed strays, 0/60 with util-linux's blkid/fsck.
That said, I think the patch is still needed, because FS_IOC_SETFSLABEL does not update the block device's mtime:
blkid -s LABEL ... # get A FS_IOC_SETFSLABEL # set B (device mtime unchanged) blkid -s LABEL ... # still get A (within 2s)
The first read writes a cache entry with TIME=now, and the ioctl leaves the device mtime untouched, so a second read within the 2s window still trusts the cache.
I reproduced this with util-linux 2.41.5 too, so "blkid -c /dev/null" is what keeps the test deterministic regardless of which blkid the image carries.
Cheers,
Baokun
Quoted 58 lines · Baokun Li
Quoted 36 lines · Theodore Tso
Quoted 7 lines · Darrick J. Wong
Quoted 5 lines
The difference between the e2fsprogs and util-linux libblkid is the time granularity: the former stores seconds only (TIME="%ld",
bid_time= time(0), comparesst_mtime<=bid_time), while the latter stores usec (TIME="sec.usec") and has an nsec tie-break in the compare, so the same-second case is much easier to hit with the e2fsprogs blkid.Perhaps xfstests-bld should use the system blkid then? IIRC the one in e2fsprogs is very old and out of date.
Historically, xfstests-bld was built in environments where libblkid wasn't available. This included Android's runtime (which uses the bionic libc), and grte (Google Runtime Environment)[1].
[1] https://github.com/bazelment/lrte/blob/master/grte/grte-build
However, as more and more tests require utilities from util-linux, xfstests-bld was taught to build util-linux. And if util-linux is being built, then we don't use e2fsprogs-libs any more, which is where libblkid is built. From build-all:
if test -z "$USE_LOCAL_E2FSLIBS" -o -d util-linux; then
SKIP_E2FSLIBS=yes
fiI've always built xfstesets-bld with util-linux, even though it is listed as an optional repository. This is because (a) util-linux is needed for Android and other non-standard Linux environments, and (b) the version of util-linux used by earlier Debian Stable was too old for some of the newer tests in fstests and blktests.
If you aren't building xfstests-bld with util-linux, it falls back to building libblkid from e2fsprogs-libs. HOWEVER, we don't actually blkid, and libblkid is always built statically. So what seems to be going on is that one of the programs built by xfstests-bld is using the older libblkid, and that results in the blkid cache being writen with a second granularity.
Quoted 5 lines
I've never noticed this issue because I've always built with util-linux. The way to solve this is to require building with the system libblkid if util-linux is not being built, and to drop e2fsprogs-libs from xfstests-bld. Or just build xfstests-bld with util-linux. :-)
Thanks for the detailed explanation.
I checked my environment again though -- it turned out not to be xfstests-bld at all. xfstests-bld never touches e2fsprogs.
What happened is that I had manually run "make install" of a locally patched e2fsprogs in the test VM, which overwrote util-linux's blkid and fsck.
Rebuilding e2fsprogs with these options disabled made the test pass:
./configure --disable-libblkid --disable-fsck \
--disable-libuuid --disable-uuidd
Quoted 6 lines
I ran unpatched generic/492 60 times on each side in a VM: 12/60 failures with the hand-installed strays, 0/60 with util-linux's blkid/fsck.
That said, I think the patch is still needed, because
FS_IOC_SETFSLABELdoes not update the block device's mtime:
Many filesystems /never/ update the bdev mtime, why would they?
Quoted 11 lines
blkid -s LABEL ... # get A FS_IOC_SETFSLABEL # set B (device mtime unchanged) blkid -s LABEL ... # still get A (within 2s)
The first read writes a cache entry with TIME=now, and the ioctl leaves the device mtime untouched, so a second read within the 2s window still trusts the cache.
I reproduced this with util-linux 2.41.5 too, so "blkid -c
/dev/null" is what keeps the test deterministic regardless of which blkid the image carries.
<nod> That was the problem I thought you were solving here ;)
--D
Quoted 5 lines
Cheers,
Baokun
Quoted 58 lines · Darrick J. Wong
Quoted 57 lines · Baokun Li
Quoted 36 lines · Theodore Tso
Quoted 7 lines · Darrick J. Wong
Quoted 5 lines
The difference between the e2fsprogs and util-linux libblkid is the time granularity: the former stores seconds only (TIME="%ld",
bid_time= time(0), comparesst_mtime<=bid_time), while the latter stores usec (TIME="sec.usec") and has an nsec tie-break in the compare, so the same-second case is much easier to hit with the e2fsprogs blkid.Perhaps xfstests-bld should use the system blkid then? IIRC the one in e2fsprogs is very old and out of date.
Historically, xfstests-bld was built in environments where libblkid wasn't available. This included Android's runtime (which uses the bionic libc), and grte (Google Runtime Environment)[1].
[1] https://github.com/bazelment/lrte/blob/master/grte/grte-build
However, as more and more tests require utilities from util-linux, xfstests-bld was taught to build util-linux. And if util-linux is being built, then we don't use e2fsprogs-libs any more, which is where libblkid is built. From build-all:
if test -z "$USE_LOCAL_E2FSLIBS" -o -d util-linux; then
SKIP_E2FSLIBS=yes
fiI've always built xfstesets-bld with util-linux, even though it is listed as an optional repository. This is because (a) util-linux is needed for Android and other non-standard Linux environments, and (b) the version of util-linux used by earlier Debian Stable was too old for some of the newer tests in fstests and blktests.
If you aren't building xfstests-bld with util-linux, it falls back to building libblkid from e2fsprogs-libs. HOWEVER, we don't actually blkid, and libblkid is always built statically. So what seems to be going on is that one of the programs built by xfstests-bld is using the older libblkid, and that results in the blkid cache being writen with a second granularity.
Quoted 5 lines
I've never noticed this issue because I've always built with util-linux. The way to solve this is to require building with the system libblkid if util-linux is not being built, and to drop e2fsprogs-libs from xfstests-bld. Or just build xfstests-bld with util-linux. :-)
Thanks for the detailed explanation.
I checked my environment again though -- it turned out not to be xfstests-bld at all. xfstests-bld never touches e2fsprogs.
What happened is that I had manually run "make install" of a locally patched e2fsprogs in the test VM, which overwrote util-linux's blkid and fsck.
Rebuilding e2fsprogs with these options disabled made the test pass:
./configure --disable-libblkid --disable-fsck \
--disable-libuuid --disable-uuidd
Quoted 7 lines
Quoted 6 lines
I ran unpatched generic/492 60 times on each side in a VM: 12/60 failures with the hand-installed strays, 0/60 with util-linux's blkid/fsck.
That said, I think the patch is still needed, because
FS_IOC_SETFSLABELdoes not update the block device's mtime:Many filesystems /never/ update the bdev mtime, why would they?
Not from FS_IOC_SETFSLABEL, at least -- as far as I can see, none of the filesystems implementing it updates the mtime, and even btrfs does not do it from its label ioctl. btrfs does have update_dev_time() for the add/remove case (5a1972bd9fd4, so that libblkid does not keep reading a stale cache), but that is not wired into its label ioctl.
I can see why. What blkid keys on is the mtime of the /dev/xxx node, and once the mount is set up we hold no reference to that path anymore -- the node can be deleted or re-created, and s_id only keeps an informational name.
Worse, one block device can have any number of /dev nodes pointing at it; an ioctl on a mountpoint fd cannot tell which one the caller means, nor find them all. So the kernel has no well-defined node to update here.
And it gets easy in userspace: for tools that take the device as an argument (tune2fs, e2label, xfs_admin), one utime(device_name, NULL) is enough to guarantee that after set the label of a device with the tool, blkid -s <device> won't see the old label.
The tricky counterpart is the standard way to invoke this ioctl: a mountpoint as the argument, xfs_io being the typical case. It seems the only option there is what the kernel would have to do anyway -- resolve the device behind the mount and update that node's mtime.
Do you have any thoughts?
Quoted 4 lines
Quoted 4 lines
blkid -s LABEL ... # get A FS_IOC_SETFSLABEL # set B (device mtime unchanged) blkid -s LABEL ... # still get A (within 2s)
Quoted 8 lines
Quoted 7 lines
The first read writes a cache entry with TIME=now, and the ioctl leaves the device mtime untouched, so a second read within the 2s window still trusts the cache.
I reproduced this with util-linux 2.41.5 too, so "blkid -c
/dev/null" is what keeps the test deterministic regardless of which blkid the image carries.<nod> That was the problem I thought you were solving here ;)
Yep, that's the idea. The point was to stay away from blkid's cache complexity -- testing how a userspace tool caches things is not really what a kernel test should be doing.
Cheers,
Baokun
Quoted 6 lines · Baokun Li
I checked my environment again though -- it turned out not to be xfstests-bld at all. xfstests-bld never touches e2fsprogs.
What happened is that I had manually run "make install" of a locally patched e2fsprogs in the test VM, which overwrote util-linux's blkid and fsck.
What e2fsprogs's configure script will do is that it checks to see if the blkid libraries are available for building against. That is, if you are using Debian or Ubuntu, you need to have the libblkid-dev package install. Among other things, this makes the following files available:
/usr/include/blkid/blkid.h/usr/lib/x86_64-linux-gnu/libblkid.a/usr/lib/x86_64-linux-gnu/libblkid.so
... which are the files needed to build against libblkid and then link against it statically or dynamically. If these files aren't available, then e2fsprogs will assume that it needs to build the local libblkid, which is what is needed when building on Android, MacOS, NetBSD, Open Solaris, etc.
Quoted 4 lines
Rebuilding e2fsprogs with these options disabled made the test pass:
./configure --disable-libblkid --disable-fsck \
--disable-libuuid --disable-uuidd
This works, but this wll also disable those e2fsprogs features that require those libraries. So mke2fs will not generate UUID's in the superblock, e2fsck won't handle LABEL=xyzzy specifiers, etc. Fortunately xfstests don't depend on these featuers, so this will work.
The way I build with a locally patched e2fsprogs is that I'll build it using the Debian build tools, which will enforce building with the necessary prereqsuite packages, and then I'll put the built packages in the directory test-appliance/debs and then build a fresh test appliance image. Or I'll just upload the packages to the VM and then install it before launching the tests.
Quoted 10 lines
That said, I think the patch is still needed, because
FS_IOC_SETFSLABELdoes not update the block device's mtime:blkid -s LABEL ... # get A FS_IOC_SETFSLABEL # set B (device mtime unchanged) blkid -s LABEL ... # still get A (within 2s)
The first read writes a cache entry with TIME=now, and the ioctl leaves the device mtime untouched, so a second read within the 2s window still trusts the cache.
The patch to xfstests might still be appropriate, but it seems to me that tune2fs should get fixed to update the device mtime (if it can), and perhaps the kernel should try to update block device inode as well as part of the FS_IOC_SETFSLABEL ioctl to address potential corner cases with systemd/udev.
- Ted
Quoted 17 lines · Theodore Tso
Quoted 6 lines · Baokun Li
I checked my environment again though -- it turned out not to be xfstests-bld at all. xfstests-bld never touches e2fsprogs.
What happened is that I had manually run "make install" of a locally patched e2fsprogs in the test VM, which overwrote util-linux's blkid and fsck.
What e2fsprogs's configure script will do is that it checks to see if the blkid libraries are available for building against. That is, if you are using Debian or Ubuntu, you need to have the libblkid-dev package install. Among other things, this makes the following files available:
/usr/include/blkid/blkid.h/usr/lib/x86_64-linux-gnu/libblkid.a/usr/lib/x86_64-linux-gnu/libblkid.so
Quoted 5 lines
... which are the files needed to build against libblkid and then link against it statically or dynamically. If these files aren't available, then e2fsprogs will assume that it needs to build the local libblkid, which is what is needed when building on Android, MacOS, NetBSD, Open Solaris, etc.
Indeed, I originally built and installed it manually in an environment without libblkid-dev.
Quoted 8 lines
Quoted 4 lines
Rebuilding e2fsprogs with these options disabled made the test pass:
./configure --disable-libblkid --disable-fsck \
--disable-libuuid --disable-uuiddThis works, but this wll also disable those e2fsprogs features that require those libraries. So mke2fs will not generate UUID's in the superblock, e2fsck won't handle LABEL=xyzzy specifiers, etc. Fortunately xfstests don't depend on these featuers, so this will work.
Hmm, what I disabled is exactly the set duplicated by util-linux:
the blkid/uuid libs and UUID/LABEL= handling all live there, so the tools just link the system ones -- there is no functional loss.
Quoted 6 lines
The way I build with a locally patched e2fsprogs is that I'll build it using the Debian build tools, which will enforce building with the necessary prereqsuite packages, and then I'll put the built packages in the directory test-appliance/debs and then build a fresh test appliance image. Or I'll just upload the packages to the VM and then install it before launching the tests.
That sounds much better than a hand install -- I'll give it a try.
Quoted 7 lines
Quoted 7 lines
That said, I think the patch is still needed, because
FS_IOC_SETFSLABELdoes not update the block device's mtime:blkid -s LABEL ... # get A FS_IOC_SETFSLABEL # set B (device mtime unchanged) blkid -s LABEL ... # still get A (within 2s)
Quoted 8 lines
Quoted 3 lines
The first read writes a cache entry with TIME=now, and the ioctl leaves the device mtime untouched, so a second read within the 2s window still trusts the cache.
The patch to xfstests might still be appropriate, but it seems to me that tune2fs should get fixed to update the device mtime (if it can), and perhaps the kernel should try to update block device inode as well as part of the
FS_IOC_SETFSLABELioctl to address potential corner cases with systemd/udev.
Yes -- we need to add a utime() in tune2fs's ioctl path to update the device mtime; I'll send a patch shortly.
For the kernel side, we don't have the dev path that the caller is using, so it is not doable there: the inode we can reach inside the ioctl is not the one userspace sees. Details are in my reply to Darrick:
https://lore.kernel.org/all/[email protected]
Also, a thought that just came up: on the blkid side, would it be feasible to not trust the cache for mounted devices and always re-probe? The mount / set label / umount / get label (within 2s) window would remain, but it would be much smaller.
Thanks,
Baokun
Quoted 10 lines · Baokun Li
Quoted 6 lines
Quoted 2 lines
./configure --disable-libblkid --disable-fsck \
--disable-libuuid --disable-uuiddThis works, but this wll also disable those e2fsprogs features that require those libraries. So mke2fs will not generate UUID's in the superblock, e2fsck won't handle LABEL=xyzzy specifiers, etc. Fortunately xfstests don't depend on these featuers, so this will work.
Hmm, what I disabled is exactly the set duplicated by util-linux:
the blkid/uuid libs and UUID/LABEL= handling all live there, so the tools just link the system ones -- there is no functional loss.
The fact that you needed the --disable options implies that e2fsprogs wasn't able to use the util-linux supplied libraries. If the configure script determines libblkid and libuuid are available for linking against, (e.g., libblkid-dev and uuid-dev are installed), the built-in libblkid and libuuid file systems are disabled.
From the configure script output:
checking for blkid_get_cache in -lblkid... yes
Using system blkid library by default
checking for uuid_generate in -luuid... yes
Using system uuid by default
Disabling uuidd by default
In contrast, if you run the configure script on MacOS, NetBSD, or a Linux system missing the libuuid / libblkid development files, you will see:
checking for uuid_generate in -luuid... no
Enabling private uuid library by default
Building uuidd by default
Quoted 4 lines
Also, a thought that just came up: on the blkid side, would it be feasible to not trust the cache for mounted devices and always re-probe? The mount / set label / umount / get label (within 2s) window would remain, but it would be much smaller.
Maybe. It's not clear how often people might be trying to query the blkid data. Also, figuring out whether or not a device is mounted is not free. That would mean that for every blkid query, it would have to read /proc/mounts to make that determination. And note that on some data center servers, there could potentially be hundreds if not thousands of entries in /proc/mounts, either because you have a huge number of bind mounts or overlayfs, and/or because you might have a huge number of read-only iscsi mounts to deliver software packages to your container based Borg or Kubernetes jobs.
I suppose you could put in the blkid cache a hint as to whether the file system is mounted, but that in itself could get stale and out of date.
Cheers,
- Ted
Quoted 25 lines · Theodore Tso
Quoted 9 lines · Baokun Li
Quoted 6 lines
Quoted 2 lines
./configure --disable-libblkid --disable-fsck \
--disable-libuuid --disable-uuiddThis works, but this wll also disable those e2fsprogs features that require those libraries. So mke2fs will not generate UUID's in the superblock, e2fsck won't handle LABEL=xyzzy specifiers, etc. Fortunately xfstests don't depend on these featuers, so this will work.
Hmm, what I disabled is exactly the set duplicated by util-linux:
the blkid/uuid libs and UUID/LABEL= handling all live there, so the tools just link the system ones -- there is no functional loss.The fact that you needed the --disable options implies that e2fsprogs wasn't able to use the util-linux supplied libraries. If the configure script determines libblkid and libuuid are available for linking against, (e.g., libblkid-dev and uuid-dev are installed), the built-in libblkid and libuuid file systems are disabled.
From the configure script output:
checking for
blkid_get_cachein -lblkid... yes
Using system blkid library by defaultchecking for
uuid_generatein -luuid... yes
Using system uuid by default
Disabling uuidd by default
Quoted 7 lines
In contrast, if you run the configure script on MacOS, NetBSD, or a Linux system missing the libuuid / libblkid development files, you will see:
checking for
uuid_generatein -luuid... no
Enabling private uuid library by default
Building uuidd by default
Right. The flags went in only after the development packages were installed. The original hand build had no libblkid-dev, so it built and installed the private copies, which is how the distro's blkid/fsck/findfs got shadowed.
With libblkid-dev, uuid-dev and pkg-config installed, a plain ./configure now prints exactly your first output. Compared with my --disable build, the only extra program it produces is the fsck wrapper.
Quoted 14 lines
Quoted 4 lines
Also, a thought that just came up: on the blkid side, would it be feasible to not trust the cache for mounted devices and always re-probe? The mount / set label / umount / get label (within 2s) window would remain, but it would be much smaller.
Maybe. It's not clear how often people might be trying to query the blkid data. Also, figuring out whether or not a device is mounted is not free. That would mean that for every blkid query, it would have to read
/proc/mountsto make that determination. And note that on some data center servers, there could potentially be hundreds if not thousands of entries in/proc/mounts, either because you have a huge number of bind mounts or overlayfs, and/or because you might have a huge number of read-only iscsi mounts to deliver software packages to your container based Borg or Kubernetes jobs.
Quoted 3 lines
I suppose you could put in the blkid cache a hint as to whether the file system is mounted, but that in itself could get stale and out of date.
Agreed -- checking whether a device is mounted on every probe might cost more than simply re-probing the device. Updating the mtime on the write side (e.g., in tune2fs, xfs_io, or the kernel) looks like the better approach.
Cheers,
Baokun