From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Tue, 26 Jun 2012 19:46:28 GMT
Subject: Re: Attempt to fix pread() threading issue on Cygwin and MinGW
Message-ID: <CABPQNSZWLvYGH6sFkWa588+fPJXV3c5ViKsEyMABViN74ghaLg@mail.gmail.com>
In-Reply-To: <4FEA06DE.6080101@ramsay1.demon.co.uk>

On Tue, Jun 26, 2012 at 9:00 PM, Ramsay Jones
<ramsay@ramsay1.demon.co.uk> wrote:
> Hi Johannes,
>
> I had an "fix pread() on MinGW" item low on my todo list. As we have
> seen recently, Cygwin has the same problem. So, I decided to try and
> fix it up last night, since I had an idea that I thought would work.
>
> Several years ago, I read somewhere (a Microsoft Press publication of
> some sort) that, even when using *synchonous* I/O, you could pass an
> OVERLAPPED structure to ReadFile() and have it use the file offset in
> that structure, rather than the implicit stream pointer.
>
> So, I modified my "hacked up test program" from the other day to try
> it out; the diff looks like:
>
> -- >8 --
> diff --git a/test-pread.c b/test-pread.c
> index 61280cb..8a2b2ff 100644
> --- a/test-pread.c
> +++ b/test-pread.c
> @@ -1,5 +1,6 @@
>  #include "git-compat-util.h"
>  #include "thread-utils.h"
> +#include <windows.h>
>
>  #define DATA_FILE "junk.data"
>  #define MAX_DATA 256 * 1024
> @@ -16,6 +17,31 @@ struct thread_data {
>
>  static struct thread_data t[NUM_THREADS+1];
>
> +ssize_t pread_(int fd, void *buf, size_t count, off_t offset)
> +{
> +       OVERLAPPED o = {0};
> +       HANDLE fh = (HANDLE)_get_osfhandle(fd);
> +       uint64_t off = offset;
> +       DWORD bytes;
> +       BOOL ret;
> +
> +       if (fh == INVALID_HANDLE_VALUE) {
> +               errno = EBADF;
> +               return -1;
> +       }
> +
> +       o.Offset = off & 0xffffffff;
> +       o.OffsetHigh = (off >> 32) & 0xffffffff;
> +
> +       ret = ReadFile(fh, buf, (DWORD)count, &bytes, &o);
> +       if (!ret) {
> +               errno = EIO;
> +               return -1;
> +       }
> +
> +       return (ssize_t)bytes;
> +}
> +
>  int create_data_file(void)
>  {
>        int i, fd = open(DATA_FILE, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> @@ -55,7 +81,7 @@ void *pread_thread(void *data)
>                ssize_t sz;
>                d->n = d->n * 1103515245 + 12345;
>                j = d->n % MAX_DATA;
> -               sz = pread(d->fd, &rd, sizeof(int), j * sizeof(int));
> +               sz = pread_(d->fd, &rd, sizeof(int), j * sizeof(int));
>                if (sz < 0 || rd != j)
>                        d->fails++;
>                d->cnt++;
>
> -- 8< --
>
> The result of running the updated program looks like:
>
>    ramsay $ gcc -I. -o test-pread test-pread.c
>    ramsay $ ./test-pread.exe
>     0: trials 524288, failed 262139
>     1: trials 500000, failed 0
>     2: trials 500000, failed 0
>     3: trials 500000, failed 0
>    ramsay $
>
> So, the results are a little disappointing. :(
>
> Although ReadFile() is indeed using the OVERLAPPED structure to specify
> the read position, it is still updating the implicit stream position.
>
> So, this implementation does not faithfully reproduce the full semantics
> of pread(), since it updates the stream position (affecting other I/O
> calls which rely on the implicit file position. eg read()).
>
> [Note: this was tested on Windows XP SP3. Maybe Vista/Win7/Win8 would
> behave differently?]
>
> This implementation should be sufficient to fix the immediate problem with
> the threaded index-pack, but I'm not sure it would be a good idea to
> provide an *almost* compliant pread().
>
> I've attached the full test program below, just for completeness.
>

We've been through this before:

http://thread.gmane.org/CABPQNSZ6VdyoLcVUUJ4z5A2A7KGP8qBZAkzdx8zAtAs2mZN25w@mail.gmail.com

My worries are the same as yours: An approach like this looks a bit
too much like pread, and might make someone else assume it works the
same.