From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <gorcunov@gmail.com>
Received: from mail-lj1-f195.google.com (mail-lj1-f195.google.com
[209.85.208.195])
(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits))
(No client certificate requested)
by dev.tarantool.org (Postfix) with ESMTPS id 7E9DD469719
for <tarantool-patches@dev.tarantool.org>;
Fri, 21 Feb 2020 15:02:39 +0300 (MSK)
Received: by mail-lj1-f195.google.com with SMTP id q8so1895898ljj.11
for <tarantool-patches@dev.tarantool.org>;
Fri, 21 Feb 2020 04:02:39 -0800 (PST)
Date: Fri, 21 Feb 2020 15:02:34 +0300
From: Cyrill Gorcunov <gorcunov@gmail.com>
Message-ID: <20200221120234.GF25861@uranus>
References: <20191204111749.22115-1-gorcunov@gmail.com>
<20191204143511.GQ10140@uranus>
<20200220221026.oo33zvtavzexbaju@tkn_work_nb>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200220221026.oo33zvtavzexbaju@tkn_work_nb>
Subject: Re: [Tarantool-patches] [PATCH v3] fio/coio: Handle partial writes
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>,
<mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>,
<mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Alexander Turenko <alexander.turenko@tarantool.org>
Cc: tml <tarantool-patches@dev.tarantool.org>
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.