liblttd: don't fill the page cache

* Linus Torvalds (torvalds@linux-foundation.org) wrote:
>
>
> On Wed, 19 May 2010, Mathieu Desnoyers wrote:
> >
> > Good point. This discard flag might do the trick and let us keep things simple.
> > The major concern here is to keep the page cache disturbance relatively low.
> > Which of new page allocation or stealing back the page has the lowest overhead
> > would have to be determined with benchmarks.
>
> We could probably make it easier somehow to do the writeback and discard
> thing, but I have had _very_ good experiences with even a rather trivial
> file writer that basically used (iirc) 8MB windows, and the logic was very
> trivial:
>
>  - before writing a new 8M window, do "start writeback"
>    (SYNC_FILE_RANGE_WRITE) on the previous window, and do
>    a wait (SYNC_FILE_RANGE_WAIT_AFTER) on the window before that.
>
> in fact, in its simplest form, you can do it like this (this is from my
> "overwrite disk images" program that I use on old disks):
>
>       for (index = 0; index < max_index ;index++) {
>               if (write(fd, buffer, BUFSIZE) != BUFSIZE)
>                       break;
>               /* This won't block, but will start writeout asynchronously */
>               sync_file_range(fd, index*BUFSIZE, BUFSIZE, SYNC_FILE_RANGE_WRITE);
>               /* This does a blocking write-and-wait on any old ranges */
>               if (index)
>                       sync_file_range(fd, (index-1)*BUFSIZE, BUFSIZE,
+SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE|SYNC_FILE_RANGE_WAIT_AFTER);
>       }
> and even if you don't actually do a discard (maybe we should add a
> SYNC_FILE_RANGE_DISCARD bit, right now you'd need to do a separate
> fadvise(FADV_DONTNEED) to throw it out) the system behavior is pretty
> nice, because the heavy writer gets good IO performance _and_ leaves only
> easy-to-free pages around after itself.

Great! I just implemented it in LTTng and it works very well !

A faced a small counter-intuitive fadvise behavior though.

  posix_fadvise(fd, 0, 0, POSIX_FADV_DONTNEED);

only seems to affect the parts of a file that already exist. So after each
splice() that appends to the file, I have to call fadvise again. I would have
expected the "0" len parameter to tell the kernel to apply the hint to the whole
file, even parts that will be added in the future. I expect we have this
behavior because fadvise() was initially made with read behavior in mind rather
than write.

For the records, I do a fadvice+async range write after each splice(). Also,
after each subbuffer write, I do a blocking write-and-wait on all pages that are
in the subbuffer prior to the one that has just been written, instead of using
the fixed 8MB window.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

diff --git a/liblttd/liblttd.c b/liblttd/liblttd.c
index 91f3a27bf1126f0f16c97adc667f17ec57f7f968..2b187c85860996f7304352453a649b0502967e24 100644 (file)
--- a/liblttd/liblttd.c
+++ b/liblttd/liblttd.c
@@ -109,6 +109,7 @@ int open_buffer_file(struct liblttd_instance *instance, char *filename,
 {
        int open_ret = 0;
        int ret = 0;
+       int fd;
 
        if (strncmp(filename, "flight-", sizeof("flight-")-1) != 0) {
                if (instance->dump_flight_only) {
@@ -129,8 +130,9 @@ int open_buffer_file(struct liblttd_instance *instance, char *filename,
                        ++instance->fd_pairs.num_pairs * sizeof(struct fd_pair));
 
        /* Open the channel in read mode */
-       instance->fd_pairs.pair[instance->fd_pairs.num_pairs-1].channel =
-               open(path_channel, O_RDONLY | O_NONBLOCK);
+       fd = open(path_channel, O_RDONLY | O_NONBLOCK);
+       instance->fd_pairs.pair[instance->fd_pairs.num_pairs-1].channel = fd;
+
        if (instance->fd_pairs.pair[instance->fd_pairs.num_pairs-1].channel == -1) {
                perror(path_channel);
                instance->fd_pairs.num_pairs--;

diff --git a/liblttd/liblttd.h b/liblttd/liblttd.h
index 0b6b0584868411a6c870330d3b802de507ec7409..aac1e5dcd0b2bc74d095a2765ab573aa8a565cae 100644 (file)
--- a/liblttd/liblttd.h
+++ b/liblttd/liblttd.h
@@ -27,6 +27,7 @@
 
 #include <pthread.h>
 #include <dirent.h>
+#include <fcntl.h>
 
 /**
  * struct fd_pair - Contains the data associated with the channel file
@@ -39,6 +40,7 @@
  * @mmap: Not used anymore.
  * @mutex: a mutex for internal library usage
  * @user_data: library user data
+ * @offset: write position in the output file descriptor (optional)
  */
 struct fd_pair {
        int channel;
@@ -47,6 +49,7 @@ struct fd_pair {
        void *mmap;
        pthread_mutex_t mutex;
        void *user_data;
+       off_t offset;
 };
 
 struct channel_trace_fd {

diff --git a/liblttd/liblttdvfs.c b/liblttd/liblttdvfs.c
index 7b6ef0e685bc28e294456a957205ecd35f318043..6e28c9bd6937ebbe86c20b4e3c4a9cb1f871fa84 100644 (file)
--- a/liblttd/liblttdvfs.c
+++ b/liblttd/liblttdvfs.c
@@ -35,7 +35,7 @@
 
 #define _REENTRANT
 #define _GNU_SOURCE
-
+#define _XOPEN_SOURCE 600
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -73,6 +73,7 @@ int liblttdvfs_on_open_channel(struct liblttd_callbacks *data, struct fd_pair *p
        int ret;
        struct stat stat_buf;
        struct liblttdvfs_channel_data *channel_data;
+       off_t offset = 0;
 
        pair->user_data = malloc(sizeof(struct liblttdvfs_channel_data));
        channel_data = pair->user_data;
@@ -94,8 +95,8 @@ int liblttdvfs_on_open_channel(struct liblttd_callbacks *data, struct fd_pair *p
                                open_ret = -1;
                                goto end;
                        }
-                       ret = lseek(channel_data->trace, 0, SEEK_END);
-                       if (ret < 0) {
+                       offset = lseek(channel_data->trace, 0, SEEK_END);
+                       if (offset < 0) {
                                perror(callbacks_data->path_trace);
                                open_ret = -1;
                                close(channel_data->trace);
@@ -115,9 +116,13 @@ int liblttdvfs_on_open_channel(struct liblttd_callbacks *data, struct fd_pair *p
                                open_ret = -1;
                                goto end;
                        }
+                       offset = 0;
+               } else {
+                       perror("Channel output file open");
+                       open_ret = -1;
+                       goto end;
                }
        }
-
 end:
        return open_ret;
 
@@ -157,6 +162,8 @@ int liblttdvfs_on_read_subbuffer(struct liblttd_callbacks *data, struct fd_pair
 {
        long ret;
        off_t offset = 0;
+       off_t orig_offset = pair->offset;
+       int outfd = ((struct liblttdvfs_channel_data *)(pair->user_data))->trace;
 
        struct liblttdvfs_data* callbacks_data = data->user_data;
 
@@ -168,20 +175,48 @@ int liblttdvfs_on_read_subbuffer(struct liblttd_callbacks *data, struct fd_pair
                printf_verbose("splice chan to pipe ret %ld\n", ret);
                if (ret < 0) {
                        perror("Error in relay splice");
-                       goto write_error;
+                       goto write_end;
                }
-               ret = splice(thread_pipe[0], NULL,
-                       ((struct liblttdvfs_channel_data *)(pair->user_data))->trace,
+               ret = splice(thread_pipe[0], NULL, outfd,
                        NULL, ret, SPLICE_F_MOVE | SPLICE_F_MORE);
                printf_verbose("splice pipe to file %ld\n", ret);
                if (ret < 0) {
                        perror("Error in file splice");
-                       goto write_error;
+                       goto write_end;
                }
                len -= ret;
-       }
+               /*
+                * Give hints to the kernel about how we access the file:
+                * POSIX_FADV_DONTNEED : we won't re-access data in a near
+                * future after we write it.
+                * We need to call fadvise again after the file grows because
+                * the kernel does not seem to apply fadvise to non-existing
+                * parts of the file.
+                */
+               ret = posix_fadvise(outfd, 0, 0, POSIX_FADV_DONTNEED);
+               if (ret != 0) {
+                       perror("fadvise");
+                       /* Just a hint, not critical. Continue. */
+                       ret = 0;
+               }
 
-write_error:
+               /* This won't block, but will start writeout asynchronously */
+               sync_file_range(outfd, pair->offset, ret,
+                               SYNC_FILE_RANGE_WRITE);
+               pair->offset += ret;
+       }
+write_end:
+       /*
+        * This does a blocking write-and-wait on any page that belongs to the
+        * subbuffer prior to the one we just wrote.
+        */
+       if (orig_offset >= pair->max_sb_size)
+               sync_file_range(outfd, orig_offset - pair->max_sb_size,
+                               pair->max_sb_size,
+                               SYNC_FILE_RANGE_WAIT_BEFORE
+                               | SYNC_FILE_RANGE_WRITE
+                               | SYNC_FILE_RANGE_WAIT_AFTER);
+       
        return ret;
 }