Re: [PATCH 0/3] writeback: minor tweaks

[Dave Chinner <david@fromorbit.com>]

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@fromorbit.com