Paul Eggert wrote: > Pádraig Brady <p...@draigbrady.com> 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.
