diff mbox

[RFC] dm: log writes target

Message ID 20150204184108.GA2801@redhat.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Mike Snitzer Feb. 4, 2015, 6:41 p.m. UTC
On Mon, Dec 08 2014 at  5:32P -0500,
Josef Bacik <jbacik@fb.com> 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:

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Mike Snitzer Feb. 4, 2015, 11:34 p.m. UTC | #1
On Wed, Feb 04 2015 at  1:41pm -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> @@ -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);
>  }

In above FIXME: s/misplaced/incomplete/

FYI sharing this quick private exchange I had with Alasdair just to make
sure I'm clear on what the above FIXME was trying to convey:

On Wed, Feb 04 2015 at  6:28pm -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Wed, Feb 04 2015 at  5:43pm -0500,
> Alasdair G Kergon <agk@redhat.com> wrote:
>
> > On Wed, Feb 04, 2015 at 01:41:09PM -0500, Mike Snitzer wrote:
> > > + // 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
> >
> > Why shouldn't we use this target on the 2nd or later line of a
> table?
>
> Wasn't saying it shouldn't.  Just was saying that Josef's target didn't
> go as far as say dm-linear with its 'start' offset.  Without it then
> this dm-log-writes target implicitly assumes every logical address
> space, be it 2nd or later, identity maps to exact same offset on the
> backing the data disk.
>
> My FIXME was too cryptic.  I can follow-up on this point.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Josef Bacik Feb. 5, 2015, 4:56 p.m. UTC | #2
On 02/04/2015 01:41 PM, Mike Snitzer wrote:
> On Mon, Dec 08 2014 at  5:32P -0500,
> Josef Bacik <jbacik@fb.com> 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:
>

Hello,

Yup I still want to land this upstream, I've been using it to find 
balance bugs and other things in btrfs.  With the holidays and all my 
family medical things going on recently I haven't had as much time to 
put into this as I wanted but I've made some improvements to it 
recently.  My plan was to wait until LSF so I could present my ideas to 
all the FS and DM guys at the same time to make sure we were happy with 
this approach and then submit shortly after that.

I will apply this and look through the FIXME's soon and send another 
update to make sure things still look good.  Thanks for looking at this,

Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

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@fb.com>");
 MODULE_LICENSE("GPL");