On Mon, May 22, 2017 at 12:05:10PM -0700, Christoph Hellwig wrote: > 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. I thought it may be a little confusing to have multiple sets of flags for a buffer, hence the counter (even though it is logically a flag). But I'm fine with it for now if we don't mind wasting the extra space. Though I suppose we could also add a smaller field and use cmpxchg() to set and clear it... thoughts? Brian -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
