On Mon, Dec 08 2014 at 5:32P -0500, Josef Bacik <jbacik@xxxxxx> wrote: > This is my latest attempt at a target for testing power fail and fs consistency. > This is based on the idea Zach Brown had where we could just walk through all > the operations done to an fs in order to verify we're doing the correct thing. > There is a userspace component as well that can be found here > > https://github.com/josefbacik/log-writes > > It is very rough as I just threw it together to test the various aspects of how > you would want to replay a log to test it. Again I would love feedback on this, > I really want to have something that we all think is usefull and eventually > incorporate it into xfstests. hey josef, i finally took a quick look at your target. has this target proven useful to you (or others) since you posted? would you still like to see this land upstream? i see no need to stack discard limits if discards are supported by the underlying device (you'll get that for free via blk_stack_limits). here is a patch that applies ontop of your original submission. mostly whitespace (i'm not pedantic about 80 char limit, etc). but i did include some small improvements and FIXMEs/questions in the patch: diff --git a/drivers/md/dm-log-writes.c b/drivers/md/dm-log-writes.c index 4aa627a..3c84777 100644 --- a/drivers/md/dm-log-writes.c +++ b/drivers/md/dm-log-writes.c @@ -137,7 +137,7 @@ static void log_end_io(struct bio *bio, int err) if (err) { unsigned long flags; - DMERR("Error writing log block %d\n", err); + DMERR("Error writing log block %d", err); spin_lock_irqsave(&lc->blocks_lock, flags); lc->logging_enabled = false; spin_unlock_irqrestore(&lc->blocks_lock, flags); @@ -335,8 +335,7 @@ static int log_writes_kthread(void *arg) spin_lock_irq(&lc->blocks_lock); if (!list_empty(&lc->logging_blocks)) { block = list_first_entry(&lc->logging_blocks, - struct pending_block, - list); + struct pending_block, list); list_del_init(&block->list); if (!lc->logging_enabled) goto next; @@ -362,8 +361,7 @@ static int log_writes_kthread(void *arg) lc->logged_entries++; atomic_inc(&lc->io_blocks); - super = (block->flags & - (LOG_FUA_FLAG | LOG_MARK_FLAG)); + super = (block->flags & (LOG_FUA_FLAG | LOG_MARK_FLAG)); if (super) atomic_inc(&lc->io_blocks); } @@ -380,9 +378,8 @@ next: lc->logging_enabled = false; spin_unlock_irq(&lc->blocks_lock); } - } else { + } else free_pending_block(lc, block); - } continue; } @@ -429,15 +426,13 @@ static int log_writes_ctr(struct dm_target *ti, unsigned int argc, char **argv) atomic_set(&lc->pending_blocks, 0); devname = dm_shift_arg(&as); - if (dm_get_device(ti, devname, dm_table_get_mode(ti->table), - &lc->dev)) { + if (dm_get_device(ti, devname, dm_table_get_mode(ti->table), &lc->dev)) { ti->error = "Device lookup failed"; goto bad; } logdevname = dm_shift_arg(&as); - if (dm_get_device(ti, logdevname, dm_table_get_mode(ti->table), - &lc->logdev)) { + if (dm_get_device(ti, logdevname, dm_table_get_mode(ti->table), &lc->logdev)) { ti->error = "Log device lookup failed"; dm_put_device(ti, lc->dev); goto bad; @@ -477,13 +472,13 @@ static int log_mark(struct log_writes_c *lc, char *data) block = kzalloc(sizeof(struct pending_block), GFP_KERNEL); if (!block) { - DMERR("Error allocating pending block\n"); + DMERR("Error allocating pending block"); return -ENOMEM; } block->data = kstrndup(data, maxsize, GFP_KERNEL); if (!block->data) { - DMERR("Error copying mark data\n"); + DMERR("Error copying mark data"); kfree(block); return -ENOMEM; } @@ -527,9 +522,10 @@ static void normal_map_bio(struct dm_target *ti, struct bio *bio) struct log_writes_c *lc = ti->private; bio->bi_bdev = lc->dev->bdev; + // FIXME: why would bi_sector ever need to be changed? + // if you just copied dm-linear then it is misplaced since there isn't an offset if (bio_sectors(bio)) - bio->bi_iter.bi_sector = - dm_target_offset(ti, bio->bi_iter.bi_sector); + bio->bi_iter.bi_sector = dm_target_offset(ti, bio->bi_iter.bi_sector); } static int log_writes_map(struct dm_target *ti, struct bio *bio) @@ -568,8 +564,8 @@ static int log_writes_map(struct dm_target *ti, struct bio *bio) if (discard_bio) alloc_size = sizeof(struct pending_block); else - alloc_size = sizeof(struct pending_block) + - sizeof(struct bio_vec) * bio_segments(bio); + alloc_size = sizeof(struct pending_block) + sizeof(struct bio_vec) * bio_segments(bio); + block = kzalloc(alloc_size, GFP_NOIO); if (!block) { DMERR("Error allocating pending block"); @@ -614,6 +610,9 @@ static int log_writes_map(struct dm_target *ti, struct bio *bio) * the actual contents into new pages so we know the data will always be * there. */ + // FIXME: why not just hold onto the original bio until "way later"? + // would doing so compromise the target's function? + // seems it'd avoid all this duplication (of state and data) in pending_block bio_for_each_segment(bv, bio, iter) { struct page *page; void *src, *dst; @@ -661,16 +660,14 @@ static int normal_end_io(struct dm_target *ti, struct bio *bio, int error) spin_lock_irqsave(&lc->blocks_lock, flags); if (block->flags & LOG_FLUSH_FLAG) { - list_splice_tail_init(&block->list, - &lc->logging_blocks); + list_splice_tail_init(&block->list, &lc->logging_blocks); list_add_tail(&block->list, &lc->logging_blocks); wake_up_process(lc->log_kthread); } else if (block->flags & LOG_FUA_FLAG) { list_add_tail(&block->list, &lc->logging_blocks); wake_up_process(lc->log_kthread); - } else { + } else list_add_tail(&block->list, &lc->unflushed_blocks); - } spin_unlock_irqrestore(&lc->blocks_lock, flags); } @@ -692,11 +689,11 @@ static void log_writes_status(struct dm_target *ti, status_type_t type, DMEMIT("%llu %llu", lc->logged_entries, (unsigned long long)lc->next_sector - 1); if (!lc->logging_enabled) - DMEMIT(" logging disabled"); + DMEMIT(" logging_disabled"); break; case STATUSTYPE_TABLE: - DMEMIT("%s %s ", lc->dev->name, lc->logdev->name); + DMEMIT("%s %s", lc->dev->name, lc->logdev->name); break; } } @@ -742,57 +739,37 @@ static int log_writes_iterate_devices(struct dm_target *ti, } /* - * Valid messages - * - * mark <mark data> - specify the marked data. + * Messages supported: + * mark <mark data> - specify the marked data. */ static int log_writes_message(struct dm_target *ti, unsigned argc, char **argv) { + int r = -EINVAL; struct log_writes_c *lc = ti->private; if (argc != 2) { - DMWARN("Invalid log-writes message arguments, expect 2 " - "argument, got %d", argc); - return -EINVAL; + DMWARN("Invalid log-writes message arguments, expect 2 arguments, got %d", argc); + return r; } - if (!strcasecmp(argv[0], "mark")) { - int ret; - - ret = log_mark(lc, argv[1]); - if (ret) - return ret; - } else { - DMWARN("Invalid argument %s", argv[0]); - return -EINVAL; - } + if (!strcasecmp(argv[0], "mark")) + r = log_mark(lc, argv[1]); + else + DMWARN("Unrecognised log writes target message received: %s", argv[0]); - return 0; + return r; } -static void log_writes_io_hints(struct dm_target *ti, - struct queue_limits *limits) +static void log_writes_io_hints(struct dm_target *ti, struct queue_limits *limits) { struct log_writes_c *lc = ti->private; struct request_queue *q = bdev_get_queue(lc->dev->bdev); - sector_t max_discard = (UINT_MAX >> SECTOR_SHIFT); - if (!q || !blk_queue_discard(q)) + if (!q || !blk_queue_discard(q)) { lc->device_supports_discard = false; - - if (lc->device_supports_discard) { - struct queue_limits *data_limits = &q->limits; - limits->max_discard_sectors = - min_t(unsigned int, data_limits->max_discard_sectors, - max_discard); - limits->discard_granularity = - max_t(unsigned int, data_limits->discard_granularity, - 1 << SECTOR_SHIFT); - } else { limits->discard_granularity = 1 << SECTOR_SHIFT; - limits->max_discard_sectors = max_discard; + limits->max_discard_sectors = (UINT_MAX >> SECTOR_SHIFT); } - dump_stack(); } static struct target_type log_writes_target = { @@ -830,6 +807,6 @@ static void __exit dm_log_writes_exit(void) module_init(dm_log_writes_init); module_exit(dm_log_writes_exit); -MODULE_DESCRIPTION(DM_NAME " write log target"); +MODULE_DESCRIPTION(DM_NAME " log writes target"); MODULE_AUTHOR("Josef Bacik <jbacik@xxxxxx>"); MODULE_LICENSE("GPL"); -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
