lkml.org 
[lkml]   [2014]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [fuse-devel] [PATCH v5 7/7] add a flag for per-operation O_DSYNC semantics
From
On Fri, Nov 7, 2014 at 9:21 AM, Roger Willcocks <roger@filmlight.ltd.uk> wrote:
>
> On Fri, 2014-11-07 at 08:43 +0200, Anton Altaparmakov wrote:
>> Hi,
>>
>> > On 7 Nov 2014, at 07:52, Anand Avati <avati@gluster.org> wrote:
>> > On Thu, Nov 6, 2014 at 8:22 PM, Anton Altaparmakov <aia21@cam.ac.uk> wrote:
>> > > On 7 Nov 2014, at 01:46, Jeff Moyer <jmoyer@redhat.com> 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 <roger@filmlight.ltd.uk>
>

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


\
 
 \ /
  Last update: 2014-11-07 21:41    [W:0.069 / U:0.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site