Re: [PATCH] New fallocate module

[Paul Eggert]

Pádraig Brady <address@hidden> writes:

> Well libc, kernel or filesystem could return ENOSYS
> so code using fallocate() has to handle it anyway.

If memory serves, ordinarily gnulib tries to catch such situations,
and to substitute a working function when the kernel just has a stub
that returns ENOSYS.  However, I suppose fallocate is different
because it can succeed on one filesystem, but fail on the other.  (Is
that right?)  If so, then you are right that fallocate-using apps must
check for ENOSYS always, and it may make sense to do it the way you
did it, i.e., simply have a stub that returns ENOSYS always.

However, in this case I suggest having the .h file make it clear that
fallocate always returns ENOSYS, rather than doing it in the .c file.
That way, the compiler can optimize out not only the call to
fallocate, but also the run-time check and error message that is
printed when fallocate fails due to running out of disk space.  I
suggest using an inline function for this rather than a macro, if
possible, as that's a bit cleaner.

Once support is added for Solaris 10 and earlier then this would get a
bit more complicated, since an inline function might be a bit
unwieldy, but that's OK; the optimization would still work on all
non-Solaris older platforms.

>> - glibc declares fallocate() in <fcntl.h>. Therefore gnulib should do the 
>> same.
>>   There is no use in creating a file "fallocate.h"; instead, put the 
>> declarations
>>   into fcntl.in.h.
>
> hmm. I was wondering about that. The reason I chose fallocate.h was
> for platform independence. Also <linux/falloc.h> needs to be handled
> independently then.

I agree with Bruno.  Gnulib should do things the glibc way unless
there's a good reason otherwise; that simplifies GNU code overall.

Looking at the patch I see a few problems with the use of fallocate in
coreutils/src/copy.c.

* If HAVE_STRUCT_STAT_ST_BLOCKS, then fallocate can be called even
when make_holes is nonzero, which surely is a bug.

* I don't see why copy.c bothers to round the destination file size to
the next multiple of the block size, as fallocate already does that.

* Suppose the source file shrinks while it's being copied.  Shouldn't
copy.c do another fallocate() call after copying is finished, to
discard the space that was allocated but not needed?

* copy.c should fall back on a native posix_fallocate if fallocate
isn't available.

* A possible optimization if HAVE_FALLOCATE || HAVE_POSIX_FALLOCATE:
copy.c can easily cache whether the most-recently-used file system
supports fallocate (or posix_fallocate), to avoid using system calls
that it knows will fail due to ENOSYS.