linux-kernel - Re: [fuse-devel] [PATCH v5 7/7] add a flag for per-operation O

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 7 Nov 2014 14:58:45 -0500
From:	Milosz Tanski <milosz@...in.com>
To:	Roger Willcocks <roger@...mlight.ltd.uk>
Cc:	Anton Altaparmakov <aia21@....ac.uk>,
	Anand Avati <avati@...ster.org>, linux-arch@...r.kernel.org,
	"linux-aio@...ck.org" <linux-aio@...ck.org>,
	linux-nfs@...r.kernel.org,
	Volker Lendecke <Volker.Lendecke@...net.de>,
	"Theodore Ts'o" <tytso@....edu>, Mel Gorman <mgorman@...e.de>,
	"fuse-devel@...ts.sourceforge.net" <fuse-devel@...ts.sourceforge.net>,
	Linux API <linux-api@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Michael Kerrisk <mtk.manpages@...il.com>,
	Christoph Hellwig <hch@...radead.org>, linux-mm@...ck.org,
	Jeff Moyer <jmoyer@...hat.com>,
	Al Viro <viro@...iv.linux.org.uk>, Tejun Heo <tj@...nel.org>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	ceph-devel <ceph-devel@...r.kernel.org>,
	Christoph Hellwig <hch@....de>, ocfs2-devel@....oracle.com
Subject: Re: [fuse-devel] [PATCH v5 7/7] add a flag for per-operation O_DSYNC semantics

On Fri, Nov 7, 2014 at 9:21 AM, Roger Willcocks <roger@...mlight.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@...ster.org> wrote:
>> > On Thu, Nov 6, 2014 at 8:22 PM, Anton Altaparmakov <aia21@....ac.uk> wrote:
>> > > On 7 Nov 2014, at 01:46, Jeff Moyer <jmoyer@...hat.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@...mlight.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@...in.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/