Re: [Tarantool-patches] [PATCH v3] fio/coio: Handle partial writes

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Fri, Feb 21, 2020 at 01:10:26AM +0300, Alexander Turenko wrote:
> I have three comments and all are up to you.
> 
> Please, rebase the patch on top of the current master (errinj.result was
> changed).

OK, will do.

> On Wed, Dec 04, 2019 at 05:35:11PM +0300, Cyrill Gorcunov wrote:
> > Writting less bytes than requested is fine. In turn our
> > fio.write/pwrite api simply returns 'true' even if only
> > some part of a buffer has been written. Thus make coio_write
> > and coio_pwrite to write the whole data in a cycle.
> 
> I would expect that coio_*() functions will have the same behaviour as
> its libc counterparts except that they'll yield a current fiber instead
> of blocking a current thread.

They do yeild current fiber until completion notification arrives.
coio_wait_done does exactly that.

> 
> So spin in a loop looks more as fio.c work for me, however I would not
> say that I have a strict vision here.
> 
> If you'll decide to keep this logic in coio, at least left comments in
> the header file for those functions.

I'm not sure I follow you here. Our all coio_ helpers do the same thing:
they yield a current fiber and wait for completion.

> > 
> > Fixes #4651
> > 
> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> > ---
> > branch gorcunov/gh-4651-partial-write-3
> 
> > +	struct errinj *inj = errinj(ERRINJ_COIO_WRITE_CHUNK, ERRINJ_INT);
> > <...>
> > +
> > +		if (inj != NULL && inj->iparam > 0)
> > +			chunk = (ssize_t)inj->iparam;
> > +		else
> > +			chunk = left;
> > +
> 
> AFAIR, we have macros for error injections, which allow to avoid
> producing any extra machine code for a release build. It seems they can
> be used here. Can you elaborate, please?

I think I didn't use them to follow current inj code used
in this file, for unification sake. But indeed maybe macors
will look better. Will take a look.

> 
> Nit: We usually split a cast operator from its argument with a
> whitespace, e.g. `(ssize_t) inj->iparam`.

No we don't, there are number of examples where we put cast
right before operand and I think it is a way more correct
because code reader should consider such cast as signle entry
together with operator itself. So I prefer this style.

> >  ssize_t
> > @@ -200,7 +221,12 @@ coio_preadn(int fd, void *buf, size_t count, off_t offset)
> >  static void
> >  coio_do_write(eio_req *req)
> >  {
> > +	struct errinj *inj = errinj(ERRINJ_COIO_WRITE_CHUNK, ERRINJ_INT);
> >  	struct coio_file_task *eio = (struct coio_file_task *)req->data;
> > +
> > +	if (inj != NULL && inj->iparam > 0)
> > +		eio->write.count = (ssize_t)inj->iparam;
> > +
> 
> (If you'll keep the loop in coio.) I would do it in coio_write() itself:
> this way the logic in coio_pwrite() and coio_write() will be quite
> similar and so easier to verify if I don't miss something.

I've no objection here, can move into coio_write.

> 
> Just wonder: is there any reason to use eio_custom() for write? pwrite
> uses eio_write(), which support both write and pwrite (it checks whether
> the offset >= 0.

Sounds reasonable. Letme recheck everything again and resend the patch.