Re: [PATCH] New fallocate module

[Pádraig Brady]

Paul Eggert wrote:
> 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?)

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.

good.

> 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.

good point

> 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.

OK agreed. I wish FALLOC_FL_KEEP_SIZE was defined in fcntl.h :(
Currently linux/falloc.h has only this define in it.

I'll leave the onus on applications to do:

#if HAVE_LINUX_ALLOC_H
#include <linux/alloc.h>
#endif

Since there is now no fallocate.[ch] I'll figure out how to add
all this to fcntl.in.h

> 
> 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.

good catch. Should be "else if" within that ifdef
(should have been originally anyway for performance reasons).

> 
> * 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.

Well because with fallocate() in the picture the source file
could have a large allocation (nblocks) but small size.
So the copy would maintain this allocation which I thought
was a desired feature. I should add a comment in any case.

> 
> * 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?

Good point. It would need to do an ftruncate() in that case.

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

I don't think so, as this will fall back to writing zeros to the disk
in the case where the filesystem/kernel does not support fallocate().
This is a different higher level functionality IMHO.
There was talk of adding a --allocate option to `truncate`,
and that would call posix_fallocate().

> * 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.

Seems like a worthwhile optimisation.

thanks!
Pádraig.