[rrd-developers] [PATCH,RFC] optional mmap based file I/O

[Bernhard Fischer]

On Tue, May 29, 2007 at 11:28:21PM +0200, Tobias Oetiker wrote:
>Hi Bernhard,
>
>> I'm attaching an update which does:
>>
>> - flag rrd_resize's old file with RRD_COPY
>> - cleanup error-handling pathes in rrd_update and fix a few typos in
>>   comments
>> - rrd_close(): implement printing mincore results for the rrd if
>>   DEBUG=2 was defined
>> - rrd_open(): madvise start addresses need to be page-aligned; implement
>>   alternative path to the fine-grained (i.e. exact) madvise by flagging
>>   just the first two pages as needed (see TWO_PAGES). Implement
>>   alternative path that records the last madvise()ed area to avoid
>>   redundant calls to madvise() on identical areas (due to
>>   page-alignment constraints) -- see CHECK_MADVISE_OVERLAPS. Implement
>>   path for USE_DIRECT_IO.
>> - configure: add check for O_DIRECT flag to open(2). Add option
>>   --enable-direct-io. Add _GNU_SOURCE to CFLAGS to silence warnings
>>   about chroot which is marked LEGACY since SUSv2 and is a non POSIX
>>   extension. Make checks for posix_fadvise() dependant on
>>   --disable-mmap, since we do not need fadvise for the mmap case.
>>
>> Please apply.
>
>done ... a few questions/things ...
>
>* what is the tought behind  (in rrd_last)
>
>     close(rrd_file->fd);
>     rrd_close(rrd_file);
>
>  shoulnd rrd_close close as well ?

Well, I was not sure about this. rrd_info does close the fd early on, so
we cannot blindly close the fd in rrd_close. If it is ok for you, we can
change rrd_close to close the fd (and adjust the callers accordingly).
>
>* why do you flag the first TWO pages ? rrd files with headers
>  takeing up two pages should be pretty rare ...

Fair enough. My small test rrd's all had about 4192 bytes of headers
(IIRC), so i thought that using 2 pages should be a good starting point.
Easy enough to change, if you are confident that 1 page is enough for
the majority.
>
>* for figuring the pagesize you should use getpagesize is
>  available.

I beg to disagree :)
getpagesize is not in SUSv3; From the manpage:
CONFORMING TO
  SVr4,  4.4BSD,  SUSv2.   In  SUSv2  the  getpagesize()  call is labeled
  LEGACY, and in POSIX.1-2001 it has been dropped; HP-UX  does  not have
  this  call.   Portable applications should employ sysconf(_SC_PAGESIZE)
  instead of this call.


See
http://www.opengroup.org/onlinepubs/009695399/functions/sysconf.html

So i vote for using sysconf, and adding checks for 
_SC_PAGESIZE, _SC_PAGE_SIZE, PAGESIZE, PAGE_SIZE

Ok?

>* if mmap is not used, then it would be cool if posix fadvise was
>  still called (have not checked the code, just saw that you
>  removed the check from configure)

fadvise is still called for the FD path. Wrapping the check for fadvise
in $enable_mmap just makes sure that we do not call fadvise for mmap,
but only check for it and eventually use it for the non-mmap path.

Thanks,