Re: trying to understand READ_META, READ_SYNC, WRITE_SYNC & co

[Christoph Hellwig <hch@lst.de>]

On Mon, Jun 21, 2010 at 12:04:06PM +0200, Jens Axboe wrote:
> It's also annotation for blktrace, so you can tell which parts of the IO
> is meta data etc. The scheduler impact is questionable, I doubt it makes
> a whole lot of difference.

> For analysis purposes, annotating all meta data IOs as such would be
> beneficial (reads as well as writes). As mentioned above, the current
> scheduler impact isn't huge. There may be some interesting test and
> benchmarks there for improving that part.

As mentioned in the previous mail the use of the flag is not very
wide spread. Currently it's only ext3/ext3 inodes and directories as well
as all metadata I/O in gfs2 that gets marked this way.  And I'd be much more
comfortable to add more annotations if it didn't also have some form
of schedule impact.  The I/O schedules in general and cfq in particular
have caused us far too many issues with such subtile differences.

> > 	This one is used in quite a few places, with many of them
> > 	obsfucated by macros like rw_is_sync, rq_is_sync and
> > 	cfq_bio_sync.  In general all uses seem to imply giving
> > 	a write request the same priority as a read request and
> > 	treat it as synchronous.  I could not spot a place where
> > 	it actually has any effect on reads.
> 
> Reads are sync by nature in the block layer, so they don't get that
> special annotation.

Well, we do give them this special annotation in a few places, but we
don't actually use it.

> So a large part of that problem is the overloaded meaning of sync. For
> some cases it means "unplug on issue", and for others it means that the
> IO itself is syncronous. The other nasty bit is the implicit plugging
> that happens behind the back of the submitter, but that's an issue to
> tackle separately. I'd suggest avoiding unnecessary churn in naming of
> those.

Well, the current naming is extremly confusing.  The best thing I could
come up with is to completely drop READ_SYNC and WRITE_SYNC and just
pass REQ_UNPLUG explicitly together with READ / WRITE_SYNC_PLUG.
There's only 5 respective 8 users of them, so explicitly documenting
our intentions there seems useful.  Especially if we want to add more
_META annotation in which case the simple READ_* / WRITE_* macros
don't do anymore either.  Similarly it might be useful to remove
READ_META/WRITE_META and replace them with explicit | REQ_META, which
is just about as short and a lot more descriptive, especially for
synchronous metadata writes.

> > 	Why do O_DIRECT writes not want to set REQ_NOIDLE (and that
> > 	exactly does REQ_NOIDLE mean anyway).  It's the only sync writes
> > 	that do not set it, so if this special case went away we
> > 	could get rid of the flag and key it off REQ_SYNC.
> 
> See above for NOIDLE. You kill O_DIRECT write throughput if you don't
> idle at the end of a write, if you have other activity on the disk.

Ok, makes sense.  Would you mind taking a patch to kill the
WRITE_ODIRECT_PLUG and just do a

	/*
	 * O_DIRECT writes are synchronous, but we must not disable the
	 * idling logic in CFQ to avoid killing performance.
	 */
	if (rw & WRITE)
		rw |= REQ_SYNC;

But that leaves the question why disabling the idling logical for
data integrity ->writepage is fine?  This gets called from ->fsync
or O_SYNC writes and will have the same impact as O_DIRECT writes.