On Fri, Feb 16, 2018 at 07:56:25AM -0500, Brian Foster wrote: > On Fri, Feb 16, 2018 at 09:31:38AM +1100, Dave Chinner wrote: > > On Fri, Feb 09, 2018 at 11:12:41AM -0500, Brian Foster wrote: > > > On Thu, Feb 01, 2018 at 05:42:02PM +1100, Dave Chinner wrote: > > > > + bp = xfs_growfs_get_hdr_buf(mp, > > > > + XFS_AG_DADDR(mp, agno, XFS_SB_DADDR), > > > > + XFS_FSS_TO_BB(mp, 1), 0, &xfs_sb_buf_ops); > > > > > > This all seems fine to me up until the point where we use uncached > > > buffers for pre-existing secondary superblocks. This may all be fine now > > > if nothing else happens to access/use secondary supers, but it seems > > > like this essentially enforces that going forward. > > > > > > Hmm, I see that scrub does appear to look at secondary superblocks via > > > cached buffers. Shouldn't we expect this path to maintain coherency with > > > an sb buffer that may have been read/cached from there? > > > > Good catch! I wrote this before scrub started looking at secondary > > superblocks. As a general rulle, we don't want to cache secondary > > superblocks as they should never be used by the kernel except in > > exceptional situations like grow or scrub. > > > > I'll have a look at making this use cached buffers that get freed > > immediately after we release them (i.e. don't go onto the LRU) and > > that should solve the problem. > > > > Ok. Though that sounds a bit odd. What is the purpose of a cached buffer > that is not cached? Serialisation of concurrent access to what is normal a single-use access code path while it is in memory. i.e. exactly the reason we have XFS_IGET_DONTCACHE and use it for things like bulkstat lookups. > Isn't the behavior you're after here (perhaps > analogous to pagecache coherency management between buffered/direct I/O) > more cleanly implemented using a cache invalidation mechanism? E.g., > invalidate cache, use uncached buffer (then perhaps invalidate again). Invalidation as a mechanism for non-coherent access sycnhronisation is completely broken model when it comes to concurrent access. We explicitly tell app developers not ot mix cached + uncached IO to the same file for exactly this reason. Using a cached buffer and using the existing xfs_buf_find/lock serialisation avoids this problem, and by freeing them immediately after we've used them we also minimise the memory footprint of single-use access patterns. > I guess I'm also a little curious why we couldn't continue to use cached > buffers here, As I said, we will continue to use cached buffers here. I'll just call xfs_buf_set_ref(bp, 0) on them so they are reclaimed when released. That means concurrent access will serialise correctly through _xfs_buf_find(), otherwise we won't keep them in memory. > but it doesn't really matter to me that much so long as > the metadata ends up coherent between subsystems.. Yup, that's the idea. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx -- 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
