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

[Reinoud Zandijk <reinoud%NetBSD.org@localhost>]

Hi David, hi folks,

On Sun, Aug 07, 2011 at 05:06:16PM +0000, David Holland wrote:
> On Sun, Aug 07, 2011 at 06:52:03PM +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.

> 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...

>  > 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? No
application CAN count on it and doing-as-if will only hide bugs.

>  >     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? 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;
        }

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.

>    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?

>  > 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. 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 :)

>  > 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.

>  > 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?

With regards,
Reinoud