Data Corruption bug with Samba's vfs_iouring and Linux 5.6.7/5.7rc3

[Jeremy Allison]

On Fri, May 08, 2020 at 02:50:55PM -0700, Jeremy Allison wrote:
> On Fri, May 08, 2020 at 11:40:30PM +0200, Stefan Metzmacher wrote:
> > Am 08.05.20 um 22:51 schrieb Jeremy Allison:
> > > On Fri, May 08, 2020 at 01:47:09PM -0700, Jeremy Allison via samba-technical wrote:
> > >> On Fri, May 08, 2020 at 09:35:31PM +0200, Stefan Metzmacher via samba-technical wrote:
> > >>>
> > >>> Thanks very much Jeremy! I didn't noticed that.
> > >>>
> > >>> I guess the attached patch should be able to fix the recursion.
> > >>
> > >> Oh Metze that's *really* ugly :-). I thought about
> > >> doing that in my code and decided it was in too bad
> > >> taste to live :-).
> > >>
> > >> Is there a cleaner way than putting "busy" and "retry"
> > >> variables in the config struct ?
> > > 
> > > And a "Goto again" as well :-(. Bleegh.
> > 
> > This version would actually work and looks a bit similar to
> > your version.
> > 
> > Can you life with that version?
> 
> Yes I can live with that :-). It at least moves the horror
> to the wrapper function where you can at least concentrate
> all your attention as to why it's doing what it's doing :-).
> 
> RB+ from me if you add a comment header above the function
> as well as in the commit so it explains what it's doing.
> 
> Feel free to crib my ascii art to explain in the header
> comment too :-).

There's just one final comment I want to make before
my version goes quietly into that good night... :-).

I will grant that your version leaves a cleaner set of code
paths throughout the io_uring pread/pwrite code and deals with
the short read/write issues in a more "natural" way
without my extra logic branches dealing with "short read"
return etc.

But the fact that you missed the recursion might make
you consider if indeed it's a simpler fix than having
all the extra logic made explicit. Sometimes explicit
is good to draw people's attention to complexity.

Having said that, I'll go with whatever you decide
on this one :-). I already gave me RB+ for the fix,
so I'll support whichever one you pick. But it would
be good to get the fix pushed and back-ported into
4.12 asap to cover the data corruption.

Thanks for working on this with me. I do learn things
from the elegance of your code (usually :-).

Jeremy.