From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; Fri, 21 Feb 2020 15:02:39 +0300 (MSK) Received: by mail-lj1-f195.google.com with SMTP id q8so1895898ljj.11 for ; Fri, 21 Feb 2020 04:02:39 -0800 (PST) Date: Fri, 21 Feb 2020 15:02:34 +0300 From: Cyrill Gorcunov 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tml 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 > > --- > > 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.