akiradeveloper / dm-writeboost

Log-structured Caching for Linux
GNU General Public License v2.0
123 stars 19 forks source link

revisit writeboost_end_io #137

Closed akiradeveloper closed 8 years ago

akiradeveloper commented 8 years ago

I've thought by myself on this point many many times but the code is possibly mistaken.

return 0 may be wrong. This could lead to acking ok in case of internal failure.

static int writeboost_end_io(struct dm_target *ti, struct bio *bio, int error)
{
    struct wb_device *wb = ti->private;
    struct per_bio_data *pbd = per_bio_data(wb, bio);

    switch (pbd->type) {
    case PBD_NONE:
        return 0;
    case PBD_WILL_CACHE:
        read_cache_cell_copy_data(wb, bio, error);
        return 0;
    case PBD_READ_SEG:
        dec_inflight_ios(wb, pbd->seg);
        return 0;
    default:
        BUG();
    }
}

I can write reproducer for this problem and if it's really the case I will resolve this in 2.2.4 and release quickly because it's super critical. Sorry for the unscheduled releases

This could be a solver for #111. I can make an analysis if I got a dmsetup status

akiradeveloper commented 8 years ago

no problem at all.

There is a fault injection test for this case and it's passed green.

  test("read from backing") {
    slowDevice(Sector.M(128)) { _slow => Linear.Table(_slow).create { slow =>
      fastDevice(Sector.M(32)) { _fast => Linear.Table(_fast).create { fast =>
        Writeboost.sweepCaches(fast)
        Writeboost.Table(slow, fast).create { s =>
          slow.reload(Flakey.Table(_slow, 0, 1)) // should use _slow
          fast.reload(Flakey.Table(_fast, 0, 1))

          // error detected from backing device
          logger.info("reading")
          val st1 = s.status
          intercept[Exception] {
            s.bdev.read(Sector(0), Sector(7))
          }

return 0 means "take the original error as is". so the original error isn't ignored.