First three bits are identical in rq->cmd_flags and bio->bi_rw?

First three bits are identical in rq->cmd_flags and bio->bi_rw?

[David Woodhouse]

According to the comment in blk_rq_io_prep(), about line 2030 of
blk-core.c:

	/* first two bits are identical in rq->cmd_flags and bio->bi_rw */
	rq->cmd_flags |= (bio->bi_rw & 3);

Is this true? These are the bits in bi_rw:

#define BIO_RW		0
#define BIO_RW_AHEAD	1
#define BIO_RW_BARRIER	2
#define BIO_RW_FAILFAST	3
#define BIO_RW_SYNC	4
#define BIO_RW_META	5

And these are the (first) bits in rq->cmd_flags:
	__REQ_RW,		/* not set, read. set, write */
	__REQ_FAILFAST,		/* no low level driver retries */
	__REQ_SORTED,		/* elevator knows about this request */
	__REQ_SOFTBARRIER,	/* may not be passed by ioscheduler */
	__REQ_HARDBARRIER,	/* may not be passed by drive either */
	__REQ_FUA,		/* forced unit access */

Are we intending that BIO_RW_AHEAD maps to REQ_FAILFAST? When I started
composing this mail I was just going to call it a lie -- but now I think
about it, it makes a certain amount of sense. The comment really ought
to be a little more explicit though, if that's the case. That's not
_identical_, even if it's intentional.

In blk_alloc_request() we have an even more interesting claim -- this
time it's _three_ bits which are identical:

	/*
	 * first three bits are identical in rq->cmd_flags and bio->bi_rw,
	 * see bio.h and blkdev.h
	 */
	rq->cmd_flags = rw | REQ_ALLOCED;

AFAICT the only thing passed in via the 'rw' variable is the R/W bit and
maybe REQ_RW_SYNC when it's called from get_request(), so the comment
doesn't matter and can just be removed, I think?

-- 
dwmw2

Re: First three bits are identical in rq->cmd_flags and bio->bi_rw?

[Jens Axboe]

On Tue, Aug 05 2008, David Woodhouse wrote:
> According to the comment in blk_rq_io_prep(), about line 2030 of
> blk-core.c:
> 
> 	/* first two bits are identical in rq->cmd_flags and bio->bi_rw */
> 	rq->cmd_flags |= (bio->bi_rw & 3);
> 
> Is this true? These are the bits in bi_rw:
> 
> #define BIO_RW		0
> #define BIO_RW_AHEAD	1
> #define BIO_RW_BARRIER	2
> #define BIO_RW_FAILFAST	3
> #define BIO_RW_SYNC	4
> #define BIO_RW_META	5
> 
> And these are the (first) bits in rq->cmd_flags:
> 	__REQ_RW,		/* not set, read. set, write */
> 	__REQ_FAILFAST,		/* no low level driver retries */
> 	__REQ_SORTED,		/* elevator knows about this request */
> 	__REQ_SOFTBARRIER,	/* may not be passed by ioscheduler */
> 	__REQ_HARDBARRIER,	/* may not be passed by drive either */
> 	__REQ_FUA,		/* forced unit access */
> 
> Are we intending that BIO_RW_AHEAD maps to REQ_FAILFAST? When I started
> composing this mail I was just going to call it a lie -- but now I think
> about it, it makes a certain amount of sense. The comment really ought
> to be a little more explicit though, if that's the case. That's not
> _identical_, even if it's intentional.
> 
> In blk_alloc_request() we have an even more interesting claim -- this
> time it's _three_ bits which are identical:
> 
> 	/*
> 	 * first three bits are identical in rq->cmd_flags and bio->bi_rw,
> 	 * see bio.h and blkdev.h
> 	 */
> 	rq->cmd_flags = rw | REQ_ALLOCED;
> 
> AFAICT the only thing passed in via the 'rw' variable is the R/W bit and
> maybe REQ_RW_SYNC when it's called from get_request(), so the comment
> doesn't matter and can just be removed, I think?

That last comment is stale, the first two bits is the correct one (it
used to be all three, but not anymore). RA does indeed just map to
failfast at the request end, since we don't a lot of error handling on
those. But yeah, the comment could do with an explicit note to that
effect :-)

-- 
Jens Axboe