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>
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>
While fixing the build for old kernels, I noticed that in some of them
you get a warning when you prink a buffer head's b_blocknr as 0x%llx.
I haven't checked but I'm guessing that, at some point, sector_t on
64-bit archs went from being 'unsigned long' to 'unsigned long long',
so these warnings went away. Anyway, just cast b_blocknr before printing
it in all kernels.
At some point I do need to attempt a 32-bit build though. It's been a
long time, and I'm sure there will be much worse problems than this one.
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 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>
The apfs_total_blocks_freed and apfs_total_blocks_alloced fields of the
volume superblock keep track of the total number of blocks that have
been freed/alloced through the whole lifetime of the volume so far.
Since the value of these fields depends on historical operations, it
can't be calculated from the volume contents, so I figured I could just
ignore them without consequences.
This seems to be mostly correct, but the official fsck does complain if
the freed blocks appear to be more than the alloced blocks. It's just a
warning, not an error, but it does often trigger after the official
driver makes changes to one of my images. So, try to be careful with
these two fields from now on.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
In the previous patch I forgot to consider that index nodes have
different value sizes than leaf nodes, even for fixed key/value size
trees. Fix it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
When I started working with OMAP_SNAPSHOT trees, I forgot to add support
for them inside apfs_node_min_table_size(). The result is that the index
of each of those trees is initialized to a tiny minimum size, as if the
keys and values were of variable length. This went unnoticed for a while
because of other bugs in my fsck, which I've recently fixed in
5a20a212d8f0 ("apfsck: fix checks of index size for fixed kv size"). The
official fsck always complained though, I just wasn't using it:
** Checking the snapshot metadata.
error: btn: invalid btn_table_space (0, 64), given btn_flags (0x7)
So, implement apfs_node_min_table_size() properly for the OMAP_SNAPSHOT
tree type. For consistency with the fsck, also implement the FEXT tree
type, but I don't actually support writes to sealed volumes in the
driver at the moment.
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>
Add suport for reads from sealed volumes. The main differences are that
catalog index nodes are bigger, and that file extents are kept in a
separate tree.
Writes could in theory be supported as well, as we do in our internal
tool. It would probably be a bit too hacky though, so I'll wait to see
if anybody asks for it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Define an in-memory omap structure to be shared by all mounts of the
same volume, including both the current transaction and the snapshots.
The on-disk omap is already shared by all of them, so this will make for
saner code.
The patch fixes two existing issues. One is a regression introduced by
39878b3753be ("Preserve omap records in snapshots"), which was causing
regular failures in generic/013 of xfstests. The problem was with the
hacky way in which I told apart container and volume omaps, by setting
sbi->s_vsb_raw to NULL before calling apfs_map_volume_super(); it seems
that the dentry cache or something needs this to be set at all times.
Now that the "latest_snap" field has been moved from the sbi to the omap
struct, we no longer need to handle containers and volumes differently:
container omaps are the same as volume omaps without snapshots.
The other issue is that snapshot mounts only read the location of the
omap root once (on mount), but subsequent writes to the volume will free
this original block and potentially allocate it for something else. I've
never seen this in practice yet, but I would expect the snapshot mount
to start complaining about filesystem corruption. Now that the omap root
node is shared by all mounts of the volume, this problem should be gone.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Attempts to create a second snapshot on a same volume fail, with the
driver reporting corruption. The problem is in apfs_node_locate_key():
we assume that all nodes with fixed key/value length are omap nodes, and
so we work with a wrong value for the length of keys in the snapshots
tree.
These problems would never occur if we just read the key/value lengths
from the root node info footer, and maybe kept them in a btree struct
shared by all the nodes. For now though, just hardcode the correct
length value for trees of this type.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
With the previous patch in place we can now take snapshots, but they
become immediately corrupted when a new transaction begins and changes
the omap records. This happens even if file contents aren't modified,
so it becomes impossible to test this new functionality at all.
So, always preserve omap records that belong to a snapshot. For this
check, the disk layout provides om_most_recent_snap in the object map;
add a matching s_latest_snap field to the in-memory superblock structure
to make it easier to work with.
Bigger changes are needed in apfs_omap_lookup_block(), because we need
to work with an omap record's xid, which has so far mostly been ignored.
Replace apfs_bno_from_query() with a new apfs_omap_map_from_query()
which doesn't discard so much valuable metadata. This change also allows
us to start preserving the flags in the omap record, which may also be
important.
Another problem in apfs_omap_lookup_block() is that we need a way to
tell apart volume omaps from the container omap, which won't be affected
by snapshots. This is all a little hacky, and it would be much cleaner
if I had an in-memory omap struct to pass around instead of the root
nodes. I should look into that in the future.
With this patch in place, my fsck reports no corruption after a single
snapshot is created and the volume is unmounted.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Implement an ioctl that creates a snapshot for the current transaction.
A matching userland tool has already been added to apfsprogs.
Note that, at this point, snapshots won't be handled correctly on later
writes at all. Being able to actually create snapshots for testing is
only the first step in that direction.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I'm trying to build with "-W" for the first time, and it reports that
apfs_free_query() doesn't use the 'sb' parameter. Get rid of it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Get rid of the reference counting for in-memory nodes, which is just a
hacky way to avoid accidentally freeing root nodes that are remembered
by the superblock structure. Instead, never delete the root nodes as
part of apfs_free_query() and let the caller decide how to deal with
them.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Rework apfs_node_split() to take advantage of the apfs_object changes
from last patch and avoid the fake buffer head. Now the temporary node
used for the split can be handled like any other node, so do that
instead of using big stack allocations, which are unacceptable in the
kernel.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Add a 'data' field to in-memory objects and point it to the buffer
head's 'b_data'; from now on, always access the data through this field
instead of the bh. To make sure I remembered to clean up all instances,
rename the buffer head in the object from 'bh' to 'o_bh'.
The point is to allow nodes to work with a data block that exists only
in memory, and the next patch will take advantage of this to improve
apfs_node_split(). Since objects that are not nodes will not need this
feature, only adjust the cleanup done in apfs_node_release() for now,
though maybe object cleanups should all be done in one place.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The record count in a node is a 32-bit number but apfs_node_is_valid()
immediately casts it to an int, so the result may be negative if the
count is unnaturally large. This would bypass the final check in the
function ('records * entry_size <= index_size'), and these sanity checks
are important to prevent out-of-bounds operations, so fix it.
A huge record count in the final check also makes it possible for
multiplication to overflow, so add a check for a rough upper bound
on the record count before actually comparing it to the index size.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Most checksum calculations are already done during transaction commit,
but there are a few exceptions, mostly in the spaceman code. When a
large file is copied, close to 10% of the time is spent in these useless
recalculations. So, defer all of them instead.
Sadly (and confusingly), I don't see any actual performance improvement
in my benchmarks, but I think it's better to leave this in place anyway.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Don't call apfs_sb_bread() to work on blocks that have no valuable data
and will just be overwritten. It's wasteful because the driver must
sleep until the unnecessary read is done. Work instead with a new
apfs_getblk(). The result is a 5% performance improvement when rsyncing
a whole volume.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Update the offset of the current query record key if apfs_node_insert()
expands the table of contents. Otherwise, the function may still find
later on that there's no room for the new record, and return ENOSPC
leaving an inconsistent query. A caller that attempts to refresh such a
query would run into trouble.
This bug was discovered with generic/013 of xfstests.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The header for apfs_node_split() claims the the query parent will always
be set to NULL on success, but this isn't done correctly if the node is
only defragmented. The problem is that apfs_btree_insert() relies on
this behaviour to decide if a split has been attempted yet, and the
current implementation error will make it loop forever.
So fix apfs_node_split() and make it act as documented.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
