cUnbufferedFile and NFS mounted video dir

Message ID 43D9404C.1020602@o2.pl
State New
Headers

Commit Message

Artur Skawina Jan. 26, 2006, 9:34 p.m. UTC
  Andreas Holzhammer - GMX wrote:
> I'm using a diskless VDR 1.3.38 and noticed that during recordings the 
> network is saturated for 1-2 seconds every 30 seconds. I haven't seen 
> this with 1.3.22, so I asume this might be related to the 
> cUnbufferedFile class in 1.3.35+.
> 
> This leads to very uneven load on the vdr client (and the NFS-Server), 
> where I've seen 10000 interrupts per second during the flushes (a few 
> hundred irqs in between). I'm not sure whether this contributes to some 
> of my recordings being broken (c*Repacker errors), but I'd like to 
> distribute the writes more evenly.
> 
> Is there a way to get cUnbufferedFile to write the data every second, er 
> even continuously? The NFS-server is going to take care of writing the 
> data to disk anyway.

the attached patch makes vdr behave.

i did this for the reasons you mention above; haven't seen any repacker errors 
though (neither w/ or w/o the patch).
  

Comments

Klaus Schmidinger Jan. 26, 2006, 10:53 p.m. UTC | #1
Artur Skawina wrote:
> Andreas Holzhammer - GMX wrote:
> 
>> I'm using a diskless VDR 1.3.38 and noticed that during recordings the 
>> network is saturated for 1-2 seconds every 30 seconds. I haven't seen 
>> this with 1.3.22, so I asume this might be related to the 
>> cUnbufferedFile class in 1.3.35+.
>>
>> This leads to very uneven load on the vdr client (and the NFS-Server), 
>> where I've seen 10000 interrupts per second during the flushes (a few 
>> hundred irqs in between). I'm not sure whether this contributes to 
>> some of my recordings being broken (c*Repacker errors), but I'd like 
>> to distribute the writes more evenly.
>>
>> Is there a way to get cUnbufferedFile to write the data every second, 
>> er even continuously? The NFS-server is going to take care of writing 
>> the data to disk anyway.
> 
> 
> the attached patch makes vdr behave.
> ...
> --- vdr-1.3.39.org/cutter.c	2005-10-31 12:26:44.000000000 +0000
> +++ vdr-1.3.39/cutter.c	2006-01-15 18:31:51.000000000 +0000
> ...
> @@ -118,10 +121,11 @@ void cCuttingThread::Action(void)
>                   break;
>                if (FileSize > MEGABYTE(Setup.MaxVideoFileSize)) {
>                   toFile = toFileName->NextFile();
> -                 if (toFile < 0) {
> +                 if (!toFile) {
>...
> @@ -158,10 +162,11 @@ void cCuttingThread::Action(void)
>                   cutIn = true;
>                   if (Setup.SplitEditedFiles) {
>                      toFile = toFileName->NextFile();
> -                    if (toFile < 0) {
> +                    if (!toFile) {
>...

Are there more than these two actual bugfixes hidden in your patch?
It might be a good idea to post such fixes separately, because I
would certainly adopt these right away ;-)

Klaus
  
André Weidemann Jan. 26, 2006, 11:21 p.m. UTC | #2
Artur Skawina wrote:

> the attached patch makes vdr behave.
> 
> i did this for the reasons you mention above; haven't seen any repacker 
> errors though (neither w/ or w/o the patch).
> 
I have tested your v5 patch before and it worked great.

Option 1 has no effect at all here on my machine, but option 2 writes 
the recordings continuously over NFS to my server.
Thank you very much for this nice patch.

BTW: Ever since I changed from kernel 2.4 to 2.6 I had this problems of 
burst writing... even with versions before 1.3.25.

Thanks again.
  André.
  
Artur Skawina Jan. 27, 2006, 2:05 p.m. UTC | #3
Klaus Schmidinger wrote:
> Artur Skawina wrote:
>> --- vdr-1.3.39.org/cutter.c    2005-10-31 12:26:44.000000000 +0000
>> +++ vdr-1.3.39/cutter.c    2006-01-15 18:31:51.000000000 +0000
>> ...
>> @@ -118,10 +121,11 @@ void cCuttingThread::Action(void)
>>                   break;
>>                if (FileSize > MEGABYTE(Setup.MaxVideoFileSize)) {
>>                   toFile = toFileName->NextFile();
>> -                 if (toFile < 0) {
>> +                 if (!toFile) {
>> ...
>> @@ -158,10 +162,11 @@ void cCuttingThread::Action(void)
>>                   cutIn = true;
>>                   if (Setup.SplitEditedFiles) {
>>                      toFile = toFileName->NextFile();
>> -                    if (toFile < 0) {
>> +                    if (!toFile) {
>> ...
> 
> Are there more than these two actual bugfixes hidden in your patch?
> It might be a good idea to post such fixes separately, because I
> would certainly adopt these right away ;-)

i think these were the only two obvious ones; noticed them while playing w/ 
cutting readahead sizes; didn't think they were serious enough, so just fixed 
them in place.

However i consider the whole patch a bugfix :) Now that the fsyncs are disabled 
it probably isn't that critical though.

Having some kind of pluggable storage backend would be even better, as it would 
eg make nfs unnecessary and allow cache control and O_DIRECT even when using 
remote storage.

artur
  
Klaus Schmidinger Jan. 28, 2006, 1:51 p.m. UTC | #4
Artur Skawina wrote:
> Klaus Schmidinger wrote:
> 
>> Artur Skawina wrote:
>>
>>> --- vdr-1.3.39.org/cutter.c    2005-10-31 12:26:44.000000000 +0000
>>> +++ vdr-1.3.39/cutter.c    2006-01-15 18:31:51.000000000 +0000
>>> ...
>>> @@ -118,10 +121,11 @@ void cCuttingThread::Action(void)
>>>                   break;
>>>                if (FileSize > MEGABYTE(Setup.MaxVideoFileSize)) {
>>>                   toFile = toFileName->NextFile();
>>> -                 if (toFile < 0) {
>>> +                 if (!toFile) {
>>> ...
>>> @@ -158,10 +162,11 @@ void cCuttingThread::Action(void)
>>>                   cutIn = true;
>>>                   if (Setup.SplitEditedFiles) {
>>>                      toFile = toFileName->NextFile();
>>> -                    if (toFile < 0) {
>>> +                    if (!toFile) {
>>> ...
>>
>>
>> Are there more than these two actual bugfixes hidden in your patch?
>> It might be a good idea to post such fixes separately, because I
>> would certainly adopt these right away ;-)
> 
> 
> i think these were the only two obvious ones; noticed them while playing 
> w/ cutting readahead sizes; didn't think they were serious enough, so 
> just fixed them in place.
> 
> However i consider the whole patch a bugfix :) Now that the fsyncs are 
> disabled it probably isn't that critical though.

So does this mean that without the fdatasync() calls it works ok as
it is in plain vanilla VDR (without any patch)?

What is the exact meaning of the "WriteStrategy" option?
Does it simply turn the fadvise stuff on/off, or is there more
magic behind it? I would prefer a version that works without this
option, because this is nothing a normal user would easily know
how to set.

So, if you can provide a patch against the latest VDR version that
still uses "#ifdef USE_FADVISE" to completely turn the fadvise
stuff off, and does _not_ introduce another setup option, I might
consider including it for the final version 1.4.

Klaus
  

Patch

--- vdr-1.3.39.org/config.c	2006-01-13 15:19:37.000000000 +0000
+++ vdr-1.3.39/config.c	2006-01-15 18:31:51.000000000 +0000
@@ -424,6 +424,7 @@  bool cSetup::Parse(const char *Name, con
   else if (!strcasecmp(Name, "UseSmallFont"))        UseSmallFont       = atoi(Value);
   else if (!strcasecmp(Name, "MaxVideoFileSize"))    MaxVideoFileSize   = atoi(Value);
   else if (!strcasecmp(Name, "SplitEditedFiles"))    SplitEditedFiles   = atoi(Value);
+  else if (!strcasecmp(Name, "WriteStrategy"))       WriteStrategy      = atoi(Value);
   else if (!strcasecmp(Name, "MinEventTimeout"))     MinEventTimeout    = atoi(Value);
   else if (!strcasecmp(Name, "MinUserInactivity"))   MinUserInactivity  = atoi(Value);
   else if (!strcasecmp(Name, "MultiSpeedMode"))      MultiSpeedMode     = atoi(Value);
@@ -491,6 +492,7 @@  bool cSetup::Save(void)
   Store("UseSmallFont",       UseSmallFont);
   Store("MaxVideoFileSize",   MaxVideoFileSize);
   Store("SplitEditedFiles",   SplitEditedFiles);
+  Store("WriteStrategy",      WriteStrategy);
   Store("MinEventTimeout",    MinEventTimeout);
   Store("MinUserInactivity",  MinUserInactivity);
   Store("MultiSpeedMode",     MultiSpeedMode);
--- vdr-1.3.39.org/config.h	2006-01-13 15:17:19.000000000 +0000
+++ vdr-1.3.39/config.h	2006-01-15 18:31:51.000000000 +0000
@@ -231,6 +231,7 @@  public:
   int UseSmallFont;
   int MaxVideoFileSize;
   int SplitEditedFiles;
+  int WriteStrategy;
   int MinEventTimeout, MinUserInactivity;
   int MultiSpeedMode;
   int ShowReplayMode;
--- vdr-1.3.39.org/cutter.c	2005-10-31 12:26:44.000000000 +0000
+++ vdr-1.3.39/cutter.c	2006-01-15 18:31:51.000000000 +0000
@@ -66,6 +66,8 @@  void cCuttingThread::Action(void)
      toFile = toFileName->Open();
      if (!fromFile || !toFile)
         return;
+     fromFile->setreadahead(MEGABYTE(20));
+     toFile->setreadahead(MEGABYTE(20));
      int Index = Mark->position;
      Mark = fromMarks.Next(Mark);
      int FileSize = 0;
@@ -90,6 +92,7 @@  void cCuttingThread::Action(void)
            if (fromIndex->Get(Index++, &FileNumber, &FileOffset, &PictureType, &Length)) {
               if (FileNumber != CurrentFileNumber) {
                  fromFile = fromFileName->SetOffset(FileNumber, FileOffset);
+                 fromFile->setreadahead(MEGABYTE(20));
                  CurrentFileNumber = FileNumber;
                  }
               if (fromFile) {
@@ -118,10 +121,11 @@  void cCuttingThread::Action(void)
                  break;
               if (FileSize > MEGABYTE(Setup.MaxVideoFileSize)) {
                  toFile = toFileName->NextFile();
-                 if (toFile < 0) {
+                 if (!toFile) {
                     error = "toFile 1";
                     break;
                     }
+                 toFile->setreadahead(MEGABYTE(20));
                  FileSize = 0;
                  }
               LastIFrame = 0;
@@ -158,10 +162,11 @@  void cCuttingThread::Action(void)
                  cutIn = true;
                  if (Setup.SplitEditedFiles) {
                     toFile = toFileName->NextFile();
-                    if (toFile < 0) {
+                    if (!toFile) {
                        error = "toFile 2";
                        break;
                        }
+                    toFile->setreadahead(MEGABYTE(20));
                     FileSize = 0;
                     }
                  }
--- vdr-1.3.39.org/menu.c	2006-01-15 15:02:36.000000000 +0000
+++ vdr-1.3.39/menu.c	2006-01-15 18:31:51.000000000 +0000
@@ -2529,6 +2529,7 @@  cMenuSetupRecord::cMenuSetupRecord(void)
   Add(new cMenuEditIntItem( tr("Setup.Recording$Instant rec. time (min)"),   &data.InstantRecordTime, 1, MAXINSTANTRECTIME));
   Add(new cMenuEditIntItem( tr("Setup.Recording$Max. video file size (MB)"), &data.MaxVideoFileSize, MINVIDEOFILESIZE, MAXVIDEOFILESIZE));
   Add(new cMenuEditBoolItem(tr("Setup.Recording$Split edited files"),        &data.SplitEditedFiles));
+  Add(new cMenuEditIntItem( tr("Setup.Recording$Write strategy"),            &data.WriteStrategy, 0, 2));
 }
 
 // --- cMenuSetupReplay ------------------------------------------------------
--- vdr-1.3.39.org/tools.c	2006-01-15 14:31:45.000000000 +0000
+++ vdr-1.3.39/tools.c	2006-01-15 18:31:51.000000000 +0000
@@ -7,6 +7,7 @@ 
  * $Id: tools.c 1.110 2006/01/15 14:31:45 kls Exp $
  */
 
+#include "config.h"
 #include "tools.h"
 #include <ctype.h>
 #include <dirent.h>
@@ -1040,11 +1041,12 @@  bool cSafeFile::Close(void)
 
 // --- cUnbufferedFile -------------------------------------------------------
 
-//#define USE_FADVISE
+#define USE_FADVISE
 
-#define READ_AHEAD MEGABYTE(2)
-#define WRITE_BUFFER MEGABYTE(10)
+#define WRITE_BUFFER KILOBYTE(800)
 
+#define usefadvise() (Setup.WriteStrategy!=1)
+                               
 cUnbufferedFile::cUnbufferedFile(void)
 {
   fd = -1;
@@ -1059,8 +1061,18 @@  int cUnbufferedFile::Open(const char *Fi
 {
   Close();
   fd = open(FileName, Flags, Mode);
-  begin = end = ahead = -1;
+  curpos = 0;
+  begin = lastpos = ahead = 0;
+  readahead = 128*1024;
+  pendingreadahead = 0;
   written = 0;
+  totwritten = 0;
+  if (fd >= 0) {
+     // we really mean POSIX_FADV_SEQUENTIAL, but we do our own readahead
+     // so turn off the kernel one.
+     if (usefadvise())
+        posix_fadvise(fd, 0, 0, POSIX_FADV_RANDOM);
+     }
   return fd;
 }
 
@@ -1068,15 +1080,10 @@  int cUnbufferedFile::Close(void)
 {
 #ifdef USE_FADVISE
   if (fd >= 0) {
-     if (ahead > end)
-        end = ahead;
-     if (begin >= 0 && end > begin) {
-        //dsyslog("close buffer: %d (flush: %d bytes, %ld-%ld)", fd, written, begin, end);
-        if (written)
-           fdatasync(fd);
-        posix_fadvise(fd, begin, end - begin, POSIX_FADV_DONTNEED);
-        }
-     begin = end = ahead = -1;
+     if (totwritten)    // if we wrote anything make sure the data has hit the disk before
+        fdatasync(fd);  // calling fadvise, as this is our last chance to un-cache it.
+     posix_fadvise(fd, 0, 0, POSIX_FADV_DONTNEED);
+     begin = lastpos = ahead = 0;
      written = 0;
      }
 #endif
@@ -1085,45 +1092,98 @@  int cUnbufferedFile::Close(void)
   return close(OldFd);
 }
 
-off_t cUnbufferedFile::Seek(off_t Offset, int Whence)
-{
-  if (fd >= 0)
-     return lseek(fd, Offset, Whence);
-  return -1;
-}
-
+// When replaying and going eg FF->PLAY the position jumps back 2..8M
+// hence we might not want to drop that data at once. 
+// Ignoring this for now to avoid making this even more complex,
+// but we could at least try to handle the common cases.
+// (PLAY->FF->PLAY, small jumps, moving editing marks etc)
+
+#define KREADAHEAD MEGABYTE(4) // amount of kernel/fs prefetch that could
+                               // happen in addition to our own.
+#define FADVGRAN   4096        // AKA fadvise-chunk-size; PAGE_SIZE or
+                               // getpagesize(2) would also work.
+                               
 ssize_t cUnbufferedFile::Read(void *Data, size_t Size)
 {
   if (fd >= 0) {
 #ifdef USE_FADVISE
-     off_t pos = lseek(fd, 0, SEEK_CUR);
-     // jump forward - adjust end position
-     if (pos > end)
-        end = pos;
-     // after adjusting end - don't clear more than previously requested
-     if (end > ahead)
-        end = ahead;
-     // jump backward - drop read ahead of previous run
-     if (pos < begin)
-        end = ahead;
-     if (begin >= 0 && end > begin)
-        posix_fadvise(fd, begin - KILOBYTE(200), end - begin + KILOBYTE(200), POSIX_FADV_DONTNEED);//XXX macros/parameters???
-     begin = pos;
+     off_t jumped = curpos-lastpos; // nonzero means we're not at the last offset
+     if (jumped) {                  // ie some kind of jump happened.
+        pendingreadahead += ahead-lastpos+KREADAHEAD;
+        // jumped forward? - treat as if we did read all the way to current pos.
+        if (jumped >= 0) {
+           lastpos = curpos;
+           // but clamp at ahead so we don't clear more than previously requested.
+           // (would be mostly harmless anyway, unless we got more than one reader of this file)
+           if (lastpos > (ahead+KREADAHEAD))
+              lastpos = ahead+KREADAHEAD;
+        }
+        // jumped backward? - drop both last read _and_ read-ahead
+        else
+          if (curpos < begin)
+              lastpos = ahead+KREADAHEAD;
+           // jumped backward, but still inside prev read window? - pretend we read less.
+           else /* if (curpos >= begin) */
+              lastpos = curpos;
+        }
+        
 #endif
      ssize_t bytesRead = safe_read(fd, Data, Size);
 #ifdef USE_FADVISE
+     // Now drop data accessed during _previous_ Read(). (ie. begin...lastpos)
+     // fadvise() internally has page granularity; it drops only whole pages
+     // from the range we specify (since it cannot know if we will be accessing
+     // the rest). This is why we drop only the previously read data, and do it
+     // _after_ the current read() call, while rounding up the window to make
+     // sure that even not PAGE_SIZE-aligned data gets freed.
+     // Try to merge the fadvise calls a bit in order to reduce overhead.
+     if (usefadvise() && begin >= 0 && lastpos > begin)
+        if (jumped || (size_t)(lastpos-begin) > readahead) {
+           //dsyslog("taildrop:    %ld..%ld size %ld", begin, lastpos, lastpos-begin);
+           posix_fadvise(fd, begin-(FADVGRAN-1), lastpos-begin+(FADVGRAN-1)*2, POSIX_FADV_DONTNEED);
+           begin = curpos;
+           }
+        
      if (bytesRead > 0) {
-        pos += bytesRead;
-        end = pos;
-        // this seems to trigger a non blocking read - this
-        // may or may not have been finished when we will be called next time.
-        // If it is not finished we can't release the not yet filled buffers.
-        // So this is commented out till we find a better solution.
-        //posix_fadvise(fd, pos, READ_AHEAD, POSIX_FADV_WILLNEED);
-        ahead = pos + READ_AHEAD;
+        curpos += bytesRead;
+        //dsyslog("jump: %06ld ra: %06ld size: %ld", jumped, (long)readahead, (long)Size);
+
+        // no jump? (allow small forward jump still inside readahead window).
+        if (usefadvise() && jumped>=0 && jumped<=(off_t)readahead) {
+           // Trigger the readahead IO, but only if we've used at least
+           // 1/4 of the previously requested area. This avoids calling
+           // fadvise() after every read() call.
+           //if (ahead<(off_t)(curpos+readahead/2)) {
+           if ((ahead-curpos)<(off_t)(readahead-readahead/4)) {
+              posix_fadvise(fd, curpos, readahead, POSIX_FADV_WILLNEED);
+              ahead = curpos + readahead;
+              }
+           if ( readahead < Size*64 ) { // automagically tune readahead size.
+              readahead = Size*64;
+              //dsyslog("Readahead for fd: %d increased to %ld", fd, (long)readahead);
+              }
+           }
+        else {
+           // jumped - we really don't want any readahead now. otherwise
+           // eg fast-rewind gets in trouble.
+           //ahead = curpos;
+           // let's try a little readahead...
+           posix_fadvise(fd, curpos, KILOBYTE(32), POSIX_FADV_WILLNEED);
+           ahead = curpos + KILOBYTE(32);
+
+           // Every now and then, flush all cached data from this file; mostly
+           // to get rid of nonflushed readahead coming from _previous_ jumps
+           // (if the readahead I/O hasn't finished by the time we called
+           // fadvice() to undo it, the data could still be cached).
+           // The accounting does not have to be 100% accurate, as long as
+           // this triggers after _some_ jumps we should be ok.
+           if (usefadvise() && pendingreadahead>MEGABYTE(40)) {
+              pendingreadahead = 0;
+              posix_fadvise(fd, 0, 0, POSIX_FADV_DONTNEED);
+              }
+           }
         }
-     else
-        end = pos;
+     lastpos = curpos;
 #endif
      return bytesRead;
      }
@@ -1132,29 +1192,37 @@  ssize_t cUnbufferedFile::Read(void *Data
 
 ssize_t cUnbufferedFile::Write(const void *Data, size_t Size)
 {
+  //dsyslog("Unbuffered:Write: fd: %d  %8ld..%8ld  size: %5ld", fd, curpos, curpos+Size, (long)Size);
   if (fd >=0) {
-#ifdef USE_FADVISE
-     off_t pos = lseek(fd, 0, SEEK_CUR);
-#endif
      ssize_t bytesWritten = safe_write(fd, Data, Size);
 #ifdef USE_FADVISE
-     if (bytesWritten >= 0) {
+     if (bytesWritten > 0) {
+        begin = min(begin, curpos);
+        curpos += bytesWritten;
         written += bytesWritten;
-        if (begin >= 0) {
-           if (pos < begin)
-              begin = pos;
-           }
-        else
-           begin = pos;
-        if (pos + bytesWritten > end)
-           end = pos + bytesWritten;
+        lastpos = max(lastpos, curpos);
         if (written > WRITE_BUFFER) {
-           //dsyslog("flush buffer: %d (%d bytes, %ld-%ld)", fd, written, begin, end);
-           fdatasync(fd);
-           if (begin >= 0 && end > begin)
-              posix_fadvise(fd, begin, end - begin, POSIX_FADV_DONTNEED);
-           begin = end = -1;
+           //dsyslog("flush buffer: %d (%d bytes, %ld-%ld)", fd, written, begin, lastpos);
+           if (usefadvise() && lastpos>begin) {
+              off_t headdrop = min(begin, WRITE_BUFFER*2L);
+              posix_fadvise(fd, begin-headdrop, lastpos-begin+headdrop, POSIX_FADV_DONTNEED);
+              }
+           begin = lastpos = max(0L, curpos-4095);
+           totwritten += written;
            written = 0;
+           // The above fadvise() works when writing slowly (recording), but could
+           // leave cached data around when writing at a high rate (cutting).
+           // Also, it seems in some setups, the above does not trigger any I/O and
+           // the fdatasync() call below has to do all the work (reiserfs with some
+           // kind of write gathering enabled).
+           // We add 'readahead' to the threshold in an attempt to increase cutting
+           // speed; it's a tradeoff -- speed vs RAM-used.
+           if (usefadvise() && totwritten>(MEGABYTE(32)+readahead)) {
+              //fdatasync(fd);
+              off_t headdrop = min(curpos-totwritten, totwritten*2L);
+              posix_fadvise(fd, curpos-totwritten-headdrop, totwritten+headdrop, POSIX_FADV_DONTNEED);
+              totwritten = 0;
+              }
            }
         }
 #endif
--- vdr-1.3.39.org/tools.h	2006-01-08 11:40:37.000000000 +0000
+++ vdr-1.3.39/tools.h	2006-01-26 21:02:24.000000000 +0000
@@ -237,22 +237,40 @@  public:
 /// cUnbufferedFile is used for large files that are mainly written or read
 /// in a streaming manner, and thus should not be cached.
 
+#include <sys/types.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
 class cUnbufferedFile {
 private:
   int fd;
+  off_t curpos;
   off_t begin;
-  off_t end;
+  off_t lastpos;
   off_t ahead;
-  ssize_t written;
+  size_t pendingreadahead;
+  size_t readahead;
+  size_t written;
+  size_t totwritten;
 public:
   cUnbufferedFile(void);
   ~cUnbufferedFile();
   int Open(const char *FileName, int Flags, mode_t Mode = DEFFILEMODE);
   int Close(void);
-  off_t Seek(off_t Offset, int Whence);
+  off_t Seek(off_t Offset, int Whence)
+  {
+    //dsyslog("Seek: fd: %d offs: %ld whence: %d diff: %ld", fd, (long)Offset, Whence, Offset-curpos);
+    if (Whence == SEEK_SET && Offset == curpos)
+       return curpos;
+
+    curpos = lseek(fd, Offset, Whence);
+    return curpos;
+  }
+
   ssize_t Read(void *Data, size_t Size);
   ssize_t Write(const void *Data, size_t Size);
   static cUnbufferedFile *Create(const char *FileName, int Flags, mode_t Mode = DEFFILEMODE);
+  void setreadahead(size_t ra) { readahead = ra; };
   };
 
 class cLockFile {