[vdr] [PATCH] UnbufferedFile improvements v7

[Artur Skawina]

Klaus Schmidinger wrote:
> Artur Skawina wrote:
>> this time with a new approach to read caching. Should make watching 
>> and editing recordings on a non-idle (and/or slow) machine more 
>> comfortable.
>>
>> The difference to previous versions (and stock fadvise-enabled vdr) is 
>> that previously, read data was almost immediately forgotten after it 
>> was used; now a certain amount of recently accessed data (at most 
>> ~16M) is kept around. This means that short seeks (jumps) when 
>> replaying do not cause disk accesses. Things like switching play mode, 
>> FF, setting and moving editing marks shouldn't usually block waiting 
>> for disk IO. The changes are most noticeable when eg. several 
>> recordings are happening in the background.
>>
>> I did very little testing, treat this as beta quality at best. Seems 
>> to work ok, but i won't probably have time to test it further for a 
>> few days; maybe somebody wants to play w/ this, or even better take a 
>> look at the Read() path...

after running w/ it for a week, i'd now say it works; haven't seen any problems 
that could be related to the Read() code.

> Attached is the patch as I would add it for the next version.

Why not the inlined Seeks()? The IO is very inefficient anyway (tiny read/write 
sizes), having an extra syscall (seek) for every read/write call doesn't help...
(yes, it may not be pretty, but it lets the compiler optimize the calls away, 
w/o touching any callers. Unless you fixed the callers, then ignore this)

> Please take a look at the lines marked CLARIFY and let me know
> if you have any comment on these.

i'll have to rediff this and compare w/ my current version; will post merged 
version later. Comments below.

> +++ cutter.c	2006/02/04 13:40:20
> @@ -66,6 +66,8 @@
>       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;
> @@ -122,6 +125,7 @@
>                      error = "toFile 1";
>                      break;
>                      }
> +                 toFile->SetReadAhead(MEGABYTE(20));
>                   FileSize = 0;
>                   }
>                LastIFrame = 0;
> @@ -162,6 +166,7 @@
>                         error = "toFile 2";
>                         break;
>                         }
> +                    toFile->SetReadAhead(MEGABYTE(20));
>                      FileSize = 0;
>                      }
>                   }

i've dropped the toFile->SetReadAhead() hack -- it doesn't change that much when 
the datasyncs aren't there anyway.

> +++ tools.c	2006/02/04 13:55:24
> +        // Read ahead:
> +        // no jump? (allow small forward jump still inside readahead window).
> +        if (jumped >= 0 && jumped <= (off_t)readahead) {
> +           // Trigger the readahead IO, but only if we've used at least
> +           // 1/2 of the previously requested area. This avoids calling
> +           // fadvise() after every read() call.
> +           if (ahead - curpos < (off_t)(readahead - readahead / 2)) {//XXX CLARIFY: isn't this just 'readahead / 2'???

historical reasons -- it used to be 1/4, not 1/2. I went back to 1/2 after 
switching to the new read caching strategy; previously i was seeing stalls when 
replaying and eg cutting at the same time. With the new code hopefully 1/2 is 
enough and reduces the number of fadvise calls.

> +           // 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 (totwritten > MEGABYTE(32) + readahead) {
> +              //fdatasync(fd);//XXX CLARIFY: so what, is this necessary or not???

it is not, at least not here, however i left it commented out in case there are 
setups where it actually helps -- it's easier to uncomment this one line for 
testing than to find the right place to insert the call. It's not on by default 
because fsyncs are relatively expensive; i'd rather avoid them unless absolutely 
required. (i've updated the comments in this area since, they will be in the 
merged version)

artur

[for faster responses please cc me; esp. if it's a reply to an older thread -- i 
almost missed this]