RFC bio flags and ops in v4.10+

RFC bio flags and ops in v4.10+

[Ashlie Martinez]

Hello,

I was working on porting parts of a file system crash consistency
checking tool called CrashMonkey [1] to linux kernels 4.9 and 4.14
when I noticed an inconsistency in how the bio->bi_opf field is
treated. According to the comments in /include/linux/blk_types.h, the
REQ_OP should be the upper bits in bi_opf, while the request flags
should be in the lower bits. In kernel 4.9, the accessor and setter
methods for this field appear to be correct. However, kernels 4.10+ do
not properly shift the REQ_OP argument up for either setting it or
getting it from bi_opf. Is this the intended behavior or is it a
mistake in how the code was written? Please note that I have not
checked all the releases between 4.9 and 5.0-rc3, but I know 4.10,
4.14, 4.15, and 5.0-rc3 contain the same or similar code for setting
bio->bi_opf.

[1] https://github.com/utsaslab/crashmonkey

Re: RFC bio flags and ops in v4.10+

[Bart Van Assche]

On 1/21/19 8:45 PM, Ashlie Martinez wrote:
> I was working on porting parts of a file system crash consistency
> checking tool called CrashMonkey [1] to linux kernels 4.9 and 4.14
> when I noticed an inconsistency in how the bio->bi_opf field is
> treated. According to the comments in /include/linux/blk_types.h, the
> REQ_OP should be the upper bits in bi_opf, while the request flags
> should be in the lower bits. In kernel 4.9, the accessor and setter
> methods for this field appear to be correct. However, kernels 4.10+ do
> not properly shift the REQ_OP argument up for either setting it or
> getting it from bi_opf. Is this the intended behavior or is it a
> mistake in how the code was written? Please note that I have not
> checked all the releases between 4.9 and 5.0-rc3, but I know 4.10,
> 4.14, 4.15, and 5.0-rc3 contain the same or similar code for setting
> bio->bi_opf.

Hi Ashlie,

Are you perhaps referring to commit ef295ecf090d ("block: better op and 
flags encoding")? Have you noticed that that commit modified the 
encoding such that the operation ends up in the lower 8 bits and the 
flags in the upper 24 bits? That patch namely includes the following change:

+enum req_flag_bits {
+	__REQ_FAILFAST_DEV =	/* no driver retries of device errors */
+		REQ_OP_BITS,

Bart.

Re: RFC bio flags and ops in v4.10+

[Ashlie Martinez]

On Mon, Jan 21, 2019 at 9:04 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 1/21/19 8:45 PM, Ashlie Martinez wrote:
> > I was working on porting parts of a file system crash consistency
> > checking tool called CrashMonkey [1] to linux kernels 4.9 and 4.14
> > when I noticed an inconsistency in how the bio->bi_opf field is
> > treated. According to the comments in /include/linux/blk_types.h, the
> > REQ_OP should be the upper bits in bi_opf, while the request flags
> > should be in the lower bits. In kernel 4.9, the accessor and setter
> > methods for this field appear to be correct. However, kernels 4.10+ do
> > not properly shift the REQ_OP argument up for either setting it or
> > getting it from bi_opf. Is this the intended behavior or is it a
> > mistake in how the code was written? Please note that I have not
> > checked all the releases between 4.9 and 5.0-rc3, but I know 4.10,
> > 4.14, 4.15, and 5.0-rc3 contain the same or similar code for setting
> > bio->bi_opf.
>
> Hi Ashlie,
>
> Are you perhaps referring to commit ef295ecf090d ("block: better op and
> flags encoding")? Have you noticed that that commit modified the
> encoding such that the operation ends up in the lower 8 bits and the
> flags in the upper 24 bits? That patch namely includes the following change:
>
> +enum req_flag_bits {
> +       __REQ_FAILFAST_DEV =    /* no driver retries of device errors */
> +               REQ_OP_BITS,
>

Ah, I missed that assignment hiding on the next line (possibly because
I've been staring at the code for various projects for too long :) ),
thanks for pointing it out for me! One (minor) nit about the current
code then: it appears that the comment on bio->bi_opf  still refers to
the old arrangement where the REQ_OP occupied the upper bits of the
field, making it hard to figure out what's happening at a glance

> Bart.

Re: RFC bio flags and ops in v4.10+

[Bart Van Assche]

On Mon, 2019-01-21 at 21:12 -0800, Ashlie Martinez wrote:
> On Mon, Jan 21, 2019 at 9:04 PM Bart Van Assche <bvanassche@acm.org> wrote:
> > 
> > On 1/21/19 8:45 PM, Ashlie Martinez wrote:
> > > I was working on porting parts of a file system crash consistency
> > > checking tool called CrashMonkey [1] to linux kernels 4.9 and 4.14
> > > when I noticed an inconsistency in how the bio->bi_opf field is
> > > treated. According to the comments in /include/linux/blk_types.h, the
> > > REQ_OP should be the upper bits in bi_opf, while the request flags
> > > should be in the lower bits. In kernel 4.9, the accessor and setter
> > > methods for this field appear to be correct. However, kernels 4.10+ do
> > > not properly shift the REQ_OP argument up for either setting it or
> > > getting it from bi_opf. Is this the intended behavior or is it a
> > > mistake in how the code was written? Please note that I have not
> > > checked all the releases between 4.9 and 5.0-rc3, but I know 4.10,
> > > 4.14, 4.15, and 5.0-rc3 contain the same or similar code for setting
> > > bio->bi_opf.
> > 
> > Hi Ashlie,
> > 
> > Are you perhaps referring to commit ef295ecf090d ("block: better op and
> > flags encoding")? Have you noticed that that commit modified the
> > encoding such that the operation ends up in the lower 8 bits and the
> > flags in the upper 24 bits? That patch namely includes the following change:
> > 
> > +enum req_flag_bits {
> > +       __REQ_FAILFAST_DEV =    /* no driver retries of device errors */
> > +               REQ_OP_BITS,
> > 
> 
> Ah, I missed that assignment hiding on the next line (possibly because
> I've been staring at the code for various projects for too long :) ),
> thanks for pointing it out for me! One (minor) nit about the current
> code then: it appears that the comment on bio->bi_opf  still refers to
> the old arrangement where the REQ_OP occupied the upper bits of the
> field, making it hard to figure out what's happening at a glance

If you would like to submit a patch that updates the header file comments,
I think it would be welcome :-)

Bart.