On 13.03.24 20:10, David Hildenbrand wrote:
On 13.03.24 18:19, Darrick J. Wong wrote:
On Wed, Mar 13, 2024 at 01:29:12PM +0100, David Hildenbrand wrote:
On 12.03.24 17:44, Darrick J. Wong wrote:
On Tue, Mar 12, 2024 at 04:33:14PM +0100, David Hildenbrand wrote:
On 12.03.24 16:13, David Hildenbrand wrote:
On 11.03.24 23:38, Darrick J. Wong wrote:
[add willy and linux-mm]
On Thu, Mar 07, 2024 at 08:40:17PM -0800, Eric Biggers wrote:
On Thu, Mar 07, 2024 at 07:46:50PM -0800, Darrick J. Wong wrote:
BTW, is xfs_repair planned to do anything about any such extra blocks?
Sorry to answer your question with a question, but how much checking is
$filesystem expected to do for merkle trees?
In theory xfs_repair could learn how to interpret the verity descriptor,
walk the merkle tree blocks, and even read the file data to confirm
intactness. If the descriptor specifies the highest block address then
we could certainly trim off excess blocks. But I don't know how much of
libfsverity actually lets you do that; I haven't looked into that
deeply. :/
For xfs_scrub I guess the job is theoretically simpler, since we only
need to stream reads of the verity files through the page cache and let
verity tell us if the file data are consistent.
For both tools, if something finds errors in the merkle tree structure
itself, do we turn off verity? Or do we do something nasty like
truncate the file?
As far as I know (I haven't been following btrfs-progs, but I'm familiar with
e2fsprogs and f2fs-tools), there isn't yet any precedent for fsck actually
validating the data of verity inodes against their Merkle trees.
e2fsck does delete the verity metadata of inodes that don't have the verity flag
enabled. That handles cleaning up after a crash during FS_IOC_ENABLE_VERITY.
I suppose that ideally, if an inode's verity metadata is invalid, then fsck
should delete that inode's verity metadata and remove the verity flag from the
inode. Checking for a missing or obviously corrupt fsverity_descriptor would be
fairly straightforward, but it probably wouldn't catch much compared to actually
validating the data against the Merkle tree. And actually validating the data
against the Merkle tree would be complex and expensive. Note, none of this
would work on files that are encrypted.
Re: libfsverity, I think it would be possible to validate a Merkle tree using
libfsverity_compute_digest() and the callbacks that it supports. But that's not
quite what it was designed for.
Is there an ioctl or something that allows userspace to validate an
entire file's contents? Sort of like what BLKVERIFY would have done for
block devices, except that we might believe its answers?
Just reading the whole file and seeing whether you get an error would do it.
Though if you want to make sure it's really re-reading the on-disk data, it's
necessary to drop the file's pagecache first.
I tried a straight pagecache read and it worked like a charm!
But then I thought to myself, do I really want to waste memory bandwidth
copying a bunch of data? No. I don't even want to incur system call
overhead from reading a single byte every $pagesize bytes.
So I created 2M mmap areas and read a byte every $pagesize bytes. That
worked too, insofar as SIGBUSes are annoying to handle. But it's
annoying to take signals like that.
Then I started looking at madvise. MADV_POPULATE_READ looked exactly
like what I wanted -- it prefaults in the pages, and "If populating
fails, a SIGBUS signal is not generated; instead, an error is returned."
Yes, these were the expected semantics :)
But then I tried rigging up a test to see if I could catch an EIO, and
instead I had to SIGKILL the process! It looks filemap_fault returns
VM_FAULT_RETRY to __xfs_filemap_fault, which propagates up through
__do_fault -> do_read_fault -> do_fault -> handle_pte_fault ->
handle_mm_fault -> faultin_page -> __get_user_pages. At faultin_pages,
the VM_FAULT_RETRY is translated to -EBUSY.
__get_user_pages squashes -EBUSY to 0, so faultin_vma_page_range returns
that to madvise_populate. Unfortunately, madvise_populate increments
its loop counter by the return value (still 0) so it runs in an
infinite loop. The only way out is SIGKILL.
That's certainly unexpected. One user I know is QEMU, which primarily
uses MADV_POPULATE_WRITE to prefault page tables. Prefaulting in QEMU is
primarily used with shmem/hugetlb, where I haven't heard of any such
endless loops.
So I don't know what the correct behavior is here, other than the
infinite loop seems pretty suspect. Is it the correct behavior that
madvise_populate returns EIO if __get_user_pages ever returns zero?
That doesn't quite sound right if it's the case that a zero return could
also happen if memory is tight.
madvise_populate() ends up calling faultin_vma_page_range() in a loop.
That one calls __get_user_pages().
__get_user_pages() documents: "0 return value is possible when the fault
would need to be retried."
So that's what the caller does. IIRC, there are cases where we really
have to retry (at least once) and will make progress, so treating "0" as
an error would be wrong.
Staring at other __get_user_pages() users, __get_user_pages_locked()
documents: "Please note that this function, unlike __get_user_pages(),
will not return 0 for nr_pages > 0, unless FOLL_NOWAIT is used.".
But there is some elaborate retry logic in there, whereby the retry will
set FOLL_TRIED->FAULT_FLAG_TRIED, and I think we'd fail on the second
retry attempt (there are cases where we retry more often, but that's
related to something else I believe).
So maybe we need a similar retry logic in faultin_vma_page_range()? Or
make it use __get_user_pages_locked(), but I recall when I introduced
MADV_POPULATE_READ, there was a catch to it.
I'm trying to figure out who will be setting the VM_FAULT_SIGBUS in the
mmap()+access case you describe above.
Staring at arch/x86/mm/fault.c:do_user_addr_fault(), I don't immediately see
how we would transition from a VM_FAULT_RETRY loop to VM_FAULT_SIGBUS.
Because VM_FAULT_SIGBUS would be required for that function to call
do_sigbus().
The code I was looking at yesterday in filemap_fault was:
page_not_uptodate:
/*
* Umm, take care of errors if the page isn't up-to-date.
* Try to re-read it _once_. We do this synchronously,
* because there really aren't any performance issues here
* and we need to check for errors.
*/
fpin = maybe_unlock_mmap_for_io(vmf, fpin);
error = filemap_read_folio(file, mapping->a_ops->read_folio, folio);
if (fpin)
goto out_retry;
folio_put(folio);
if (!error || error == AOP_TRUNCATED_PAGE)
goto retry_find;
filemap_invalidate_unlock_shared(mapping);
return VM_FAULT_SIGBUS;
Wherein I /think/ fpin is non-null in this case, so if
filemap_read_folio returns an error, we'll do this instead:
out_retry:
/*
* We dropped the mmap_lock, we need to return to the fault handler to
* re-find the vma and come back and find our hopefully still populated
* page.
*/
if (!IS_ERR(folio))
folio_put(folio);
if (mapping_locked)
filemap_invalidate_unlock_shared(mapping);
if (fpin)
fput(fpin);
return ret | VM_FAULT_RETRY;
and since ret was 0 before the goto, the only return code is
VM_FAULT_RETRY. I had speculated that perhaps we could instead do:
if (fpin) {
if (error)
ret |= VM_FAULT_SIGBUS;
goto out_retry;
}
But I think the hard part here is that there doesn't seem to be any
distinction between transient read errors (e.g. disk cable fell out) vs.
semi-permanent errors (e.g. verity says the hash doesn't match).
AFAICT, either the read(ahead) sets uptodate and callers read the page,
or it doesn't set it and callers treat that as an error-retry
opportunity.
For the transient error case VM_FAULT_RETRY makes perfect sense; for the
second case I imagine we'd want something closer to _SIGBUS.
Agreed, it's really hard to judge when it's the right time to give up
retrying. At least with MADV_POPULATE_READ we should try achieving the same
behavior as with mmap()+read access. So if the latter manages to trigger
SIGBUS, MADV_POPULATE_READ should return an error.
Is there an easy way to for me to reproduce this scenario?
Yes. Take this Makefile:
CFLAGS=-Wall -Werror -O2 -g -Wno-unused-variable
all: mpr
and this C program mpr.c:
/* test MAP_POPULATE_READ on a file */
#include <stdio.h>
#include <errno.h>
#include <fcntl.h>
#include <unistd.h>
#include <string.h>
#include <sys/stat.h>
#include <sys/mman.h>
#define min(a, b) ((a) < (b) ? (a) : (b))
#define BUFSIZE (2097152)
int main(int argc, char *argv[])
{
struct stat sb;
long pagesize = sysconf(_SC_PAGESIZE);
off_t read_sz, pos;
void *addr;
char c;
int fd, ret;
if (argc != 2) {
printf("Usage: %s fname\n", argv[0]);
return 1;
}
fd = open(argv[1], O_RDONLY);
if (fd < 0) {
perror(argv[1]);
return 1;
}
ret = fstat(fd, &sb);
if (ret) {
perror("fstat");
return 1;
}
/* Validate the file contents with regular reads */
for (pos = 0; pos < sb.st_size; pos += sb.st_blksize) {
ret = pread(fd, &c, 1, pos);
if (ret < 0) {
if (errno != EIO) {
perror("pread");
return 1;
}
printf("%s: at offset %llu: %s\n", argv[1],
(unsigned long long)pos,
strerror(errno));
break;
}
}
ret = pread(fd, &c, 1, sb.st_size);
if (ret < 0) {
if (errno != EIO) {
perror("pread");
return 1;
}
printf("%s: at offset %llu: %s\n", argv[1],
(unsigned long long)sb.st_size,
strerror(errno));
}
/* Validate the file contents with MADV_POPULATE_READ */
read_sz = ((sb.st_size + (pagesize - 1)) / pagesize) * pagesize;
printf("%s: read bytes %llu\n", argv[1], (unsigned long long)read_sz);
for (pos = 0; pos < read_sz; pos += BUFSIZE) {
unsigned int mappos;
size_t maplen = min(read_sz - pos, BUFSIZE);
addr = mmap(NULL, maplen, PROT_READ, MAP_SHARED, fd, pos);
if (addr == MAP_FAILED) {
perror("mmap");
return 1;
}
ret = madvise(addr, maplen, MADV_POPULATE_READ);
if (ret) {
perror("madvise");
return 1;
}
ret = munmap(addr, maplen);
if (ret) {
perror("munmap");
return 1;
}
}
ret = close(fd);
if (ret) {
perror("close");
return 1;
}
return 0;
}
and this shell script mpr.sh:
#!/bin/bash -x
# Try to trigger infinite loop with regular IO errors and MADV_POPULATE_READ
scriptdir="$(dirname "$0")"
commands=(dmsetup mkfs.xfs xfs_io timeout strace "$scriptdir/mpr")
for cmd in "${commands[@]}"; do
if ! command -v "$cmd" &>/dev/null; then
echo "$cmd: Command required for this program."
exit 1
fi
done
dev="${1:-/dev/sda}"
mnt="${2:-/mnt}"
dmtarget="dumbtarget"
# Clean up any old mounts
umount "$dev" "$mnt"
dmsetup remove "$dmtarget"
rmmod xfs
# Create dm linear mapping to block device and format filesystem
sectors="$(blockdev --getsz "$dev")"
tgt="/dev/mapper/$dmtarget"
echo "0 $sectors linear $dev 0" | dmsetup create "$dmtarget"
mkfs.xfs -f "$tgt"
# Create a file that we'll read, then cycle mount to zap pagecache
mount "$tgt" "$mnt"
xfs_io -f -c "pwrite -S 0x58 0 1m" "$mnt/a"
umount "$mnt"
mount "$tgt" "$mnt"
# Load file metadata
stat "$mnt/a"
# Induce EIO errors on read
dmsetup suspend --noflush --nolockfs "$dmtarget"
echo "0 $sectors error" | dmsetup load "$dmtarget"
dmsetup resume "$dmtarget"
# Try to provoke the kernel; kill the process after 10s so we can clean up
timeout -s KILL 10s strace -s99 -e madvise "$scriptdir/mpr" "$mnt/a"
# Stop EIO errors so we can unmount
dmsetup suspend --noflush --nolockfs "$dmtarget"
echo "0 $sectors linear $dev 0" | dmsetup load "$dmtarget"
dmsetup resume "$dmtarget"
# Unmount and clean up after ourselves
umount "$mnt"
dmsetup remove "$dmtarget"
<EOF>
make the C program, then run ./mpr.sh <device> <mountpoint>. It should
stall in the madvise call until timeout sends sigkill to the program;
you can crank the 10s timeout up if you want.
<insert usual disclaimer that I run all these things in scratch VMs>
Yes, seems to work, nice!
[ 452.455636] buffer_io_error: 6 callbacks suppressed
[ 452.455638] Buffer I/O error on dev dm-0, logical block 16, async page read
[ 452.456169] Buffer I/O error on dev dm-0, logical block 17, async page read
[ 452.456456] Buffer I/O error on dev dm-0, logical block 18, async page read
[ 452.456754] Buffer I/O error on dev dm-0, logical block 19, async page read
[ 452.457061] Buffer I/O error on dev dm-0, logical block 20, async page read
[ 452.457350] Buffer I/O error on dev dm-0, logical block 21, async page read
[ 452.457639] Buffer I/O error on dev dm-0, logical block 22, async page read
[ 452.457942] Buffer I/O error on dev dm-0, logical block 23, async page read
[ 452.458242] Buffer I/O error on dev dm-0, logical block 16, async page read
[ 452.458552] Buffer I/O error on dev dm-0, logical block 17, async page read
+ timeout -s KILL 10s strace -s99 -e madvise ./mpr /mnt/tmp//a
/mnt/tmp//a: at offset 0: Input/output error
/mnt/tmp//a: read bytes 1048576
madvise(0x7f9393624000, 1048576, MADV_POPULATE_READ./mpr.sh: line 45: 2070 Killed tim"
And once I switch over to reading instead of MADV_POPULATE_READ:
[ 753.940230] buffer_io_error: 6 callbacks suppressed
[ 753.940233] Buffer I/O error on dev dm-0, logical block 8192, async page read
[ 753.941402] Buffer I/O error on dev dm-0, logical block 8193, async page read
[ 753.942084] Buffer I/O error on dev dm-0, logical block 8194, async page read
[ 753.942738] Buffer I/O error on dev dm-0, logical block 8195, async page read
[ 753.943412] Buffer I/O error on dev dm-0, logical block 8196, async page read
[ 753.944088] Buffer I/O error on dev dm-0, logical block 8197, async page read
[ 753.944741] Buffer I/O error on dev dm-0, logical block 8198, async page read
[ 753.945415] Buffer I/O error on dev dm-0, logical block 8199, async page read
[ 753.946105] Buffer I/O error on dev dm-0, logical block 8192, async page read
[ 753.946661] Buffer I/O error on dev dm-0, logical block 8193, async page read
+ timeout -s KILL 10s strace -s99 -e madvise ./mpr /mnt/tmp//a
/mnt/tmp//a: at offset 0: Input/output error
/mnt/tmp//a: read bytes 1048576
--- SIGBUS {si_signo=SIGBUS, si_code=BUS_ADRERR, si_addr=0x7f34f82d8000} ---
+++ killed by SIGBUS (core dumped) +++
timeout: the monitored command dumped core
./mpr.sh: line 45: 2388 Bus error timeout -s KILL 10s strace -s99 -e madvise "$scriptdir"
Let me dig how the fault handler is able to conclude SIGBUS here!
Might be as simple as this:
diff --git a/mm/gup.c b/mm/gup.c
index df83182ec72d5..62df1548b7779 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1734,8 +1734,8 @@ long faultin_vma_page_range(struct vm_area_struct *vma, unsigned long start,
if (check_vma_flags(vma, gup_flags))
return -EINVAL;
- ret = __get_user_pages(mm, start, nr_pages, gup_flags,
- NULL, locked);
+ ret = __get_user_pages_locked(mm, start, nr_pages, NULL, locked,
+ gup_flags);
lru_add_drain();
return ret;
}
[ 57.154542] Buffer I/O error on dev dm-0, logical block 16, async page read
[ 57.155276] Buffer I/O error on dev dm-0, logical block 17, async page read
[ 57.155911] Buffer I/O error on dev dm-0, logical block 18, async page read
[ 57.156568] Buffer I/O error on dev dm-0, logical block 19, async page read
[ 57.157245] Buffer I/O error on dev dm-0, logical block 20, async page read
[ 57.157880] Buffer I/O error on dev dm-0, logical block 21, async page read
[ 57.158539] Buffer I/O error on dev dm-0, logical block 22, async page read
[ 57.159197] Buffer I/O error on dev dm-0, logical block 23, async page read
[ 57.159838] Buffer I/O error on dev dm-0, logical block 16, async page read
[ 57.160492] Buffer I/O error on dev dm-0, logical block 17, async page read
+ timeout -s KILL 10s strace -s99 -e madvise ./mpr /mnt/tmp//a
[ 57.190472] Retrying
/mnt/tmp//a: at offset 0: Input/output error
/mnt/tmp//a: read bytes 1048576
madvise(0x7f0fa0384000, 1048576, MADV_POPULATE_READ) = -1 EFAULT (Bad address)
madvise: Bad address
And -EFAULT is what MADV_POPULATE_READ is documented to return for SIGBUS.
(there are a couple of ways we could speedup MADV_POPULATE_READ, maybe
at some point using VMA locks)
--
Cheers,
David / dhildenb
