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>
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>
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>
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>
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>
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 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>
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>
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>
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>
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>
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>
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>
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>
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>
Ernesto A. Fernández
Woody Suwalski
Woody Suwalski