[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] New fallocate module
From: |
Pádraig Brady |
Subject: |
Re: [PATCH] New fallocate module |
Date: |
Fri, 22 May 2009 23:54:41 +0100 |
User-agent: |
Thunderbird 2.0.0.6 (X11/20071008) |
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.
- [PATCH] New fallocate module, Pádraig Brady, 2009/05/21
- Re: [PATCH] New fallocate module, Bruno Haible, 2009/05/22
- Re: [PATCH] New fallocate module, Pádraig Brady, 2009/05/22
- Re: [PATCH] New fallocate module, Paul Eggert, 2009/05/22
- Re: [PATCH] New fallocate module,
Pádraig Brady <=
- Re: [PATCH] New fallocate module, Pádraig Brady, 2009/05/27
- Re: [PATCH] New fallocate module, Eric Blake, 2009/05/28
- Re: [PATCH] New fallocate module, Bruno Haible, 2009/05/28
- Re: [PATCH] New fallocate module, Pádraig Brady, 2009/05/28
- Re: [PATCH] New fallocate module, Pádraig Brady, 2009/05/28