Return-Path: linux-nfs-owner@vger.kernel.org Received: from mail-la0-f44.google.com ([209.85.215.44]:56089 "EHLO mail-la0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752759AbaKGT6r convert rfc822-to-8bit (ORCPT ); Fri, 7 Nov 2014 14:58:47 -0500 Received: by mail-la0-f44.google.com with SMTP id gf13so5093554lab.3 for ; Fri, 07 Nov 2014 11:58:46 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1415370078.11083.511.camel@montana.filmlight.ltd.uk> References: <1415370078.11083.511.camel@montana.filmlight.ltd.uk> Date: Fri, 7 Nov 2014 14:58:45 -0500 Message-ID: Subject: Re: [fuse-devel] [PATCH v5 7/7] add a flag for per-operation O_DSYNC semantics From: Milosz Tanski To: Roger Willcocks Cc: Anton Altaparmakov , 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 , 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 , Christoph Hellwig , ocfs2-devel@oss.oracle.com Content-Type: text/plain; charset=UTF-8 Sender: linux-nfs-owner@vger.kernel.org List-ID: On Fri, Nov 7, 2014 at 9:21 AM, Roger Willcocks wrote: > > 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 > This is what I changed it to (and will be sending that out for the next version). -- Milosz Tanski CTO 16 East 34th Street, 15th floor New York, NY 10016 p: 646-253-9055 e: milosz@adfin.com