On Fri, Feb 14, 2014 at 11:15 AM, Zachary Turner <ztur...@chromium.org> wrote: > For the mixed read, we wouldn't be looking for another caller of pread() > (since it doesn't care what the file pointer is), but instead a caller of > read() or lseek(). In index-pack.c, I see two possible culprits: > > 1) A call to xread() from inside fill() > 2) A call to lseek in parse_pack_objects() > > Do you think these could be related? If so, maybe that opens up some other > solutions?
>From my observations, it's not that simple. As you pointed out to me before, fill() is only called before the threading part of the code, and lseek is only called after the threading part; and the lseek() is lseek(fd, 0, SEEK_CUR), so it's purely advisory. Also, here is the error output we got before you added ReOpenFile: remote: Total 2514467 (delta 1997300), reused 2513040 (delta 1997113) Checking connectivity... error: packfile d:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack does not match index warning: packfile d:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack cannot be accessed error: packfile d:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack does not match index warning: packfile d:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack cannot be accessed error: packfile d:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack does not match index warning: packfile d:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack cannot be accessed fatal: bad object e0f9f23f765a45e6d80863a8f881ee735c9347fe The 'Checking connectivity...' message comes from builtin/clone.c, which runs in a separate process from builtin/index-pack.c. What this suggests to me is that file descriptors for the loose object files are not being flushed or closed properly before index-pack finishes. > BTW, the version you posted isn't thread safe. Suppose thread A and thread > B execute this function at the same time. A executes through the > ReadFile(), but does not yet reset the second lseek64. B then executes the > first lseek64(), storing off the modified file pointer. Then A finishes, > then B finishes. At the end, the file pointer is still modified. Yes, that. I would also point out that in our experiments, ReOpenFile is not nearly as expensive as its name might suggest. Since the solution using ReOpenFile is pretty solidly thread-safe (at least as far as we can tell), I'm in favor of using it unless or until we properly root-case the failure. Stefan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
