Return-Path: linux-nfs-owner@vger.kernel.org Received: from c.mx.filmlight.ltd.uk ([54.76.112.217]:49438 "EHLO c.mx.filmlight.ltd.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751579AbaKGOaj (ORCPT ); Fri, 7 Nov 2014 09:30:39 -0500 Subject: Re: [fuse-devel] [PATCH v5 7/7] add a flag for per-operation O_DSYNC semantics From: Roger Willcocks To: Anton Altaparmakov Cc: Anand Avati , linux-arch@vger.kernel.org, linux-aio@kvack.org, linux-nfs@vger.kernel.org, Volker Lendecke , "Theodore Ts'o" , Mel Gorman , "fuse-devel@lists.sourceforge.net" , linux-api@vger.kernel.org, Linux Kernel Mailing List , Michael Kerrisk , Christoph Hellwig , linux-mm@kvack.org, Jeff Moyer , Al Viro , Tejun Heo , linux-fsdevel@vger.kernel.org, ceph-devel@vger.kernel.org, Christoph Hellwig , ocfs2-devel@oss.oracle.com, Milosz Tanski In-Reply-To: References: Content-Type: text/plain Date: Fri, 07 Nov 2014 14:21:18 +0000 Message-Id: <1415370078.11083.511.camel@montana.filmlight.ltd.uk> Mime-Version: 1.0 Sender: linux-nfs-owner@vger.kernel.org List-ID: On Fri, 2014-11-07 at 08:43 +0200, Anton Altaparmakov wrote: > Hi, > > > On 7 Nov 2014, at 07:52, Anand Avati wrote: > > On Thu, Nov 6, 2014 at 8:22 PM, Anton Altaparmakov wrote: > > > On 7 Nov 2014, at 01:46, Jeff Moyer wrote: > > > Minor nit, but I'd rather read something that looks like this: > > > > > > if (type == READ && (flags & RWF_NONBLOCK)) > > > return -EAGAIN; > > > else if (type == WRITE && (flags & RWF_DSYNC)) > > > return -EINVAL; > > > > But your version is less logically efficient for the case where "type == READ" is true and "flags & RWF_NONBLOCK" is false because your version then has to do the "if (type == WRITE" check before discovering it does not need to take that branch either, whilst the original version does not have to do such a test at all. > > > > Seriously? > > Of course seriously. > > > Just focus on the code readability/maintainability which makes the code most easily understood/obvious to a new pair of eyes, and leave such micro-optimizations to the compiler.. > > The original version is more readable (IMO) and this is not a micro-optimization. It is people like you who are responsible for the fact that we need faster and faster computers to cope with the inefficient/poor code being written more and more... > Your original version needs me to know that type can only be either READ or WRITE (and not, for instance, READONLY or READWRITE or some other random special case) and it rings alarm bells when I first see it. If you want to keep the micro optimization, you need an assertion to acknowledge the potential bug and a comment to make the code obvious: + assert(type == READ || type == WRITE); + if (type == READ) { + if (flags & RWF_NONBLOCK) + return -EAGAIN; + } else { /* WRITE */ + if (flags & RWF_DSYNC) + return -EINVAL; + } but since what's really happening here is two separate and independent error checks, Jeff's version is still better, even if it does take an extra couple of nanoseconds. Actually I'd probably write: if (type == READ && (flags & RWF_NONBLOCK)) return -EAGAIN; if (type == WRITE && (flags & RWF_DSYNC)) return -EINVAL; (no 'else' since the code will never be reached if the first test is true). -- Roger Willcocks