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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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'm getting somewhat regular failures on generic/112 because the ip free
queue for the test image has a one-node limit, and it sometimes gets
full. The thing is, the node actually has plenty of room, but it's all
in the key free list, so I can't allocate values. From now on, if an
insertion fails with ENOSPC, check the available room and defragment
the node if it should be enough. Then retry the insertion instead of
jumping straight to a split.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The previous patch was intended to trigger an immediate abort when a
free queue violated its own node limit. But I managed to trigger this
event again with generic/112, and to my surprise there were several
operations in dmesg before the abort finally happened. The problem here
is in apfs_flush_fq_rec(): when apfs_main_free() fails (in this case
because the ip free queue is full), the error code is reported but not
bubbled up. This is a mistake of course, so fix it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Free queues have a node limit that is set by the mkfs. Very rarely,
generic/112 manages to break this limit, and corruption is reported.
Preventing corruption is always the number one priority, so add a check
for this limit after every free queue insertion so that we can abort
immediately when it fails.
Remember though, that there is a deeper issue here: either the btrees
are becoming too fragmented, or the flushes are not frequent enough. The
queues should never fill like this.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The driver is much closer to being usable, so I might start getting
subtler bug reports soon. To make them easier to handle, put error
messages all over the place. I should have done this from the beginning,
but I guess I didn't fully understand the need back then.
From now my general policy will be to use apfs_warn() for user errors or
unsupported features; apfs_err() for things that are probably corruption
or io errors; and apfs_alert() for things that are most likely bugs.
These last two should be rare, so the same error/alert will be thrown by
several layers in the callstack to provide as much information as
possible. Be careful and don't flood the console on normal situations.
Also, make messages with a log level lower than warning output their
function name and line number, which I think will help debugging more
than the actual messages.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
When running xfstests generic/142 (a file clone test), I encounter the
following error:
APFS (2cg): creation of cpm blocks not yet supported
This is expected to happen when we have an absurdly large number of free
queue records. In this particular test, many writes are made to the
middle of files, so each of these writes becomes its own extent. When
the file is later deleted, all of these extents are deleted in a single
massive transaction. Each of them becomes its own free queue record, and
there is no opportunity to flush the queue.
There are three problems in here. The first one is that writes to the
middle of files should be allowed to allocate longer extents. The second
one is that truncation of orphan files should get split in multiple
transactions. These problems are important, but for now I just want to
test file clones, so I'll leave the fixes for later.
The third problem, much easier to fix, is that the driver should never
create bursts of small consecutive free queue records: it should just
merge them all into a bigger one. For that purpose, keep a cache of the
latest freed range, and always attempt to extend it instead of creating
a new record directly.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I ran into a stacktrace while testing the cknodes option for the
previous patch. The error message was "VFS: brelse: Trying to free free
buffer", and it showed up when apfs_read_node() failed while flushing a
free queue. The problem is in the never tested cleanup code for
apfs_read_spaceman(): the buffer is released, but dangling pointers
remain in the superblock.
At some point I need to start testing with error injection to find stuff
like this, but for now at least fix the bugs that show up during other
work, such as this one.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Luflosi has reported a long time ago that the cknodes mount option does
not work well on readwrite mounts:
https://github.com/linux-apfs/linux-apfs-rw/issues/15
I don't care much for the cknodes option, and I don't know if anybody is
actually using it, but I shouldn't ignore a simple bug like this for
such a long time.
The problem here is that there are several places in the code where an
object will get its checksum verified after getting changed, but before
the transaction is committed and the checksum updated. As a solution,
refuse to verify buffers that are already part of a transaction. Try to
make sure that a check happens before the join.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Used to apfs_warn(), it seems that I forgot to put a newline at the end
of the message string that warns about a versioned space manager. I just
noticed it now because I was doing the same thing to report unsupported
compression algorithms. Fix it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
