rbd: RBD_DEV_FLAG_THICK rbd_dev_flags bit

PATCH In review KAMEI Hitoshi 2 participants 8 messages Mar 19, 2018 – Mar 27, 2018 1 message not in the archive

Subsystems (2)

Block layer RADOS block device (RBD)

lore

Share & tools

lore

KAMEI Hitoshi

author

2018-03-19 12:01 UTC #0 lore raw

PATCH 1/2 rbd: RBD_DEV_FLAG_THICK rbd_dev_flags bit

This patch adds a user interface to prevent from issuing discard requests onto the specified rbd device. This is needed for thick-provisioned images. To avoid discarding allocated blocks on thick-provision image, users can specify this flag bit via sysfs (/sys/bus/rbd/devices/<dev-id>/thick).

When users write "1" to the file, rbd doesn't issue discard operation; meanwhile, if users write "0" to the file then rbd issues discard operation.

Signed-off-by Hitoshi Kamei <[email protected]>

Cc Mitsuo Hayasaka <[email protected]>

---
 drivers/block/rbd.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

drivers/block/rbd.c +38 -0

index 8e40da093766..dba60ef43a47 100644
@@ -427,6 +427,7 @@ enum rbd_dev_flags {
 	RBD_DEV_FLAG_EXISTS,	/* mapped snapshot has not been deleted */
 	RBD_DEV_FLAG_REMOVING,	/* this mapping is being removed */
 	RBD_DEV_FLAG_BLACKLISTED, /* our ceph_client is blacklisted */
+	RBD_DEV_FLAG_THICK,	/* image is thick-provisioned */
 };
 
 static DEFINE_MUTEX(client_mutex);	/* Serialize client creation */

@@ -4011,6 +4012,15 @@ static void rbd_queue_workfn(struct work_struct *work)
 		goto err;
 	}
 
+	/* Ignore/skip discard requests for thick-provision image */
+
+	if (op_type == OBJ_OP_DISCARD &&
+	    test_bit(RBD_DEV_FLAG_THICK, &rbd_dev->flags)) {
+		dout("%s: Ignored a discard request\n", __func__);
+		result = 0;
+		goto err_rq;
+	}
+
 	/* Ignore/skip any zero-length requests */
 
 	if (!length) {

@@ -4600,6 +4610,32 @@ static ssize_t rbd_image_refresh(struct device *dev,
 	return size;
 }
 
+static ssize_t rbd_thick_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf,
+				 size_t size)
+{
+	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
+
+	spin_lock_irq(&rbd_dev->lock);
+	if (!strncmp(buf, "1", size))
+		set_bit(RBD_DEV_FLAG_THICK, &rbd_dev->flags);
+	else if (!strncmp(buf, "0", size))
+		clear_bit(RBD_DEV_FLAG_THICK, &rbd_dev->flags);
+	spin_unlock_irq(&rbd_dev->lock);
+
+	return size;
+}
+
+static ssize_t rbd_thick_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
+	int is_thick = test_bit(RBD_DEV_FLAG_THICK, &rbd_dev->flags) ? 1 : 0;
+
+	return sprintf(buf, "%d\n", is_thick);
+}
+
 static DEVICE_ATTR(size, S_IRUGO, rbd_size_show, NULL);
 static DEVICE_ATTR(features, S_IRUGO, rbd_features_show, NULL);
 static DEVICE_ATTR(major, S_IRUGO, rbd_major_show, NULL);

@@ -4616,6 +4652,7 @@ static DEVICE_ATTR(refresh, S_IWUSR, NULL, rbd_image_refresh);
 static DEVICE_ATTR(current_snap, S_IRUGO, rbd_snap_show, NULL);
 static DEVICE_ATTR(snap_id, S_IRUGO, rbd_snap_id_show, NULL);
 static DEVICE_ATTR(parent, S_IRUGO, rbd_parent_show, NULL);
+static DEVICE_ATTR(thick, 0644, rbd_thick_show, rbd_thick_store);
 
 static struct attribute *rbd_attrs[] = {
 	&dev_attr_size.attr,

@@ -4634,6 +4671,7 @@ static struct attribute *rbd_attrs[] = {
 	&dev_attr_snap_id.attr,
 	&dev_attr_parent.attr,
 	&dev_attr_refresh.attr,
+	&dev_attr_thick.attr,
 	NULL
 };

Signature · 1 line

2.15.1

亀井仁志 / KAMEI，HITOSHI

author

2018-03-22 11:57 UTC #2 lore raw

In reply to a message not in the archive

Hi Yang,

Quoted 1 line

I am not sure is this the best way for this case, what about adding an option in "rbd map -o thick rbd/test"?

I will add such option to the rbd map command to manipulate image settings. So, the end-user do not change the settings directly via sysfs file.

Quoted 7 lines

	@@ -4011,6 +4012,15 @@ static void rbd_queue_workfn(struct work_struct *work)
	 		goto err;
	 	}
	
	+	/* Ignore/skip discard requests for thick-provision image */

Just ignore? or return -EOPNOTSUPP?

Thanks, I think -EOPNOTSUPP is better because user programs cannot know the result of requested operation when the kernel rbd driver ignores discard request. The result of requested operation when the kernel rbd driver ignores discard requests, which probably misleads the user programs.

Quoted 1 line

In addition, we should not ignore the REQ_OP_WRITE_ZEROES.

Relating to the above, the return code of REQ_OP_WRITE_ZEROS request is also -EOPNOTSUPP instead of ignoring. I think the result of -EOPNOTSUPP is also better for this request because the kernel rbd driver can expect that user programs write zero data by itself.

Quoted 6 lines

	+	spin_lock_irq(&rbd_dev->lock);
	+	if (!strncmp(buf, "1", size))
	+		set_bit(RBD_DEV_FLAG_THICK, &rbd_dev->flags);
	+	else if (!strncmp(buf, "0", size))
	+		clear_bit(RBD_DEV_FLAG_THICK, &rbd_dev->flags);

else ?

I will add the warning message in else case.

Quoted 4 lines

	 static DEVICE_ATTR(snap_id, S_IRUGO, rbd_snap_id_show, NULL);
	 static DEVICE_ATTR(parent, S_IRUGO, rbd_parent_show, NULL);
	+static DEVICE_ATTR(thick, 0644, rbd_thick_show, rbd_thick_store);

maybe S_IRUGO | S_IWUGO?

I will change the code by using the flags.

Regards,

Signature · 1 line

Hitoshi Kamei

Ilya Dryomov

maintainer

2018-03-23 09:31 UTC #3 lore raw

Quoted 20 lines · 亀井仁志 / KAMEI，HITOSHI

Hi Yang,
Quoted 1 line
I am not sure is this the best way for this case, what about adding an option in "rbd map -o thick rbd/test"?
I will add such option to the rbd map command to manipulate image settings. So, the end-user do not change the settings directly via sysfs file.
Quoted 7 lines
      @@ -4011,6 +4012,15 @@ static void rbd_queue_workfn(struct work_struct *work)
                      goto err;
              }

      +       /* Ignore/skip discard requests for thick-provision image */
Just ignore? or return -EOPNOTSUPP?
Thanks, I think -EOPNOTSUPP is better because user programs cannot know the result of requested operation when the kernel rbd driver ignores discard request. The result of requested operation when the kernel rbd driver ignores discard requests, which probably misleads the user programs.

Quoted 6 lines

Quoted 1 line
In addition, we should not ignore the REQ_OP_WRITE_ZEROES.
Relating to the above, the return code of REQ_OP_WRITE_ZEROS request is also -EOPNOTSUPP instead of ignoring. I think the result of -EOPNOTSUPP is also better for this request because the kernel rbd driver can expect that user programs write zero data by itself.

REQ_OP_WRITE_ZEROES should continue to work, we just need to make sure it never issues truncates or deletes and instead writes zeroes explicitly.

I think we should be explicit about the fact that discard is not supported instead of accepting the discard request and failing it in rbd_queue_workfn(). Attached patch is what I have in mind.

Thanks,

Ilya

Ilya Dryomov

maintainer

2018-03-23 09:34 UTC #4 lore raw

Quoted 29 lines · Ilya Dryomov

Quoted 20 lines · 亀井仁志 / KAMEI，HITOSHI
Hi Yang,
Quoted 1 line
I am not sure is this the best way for this case, what about adding an option in "rbd map -o thick rbd/test"?
I will add such option to the rbd map command to manipulate image settings. So, the end-user do not change the settings directly via sysfs file.
Quoted 7 lines
      @@ -4011,6 +4012,15 @@ static void rbd_queue_workfn(struct work_struct *work)
                      goto err;
              }

      +       /* Ignore/skip discard requests for thick-provision image */
Just ignore? or return -EOPNOTSUPP?
Thanks, I think -EOPNOTSUPP is better because user programs cannot know the result of requested operation when the kernel rbd driver ignores discard request. The result of requested operation when the kernel rbd driver ignores discard requests, which probably misleads the user programs.
Quoted 6 lines
Quoted 1 line
In addition, we should not ignore the REQ_OP_WRITE_ZEROES.
Relating to the above, the return code of REQ_OP_WRITE_ZEROS request is also -EOPNOTSUPP instead of ignoring. I think the result of -EOPNOTSUPP is also better for this request because the kernel rbd driver can expect that user programs write zero data by itself.

Quoted 7 lines

REQ_OP_WRITE_ZEROES should continue to work, we just need to make sure it never issues truncates or deletes and instead writes zeroes explicitly.
I think we should be explicit about the fact that discard is not supported instead of accepting the discard request and failing it in rbd_queue_workfn(). Attached patch is what I have in mind.

Now with the patch.

Thanks,

Ilya

From 1db405366371cb12e5182815c06f6f491af4b63c Mon Sep 17 00:00:00 2001
From: Ilya Dryomov <[email protected]>
Date: Fri, 23 Mar 2018 09:14:47 +0100
Subject: [PATCH] rbd: notrim map option

Add an option to turn off discard and write zeroes offload support to avoid deprovisioning a fully provisioned image. When enabled, discard requests will fail with -EOPNOTSUPP, write zeroes requests will fall back to manually zeroing.

Signed-off-by Ilya Dryomov <[email protected]>

---
 drivers/block/rbd.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

drivers/block/rbd.c +14 -5

index 1a90e5dd7470..dbe440a6531e 100644
@@ -733,6 +733,7 @@ enum {
 	Opt_read_write,
 	Opt_lock_on_read,
 	Opt_exclusive,
+	Opt_notrim,
 	Opt_err
 };

@@ -746,6 +747,7 @@ static match_table_t rbd_opts_tokens = {
 	{Opt_read_write, "rw"},		/* Alternate spelling */
 	{Opt_lock_on_read, "lock_on_read"},
 	{Opt_exclusive, "exclusive"},
+	{Opt_notrim, "notrim"},
 	{Opt_err, NULL}
 };

@@ -754,12 +756,14 @@ struct rbd_options {
 	bool	read_only;
 	bool	lock_on_read;
 	bool	exclusive;
+	bool	trim;
 };
 
 #define RBD_QUEUE_DEPTH_DEFAULT	BLKDEV_MAX_RQ
 #define RBD_READ_ONLY_DEFAULT	false
 #define RBD_LOCK_ON_READ_DEFAULT false
 #define RBD_EXCLUSIVE_DEFAULT	false
+#define RBD_TRIM_DEFAULT	true
 
 static int parse_rbd_opts_token(char *c, void *private)
 {

@@ -801,6 +805,9 @@ static int parse_rbd_opts_token(char *c, void *private)
 	case Opt_exclusive:
 		rbd_opts->exclusive = true;
 		break;
+	case Opt_notrim:
+		rbd_opts->trim = false;
+		break;
 	default:
 		/* libceph prints "bad option" msg */
 		return -EINVAL;

@@ -3940,11 +3947,12 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 	blk_queue_io_min(q, objset_bytes);
 	blk_queue_io_opt(q, objset_bytes);
 
-	/* enable the discard support */
-	queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
-	q->limits.discard_granularity = objset_bytes;
-	blk_queue_max_discard_sectors(q, objset_bytes >> SECTOR_SHIFT);
-	blk_queue_max_write_zeroes_sectors(q, objset_bytes >> SECTOR_SHIFT);
+	if (rbd_dev->opts->trim) {
+		queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
+		q->limits.discard_granularity = objset_bytes;
+		blk_queue_max_discard_sectors(q, objset_bytes >> SECTOR_SHIFT);
+		blk_queue_max_write_zeroes_sectors(q, objset_bytes >> SECTOR_SHIFT);
+	}
 
 	if (!ceph_test_opt(rbd_dev->rbd_client->client, NOCRC))
 		q->backing_dev_info->capabilities |= BDI_CAP_STABLE_WRITES;

@@ -5170,6 +5178,7 @@ static int rbd_add_parse_args(const char *buf,
 	rbd_opts->queue_depth = RBD_QUEUE_DEPTH_DEFAULT;
 	rbd_opts->lock_on_read = RBD_LOCK_ON_READ_DEFAULT;
 	rbd_opts->exclusive = RBD_EXCLUSIVE_DEFAULT;
+	rbd_opts->trim = RBD_TRIM_DEFAULT;
 
 	copts = ceph_parse_options(options, mon_addrs,
 					mon_addrs + mon_addrs_size - 1,

Signature · 1 line

2.4.3

亀井仁志 / KAMEI，HITOSHI

author

2018-03-26 12:31 UTC #5 lore raw

Hi Ilya,

I think your patch fully completes our purpose and I confirmed that the kernel with the patch could work well by testing in my environment.

I added the notrim option to rbd map command in accordance with your kernel rbd driver patch, and I pushed the patch to GitHub
(https://github.com/hitoshikamei/ceph/tree/rbdmap-notrim).

So, could you please merge your patch to the kernel? If your patch is merged, then my patch can work, so I will send the Pull Request.

Regards,

Signature · 1 line

Hitoshi Kamei

Ilya Dryomov

maintainer

2018-03-26 13:24 UTC #6 lore raw

Quoted 8 lines · 亀井仁志 / KAMEI，HITOSHI

Hi Ilya,
I think your patch fully completes our purpose and I confirmed that the kernel with the patch could work well by testing in my environment.
I added the notrim option to rbd map command in accordance with your kernel rbd driver patch, and I pushed the patch to GitHub
(https://github.com/hitoshikamei/ceph/tree/rbdmap-notrim).

Change the man page text to say

* notrim - Turn off discard and write zeroes offload support to avoid deprovisioning a fully provisioned image (since 4.17). When enabled, discard requests will fail with -EOPNOTSUPP, write zeroes requests will fall back to manually zeroing.

and merge it into the first patch.

Quoted 3 lines

So, could you please merge your patch to the kernel? If your patch is merged, then my patch can work, so I will send the Pull Request.

https://github.com/ceph/ceph-client/commit/7ddd22f2dd1d82da9a4cc0c54dc10760a53964f0

You can open the PR right now, we just won't merge it until the kernel patch is in.

Thanks,

Ilya

KAMEI Hitoshi

author

2018-03-27 09:44 UTC #7 lore raw

Hi Ilya,

Thank you for reviewing.
I merged two patches into one patch and pushed the new patch to GitHub. And I opened the PR. Could you check the PR?

And I forgot to mention in previous email that I modified your patch to apply the latest kernel because I couldn't apply your patch directly by using git am command due to a little bit problem, and I tested the kernel with the modified patch and rbd map command with notrim option.

The code described below is the modified patch by my hand.

Regards,

--- From here

drivers/block/rbd.c +14 -5

index 8e40da093766..c62e3788858c 100644
@@ -775,6 +775,7 @@ enum {
 	Opt_read_write,
 	Opt_lock_on_read,
 	Opt_exclusive,
+	Opt_notrim,
 	Opt_err
 };

@@ -788,6 +789,7 @@ static match_table_t rbd_opts_tokens = {
 	{Opt_read_write, "rw"},		/* Alternate spelling */
 	{Opt_lock_on_read, "lock_on_read"},
 	{Opt_exclusive, "exclusive"},
+	{Opt_notrim, "notrim"},
 	{Opt_err, NULL}
 };

@@ -796,12 +798,14 @@ struct rbd_options {
 	bool	read_only;
 	bool	lock_on_read;
 	bool	exclusive;
+	bool	trim;
 };
 
 #define RBD_QUEUE_DEPTH_DEFAULT	BLKDEV_MAX_RQ
 #define RBD_READ_ONLY_DEFAULT	false
 #define RBD_LOCK_ON_READ_DEFAULT false
 #define RBD_EXCLUSIVE_DEFAULT	false
+#define RBD_TRIM_DEFAULT	true
 
 static int parse_rbd_opts_token(char *c, void *private)
 {

@@ -843,6 +847,9 @@ static int parse_rbd_opts_token(char *c, void *private)
 	case Opt_exclusive:
 		rbd_opts->exclusive = true;
 		break;
+	case Opt_notrim:
+		rbd_opts->trim = false;
+		break;
 	default:
 		/* libceph prints "bad option" msg */
 		return -EINVAL;

@@ -4382,11 +4389,12 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 	blk_queue_io_min(q, segment_size);
 	blk_queue_io_opt(q, segment_size);
 
-	/* enable the discard support */
-	queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
-	q->limits.discard_granularity = segment_size;
-	blk_queue_max_discard_sectors(q, segment_size / SECTOR_SIZE);
-	blk_queue_max_write_zeroes_sectors(q, segment_size / SECTOR_SIZE);
+	if (rbd_dev->opts->trim) {
+		queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
+		q->limits.discard_granularity = segment_size;
+		blk_queue_max_discard_sectors(q, segment_size / SECTOR_SIZE);
+		blk_queue_max_write_zeroes_sectors(q, segment_size / SECTOR_SIZE);
+	}
 
 	if (!ceph_test_opt(rbd_dev->rbd_client->client, NOCRC))
 		q->backing_dev_info->capabilities |= BDI_CAP_STABLE_WRITES;

@@ -5637,6 +5645,7 @@ static int rbd_add_parse_args(const char *buf,
 	rbd_opts->queue_depth = RBD_QUEUE_DEPTH_DEFAULT;
 	rbd_opts->lock_on_read = RBD_LOCK_ON_READ_DEFAULT;
 	rbd_opts->exclusive = RBD_EXCLUSIVE_DEFAULT;
+	rbd_opts->trim = RBD_TRIM_DEFAULT;
 
 	copts = ceph_parse_options(options, mon_addrs,
 					mon_addrs + mon_addrs_size - 1,

--- To here

--
Hitoshi Kamei

Ilya Dryomov

maintainer

2018-03-27 16:27 UTC #8 lore raw

Quoted 6 lines · KAMEI Hitoshi

Hi Ilya,
Thank you for reviewing.
I merged two patches into one patch and pushed the new patch to GitHub. And I opened the PR. Could you check the PR?

Quoted 6 lines

And I forgot to mention in previous email that I modified your patch to apply the latest kernel because I couldn't apply your patch directly by using git am command due to a little bit problem, and I tested the kernel with the modified patch and rbd map command with notrim option.
The code described below is the modified patch by my hand.

Right, my patch is based on ceph/ceph-client.git:testing which has some unrelated rbd_init_disk() changes.

Thanks,

Ilya