Re: [PATCH] xfs: use atomic to provide buffer I/O accounting serialization

[Christoph Hellwig <hch@infradead.org>]

On Mon, May 22, 2017 at 02:29:11PM -0400, Brian Foster wrote:
> We've had user reports of unmount hangs in xfs_wait_buftarg() that
> analysis shows is due to btp->bt_io_count == -1. bt_io_count
> represents the count of in-flight asynchronous buffers and thus
> should always be >= 0. xfs_wait_buftarg() waits for this value to
> stabilize to zero in order to ensure that all untracked (with
> respect to the lru) buffers have completed I/O processing before
> unmount proceeds to tear down in-core data structures.
> 
> The value of -1 implies an I/O accounting decrement race. Indeed,
> the fact that xfs_buf_ioacct_dec() is called from xfs_buf_rele()
> (where the buffer lock is no longer held) means that bp->b_flags can
> be updated from an unsafe context. While a user-level reproducer is
> currently not available, some intrusive hacks to run racing buffer
> lookups/ioacct/releases from multiple threads was used to
> successfully manufacture this problem.
> 
> Existing callers do not expect to acquire the buffer lock from
> xfs_buf_rele(). Therefore, we can not safely update ->b_flags from
> this context. To close the race, replace the in-flight buffer flag
> with a per-buffer atomic for tracking accounting against the
> buftarg. This field resides in a hole in the existing data structure
> and thus does not increase the size of xfs_buf.

I hate these uses of atomic_t as binary flags.  Can you use
test_and_set_bit and friends wit a bitop?  This would require
an unsigned long which an actually be larger than an atomic_t,
but it's both cleaner and provides headroom for additional atomic flags
in the future.