All of lore.kernel.org
 help / color / mirror / Atom feed
* dm-flakey NOT to return -EIO on READ?
@ 2016-07-06  6:33 Akira Hayakawa
  2016-07-06  9:52 ` Lukas Herbolt
  0 siblings, 1 reply; 12+ messages in thread
From: Akira Hayakawa @ 2016-07-06  6:33 UTC (permalink / raw)
  To: device-mapper development

Hi,

I am using dm-flakey to emulate a broken device that should return -EIO on both read and write.
I use the parameter up_interval=0 and down_interval=1.

But when I am dd-ing the flakey device, while write fails, read succeeds.
This isn't the behavior I expect.

Then I looked into the code.

326 static int flakey_end_io(struct dm_target *ti, struct bio *bio, int error)
327 {
328         struct flakey_c *fc = ti->private;
329         struct per_bio_data *pb = dm_per_bio_data(bio, sizeof(struct per_bio_data));
330 
331         /*
332          * Corrupt successful READs while in down state.
333          * If flags were specified, only corrupt those that match.
334          */
335         if (fc->corrupt_bio_byte && !error && pb->bio_submitted &&
336             (bio_data_dir(bio) == READ) && (fc->corrupt_bio_rw == READ) &&
337             all_corrupt_bio_flags_match(bio, fc))
338                 corrupt_bio_data(bio, fc);
339 
340         return error;
341 }

When the bio direction is READ and currupt_bio_bytes isn't specified
the READ bio is handled normally right?

I think READ requests should return -EIO if

1. corrupt_bio_bytes isn't specified
2. but the requested is handled during down interval.

as well as WRITE requests:

305                 /*
306                  * Corrupt matching writes.
307                  */
308                 if (fc->corrupt_bio_byte && (fc->corrupt_bio_rw == WRITE)) {
309                         if (all_corrupt_bio_flags_match(bio, fc))
310                                 corrupt_bio_data(bio, fc);
311                         goto map_bio;
312                 }
313 
314                 /*
315                  * By default, error all I/O.
316                  */
317                 return -EIO;
318         }

This code is similar to what we see in the end_io.

If my understanding is correct, I would like to modify dm-flakey to return -EIO in the end_io.

I would like to request for comments.

Thanks,

- Akira

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: dm-flakey NOT to return -EIO on READ?
  2016-07-06  6:33 dm-flakey NOT to return -EIO on READ? Akira Hayakawa
@ 2016-07-06  9:52 ` Lukas Herbolt
  2016-07-06 12:54   ` Akira Hayakawa
  0 siblings, 1 reply; 12+ messages in thread
From: Lukas Herbolt @ 2016-07-06  9:52 UTC (permalink / raw)
  To: Akira Hayakawa; +Cc: device-mapper development


[-- Attachment #1.1: Type: text/plain, Size: 2523 bytes --]

Hi,
Yes this part is wrong and reads are not dropped.
I have a patch ready, just have to send it out.

Lukas

On Wed, Jul 6, 2016 at 8:33 AM, Akira Hayakawa <ruby.wktk@gmail.com> wrote:

> Hi,
>
> I am using dm-flakey to emulate a broken device that should return -EIO on
> both read and write.
> I use the parameter up_interval=0 and down_interval=1.
>
> But when I am dd-ing the flakey device, while write fails, read succeeds.
> This isn't the behavior I expect.
>
> Then I looked into the code.
>
> 326 static int flakey_end_io(struct dm_target *ti, struct bio *bio, int
> error)
> 327 {
> 328         struct flakey_c *fc = ti->private;
> 329         struct per_bio_data *pb = dm_per_bio_data(bio, sizeof(struct
> per_bio_data));
> 330
> 331         /*
> 332          * Corrupt successful READs while in down state.
> 333          * If flags were specified, only corrupt those that match.
> 334          */
> 335         if (fc->corrupt_bio_byte && !error && pb->bio_submitted &&
> 336             (bio_data_dir(bio) == READ) && (fc->corrupt_bio_rw ==
> READ) &&
> 337             all_corrupt_bio_flags_match(bio, fc))
> 338                 corrupt_bio_data(bio, fc);
> 339
> 340         return error;
> 341 }
>
> When the bio direction is READ and currupt_bio_bytes isn't specified
> the READ bio is handled normally right?
>
> I think READ requests should return -EIO if
>
> 1. corrupt_bio_bytes isn't specified
> 2. but the requested is handled during down interval.
>
> as well as WRITE requests:
>
> 305                 /*
> 306                  * Corrupt matching writes.
> 307                  */
> 308                 if (fc->corrupt_bio_byte && (fc->corrupt_bio_rw ==
> WRITE)) {
> 309                         if (all_corrupt_bio_flags_match(bio, fc))
> 310                                 corrupt_bio_data(bio, fc);
> 311                         goto map_bio;
> 312                 }
> 313
> 314                 /*
> 315                  * By default, error all I/O.
> 316                  */
> 317                 return -EIO;
> 318         }
>
> This code is similar to what we see in the end_io.
>
> If my understanding is correct, I would like to modify dm-flakey to return
> -EIO in the end_io.
>
> I would like to request for comments.
>
> Thanks,
>
> - Akira
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>



-- 
Lukas Herbolt
RHCE, RH436, BSc, SSc
Senior Technical Support Engineer
Global Support Services (GSS)
Email:    lherbolt@redhat.com

[-- Attachment #1.2: Type: text/html, Size: 4325 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: dm-flakey NOT to return -EIO on READ?
  2016-07-06  9:52 ` Lukas Herbolt
@ 2016-07-06 12:54   ` Akira Hayakawa
  2016-07-17  1:36     ` Akira Hayakawa
  0 siblings, 1 reply; 12+ messages in thread
From: Akira Hayakawa @ 2016-07-06 12:54 UTC (permalink / raw)
  To: Lukas Herbolt; +Cc: device-mapper development

Thanks,

I want the patch in the main-tree quickly.
Because without it, my tests will not be green.
This is really annoying.

I made a quick reproducer of this problem.
If you are looking for one, this will help.
(You need to install sbt first)

https://github.com/akiradeveloper/writeboost-test-suite

  test("flakey (read)") {
    Memory(Sector.M(16)) { a =>
      Flakey.Table(a, 0, 1).create { b =>
        intercept[Exception] {
          Shell(s"dd status=none if=${b.bdev.path} iflag=direct of=/dev/null")
        }
      }
    }
  }

$ sudo sbt "testOnly dmtest.FlakeyTest"
12:44:00.643 [pool-2-thread-3-ScalaTest-running-FlakeyTest] INFO dmtest - [TEST] flakey (read)
12:44:00.752 [pool-2-thread-3-ScalaTest-running-FlakeyTest] DEBUG dmtest - reload: table=0 32768 flakey /dev/loop0 0 0 1
[info] FlakeyTest:
[info] - flakey (read) *** FAILED ***
[info]   Expected exception java.lang.Exception to be thrown, but no exception was thrown. (FlakeyTest.scala:9)
[info] Run completed in 2 seconds, 781 milliseconds.
[info] Total number of tests run: 1
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 0, failed 1, canceled 0, ignored 0, pending 0

- Akira

On 2016/07/06 18:52, Lukas Herbolt wrote:
> Hi,
> Yes this part is wrong and reads are not dropped.
> I have a patch ready, just have to send it out.
> 
> Lukas
> 
> On Wed, Jul 6, 2016 at 8:33 AM, Akira Hayakawa <ruby.wktk@gmail.com> wrote:
> 
>> Hi,
>>
>> I am using dm-flakey to emulate a broken device that should return -EIO on
>> both read and write.
>> I use the parameter up_interval=0 and down_interval=1.
>>
>> But when I am dd-ing the flakey device, while write fails, read succeeds.
>> This isn't the behavior I expect.
>>
>> Then I looked into the code.
>>
>> 326 static int flakey_end_io(struct dm_target *ti, struct bio *bio, int
>> error)
>> 327 {
>> 328         struct flakey_c *fc = ti->private;
>> 329         struct per_bio_data *pb = dm_per_bio_data(bio, sizeof(struct
>> per_bio_data));
>> 330
>> 331         /*
>> 332          * Corrupt successful READs while in down state.
>> 333          * If flags were specified, only corrupt those that match.
>> 334          */
>> 335         if (fc->corrupt_bio_byte && !error && pb->bio_submitted &&
>> 336             (bio_data_dir(bio) == READ) && (fc->corrupt_bio_rw ==
>> READ) &&
>> 337             all_corrupt_bio_flags_match(bio, fc))
>> 338                 corrupt_bio_data(bio, fc);
>> 339
>> 340         return error;
>> 341 }
>>
>> When the bio direction is READ and currupt_bio_bytes isn't specified
>> the READ bio is handled normally right?
>>
>> I think READ requests should return -EIO if
>>
>> 1. corrupt_bio_bytes isn't specified
>> 2. but the requested is handled during down interval.
>>
>> as well as WRITE requests:
>>
>> 305                 /*
>> 306                  * Corrupt matching writes.
>> 307                  */
>> 308                 if (fc->corrupt_bio_byte && (fc->corrupt_bio_rw ==
>> WRITE)) {
>> 309                         if (all_corrupt_bio_flags_match(bio, fc))
>> 310                                 corrupt_bio_data(bio, fc);
>> 311                         goto map_bio;
>> 312                 }
>> 313
>> 314                 /*
>> 315                  * By default, error all I/O.
>> 316                  */
>> 317                 return -EIO;
>> 318         }
>>
>> This code is similar to what we see in the end_io.
>>
>> If my understanding is correct, I would like to modify dm-flakey to return
>> -EIO in the end_io.
>>
>> I would like to request for comments.
>>
>> Thanks,
>>
>> - Akira
>>
>> --
>> dm-devel mailing list
>> dm-devel@redhat.com
>> https://www.redhat.com/mailman/listinfo/dm-devel
>>
> 
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: dm-flakey NOT to return -EIO on READ?
  2016-07-06 12:54   ` Akira Hayakawa
@ 2016-07-17  1:36     ` Akira Hayakawa
  2016-07-29  6:25       ` Lukas Herbolt
  0 siblings, 1 reply; 12+ messages in thread
From: Akira Hayakawa @ 2016-07-17  1:36 UTC (permalink / raw)
  To: Lukas Herbolt; +Cc: device-mapper development

> On 2016/07/06 18:52, Lukas Herbolt wrote:
>> Hi,
>> Yes this part is wrong and reads are not dropped.
>> I have a patch ready, just have to send it out.

btw, is this fix going to be included in the next release?

On 2016/07/06 21:54, Akira Hayakawa wrote:
> Thanks,
> 
> I want the patch in the main-tree quickly.
> Because without it, my tests will not be green.
> This is really annoying.
> 
> I made a quick reproducer of this problem.
> If you are looking for one, this will help.
> (You need to install sbt first)
> 
> https://github.com/akiradeveloper/writeboost-test-suite
> 
>   test("flakey (read)") {
>     Memory(Sector.M(16)) { a =>
>       Flakey.Table(a, 0, 1).create { b =>
>         intercept[Exception] {
>           Shell(s"dd status=none if=${b.bdev.path} iflag=direct of=/dev/null")
>         }
>       }
>     }
>   }
> 
> $ sudo sbt "testOnly dmtest.FlakeyTest"
> 12:44:00.643 [pool-2-thread-3-ScalaTest-running-FlakeyTest] INFO dmtest - [TEST] flakey (read)
> 12:44:00.752 [pool-2-thread-3-ScalaTest-running-FlakeyTest] DEBUG dmtest - reload: table=0 32768 flakey /dev/loop0 0 0 1
> [info] FlakeyTest:
> [info] - flakey (read) *** FAILED ***
> [info]   Expected exception java.lang.Exception to be thrown, but no exception was thrown. (FlakeyTest.scala:9)
> [info] Run completed in 2 seconds, 781 milliseconds.
> [info] Total number of tests run: 1
> [info] Suites: completed 1, aborted 0
> [info] Tests: succeeded 0, failed 1, canceled 0, ignored 0, pending 0
> 
> - Akira
> 
> On 2016/07/06 18:52, Lukas Herbolt wrote:
>> Hi,
>> Yes this part is wrong and reads are not dropped.
>> I have a patch ready, just have to send it out.
>>
>> Lukas
>>
>> On Wed, Jul 6, 2016 at 8:33 AM, Akira Hayakawa <ruby.wktk@gmail.com> wrote:
>>
>>> Hi,
>>>
>>> I am using dm-flakey to emulate a broken device that should return -EIO on
>>> both read and write.
>>> I use the parameter up_interval=0 and down_interval=1.
>>>
>>> But when I am dd-ing the flakey device, while write fails, read succeeds.
>>> This isn't the behavior I expect.
>>>
>>> Then I looked into the code.
>>>
>>> 326 static int flakey_end_io(struct dm_target *ti, struct bio *bio, int
>>> error)
>>> 327 {
>>> 328         struct flakey_c *fc = ti->private;
>>> 329         struct per_bio_data *pb = dm_per_bio_data(bio, sizeof(struct
>>> per_bio_data));
>>> 330
>>> 331         /*
>>> 332          * Corrupt successful READs while in down state.
>>> 333          * If flags were specified, only corrupt those that match.
>>> 334          */
>>> 335         if (fc->corrupt_bio_byte && !error && pb->bio_submitted &&
>>> 336             (bio_data_dir(bio) == READ) && (fc->corrupt_bio_rw ==
>>> READ) &&
>>> 337             all_corrupt_bio_flags_match(bio, fc))
>>> 338                 corrupt_bio_data(bio, fc);
>>> 339
>>> 340         return error;
>>> 341 }
>>>
>>> When the bio direction is READ and currupt_bio_bytes isn't specified
>>> the READ bio is handled normally right?
>>>
>>> I think READ requests should return -EIO if
>>>
>>> 1. corrupt_bio_bytes isn't specified
>>> 2. but the requested is handled during down interval.
>>>
>>> as well as WRITE requests:
>>>
>>> 305                 /*
>>> 306                  * Corrupt matching writes.
>>> 307                  */
>>> 308                 if (fc->corrupt_bio_byte && (fc->corrupt_bio_rw ==
>>> WRITE)) {
>>> 309                         if (all_corrupt_bio_flags_match(bio, fc))
>>> 310                                 corrupt_bio_data(bio, fc);
>>> 311                         goto map_bio;
>>> 312                 }
>>> 313
>>> 314                 /*
>>> 315                  * By default, error all I/O.
>>> 316                  */
>>> 317                 return -EIO;
>>> 318         }
>>>
>>> This code is similar to what we see in the end_io.
>>>
>>> If my understanding is correct, I would like to modify dm-flakey to return
>>> -EIO in the end_io.
>>>
>>> I would like to request for comments.
>>>
>>> Thanks,
>>>
>>> - Akira
>>>
>>> --
>>> dm-devel mailing list
>>> dm-devel@redhat.com
>>> https://www.redhat.com/mailman/listinfo/dm-devel
>>>
>>
>>
>>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: dm-flakey NOT to return -EIO on READ?
  2016-07-17  1:36     ` Akira Hayakawa
@ 2016-07-29  6:25       ` Lukas Herbolt
  2016-07-29  6:32         ` Akira Hayakawa
  0 siblings, 1 reply; 12+ messages in thread
From: Lukas Herbolt @ 2016-07-29  6:25 UTC (permalink / raw)
  To: Akira Hayakawa; +Cc: device-mapper development


[-- Attachment #1.1: Type: text/plain, Size: 4786 bytes --]

Hi Akira,
Sorry I was on holiday.
I have patch ready, would you be able to test it before posting it.

Let me know if I should build the kernel for you or just share the source
with you.

Lukas

On Sun, Jul 17, 2016 at 3:36 AM, Akira Hayakawa <ruby.wktk@gmail.com> wrote:

> > On 2016/07/06 18:52, Lukas Herbolt wrote:
> >> Hi,
> >> Yes this part is wrong and reads are not dropped.
> >> I have a patch ready, just have to send it out.
>
> btw, is this fix going to be included in the next release?
>
> On 2016/07/06 21:54, Akira Hayakawa wrote:
> > Thanks,
> >
> > I want the patch in the main-tree quickly.
> > Because without it, my tests will not be green.
> > This is really annoying.
> >
> > I made a quick reproducer of this problem.
> > If you are looking for one, this will help.
> > (You need to install sbt first)
> >
> > https://github.com/akiradeveloper/writeboost-test-suite
> >
> >   test("flakey (read)") {
> >     Memory(Sector.M(16)) { a =>
> >       Flakey.Table(a, 0, 1).create { b =>
> >         intercept[Exception] {
> >           Shell(s"dd status=none if=${b.bdev.path} iflag=direct
> of=/dev/null")
> >         }
> >       }
> >     }
> >   }
> >
> > $ sudo sbt "testOnly dmtest.FlakeyTest"
> > 12:44:00.643 [pool-2-thread-3-ScalaTest-running-FlakeyTest] INFO dmtest
> - [TEST] flakey (read)
> > 12:44:00.752 [pool-2-thread-3-ScalaTest-running-FlakeyTest] DEBUG dmtest
> - reload: table=0 32768 flakey /dev/loop0 0 0 1
> > [info] FlakeyTest:
> > [info] - flakey (read) *** FAILED ***
> > [info]   Expected exception java.lang.Exception to be thrown, but no
> exception was thrown. (FlakeyTest.scala:9)
> > [info] Run completed in 2 seconds, 781 milliseconds.
> > [info] Total number of tests run: 1
> > [info] Suites: completed 1, aborted 0
> > [info] Tests: succeeded 0, failed 1, canceled 0, ignored 0, pending 0
> >
> > - Akira
> >
> > On 2016/07/06 18:52, Lukas Herbolt wrote:
> >> Hi,
> >> Yes this part is wrong and reads are not dropped.
> >> I have a patch ready, just have to send it out.
> >>
> >> Lukas
> >>
> >> On Wed, Jul 6, 2016 at 8:33 AM, Akira Hayakawa <ruby.wktk@gmail.com>
> wrote:
> >>
> >>> Hi,
> >>>
> >>> I am using dm-flakey to emulate a broken device that should return
> -EIO on
> >>> both read and write.
> >>> I use the parameter up_interval=0 and down_interval=1.
> >>>
> >>> But when I am dd-ing the flakey device, while write fails, read
> succeeds.
> >>> This isn't the behavior I expect.
> >>>
> >>> Then I looked into the code.
> >>>
> >>> 326 static int flakey_end_io(struct dm_target *ti, struct bio *bio, int
> >>> error)
> >>> 327 {
> >>> 328         struct flakey_c *fc = ti->private;
> >>> 329         struct per_bio_data *pb = dm_per_bio_data(bio,
> sizeof(struct
> >>> per_bio_data));
> >>> 330
> >>> 331         /*
> >>> 332          * Corrupt successful READs while in down state.
> >>> 333          * If flags were specified, only corrupt those that match.
> >>> 334          */
> >>> 335         if (fc->corrupt_bio_byte && !error && pb->bio_submitted &&
> >>> 336             (bio_data_dir(bio) == READ) && (fc->corrupt_bio_rw ==
> >>> READ) &&
> >>> 337             all_corrupt_bio_flags_match(bio, fc))
> >>> 338                 corrupt_bio_data(bio, fc);
> >>> 339
> >>> 340         return error;
> >>> 341 }
> >>>
> >>> When the bio direction is READ and currupt_bio_bytes isn't specified
> >>> the READ bio is handled normally right?
> >>>
> >>> I think READ requests should return -EIO if
> >>>
> >>> 1. corrupt_bio_bytes isn't specified
> >>> 2. but the requested is handled during down interval.
> >>>
> >>> as well as WRITE requests:
> >>>
> >>> 305                 /*
> >>> 306                  * Corrupt matching writes.
> >>> 307                  */
> >>> 308                 if (fc->corrupt_bio_byte && (fc->corrupt_bio_rw ==
> >>> WRITE)) {
> >>> 309                         if (all_corrupt_bio_flags_match(bio, fc))
> >>> 310                                 corrupt_bio_data(bio, fc);
> >>> 311                         goto map_bio;
> >>> 312                 }
> >>> 313
> >>> 314                 /*
> >>> 315                  * By default, error all I/O.
> >>> 316                  */
> >>> 317                 return -EIO;
> >>> 318         }
> >>>
> >>> This code is similar to what we see in the end_io.
> >>>
> >>> If my understanding is correct, I would like to modify dm-flakey to
> return
> >>> -EIO in the end_io.
> >>>
> >>> I would like to request for comments.
> >>>
> >>> Thanks,
> >>>
> >>> - Akira
> >>>
> >>> --
> >>> dm-devel mailing list
> >>> dm-devel@redhat.com
> >>> https://www.redhat.com/mailman/listinfo/dm-devel
> >>>
> >>
> >>
> >>
>



-- 
Lukas Herbolt
RHCE, RH436, BSc, SSc
Senior Technical Support Engineer
Global Support Services (GSS)
Email:    lherbolt@redhat.com

[-- Attachment #1.2: Type: text/html, Size: 8049 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: dm-flakey NOT to return -EIO on READ?
  2016-07-29  6:25       ` Lukas Herbolt
@ 2016-07-29  6:32         ` Akira Hayakawa
  2016-07-29 14:31           ` Lukas Herbolt
  0 siblings, 1 reply; 12+ messages in thread
From: Akira Hayakawa @ 2016-07-29  6:32 UTC (permalink / raw)
  To: Lukas Herbolt; +Cc: device-mapper development

Hi Lukas,

> I have patch ready, would you be able to test it before posting it.
> 
> Let me know if I should build the kernel for you or just share the source
> with you.

OK.
Please share the git tree and specify the branch to test.

Akira

On 2016/07/29 15:25, Lukas Herbolt wrote:
> Hi Akira,
> Sorry I was on holiday.
> I have patch ready, would you be able to test it before posting it.
> 
> Let me know if I should build the kernel for you or just share the source
> with you.
> 
> Lukas
> 
> On Sun, Jul 17, 2016 at 3:36 AM, Akira Hayakawa <ruby.wktk@gmail.com> wrote:
> 
>>> On 2016/07/06 18:52, Lukas Herbolt wrote:
>>>> Hi,
>>>> Yes this part is wrong and reads are not dropped.
>>>> I have a patch ready, just have to send it out.
>>
>> btw, is this fix going to be included in the next release?
>>
>> On 2016/07/06 21:54, Akira Hayakawa wrote:
>>> Thanks,
>>>
>>> I want the patch in the main-tree quickly.
>>> Because without it, my tests will not be green.
>>> This is really annoying.
>>>
>>> I made a quick reproducer of this problem.
>>> If you are looking for one, this will help.
>>> (You need to install sbt first)
>>>
>>> https://github.com/akiradeveloper/writeboost-test-suite
>>>
>>>   test("flakey (read)") {
>>>     Memory(Sector.M(16)) { a =>
>>>       Flakey.Table(a, 0, 1).create { b =>
>>>         intercept[Exception] {
>>>           Shell(s"dd status=none if=${b.bdev.path} iflag=direct
>> of=/dev/null")
>>>         }
>>>       }
>>>     }
>>>   }
>>>
>>> $ sudo sbt "testOnly dmtest.FlakeyTest"
>>> 12:44:00.643 [pool-2-thread-3-ScalaTest-running-FlakeyTest] INFO dmtest
>> - [TEST] flakey (read)
>>> 12:44:00.752 [pool-2-thread-3-ScalaTest-running-FlakeyTest] DEBUG dmtest
>> - reload: table=0 32768 flakey /dev/loop0 0 0 1
>>> [info] FlakeyTest:
>>> [info] - flakey (read) *** FAILED ***
>>> [info]   Expected exception java.lang.Exception to be thrown, but no
>> exception was thrown. (FlakeyTest.scala:9)
>>> [info] Run completed in 2 seconds, 781 milliseconds.
>>> [info] Total number of tests run: 1
>>> [info] Suites: completed 1, aborted 0
>>> [info] Tests: succeeded 0, failed 1, canceled 0, ignored 0, pending 0
>>>
>>> - Akira
>>>
>>> On 2016/07/06 18:52, Lukas Herbolt wrote:
>>>> Hi,
>>>> Yes this part is wrong and reads are not dropped.
>>>> I have a patch ready, just have to send it out.
>>>>
>>>> Lukas
>>>>
>>>> On Wed, Jul 6, 2016 at 8:33 AM, Akira Hayakawa <ruby.wktk@gmail.com>
>> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> I am using dm-flakey to emulate a broken device that should return
>> -EIO on
>>>>> both read and write.
>>>>> I use the parameter up_interval=0 and down_interval=1.
>>>>>
>>>>> But when I am dd-ing the flakey device, while write fails, read
>> succeeds.
>>>>> This isn't the behavior I expect.
>>>>>
>>>>> Then I looked into the code.
>>>>>
>>>>> 326 static int flakey_end_io(struct dm_target *ti, struct bio *bio, int
>>>>> error)
>>>>> 327 {
>>>>> 328         struct flakey_c *fc = ti->private;
>>>>> 329         struct per_bio_data *pb = dm_per_bio_data(bio,
>> sizeof(struct
>>>>> per_bio_data));
>>>>> 330
>>>>> 331         /*
>>>>> 332          * Corrupt successful READs while in down state.
>>>>> 333          * If flags were specified, only corrupt those that match.
>>>>> 334          */
>>>>> 335         if (fc->corrupt_bio_byte && !error && pb->bio_submitted &&
>>>>> 336             (bio_data_dir(bio) == READ) && (fc->corrupt_bio_rw ==
>>>>> READ) &&
>>>>> 337             all_corrupt_bio_flags_match(bio, fc))
>>>>> 338                 corrupt_bio_data(bio, fc);
>>>>> 339
>>>>> 340         return error;
>>>>> 341 }
>>>>>
>>>>> When the bio direction is READ and currupt_bio_bytes isn't specified
>>>>> the READ bio is handled normally right?
>>>>>
>>>>> I think READ requests should return -EIO if
>>>>>
>>>>> 1. corrupt_bio_bytes isn't specified
>>>>> 2. but the requested is handled during down interval.
>>>>>
>>>>> as well as WRITE requests:
>>>>>
>>>>> 305                 /*
>>>>> 306                  * Corrupt matching writes.
>>>>> 307                  */
>>>>> 308                 if (fc->corrupt_bio_byte && (fc->corrupt_bio_rw ==
>>>>> WRITE)) {
>>>>> 309                         if (all_corrupt_bio_flags_match(bio, fc))
>>>>> 310                                 corrupt_bio_data(bio, fc);
>>>>> 311                         goto map_bio;
>>>>> 312                 }
>>>>> 313
>>>>> 314                 /*
>>>>> 315                  * By default, error all I/O.
>>>>> 316                  */
>>>>> 317                 return -EIO;
>>>>> 318         }
>>>>>
>>>>> This code is similar to what we see in the end_io.
>>>>>
>>>>> If my understanding is correct, I would like to modify dm-flakey to
>> return
>>>>> -EIO in the end_io.
>>>>>
>>>>> I would like to request for comments.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> - Akira
>>>>>
>>>>> --
>>>>> dm-devel mailing list
>>>>> dm-devel@redhat.com
>>>>> https://www.redhat.com/mailman/listinfo/dm-devel
>>>>>
>>>>
>>>>
>>>>
>>
> 
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: dm-flakey NOT to return -EIO on READ?
  2016-07-29  6:32         ` Akira Hayakawa
@ 2016-07-29 14:31           ` Lukas Herbolt
  2016-07-29 14:35             ` Mike Snitzer
  0 siblings, 1 reply; 12+ messages in thread
From: Lukas Herbolt @ 2016-07-29 14:31 UTC (permalink / raw)
  To: Akira Hayakawa; +Cc: device-mapper development


[-- Attachment #1.1: Type: text/plain, Size: 5604 bytes --]

 - http://people.redhat.com/~lherbolt/dm-flakey/v4.7-dm_flakey.tar.gz

Lukas

On Fri, Jul 29, 2016 at 8:32 AM, Akira Hayakawa <ruby.wktk@gmail.com> wrote:

> Hi Lukas,
>
> > I have patch ready, would you be able to test it before posting it.
> >
> > Let me know if I should build the kernel for you or just share the source
> > with you.
>
> OK.
> Please share the git tree and specify the branch to test.
>
> Akira
>
> On 2016/07/29 15:25, Lukas Herbolt wrote:
> > Hi Akira,
> > Sorry I was on holiday.
> > I have patch ready, would you be able to test it before posting it.
> >
> > Let me know if I should build the kernel for you or just share the source
> > with you.
> >
> > Lukas
> >
> > On Sun, Jul 17, 2016 at 3:36 AM, Akira Hayakawa <ruby.wktk@gmail.com>
> wrote:
> >
> >>> On 2016/07/06 18:52, Lukas Herbolt wrote:
> >>>> Hi,
> >>>> Yes this part is wrong and reads are not dropped.
> >>>> I have a patch ready, just have to send it out.
> >>
> >> btw, is this fix going to be included in the next release?
> >>
> >> On 2016/07/06 21:54, Akira Hayakawa wrote:
> >>> Thanks,
> >>>
> >>> I want the patch in the main-tree quickly.
> >>> Because without it, my tests will not be green.
> >>> This is really annoying.
> >>>
> >>> I made a quick reproducer of this problem.
> >>> If you are looking for one, this will help.
> >>> (You need to install sbt first)
> >>>
> >>> https://github.com/akiradeveloper/writeboost-test-suite
> >>>
> >>>   test("flakey (read)") {
> >>>     Memory(Sector.M(16)) { a =>
> >>>       Flakey.Table(a, 0, 1).create { b =>
> >>>         intercept[Exception] {
> >>>           Shell(s"dd status=none if=${b.bdev.path} iflag=direct
> >> of=/dev/null")
> >>>         }
> >>>       }
> >>>     }
> >>>   }
> >>>
> >>> $ sudo sbt "testOnly dmtest.FlakeyTest"
> >>> 12:44:00.643 [pool-2-thread-3-ScalaTest-running-FlakeyTest] INFO dmtest
> >> - [TEST] flakey (read)
> >>> 12:44:00.752 [pool-2-thread-3-ScalaTest-running-FlakeyTest] DEBUG
> dmtest
> >> - reload: table=0 32768 flakey /dev/loop0 0 0 1
> >>> [info] FlakeyTest:
> >>> [info] - flakey (read) *** FAILED ***
> >>> [info]   Expected exception java.lang.Exception to be thrown, but no
> >> exception was thrown. (FlakeyTest.scala:9)
> >>> [info] Run completed in 2 seconds, 781 milliseconds.
> >>> [info] Total number of tests run: 1
> >>> [info] Suites: completed 1, aborted 0
> >>> [info] Tests: succeeded 0, failed 1, canceled 0, ignored 0, pending 0
> >>>
> >>> - Akira
> >>>
> >>> On 2016/07/06 18:52, Lukas Herbolt wrote:
> >>>> Hi,
> >>>> Yes this part is wrong and reads are not dropped.
> >>>> I have a patch ready, just have to send it out.
> >>>>
> >>>> Lukas
> >>>>
> >>>> On Wed, Jul 6, 2016 at 8:33 AM, Akira Hayakawa <ruby.wktk@gmail.com>
> >> wrote:
> >>>>
> >>>>> Hi,
> >>>>>
> >>>>> I am using dm-flakey to emulate a broken device that should return
> >> -EIO on
> >>>>> both read and write.
> >>>>> I use the parameter up_interval=0 and down_interval=1.
> >>>>>
> >>>>> But when I am dd-ing the flakey device, while write fails, read
> >> succeeds.
> >>>>> This isn't the behavior I expect.
> >>>>>
> >>>>> Then I looked into the code.
> >>>>>
> >>>>> 326 static int flakey_end_io(struct dm_target *ti, struct bio *bio,
> int
> >>>>> error)
> >>>>> 327 {
> >>>>> 328         struct flakey_c *fc = ti->private;
> >>>>> 329         struct per_bio_data *pb = dm_per_bio_data(bio,
> >> sizeof(struct
> >>>>> per_bio_data));
> >>>>> 330
> >>>>> 331         /*
> >>>>> 332          * Corrupt successful READs while in down state.
> >>>>> 333          * If flags were specified, only corrupt those that
> match.
> >>>>> 334          */
> >>>>> 335         if (fc->corrupt_bio_byte && !error && pb->bio_submitted
> &&
> >>>>> 336             (bio_data_dir(bio) == READ) && (fc->corrupt_bio_rw ==
> >>>>> READ) &&
> >>>>> 337             all_corrupt_bio_flags_match(bio, fc))
> >>>>> 338                 corrupt_bio_data(bio, fc);
> >>>>> 339
> >>>>> 340         return error;
> >>>>> 341 }
> >>>>>
> >>>>> When the bio direction is READ and currupt_bio_bytes isn't specified
> >>>>> the READ bio is handled normally right?
> >>>>>
> >>>>> I think READ requests should return -EIO if
> >>>>>
> >>>>> 1. corrupt_bio_bytes isn't specified
> >>>>> 2. but the requested is handled during down interval.
> >>>>>
> >>>>> as well as WRITE requests:
> >>>>>
> >>>>> 305                 /*
> >>>>> 306                  * Corrupt matching writes.
> >>>>> 307                  */
> >>>>> 308                 if (fc->corrupt_bio_byte && (fc->corrupt_bio_rw
> ==
> >>>>> WRITE)) {
> >>>>> 309                         if (all_corrupt_bio_flags_match(bio, fc))
> >>>>> 310                                 corrupt_bio_data(bio, fc);
> >>>>> 311                         goto map_bio;
> >>>>> 312                 }
> >>>>> 313
> >>>>> 314                 /*
> >>>>> 315                  * By default, error all I/O.
> >>>>> 316                  */
> >>>>> 317                 return -EIO;
> >>>>> 318         }
> >>>>>
> >>>>> This code is similar to what we see in the end_io.
> >>>>>
> >>>>> If my understanding is correct, I would like to modify dm-flakey to
> >> return
> >>>>> -EIO in the end_io.
> >>>>>
> >>>>> I would like to request for comments.
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>> - Akira
> >>>>>
> >>>>> --
> >>>>> dm-devel mailing list
> >>>>> dm-devel@redhat.com
> >>>>> https://www.redhat.com/mailman/listinfo/dm-devel
> >>>>>
> >>>>
> >>>>
> >>>>
> >>
> >
> >
> >
>



-- 
Lukas Herbolt
RHCE, RH436, BSc, SSc
Senior Technical Support Engineer
Global Support Services (GSS)
Email:    lherbolt@redhat.com

[-- Attachment #1.2: Type: text/html, Size: 9768 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: dm-flakey NOT to return -EIO on READ?
  2016-07-29 14:31           ` Lukas Herbolt
@ 2016-07-29 14:35             ` Mike Snitzer
  2016-07-29 17:30               ` Mike Snitzer
  0 siblings, 1 reply; 12+ messages in thread
From: Mike Snitzer @ 2016-07-29 14:35 UTC (permalink / raw)
  To: Lukas Herbolt; +Cc: device-mapper development, Akira Hayakawa

On Fri, Jul 29 2016 at 10:31am -0400,
Lukas Herbolt <lherbolt@redhat.com> wrote:

>  - http://people.redhat.com/~lherbolt/dm-flakey/v4.7-dm_flakey.tar.gz
> 
> Lukas

Please post an incremental patch relative to 4.7 or latest Linus
kernel.  Don't make people suffer with tarballs of whatever you've
packaged above.

Mike

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: dm-flakey NOT to return -EIO on READ?
  2016-07-29 14:35             ` Mike Snitzer
@ 2016-07-29 17:30               ` Mike Snitzer
  2016-07-29 18:00                 ` Lukas Herbolt
                                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Mike Snitzer @ 2016-07-29 17:30 UTC (permalink / raw)
  To: Lukas Herbolt; +Cc: device-mapper development, Akira Hayakawa

On Fri, Jul 29 2016 at 10:35am -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Fri, Jul 29 2016 at 10:31am -0400,
> Lukas Herbolt <lherbolt@redhat.com> wrote:
> 
> >  - http://people.redhat.com/~lherbolt/dm-flakey/v4.7-dm_flakey.tar.gz
> > 
> > Lukas
> 
> Please post an incremental patch relative to 4.7 or latest Linus
> kernel.  Don't make people suffer with tarballs of whatever you've
> packaged above.

I pulled out your incremental diff.  Your proposed fix isn't correct.  It
breaks the corrupt_bio_byte feature.  We cannot blindly drop reads early
in flakey_map(), like you've proposed, because the corrupt_bio_byte
feature relies on corrupt data being returned on read during the
down_interval.

The issue reported is: reads aren't dropped during the "down_interval".

I do agree that this needs fixing, and it was commit a3998799fb4df ("dm
flakey: add corrupt_bio_byte feature") that caused this regression.

But a fix must not break the corrupt_bio_byte feature.

This fix should resolve the problem, I'll be staging it to go upstream
during the 4.8-rc cycle (Akira, please test to verify your test fails as
expected now):

From: Mike Snitzer <snitzer@redhat.com>
Date: Fri, 29 Jul 2016 13:19:55 -0400
Subject: [PATCH] dm flakey: error READ bios during the down_interval

When the corrupt_bio_byte feature was introduced it caused READ bios to
no longer be errored with -EIO during the down_interval.  This had to do
with the complexity of needing to submit READs if the corrupt_bio_byte
feature was used.

Fix it so READ bios are properly errored with -EIO; doing so early in
flakey_map() as long as there isn't a match for the corrupt_bio_byte
feature.

Fixes: a3998799fb4df ("dm flakey: add corrupt_bio_byte feature")
Reported-by: Akira Hayakawa <ruby.wktk@gmail.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Cc: stable@vger.kernel.org
---
 drivers/md/dm-flakey.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/md/dm-flakey.c b/drivers/md/dm-flakey.c
index 29b99fb..19db13e 100644
--- a/drivers/md/dm-flakey.c
+++ b/drivers/md/dm-flakey.c
@@ -289,10 +289,16 @@ static int flakey_map(struct dm_target *ti, struct bio *bio)
 		pb->bio_submitted = true;
 
 		/*
-		 * Map reads as normal.
+		 * Map reads as normal only if corrupt_bio_byte set.
 		 */
-		if (bio_data_dir(bio) == READ)
-			goto map_bio;
+		if (bio_data_dir(bio) == READ) {
+			/* If flags were specified, only corrupt those that match. */
+			if (fc->corrupt_bio_byte && (fc->corrupt_bio_rw == READ) &&
+			    all_corrupt_bio_flags_match(bio, fc))
+				goto map_bio;
+			else
+				return -EIO;
+		}
 
 		/*
 		 * Drop writes?
@@ -330,12 +336,13 @@ static int flakey_end_io(struct dm_target *ti, struct bio *bio, int error)
 
 	/*
 	 * Corrupt successful READs while in down state.
-	 * If flags were specified, only corrupt those that match.
 	 */
-	if (fc->corrupt_bio_byte && !error && pb->bio_submitted &&
-	    (bio_data_dir(bio) == READ) && (fc->corrupt_bio_rw == READ) &&
-	    all_corrupt_bio_flags_match(bio, fc))
-		corrupt_bio_data(bio, fc);
+	if (!error && pb->bio_submitted && (bio_data_dir(bio) == READ)) {
+		if (fc->corrupt_bio_byte)
+			corrupt_bio_data(bio, fc);
+		else
+			return -EIO;
+	}
 
 	return error;
 }
-- 
2.7.4 (Apple Git-66)

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: dm-flakey NOT to return -EIO on READ?
  2016-07-29 17:30               ` Mike Snitzer
@ 2016-07-29 18:00                 ` Lukas Herbolt
  2016-07-29 22:55                 ` Akira Hayakawa
  2016-07-30  0:26                 ` Akira Hayakawa
  2 siblings, 0 replies; 12+ messages in thread
From: Lukas Herbolt @ 2016-07-29 18:00 UTC (permalink / raw)
  To: Mike Snitzer; +Cc: device-mapper development, Akira Hayakawa


[-- Attachment #1.1: Type: text/plain, Size: 4247 bytes --]

Thanks Mike for fixing.
This was my first attempt to fix something in kernel so I wasn't sure about
the patch itself.
 - Lukas

On Fri, Jul 29, 2016 at 7:30 PM, Mike Snitzer <snitzer@redhat.com> wrote:

> On Fri, Jul 29 2016 at 10:35am -0400,
> Mike Snitzer <snitzer@redhat.com> wrote:
>
> > On Fri, Jul 29 2016 at 10:31am -0400,
> > Lukas Herbolt <lherbolt@redhat.com> wrote:
> >
> > >  - http://people.redhat.com/~lherbolt/dm-flakey/v4.7-dm_flakey.tar.gz
> > >
> > > Lukas
> >
> > Please post an incremental patch relative to 4.7 or latest Linus
> > kernel.  Don't make people suffer with tarballs of whatever you've
> > packaged above.
>
> I pulled out your incremental diff.  Your proposed fix isn't correct.  It
> breaks the corrupt_bio_byte feature.  We cannot blindly drop reads early
> in flakey_map(), like you've proposed, because the corrupt_bio_byte
> feature relies on corrupt data being returned on read during the
> down_interval.
>
> The issue reported is: reads aren't dropped during the "down_interval".
>
> I do agree that this needs fixing, and it was commit a3998799fb4df ("dm
> flakey: add corrupt_bio_byte feature") that caused this regression.
>
> But a fix must not break the corrupt_bio_byte feature.
>
> This fix should resolve the problem, I'll be staging it to go upstream
> during the 4.8-rc cycle (Akira, please test to verify your test fails as
> expected now):
>
> From: Mike Snitzer <snitzer@redhat.com>
> Date: Fri, 29 Jul 2016 13:19:55 -0400
> Subject: [PATCH] dm flakey: error READ bios during the down_interval
>
> When the corrupt_bio_byte feature was introduced it caused READ bios to
> no longer be errored with -EIO during the down_interval.  This had to do
> with the complexity of needing to submit READs if the corrupt_bio_byte
> feature was used.
>
> Fix it so READ bios are properly errored with -EIO; doing so early in
> flakey_map() as long as there isn't a match for the corrupt_bio_byte
> feature.
>
> Fixes: a3998799fb4df ("dm flakey: add corrupt_bio_byte feature")
> Reported-by: Akira Hayakawa <ruby.wktk@gmail.com>
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/md/dm-flakey.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/md/dm-flakey.c b/drivers/md/dm-flakey.c
> index 29b99fb..19db13e 100644
> --- a/drivers/md/dm-flakey.c
> +++ b/drivers/md/dm-flakey.c
> @@ -289,10 +289,16 @@ static int flakey_map(struct dm_target *ti, struct
> bio *bio)
>                 pb->bio_submitted = true;
>
>                 /*
> -                * Map reads as normal.
> +                * Map reads as normal only if corrupt_bio_byte set.
>                  */
> -               if (bio_data_dir(bio) == READ)
> -                       goto map_bio;
> +               if (bio_data_dir(bio) == READ) {
> +                       /* If flags were specified, only corrupt those
> that match. */
> +                       if (fc->corrupt_bio_byte && (fc->corrupt_bio_rw ==
> READ) &&
> +                           all_corrupt_bio_flags_match(bio, fc))
> +                               goto map_bio;
> +                       else
> +                               return -EIO;
> +               }
>
>                 /*
>                  * Drop writes?
> @@ -330,12 +336,13 @@ static int flakey_end_io(struct dm_target *ti,
> struct bio *bio, int error)
>
>         /*
>          * Corrupt successful READs while in down state.
> -        * If flags were specified, only corrupt those that match.
>          */
> -       if (fc->corrupt_bio_byte && !error && pb->bio_submitted &&
> -           (bio_data_dir(bio) == READ) && (fc->corrupt_bio_rw == READ) &&
> -           all_corrupt_bio_flags_match(bio, fc))
> -               corrupt_bio_data(bio, fc);
> +       if (!error && pb->bio_submitted && (bio_data_dir(bio) == READ)) {
> +               if (fc->corrupt_bio_byte)
> +                       corrupt_bio_data(bio, fc);
> +               else
> +                       return -EIO;
> +       }
>
>         return error;
>  }
> --
> 2.7.4 (Apple Git-66)
>
>


-- 
Lukas Herbolt
RHCE, RH436, BSc, SSc
Senior Technical Support Engineer
Global Support Services (GSS)
Email:    lherbolt@redhat.com

[-- Attachment #1.2: Type: text/html, Size: 6565 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: dm-flakey NOT to return -EIO on READ?
  2016-07-29 17:30               ` Mike Snitzer
  2016-07-29 18:00                 ` Lukas Herbolt
@ 2016-07-29 22:55                 ` Akira Hayakawa
  2016-07-30  0:26                 ` Akira Hayakawa
  2 siblings, 0 replies; 12+ messages in thread
From: Akira Hayakawa @ 2016-07-29 22:55 UTC (permalink / raw)
  To: Mike Snitzer, Lukas Herbolt; +Cc: device-mapper development

I just looked at the patch. (not tested yet)

I think the else part in the endio isn't necessary.

> +		else
> +			return -EIO;

Akira


On 2016/07/30 2:30, Mike Snitzer wrote:
> On Fri, Jul 29 2016 at 10:35am -0400,
> Mike Snitzer <snitzer@redhat.com> wrote:
> 
>> On Fri, Jul 29 2016 at 10:31am -0400,
>> Lukas Herbolt <lherbolt@redhat.com> wrote:
>>
>>>  - http://people.redhat.com/~lherbolt/dm-flakey/v4.7-dm_flakey.tar.gz
>>>
>>> Lukas
>>
>> Please post an incremental patch relative to 4.7 or latest Linus
>> kernel.  Don't make people suffer with tarballs of whatever you've
>> packaged above.
> 
> I pulled out your incremental diff.  Your proposed fix isn't correct.  It
> breaks the corrupt_bio_byte feature.  We cannot blindly drop reads early
> in flakey_map(), like you've proposed, because the corrupt_bio_byte
> feature relies on corrupt data being returned on read during the
> down_interval.
> 
> The issue reported is: reads aren't dropped during the "down_interval".
> 
> I do agree that this needs fixing, and it was commit a3998799fb4df ("dm
> flakey: add corrupt_bio_byte feature") that caused this regression.
> 
> But a fix must not break the corrupt_bio_byte feature.
> 
> This fix should resolve the problem, I'll be staging it to go upstream
> during the 4.8-rc cycle (Akira, please test to verify your test fails as
> expected now):
> 
> From: Mike Snitzer <snitzer@redhat.com>
> Date: Fri, 29 Jul 2016 13:19:55 -0400
> Subject: [PATCH] dm flakey: error READ bios during the down_interval
> 
> When the corrupt_bio_byte feature was introduced it caused READ bios to
> no longer be errored with -EIO during the down_interval.  This had to do
> with the complexity of needing to submit READs if the corrupt_bio_byte
> feature was used.
> 
> Fix it so READ bios are properly errored with -EIO; doing so early in
> flakey_map() as long as there isn't a match for the corrupt_bio_byte
> feature.
> 
> Fixes: a3998799fb4df ("dm flakey: add corrupt_bio_byte feature")
> Reported-by: Akira Hayakawa <ruby.wktk@gmail.com>
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/md/dm-flakey.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/md/dm-flakey.c b/drivers/md/dm-flakey.c
> index 29b99fb..19db13e 100644
> --- a/drivers/md/dm-flakey.c
> +++ b/drivers/md/dm-flakey.c
> @@ -289,10 +289,16 @@ static int flakey_map(struct dm_target *ti, struct bio *bio)
>  		pb->bio_submitted = true;
>  
>  		/*
> -		 * Map reads as normal.
> +		 * Map reads as normal only if corrupt_bio_byte set.
>  		 */
> -		if (bio_data_dir(bio) == READ)
> -			goto map_bio;
> +		if (bio_data_dir(bio) == READ) {
> +			/* If flags were specified, only corrupt those that match. */
> +			if (fc->corrupt_bio_byte && (fc->corrupt_bio_rw == READ) &&
> +			    all_corrupt_bio_flags_match(bio, fc))
> +				goto map_bio;
> +			else
> +				return -EIO;
> +		}
>  
>  		/*
>  		 * Drop writes?
> @@ -330,12 +336,13 @@ static int flakey_end_io(struct dm_target *ti, struct bio *bio, int error)
>  
>  	/*
>  	 * Corrupt successful READs while in down state.
> -	 * If flags were specified, only corrupt those that match.
>  	 */
> -	if (fc->corrupt_bio_byte && !error && pb->bio_submitted &&
> -	    (bio_data_dir(bio) == READ) && (fc->corrupt_bio_rw == READ) &&
> -	    all_corrupt_bio_flags_match(bio, fc))
> -		corrupt_bio_data(bio, fc);
> +	if (!error && pb->bio_submitted && (bio_data_dir(bio) == READ)) {
> +		if (fc->corrupt_bio_byte)
> +			corrupt_bio_data(bio, fc);
> +		else
> +			return -EIO;
> +	}
>  
>  	return error;
>  }
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: dm-flakey NOT to return -EIO on READ?
  2016-07-29 17:30               ` Mike Snitzer
  2016-07-29 18:00                 ` Lukas Herbolt
  2016-07-29 22:55                 ` Akira Hayakawa
@ 2016-07-30  0:26                 ` Akira Hayakawa
  2 siblings, 0 replies; 12+ messages in thread
From: Akira Hayakawa @ 2016-07-30  0:26 UTC (permalink / raw)
  To: Mike Snitzer; +Cc: device-mapper development, Lukas Herbolt

Mike,

> This fix should resolve the problem, I'll be staging it to go upstream
> during the 4.8-rc cycle (Akira, please test to verify your test fails as
> expected now):

I tested with the patch and it's now ok.

Akira

On 2016/07/30 2:30, Mike Snitzer wrote:
> On Fri, Jul 29 2016 at 10:35am -0400,
> Mike Snitzer <snitzer@redhat.com> wrote:
> 
>> On Fri, Jul 29 2016 at 10:31am -0400,
>> Lukas Herbolt <lherbolt@redhat.com> wrote:
>>
>>>  - http://people.redhat.com/~lherbolt/dm-flakey/v4.7-dm_flakey.tar.gz
>>>
>>> Lukas
>>
>> Please post an incremental patch relative to 4.7 or latest Linus
>> kernel.  Don't make people suffer with tarballs of whatever you've
>> packaged above.
> 
> I pulled out your incremental diff.  Your proposed fix isn't correct.  It
> breaks the corrupt_bio_byte feature.  We cannot blindly drop reads early
> in flakey_map(), like you've proposed, because the corrupt_bio_byte
> feature relies on corrupt data being returned on read during the
> down_interval.
> 
> The issue reported is: reads aren't dropped during the "down_interval".
> 
> I do agree that this needs fixing, and it was commit a3998799fb4df ("dm
> flakey: add corrupt_bio_byte feature") that caused this regression.
> 
> But a fix must not break the corrupt_bio_byte feature.
> 
> This fix should resolve the problem, I'll be staging it to go upstream
> during the 4.8-rc cycle (Akira, please test to verify your test fails as
> expected now):
> 
> From: Mike Snitzer <snitzer@redhat.com>
> Date: Fri, 29 Jul 2016 13:19:55 -0400
> Subject: [PATCH] dm flakey: error READ bios during the down_interval
> 
> When the corrupt_bio_byte feature was introduced it caused READ bios to
> no longer be errored with -EIO during the down_interval.  This had to do
> with the complexity of needing to submit READs if the corrupt_bio_byte
> feature was used.
> 
> Fix it so READ bios are properly errored with -EIO; doing so early in
> flakey_map() as long as there isn't a match for the corrupt_bio_byte
> feature.
> 
> Fixes: a3998799fb4df ("dm flakey: add corrupt_bio_byte feature")
> Reported-by: Akira Hayakawa <ruby.wktk@gmail.com>
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/md/dm-flakey.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/md/dm-flakey.c b/drivers/md/dm-flakey.c
> index 29b99fb..19db13e 100644
> --- a/drivers/md/dm-flakey.c
> +++ b/drivers/md/dm-flakey.c
> @@ -289,10 +289,16 @@ static int flakey_map(struct dm_target *ti, struct bio *bio)
>  		pb->bio_submitted = true;
>  
>  		/*
> -		 * Map reads as normal.
> +		 * Map reads as normal only if corrupt_bio_byte set.
>  		 */
> -		if (bio_data_dir(bio) == READ)
> -			goto map_bio;
> +		if (bio_data_dir(bio) == READ) {
> +			/* If flags were specified, only corrupt those that match. */
> +			if (fc->corrupt_bio_byte && (fc->corrupt_bio_rw == READ) &&
> +			    all_corrupt_bio_flags_match(bio, fc))
> +				goto map_bio;
> +			else
> +				return -EIO;
> +		}
>  
>  		/*
>  		 * Drop writes?
> @@ -330,12 +336,13 @@ static int flakey_end_io(struct dm_target *ti, struct bio *bio, int error)
>  
>  	/*
>  	 * Corrupt successful READs while in down state.
> -	 * If flags were specified, only corrupt those that match.
>  	 */
> -	if (fc->corrupt_bio_byte && !error && pb->bio_submitted &&
> -	    (bio_data_dir(bio) == READ) && (fc->corrupt_bio_rw == READ) &&
> -	    all_corrupt_bio_flags_match(bio, fc))
> -		corrupt_bio_data(bio, fc);
> +	if (!error && pb->bio_submitted && (bio_data_dir(bio) == READ)) {
> +		if (fc->corrupt_bio_byte)
> +			corrupt_bio_data(bio, fc);
> +		else
> +			return -EIO;
> +	}
>  
>  	return error;
>  }
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-07-30  0:26 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-06  6:33 dm-flakey NOT to return -EIO on READ? Akira Hayakawa
2016-07-06  9:52 ` Lukas Herbolt
2016-07-06 12:54   ` Akira Hayakawa
2016-07-17  1:36     ` Akira Hayakawa
2016-07-29  6:25       ` Lukas Herbolt
2016-07-29  6:32         ` Akira Hayakawa
2016-07-29 14:31           ` Lukas Herbolt
2016-07-29 14:35             ` Mike Snitzer
2016-07-29 17:30               ` Mike Snitzer
2016-07-29 18:00                 ` Lukas Herbolt
2016-07-29 22:55                 ` Akira Hayakawa
2016-07-30  0:26                 ` Akira Hayakawa

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.