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>
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>
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>
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>
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>
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'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>
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>
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>
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>
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>
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>
Luflosi reports that the build is broken once again for Linux 4.14 and
4.19:
https://github.com/linux-apfs/linux-apfs-rw/issues/43
The problem here is that the clone and dedupe operations used to be
implemented with separate methods. Since Linux 4.20, they both use the
same ->remap_file_range(), with some flags passed to tell them apart.
So when I implemented clones I used this new version and broke the build
for the old ones. Fix it now.
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>
If __apfs_get_block() gets called with a block number after the end of
the file, it still maps the buffer head successfully but using invalid
information from the previous extent. So far this has never been a
problem for any of the callers, but the previous patch implemented
FIEMAP and generic_block_fiemap() appears to use an unmapped buffer head
to check for EOF. With the current code, it just loops forever.
So, do the right thing and check that the logical block is in range
before doing any extent lookups. At this point there is no reason why
apfs_extent_read() should ever fail to find a matching extent, so make
it return EFSCORRUPTED when that happens.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
If an UPDATE record is created to take a reference, and then a reference
to that same range gets put, then the UPDATE extent will simply get
deleted. When that happens, we mistakenly mark all of its blocks as
free, but the blocks are still in use by the original snapshot. Fix
this.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
When an inode becomes a clone, the internal flags from the original get
copied to it. The problem is that a few of these flags imply the
presence of a matching xfield, but those haven't been created at this
point. The sparse xfield, in particular, may never get created at all if
the original file is no longer sparse. Don't copy those xfield flags,
just set them when the xfield gets created as usual.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The return value for apfs_remap_file_range() on sucess is supposed to be
the length of the copied region, not 0. I haven't checked if this has
any practical consequences, but fix it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I'm seeing a silly regression in generic/430: now that clones are
implemented, copy_file_range() has started to use them under the hood,
but it only wants to clone a portion of the file, and that's not
something I'm interested in supporting.
To avoid this on newer kernels, set ->copy_file_range() to the generic
implementation. For older kernels, start throwing errors if anything
calls apfs_remap_file_range() for partial ranges. This is the right
thing to do in general, since we don't support that.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Ernesto A. Fernández
Integral
Woody Suwalski