From: "J. Bruce Fields" <bfields@fieldses.org>
To: NeilBrown <neilb@suse.com>
Cc: Jeff Layton <jlayton@kernel.org>,
Alexander Viro <viro@zeniv.linux.org.uk>,
Martin Wilck <mwilck@suse.de>,
linux-fsdevel@vger.kernel.org,
Frank Filz <ffilzlnx@mindspring.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 01/12] fs/locks: rename some lists and pointers.
Date: Thu, 8 Nov 2018 22:11:38 -0500 [thread overview]
Message-ID: <20181109031138.GB8831@fieldses.org> (raw)
In-Reply-To: <87muqjw0of.fsf@notabene.neil.brown.name>
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.
next prev parent reply other threads:[~2018-11-09 12:50 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-05 1:30 [PATCH 00/12] Series short description NeilBrown
2018-11-05 1:30 ` [PATCH 02/12] fs/locks: split out __locks_wake_up_blocks() NeilBrown
2018-11-05 1:30 ` [PATCH 08/12] fs/locks: always delete_block after waiting NeilBrown
2018-11-05 1:30 ` [PATCH 06/12] locks: use properly initialized file_lock when unlocking NeilBrown
2018-11-05 1:30 ` [PATCH 01/12] fs/locks: rename some lists and pointers NeilBrown
2018-11-08 20:26 ` J. Bruce Fields
2018-11-09 0:32 ` NeilBrown
2018-11-09 3:11 ` J. Bruce Fields [this message]
2018-11-05 1:30 ` [PATCH 04/12] gfs2: properly initial file_lock used for unlock NeilBrown
2018-11-05 12:18 ` Jeff Layton
2018-11-06 1:48 ` NeilBrown
2018-11-06 13:20 ` Jeff Layton
2018-11-05 1:30 ` [PATCH 05/12] ocfs2: " NeilBrown
2018-11-05 1:30 ` [PATCH 07/12] fs/locks: allow a lock request to block other requests NeilBrown
2018-11-05 1:30 ` [PATCH 09/12] fs/locks: change all *_conflict() functions to return bool NeilBrown
2018-11-05 1:30 ` [PATCH 03/12] NFS: use locks_copy_lock() to copy locks NeilBrown
2018-11-05 1:30 ` [PATCH 11/12] locks: merge posix_unblock_lock() and locks_delete_block() NeilBrown
2018-11-05 1:30 ` [PATCH 10/12] fs/locks: create a tree of dependent requests NeilBrown
2018-11-08 21:30 ` J. Bruce Fields
2018-11-09 0:38 ` NeilBrown
2018-11-09 3:09 ` J. Bruce Fields
2018-11-09 6:24 ` NeilBrown
2018-11-09 15:08 ` J. Bruce Fields
2018-11-05 1:30 ` [PATCH 12/12] VFS: locks: remove unnecessary white space NeilBrown
2018-11-08 21:35 ` [PATCH 00/12] Series short description J. Bruce Fields
2018-11-12 1:14 [PATCH 00/12 v5] locks: avoid thundering-herd wake-ups NeilBrown
2018-11-12 1:14 ` [PATCH 01/12] fs/locks: rename some lists and pointers NeilBrown
2018-11-12 15:06 ` J. Bruce Fields
2018-11-29 23:04 [PATCH 00/12 v6] fs/locks: avoid thundering-herd wake-ups NeilBrown
2018-11-29 23:04 ` [PATCH 01/12] fs/locks: rename some lists and pointers NeilBrown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181109031138.GB8831@fieldses.org \
--to=bfields@fieldses.org \
--cc=ffilzlnx@mindspring.com \
--cc=jlayton@kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mwilck@suse.de \
--cc=neilb@suse.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).