From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 7297F469719 for ; Fri, 21 Feb 2020 17:48:38 +0300 (MSK) Date: Fri, 21 Feb 2020 17:48:58 +0300 From: Alexander Turenko Message-ID: <20200221144858.oppuaufu5mw5i3xo@tkn_work_nb> References: <20191204111749.22115-1-gorcunov@gmail.com> <20191204143511.GQ10140@uranus> <20200220221026.oo33zvtavzexbaju@tkn_work_nb> <20200221120234.GF25861@uranus> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200221120234.GF25861@uranus> 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: Cyrill Gorcunov Cc: tml > > 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. I meant handling of a partial write: whether a function should block/yield or report amount of written bytes to a caller. So, my points here are the following: * coio_write() should reflect write() in handling partial writes; * fio lua module should handle partial writes on its own. There is one interesting thing I found: We have src/lib/core/fio.[ch], which handles partial writes, but src/lua/fio.[ch] (Lua module) does not use it. It looks unusual. > > > > > > > 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. You're right. 1. `man 1 indent` shows -ncs (--no-space-after-casts) in the Linux style description. 2. It is also copy-pasted to [1] without -npsl (--dont-break-procedure-type). 3. Vladimir D. uses it in this way in vinyl code. 4. The type cast operation has the same priority as other unary operations. Don't know why I was sure that a space should be here. [1]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure-guidelines---checklist