Re: RFC: lseek() extensions: SEEK_HOLE / SEEK_DATA with patch

[David Holland <dholland-tech%netbsd.org@localhost>]

On Sun, Aug 07, 2011 at 10:03:34PM +0200, Reinoud Zandijk wrote:
 > > > > > i've implemented the SEEK_HOLE / SEEK_DATA additions to lseek() as
 > > > > > introduced by Solaris for ZFS.
 > > > One `seeks' the next hole of data block in the file from a given
 > > > pos? Sounds logical to me.
 > > 
 > > But you don't. You want to return the position of the hole, but
 > > setting the current file position (seeking) to it in the file is
 > 
 > The setting of the file position is not done in this code; its done in
 > libc/stdio.c's __sseek:113.

Um. We're talking about the lseek *system call*, not about stdio. What
does lseek do? It sets the file position... and yes, the code you
wrote in your patch sets the file position when it does SEEK_HOLE and
SEEK_DATA.

 > > Even your example code does this: it seeks repeatedly, teleporting the
 > > seek pointer all over the place for no reason, then does a bunch of
 > > reads.
 > > > So yes, it changes the file pointer too but thats a side-effect
 > 
 > See above; if it didn't change the file pointer then it would still work...

But it does change the file pointer, and it shouldn't.

 > >  > and the file pointer is not meant to be used by multiple threads
 > >  > anyway. Just use pread()/pwrite() with mutexes. 
 > > 
 > > ...precisely! You cannot safely use this API from multiple threads. A
 > > proper API for this functionality, that didn't involve the seek
 > > pointer for no reason, would be thread-safe.
 > 
 > If you agree with me that the file pointer ought not to be used in
 > multi-threading applications, then why would it matter if it changes?

If you're writing a background thread that runs behind legacy
sequential code, for example.

Furthermore, writing trash into shared state on the grounds that
nobody "should be" using it is extremely poor design.

 > >  >     while (pos < extent) {
 > >  >         data = lseek(fdi, pos, SEEK_DATA);
 > >  >         hole = lseek(fdi, data, SEEK_HOLE);
 > >  >         printf("DATA = %"PRIi64"\n", data);
 > >  >         printf("HOLE = %"PRIi64"\n", hole);
 > >  >         ....
 > 
 > If you read the manpage you can see that the lseek() *does* look at
 > the position given; if its inside a hole or inside an datablock it
 > will repond on this. It won't allways give the NEXT one, it will
 > give the first one of that type. So if the position `pos' refers to
 > is a DATA, it will return 'pos' and the next call to seek the HOLE
 > won't give a different result.

so?

 > > So? Not is that not thread-safe, it also doesn't actually copy
 > > anything. How about:
 > 
 > I left that piece out; it could have been:
 > 
 >      pos = 0;
 >      for (;;) {
 >              data = lseek(fdi, pos, SEEK_DATA);
 >              if (data < 0)
 >                      break;
 >              hole = lseek(fdi, data, SEEK_HOLE);
 >              amount = MAX(sizeof(buf, hole-data);
 >              pread(fdi, buf, amount, pos);
 >              pwrite(outfd, buf, amount, pos);
 >              pos += amount;
 >      }

That skips the data and copies the holes.

 > This code has the benefit to be able to handle growing files too; a second
 > thread might have added stuff to the file since the copy started.

So will any proper interface.

 > >    struct range range;
 > > 
 > >    while (pos < len) {
 > >       fgetrange(fd, pos, &range);
 > >       if (range.r_type == RANGE_DATA) {
 > >          amount = range.r_end - pos;
 > >          if (amount > sizeof(buf)) {
 > >             amount = sizeof(buf);
 > >          }
 > >          pread(fd, buf, amount, pos);
 > >          pwrite(outfd, buf, amount, pos);
 > >       }
 > >       pos = range.r_end;
 > >    }
 > 
 > I presume you meant pos += amount?

Yes, I suppose otherwise it won't quite work unless the buffer is
infinitely long. Hardly important...

 > >  > And yes, it could be changed during the two calls but thats a race
 > >  > condition anyway.
 > > 
 > > Yes THAT'S THE POINT
 > 
 > The file position changing by another thread between the two calls is
 > irrelevant since the call looks at the given file pointer and is not looking
 > at the current file pointer at all.

It is nonetheless causing the file position to flap. There is nothing
good about that.

 > Its due to stdio.h that it changes the
 > file pointer on each non-null kernel return. Heck, why not implement a
 > tread-local-storage file pointer now we're here :)

Um, perhaps because you have no idea what you're talking about...

 > >  > I don't think whatever interface one uses that
 > >  > you can access a single file without careful design.
 > > 
 > > Buncombe.
 > 
 > A thread-local-storage file pointer will help i agree, but then you
 > still need application-layer protection between threads to not
 > overwrite or modify file data.

Nope. Buncombe. The interface I suggested above is fully thread-safe.

 > >  > Since its also used by Solaris and Linux (work-in-progress) 
 > > 
 > > That doesn't make it right...
 > 
 > True... but does it make sense to have a file pointer at all other
 > than a convenient way to ease sequentialised read/write in one
 > thread without having to keep the position manually?

You're welcome to throw out one of the most fundamental properties of
the Unix API... in your own private branch.

And if you think it doesn't make sense to have a file pointer at all,
why are you wedging this unrelated functionality into the code that
adjusts the file pointer?

-- 
David A. Holland
dholland%netbsd.org@localhost