[Ocfs2-devel] [fuse-devel] [PATCH v5 7/7] add a flag for per-operation O_DSYNC semantics

[Milosz Tanski]

On Fri, Nov 7, 2014 at 9:21 AM, Roger Willcocks <roger at 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 at gluster.org> wrote:
>> > On Thu, Nov 6, 2014 at 8:22 PM, Anton Altaparmakov <aia21 at cam.ac.uk> wrote:
>> > > On 7 Nov 2014, at 01:46, Jeff Moyer <jmoyer at 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 at 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 at adfin.com