On 05/06/2014 05:49 AM, Stefan Hajnoczi wrote:
On Mon, May 05, 2014 at 10:01:39PM +0200, Max Reitz wrote:
The current version of raw-posix always uses ioctl(FS_IOC_FIEMAP) if
FIEMAP is available; lseek with SEEK_HOLE/SEEK_DATA are not even
compiled in in this case. However, there may be implementations which
support the latter but not the former (e.g., NFSv4.2). In this case,
raw-posix should fall back to lseek with SEEK_HOLE/SEEK_DATA if FIEMAP
does not work.
A bigger cleanup is extracting the FIEMAP and SEEK_HOLE/SEEK_DATA
implementations into their own static functions. Then
raw_co_get_block_status() becomes simpler and doesn't need ifdefs:
ret = try_fiemap(...);
if (ret < 0) {
ret = try_seekhole(...);
}
if (ret < 0) {
...report every block allocated by default....
}
In other words, let normal C control flow describe the relationships
between these code paths. Use ifdef only to nop out try_fiemap() and
try_seekhole().
What do you think?
I like the idea - separating control flow from #ifdefs (by having stubs
on the other end of the ifdef) definitely makes algorithms easier to
understand.
More things to consider: GNU Coreutils has support for both fiemap and
seek_hole, but favors seek_hole first, for a couple reasons. First,
FIEMAP has not always been reliable: on some older kernel/filesystem
pairs, fiemap could return stale results, which led cp(1) to cause data
loss unless it did an fsync() first to get the fiemap to be stable - but
the cost of the fsync() made the operation slower than if fiemap were
never used. Second, POSIX will be standardizing seek_hole in its next
revision [1] (still several years out, but the fact that it is an
announced intention means people are starting to implement it now).
fiemap, on the other hand, remains a Linux-only extension. Yes, fiemap
provides more details than seek_hole (and is the ONLY way to know the
difference between a hole that has reserved space on the disk vs a hole
that will require allocation if is written to), but if all you need to
know is whether a hole exists (rather than what type of hole), then
seek_hole is MUCH simpler.
[1] http://austingroupbugs.net/view.php?id=415