Re: Tar doesn't handle short reads

[Dominique Martinet]

Nikos Tsipinakis wrote on Mon, Jul 27, 2020 at 11:41:30AM +0300:
> 2. The build directory should be in a 9p filesystem (in a VM)
> 3. Kernel version should be >=5.6 (it doesn't error on 4.19)

<9p maintainer hat on>

Probably >5.6 (strict), the patch which broke this is 52cbee2a57[1]
("9p: read only once on O_NONBLOCK") which landed in 5.7 (well I guess
it might have been backported on stable 5.6.x releases)

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=52cbee2a5768d1cf6051286490be43e1712674b3


It's interesting to see an application actually does open regular files
with O_NONBLOCK; when this was discussed in the pull request[2]
basically the agreement was that apps should either just not be setting
nonblock (which ought to be no-op on regular files), or would handle
short reads so it would probably be fine™
(But I guess life always proves us wrong, sorry for the trouble)

[2] 
https://lkml.kernel.org/r/CAHk-=whVEPEsKhU4w9y_sjbg=4yYHKDfgzrpFdy=-f9j+jTO3w@mail.gmail.com

> Condition 3 threw me off and I started looking into the kernel as the source 
> of
> this, however, I now believe it's some change in the 9p driver that simply
> uncovered it.
> 
> $ strace -f dpkg-source -b .
> [...]
> 72606 execve("/home/nikos/tar-1.32/src//tar", ["tar", "-cf", "-", 
> "--format=gnu", "--sort=name", "--mtime", "@1595708407", "--clamp-mtime", 
> "--null", "--numeric-owner", "--owner=0", "--group=0", "--exclude=*.a", 
> "--exclude=*.la", "--exclude=*.o", "--exclude=*.so", "--exclude=.*.sw?", 
> "--exclude=*/*~", "--exclude=,,*", "--exclude=.[#~]*", "--exclude=.arch-ids", 
> "--exclude=.arch-inventory", "--exclude=.be", "--exclude=.bzr", 
> "--exclude=.bzr.backup", "--exclude=.bzr.tags", "--exclude=.bzrignore", 
> "--exclude=.cvsignore", "--exclude=.deps", "--exclude=.git", 
> "--exclude=.gitattributes", "--exclude=.gitignore", ...], 0x55b9e7b2b4d0 /* 
> 22 vars */ <unfinished ...>
> [...]
> 72606 newfstatat(3, "changelog", {st_mode=S_IFREG|0644, st_size=4132, ...}, 
> AT_SYMLINK_NOFOLLOW) = 0
> 72606 openat(3, "changelog", 
> O_RDONLY|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW|O_CLOEXEC) = 4

So another possible "fix" would be to remove the O_NONBLOCK here
(possibly based on some logic e.g. only for regular files if it is
needed for soem other type of files)
However I would advise to just do both if we can; short reads can always
happen due to various bugs even if it's not "features" like what
happened in 9p.


(FWIW, the motivation for the change is that we otherwise have no way to
make synthetic filesystems work nicely with the 9p client on streaming
fake files, so I'd rather not revert this unless we find another way to
e.g. 'cat' a file on 9p filesystem and get data as it appears without
having the kernel do the buffering ; but if you have a solution for this
I'm open to changes)

Thanks,
-- 
Dominique