linux-kernel - Re: [PATCH 01/12] fs/locks: rename some lists and pointers.

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181109031138.GB8831@fieldses.org>
Date:   Thu, 8 Nov 2018 22:11:38 -0500
From:   "J. Bruce Fields" <bfields@...ldses.org>
To:     NeilBrown <neilb@...e.com>
Cc:     Jeff Layton <jlayton@...nel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Martin Wilck <mwilck@...e.de>, linux-fsdevel@...r.kernel.org,
        Frank Filz <ffilzlnx@...dspring.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/12] fs/locks: rename some lists and pointers.

On Fri, Nov 09, 2018 at 11:32:16AM +1100, NeilBrown wrote:
> On Thu, Nov 08 2018, J. Bruce Fields wrote:
> 
> > On Mon, Nov 05, 2018 at 12:30:47PM +1100, NeilBrown wrote:
> >> struct file lock contains an 'fl_next' pointer which
> >> is used to point to the lock that this request is blocked
> >> waiting for.  So rename it to fl_blocker.
> >> 
> >> The fl_blocked list_head in an active lock is the head of a list of
> >> blocked requests.  In a request it is a node in that list.
> >> These are two distinct uses, so replace with two list_heads
> >> with different names.
> >> fl_blocked is the head of a list of blocked requests
> >> fl_block is a node on that list.
> >
> > Reading these, I have a lot of trouble keeping fl_blocked, fl_block, and
> > fl_blocker straight.  Is it just me?
> 
> "Naming is hard" - but that is no excuse.
> I suspect it isn't just you.
> 
> I particularly like "fl_blocker".
> 
> 		error = wait_event_interruptible(fl->fl_wait, !fl->fl_blocker);
> 
> reads well to me - wait until this lock has a no blocker - i.e. until
> nothing blocks it.
> 
> fl_blocked could be fl_blockees (the things that I block), but I doubt
> that is an improvement.

Maybe.  The plural might help me remember that it's the head of a list?

> > I guess they form a tree, so fl_children, fl_siblings, and fl_parent
> > might be easier for me to keep straight.
> 
> This requires one to know a priori that the tree records which locks
> block which requests, which is obvious to us now, but might not be so
> obvious in 5 years time when we look at this code again.
> 
> An I have never really liked the "siblings" naming. 'struct dentry' uses
> "d_child", which is possibly ever more confusing.
> I would like it to be obvious that this is a list-member, not a
> list-head.  Rusty once posted patches to allow the list head to be a
> different type to the members, but that fell on deaf ears.
> So
>    fl_blocked_member
> might be an improvement - this is a member of the fl_blocked list.
> It would be easier to search for than fl_block - which needs
> fl_block[^a-z] to avoid false positives.

Yeah, maybe, if it's not too long.

> I'd be quite happy to change fl_block is any two people can agree on a
> better name.  I'm less inclined to change the others without a really
> good proposal.
> 
> Hmmm. what is the inverse of "Block"?  If I block you then you ....  I
> know, you are a usurper.
> So
>   fl_blocker points to the "parent"
>   fl_usurpers is a list of "children"
>   fl_usurpers_member is my linkage in that list.
> or not.

"Usurper" isn't doing it for me.

Yeah, I've got no clever scheme.

--b.