Re: [V9fs-developer] tar does not support partial reads

[Christian Schoenebeck]

On Mittwoch, 21. Juli 2021 02:00:20 CEST Dominique Martinet wrote:
> Daniel P. Berrangé wrote on Tue, Jul 20, 2021 at 06:14:53PM +0100:
> > > > When attempting to read a file (other than a pipe or FIFO) that 
supports non-blocking reads and has no data currently available:
> > > >     - If O_NONBLOCK is set, read() shall return -1 and set errno to
> > > >     [EAGAIN].
> > > >     
> > > >     - If O_NONBLOCK is clear, read() shall block the calling thread
> > > >     until some data becomes available.
> > > >     
> > > >     - The use of the O_NONBLOCK flag has no effect if there is some
> > > >     data available.> > > 
> > > > and
> > > > 
> > > > [EAGAIN]
> > > > 
> > > >     The file is neither a pipe, nor a FIFO, nor a socket, the
> > > >     O_NONBLOCK flag is set for the file descriptor, and the thread
> > > >     would be delayed in the read operation.
> Not much time to reply now (will follow up in more details tomorrow),
> but that was (unfortunately?) a voluntary change:
> 
> http://lkml.kernel.org/r/20200205003457.24340-2-l29ah@cock.li
> 
> with the argument that if some program goes out of its way to use
> O_NONBLOCK on open it can also handle short reads (which by the way can
> also happen without O_NONBLOCK as bugs on other filesystems, so in my
> opinion is something that should be handled regardless of what we do
> here -- I've seen enough data being eaten by programs that take
> shortcuts on IO as soon as something goes wrong instead of erroring or
> taking care of these)
> 
> Unfortunately GNU tar doesn't, and Nikos Tsipinakis (added to Ccs) sent
> a patch to bug-tar@ in ... september last year which looked good enough
> to me but didn't seem to be taken? I didn't check.

For reference, Nikos' full_read() patch for tar from Sept 2020:
https://lists.gnu.org/archive/html/bug-tar/2020-09/msg00001.html

> I agree this isn't posix, but it was discussed as a quirk that seemed
> better than yet another weird mount option that e.g. NFS has for special
> netfs behaviour.
> 
> The argument was for synthetic fs having a file that would stream things
> and implementing tail -f like behaviour on it, problem being that if
> they would make the fake-file a pipe it would stay local to the linux
> client and not go through the 9p server, so it was deemed difficult to
> emulate the behaviour.
> If you have a practical way of doing this then I'll be happy to revert
> Sergey's commit (also added to cc), as I can agree sticking to posix
> when possible is better.

Allowing partial reads is useful, sometimes even necessary. That POSIX 
mandates this to be an exclusive feature for non-regular files (i.e. being 
explicitly marked as such) no longer fits into our time.

Forcing either one behaviour system wide for all apps will still break things, 
no matter if hard coded or as a mount option.

I would prefer a more general solution allowing individual apps to decide 
whether they want to allow short reads or not, e.g. by adding a public flag 
like O_PARTIAL to fcntl.h on Linux side.

> > > I was thinking to something like that (not tested yet):
> > > 
> > > --- a/fs/9p/vfs_file.c
> > > +++ b/fs/9p/vfs_file.c
> > > @@ -389,8 +389,22 @@ v9fs_file_read_iter(struct kiocb *iocb, struct
> > > iov_iter *t>> > 
> > >         p9_debug(P9_DEBUG_VFS, "count %zu offset %lld\n",
> > >         
> > >                  iov_iter_count(to), iocb->ki_pos);
> > > 
> > > -       if (iocb->ki_filp->f_flags & O_NONBLOCK)
> > > +       if (iocb->ki_filp->f_flags & O_NONBLOCK) {
> > > +               size_t count = iov_iter_count(to);
> > > +
> > > 
> > >                 ret = p9_client_read_once(fid, iocb->ki_pos, to, &err);
> > > 
> > > +               if (!ret)
> > > +                       return err;
> > > +
> > > +               /*
> > > +                * POSIX requires to ignore O_NONBLOCK if some data is
> > > +                * already available.
> > > +                */
> > > +               if (ret != count) {
> > > +                       iocb->ki_pos += ret;
> > > +                       ret = p9_client_read(fid, iocb->ki_pos, to,
> > > &err);
> > > +               }
> 
> That seems overly complicated, just use p9_client_read as per the else
> (e.g. remove the condition) if that's what you want.

Best regards,
Christian Schoenebeck