pread() over NFS (again) [1.5.5.4]

>   Receiving objects: 100% (253/253), 5.27 MiB | 9136 KiB/s, done.
>   fatal: cannot pread pack file: No such file or directory
>   fatal: index-pack failed
> 
> The end of the strace looks like so:
> pread(3, "", 205, 1373)                 = 0
> write(2, "fatal: cannot pread pack file: N"..., 57) = 57

> Christian Holtje <docwhat@gmail.com> wrote:
>> I have read all the threads on git having trouble with pread() and I  
>> didn't see anything to help.
> ...
>>   Receiving objects: 100% (253/253), 5.27 MiB | 9136 KiB/s, done.
>>   fatal: cannot pread pack file: No such file or directory
>>   fatal: index-pack failed
>> 
>> The end of the strace looks like so:
>> pread(3, "", 205, 1373)                 = 0
>> write(2, "fatal: cannot pread pack file: N"..., 57) = 57
>
> Hmmph.  So pread for a length of 205 can return 0 on NFS?  Is this
> a transient error?  If so, perhaps a patch like this might help:
>
> diff --git a/index-pack.c b/index-pack.c
> index 5ac91ba..737f757 100644
> --- a/index-pack.c
> +++ b/index-pack.c
> @@ -309,14 +309,19 @@ static void *get_data_from_pack(struct object_entry *obj)
>  	unsigned char *src, *data;
>  	z_stream stream;
>  	int st;
> +	int attempts = 0;
>  
>  	src = xmalloc(len);
>  	data = src;
>  	do {
>  		ssize_t n = pread(pack_fd, data + rdy, len - rdy, from + rdy);
> -		if (n <= 0)
> +		if (n <= 0) {
> +			if (n == 0 && ++attempts < 10)
> +				continue;
>  			die("cannot pread pack file: %s", strerror(errno));
> +		}
>  		rdy += n;
> +		attempts = 0;
>  	} while (rdy < len);
>  	data = xmalloc(obj->size);
>  	memset(&stream, 0, sizeof(stream));
>
>
> The file shouldn't be short unless someone truncated it, or there
> is a bug in index-pack.  Neither is very likely, but I don't think
> we would want to retry pread'ing the same block forever.

> "Shawn O. Pearce" <spearce@spearce.org> writes:
> > Christian Holtje <docwhat@gmail.com> wrote:
> >> I have read all the threads on git having trouble with pread() and I  
> >> didn't see anything to help.
> > ...
> >>   Receiving objects: 100% (253/253), 5.27 MiB | 9136 KiB/s, done.
> >>   fatal: cannot pread pack file: No such file or directory
> >>   fatal: index-pack failed
> >> 
> >> The end of the strace looks like so:
> >> pread(3, "", 205, 1373)                 = 0
> >> write(2, "fatal: cannot pread pack file: N"..., 57) = 57
> >
> > Hmmph.  So pread for a length of 205 can return 0 on NFS?  Is this
> > a transient error?  If so, perhaps a patch like this might help:

> > The file shouldn't be short unless someone truncated it, or there
> > is a bug in index-pack.  Neither is very likely, but I don't think
> > we would want to retry pread'ing the same block forever.
> 
> I don't think we would want to retry even once.  Return value of 0 from
> pread is defined to be an EOF, isn't it?

> Junio C Hamano <gitster@pobox.com> wrote:
>> "Shawn O. Pearce" <spearce@spearce.org> writes:
>>> Christian Holtje <docwhat@gmail.com> wrote:
>>>> I have read all the threads on git having trouble with pread()  
>>>> and I
>>>> didn't see anything to help.
>>> ...
>>>>  Receiving objects: 100% (253/253), 5.27 MiB | 9136 KiB/s, done.
>>>>  fatal: cannot pread pack file: No such file or directory
>>>>  fatal: index-pack failed
>>>>
>>>> The end of the strace looks like so:
>>>> pread(3, "", 205, 1373)                 = 0
>>>> write(2, "fatal: cannot pread pack file: N"..., 57) = 57
>>>
>>> Hmmph.  So pread for a length of 205 can return 0 on NFS?  Is this
>>> a transient error?  If so, perhaps a patch like this might help:
> ...
>>> The file shouldn't be short unless someone truncated it, or there
>>> is a bug in index-pack.  Neither is very likely, but I don't think
>>> we would want to retry pread'ing the same block forever.
>>
>> I don't think we would want to retry even once.  Return value of 0  
>> from
>> pread is defined to be an EOF, isn't it?
>
> Indeed, it is defined to be EOF, but EOF here makes no sense.
>
> We have a file position we saw once before as the start of a delta.
> We wrote it down to disk.  We want to go back and open it up, as
> we have the base decompressed and in memory and need to compute
> the SHA-1 of the object that resides at this offset.
>
> And *wham* we get an EOF.  Where there should be data.  Where we
> know there is data.
>
> I'm open to the idea that index-pack has a bug, but I doubt it.
> We shovel hundreds of megabytes through that on a daily basis
> across all of the git users, and nobody ever sees it crash out
> with an EOF in the middle of an object it knows to be present.
> Except poor Christian on NFS.
>
> Actually, I think the last time someone reported something like this
> in Git it turned out to be an NFS kernel bug.  I didn't quote it
> in my reply to him, but I think he did say this was a linux client,
> linux server.

> We have a file position we saw once before as the start of a delta.
> We wrote it down to disk.  We want to go back and open it up, as
> we have the base decompressed and in memory and need to compute
> the SHA-1 of the object that resides at this offset.
>
> And *wham* we get an EOF.  Where there should be data.  Where we
> know there is data.

> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > We have a file position we saw once before as the start of a delta.
> > We wrote it down to disk.  We want to go back and open it up, as
> > we have the base decompressed and in memory and need to compute
> > the SHA-1 of the object that resides at this offset.
> >
> > And *wham* we get an EOF.  Where there should be data.  Where we
> > know there is data.
> 
> We have written that earlier in the same process?  Are we playing games
> with mixed mmap() and pread()?  Is fsync() or msync() or unmap/remap
> needed?

> > Actually, I think the last time someone reported something like this
> > in Git it turned out to be an NFS kernel bug.  I didn't quote it
> > in my reply to him, but I think he did say this was a linux client,
> > linux server.
> 
> This is getting into the area Linus would immediately know the answer to,
> but he is away for this week.

>> "The file shouldn't be short unless someone truncated it, or there
>> is a bug in index-pack.  Neither is very likely, but I don't think
>> we would want to retry pread'ing the same block forever.
>
> I don't think we would want to retry even once.  Return value of 0  
> from
> pread is defined to be an EOF, isn't it?

> On Jun 26, 2008, at 1:56 PM, Junio C Hamano wrote:
>
>>> "The file shouldn't be short unless someone truncated it, or there
>>> is a bug in index-pack.  Neither is very likely, but I don't think
>>> we would want to retry pread'ing the same block forever.
>>
>> I don't think we would want to retry even once.  Return value of 0
>> from
>> pread is defined to be an EOF, isn't it?
>
> No, it seems to be a simple error-out in this case. We have 2.4.20
> systems with nfs-utils 0.3.3 and used to frequently get the same error
> while pushing. I made a similar change back in February and haven't
> had a problem since:
>
> diff --git a/index-pack.c b/index-pack.c
> index 5ac91ba..85c8bdb 100644
> --- a/index-pack.c
> +++ b/index-pack.c
> @@ -313,7 +313,14 @@ static void *get_data_from_pack(struct
> object_entry *obj)
> 	src = xmalloc(len);
> 	data = src;
> 	do {
> +		// It appears that if multiple threads read across NFS, the
> +		// second read will fail. I know this is awful, but we wait for
> +		// a little bit and try again.
> 		ssize_t n = pread(pack_fd, data + rdy, len - rdy, from + rdy);
> +		if (n <= 0) {
> +			sleep(1);
> +			n = pread(pack_fd, data + rdy, len - rdy, from + rdy);
> +		}
> 		if (n <= 0)
> 			die("cannot pread pack file: %s", strerror(errno));
> 		rdy += n;
>
> I use a sleep request since it seems less likely that the other thread
> will have an outstanding request after a second of waiting.

> logank@sent.com writes:
> 
> > On Jun 26, 2008, at 1:56 PM, Junio C Hamano wrote:
> >
> >>> "The file shouldn't be short unless someone truncated it, or there
> >>> is a bug in index-pack.  Neither is very likely, but I don't think
> >>> we would want to retry pread'ing the same block forever.
> >>
> >> I don't think we would want to retry even once.  Return value of 0
> >> from
> >> pread is defined to be an EOF, isn't it?
> >
> > No, it seems to be a simple error-out in this case. We have 2.4.20
> > systems with nfs-utils 0.3.3 and used to frequently get the same error
> > while pushing. I made a similar change back in February and haven't
> > had a problem since:
> >
> > diff --git a/index-pack.c b/index-pack.c
> > index 5ac91ba..85c8bdb 100644
> > --- a/index-pack.c
> > +++ b/index-pack.c
> > @@ -313,7 +313,14 @@ static void *get_data_from_pack(struct
> > object_entry *obj)
> > 	src = xmalloc(len);
> > 	data = src;
> > 	do {
> > +		// It appears that if multiple threads read across NFS, the
> > +		// second read will fail. I know this is awful, but we wait for
> > +		// a little bit and try again.
> > 		ssize_t n = pread(pack_fd, data + rdy, len - rdy, from + rdy);
> > +		if (n <= 0) {
> > +			sleep(1);
> > +			n = pread(pack_fd, data + rdy, len - rdy, from + rdy);
> > +		}
> > 		if (n <= 0)
> > 			die("cannot pread pack file: %s", strerror(errno));
> > 		rdy += n;
> >
> > I use a sleep request since it seems less likely that the other thread
> > will have an outstanding request after a second of waiting.
> 
> Gaah.  Don't we have NFS experts in house?  Bruce, perhaps?

> On Thu, Jun 26, 2008 at 04:38:40PM -0700, Junio C Hamano wrote:
> > logank@sent.com writes:
> > 
> > > On Jun 26, 2008, at 1:56 PM, Junio C Hamano wrote:
> > >
> > >>> "The file shouldn't be short unless someone truncated it, or there
> > >>> is a bug in index-pack.  Neither is very likely, but I don't think
> > >>> we would want to retry pread'ing the same block forever.
> > >>
> > >> I don't think we would want to retry even once.  Return value of 0
> > >> from
> > >> pread is defined to be an EOF, isn't it?
> > >
> > > No, it seems to be a simple error-out in this case. We have 2.4.20
> > > systems with nfs-utils 0.3.3 and used to frequently get the same error
> > > while pushing. I made a similar change back in February and haven't
> > > had a problem since:
> > >
> > > diff --git a/index-pack.c b/index-pack.c
> > > index 5ac91ba..85c8bdb 100644
> > > --- a/index-pack.c
> > > +++ b/index-pack.c
> > > @@ -313,7 +313,14 @@ static void *get_data_from_pack(struct
> > > object_entry *obj)
> > > 	src = xmalloc(len);
> > > 	data = src;
> > > 	do {
> > > +		// It appears that if multiple threads read across NFS, the
> > > +		// second read will fail. I know this is awful, but we wait for
> > > +		// a little bit and try again.
> > > 		ssize_t n = pread(pack_fd, data + rdy, len - rdy, from + rdy);
> > > +		if (n <= 0) {
> > > +			sleep(1);
> > > +			n = pread(pack_fd, data + rdy, len - rdy, from + rdy);
> > > +		}
> > > 		if (n <= 0)
> > > 			die("cannot pread pack file: %s", strerror(errno));
> > > 		rdy += n;
> > >
> > > I use a sleep request since it seems less likely that the other thread
> > > will have an outstanding request after a second of waiting.
> > 
> > Gaah.  Don't we have NFS experts in house?  Bruce, perhaps?
> 
> Trond, you don't have any idea why a 2.6.9-42.0.8.ELsmp client (2.4.28
> server) might be returning spurious 0's from pread()?
> 
> Seems like everything is happening from that one client--the file isn't
> being simultaneously accessed from the server or from another client.

> On Thu, 2008-06-26 at 22:57 -0400, J. Bruce Fields wrote:
> > On Thu, Jun 26, 2008 at 04:38:40PM -0700, Junio C Hamano wrote:
> > > logank@sent.com writes:
> > > 
> > > > On Jun 26, 2008, at 1:56 PM, Junio C Hamano wrote:
> > > >
> > > >>> "The file shouldn't be short unless someone truncated it, or there
> > > >>> is a bug in index-pack.  Neither is very likely, but I don't think
> > > >>> we would want to retry pread'ing the same block forever.
> > > >>
> > > >> I don't think we would want to retry even once.  Return value of 0
> > > >> from
> > > >> pread is defined to be an EOF, isn't it?
> > > >
> > > > No, it seems to be a simple error-out in this case. We have 2.4.20
> > > > systems with nfs-utils 0.3.3 and used to frequently get the same error
> > > > while pushing. I made a similar change back in February and haven't
> > > > had a problem since:
> > > >
> > > > diff --git a/index-pack.c b/index-pack.c
> > > > index 5ac91ba..85c8bdb 100644
> > > > --- a/index-pack.c
> > > > +++ b/index-pack.c
> > > > @@ -313,7 +313,14 @@ static void *get_data_from_pack(struct
> > > > object_entry *obj)
> > > > 	src = xmalloc(len);
> > > > 	data = src;
> > > > 	do {
> > > > +		// It appears that if multiple threads read across NFS, the
> > > > +		// second read will fail. I know this is awful, but we wait for
> > > > +		// a little bit and try again.
> > > > 		ssize_t n = pread(pack_fd, data + rdy, len - rdy, from + rdy);
> > > > +		if (n <= 0) {
> > > > +			sleep(1);
> > > > +			n = pread(pack_fd, data + rdy, len - rdy, from + rdy);
> > > > +		}
> > > > 		if (n <= 0)
> > > > 			die("cannot pread pack file: %s", strerror(errno));
> > > > 		rdy += n;
> > > >
> > > > I use a sleep request since it seems less likely that the other thread
> > > > will have an outstanding request after a second of waiting.
> > > 
> > > Gaah.  Don't we have NFS experts in house?  Bruce, perhaps?
> > 
> > Trond, you don't have any idea why a 2.6.9-42.0.8.ELsmp client (2.4.28
> > server) might be returning spurious 0's from pread()?
> > 
> > Seems like everything is happening from that one client--the file isn't
> > being simultaneously accessed from the server or from another client.
> 
> Is the file only being read, or could there be a simultaneous write to
> the same file? I'm surmising this could be an effect resulting from
> simultaneous cache invalidations: prior to Linux 2.6.20 or so, we
> weren't rigorously following the VFS/VM rules for page locking, and so
> page cache invalidation in particular could have some curious
> side-effects.

> Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> > Is the file only being read, or could there be a simultaneous write to
> > the same file? I'm surmising this could be an effect resulting from
> > simultaneous cache invalidations: prior to Linux 2.6.20 or so, we
> > weren't rigorously following the VFS/VM rules for page locking, and so
> > page cache invalidation in particular could have some curious
> > side-effects.
> 
> The file was created and opened O_CREAT|O_EXCL|O_RDWR, by this
> process, written linearly using write(2), without any lseeks.
> We kept the file descriptor open and starting issuing pread(2)
> calls for earlier offsets we had alread written.  One of those
> kicks back EOF far too early (and results in this bug report).
> 
> Note the only accesses we are using is write(2) and pread(2), and
> once we start reading we don't ever go back to writing.

> On Jun 26, 2008, at 1:56 PM, Junio C Hamano wrote:
>
>>> "The file shouldn't be short unless someone truncated it, or there
>>> is a bug in index-pack.  Neither is very likely, but I don't think
>>> we would want to retry pread'ing the same block forever.
>>
>> I don't think we would want to retry even once.  Return value of 0  
>> from
>> pread is defined to be an EOF, isn't it?
>
> No, it seems to be a simple error-out in this case. We have 2.4.20  
> systems

> with nfs-utils 0.3.3 and used to frequently get the same error  
> while pushing. I made a similar change back in February and haven't had a 
> problem since:
>
> diff --git a/index-pack.c b/index-pack.c
> index 5ac91ba..85c8bdb 100644
> --- a/index-pack.c
> +++ b/index-pack.c
> @@ -313,7 +313,14 @@ static void *get_data_from_pack(struct object_entry 
> *obj)
> 	src = xmalloc(len);
> 	data = src;
> 	do {
> +		// It appears that if multiple threads read across NFS, the
> +		// second read will fail. I know this is awful, but we wait for
> +		// a little bit and try again.
> 		ssize_t n = pread(pack_fd, data + rdy, len - rdy, from + rdy);
> +		if (n <= 0) {
> +			sleep(1);
> +			n = pread(pack_fd, data + rdy, len - rdy, from + rdy);
> +		}
> 		if (n <= 0)
> 			die("cannot pread pack file: %s", strerror(errno));
> 		rdy += n;
>
> I use a sleep request since it seems less likely that the other thread  
> will have an outstanding request after a second of waiting.
>
> -- 
>                                                         Logan Kennelly
>       ,,,
>      (. .)
> --ooO-(_)-Ooo--
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

> On Thu, Jun 26, 2008 at 04:36:27PM -0700, logank@sent.com wrote:
>> No, it seems to be a simple error-out in this case. We have 2.4.20
>> systems
>
> That version's for the client or the server?

> diff --git a/index-pack.c b/index-pack.c
> index 5ac91ba..85c8bdb 100644
> --- a/index-pack.c
> +++ b/index-pack.c
> @@ -313,7 +313,14 @@ static void *get_data_from_pack(struct  
> object_entry *obj)
> 	src = xmalloc(len);
> 	data = src;
> 	do {
> +		// It appears that if multiple threads read across NFS, the
> +		// second read will fail. I know this is awful, but we wait for
> +		// a little bit and try again.
> 		ssize_t n = pread(pack_fd, data + rdy, len - rdy, from + rdy);
> +		if (n <= 0) {
> +			sleep(1);
> +			n = pread(pack_fd, data + rdy, len - rdy, from + rdy);
> +		}
> 		if (n <= 0)
> 			die("cannot pread pack file: %s", strerror(errno));
> 		rdy += n;