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

[Roger Willcocks]

On Fri, 2014-11-07 at 08:43 +0200, Anton Altaparmakov wrote:
> Hi,
> 
> > On 7 Nov 2014, at 07:52, Anand Avati <avati@xxxxxxxxxxx> wrote:
> > On Thu, Nov 6, 2014 at 8:22 PM, Anton Altaparmakov <aia21@xxxxxxxxx> wrote:
> > > On 7 Nov 2014, at 01:46, Jeff Moyer <jmoyer@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/