dm-thin/xfs: prototype a block reservation allocation model

From: Mike Snitzer <[email protected]>

[BF:
- Killed wrapper functions.
- Condensed to single bdev op.]

Signed-off-by Mike Snitzer <[email protected]>

---
 include/linux/blkdev.h | 6 ++++++
 1 file changed, 6 insertions(+)

include/linux/blkdev.h +4 -0

index 669e419..6c6ea96 100644
@@ -1650,6 +1650,10 @@ struct blk_dax_ctl {
 	pfn_t pfn;
 };
 
+#define BDEV_RES_GET		0
+#define BDEV_RES_MOD		(1 << 0)
+#define BDEV_RES_PROVISION	(1 << 1)
+
 struct block_device_operations {
 	int (*open) (struct block_device *, fmode_t);
 	void (*release) (struct gendisk *, fmode_t);

From: Joe Thornber <[email protected]>

Experimental reserve interface for XFS guys to play with.

I have big reservations (no pun intended) about this patch.

[BF:
- Support for reservation reduction.
- Support for space provisioning.
- Condensed to a single function.]

Not-Signed-off-by Joe Thornber <[email protected]>

Not-Signed-off-by Mike Snitzer <[email protected]>

---
 drivers/md/dm-thin.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 171 insertions(+), 10 deletions(-)

drivers/md/dm-thin.c +105 -10

index 92237b6..32bc5bd 100644
@@ -271,6 +271,8 @@ struct pool {
 	process_mapping_fn process_prepared_discard;
 
 	struct dm_bio_prison_cell **cell_sort_array;
+
+	dm_block_t reserve_count;
 };
 
 static enum pool_mode get_pool_mode(struct pool *pool);

@@ -318,6 +320,8 @@ struct thin_c {
 	 */
 	atomic_t refcount;
 	struct completion can_destroy;
+
+	dm_block_t reserve_count;
 };
 
 /*----------------------------------------------------------------*/

@@ -1359,24 +1363,19 @@ static void check_low_water_mark(struct pool *pool, dm_block_t free_blocks)
 	}
 }
 
-static int alloc_data_block(struct thin_c *tc, dm_block_t *result)
+static int get_free_blocks(struct pool *pool, dm_block_t *free_blocks)
 {
 	int r;
-	dm_block_t free_blocks;
-	struct pool *pool = tc->pool;
-
-	if (WARN_ON(get_pool_mode(pool) != PM_WRITE))
-		return -EINVAL;
 
-	r = dm_pool_get_free_block_count(pool->pmd, &free_blocks);
+	r = dm_pool_get_free_block_count(pool->pmd, free_blocks);
 	if (r) {
 		metadata_operation_failed(pool, "dm_pool_get_free_block_count", r);
 		return r;
 	}
 
-	check_low_water_mark(pool, free_blocks);
+	check_low_water_mark(pool, *free_blocks);
 
-	if (!free_blocks) {
+	if (!*free_blocks) {
 		/*
 		 * Try to commit to see if that will free up some
 		 * more space.

@@ -1385,7 +1384,7 @@ static int alloc_data_block(struct thin_c *tc, dm_block_t *result)
 		if (r)
 			return r;
 
-		r = dm_pool_get_free_block_count(pool->pmd, &free_blocks);
+		r = dm_pool_get_free_block_count(pool->pmd, free_blocks);
 		if (r) {
 			metadata_operation_failed(pool, "dm_pool_get_free_block_count", r);
 			return r;

@@ -1397,6 +1396,76 @@ static int alloc_data_block(struct thin_c *tc, dm_block_t *result)
 		}
 	}
 
+	return r;
+}
+
+/*
+ * Returns true iff either:
+ * i) decrement succeeded (ie. there was reserve left)
+ * ii) there is extra space in the pool
+ */
+static bool dec_reserve_count(struct thin_c *tc, dm_block_t free_blocks)
+{
+	bool r = false;
+	unsigned long flags;
+
+	if (!free_blocks)
+		return false;
+
+	spin_lock_irqsave(&tc->pool->lock, flags);
+	if (tc->reserve_count > 0) {
+		tc->reserve_count--;
+		tc->pool->reserve_count--;
+		r = true;
+	} else {
+		if (free_blocks > tc->pool->reserve_count)
+			r = true;
+	}
+	spin_unlock_irqrestore(&tc->pool->lock, flags);
+
+	return r;
+}
+
+static int set_reserve_count(struct thin_c *tc, dm_block_t count)
+{
+	int r;
+	dm_block_t free_blocks;
+	int64_t delta;
+	unsigned long flags;
+
+	r = get_free_blocks(tc->pool, &free_blocks);
+	if (r)
+		return r;
+
+	spin_lock_irqsave(&tc->pool->lock, flags);
+	delta = count - tc->reserve_count;
+	if (tc->pool->reserve_count + delta > free_blocks)
+		r = -ENOSPC;
+	else {
+		tc->reserve_count = count;
+		tc->pool->reserve_count += delta;
+	}
+	spin_unlock_irqrestore(&tc->pool->lock, flags);
+
+	return r;
+}
+
+static int alloc_data_block(struct thin_c *tc, dm_block_t *result)
+{
+	int r;
+	dm_block_t free_blocks;
+	struct pool *pool = tc->pool;
+
+	if (WARN_ON(get_pool_mode(pool) != PM_WRITE))
+		return -EINVAL;
+
+	r = get_free_blocks(tc->pool, &free_blocks);
+	if (r)
+		return r;
+
+	if (!dec_reserve_count(tc, free_blocks))
+		return -ENOSPC;
+
 	r = dm_pool_alloc_data_block(pool->pmd, result);
 	if (r) {
 		metadata_operation_failed(pool, "dm_pool_alloc_data_block", r);

@@ -2880,6 +2949,7 @@ static struct pool *pool_create(struct mapped_device *pool_md,
 	pool->last_commit_jiffies = jiffies;
 	pool->pool_md = pool_md;
 	pool->md_dev = metadata_dev;
+	pool->reserve_count = 0;
 	__pool_table_insert(pool);
 
 	return pool;

@@ -3936,6 +4006,7 @@ static void thin_dtr(struct dm_target *ti)
 
 	spin_lock_irqsave(&tc->pool->lock, flags);
 	list_del_rcu(&tc->list);
+	tc->pool->reserve_count -= tc->reserve_count;
 	spin_unlock_irqrestore(&tc->pool->lock, flags);
 	synchronize_rcu();
 

@@ -4074,6 +4145,7 @@ static int thin_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	init_completion(&tc->can_destroy);
 	list_add_tail_rcu(&tc->list, &tc->pool->active_thins);
 	spin_unlock_irqrestore(&tc->pool->lock, flags);
+	tc->reserve_count = 0;
 	/*
 	 * This synchronize_rcu() call is needed here otherwise we risk a
 	 * wake_worker() call finding no bios to process (because the newly

@@ -4271,6 +4343,94 @@ static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits)
 	limits->max_discard_sectors = 2048 * 1024 * 16; /* 16G */
 }
 
+static int thin_provision_space(struct dm_target *ti, sector_t offset,
+				sector_t len, sector_t *res)
+{
+	struct thin_c *tc = ti->private;
+	struct pool *pool = tc->pool;
+	sector_t end;
+	dm_block_t pblock;
+	dm_block_t vblock;
+	int error;
+	struct dm_thin_lookup_result lookup;
+
+	if (!is_factor(offset, pool->sectors_per_block))
+		return -EINVAL;
+
+	if (!len || !is_factor(len, pool->sectors_per_block))
+		return -EINVAL;
+
+	if (res && !is_factor(*res, pool->sectors_per_block))
+		return -EINVAL;
+
+	end = offset + len;
+
+	while (offset < end) {

drivers/md/dm-thin.c +52 -0

+		if (error)
+			return error;
+
+		if (res && *res)
+			*res -= pool->sectors_per_block;
+next:
+		offset += pool->sectors_per_block;
+	}
+
+	return 0;
+}
+
+static int thin_reserve_space(struct dm_target *ti, int mode, sector_t offset,
+			      sector_t len, sector_t *res)
+{
+	struct thin_c *tc = ti->private;
+	struct pool *pool = tc->pool;
+	sector_t blocks;
+	unsigned long flags;
+	int error;
+
+	if (mode == BDEV_RES_PROVISION)
+		return thin_provision_space(ti, offset, len, res);
+
+	/* res required for get/set */
+	error = -EINVAL;
+	if (!res)
+		return error;
+
+	if (mode == BDEV_RES_GET) {
+		spin_lock_irqsave(&tc->pool->lock, flags);
+		*res = tc->reserve_count * pool->sectors_per_block;
+		spin_unlock_irqrestore(&tc->pool->lock, flags);
+		error = 0;
+	} else if (mode == BDEV_RES_MOD) {
+		/*
+		* @res must always be a factor of the pool's blocksize; upper
+		* layers can rely on the bdev's minimum_io_size for this.
+		*/
+		if (!is_factor(*res, pool->sectors_per_block))
+			return error;
+
+		blocks = *res;
+		(void) sector_div(blocks, pool->sectors_per_block);
+
+		error = set_reserve_count(tc, blocks);
+	}
+
+	return error;
+}
+
 static struct target_type thin_target = {
 	.name = "thin",
 	.version = {1, 18, 0},

@@ -4285,6 +4445,7 @@ static struct target_type thin_target = {
 	.status = thin_status,
 	.iterate_devices = thin_iterate_devices,
 	.io_hints = thin_io_hints,
+	.reserve_space = thin_reserve_space,
 };
 
 /*----------------------------------------------------------------*/

Signature · 6 lines

2.4.11

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 21 lines · Brian Foster

From: Joe Thornber <[email protected]>

Experimental reserve interface for XFS guys to play with.

I have big reservations (no pun intended) about this patch.

[BF:
- Support for reservation reduction.
- Support for space provisioning.
- Condensed to a single function.]

Not-Signed-off-by Joe Thornber <[email protected]>

Not-Signed-off-by Mike Snitzer <[email protected]>

---
 drivers/md/dm-thin.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 171 insertions(+), 10 deletions(-)

drivers/md/dm-thin.c +0 -0

index 92237b6..32bc5bd 100644

Quoted 33 lines

drivers/md/dm-thin.c +29 -0

@@ -4271,6 +4343,94 @@ static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits)
 	limits->max_discard_sectors = 2048 * 1024 * 16; /* 16G */
 }
 
+static int thin_provision_space(struct dm_target *ti, sector_t offset,
+				sector_t len, sector_t *res)
+{
+	struct thin_c *tc = ti->private;
+	struct pool *pool = tc->pool;
+	sector_t end;
+	dm_block_t pblock;
+	dm_block_t vblock;
+	int error;
+	struct dm_thin_lookup_result lookup;
+
+	if (!is_factor(offset, pool->sectors_per_block))
+		return -EINVAL;
+
+	if (!len || !is_factor(len, pool->sectors_per_block))
+		return -EINVAL;
+
+	if (res && !is_factor(*res, pool->sectors_per_block))
+		return -EINVAL;
+
+	end = offset + len;
+
+	while (offset < end) {
+		vblock = offset;
+		do_div(vblock, pool->sectors_per_block);
+
+		error = dm_thin_find_block(tc->td, vblock, true, &lookup);
+		if (error == 0)
+			goto next;

drivers/md/dm-thin.c static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits) · line 4378

Quoted 4 lines

drivers/md/dm-thin.c +4 -0

+		if (error != -ENODATA)
+			return error;
+
+		error = alloc_data_block(tc, &pblock);

So this means that if fallocate wants to BDEV_RES_PROVISION N blocks, it must first increase the reservation (BDEV_RES_MOD) by N blocks to avoid using up space that was previously reserved by some other caller. I think?

drivers/md/dm-thin.c static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits) · line 4382

Quoted 4 lines

drivers/md/dm-thin.c +4 -0

+		if (error)
+			return error;
+
+		error = dm_thin_insert_block(tc->td, vblock, pblock);

Having reserved and mapped blocks, what happens when we try to read them? Do we actually get zeroes, or does the read go straight through to whatever happens to be in the disk blocks? I don't think it's correct that we could BDEV_RES_PROVISION and end up with stale credit card numbers from some other thin device.

(PS: I don't know enough about thinp to know if this has already been taken care of. I didn't see anything, but who knows what I missed. :))

--D

Quoted 69 lines

drivers/md/dm-thin.c +52 -0

+		if (error)
+			return error;
+
+		if (res && *res)
+			*res -= pool->sectors_per_block;
+next:
+		offset += pool->sectors_per_block;
+	}
+
+	return 0;
+}
+
+static int thin_reserve_space(struct dm_target *ti, int mode, sector_t offset,
+			      sector_t len, sector_t *res)
+{
+	struct thin_c *tc = ti->private;
+	struct pool *pool = tc->pool;
+	sector_t blocks;
+	unsigned long flags;
+	int error;
+
+	if (mode == BDEV_RES_PROVISION)
+		return thin_provision_space(ti, offset, len, res);
+
+	/* res required for get/set */
+	error = -EINVAL;
+	if (!res)
+		return error;
+
+	if (mode == BDEV_RES_GET) {
+		spin_lock_irqsave(&tc->pool->lock, flags);
+		*res = tc->reserve_count * pool->sectors_per_block;
+		spin_unlock_irqrestore(&tc->pool->lock, flags);
+		error = 0;
+	} else if (mode == BDEV_RES_MOD) {
+		/*
+		* @res must always be a factor of the pool's blocksize; upper
+		* layers can rely on the bdev's minimum_io_size for this.
+		*/
+		if (!is_factor(*res, pool->sectors_per_block))
+			return error;
+
+		blocks = *res;
+		(void) sector_div(blocks, pool->sectors_per_block);
+
+		error = set_reserve_count(tc, blocks);
+	}
+
+	return error;
+}
+
 static struct target_type thin_target = {
 	.name = "thin",
 	.version = {1, 18, 0},

@@ -4285,6 +4445,7 @@ static struct target_type thin_target = {
 	.status = thin_status,
 	.iterate_devices = thin_iterate_devices,
 	.io_hints = thin_io_hints,
+	.reserve_space = thin_reserve_space,
 };
 
 /*----------------------------------------------------------------*/

Signature · 6 lines

2.4.11

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

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

Quoted 22 lines · Darrick J. Wong

Quoted 21 lines · Brian Foster

From: Joe Thornber <[email protected]>

Experimental reserve interface for XFS guys to play with.

I have big reservations (no pun intended) about this patch.

[BF:
- Support for reservation reduction.
- Support for space provisioning.
- Condensed to a single function.]

Not-Signed-off-by Joe Thornber <[email protected]>

Not-Signed-off-by Mike Snitzer <[email protected]>

---
 drivers/md/dm-thin.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 171 insertions(+), 10 deletions(-)

drivers/md/dm-thin.c +0 -0

index 92237b6..32bc5bd 100644

...

Quoted 33 lines

Quoted 33 lines

drivers/md/dm-thin.c +29 -0

@@ -4271,6 +4343,94 @@ static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits)
 	limits->max_discard_sectors = 2048 * 1024 * 16; /* 16G */
 }
 
+static int thin_provision_space(struct dm_target *ti, sector_t offset,
+				sector_t len, sector_t *res)
+{
+	struct thin_c *tc = ti->private;
+	struct pool *pool = tc->pool;
+	sector_t end;
+	dm_block_t pblock;
+	dm_block_t vblock;
+	int error;
+	struct dm_thin_lookup_result lookup;
+
+	if (!is_factor(offset, pool->sectors_per_block))
+		return -EINVAL;
+
+	if (!len || !is_factor(len, pool->sectors_per_block))
+		return -EINVAL;
+
+	if (res && !is_factor(*res, pool->sectors_per_block))
+		return -EINVAL;
+
+	end = offset + len;
+
+	while (offset < end) {
+		vblock = offset;
+		do_div(vblock, pool->sectors_per_block);
+
+		error = dm_thin_find_block(tc->td, vblock, true, &lookup);
+		if (error == 0)
+			goto next;

Quoted 9 lines

drivers/md/dm-thin.c static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits) · line 4378

Quoted 4 lines

drivers/md/dm-thin.c +4 -0

+		if (error != -ENODATA)
+			return error;
+
+		error = alloc_data_block(tc, &pblock);

So this means that if fallocate wants to BDEV_RES_PROVISION N blocks, it must first increase the reservation (BDEV_RES_MOD) by N blocks to avoid using up space that was previously reserved by some other caller. I think?

Yes, assuming this is being called from a filesystem using the reservation mechanism.

Quoted 11 lines

drivers/md/dm-thin.c static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits) · line 4382

Quoted 4 lines

drivers/md/dm-thin.c +4 -0

+		if (error)
+			return error;
+
+		error = dm_thin_insert_block(tc->td, vblock, pblock);

Having reserved and mapped blocks, what happens when we try to read them? Do we actually get zeroes, or does the read go straight through to whatever happens to be in the disk blocks? I don't think it's correct that we could BDEV_RES_PROVISION and end up with stale credit card numbers from some other thin device.

Agree, but I'm not really sure how this works in thinp tbh. fallocate wasn't really on my mind when doing this. I was simply trying to cobble together what I could to facilitate making progress on the fs parts (e.g., I just needed a call that allocated blocks and consumed reservation in the process).

Skimming through the dm-thin code, it looks like a (configurable) block zeroing mechanism can be triggered from somewhere around provision_block()->schedule_zero(), depending on whether the incoming write overwrites the newly allocated block. If that's the case, then I suspect that means reads would just fall through to the block and return whatever was on disk. This code would probably need to tie into that zeroing mechanism one way or another to deal with that issue. (Though somebody who actually knows something about dm-thin should verify that. :)

Brian

Quoted 70 lines

(PS: I don't know enough about thinp to know if this has already been taken care of. I didn't see anything, but who knows what I missed. :))

--D

Quoted 65 lines

drivers/md/dm-thin.c +52 -0

+		if (error)
+			return error;
+
+		if (res && *res)
+			*res -= pool->sectors_per_block;
+next:
+		offset += pool->sectors_per_block;
+	}
+
+	return 0;
+}
+
+static int thin_reserve_space(struct dm_target *ti, int mode, sector_t offset,
+			      sector_t len, sector_t *res)
+{
+	struct thin_c *tc = ti->private;
+	struct pool *pool = tc->pool;
+	sector_t blocks;
+	unsigned long flags;
+	int error;
+
+	if (mode == BDEV_RES_PROVISION)
+		return thin_provision_space(ti, offset, len, res);
+
+	/* res required for get/set */
+	error = -EINVAL;
+	if (!res)
+		return error;
+
+	if (mode == BDEV_RES_GET) {
+		spin_lock_irqsave(&tc->pool->lock, flags);
+		*res = tc->reserve_count * pool->sectors_per_block;
+		spin_unlock_irqrestore(&tc->pool->lock, flags);
+		error = 0;
+	} else if (mode == BDEV_RES_MOD) {
+		/*
+		* @res must always be a factor of the pool's blocksize; upper
+		* layers can rely on the bdev's minimum_io_size for this.
+		*/
+		if (!is_factor(*res, pool->sectors_per_block))
+			return error;
+
+		blocks = *res;
+		(void) sector_div(blocks, pool->sectors_per_block);
+
+		error = set_reserve_count(tc, blocks);
+	}
+
+	return error;
+}
+
 static struct target_type thin_target = {
 	.name = "thin",
 	.version = {1, 18, 0},

@@ -4285,6 +4445,7 @@ static struct target_type thin_target = {
 	.status = thin_status,
 	.iterate_devices = thin_iterate_devices,
 	.io_hints = thin_io_hints,
+	.reserve_space = thin_reserve_space,
 };
 
 /*----------------------------------------------------------------*/

Signature · 1 line

2.4.11

Quoted 8 lines

Quoted 4 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

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

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

Quoted 88 lines · Brian Foster

Quoted 22 lines · Darrick J. Wong

Quoted 21 lines · Brian Foster

From: Joe Thornber <[email protected]>

Experimental reserve interface for XFS guys to play with.

I have big reservations (no pun intended) about this patch.

[BF:
- Support for reservation reduction.
- Support for space provisioning.
- Condensed to a single function.]

Not-Signed-off-by Joe Thornber <[email protected]>

Not-Signed-off-by Mike Snitzer <[email protected]>

---
 drivers/md/dm-thin.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 171 insertions(+), 10 deletions(-)

drivers/md/dm-thin.c +0 -0

index 92237b6..32bc5bd 100644

...

Quoted 33 lines

Quoted 33 lines

drivers/md/dm-thin.c +29 -0

@@ -4271,6 +4343,94 @@ static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits)
 	limits->max_discard_sectors = 2048 * 1024 * 16; /* 16G */
 }
 
+static int thin_provision_space(struct dm_target *ti, sector_t offset,
+				sector_t len, sector_t *res)
+{
+	struct thin_c *tc = ti->private;
+	struct pool *pool = tc->pool;
+	sector_t end;
+	dm_block_t pblock;
+	dm_block_t vblock;
+	int error;
+	struct dm_thin_lookup_result lookup;
+
+	if (!is_factor(offset, pool->sectors_per_block))
+		return -EINVAL;
+
+	if (!len || !is_factor(len, pool->sectors_per_block))
+		return -EINVAL;
+
+	if (res && !is_factor(*res, pool->sectors_per_block))
+		return -EINVAL;
+
+	end = offset + len;
+
+	while (offset < end) {
+		vblock = offset;
+		do_div(vblock, pool->sectors_per_block);
+
+		error = dm_thin_find_block(tc->td, vblock, true, &lookup);
+		if (error == 0)
+			goto next;

Quoted 9 lines

drivers/md/dm-thin.c static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits) · line 4378

Quoted 4 lines

drivers/md/dm-thin.c +4 -0

+		if (error != -ENODATA)
+			return error;
+
+		error = alloc_data_block(tc, &pblock);

So this means that if fallocate wants to BDEV_RES_PROVISION N blocks, it must first increase the reservation (BDEV_RES_MOD) by N blocks to avoid using up space that was previously reserved by some other caller. I think?

Yes, assuming this is being called from a filesystem using the reservation mechanism.

Quoted 11 lines

drivers/md/dm-thin.c static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits) · line 4382

Quoted 4 lines

drivers/md/dm-thin.c +4 -0

+		if (error)
+			return error;
+
+		error = dm_thin_insert_block(tc->td, vblock, pblock);

Having reserved and mapped blocks, what happens when we try to read them? Do we actually get zeroes, or does the read go straight through to whatever happens to be in the disk blocks? I don't think it's correct that we could BDEV_RES_PROVISION and end up with stale credit card numbers from some other thin device.

Agree, but I'm not really sure how this works in thinp tbh. fallocate wasn't really on my mind when doing this. I was simply trying to cobble together what I could to facilitate making progress on the fs parts (e.g., I just needed a call that allocated blocks and consumed reservation in the process).

Quoted 10 lines

Skimming through the dm-thin code, it looks like a (configurable) block zeroing mechanism can be triggered from somewhere around provision_block()->schedule_zero(), depending on whether the incoming write overwrites the newly allocated block. If that's the case, then I suspect that means reads would just fall through to the block and return whatever was on disk. This code would probably need to tie into that zeroing mechanism one way or another to deal with that issue. (Though somebody who actually knows something about dm-thin should verify that. :)

BTW, if that mechanism is in fact doing I/O, that might not be the appropriate solution for fallocate. Perhaps we'd have to consider an unwritten flag or some such in dm-thin, if possible.

Brian

Quoted 84 lines

Brian

Quoted 70 lines

(PS: I don't know enough about thinp to know if this has already been taken care of. I didn't see anything, but who knows what I missed. :))

--D

Quoted 65 lines

drivers/md/dm-thin.c +52 -0

+		if (error)
+			return error;
+
+		if (res && *res)
+			*res -= pool->sectors_per_block;
+next:
+		offset += pool->sectors_per_block;
+	}
+
+	return 0;
+}
+
+static int thin_reserve_space(struct dm_target *ti, int mode, sector_t offset,
+			      sector_t len, sector_t *res)
+{
+	struct thin_c *tc = ti->private;
+	struct pool *pool = tc->pool;
+	sector_t blocks;
+	unsigned long flags;
+	int error;
+
+	if (mode == BDEV_RES_PROVISION)
+		return thin_provision_space(ti, offset, len, res);
+
+	/* res required for get/set */
+	error = -EINVAL;
+	if (!res)
+		return error;
+
+	if (mode == BDEV_RES_GET) {
+		spin_lock_irqsave(&tc->pool->lock, flags);
+		*res = tc->reserve_count * pool->sectors_per_block;
+		spin_unlock_irqrestore(&tc->pool->lock, flags);
+		error = 0;
+	} else if (mode == BDEV_RES_MOD) {
+		/*
+		* @res must always be a factor of the pool's blocksize; upper
+		* layers can rely on the bdev's minimum_io_size for this.
+		*/
+		if (!is_factor(*res, pool->sectors_per_block))
+			return error;
+
+		blocks = *res;
+		(void) sector_div(blocks, pool->sectors_per_block);
+
+		error = set_reserve_count(tc, blocks);
+	}
+
+	return error;
+}
+
 static struct target_type thin_target = {
 	.name = "thin",
 	.version = {1, 18, 0},

@@ -4285,6 +4445,7 @@ static struct target_type thin_target = {
 	.status = thin_status,
 	.iterate_devices = thin_iterate_devices,
 	.io_hints = thin_io_hints,
+	.reserve_space = thin_reserve_space,
 };
 
 /*----------------------------------------------------------------*/

Signature · 1 line

2.4.11

Quoted 8 lines

Quoted 4 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

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

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

Quoted 67 lines · Brian Foster

Quoted 22 lines · Darrick J. Wong

Quoted 21 lines · Brian Foster

From: Joe Thornber <[email protected]>

Experimental reserve interface for XFS guys to play with.

I have big reservations (no pun intended) about this patch.

[BF:
- Support for reservation reduction.
- Support for space provisioning.
- Condensed to a single function.]

Not-Signed-off-by Joe Thornber <[email protected]>

Not-Signed-off-by Mike Snitzer <[email protected]>

---
 drivers/md/dm-thin.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 171 insertions(+), 10 deletions(-)

drivers/md/dm-thin.c +0 -0

index 92237b6..32bc5bd 100644

...

Quoted 42 lines

Quoted 27 lines

drivers/md/dm-thin.c +23 -0

@@ -4271,6 +4343,94 @@ static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits)
 	limits->max_discard_sectors = 2048 * 1024 * 16; /* 16G */
 }
 
+static int thin_provision_space(struct dm_target *ti, sector_t offset,
+				sector_t len, sector_t *res)
+{
+	struct thin_c *tc = ti->private;
+	struct pool *pool = tc->pool;
+	sector_t end;
+	dm_block_t pblock;
+	dm_block_t vblock;
+	int error;
+	struct dm_thin_lookup_result lookup;
+
+	if (!is_factor(offset, pool->sectors_per_block))
+		return -EINVAL;
+
+	if (!len || !is_factor(len, pool->sectors_per_block))
+		return -EINVAL;
+
+	if (res && !is_factor(*res, pool->sectors_per_block))
+		return -EINVAL;
+
+	end = offset + len;
+
+	while (offset < end) {

drivers/md/dm-thin.c static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits) · line 4378

Quoted 10 lines

drivers/md/dm-thin.c +10 -0

+		vblock = offset;
+		do_div(vblock, pool->sectors_per_block);
+
+		error = dm_thin_find_block(tc->td, vblock, true, &lookup);
+		if (error == 0)
+			goto next;
+		if (error != -ENODATA)
+			return error;
+
+		error = alloc_data_block(tc, &pblock);

So this means that if fallocate wants to BDEV_RES_PROVISION N blocks, it must first increase the reservation (BDEV_RES_MOD) by N blocks to avoid using up space that was previously reserved by some other caller. I think?

Quoted 2 lines

Yes, assuming this is being called from a filesystem using the reservation mechanism.

Quoted 18 lines

Quoted 11 lines

drivers/md/dm-thin.c static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits) · line 4382

Quoted 4 lines

drivers/md/dm-thin.c +4 -0

+		if (error)
+			return error;
+
+		error = dm_thin_insert_block(tc->td, vblock, pblock);

Having reserved and mapped blocks, what happens when we try to read them? Do we actually get zeroes, or does the read go straight through to whatever happens to be in the disk blocks? I don't think it's correct that we could BDEV_RES_PROVISION and end up with stale credit card numbers from some other thin device.

Agree, but I'm not really sure how this works in thinp tbh. fallocate wasn't really on my mind when doing this. I was simply trying to cobble together what I could to facilitate making progress on the fs parts (e.g., I just needed a call that allocated blocks and consumed reservation in the process).

Quoted 10 lines

Skimming through the dm-thin code, it looks like a (configurable) block zeroing mechanism can be triggered from somewhere around provision_block()->schedule_zero(), depending on whether the incoming write overwrites the newly allocated block. If that's the case, then I suspect that means reads would just fall through to the block and return whatever was on disk. This code would probably need to tie into that zeroing mechanism one way or another to deal with that issue. (Though somebody who actually knows something about dm-thin should verify that. :)

BTW, if that mechanism is in fact doing I/O, that might not be the appropriate solution for fallocate. Perhaps we'd have to consider an unwritten flag or some such in dm-thin, if possible.

Quoted 68 lines · Brian Foster

Quoted 67 lines · Brian Foster

Quoted 22 lines · Darrick J. Wong

Quoted 21 lines · Brian Foster

From: Joe Thornber <[email protected]>

Experimental reserve interface for XFS guys to play with.

I have big reservations (no pun intended) about this patch.

[BF:
- Support for reservation reduction.
- Support for space provisioning.
- Condensed to a single function.]

Not-Signed-off-by Joe Thornber <[email protected]>

Not-Signed-off-by Mike Snitzer <[email protected]>

---
 drivers/md/dm-thin.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 171 insertions(+), 10 deletions(-)

drivers/md/dm-thin.c +0 -0

index 92237b6..32bc5bd 100644

...

Quoted 42 lines

Quoted 27 lines

drivers/md/dm-thin.c +23 -0

@@ -4271,6 +4343,94 @@ static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits)
 	limits->max_discard_sectors = 2048 * 1024 * 16; /* 16G */
 }
 
+static int thin_provision_space(struct dm_target *ti, sector_t offset,
+				sector_t len, sector_t *res)
+{
+	struct thin_c *tc = ti->private;
+	struct pool *pool = tc->pool;
+	sector_t end;
+	dm_block_t pblock;
+	dm_block_t vblock;
+	int error;
+	struct dm_thin_lookup_result lookup;
+
+	if (!is_factor(offset, pool->sectors_per_block))
+		return -EINVAL;
+
+	if (!len || !is_factor(len, pool->sectors_per_block))
+		return -EINVAL;
+
+	if (res && !is_factor(*res, pool->sectors_per_block))
+		return -EINVAL;
+
+	end = offset + len;
+
+	while (offset < end) {

drivers/md/dm-thin.c static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits) · line 4378

Quoted 10 lines

drivers/md/dm-thin.c +10 -0

+		vblock = offset;
+		do_div(vblock, pool->sectors_per_block);
+
+		error = dm_thin_find_block(tc->td, vblock, true, &lookup);
+		if (error == 0)
+			goto next;
+		if (error != -ENODATA)
+			return error;
+
+		error = alloc_data_block(tc, &pblock);

So this means that if fallocate wants to BDEV_RES_PROVISION N blocks, it must first increase the reservation (BDEV_RES_MOD) by N blocks to avoid using up space that was previously reserved by some other caller. I think?

Quoted 2 lines

Quoted 2 lines

Yes, assuming this is being called from a filesystem using the reservation mechanism.

Brian, I need to circle back with you to understand why XFS even needs reservation as opposed to just using something like fallocate (which would provision the space before you actually initiate the IO that would use it). But we can discuss that in person and then report back to the list if it makes it easier...

Quoted 29 lines

Quoted 18 lines

Quoted 11 lines

drivers/md/dm-thin.c static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits) · line 4382

Quoted 4 lines

drivers/md/dm-thin.c +4 -0

+		if (error)
+			return error;
+
+		error = dm_thin_insert_block(tc->td, vblock, pblock);

Having reserved and mapped blocks, what happens when we try to read them? Do we actually get zeroes, or does the read go straight through to whatever happens to be in the disk blocks? I don't think it's correct that we could BDEV_RES_PROVISION and end up with stale credit card numbers from some other thin device.

Agree, but I'm not really sure how this works in thinp tbh. fallocate wasn't really on my mind when doing this. I was simply trying to cobble together what I could to facilitate making progress on the fs parts (e.g., I just needed a call that allocated blocks and consumed reservation in the process).

Quoted 10 lines

Skimming through the dm-thin code, it looks like a (configurable) block zeroing mechanism can be triggered from somewhere around provision_block()->schedule_zero(), depending on whether the incoming write overwrites the newly allocated block. If that's the case, then I suspect that means reads would just fall through to the block and return whatever was on disk. This code would probably need to tie into that zeroing mechanism one way or another to deal with that issue. (Though somebody who actually knows something about dm-thin should verify that. :)

Quoted 3 lines

BTW, if that mechanism is in fact doing I/O, that might not be the appropriate solution for fallocate. Perhaps we'd have to consider an unwritten flag or some such in dm-thin, if possible.

DM thinp defaults to enabling 'zero_new_blocks' (can be disabled using the 'skip_block_zeroing' feature when loading the DM table for the thin-pool). With block-zeroing any blocks that are provisioned _will_ be overwritten with zeroes (using dm-kcopyd which is trained to use WRITE_SAME if supported).

But yeah, for fallocate.. certainly not something we want as it defeats the point of fallocate being cheap.

So we probably would need a flag comparable to the ext4-stale-flag-that-shall-not-be-named ;)

Mike

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 7 lines · Mike Snitzer

Brian, I need to circle back with you to understand why XFS even needs reservation as opposed to just using something like fallocate (which would provision the space before you actually initiate the IO that would use it). But we can discuss that in person and then report back to the list if it makes it easier...

The primary reason is delayed allocation. Buffered writes to the fs copy data into the pagecache before the physical space has been allocated. E.g., we only modify the free blocks counters at write() time in order to guarantee that we have space somewhere in the fs. The physical extents aren't allocated until later at writeback time.

So reservation from dm-thin basically extends the mechanism to also guarantee that the underlying thin volume has space for writes that we've received but haven't written back yet.

Quoted 33 lines

Quoted 29 lines

Quoted 18 lines

Quoted 11 lines

drivers/md/dm-thin.c static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits) · line 4382

Quoted 4 lines

drivers/md/dm-thin.c +4 -0

+		if (error)
+			return error;
+
+		error = dm_thin_insert_block(tc->td, vblock, pblock);

Having reserved and mapped blocks, what happens when we try to read them? Do we actually get zeroes, or does the read go straight through to whatever happens to be in the disk blocks? I don't think it's correct that we could BDEV_RES_PROVISION and end up with stale credit card numbers from some other thin device.

Agree, but I'm not really sure how this works in thinp tbh. fallocate wasn't really on my mind when doing this. I was simply trying to cobble together what I could to facilitate making progress on the fs parts (e.g., I just needed a call that allocated blocks and consumed reservation in the process).

Quoted 10 lines

Skimming through the dm-thin code, it looks like a (configurable) block zeroing mechanism can be triggered from somewhere around provision_block()->schedule_zero(), depending on whether the incoming write overwrites the newly allocated block. If that's the case, then I suspect that means reads would just fall through to the block and return whatever was on disk. This code would probably need to tie into that zeroing mechanism one way or another to deal with that issue. (Though somebody who actually knows something about dm-thin should verify that. :)

Quoted 3 lines

BTW, if that mechanism is in fact doing I/O, that might not be the appropriate solution for fallocate. Perhaps we'd have to consider an unwritten flag or some such in dm-thin, if possible.

Quoted 6 lines

DM thinp defaults to enabling 'zero_new_blocks' (can be disabled using the 'skip_block_zeroing' feature when loading the DM table for the thin-pool). With block-zeroing any blocks that are provisioned _will_ be overwritten with zeroes (using dm-kcopyd which is trained to use WRITE_SAME if supported).

Ok, thanks.

Quoted 3 lines

But yeah, for fallocate.. certainly not something we want as it defeats the point of fallocate being cheap.

Indeed.

Quoted 3 lines

So we probably would need a flag comparable to the ext4-stale-flag-that-shall-not-be-named ;)

Any chance to support an unwritten flag for all blocks that are allocated via fallocate? E.g., subsequent reads detect the flag and return zeroes as if the block wasn't there and a subsequent write clears the flag (doing any partial block zeroing that might be necessary as well).

Quoted 15 lines · Brian Foster

Quoted 7 lines · Mike Snitzer

Brian, I need to circle back with you to understand why XFS even needs reservation as opposed to just using something like fallocate (which would provision the space before you actually initiate the IO that would use it). But we can discuss that in person and then report back to the list if it makes it easier...

The primary reason is delayed allocation. Buffered writes to the fs copy data into the pagecache before the physical space has been allocated. E.g., we only modify the free blocks counters at write() time in order to guarantee that we have space somewhere in the fs. The physical extents aren't allocated until later at writeback time.

Quoted 3 lines

So reservation from dm-thin basically extends the mechanism to also guarantee that the underlying thin volume has space for writes that we've received but haven't written back yet.

OK, so even if/when we have bdev_fallocate support that would be more rigid than XFS would like.

As you've said, the XFS established reservation is larger than is really needed. Whereas regularly provisioning more than is actually needed is a recipe for disaster.

Quoted 52 lines

Quoted 33 lines

Quoted 29 lines

Quoted 18 lines

Quoted 11 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+		if (error)
+			return error;
+
+		error = dm_thin_insert_block(tc->td, vblock, pblock);

Having reserved and mapped blocks, what happens when we try to read them? Do we actually get zeroes, or does the read go straight through to whatever happens to be in the disk blocks? I don't think it's correct that we could BDEV_RES_PROVISION and end up with stale credit card numbers from some other thin device.

Agree, but I'm not really sure how this works in thinp tbh. fallocate wasn't really on my mind when doing this. I was simply trying to cobble together what I could to facilitate making progress on the fs parts (e.g., I just needed a call that allocated blocks and consumed reservation in the process).

Quoted 10 lines

Skimming through the dm-thin code, it looks like a (configurable) block zeroing mechanism can be triggered from somewhere around provision_block()->schedule_zero(), depending on whether the incoming write overwrites the newly allocated block. If that's the case, then I suspect that means reads would just fall through to the block and return whatever was on disk. This code would probably need to tie into that zeroing mechanism one way or another to deal with that issue. (Though somebody who actually knows something about dm-thin should verify that. :)

Quoted 3 lines

BTW, if that mechanism is in fact doing I/O, that might not be the appropriate solution for fallocate. Perhaps we'd have to consider an unwritten flag or some such in dm-thin, if possible.

Quoted 6 lines

DM thinp defaults to enabling 'zero_new_blocks' (can be disabled using the 'skip_block_zeroing' feature when loading the DM table for the thin-pool). With block-zeroing any blocks that are provisioned _will_ be overwritten with zeroes (using dm-kcopyd which is trained to use WRITE_SAME if supported).

Ok, thanks.

Quoted 3 lines

But yeah, for fallocate.. certainly not something we want as it defeats the point of fallocate being cheap.

Indeed.

Quoted 3 lines

So we probably would need a flag comparable to the ext4-stale-flag-that-shall-not-be-named ;)

Quoted 5 lines

Any chance to support an unwritten flag for all blocks that are allocated via fallocate? E.g., subsequent reads detect the flag and return zeroes as if the block wasn't there and a subsequent write clears the flag (doing any partial block zeroing that might be necessary as well).

Yeah, I've already started talking to Joe about doing exactly that. Without it we cannot securely provide fallocate support in DM thinp.

I'll keep discussing with Joe... he doesn't like this requirement but we'll work through it.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Hi all,

This is v2 of the XFS and block device reservation experiment. The significant changes in v2 are that the bdev interface has been condensed to a single callback function, the XFS transaction reservation management has been reworked to make transactions responsible for tracking and releasing excess reservation (for non-delalloc cases) and a workaround for the fallocate over-reservation issue is included. Beyond that, this version adds a bunch of miscellaneous cleanups and fixes some of the nastier locking/leak issues present in the first rfc.

Patches 1-2 refactor some XFS reserve pool and block accounting code in preparation for subsequent patches. Patches 3-5 add block/device-mapper reservation support. Patches 6-10 add the core reservation infrastructure and management bits to XFS. See the link to the original rfc below for instructions and further details around the purpose of this series.

Finally, note that this is still highly experimental/theoretical and should not be used on production systems. Thoughts, reviews, flames appreciated.

Quoted 18 lines · Brian Foster

Hi all,

This is v2 of the XFS and block device reservation experiment. The significant changes in v2 are that the bdev interface has been condensed to a single callback function, the XFS transaction reservation management has been reworked to make transactions responsible for tracking and releasing excess reservation (for non-delalloc cases) and a workaround for the fallocate over-reservation issue is included. Beyond that, this version adds a bunch of miscellaneous cleanups and fixes some of the nastier locking/leak issues present in the first rfc.

Patches 1-2 refactor some XFS reserve pool and block accounting code in preparation for subsequent patches. Patches 3-5 add block/device-mapper reservation support. Patches 6-10 add the core reservation infrastructure and management bits to XFS. See the link to the original rfc below for instructions and further details around the purpose of this series.

Quoted 3 lines

Finally, note that this is still highly experimental/theoretical and should not be used on production systems. Thoughts, reviews, flames appreciated.

Thanks for carrying on with this work Brian.

I've started to review your patchset and Darrick's fallocate patchset. I've pushed a branch to linux-dm.git that combines the 2, see:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/log/?h=dm-fallocate

and then added this RFC patch, at the end, which relies on both of your patchsets -- you'll see blkdev_ensure_space_exists() has a FIXME which implies it isn't much more than simply stubbed out at this point (completely untested):

From: Mike Snitzer <[email protected]>
Date: Tue, 12 Apr 2016 15:54:31 -0400
Subject: [RFC PATCH] block: wire blkdev_fallocate() to block_device_operations' reserve_space

This effectively exposes the primitive for "ensure space exists". It relies on block_device_operations' reserve_space method.

Signed-off-by Mike Snitzer <[email protected]>

---
 block/blk-lib.c        | 26 ++++++++++++++++++++++++++
 fs/block_dev.c         | 20 +++++++++++---------
 include/linux/blkdev.h |  2 ++
 3 files changed, 39 insertions(+), 9 deletions(-)

block/blk-lib.c +14 -0

index 9dca6bb..5042a84 100644
@@ -314,3 +314,29 @@ int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
 	return __blkdev_issue_zeroout(bdev, sector, nr_sects, gfp_mask);
 }
 EXPORT_SYMBOL(blkdev_issue_zeroout);
+
+/**
+ * blkdev_ensure_space_exists - preallocate a block range
+ * @bdev:	blockdev to preallocate space for
+ * @sector:	start sector
+ * @nr_sects:	number of sectors to preallocate
+ * @gfp_mask:	memory allocation flags (for bio_alloc)
+ * @flags:	FALLOC_FL_* to control behaviour
+ *
+ * Description:
+ *    Ensure space exists, or is preallocated, for the sectors in question.
+ */
+int blkdev_ensure_space_exists(struct block_device *bdev, sector_t sector,
+		sector_t nr_sects, unsigned long flags)

block/blk-lib.c +2 -0

+}
+EXPORT_SYMBOL(blkdev_ensure_space_exists);

fs/block_dev.c +1 -5

index 5a2c3ab..b34c07b 100644
@@ -1801,17 +1801,13 @@ long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len)
 	struct request_queue *q = bdev_get_queue(bdev);
 	struct address_space *mapping;
 	loff_t end = start + len - 1;
-	loff_t bs_mask, isize;
+	loff_t isize;
 	int error;
 
 	/* We only support zero range and punch hole. */
 	if (mode & ~BLKDEV_FALLOC_FL_SUPPORTED)
 		return -EOPNOTSUPP;
 
-	/* We haven't a primitive for "ensure space exists" right now. */
-	if (!(mode & ~FALLOC_FL_KEEP_SIZE))
-		return -EOPNOTSUPP;
-
 	/* Only punch if the device can do zeroing discard. */
 	if ((mode & FALLOC_FL_PUNCH_HOLE) &&
 	    (!blk_queue_discard(q) || !q->limits.discard_zeroes_data))

@@ -1829,9 +1825,12 @@ long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len)
 			return -EINVAL;
 	}
 

fs/block_dev.c +0 -0

 		error = blkdev_issue_zeroout(bdev, start >> 9, len >> 9,
 					    GFP_KERNEL, false);
 	else if (mode & FALLOC_FL_PUNCH_HOLE)

include/linux/blkdev.h +2 -0

index 6c6ea96..4147af2 100644
@@ -1132,6 +1132,8 @@ extern int blkdev_issue_write_same(struct block_device *bdev, sector_t sector,
 		sector_t nr_sects, gfp_t gfp_mask, struct page *page);
 extern int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
 		sector_t nr_sects, gfp_t gfp_mask, bool discard);
+extern int blkdev_ensure_space_exists(struct block_device *bdev, sector_t sector,
+		sector_t nr_sects, unsigned long flags);
 static inline int sb_issue_discard(struct super_block *sb, sector_t block,
 		sector_t nr_blocks, gfp_t gfp_mask, unsigned long flags)
 {

Signature · 6 lines

2.6.4 (Apple Git-63)

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

Quoted 31 lines · Mike Snitzer

Quoted 18 lines · Brian Foster

Hi all,

This is v2 of the XFS and block device reservation experiment. The significant changes in v2 are that the bdev interface has been condensed to a single callback function, the XFS transaction reservation management has been reworked to make transactions responsible for tracking and releasing excess reservation (for non-delalloc cases) and a workaround for the fallocate over-reservation issue is included. Beyond that, this version adds a bunch of miscellaneous cleanups and fixes some of the nastier locking/leak issues present in the first rfc.

Patches 1-2 refactor some XFS reserve pool and block accounting code in preparation for subsequent patches. Patches 3-5 add block/device-mapper reservation support. Patches 6-10 add the core reservation infrastructure and management bits to XFS. See the link to the original rfc below for instructions and further details around the purpose of this series.

Quoted 3 lines

Finally, note that this is still highly experimental/theoretical and should not be used on production systems. Thoughts, reviews, flames appreciated.

Thanks for carrying on with this work Brian.

I've started to review your patchset and Darrick's fallocate patchset. I've pushed a branch to linux-dm.git that combines the 2, see:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/log/?h=dm-fallocate

Quoted 4 lines

and then added this RFC patch, at the end, which relies on both of your patchsets -- you'll see blkdev_ensure_space_exists() has a FIXME which implies it isn't much more than simply stubbed out at this point (completely untested):

Hmm, ok, but -rc3 broke a bunch of stuff. Guess I should repost with all the PAGE_CACHE_ -> PAGE_ stuff fixed. :)

Quoted 36 lines

From: Mike Snitzer <[email protected]>
Date: Tue, 12 Apr 2016 15:54:31 -0400
Subject: [RFC PATCH] block: wire blkdev_fallocate() to block_device_operations' reserve_space

This effectively exposes the primitive for "ensure space exists". It relies on block_device_operations' reserve_space method.

Signed-off-by Mike Snitzer <[email protected]>

---
 block/blk-lib.c        | 26 ++++++++++++++++++++++++++
 fs/block_dev.c         | 20 +++++++++++---------
 include/linux/blkdev.h |  2 ++
 3 files changed, 39 insertions(+), 9 deletions(-)

block/blk-lib.c +14 -0

index 9dca6bb..5042a84 100644
@@ -314,3 +314,29 @@ int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
 	return __blkdev_issue_zeroout(bdev, sector, nr_sects, gfp_mask);
 }
 EXPORT_SYMBOL(blkdev_issue_zeroout);
+
+/**
+ * blkdev_ensure_space_exists - preallocate a block range
+ * @bdev:	blockdev to preallocate space for
+ * @sector:	start sector
+ * @nr_sects:	number of sectors to preallocate
+ * @gfp_mask:	memory allocation flags (for bio_alloc)
+ * @flags:	FALLOC_FL_* to control behaviour
+ *
+ * Description:
+ *    Ensure space exists, or is preallocated, for the sectors in question.
+ */
+int blkdev_ensure_space_exists(struct block_device *bdev, sector_t sector,
+		sector_t nr_sects, unsigned long flags)

block/blk-lib.c int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector, · line 340

Quoted 10 lines

block/blk-lib.c +10 -0

+{
+	sector_t res;
+	const struct block_device_operations *ops = bdev->bd_disk->fops;
+
+	if (!ops->reserve_space)
+		return -EOPNOTSUPP;
+
+	// FIXME: check with Brian Foster on whether it makes sense to
+	// use BDEV_RES_GET/BDEV_RES_MOD instead of BDEV_RES_PROVISION?
+	return ops->reserve_space(bdev, BDEV_RES_PROVISION, sector, nr_sects, &res);

/me thinks BDEV_RES_PROVISION is correct here, because regular-mode file fallocate (for ext4/xfs anyway) allocates blocks and maps them to specific file offsets as unwritten extents. afaict RES_PROVISION -> thin_provision_space() and thin_provision_space() seems to allocate blocks and map them to the device's LBAs.

If I'm reading the patches correctly, RES_GET/RES_MOD seem to reserve N blocks but doesn't map them to any specific LBA.

Quoted 29 lines

block/blk-lib.c +2 -0

+}
+EXPORT_SYMBOL(blkdev_ensure_space_exists);

fs/block_dev.c +1 -5

index 5a2c3ab..b34c07b 100644
@@ -1801,17 +1801,13 @@ long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len)
 	struct request_queue *q = bdev_get_queue(bdev);
 	struct address_space *mapping;
 	loff_t end = start + len - 1;
-	loff_t bs_mask, isize;
+	loff_t isize;
 	int error;
 
 	/* We only support zero range and punch hole. */
 	if (mode & ~BLKDEV_FALLOC_FL_SUPPORTED)
 		return -EOPNOTSUPP;
 
-	/* We haven't a primitive for "ensure space exists" right now. */
-	if (!(mode & ~FALLOC_FL_KEEP_SIZE))
-		return -EOPNOTSUPP;
-
 	/* Only punch if the device can do zeroing discard. */
 	if ((mode & FALLOC_FL_PUNCH_HOLE) &&
 	    (!blk_queue_discard(q) || !q->limits.discard_zeroes_data))

@@ -1829,9 +1825,12 @@ long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len)
 			return -EINVAL;
 	}
 

fs/block_dev.c long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len) · line 1834

Quoted 10 lines

fs/block_dev.c +6 -3

-	/* Don't allow IO that isn't aligned to logical block size */
-	bs_mask = bdev_logical_block_size(bdev) - 1;
-	if ((start | len) & bs_mask)
+	/*
+	 * Don't allow IO that isn't aligned to minimum IO size (io_min)
+	 * - for normal device's io_min is usually logical block size
+	 * - but for more exotic devices (e.g. DM thinp) it may be larger
+	 */
+	if ((start | len) % bdev_io_min(bdev))
 		return -EINVAL;

Noted. Will update the original patch.

fs/block_dev.c long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len) · line 1844

Quoted 10 lines

fs/block_dev.c +4 -1

 	/* Invalidate the page cache, including dirty pages. */

@@ -1839,7 +1838,10 @@ long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len)
 	truncate_inode_pages_range(mapping, start, end);
 
 	error = -EINVAL;
-	if (mode & FALLOC_FL_ZERO_RANGE)
+	if (!(mode & ~FALLOC_FL_KEEP_SIZE))
+		error = blkdev_ensure_space_exists(bdev, start >> 9, len >> 9,
+						   mode);
+	else if (mode & FALLOC_FL_ZERO_RANGE)

This whole thing got converted to a switch statement due to some feedback from hch.

Anyway, will try to have a new blockdev fallocate patchset done by the end of the day.

(Is there a test case for this?)

Quoted 32 lines · Darrick J. Wong

Quoted 31 lines · Mike Snitzer

Quoted 18 lines · Brian Foster

Hi all,

This is v2 of the XFS and block device reservation experiment. The significant changes in v2 are that the bdev interface has been condensed to a single callback function, the XFS transaction reservation management has been reworked to make transactions responsible for tracking and releasing excess reservation (for non-delalloc cases) and a workaround for the fallocate over-reservation issue is included. Beyond that, this version adds a bunch of miscellaneous cleanups and fixes some of the nastier locking/leak issues present in the first rfc.

Patches 1-2 refactor some XFS reserve pool and block accounting code in preparation for subsequent patches. Patches 3-5 add block/device-mapper reservation support. Patches 6-10 add the core reservation infrastructure and management bits to XFS. See the link to the original rfc below for instructions and further details around the purpose of this series.

Quoted 3 lines

Finally, note that this is still highly experimental/theoretical and should not be used on production systems. Thoughts, reviews, flames appreciated.

Thanks for carrying on with this work Brian.

I've started to review your patchset and Darrick's fallocate patchset. I've pushed a branch to linux-dm.git that combines the 2, see:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/log/?h=dm-fallocate

Quoted 7 lines

Quoted 4 lines

and then added this RFC patch, at the end, which relies on both of your patchsets -- you'll see blkdev_ensure_space_exists() has a FIXME which implies it isn't much more than simply stubbed out at this point (completely untested):

Hmm, ok, but -rc3 broke a bunch of stuff. Guess I should repost with all the PAGE_CACHE_ -> PAGE_ stuff fixed. :)

Yeah, the kernel.org kbuild robots just spammed us about that same exact breakage.

Quoted 53 lines

Quoted 36 lines

From: Mike Snitzer <[email protected]>
Date: Tue, 12 Apr 2016 15:54:31 -0400
Subject: [RFC PATCH] block: wire blkdev_fallocate() to block_device_operations' reserve_space

This effectively exposes the primitive for "ensure space exists". It relies on block_device_operations' reserve_space method.

Signed-off-by Mike Snitzer <[email protected]>

---
 block/blk-lib.c        | 26 ++++++++++++++++++++++++++
 fs/block_dev.c         | 20 +++++++++++---------
 include/linux/blkdev.h |  2 ++
 3 files changed, 39 insertions(+), 9 deletions(-)

block/blk-lib.c +14 -0

index 9dca6bb..5042a84 100644
@@ -314,3 +314,29 @@ int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
 	return __blkdev_issue_zeroout(bdev, sector, nr_sects, gfp_mask);
 }
 EXPORT_SYMBOL(blkdev_issue_zeroout);
+
+/**
+ * blkdev_ensure_space_exists - preallocate a block range
+ * @bdev:	blockdev to preallocate space for
+ * @sector:	start sector
+ * @nr_sects:	number of sectors to preallocate
+ * @gfp_mask:	memory allocation flags (for bio_alloc)
+ * @flags:	FALLOC_FL_* to control behaviour
+ *
+ * Description:
+ *    Ensure space exists, or is preallocated, for the sectors in question.
+ */
+int blkdev_ensure_space_exists(struct block_device *bdev, sector_t sector,
+		sector_t nr_sects, unsigned long flags)

block/blk-lib.c int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector, · line 340

Quoted 10 lines

block/blk-lib.c +10 -0

+{
+	sector_t res;
+	const struct block_device_operations *ops = bdev->bd_disk->fops;
+
+	if (!ops->reserve_space)
+		return -EOPNOTSUPP;
+
+	// FIXME: check with Brian Foster on whether it makes sense to
+	// use BDEV_RES_GET/BDEV_RES_MOD instead of BDEV_RES_PROVISION?
+	return ops->reserve_space(bdev, BDEV_RES_PROVISION, sector, nr_sects, &res);

/me thinks BDEV_RES_PROVISION is correct here, because regular-mode file fallocate (for ext4/xfs anyway) allocates blocks and maps them to specific file offsets as unwritten extents. afaict RES_PROVISION -> thin_provision_space() and thin_provision_space() seems to allocate blocks and map them to the device's LBAs.

Quoted 2 lines

If I'm reading the patches correctly, RES_GET/RES_MOD seem to reserve N blocks but doesn't map them to any specific LBA.

Right that is how I read it too. I just put that FIXME in to cover my ass incase I was being an idiot ;)

Quoted 29 lines

Quoted 29 lines

block/blk-lib.c +2 -0

+}
+EXPORT_SYMBOL(blkdev_ensure_space_exists);

fs/block_dev.c +1 -5

index 5a2c3ab..b34c07b 100644
@@ -1801,17 +1801,13 @@ long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len)
 	struct request_queue *q = bdev_get_queue(bdev);
 	struct address_space *mapping;
 	loff_t end = start + len - 1;
-	loff_t bs_mask, isize;
+	loff_t isize;
 	int error;
 
 	/* We only support zero range and punch hole. */
 	if (mode & ~BLKDEV_FALLOC_FL_SUPPORTED)
 		return -EOPNOTSUPP;
 
-	/* We haven't a primitive for "ensure space exists" right now. */
-	if (!(mode & ~FALLOC_FL_KEEP_SIZE))
-		return -EOPNOTSUPP;
-
 	/* Only punch if the device can do zeroing discard. */
 	if ((mode & FALLOC_FL_PUNCH_HOLE) &&
 	    (!blk_queue_discard(q) || !q->limits.discard_zeroes_data))

@@ -1829,9 +1825,12 @@ long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len)
 			return -EINVAL;
 	}
 

Quoted 12 lines

fs/block_dev.c long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len) · line 1834

Quoted 10 lines

fs/block_dev.c +6 -3

-	/* Don't allow IO that isn't aligned to logical block size */
-	bs_mask = bdev_logical_block_size(bdev) - 1;
-	if ((start | len) & bs_mask)
+	/*
+	 * Don't allow IO that isn't aligned to minimum IO size (io_min)
+	 * - for normal device's io_min is usually logical block size
+	 * - but for more exotic devices (e.g. DM thinp) it may be larger
+	 */
+	if ((start | len) % bdev_io_min(bdev))
 		return -EINVAL;

Noted. Will update the original patch.

OK, thanks.

Once your new patchset is available I'll rebase my 'dm-fallocate' test branch accordingly.

Quoted 14 lines

fs/block_dev.c long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len) · line 1844

Quoted 10 lines

fs/block_dev.c +4 -1

 	/* Invalidate the page cache, including dirty pages. */

@@ -1839,7 +1838,10 @@ long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len)
 	truncate_inode_pages_range(mapping, start, end);
 
 	error = -EINVAL;
-	if (mode & FALLOC_FL_ZERO_RANGE)
+	if (!(mode & ~FALLOC_FL_KEEP_SIZE))
+		error = blkdev_ensure_space_exists(bdev, start >> 9, len >> 9,
+						   mode);
+	else if (mode & FALLOC_FL_ZERO_RANGE)

This whole thing got converted to a switch statement due to some feedback from hch.

Quoted 4 lines

Anyway, will try to have a new blockdev fallocate patchset done by the end of the day.

(Is there a test case for this?)

No, but once my patch is in place to join your patchset with Brian's then any basic fallocate tests against a DM thinp volume _should_ work.

/me assumes xfstests has such tests? Only missing bit would be to layer the filesystem ontop of DM thinp? Or extend the tests your added to test DM thinp devices directly. I think Eric Sandeen (now cc'd) made xfstests capable or creating DM thinp volumes for certain tests.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 27 lines · Mike Snitzer

fs/block_dev.c +1 -5

index 5a2c3ab..b34c07b 100644
@@ -1801,17 +1801,13 @@ long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len)
 	struct request_queue *q = bdev_get_queue(bdev);
 	struct address_space *mapping;
 	loff_t end = start + len - 1;
-	loff_t bs_mask, isize;
+	loff_t isize;
 	int error;
 
 	/* We only support zero range and punch hole. */
 	if (mode & ~BLKDEV_FALLOC_FL_SUPPORTED)
 		return -EOPNOTSUPP;
 
-	/* We haven't a primitive for "ensure space exists" right now. */
-	if (!(mode & ~FALLOC_FL_KEEP_SIZE))
-		return -EOPNOTSUPP;
-
 	/* Only punch if the device can do zeroing discard. */
 	if ((mode & FALLOC_FL_PUNCH_HOLE) &&
 	    (!blk_queue_discard(q) || !q->limits.discard_zeroes_data))

@@ -1829,9 +1825,12 @@ long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len)
 			return -EINVAL;
 	}
 

fs/block_dev.c long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len) · line 1834

Quoted 10 lines

fs/block_dev.c +6 -3

-	/* Don't allow IO that isn't aligned to logical block size */
-	bs_mask = bdev_logical_block_size(bdev) - 1;
-	if ((start | len) & bs_mask)
+	/*
+	 * Don't allow IO that isn't aligned to minimum IO size (io_min)
+	 * - for normal device's io_min is usually logical block size
+	 * - but for more exotic devices (e.g. DM thinp) it may be larger
+	 */
+	if ((start | len) % bdev_io_min(bdev))
 		return -EINVAL;

Noted. Will update the original patch.

Quoted 28 lines · Darrick J. Wong

Quoted 27 lines · Mike Snitzer

fs/block_dev.c +1 -5

index 5a2c3ab..b34c07b 100644
@@ -1801,17 +1801,13 @@ long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len)
 	struct request_queue *q = bdev_get_queue(bdev);
 	struct address_space *mapping;
 	loff_t end = start + len - 1;
-	loff_t bs_mask, isize;
+	loff_t isize;
 	int error;
 
 	/* We only support zero range and punch hole. */
 	if (mode & ~BLKDEV_FALLOC_FL_SUPPORTED)
 		return -EOPNOTSUPP;
 
-	/* We haven't a primitive for "ensure space exists" right now. */
-	if (!(mode & ~FALLOC_FL_KEEP_SIZE))
-		return -EOPNOTSUPP;
-
 	/* Only punch if the device can do zeroing discard. */
 	if ((mode & FALLOC_FL_PUNCH_HOLE) &&
 	    (!blk_queue_discard(q) || !q->limits.discard_zeroes_data))

@@ -1829,9 +1825,12 @@ long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len)
 			return -EINVAL;
 	}
 

Quoted 9 lines

fs/block_dev.c long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len) · line 1833

Quoted 9 lines

fs/block_dev.c +6 -3

-	/* Don't allow IO that isn't aligned to logical block size */
-	bs_mask = bdev_logical_block_size(bdev) - 1;
-	if ((start | len) & bs_mask)
+	/*
+	 * Don't allow IO that isn't aligned to minimum IO size (io_min)
+	 * - for normal device's io_min is usually logical block size
+	 * - but for more exotic devices (e.g. DM thinp) it may be larger
+	 */
+	if ((start | len) % bdev_io_min(bdev))

Quoted 3 lines

fs/block_dev.c long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len) · line 1834

Quoted 1 line

fs/block_dev.c +0 -0

 		return -EINVAL;

Noted. Will update the original patch.

BTW, I just noticed your "block: require write_same and discard requests align to logical block size" -- doesn't look right.

But maybe I'm just too hyper-focused on DM thinp's needs (which would much prefer these checks be done in terms of minimum_io_size, rather than logical_block_size, and _not_ assuming power-of-2 math will work).

But at least for discard: your lbs-based check is fine; since we have discard_granularity to cover thinp's more specific requirements.

Quoted 31 lines · Mike Snitzer

Quoted 28 lines · Darrick J. Wong

Quoted 27 lines · Mike Snitzer

fs/block_dev.c +1 -5

index 5a2c3ab..b34c07b 100644
@@ -1801,17 +1801,13 @@ long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len)
 	struct request_queue *q = bdev_get_queue(bdev);
 	struct address_space *mapping;
 	loff_t end = start + len - 1;
-	loff_t bs_mask, isize;
+	loff_t isize;
 	int error;
 
 	/* We only support zero range and punch hole. */
 	if (mode & ~BLKDEV_FALLOC_FL_SUPPORTED)
 		return -EOPNOTSUPP;
 
-	/* We haven't a primitive for "ensure space exists" right now. */
-	if (!(mode & ~FALLOC_FL_KEEP_SIZE))
-		return -EOPNOTSUPP;
-
 	/* Only punch if the device can do zeroing discard. */
 	if ((mode & FALLOC_FL_PUNCH_HOLE) &&
 	    (!blk_queue_discard(q) || !q->limits.discard_zeroes_data))

@@ -1829,9 +1825,12 @@ long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len)
 			return -EINVAL;
 	}
 

Quoted 9 lines

Quoted 9 lines

fs/block_dev.c long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len) · line 1833

Quoted 9 lines

fs/block_dev.c +6 -3

-	/* Don't allow IO that isn't aligned to logical block size */
-	bs_mask = bdev_logical_block_size(bdev) - 1;
-	if ((start | len) & bs_mask)
+	/*
+	 * Don't allow IO that isn't aligned to minimum IO size (io_min)
+	 * - for normal device's io_min is usually logical block size
+	 * - but for more exotic devices (e.g. DM thinp) it may be larger
+	 */
+	if ((start | len) % bdev_io_min(bdev))

I started by noticing the 64-bit division.

However, in researching alignment
requirements for fallocate, I noticed that nothing says that we can return -EINVAL for unaligned offset/len for allocate or punch. For file allocations ext4 and xfs simply enlarge the range so that the ends are aligned to the logical block size; for punch they both shrink the range to deallocate until the ends are aligned, and write zeroes to the partial blocks.

At least for user-visible fallocate we should do likewise, but for the internal blkdev_ helpers I think it makes more sense to check lbs alignment and let the lower level driver reject the IO if min_io alignment is a hard requirement. Documentation/block/queue-sysfs.txt says that the min_io is the smallest /preferred/ size.

But, before that, I'll push out some new fallocate patches for -rc3.

Quoted 6 lines

Quoted 3 lines

fs/block_dev.c long blkdev_fallocate(struct file *file, int mode, loff_t start, loff_t len) · line 1834

Quoted 1 line

fs/block_dev.c +0 -0

 		return -EINVAL;

Noted. Will update the original patch.

BTW, I just noticed your "block: require write_same and discard requests align to logical block size" -- doesn't look right.

What happens if we pass a request to thinp that isn't aligned to minimum_io_size? Does it reject the command?