Starting with the 7.2 kernel, buffer heads no longer lose a reference
during the submit-wait procedure. I didn't notice that change, so now I
would expect all buffer heads to be leaking. The fact that I still can't
tell the difference makes me believe, like I mentioned before, that
somehow the kernel is still disposing of the buffer heads. Either way,
fix this.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
This is the last of the three leaks discovered by Valgrind in the
userland port. During the loop that writes ephemeral objects to disk,
checkpoint mapping blocks are created and kept around for multiple
iterations until they are full. At that point, the cpm buffer head gets
released and we get a new one. All that looks correct, but no release
happens after the final loop. Fix it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
This is the second bug discovered by valgrind in the userland tool. The
buffer head for the ip bitmap gets nulled after joining the transaction,
but we still hold a reference to it (the transactions takes a new one).
Fix it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Work on the userland port with valgrind has reavealed a number of
longstanding leaks in the kernel driver. For some reason I always
assumed that kmemleak would eventually notice issues with buffer heads,
but it clearly doesn't. This bug in particular must have been around
since the very beginning: all buffer heads for checkpoint descriptor
blocks get leaked during mount.
Since nobody noticed the problem so far, I'm guessing that the kernel
somehow disposes of abandoned buffer heads. Perhaps it has something to
do with the old issue from patch 517dd221df ("Forcefully free buffer
heads on transaction commit")? Either way, fix the leak and get the code
to match the userland port.
One thing to note: before pushing this patch I tried asking Github
Copilot to look for leaks in the super.c file and it spotted the problem
immediately. It seems to have become far more useful since the last time
I tried it, so once I'm done fixing the current crop of leaks I should
try and see what it can find.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The build is broken for the 7.2 release candidate. The reason is that
submit_bh() is gone now, replaced with a new bh_submit() that also takes
the I/O end handler as an argument. Adjust apfs_submit_bh() to these
changes.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I've been trying to build the driver for userland lately. Most of the
code can be left as it is as long as I spoof the kernel headers and
everything else that the kernel provides. The one exception is my
uintmax_t typedef, because macos is very determined to provide its own
definition for that type. I guess that makes sense, given that the lzfse
code came from Apple in the first place.
Anyway, none of this is a problem for the driver, but since I want to
keep both codebases identical, put the same macro guards in here.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I notice a lot of traffic to the following issue:
https://github.com/linux-apfs/linux-apfs-rw/issues/96
Recent distro kernels seem to ship with a built-in libcrc32c, so the
modprobe step described in the README is not necessary and will fail. I
assumed it would be clear to the users that they can just keep going
when that happens, but some of them are being extra careful, which is
probably the right attitude to have when dealing with a filesystem.
Rephrase the README to make it clear that the modprobe step is not
usually needed anymore.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
This quick release adds support for the fsconfig() syscall, otherwise
newer versions of the mount tool won't accept any options.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
SergDL reports that mounts seem broken for the 7.0 kernel:
https://github.com/linux-apfs/linux-apfs-rw/issues/119
I never encountered this problem during testing, probably because my
version of the mount tool is older. It seems that mount options are now
set one by one using the fsconfig() syscall, and that's not possible if
we parse them all at once with ->parse_monolithic().
Implement ->parse_param() instead, at least for the latest kernel. I do
wonder if this issue won't pop up with older kernels though, since the
fsconfig() syscall has been around since 5.2. I guess I'll wait to see
if somebody complains about it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Instead of setting the default mount options right before parsing the
actual options, set them on the superblock as soon as it gets allocated.
This will allow the parsing code to be called more than once by the new
mount api.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The new mount api forces me to make one function call for each option to
parse. This change means that the default options will need to get set
elsewhere, otherwise each new call would reset to default.
As a first step, move that code from parse_options() into a new
apfs_set_default_opts().
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
This release adds support for the upcoming 7.0 kernel version, with a
patch by Alessio Faina. Aditya Garg also contributes a fix for a build
issue caused by the makefile.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Reorganize the version check in the out_free_sbi label to better match
the simpler one at out_unmap_super.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Following the previous patch, which made parse_options() always run
right after preparse_options(), merge both functions into one.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Since the mount api conversion, kernel version 7.0 and above parse all
the options at once before moving on to the rest of the mount process.
There is no reason that other kernel versions can't do the same; in fact
that was originally my intention, as explained in the commit message for
af2ee526c0 ("Avoid double parsing of mount options"). So always call
parse_options() right after preparse_options(). The plan is to unify
these functions next.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Inside parse_options(), some kernel versions retrieve the superblock
info from the vfs superblock, while others retrieve it from the
filesystem context. Instead just pass the info as an argument and
simplify the version checks.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I seem to have missed a few whitespace issues during my review of the
recent mount api conversion. Fix them now.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
At this point, the nx_flags variable only exists to get copied into
s_mount_opt of the superblock. Just work with s_mount_opt directly and
get rid of the goto label.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I'm trying to unify mount option processing across kernel versions as
much as possible. Instead of calling parse_options_set_flags() directly
from parse_options() whenever possible, always set the new s_mount_opt
and use it to call parse_options_set_flags() later.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Move s_uid/s_gid initialization inside parse_options() for all kernel
versions, getting rid of a version check for kernels 7.0 and above.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
As all the in-tree modules has been ported to the new Kernel 7
fs mount API, the old API has been removed.
This patch add the support for the new set of APIs, while
keeping previous kernel support intact.
The code is in review on the Canonical Launchpad bug
https://launchpad.net/bugs/2142837 as well
Signed-off-by: Alessio Faina <alessio.faina@canonical.com>
The build is broken for the 6.19 release candidate:
https://github.com/linux-apfs/linux-apfs-rw/issues/109
The problem is that the inode i_state field can no longer be accessed
directly. Switch to the new helpers, using a wrapper to keep the code
consistent for all kernel versions.
Note that I always use the *_raw version of the helpers to avoid a lockdep
check that will not pass. I can't tell for sure if this is a bug.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The build is broken for the 6.18 release candidate:
https://github.com/linux-apfs/linux-apfs-rw/issues/107
The problem is that ->d_name has become const, and that qualifier gets
dropped when we pass it around in the driver. Just treat is as const for
all kernel versions.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Once more, the build is still broken for the 6.17 release candidate.
This time the problem is that we are no longer allowed to set s_d_op
directly. Use the new helper instead.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The build is still broken for the 6.17 release candidate. The problem is
that ->write_begin() and ->write_end() now take a kiocb struct as the
first argument instead of a file. Add the proper version checks. The
driver does nothing with this argument but pass it along, so continue to
call it "file" to keep the patch simple.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The build is broken for the 6.17 release candidate:
https://github.com/linux-apfs/linux-apfs-rw/issues/102
Upstream has renamed the fileattr struct to file_kattr, so that needs to
happen here as well. To avoid excessive nesting of version checks, don't
change the actual code and just rewrite it on build time with a macro. I
hope this doesn't cause any trouble in the future...
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
This release adds support for the upcoming 6.15 kernel version, and for
RHEL 9.6.
A longstanding objtool warning has also been fixed.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I recently started to use the __annotate_jump_table macro to silence an
objtool warning, but it seems that this macro was only added for the 5.3
kernel. Just define it to nothing for older releases, the objtool
warnings probably don't matter that much.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The ->mkdir() method now needs to return a dentry struct instead of an
int, but this is meant for clustered filesystems or something so just
return NULL on success, like most filesystems do.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
It seems that wait_for_stable_page(), page_has_buffers() and
grab_cache_page_write_begin() are no longer available, so put
implementations inside the driver.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I don't intend to remove all typedefs from the lzfse code, but this one
is triggering a false positive for another issue in checkpatch. Get rid
of it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The kernel has macros for these things, so use them instead and silence
the checkpatch warnings.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Another checkpatch warning: remove all braces around blocks that have a
single statement, except for two cases where it makes more sense to
reorganize long comments so that the warnings stop.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Change the comment style in the lzfse code (by hand) to more or less
match the rest of the driver.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Continuing with the lzfse code cleanup: it seems that clang-format can
at least fix the whitespace reliably, so run it and get that out of the
way. But set the column limit to 100 like checkpatch uses, to avoid too
much awkward line wrapping on the parts of the code that go far to the
right.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The kernel provides its own __always_inline macro, so get rid of those
that came with lzfse.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I don't think any rare compilers are used to build the kernel, so just
assume gcc/clang everywhere. Also get rid of the dll checks.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Jumptables are used elsewhere in the kernel, so it's safe to assume that
the compiler supports them. Clean up all the dead fallback code for
lzvn decompression.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Since I first copied the lzfse code into the driver, the build has been
throwing the following error:
lzfse/lzvn_decode_base.o: warning: objtool: lzvn_decode+0x10: sibling call from callable instruction with modified stack frame
I've been ignoring this because objtool warnings are probably not a big
deal for us; I think they are mostly for live patches. I'm also not
familiar with the lzfse code so I assumed this would be hard to fix.
It's not though: just annotate the jump table correctly.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I have no real intention of ever supporting compressed writes. Last I
checked, the official implementation simply decompresses files when you
try to write to them, so maybe I will do that here as well at some
point. For now users that need this can work with a copy of the file
(and delete the original).
So, get rid of all the unused lzfse/lzvn compression code. This will
make it easier to fix the code style in the following patches.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
This release adds support for the upcoming 6.14 upstream kernel, and for
a handful of RHEL 9 kernels that have been broken for a while.
Some improvements have been made to ENOSPC behaviour, but the work here
isn't quite finished yet.
Read-only support is added for fusion drives. Writes will probably never
get implemented, though I might reconsider this if people actually ask
for the feature.
Read-only mounts of volumes with so-called "dataless snapshots" are also
tentatively allowed. I have no idea what this is about but there should
be no risk.
Free space as reported by statfs is now much more accurate. A few other
bugs were also fixed, but nothing big enough to deserve notice.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Failing to read an ephemeral object does not abort the transaction, it
just returns an error. The problem is that, by this point, nx_eph_list
is already set: the next time a transaction starts the reads will get
skipped, and things will proceed as if there was no error.
I got this series of messages from xfstests (no idea which test, but
clearly one that fakes I/O errors):
[18920.212351] Buffer I/O error on dev dm-0, logical block 3932144, async page read
[18920.515341] APFS (dm-0:0): failed to read cpm block (apfs_read_single_cpm_block:231)
[18920.520378] APFS (dm-0:0): failed to read cpm block 0 (apfs_read_ephemeral_objects:300)
[18920.524371] APFS (dm-0:0): failed to read the ephemeral objects (apfs_transaction_start:384)
[18920.529524] APFS (dm-0:0): no mapping for oid 0x400 (apfs_ephemeral_object_lookup:216)
[18920.532784] APFS (dm-0:0): no spaceman object for oid 0x400 (apfs_read_spaceman:744)
[18920.536222] APFS (dm-0:0): failed to read the spaceman (apfs_transaction_start:395)
[18920.538634] APFS (dm-0:0): aborting transaction (apfs_transaction_abort:1001)
[18920.646469] APFS (dm-0:0): no mapping for oid 0x400 (apfs_ephemeral_object_lookup:216)
[18920.653922] APFS (dm-0:0): no spaceman object for oid 0x400 (apfs_spaceman_get_free_blkcnt:1468)
[18920.766489] APFS (dm-0:0): no mapping for oid 0x400 (apfs_ephemeral_object_lookup:216)
[18920.770446] APFS (dm-0:0): no spaceman object for oid 0x400 (apfs_spaceman_get_free_blkcnt:1468)
No harm was done because the next transaction still got aborted as soon
as we tried to read the spaceman. But, if at some point in the future we
stop fully flushing free queues on each start, we could run into more
subtle bugs involving missing fq nodes.
This all seems unlikely but, to be safe, make the mount read-only when
we fail to get the ephemeral objects. It's really the same situation as
any other abort, so it's not a problem.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
As mentioned in the previous commit message, generic/269 of xfstests
makes the main free queue unbalanced enough that insertions fail.
Clearing the free queue when the transaction starts is not enough to
prevent it, because the whole disaster can happen without a single
commit.
So, start forcing commits when the main queue has too many nodes. I
don't think there is a problem with being too strict here, because a
truly full main queue would be gigantic for most normal filesystems.
Just force a commit when we reach half the limit (rounded up). Of course
make an exception for filesystems with a single main fq node, since
those can't even become unbalanced in the first place and we don't want
to commit nonstop.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Like so many times before, I'm running out of space in the main free
queue. This time the reproducer is generic/269 of xfstests. The queue
isn't really that full, the problem is that it's very unbalanced. We
don't really have any mechanism in place to rebalance btrees, so the
easiest way forward is to just empty the free queue entirely whenever
possible, that is, when a transaction starts.
Of course this means that I no longer hold on to blocks from old
transactions, but I never had any real reason to do that. Also, there's
no longer any point in retrying fq insertions that fail, since it's just
not possible to safely flush any other blocks.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The previous patch reused the number "20" from
apfs_transaction_has_room() inside apfs_main_free(). The next commits
might need to tune that value, so it's better to just define a shared
macro for both.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Running generic/269 of xfstests quickly gets orphan cleanups to hit
ENOSPC, which prints an error message. The problem is that this doesn't
happen just once, but over and over again, completely flooding dmesg for
no good reason.
Retrying cleanups after ENOSPC does have some value, but wait until we
actually made some room. For other errors it's just a bad idea, since
something has gone seriously wrong; we'll usually just get EROFS anyway,
but just don't retry at all.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The intention of my ongoing ENOSPC work is that we should always catch
that error during transaction start (so that we don't need to abort),
but so far xfstests is still beating my checks somehow. That's a bug
though, so start logging an error message to make it easier to spot.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The statfs code was one of the first things I implemented (most likely
before I could even mount the filesystem in any meaningful sense), and I
never tested it much. It was bound to have bugs.
Now that we handle ENOSPC properly (or so I hope), I'm starting to try
my luck with the enospc group from xfstests. The first issue I've
noticed is that generic/275 complains that it "could not sufficiently
fill filesystem". The container is actually very much full, but statfs
reports used space incorrectly: it adds up the blocks allocated by each
volume, neglecting all the blocks that don't belong to any of them.
At the time I didn't know anything about the space manager, so I guess
this seemed like a good enough solution. Fix it now.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
If a function throws ENOSPC halfway through an operation, the
transaction will get aborted so that the filesystem isn't left in an
inconsistent state. This means that the mount will go read-only, and
that the user will lose any uncommitted changes.
So, it's better if we detect ENOSPC before starting an operation. I
tried this before with commit a52b73ed97 ("Check free space before
starting a transaction"), but the code was messy and it would have
required constant updates, which I neglected of course. The problem was
that I wanted to be as accurate as possible with my calculations of
required space, but I'm not sure there is any point.
Instead, require a large fixed amount of space (512 KiB) for most
operations, and a much smaller but still almost certainly sufficient
amount of space (80 KiB) for deletions. Hopefully this will be enough
for users to maneuver around ENOSPC situations. It's not perfect, but no
CoW filesystem can be truly perfect in this regard.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The build is still broken for redhat kernels:
https://github.com/linux-apfs/linux-apfs-rw/issues/89#issuecomment-2691955523
Fix the remaining issues. For the previous patch I was just looking at
the source, but this time I set up a rhel vm and tried a build and a
mount for each of the 9.x kernels, so hopefully it's over. I didn't know
you could get rhel for free as a developer, I guess it makes sense.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The build is broken for redhat kernels:
https://github.com/linux-apfs/linux-apfs-rw/issues/89
This seems to have been the case for more than a year, but nobody had
reported it so far, so I guess the driver isn't getting much use over
there.
We had encountered a similar problem before in commit 1a0b9fb8af ("Fix
build for RHEL 9 kernels"), but back then I thought it was just a
one-off thing. I was wrong: redhat kernels often backport patches that
break internal apis. I don't know how other out-of-tree drivers handle
this, but for now I will have to rely on user reports.
So, go through all RHEL 9 kernels one by one, find out which api changes
were applied to each, and add the corresponding macro version checks to
the driver.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
If the user mixes up the main and tier 2 devices, the mount fails with
confusing errors. We can do better here: the fusion uuid has a bit to
tell which is the main device, so check that early and fail with a good
explanation. While we are at it, also add checks to make sure what we
are mounting is indeed a fusion drive.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
While testing the new "tier2" mount option, I realized that the driver
gives no feedback to the user when a mount option is invalid. I have my
doubts about logging an error here because, while the apfs driver is
inserted, the message may show up for unrelated mounts as well. I think
this sort of issues are unavoidable though, and XFS does log a warning
for invalid options, so do the same thing.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
So far I haven't been able to figure out fusion drives to an extent that
would allow me to implement writes. I'm tired of it, and I think the
effort is probably pointless because apfs fusion drives aren't all that
popular. I might reconsider if people request the feature, but for now
just add read-only support, which is almost trivial.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Make the block device info field of the container superblock into a
pointer. This will make it easier to tell if we are working with a
fusion drive.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
For kernels before 6.5, blkdev_put() needs to be told the mode of the
block device. This information is stored in the vfs superblock, but I
can't access that from apfs_free_main_super() because that function may
be needed early on mount.
My somewhat hacky solution so far has been to guess the mode from the
setting of the APFS_READWRITE flag. This isn't accurate because the mode
is actually determined by SB_RDONLY, but it still seems to work - I
really haven't checked what happens to the mode inside blkdev_put().
For fusion drives, I will soon need to reuse apfs_blkdev_cleanup()
inside apfs_attach_nxi(). In this case the mode is available, so working
with the different definitions of it for each kernel version would make
a huge mess.
Since we now have apfs_blkdev_info for portablity, just keep the mode in
there and let apfs_blkdev_cleanup() retrieve it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Put all information about the container's block device into a single
struct, and move all related code into wrapper functions that work with
that struct. Hopefully this will make it easier to add a second block
device for the tier 2 of a fusion drive.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
When obtaining a buffer head, always let some wrapper specify the block
device. This is just a cosmetic thing for now, but it will be important
when dealing with fusion drives.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The header comment from apfs_query says that it only gets used for the
catalog and the object map. That claim is extremely out of date, of
course. Get rid of it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The paper I followed in the early days of the driver (before the
official reference was published) referred to record values as "data",
so some of my oldest code uses that name as well. This is inconsistent,
and not very clear because the word "data" shows up everywhere. Reword
all instances of this I can identify.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
For safety, I think it's always better for a filesystem to reject any
malformed data it can notice. Omap and free queue keys have a fixed
size, so the size check we run to prevent out-of-bounds reads, while
correct, should be made more strict. This is what we already do for most
record types these days, both keys and values.
Take this opportunity to improve the code style here and make it more
consistent.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
There is no "paddr" argument to either apfs_take_single_extent() or
apfs_put_single_extent(). Fix this.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Provide a proper error message on dmesg when a user attempts to create a
snapshot with a name that's already in use, otherwise "File exists" may
be a bit confusing.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
A user has encountered a volume with the dataless snapshots flag:
https://github.com/linux-apfs/linux-apfs-rw/issues/86
I'm not really sure what "dataless snapshots" are, and so far I have
failed to trigger them under macos. I can set the
SNAP_META_PENDING_DATALESS flag in a snapshot myself, and macos will
call it "dataless", but their fsck doesn't complain no matter what I do
with the other fields, so I can't tell what is expected.
I can't imagine any risk in mounting such a volume read-only, so allow
that much from now on. Keep writes banned just in case, and avoid
mounting the dataless snapshots themselves, since I have no idea what
that entails.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
While testing eda21f33e9 ("Check the superblock magic before the block
size"), I noticed that early log messages such as those don't include
device information, just the filesystem type. The problem is that the
s_id field of the vfs superblock is only getting set after the call to
apfs_read_main_super(); there is no real reason for this, so invert it.
While paying attention to this field for the first time, I realized that
I'm making a huge mess of it. Most filesystems use a "%pg" format
specifier: this prints the name of the block_device pointer passed as an
argument, so the logs are very clear. Meanwhile I was just printing the
dev_t... and putting a "g" afterwards? I guess I somehow got confused
while trying to copy the others. Do this right.
That said, an apfs superblock is not uniquely identified by its device:
we have volumes and snapshots as well. The comments in the super_block
structure claim that s_id is just an "Informational name", so I guess I
can set it to whatever I find useful. Provide the volume number and
snapshot name (if any) as well.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Github doesn't support rst notes, so it just looks silly over there.
Make it into a regular paragraph.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Some users seem unaware of the existence of packages for the driver, and
I guess that's my fault because the (outdated) readme doesn't mention
them at all. Add a small note about it in the beginning of the "build"
section.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
These days I'm far more careful about testing multiple kernel versions,
so the warning in the second paragraph of the readme ("only a few of
those have actually been tested") is a bit excessive. Simplify that
stuff. Don't bother mentioning the support for 4.9-4.11 kernels, it has
some performance issues and they are old enough that it probably doesn't
matter much.
The rest of the paragraph is about bug reporting. Move this into a
separate section, so that it's easier to find, and to give me room to
explain the use of dmesg.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The driver is not that bad these days. Corruption is still a real risk
of course, but when it happens it's a bug related to some untested
corner case (like the recent issue with filesystems above 1.32 TiB), not
expected behaviour. So tone down the warnings, but make it clear that
it's not all that safe yet.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
A distro packager has requested writable mounts by default, and is
willing to take responsibility for any potential harm to their users:
https://github.com/linux-apfs/linux-apfs-rw/issues/81
Add a build flag to enable this, but leave it undocumented to avoid
confusing anyone.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
When an attempt is made to mount something other than an apfs container,
the driver currently throws a confusing "bad blocksize" error to dmesg.
I would expect this to be encountered fairly often, when users try to
mount the disk as a whole instead of the apfs partition.
Start checking the superblock magic before the block size so that we can
give a more helpful error message.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
This release fixes a serious data corruption bug for containers bigger
than ~1.32 TiB, which is immediately triggered by a writable mount. This
goes to show why mounts are still read-only by default.
Another much smaller data loss bug for much bigger containers (~7 TiB)
is also addressed.
Support is added for the 6.12 kernel release candidate. A bit early this
time, so let's hope the build doesn't break again for the final release.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Building an old kernel (5.3) with an old version of gcc (4.9.2), I get
some warnings about "comparison of distinct pointer types". I'm pretty
sure this doesn't matter, and I don't even know if the blame lies with
the old compiler or the old kernel, but just silence this by making the
integer literals of the warning unsigned like the other argument.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
This has been around for a while so I guess checkpatch isn't picking up
on it for some reason.
[eafer: took this from a bigger patch, added commit message]
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The build is broken for the 6.12 release candidate. The problem is that
->write_begin() and ->write_end() now take folios as parameters instead
of pages. The same is true, of course, of the associated
__block_write_begin() and generic_write_end(). Update the driver.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I never fully understood how the ip bitmap list was supposed to work,
but now I'm noticing that the difference between what I do and what the
official driver does are bad enough to quickly corrupt the list when a
container moves back and forth. Of course this is only an issue for
filesystems above 1.32 TiB or so, but it's still pretty serious.
So, I thought about it harder and I'm pretty sure I finally figured it
out: the array entries for each free bitmap are actually kept in a
linked list, that starts in sm_ip_bm_free_head and ends in
sm_ip_bm_free_tail. All entries outside the list (that is, the entries
for the used blocks) are marked with the invalid index 0xFFFF. The tail
itself also gets 0xFFFF even though it's free, because it has no next
entry - so that accounts for the constant off-by-one weirdness in the
number of free blocks.
So, implement all this stuff correctly. Stop assuming that all ip
bitmaps in use are consecutive, and stop rotating them all in lockstep
as soon as the transaction begins. Instead, read all ip bitmaps into
memory during the first transaction and keep them there, only tracking
if they have been dirtied. At commit time, allocate new blocks for the
dirty bitmaps and write them back to disk.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Currently, the driver doesn't allow multiblock ephemeral objects to wrap
around. When an object doesn't fit in the checkpoint data area, we just
leave some empty space and adjust the mappings accordingly. This seemed
like a simple and reasonable solution to me, but the official driver
actually demands that all ephemeral objects are kept consecutive (and in
the same order as their mappings, which we already do).
In practice this means that if you are working with a container with a
multiblock spaceman (say 7T in size), and if the unmount transaction
just so happens to be the one where the spaceman doesn't fit, then you
will be left with a container that is seen as corrupt by the official
driver. To make matters worse, checkpoint integrity is checked on mount,
so macos will actually mount the container no problem, but silently
discarding my final transaction.
This is all, of course, extremely unlikely. Such large containers are
rare, and checkpoint data areas for them are huge so a wrap-around takes
a while to happen. And it needs to happen precisely during the unmount
transaction, and that same transaction has to involve important data the
user doesn't want to lose. All in all it's not a serious issue, but get
it fixed.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
This release prevents deadlock if a single volume gets mounted and
unmounted at the same time. It also fixes two other (smaller) mount bugs
found while testing this one.
Support is added for the upcoming 6.11 kernel release, with a patch by
Woody Suwalski and a later patch of my own because things broke a second
time.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Originally all mount options were parsed inside apfs_fill_super(),
because I was following the example of other filesystems (such as ext2)
that have no subvolumes or snapshots. When I implemented those features,
I realized that some mount options would be needed early on mount, so
that sget() could use them to decide if the superblock is already
mounted.
My lazy solution at the time was to leave parse_options() as it was, but
also add some redundant code early on mount to read the needed options.
Just looking at the amount of TODOs in those functions makes it clear
that this was always very silly. And now that the locking during mount
has been simplified, this is causing a race, and I want to take that
stuff seriously.
The bug is that while parse_options() rereads the mount options needed
by sget(), there is nothing preventing another concurrent mount from
testing the superblock. Say the first mount is for volume 1: the volume
number gets briefly reset to the default value of zero, and if the other
mount actually wants volume 0, sget() will consider this a match.
Anyway, split parse_options() into two equally clean functions, one for
early mount and another to get called within apfs_fill_super(). I even
considered simply parsing all the options early during mount, but that
would require a few more changes because the nxi is not attached at that
point. Maybe some other time.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The build has become broken once again for the 6.11 kernel:
https://github.com/linux-apfs/linux-apfs-rw/issues/75
The problem is that the second release candidate introduced generic
MAX() and MIN() macros for everyone to use, and that crashes with our
own definitions. So, only define them for older kernels.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The omap cache has been in place for years, and it seems that its
spinlock was never actually initialized before use. I have no idea what
the consequences of this are, but for one it does cause lockdep to turn
itself off after every mount. Fix it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
For the previous patch I had to carefully test all the possible errors
during mount and unmount. I noticed that injecting an error inside
apfs_set_super() triggers npd. The cause is that the error code returned
by sget() gets swallowed and the kernel is allowed to think that the
mount succeeded.
This is not a new issue, it's been around since apfs_mount() was first
implemented. I never noticed because apfs_set_super() would only fail if
we ran out of anonymous devices. So fix it in a new commit.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Repeatedly running generic/604 of xfstests eventually hangs the kernel.
This is a test that forces a mount to race with an unmount. The problem
is a standard deadlock: ->put_super() gets called while the s_umount
lock is held, and then starts a transaction, which takes nxs_mutex;
meanwhile ->mount() first takes nxs_mutex and then calls sget(), which
takes s_umount.
Before this patch, I had never really put much thought as to where
exactly nxs_mutex is needed. It's obviously the lock for the list of
mounted containers, but it's also used as a generic way to avoid
modifications to a container while another of its subvolumes is getting
mounted. It's always been obvious that I'm holding it for too long and
in too many places, so I want to finally put some effort into figuring
this out now, and leave a record here so that I don't forget.
The first obvious point is that the mutex can be dropped before calling
sget(), and retaken later. There should be no problem if another mount
called it at the same time, and it's enough to fix the bug. But putting
some thought here exposes an existing issue with this code: sget() may
actually test some superblock that is already half-unmounted and had its
info freed, so apfs_test_super() is risking npd or use-after-free.
Looking at other filesystems I realize that my mistake was freeing the
info inside ->put_super(), because at that point the super is still
listed in the fs_supers list for apfs. Instead ->kill_sb() should free
the info itself, after generic_shutdown_super() is done (which calls
->put_super()), and after kill_super_notify() has taken care of the
list. Since kill_super_notify() can't be called from inside a driver, a
little hack is needed to call it through kill_anon_super(). The cleanups
for apfs_mount() also need to be reconsidered, since ->put_super() is
only called by deactivate_locked_super() after the root dentry is set,
but the rest of ->kill_sb() runs either way.
Going back to the mutex: even if a race for sget() itself is not an
issue, dropping it there does have other consequences. The reference
count for the container superblock is increased before sget(), and it's
later checked on some initializations to see if this is the first mount.
That won't do anymore, because a second mount could increase the refcnt
before the mutex is retaken, and the first mount would never complete
its work. Use other fields for this check: in apfs_read_main_super() use
the blocksize (which is zero before the first mount sets it); for
apfs_set_nx_flags(), define a new flag to tell if the other flags have
been set.
This already tells us two functions that definitely need to be protected
by the mutex, so that they can complete their initializations before any
other mount comes to check them. The situation is similar for
apfs_first_read_omap(), which goes through the container's volume list
to check for previous reads. After going line by line through the mount
code, I'm fairly confident that the mutex only needs to be held for
apfs_attach_nxi() and these three functions. For unmount it's the same:
apfs_free_main_super() and apfs_put_omap() require the mutex. The second
will now unset the omap pointer for the volume atomically, so that it
can be checked by apfs_first_read_omap(); it is renamed accordingly.
So far this is all about the races between different mounts and umounts;
but we also don't want the mount to read inconsistent data from disk
because some existing mount happened to write to it. Back when the
driver could only mount one volume at a time, apfs_fill_super() didn't
need any locking, so when multiple mounts got implemented I decided to
just ban transactions during mounts entirely, by taking the nxs_mutex.
This double lock looks very awkward and is a factor on the deadlock we
are fixing, so let's clean that up now: use the read semaphore itself to
protect apfs_fill_super(). Of course this means that two mounts for the
same container can now run through this function at the same time, so be
careful with changes to the shared nxi: we've already dealt with this
issue for apfs_set_nx_flags(), but apfs_set_trans_buffer_limit() might
also be unsafe. Just in case, move it to apfs_read_main_super(), it
makes more sense that way anyway.
By the way, apfs_read_main_super() also reads from disk, but it only
does it on the first mount, so there is no risk of racing against a
transaction. There is one function that writes without a transaction,
namely apfs_make_super_copy(), but that function takes the nxs_mutex to
ensure that no mount will begin during the superblock update.
I think that covers everything? I'm really not sure if this writeup was
clear at all, I have a hard time reasoning about locks. I also put a
simpler list of the uses of nxs_mutex in a comment next to its
declaration, which should be more practical as a reference.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I just picked up a patch to add support for the 6.11 kernel. I only did
a shallow review, and now I notice that the code was reordered and a
null buffer head is getting dereferenced. Revert that.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Transactions are now committed regularly, so it should no longer be
strictly necessary to unmount the filesystem cleanly for writes to take
effect. It's still recommended, of course.
The release also fixes two bugs. One was a big performance regression
from 0.3.9, seen when deleting large numbers of files. The other was
that mounts would go read-only if two snapshots were attempted with the
same name.
Support is added for the new 6.10 kernel version, with a patch by Woody
Suwalski.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Attempting to create two snapshots with the same name will abort a
transaction and leave the filesystem in a read-only state. From the
comment it seems that I was aware of this issue when I first implemented
snapshots, but I guess I didn't like the obvious solution in this patch
at the time, so I decided to leave it for later.
Since taking snapshots is not expected to be a bottleneck, just run the
query for the name once more ahead of time, as soon as the transaction
opens. Now if the name already exists we don't need to abort, because no
changes have taken place.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Currently my driver only commits transactions "synchronously", that is,
immediately following a user action that triggers it. This implies, in
particular, that the filesystem must get unmounted explicitly or else
data will almost certainly get dropped. While technically valid
behaviour, this is very weird and some people have complained about it
in the past.
So, stop ignoring requests for commit via apfs_transaction_commit() when
they are considered premature. Instead schedule a commit task, with a
small delay to avoid constant commits when the driver is still busy.
Note that the previous two patches were intended to help with this one:
the plan was that the commit task wouldn't need to hold a specific
volume superblock, and could instead work with the nx info. In the end
this proved too messy, so I'm just saving the volume superblock that
requested the commit in the transaction struct.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The limit on the number of buffer heads in a transaction depends only on
the ram size, so it's the same for all volumes. Move it to the container
superblock info. Frankly this could just be a global variable, set when
the driver is first inserted, but there is no problem with recalculating
it on all mounts.
The point of this change, along with the previous patch, is that soon we
won't require holding some volume to start and commit transactions.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
When I first implemented transactions, my intention was to revert the
in-memory filesystem structures to a consistent state whenever we hit
the kind of errors that can be expected during normal operation. So I
always kept some stuff from the previous transaction around to copy it
back if needed. I never tested any of this though, so it most likely
never worked.
Later improvements mean that the kind of silly errors I had in mind
(things like ENOENT) are no longer supposed to happen during
transactions. The one exception here is ENOSPC, but that's only because
I haven't yet bothered to clean up the space reservations. The errors
that do happen are things like ENOMEM, EIO or EFSCORRUPTED, and I think
it's probably ok if the mount doesn't work perfectly after one of those.
Instead we force it into a read-only state, and let the user decide how
to proceed.
So, there is now little point in keeping my original half-baked plans
littering the transaction code. Remove all that stuff.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The container superblock info keeps a list of inodes and buffers that
have been modified in the current transaction. This is a bit of a hack,
but so far I haven't figured out a way to get rid of it. The lists need
to be initialized before they are first used, and this is currently done
by apfs_transaction_start() itself, after checking if this is the first
start since the last commit. This works fine, but it's really weird,
since the list gets regularly reinitialized to "empty" after getting
emptied naturally by a commit. Initialization should really be separate
from all the code that actually works with the transaction.
Define a new apfs_transaction_init() to take care of this, and call it
from apfs_attach_nxi() which already takes care of the initialization of
all other container info fields.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
TRANSACTION_MAIN_QUEUE_MAX, TRANSACTION_BUFFERS_MAX and
TRANSACTION_STARTS_MAX were originally desfined inside transaction.c, so
I guess I didn't bother to give them the driver prefix. I don't expect
the names to collide with anything, but it's tidier to follow the same
naming conventions everywhere, so run
sed -i 's/TRANSACTION_/APFS_TRANS_/' *.{c,h}
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The iozone automatic tests suffered a big performance regression between
0.3.8 and 0.3.9, in my case from around 14m to close to 20m. The problem
goes away if the newly introduced apfs_dstream_delete_front() is tweaked
to delete files in larger chunks. That said, I don't think my choice for
TRANSACTION_MAIN_QUEUE_MAX was ever based on anything, so it's better to
adjust that instead of coming up with some hack for file deletion alone.
After a lot of testing, I think I get the best results if I raise it
from 4096 to 10000. The iozone benchmark is now better than before, at
around 13m for me.
Of course this will mean that transactions commit less often. I should
look into putting that in a workqueue like the file deletion.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
This release fixes the build for the soon to be released 6.9 kernel.
Support is tentatively added for the new 0x80 incompatible volume
feature, which appears simple enough so far and isn't causing any
problems.
A console warning that showed up during compressed file reads on recent
kernels is now fixed.
Support is improved for tiny filesystems of under 100 MiB, though they
remain awkward to test due to fragmentation issues.
The behaviour of apfs_modified_by is tweaked to better match the
official driver.
Free queues should now be much harder to fill up. The most important
change here is that files are now deleted across multiple transactions.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The official newfs_apfs now sets a 0x80 incompatible feature in all
volumes by default. The feature is undocumented as far as I know, but
after some testing it would seem that its effect is to set a flag in
preallocated extents. I can't imagine why such a feature would be
incompatible instead of read-only compatible, so there is a chance that
I'm missing something important, but for now enable support and let's
see if anything breaks.
By the way, no real changes are needed here because we don't preallocate
extents ourselves, and we put existing preallocated extents through the
same CoW as all the others. The only problem remaining is that writes
after the end of a file will not flip the "preallocation" flags that may
be set in previous blocks, but neither the official fsck nor the driver
seem to care about this. I just don't get what this flag is for, which
is a little scary of course.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I've recently noticed a number of new flags and added them to the fsck.
I heven't quite figured out most of them yet, but copy the information
here anyway. Also add checks for the new incompatible features, so that
they don't get accepted by default.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
If the latest newfs_apfs creates an image with the default options,
older versions of fsck_apfs complain about an unknown volume superblock
field called "apfs_doc_id_index_flags". Several such new fields appear
to have been added, and after some bit flipping I managed to retrieve
the names for all of them, except for "apfs_doc_id_tree_oid" which is
only a (probably correct) guess.
I have no plans for these fields right now, but document them all in
apfs_raw.h anyway, so that I don't forget.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Orphan cleanup explicitly reports the error code from apfs_iget() when
it gets swallowed, but it's always ENODATA so it looks silly. Leave the
message in place but ignore the error code.
Instead, report all error codes returned by apfs_clean_orphans() before
they get discarded by apfs_orphan_cleanup_work(). That information might
actually be useful at some point.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The official reference is a little ambiguous about apfs_modified_by. On
its first introduction it says the following:
If the implementation's information is already the last entry in this
field, you can update the field as usual (creating a duplicate), or
leave the field's value unmodified. Both behaviors are permitted.
So far the driver has followed these instructions and left the field
unmodified, but later on the reference describes the apfs_modified_by
timestamp as:
The time that the program last modified this volume.
This seems to mean that the timestamp must always change, even if the
implementation in use has not. More importantly, on the matter of the
cloneinfo xid, the reference says the following:
When unmounting a volume, the value of this field is set to the latest
transaction identifier, the same as the apfs_modified_by field.
So apfs_modified_by must always get updated on unmount, or so it seems.
I looked at older checkpoints for a few official images, and indeed they
always seem to do this on unmount, and never before. Always by shifting.
Stop lawyering about the specification and just copy what the official
driver does.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I keep running into situations where too much fragmentation gets a free
queue full, and the filesystem goes read-only with the "free queue has
too many nodes" message. I've been patching this up by trying to flush
more often, but in the end it's best to force an extra flush whenever we
run out of space.
Rework apfs_free_queue_insert_nocache() so that it checks for space
before actually inserting anything. If we are out of room, flush and
start again. Make this last-resort flush stronger with a new "force"
argument for apfs_flush_free_queue(), otherwise it might still be
insufficient. Also, stop preserving multiple transactions in the queues,
I don't really see the point and it makes all this stuff much worse.
With these changes I'm hoping that free queues will no longer fill up
in practice, but it's still possible in theory, so leave an error
message in place to identify this issue quickly.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I've been trying to run tests with a small image of around 100 MiB. I
wasn't expecting any problems, but surprisingly a lot of the tests fail.
The difference with larger containers appears to be that my hack to
avoid fragmentation (allocating data and metada in different chunks)
does not work when you only have one chunk. So this bug actually affects
any image, it's just easier to trigger here.
So, as for the bug itself: when working with large dstream xattrs (like
my apfs/002 test does), I regularly hit the "extent is not tail - bug!"
error. The problem is one that I've faced many times before with my
messy query code: queries looking for an extent without knowing it's
exact logical address can't be "exact" so they don't reliably return
ENODATA when they find nothing of value.
To deal with this, the extent code always checks the retrieved record by
calling apfs_query_found_extent(), but that only checks the record type.
In this particular case, the query is finding a proper file extent
record, but it belongs to a different dstream. I originally didn't think
such things could happen, but that has proved wrong more than once. From
now on, make apfs_query_found_extent() check the extent id as well.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
After the last patch I'm regularly getting the following error message
in the console:
APFS (800010g): index out of bounds (0 of 0) (apfs_node_locate_key:598)
What happens here is that the container xid is set to 1 by the mkfs, but
the ip fq is empty so the oldest xid is set to 0. So when the driver
first starts a transaction, apfs_flush_free_queue() will believe that
there is an actual "0" transaction to flush, and end up trying to update
the oldest xid field for the empty queue.
This is all a bit odd but not a real problem, as soon as the fq starts
getting used all the flush checks will work as expected. The annoying
message comes from apfs_free_queue_oldest_xid(), which directly calls
apfs_node_locate_key() for the first key in the root node, without
verifying if such a key even exists. Avoid that.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Currently apfs_flush_free_queue() preserves several transactions in both
the ip and main free queue. This is a problem for tiny filesystems (5
MiB, for example) because the ip only has 6 blocks and inevitably uses 2
per transaction, getting full almost immediately. Allow more recent
transactions to get flushed for the ip.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
As mentioned in the code, my implementation of apfs_compress_readpage()
was mostly copied from ext4_read_inline_page() because it was simple
enough and I didn't know what I was doing. As it turns out, I picked the
wrong function to copy: ext4_read_inline_page() only reads in-memory
data and doesn't go to sleep. We do read from disk, and we do go to
sleep, so newer kernels flood the console with warnings.
Replace kmap_atomic() with kmap(). The only expected negative effect of
this change would be worse performance on 32-bit archs, but I haven't
even tested those in years. The alternative kmap_local_page() is not
available in old kernels.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The driver requires setting an explicit "readwrite" mount option for an
actual writable mount to take place, otherwise the mount is silently
switched to read-only. The point is to prevent users that don't
understand the risks from potentially corrupting their data. Hopefully
this would push them to check the readme, but some might not even notice
that we have one (for example, if they got the driver as a distro
package). Try to explain the situation in the console.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The first 6.9 release candidate is out, and bdev_open_by_path() is gone.
We are no longer supposed to work with block device handles, we must use
a normal open file struct instead. Update the code following what was
done to btrfs in upstream commit 9ae061cf2a46 ("btrfs: port device
access to file").
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The first 6.9 release candidate is out, and the SLAB_MEM_SPREAD flag is
no longer available for kmem_cache_create(). Apparently it has been a
no-op for a while, so the least invasive way to fix the build is to just
define it as zero.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Gilgamesh reports via email that the following iozone benchmark is
failing:
./iozone -a -i 0 -r 1M -n 1g -g 1g -f /mnt/apfs/test
I've tried this myself on a small (3 GiB) filesystem, and it is the
commit after the unlink that fails, complaining about the main free
queue being full:
APFS (800010g): free queue has too many nodes (348 > 347) (apfs_free_queue_insert_nocache:849)
The problem is that random writes to a big file create a lot of extents,
and when the file is deleted they all get dumped to the main free queue
as separate records. The queue gets full and the commit fails.
Stop forcing file deletion to happen in a single transaction. Instead,
set up a workqueue whenever an orphan inode gets evicted, and leave the
cleanup for another time. Also allow the cleanup to execute only
partially (deleting the first extents like the official driver) and
reschedule itself (again via inode eviction).
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
When a query fails with ENODATA, the caller may still need to work with
the result (for instance to insert something in that location), but it
should never be confused for an actual valid record. Some changes I'm
working on trigger this problem regularly for apfs_query_found_extent(),
making it check stale records and report success. I need that function
to be reliable, so always clear the key and value lengths from queries
on failure.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
This release reworks ephemeral objects to support larger containers of
up to ~7.8 TiB, and to allow the creation of checkpoint mapping blocks.
Also the build is no longer broken for the new 6.8 kernel, thanks to a
patch by Woody Suwalski.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
In pretty much all containers I've encountered so far, every objects has
a single block. There is an annoying exception though: containers with a
size between ~6.7 TiB and ~7.8 TiB have a multiblock spaceman. I have no
simple way to deal with this because my transactions currently handle
ephemeral objects very roughly, ignoring all the cpm metadata and just
updating each block in the expected header location.
Instead of trying to hack this in somehow, start doing the right thing
as explained in the official reference and keep all ephemeral objects in
memory at all times. So, when the first transaction starts, go through
the cpm metadata and read each object along with its size and oid. When
a new object gets created (or an old one gets deleted), all changes
happen in memory. On commit time, checksums are updated and all objects
get written to disk, but nothing neads to be read.
So this is all much cleaner, and I took this opportunity to support
multiple cpm blocks as well.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
It's annoying to have to make a new commit just for this, but I had
already pushed the bad one...
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Failure to mount fusion drives currently gets reported in a confusing
way:
https://github.com/linux-apfs/linux-apfs-rw/issues/39#issuecomment-1947466697
The problem is that all feature checks are currently run at the same
time for both the container and the volume, after the volume superblock
has been mapped. Fusion drives never get that far, we can't even work
with the container's object map.
Split the feature checks into two functions, one for the container and
one for the volume. Leave the call for the volume in the same place, but
move the one for the container into apfs_map_main_super().
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
This is a small release that fixes the dkms build, recently broken by
the implementation of version reporting. From now on I will run a dkms
build before each new tag.
The release also lifts the ban on filesystems with half-written backup
superblocks, a problem that some users appear to have encountered
already.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Some users might build the driver from the tarball that github produces
on release, so the reported git commit would be an empty string. Instead
extract the version from the dkms file and use that, but add a question
mark so that I know that it might not be accurate.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The dkms build fails for the latest release:
https://github.com/linux-apfs/linux-apfs-rw/pull/60
The problem is that dkms does not call my Makefile at all, so the
version header is never generated. Dkms does have a PRE_BUILD hook
though, so use that to call a script that takes care of this.
From now on, I must always remember to run a dkms build before a new
release.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The backup superblock is only needed to locate the actual superblock, so
it doesn't matter if it's corrupted beyond the first sector, which the
disk is supposed to write atomically. If a crash happened at the right
moment during unmount, any implementation might end up with a
half-written superblock where the checksum doesn't add up, but that's
perfectly fine.
If this is encountered during mount, just report it and proceed. This is
not a theoretical concern, someone appears to have run into the problem
already:
https://techsparx.com/linux/ubuntu/mount-apfs.html
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
This release heavily reworks the node defragmentation and split logic to
fix a handful of rarely triggered data corruption bugs. The new version
is easier to understand and hopefully more robust.
Support is added for containers with a size of up to ~6.7 TiB, improving
the previous maximum of ~1.3 TiB. The next release will probably keep
pushing this limit.
Changes are made to the way that subvolume "devices" are reported. Now
we are closer to matching the behaviour of btrfs and userland tools like
Nautilus are far less confused.
The driver version information is now provided by both the on-disk
filesystem metadata and by modinfo. This may be useful for responding to
bugs at some point.
A few small issues with snapshot reporting have been addressed, as well
as an mmap-related warning in recent kernel versions.
Finally, support is added for the 6.7 kernel version (released today),
contributed by Woody Suwalski.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I'm in a rush to get the tag out today and I'm making a mess. The
previous patch set an error pointer to null in apfs_node_split(), but I
didn't notice that the actual error would get read later. Reorder this.
I usually would just squash this into the previous patch, but in my rush
I have already pushed it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
We mistakenly free an error pointer if apfs_create_node() happens to
fail. Set the node to null before the cleanup, but I guess I should try
to avoid error pointers in the future.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Running generic/013 for a few minutes still leaves the filesystem in a
corrupted state, with a missing inode record. This is another regression
caused by my big rework of the node split logic. When a single-record
node is needed but the parent update fails with -EAGAIN,
apfs_create_single_rec_node() leaves the query pointing to the aborted
new node. The refresh does not change this, so the inode record ends up
abandoned in this ghost node.
In then end all of this is caused by the weird way I originally designed
the query struct, which forces me to always keep a more or less valid
leaf query instead of just nuking the whole thing and starting over. I'd
like to change this eventually, but for now just adjust the cleanup for
apfs_create_single_rec_node().
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The previous patch introduced a regression in generic/013, seen when the
test is allowed to run for 20 minutes or so. The result is a corrupted
filesystem where one inode appears to have too many dentries, and
another inode has too few to balance.
The problem is that query refreshes just rerun the query once and expect
the correct result, but dentry queries for an existing record actually
need to be called repeatedly to deal with hash collisions. For
normalization-sensitive filesystems (which I use for most of my tests)
"collisions" are constant because there is no hash. So if a split
happens during a dentry deletion, chances are very high that the refresh
will end up pointing to the wrong record.
Don't add awkward code to deal with this to apfs_query_refresh().
Instead transform the multiple query from apfs_dentry_lookup() into a
regular exact query before returning, setting the key to the
unnormalized name. The refresh code will have no trouble working with
that.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I'm encountering some rare failures in generic/075. The problem happens
when a node split is triggered by inserting a record into the first
position. The parent needs to get updated of course, but we've lost the
reference to it after the split, so the filesystem becomes corrupted.
The reality is that this stuff has always been too hacky, and new corner
cases keep popping up. Stop trying to guess which queries need to know
and might have forgotten their ancestors: just restart the whole btree
operation after any split, refreshing the query with a simple rerun.
This has to be the safest possible approach, and I don't expect it to
hurt performance, since node splits should never be a bottleneck.
Big changes are needed to get it to work though, because the b-tree must
be in a consistent state whenever the query gets refreshed. There are
two different considerations here. First, no split can take place unless
the parent has enough room to index the new child node; otherwise the
parent split would trigger a query refresh while half the child's
records are orphaned. To sum up, every cascade of node splits must
happen from root to leaf.
The second consideration is that, even if a node has enough room, no
updates related to the actual requested operation (the leaf record
insertion/deletion/replacement) can take place at any level until all
splits are done. A different policy would mean that a query refresh
might become necessary halfway through the operation, and so the leaf
record could be found to be unexpectedly missing, or covered by the
wrong index key.
So at a given level: the split, if needed, always comes first, followed
by the function call for parent updates, and only then the node gets
modified. After a split an EAGAIN error gets thrown, which then bubbles
back up to the leaf (either through the splits themselves, or through
the parent update calls), so only a single split can happen for the
whole b-tree on each refresh iteration - always at the highest possible
level in the split cascade that would have been triggered before this
patch. The btree, of course, remains valid on EAGAIN, and the leaf
records remain the same.
Inside apfs_node_split() itself, no changes to the old node can be done
until the new one is safely inserted in the b-tree. If that triggers a
split at a higher level, then the new node must be properly cleaned up
(making use of the recent changes to apfs_delete_node()) and, as usual,
EAGAIN must get bubbled up.
A similar cleanup needs to be take place in the one other function that
creates non-root nodes: apfs_create_single_rec_node(). The new
single-record node must be repeatedly destroyed until the parent manages
to index it without any splits.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Implement apfs_query_root() and use it to simplify two functions. It
will get reused further in the following patch, including inside node.c,
so don't make it static.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Allow NULL node pointers to be "freed" with apfs_node_free(). This will
simplify some cleanups soon.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Currently node insertions/deletions are attempted immediately and they
return -ENOSPC when there is no room, letting the caller know that the
node needs to get split. But in future patches I'll need to reorder the
operations so that the node only gets updated after all the ancestors.
The previous patches already ensured that apfs_node_replace() will never
erroneously report lack of space, no matter how fragmented the node may
be. This was already true of apfs_node_insert(). So from now on we can
safely run space checks ahead of time with only the node metadata, use
that to decide if a split is needed, and only update the records after
all that is done.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
In the near future we want to make sure that apfs_node_replace() will
never fail as long as it has enough space. The defragmentation from the
previous patch is not enough for this, because it still leaves the
replaced record in place instead of moving those useless bytes into the
free area.
To solve this as simply as possible, set the table-of-contents entry for
the replaced record to zero length, for either the key or the value (or
both), depending on which we are replacing. The defragmentation code
will take care of the rest. Of course this weird toc entry remains in
place if the function fails, but it will be replaced eventually so it
should not matter.
By the way, this hack cannot be used on trees with fixed key/value
sizes, because those sizes are simply not reported in the toc. Of course
records in such trees can generally be replaced in place, so they won't
need defragmentation. There is one exception though: the ghost records
of the free queue. Luckily we don't currently call replace on free queue
leaves, so we can safely ignore this problem. Leave an assert in place
so that I don't forget.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Like we already do in apfs_node_insert(), attempt to defragment the node
before returning -ENOSPC in apfs_node_replace(). This shouldn't make
much of a difference right now (other than packing the records a bit
tighter together), but in future patches it will help us prevent
apfs_node_replace() from ever failing in normal operation.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Define a new apfs_node_update_toc_entry() to be used when replacing
records in a node. This makes apfs_node_replace() somewhat cleaner, but
more importantly it will allow that part to be reused in future patches.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Improve the documentation of the record allocation functions to make it
clear that they may return -ENOSPC, because that case will often need to
be handled separately in the future.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Some changes I'm working on will require being able to delete nodes that
have not yet been indexed in a btree. This cannot be done with the
current version of apfs_delete_node(), because it handles the parent
node modifications itself. So from now on, let the caller handle that if
it's necessary. This is also more symmetrical with apfs_create_node(),
as would be expected.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
When apfs_node_locate_data() gets called for a ghost record (that is, a
record without a value), the function correctly returns the zero length,
but the offset is left unset, and so a stale number may remain in the
query structure. This does not matter of course, because a zero-length
value will never be read, but I've found it very distracting while
debugging a separate issue with query offsets. Just set it to zero.
To be consistent, do the same in apfs_node_insert(), though I haven't
been bothered by this one yet.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I've been working on making some very needed changes to how node splits
work. The plan is to always rerun queries after a split, to keep the
query chain valid. One problem I've encountered while implementing this
is that the search key is not really preserved by apfs_update_inode().
We have a pointer to it, but it was allocated in the stack of some other
function, so the data is just noise.
I've always thought it was silly to allocate both a query and a key
before a lookup. Instead, turn the search key into a field within the
query structure, and copy it to the children as the query gets run. This
will ensure that the key remains available for future reruns.
One thing to note is that, while this simplifies most callers a little
bit, some of them still allocate a key in the stack and later copy it to
the query. I didn't want to make any big changes and risk introducing
bugs at this point, since I'm busy with other stuff.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Like I mentioned in the previous patch, df is refusing to acknowledge
multiple mounted volumes or snapshots for the same device, and it's
reporting only one. Luckily the source code for df is straightforward,
so I figured out that it calls stat() on the mount point to find the
device number instead of just accepting the one reported by mountinfo.
This way each mounted volume/snapshot can choose to report a different
number, instead of just going with the one set in the superblock.
After confirming that this is indeed what btrfs does, implement it here.
This fixes the df issue, but sadly it doesn't fix all problems with
thunar. It can tell the different mounts apart, but for some reason I
need to close it and reopen it for that to happen (in fact I now realize
that it could already do that in the previous commit). Since btrfs does
have plugins for udisks and such, maybe this is not the driver's fault,
but I'm not sure.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Nautilus and other file managers fail to unmount apfs volumes:
https://github.com/linux-apfs/linux-apfs-rw/issues/58
This has been broken for years, since commit 4a6c0e2b77 ("Support
simultaneous mount of multiple volumes"), but I never noticed because I
always use the command line, and there are no problems either there or
with libmount.
The error message ("Device `%s' is not mounted") appears to come from
udisks. I don't know much about this stuff, but from tests to my driver
it seems that userland expects /proc/self/mountinfo to report the device
number for the mounted device, not just the name. The tricks we do with
anon devices to support multiple mounts cause a confusion.
Stop using anon devices. Instead set the real device number on the
superblock, and ignore it for all purposes other that mountinfo
reporting, which is done by the common code.
This should solve the reported bug, but causes other weird behaviour.
For instance, if I mount the current transaction and later mount a
snapshot, both df and thunar refuse to acknowledge the existence of the
first mount. I don't know how btrfs deals with this, but they seem to
have plugins for udisks and libblockdev so it's probably not as simple
as a driver change.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I just noticed that the snapshot name option is not getting output in
/proc/mounts. I guess I just forgot to implement this, do it now.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I just noticed that df throws a "No data available" error if it's used
while a snapshot is mounted. The problem happens while gathering the
total number of used blocks: to iterate through the existing volume
superblocks, this requires working with the container omap, which we
otherwise only do for snapshots during mount. apfs_omap_lookup_block()
is written with the idea that snapshots will only call it for the volume
omap, and so it uses the snapshot xid to get the wanted (old) version of
the object. But for used space calculations we actually want the latest,
and in fact the container omap doesn't even have the old versions of the
volumes, so the call fails.
Reimplement apfs_omap_lookup_block() as a wrapper around a new
apfs_omap_lookup_block_with_xid() that uses the current snapshot
transaction, and use this to implement apfs_omap_lookup_newest_block()
to be used in this situation.
By the way, there was no output on dmesg when I discovered this bug.
That's no good, so add a new error message in here.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
A container with a size starting at 1454383304704 bytes (~1.3 TiB) needs
more than one block to hold the bitmap of its internal pool. Add support
for such large filesystems.
To this end, replace the single buffer head holding the bitmap in the
spaceman struct with an array, which requires allocating the spaceman
separately from the container superblock info. Then rework the alloc and
free functions to work with the whole array.
The biggest changes are in apfs_rotate_ip_bitmaps(), which now has to
copy all the bitmap blocks at once to make them available for the
transaction. This is probably not what the official implementation
intended, but it should be technically correct.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The previous commit started reporting the driver version on disk after a
modification. Report it as well in the driver metadata, so that it can
be obtained with modinfo.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The filesystem superblock has an apfs_modified_by field to report which
implementation is responsible for the latest changes. This could be
useful to track down bugs eventually, but it will need to have more
information. Right now we only report my name and the name of the
module; change it to also report the exact commit id, or the tag if
there is one. There is a strict size limit here, so shorten my name to
"eafer", which is what I use on github, so it will make the driver easy
to track down.
Also move the APFS_MODULE_ID_STRING macro definition to super.c. We'll
now need to recompile that file with every build, but if we left it
inside apfs.h then we would have to recompile everything all the time.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Some mmap related tests such as generic/030 are throwing the following
callstack on kernels 6.1 and above:
[ 9810.190496] ------------[ cut here ]------------
[ 9810.192000] irq 15 handler ata_bmdma_interrupt+0x0/0x1f0 [libata] enabled interrupts
[ 9810.193639] WARNING: CPU: 0 PID: 0 at kernel/irq/handle.c:161 __handle_irq_event_percpu+0x150/0x190
[ 9810.195220] Modules linked in: apfs(OE) libcrc32c(E) 9p(E) fscache(E) netfs(E) intel_rapl_msr(E) intel_rapl_common(E) intel_pmc_core_pltdrv(E) intel_pmc_core(E) kvm_intel(E) kvm(E) irqbypass(E) ghash_clmulni_intel(E) ppdev(E) aesni_intel(E) bochs(E) libaes(E) drm_vram_helper(E) drm_ttm_helper(E) crypto_simd(E) cryptd(E) ttm(E) rapl(E) snd_pcm(E) snd_timer(E) 9pnet_virtio(E) drm_kms_helper(E) 9pnet(E) snd(E) soundcore(E) pcspkr(E) parport_pc(E) parport(E) button(E) joydev(E) evdev(E) serio_raw(E) sg(E) binfmt_misc(E) fuse(E) drm(E) efi_pstore(E) configfs(E) qemu_fw_cfg(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) sd_mod(E) t10_pi(E) crc64_rocksoft(E) crc64(E) crc_t10dif(E) crct10dif_generic(E) ata_generic(E) ata_piix(E) libata(E) crct10dif_pclmul(E) crct10dif_common(E) crc32_pclmul(E) virtio_pci(E) virtio_pci_legacy_dev(E) crc32c_intel(E) virtio_pci_modern_dev(E) virtio(E) psmouse(E) virtio_ring(E) e1000(E) scsi_mod(E) scsi_common(E)
[ 9810.195294] i2c_piix4(E) floppy(E) [last unloaded: apfs(OE)]
[ 9810.215274] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W OE 6.0.0-rc2+ #24
[ 9810.216639] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
[ 9810.217983] RIP: 0010:__handle_irq_event_percpu+0x150/0x190
[ 9810.218904] Code: 44 00 00 e9 09 ff ff ff 80 3d 8f 16 4e 01 00 75 1b 48 8b 13 44 89 ee 48 c7 c7 e8 31 91 bc c6 05 79 16 4e 01 01 e8 90 26 81 00 <0f> 0b fa 0f 1f 44 00 00 e9 fc fe ff ff 3e 48 0f ba 6b 40 01 0f 82
[ 9810.223642] RSP: 0018:ffffaf4240003f60 EFLAGS: 00010286
[ 9810.224836] RAX: 0000000000000000 RBX: ffff94804172f080 RCX: 0000000000000000
[ 9810.226451] RDX: 0000000000010002 RSI: ffffffffbc94d2a2 RDI: 00000000ffffffff
[ 9810.228082] RBP: 0000000000000001 R08: 0000000000000000 R09: ffffaf4240003e00
[ 9810.229506] R10: 0000000000000003 R11: ffffffffbccc4778 R12: 0000000000000000
[ 9810.231182] R13: 000000000000000f R14: ffff94804019ae00 R15: 0000000000000000
[ 9810.232856] FS: 0000000000000000(0000) GS:ffff948177c00000(0000) knlGS:0000000000000000
[ 9810.234771] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 9810.236112] CR2: 00007f6d608b1210 CR3: 000000006020a004 CR4: 0000000000370ef0
[ 9810.237812] Call Trace:
[ 9810.239294] <IRQ>
[ 9810.239767] handle_irq_event+0x34/0x70
[ 9810.240688] handle_edge_irq+0x9f/0x240
[ 9810.241577] __common_interrupt+0x63/0x100
[ 9810.242444] common_interrupt+0xb4/0xd0
[ 9810.243141] </IRQ>
[ 9810.243575] <TASK>
[ 9810.244008] asm_common_interrupt+0x22/0x40
[ 9810.244820] RIP: 0010:native_safe_halt+0xb/0x10
[ 9810.245705] Code: 01 00 3e 80 48 02 20 48 8b 00 a8 08 75 bf e9 7b ff ff ff cc cc cc cc cc cc cc cc cc cc cc cc 66 90 0f 00 2d a9 eb 49 00 fb f4 <c3> cc cc cc cc 66 90 0f 00 2d 99 eb 49 00 f4 c3 cc cc cc cc cc 0f
[ 9810.249342] RSP: 0018:ffffffffbcc03e90 EFLAGS: 00000252
[ 9810.250451] RAX: ffffffffbc16ab90 RBX: ffffffffbcc139c0 RCX: 0000000000000000
[ 9810.252391] RDX: 4000000000000000 RSI: ffffffffbc94d2a2 RDI: 00000000005ec6ec
[ 9810.253784] RBP: 0000000000000000 R08: 00000a4bf1fdb94f R09: 0000000000000000
[ 9810.256613] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
[ 9810.258508] R13: 0000000000000000 R14: ffffffffbcc13118 R15: 0000000000000000
[ 9810.260053] ? ldsem_down_write+0x230/0x230
[ 9810.261021] default_idle+0xa/0x10
[ 9810.261734] default_idle_call+0x36/0xf0
[ 9810.262574] do_idle+0x220/0x2b0
[ 9810.263341] cpu_startup_entry+0x19/0x20
[ 9810.264217] rest_init+0xcb/0xd0
[ 9810.265119] arch_call_rest_init+0xa/0x14
[ 9810.266173] start_kernel+0x6d2/0x6fb
[ 9810.267059] secondary_startup_64_no_verify+0xce/0xdb
[ 9810.268261] </TASK>
[ 9810.268837] ---[ end trace 0000000000000000 ]---
I bisected this back to upstream commit f168420c62e7 ("blk-mq: don't
redirect completion for hctx withs only one ctx mapping"), but that
doesn't tell me much.
I can't tell for sure what's going wrong here, but it seems to be
related to my implementation of ->b_end_io(), which exists to force page
faults when we write to an mmapped page again after a commit. I didn't
follow any clear reference when I wrote that code, and I'm guessing
something about it is not allowed. Also, this code runs asynchronously,
which seems to fit with the nature of the problem, since my driver is
not mentioned in the callstack.
Just use the standard end_buffer_write_sync() for ->b_end_io(), and wait
for each buffer before cleaning its page. This is similar to what we do
for reads and it doesn't have any original async stuff, so hopefully it
will work better. At least none of my tests are failing anymore.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
This release adds support for reading compressed files of arbitrary
on-disk size, which is becoming more and more necessary in recent ipsw
images.
A number of bugs are addressed for systems with little ram, reported by
Gilgamesh via email.
Support is added for the 6.6 kernel version, mainly thanks to Woody
Suwalski. I'm afraid the driver sometimes triggers a warning for recent
kernels, but I didn't manage to fix it in time for the new upstream
release, and it's better to at least get things to build.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Several of xfstests (generic/249 for example) are failing on kernel 6.6
because of sendfile issues. Starting with upstream commit 36e2c7421f02
("fs: don't allow splice read/write without explicit ops"), the
->splice_read() and ->splice_write() methods need to be set explicitly,
or else we just get EINVAL when sendfile needs them. This change
happened in the 5.10 release, but I never noticed because I usually only
run tests on 5.3.
So, set the methods as required, taking into consideration the kernel
version because there was another related change in commit 2cb1e08985e3
("splice: Use filemap_splice_read() instead of
generic_file_splice_read()").
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Attempts to run xfstests for the 6.6 kernel fail brutally. Transactions
are getting aborted inside apfs_update_time(), which has never happened
before. The problem comes from upstream commit 541d4c798a59 ("fs: drop
the timespec64 arg from generic_update_time"): generic_update_time()
can't fail and used to always return a symbolic 0, but now instead
returns some flags - which my code mistakes for an error. We don't care
about the flags, so just ignore the return value for all kernel
versions.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I was reading the list of deprecated interfaces at [0] and it mentions
that the use of BUG() and BUG_ON() is discouraged in the kernel, and
that we should be prepared to fail gracefully even for impossible
situations.
I definitely agree with this: so far I've used BUG_ON() to implement
ASSERT(), but in practice I always turn that into a WARN_ON() while
debugging. Just leave it as WARN_ON() permanently.
I also use BUG() in apfs_query_storage() to mark an impossible query
tree flag. I can't imagine how this would ever be reached, but still
keep going and return an invalid storage type. This way the error can
get reported later, after calling either apfs_create_node() or
apfs_read_node(), which have been modified to fail gracefully and
complain with apfs_alert() in this situation. I also considered putting
an assertion in apfs_query_storage(), but that would require reordering
apfs.h and it seems unnecessarily messy.
Warnings should not be abused either, because of the panic_on_warn
option. Replace our one call to WARN_ON(), inside apfs_btree_replace(),
with another apfs_alert(). This function was already failing gracefully.
[0]: https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
According to checkpatch.pl, the use of zero-length arrays is now
deprecated in the kernel. Replace them all with C99 flexible array
members.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Some code that wasn't originally written by me has a style that differs
slightly from the rest of the module. Since I've been refactoring
compress.c quite a lot lately, I have made the inconsistencies more
visible, so I think it's a good time to switch everything to the typical
kernel style.
For this purpose I'm running checkpatch.pl for the first time in years,
and it has detected a handful of other issues that I introduced myself.
Fix most of the issues now, but I prefer to leave the zero-length array
stuff for a separate patch.
For the record, at the moment I'm choosing to ignore style inconsistency
in the compression libraries, even in libzbitmap which I wrote myself. I
prefer to keep them as close to upstream as possible.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
For some reason I never remembered to take the big lock during reads of
compressed files. Do it. As always, my locking is coarse and hacky, and
I will have get back to it at some point.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
After the recent refactoring of the compression code, the fail_enomem
label feels a little unnecessary. Get rid of it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
While debugging the previous patch, I noticed that my implementation of
compressed ->readpage() from f9ca9061bb ("Implement mmap for
compressed files") has a serious issue: if something goes wrong during
the read, the error code is swallowed and stale garbage becomes visible
to the user.
I messed this up because I was following ext4_read_inline_page(), which
first calls ext4_read_inline_data() and then, without checking the
return code, calls SetPageUptodate(). But ext4_read_inline_data() can't
actually fail, so this is not a problem. In my version the function
called is apfs_compress_file_read_page(), which does fail sometimes,
leaving a stale page marked as updated. Fix it, and also return the
error code correctly.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Currently, compressed files that are stored directly in the decmpfs
xattr are handled separately: they get entirely uncompressed on open,
and on reads the contents simply get copied. To get rid of this code
duplication, use a new apfs_compressed_data struct to read from both
inline and dstream xattrs, and let the regular resource-compressed code
handle reads for both types.
These changes weren't made exclusively for code quality. A compressed
file may be too big to fit inside decmpfs, but still small enough to fit
inside a xattr. If that happens, the resulting resource fork will be an
inline xattr, and the dstream read code implemented with e4bb06ecef
("Avoid reading whole compressed files into memory") will break down. So
the apfs_compressed_data struct is strictly necessary for the resource
case; using the same struct for both cases simplifies testing, because
inline resource compressed files are rare (I've seen them, but right now
I don't even have an image with one of them).
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
After commit e4bb06ecef ("Avoid reading whole compressed files into
memory"), performance for compressed reads worsened by about 50%. The
problem is that, when we stopped reading the xattr all at once, we lost
the advantage of letting the kernel batch and schedule the buffer reads.
The ideal way to handle this would be to make a fake inode for the xattr
dstream, and let the kernel do readahead in the usual manner. I don't
feel very confident that I can get that right though, so I'm leaving it
for some other time.
For now, just request all the blocks as soon as a read begins (I would
have preferred to do it on open, but that's slower for some reason), and
ignore the results. Compressed data is supposed to be small so, as long
as there is enough memory, this has all the performance benefits of the
original implementation (in fact it's somewhat faster), but it still
avoids the problems with large virtual memory allocation.
Of course, unlike normal readahead, this will be useless if memory is
scarce, or if we don't read the whole file sequentially.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I've neglected to check for failure after allocating an array in
apfs_nonsparse_dstream_read(). This is not a new bug: it got carried
over from the original xattr code, which has been around since
84c3ecebac ("Don't read dstream xattr blocks one by one"). Fix it now.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
In commit e4bb06ecef ("Avoid reading whole compressed files into
memory"), I moved part of the code of apfs_compress_file_read_block()
into a separate apfs_compress_file_read_from_block() that must call it.
I made a mistake, though: the original version of the function would
reset the return value before returning, but this version no longer does
it. This doesn't play well with the decompression functions for some of
the algorithms, which return decompressed lengths. So we get a "failed
to read block into buffer" message and the decompression is abandoned.
Fix this.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
When implementing compressed mmap I was forced to duplicate the code for
regular compressed reads, so that one version could copy to a user
buffer and the other to the kernel. To simplify this, get rid of the
regular version and instead use the generic read iterator, which will
make use of the same mmap code.
This should also allow regular compressed reads to take advantage of the
page cache.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
When compressed mmap was implemented, apfs_compress_file_mmap() was
added with the purpose of ignoring calls to ->page_mkwrite(). With the
last patch, I no longer allow compressed files to be opened for writing
at all, so I can just use regular ->mmap() for all files.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
In commit 31df49c6d8 ("Ban writes to compressed files explicitly"), I
blocked writes to compressed files when they actually happen, not when
the file gets opened. My fear was that banning rw open entirely could
break other people's code, since I'm not sure who is using this driver
and how.
Now I've implemented compressed mmap, and blocking writes when they
happen is becoming a little too hacky. Do this properly, and wait to see
if anyone complains.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Attempts to mmap compressed files currently fail with ENODEV. I guess I
had forgotten about compressed mmap. Implement it now, in read-only mode
like all compressed operations so far.
Since the existing read code works with userland buffers, it can't be
reused to copy stuff into the kernel. For now I've been forced to
duplicate a lot of code, but I think it might be a good idea to use the
generic read implementations instead. I'll look into that soon.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Gilgamesh reports via email that his kernel often crashes on my call to
page_mkclean() due to "BUG_ON(!PageLocked(page))".
I should have expected problems such as this, since the locking in
apfs_end_buffer_write_sync() is not sane at all. Apparently back when I
wrote this, I only realized that I needed to lock the page because of
the above-mentioned panic. So I tried to take the lock, but this caused
a deadlock in generic/013 of xfstests. Instead of figuring out what the
problem was, I just replaced lock_page() with trylock_page(), to silence
the panic and still avoid deadlock. Of course this leaves us open to a
race, which in this case became visible because whatever was holding the
page released the lock before the check happened in page_mkclean().
In my defense, most of the locking in the driver is insane at this
point, so I don't usually put much thought into it unless I see actual
problems, as in this case.
So, replace trylock_page() with lock_page(), and figure out what was
causing the generic/013 deadlock in the first place. This was actually
very simple: a check of the contents of the problem page showed that
it's always a chunk-info block, and those are the only block types that
both get dirtied and added to the transaction. This is unnecessary, and
it causes some conflict on commit with whatever the kernel does to the
dirty blocks. Stop dirtying the cib blocks.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I've been ignoring the 'wait' argument to apfs_sync_fs() so far, but
this means that transactions get committed twice in a row whenever a
sync is requested. Instead, just do nothing when wait == 0. If the
caller actually needs a sync, they will have to try again with wait == 1
later anyway.
[eafer: added comment and commit message]
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I wrote apfs_xattr_get_dstream() long before error messages were changed
in d4797823ea ("Improve error reporting"), but the code wasn't
actually merged until recently, and I forgot to update the error
messages to our current standards. Fix that now.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Gilgamesh reports via email that there is a kernel crash when you try to
copy an 800Mb file on a system with 512Mb of memory. The reason is that
I don't allow buffer heads to be reclaimed before commit time (I tried
it at some point and it was trickier than it seemed), and since commit
c5dfaa2de9 ("Allow bigger transactions"), each transaction can get as
big as 256Mb.
So, until I can get around to the reclaim issue (or hopefully get rid of
buffer heads), make transactions smaller for systems with little memory.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The official implementation doesn't seem to truly support writes to
compressed files: when it's attempted, the file gets decompressed
transparently instead. I don't intend to that in this driver, unless
someone actually requests it. Users can just make a decompressed copy of
the file and replace the original.
By mistake, truncating compressed files is currently allowed, but since
compressed reads don't depend on the vfs-reported size, nothing seems to
happen. Other writes fail, but the error code is confusing and there is
no output on dmesg. Ban all writes explicitly, and explain the problem
in the console.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Currently, compressed files saved in xattrs are read into memory all at
once, and later decompressed as needed. This has always required a large
memory allocation, which can be a problem in general, but especially for
32-bit archs which can't request more than 128Mb from vmalloc(). As
mentioned in commit 1e3fa28c4f ("Increase limit for compressed size
of files"), we are beginning to encounter files bigger than that limit.
Using the new apfs_nonsparse_dstream_read() to read from arbitrary xattr
offsets, rework the compressed reads so that we only allocate memory for
one compressed block at a time. For now the change is only for resource
xattrs, since I don't expect inline xattrs to ever be this big.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Lately I'm encountering more and more compressed files that are too
large for their xattrs to be read into memory all at once, so I'll need
to be able to read xattrs from arbitrary offsets. The code for this is
very similar to what's already inside apfs_xattr_extents_read(), so spin
it into a new apfs_nonsparse_dstream_read() with a generic interface
that can later be reused from compress.c.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
This release fixes the build for the Linux 6.5 release candidate, and it
should also fix a long-standing mmap bug in Linux 5.14 and above, which
resulted in null pointer dereference.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Adamantinum reports trouble while attempting to use apfs as root with
kernel version 6.4.8:
https://github.com/linux-apfs/linux-apfs-rw/issues/50
His callstack shows an npd in mmap, and the bug can be easily reproduced
by generic/029, but not in kernel version 5.3, where I usually run my
tests. Via bisection I tracked the problem back to kernel commit
0af573780b0b ("mm: require ->set_page_dirty to be explicitly wired up").
Starting with kernel 5.14, the ->set_page_dirty() address space method
gets called unconditionally, without checking for null first, so set it
explicitly.
Kernel 5.18 replaced ->set_page_dirty() with ->dirty_folio(), so take
care of that as well.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The build is broken for the Linux 6.5 release candidate:
https://github.com/linux-apfs/linux-apfs-rw/issues/49
The fmode_t type can no longer be used for block open flags, and we must
switch to blk_mode_t instead. FMODE_EXCL is no longer needed, since just
having a holder already implies it, but this requires a change to the
blkdev_put() api. There is also a new argument to blkdev_get_by_path()
which can be safely ignored. And finally, sb->s_mode is gone, but it
doesn't seem to make any difference.
Update all the code to the new apis.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The build appears to be broken for some configurations on the Linux 6.5
release candidate:
https://github.com/linux-apfs/linux-apfs-rw/issues/49
Some sort of static overflow check is going on for strcpy(), and several
of my calls are triggering false positives. Just be safe and switch them
all to strscpy(). The qstr stuff in particular looked a bit too weird
anyway.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
A recent cryptex image contains a file with a compressed size above the
current 128Mb limit. This limit exists because of vmalloc(), and it can
be increased (at least for 64-bit archs). Do that for now, but I really
should try to restructure the code to use smaller kmalloc() allocations
instead.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
2023-06-07 20:40:53 -03:00
39 changed files with 7161 additions and 7579 deletions
File diff suppressed because it is too large.
Load diff
Loaded 20 of 39 files, more files were not shown because too many files have changed in this diff.
Show more
Reference in new issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
