Re: [PATCH v2] iomap: report collisions between directio and buffered writes to userspace

[Dave Chinner <david@fromorbit.com>]

On Tue, Nov 21, 2017 at 04:52:53AM -0800, Matthew Wilcox wrote:
> On Tue, Nov 21, 2017 at 05:48:15PM +1100, Dave Chinner wrote:
> > On Mon, Nov 20, 2017 at 08:32:40PM -0800, Matthew Wilcox wrote:
> > > On Mon, Nov 20, 2017 at 05:37:53PM -0800, Darrick J. Wong wrote:
> > > > On Tue, Nov 21, 2017 at 09:27:49AM +1100, Dave Chinner wrote:
> > > > > First thing I noticed was that "xa" as a prefix is already quite
> > > > > widely used in XFS - it's shorthand for "XFS AIL". Indeed, xa_lock
> > >
> > > The X stands for 'eXpandable' or 'eXtending'.  I really don't want to
> > > use more than a two-letter acronym for whatever the XArray ends up being
> > > called.  One of the problems with the radix tree is that everything has
> > > that 11-character 'radix_tree_' prefix; just replacing that with 'xa_'
> > > makes a huge improvement to readability.
> > 
> > Yeah, understood. That's why
> > we have very little clear
> > prefix namespace left.... :/
> > 
> > [ somedays I write something that looks sorta like a haiku, and from
> > that point on everything just starts falling out of my brain that
> > way. I blame Eric for this today! :P ]
> 
> When the namespace is
> tight we must consider the
> competing users.
> 
> The earliest us'r
> has a claim to a prefix
> we are used to it.
> 
> Also a more wide-
> spread user has a claim to
> a shorter prefix.
> 
> Would you mind changing
> your prefix to one only
> one letter longer?

We can do something
like that, though Darrick has the
final say in it.

> ... ok, I give up ;-)

Top marks for effort :)

> All your current usage of the xa_ prefix looks somewhat like this:
> 
> fs/xfs/xfs_trans_ail.c: spin_lock(&ailp->xa_lock);
> 
> with honourable mentions to:
> fs/xfs/xfs_log.c:	spin_lock(&mp->m_ail->xa_lock);
> 
> Would you mind if I bolt a patch on to the front of the series called
> something like "free up xa_ namespace" that renamed your xa_* to ail_*?
> There are no uses of the 'ail_' prefix in the kernel today.
> 
> I don't think that
> 	spin_lock(&ailp->ail_lock);
> loses any readability.

Not sure that's going to work - there's an "xa_ail" member for the
AIL list head. That would now read in places:

	if (list_empty(&ailp->ail_ail))

I'd be inclined to just drop the "xa_" prefix from the XFS
structure.  There is no information loss by removing the prefix in
the XFS code because the pointer name tells us what structure it is
pointing at.

> 
> By the way, what does AIL stand for?  It'd be nice if it were spelled out
> in at least one of the header files, maybe fs/xfs/xfs_trans_priv.h?

Active Item List. See the section "Tracking Changes" in
Documentation/filesystems/xfs-delayed-logging-design.txt for the
full rundown, but in short:

"The log item is already used to track the log items that have been
written to the log but not yet written to disk. Such log items are
considered "active" and as such are stored in the Active Item List
(AIL) which is a LSN-ordered double linked list. Items are inserted
into this list during log buffer IO completion, after which they are
unpinned and can be written to disk."

The lack of comments describing the AIL is historic - it's never
been documented in the code, nor has the whole relogging concept it
implements. I wrote the document above when introducing the CIL
(Commited Item List) because almost no-one actively working on XFS
understood how the whole journalling subsystem worked in any detail.

> > Zoetrope Array.
> > Labyrinth of illusion.
> > Structure never ends.
> 
> Thank you for making me look up zoetrope ;-)

My pleasure :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com