On Tue, Apr 12, 2016 at 07:49:45PM -0600, Jens Axboe wrote: > On 04/12/2016 06:08 PM, Dave Chinner wrote: > >On Tue, Apr 12, 2016 at 12:43:50PM -0600, Jens Axboe wrote: > >>Hi, > >> > >>Three top level patches out of the writeback throttling patchset, that > >>I think should make it into mainline. No functional changes in the > >>first two patches, and the last patch just bumps reclaim/sync > >>writeback to use WRITE_SYNC as a hint to the block layer. > > > >Whatever happened to adding a new flag to indicate that write > >requests can be throttled, rather than overloading WRITE_SYNC with > >yet another (conflicting) meaning? > > WRITE_SYNC means someone will be waiting for it. This just extends > it to cover more instances where that is the case (reclaim, > for_sync). > > For background writes, we'll want to split WRITE into WRITE and > WRITE_BACKGROUND. > > Alternatively, I can stop extending WRITE_SYNC for the other parts, > and only do the background part. But I think those parts makes > sense. > > >I've already pointed out the problems associated with tagging async, > >bulk writes as synchronous writes so that a lower layer can avoid > > I think that's our miscommunication. It's not my intent to tag async > writes as sync. If we're doing reclaim, presumable someone is > waiting for those pages to be cleaned. Hence sync. Ditto for > for_sync. No, it doesn't mean someone is waiting for them. It just means that reclaim or sync is in progress somewhere and that has optimistically scheduled background writeback to start (i.e. it only has effect if the flusher thread is currently idle) to get some work done before the sync on that superblock starts... And, IIRC, background writeback is aborted when new work is queued on the bdi flusher workqueue (i.e. when the real sync work is scheduled on a superblock), so it really doesn't help us to have background writeback block on locks or behave in any way like the real sync work does, because that will just delay the real timestamp controlled sync work that we need to do. > >throttling them. Please add a new flag for communicating whether > >writes can be throttled to the block layer instead of reusing > >WRITE_SYNC. > > If someone is waiting for them, they won't be throttled as hard > (WRITE_SYNC, WRITE_ODIRECT, etc). If someone is not waiting for them > (WRITE), then we can throttle a bit more. > > That's my intent. We can add WRITE_BACKGROUND, but then we have one > more write type. Why not just leave WRITE as the background type of > write? WRITE does not imply WRITE_BACKGROUND. Think about it. THere's a big difference between a AIO+DIO write, an O_SYNC buffered write and background writeback. The first should be WRITE (async write), the second WRITE_SYNC (because of the wait), and the later WRITE_BACKGROUND (can be throttled). Similarly, we do not want a set of flags that conflate content type (e.g. REQ_META) with behavioural characteristics (e.g. throttle avoidance). As per above, we should be able to specify REQ_META | WRITE, REQ_META | WRITE_SYNC and REQ_META | WRITE_BACKGROUND to indicate the three different behavioural cases of "write quickly", "write quickly, someone waiting" and "background metadata writeback in your own time". And, of course, this makes the code self documenting. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
