* [PATCH 0/3] nbd: add support for DISCARD
@ 2011-09-07 14:41 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2011-09-07 14:41 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-api, Paul Clements
This patch series adds support for a DISCARD command in the nbd
protocol. qemu-nbd will be the first user of the feature.
Please consider this for 3.2.
Thanks!
Cc: Paul Clements <Paul.Clements@steeleye.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Paolo Bonzini (3):
nbd: remove unused flags fields
nbd: add support for feature negotiation
nbd: map DISCARD requests to a new nbd request type
drivers/block/nbd.c | 28 ++++++++++++++++++++--------
include/linux/nbd.h | 32 ++++++++++++++++++--------------
2 files changed, 38 insertions(+), 22 deletions(-)
--
1.7.6
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 0/3] nbd: add support for DISCARD
@ 2011-09-07 14:41 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2011-09-07 14:41 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA, Paul Clements
This patch series adds support for a DISCARD command in the nbd
protocol. qemu-nbd will be the first user of the feature.
Please consider this for 3.2.
Thanks!
Cc: Paul Clements <Paul.Clements-G8/ITkJZaeZWk0Htik3J/w@public.gmane.org>
Signed-off-by: Paolo Bonzini <pbonzini-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Paolo Bonzini (3):
nbd: remove unused flags fields
nbd: add support for feature negotiation
nbd: map DISCARD requests to a new nbd request type
drivers/block/nbd.c | 28 ++++++++++++++++++++--------
include/linux/nbd.h | 32 ++++++++++++++++++--------------
2 files changed, 38 insertions(+), 22 deletions(-)
--
1.7.6
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/3] nbd: remove unused flags fields
@ 2011-09-07 14:41 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2011-09-07 14:41 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-api, Paul Clements
The flags field is never written right now. Before putting it
to new use in the next patches, clean up the uses.
Cc: Paul Clements <Paul.Clements@steeleye.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
drivers/block/nbd.c | 11 +++--------
include/linux/nbd.h | 4 ----
2 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index f533f33..be23aec 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -453,15 +453,10 @@ static void nbd_handle_req(struct nbd_device *lo, struct request *req)
if (req->cmd_type != REQ_TYPE_FS)
goto error_out;
- nbd_cmd(req) = NBD_CMD_READ;
- if (rq_data_dir(req) == WRITE) {
+ if (rq_data_dir(req) == WRITE)
nbd_cmd(req) = NBD_CMD_WRITE;
- if (lo->flags & NBD_READ_ONLY) {
- printk(KERN_ERR "%s: Write on read-only\n",
- lo->disk->disk_name);
- goto error_out;
- }
- }
+ else
+ nbd_cmd(req) = NBD_CMD_READ;
req->errors = 0;
diff --git a/include/linux/nbd.h b/include/linux/nbd.h
index d146ca1..0582054 100644
--- a/include/linux/nbd.h
+++ b/include/linux/nbd.h
@@ -42,10 +42,6 @@ enum {
#include <linux/wait.h>
#include <linux/mutex.h>
-/* values for flags field */
-#define NBD_READ_ONLY 0x0001
-#define NBD_WRITE_NOCHK 0x0002
-
struct request;
struct nbd_device {
--
1.7.6
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 1/3] nbd: remove unused flags fields
@ 2011-09-07 14:41 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2011-09-07 14:41 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA, Paul Clements
The flags field is never written right now. Before putting it
to new use in the next patches, clean up the uses.
Cc: Paul Clements <Paul.Clements-G8/ITkJZaeZWk0Htik3J/w@public.gmane.org>
Signed-off-by: Paolo Bonzini <pbonzini-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
drivers/block/nbd.c | 11 +++--------
include/linux/nbd.h | 4 ----
2 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index f533f33..be23aec 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -453,15 +453,10 @@ static void nbd_handle_req(struct nbd_device *lo, struct request *req)
if (req->cmd_type != REQ_TYPE_FS)
goto error_out;
- nbd_cmd(req) = NBD_CMD_READ;
- if (rq_data_dir(req) == WRITE) {
+ if (rq_data_dir(req) == WRITE)
nbd_cmd(req) = NBD_CMD_WRITE;
- if (lo->flags & NBD_READ_ONLY) {
- printk(KERN_ERR "%s: Write on read-only\n",
- lo->disk->disk_name);
- goto error_out;
- }
- }
+ else
+ nbd_cmd(req) = NBD_CMD_READ;
req->errors = 0;
diff --git a/include/linux/nbd.h b/include/linux/nbd.h
index d146ca1..0582054 100644
--- a/include/linux/nbd.h
+++ b/include/linux/nbd.h
@@ -42,10 +42,6 @@ enum {
#include <linux/wait.h>
#include <linux/mutex.h>
-/* values for flags field */
-#define NBD_READ_ONLY 0x0001
-#define NBD_WRITE_NOCHK 0x0002
-
struct request;
struct nbd_device {
--
1.7.6
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/3] nbd: add support for feature negotiation
@ 2011-09-07 14:41 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2011-09-07 14:41 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-api, Paul Clements
Add a IOCTL to negotiate features between the server and the
kernel module (mediated by the usermode client). Features are
stored in the pre-existing flags field.
Also zero out the flags when NBD_DO_IT completes.
Cc: Paul Clements <Paul.Clements@steeleye.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
drivers/block/nbd.c | 8 ++++++++
include/linux/nbd.h | 24 +++++++++++++++---------
2 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index be23aec..c4fe9c8 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -85,6 +85,7 @@ static const char *ioctl_cmd_to_ascii(int cmd)
case NBD_PRINT_DEBUG: return "print-debug";
case NBD_SET_SIZE_BLOCKS: return "set-size-blocks";
case NBD_DISCONNECT: return "disconnect";
+ case NBD_SET_FEATURES: return "set-features";
case BLKROSET: return "set-read-only";
case BLKFLSBUF: return "flush-buffer-cache";
}
@@ -634,6 +635,12 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *lo,
lo->xmit_timeout = arg * HZ;
return 0;
+ case NBD_SET_FEATURES:
+ if (((u64) arg) & NBD_FEATURE_RESERVED)
+ return -EINVAL;
+ lo->flags = (lo->flags & NBD_FEATURE_RESERVED) | (u64)arg;
+ return 0;
+
case NBD_SET_SIZE_BLOCKS:
lo->bytesize = ((u64) arg) * lo->blksize;
bdev->bd_inode->i_size = lo->bytesize;
@@ -672,6 +679,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *lo,
printk(KERN_WARNING "%s: queue cleared\n", lo->disk->disk_name);
if (file)
fput(file);
+ lo->flags = 0;
lo->bytesize = 0;
bdev->bd_inode->i_size = 0;
set_capacity(lo->disk, 0);
diff --git a/include/linux/nbd.h b/include/linux/nbd.h
index 0582054..926f19d 100644
--- a/include/linux/nbd.h
+++ b/include/linux/nbd.h
@@ -17,16 +17,22 @@
#include <linux/types.h>
-#define NBD_SET_SOCK _IO( 0xab, 0 )
-#define NBD_SET_BLKSIZE _IO( 0xab, 1 )
-#define NBD_SET_SIZE _IO( 0xab, 2 )
-#define NBD_DO_IT _IO( 0xab, 3 )
-#define NBD_CLEAR_SOCK _IO( 0xab, 4 )
-#define NBD_CLEAR_QUE _IO( 0xab, 5 )
-#define NBD_PRINT_DEBUG _IO( 0xab, 6 )
-#define NBD_SET_SIZE_BLOCKS _IO( 0xab, 7 )
-#define NBD_DISCONNECT _IO( 0xab, 8 )
-#define NBD_SET_TIMEOUT _IO( 0xab, 9 )
+#define NBD_SET_SOCK _IO(0xab, 0)
+#define NBD_SET_BLKSIZE _IO(0xab, 1)
+#define NBD_SET_SIZE _IO(0xab, 2)
+#define NBD_DO_IT _IO(0xab, 3)
+#define NBD_CLEAR_SOCK _IO(0xab, 4)
+#define NBD_CLEAR_QUE _IO(0xab, 5)
+#define NBD_PRINT_DEBUG _IO(0xab, 6)
+#define NBD_SET_SIZE_BLOCKS _IO(0xab, 7)
+#define NBD_DISCONNECT _IO(0xab, 8)
+#define NBD_SET_TIMEOUT _IO(0xab, 9)
+#define NBD_SET_FEATURES _IO(0xab, 10)
+
+/* bits 0 and 1 are already used for flags in the protocol, we do
+ * not want any overlap. Bits 32-63 are reserved because they
+ * are inaccessible on 32-bit platforms. */
+#define NBD_FEATURE_RESERVED 0xFFFFFFFF00000003LL
enum {
NBD_CMD_READ = 0,
--
1.7.6
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/3] nbd: add support for feature negotiation
@ 2011-09-07 14:41 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2011-09-07 14:41 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA, Paul Clements
Add a IOCTL to negotiate features between the server and the
kernel module (mediated by the usermode client). Features are
stored in the pre-existing flags field.
Also zero out the flags when NBD_DO_IT completes.
Cc: Paul Clements <Paul.Clements-G8/ITkJZaeZWk0Htik3J/w@public.gmane.org>
Signed-off-by: Paolo Bonzini <pbonzini-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
drivers/block/nbd.c | 8 ++++++++
include/linux/nbd.h | 24 +++++++++++++++---------
2 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index be23aec..c4fe9c8 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -85,6 +85,7 @@ static const char *ioctl_cmd_to_ascii(int cmd)
case NBD_PRINT_DEBUG: return "print-debug";
case NBD_SET_SIZE_BLOCKS: return "set-size-blocks";
case NBD_DISCONNECT: return "disconnect";
+ case NBD_SET_FEATURES: return "set-features";
case BLKROSET: return "set-read-only";
case BLKFLSBUF: return "flush-buffer-cache";
}
@@ -634,6 +635,12 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *lo,
lo->xmit_timeout = arg * HZ;
return 0;
+ case NBD_SET_FEATURES:
+ if (((u64) arg) & NBD_FEATURE_RESERVED)
+ return -EINVAL;
+ lo->flags = (lo->flags & NBD_FEATURE_RESERVED) | (u64)arg;
+ return 0;
+
case NBD_SET_SIZE_BLOCKS:
lo->bytesize = ((u64) arg) * lo->blksize;
bdev->bd_inode->i_size = lo->bytesize;
@@ -672,6 +679,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *lo,
printk(KERN_WARNING "%s: queue cleared\n", lo->disk->disk_name);
if (file)
fput(file);
+ lo->flags = 0;
lo->bytesize = 0;
bdev->bd_inode->i_size = 0;
set_capacity(lo->disk, 0);
diff --git a/include/linux/nbd.h b/include/linux/nbd.h
index 0582054..926f19d 100644
--- a/include/linux/nbd.h
+++ b/include/linux/nbd.h
@@ -17,16 +17,22 @@
#include <linux/types.h>
-#define NBD_SET_SOCK _IO( 0xab, 0 )
-#define NBD_SET_BLKSIZE _IO( 0xab, 1 )
-#define NBD_SET_SIZE _IO( 0xab, 2 )
-#define NBD_DO_IT _IO( 0xab, 3 )
-#define NBD_CLEAR_SOCK _IO( 0xab, 4 )
-#define NBD_CLEAR_QUE _IO( 0xab, 5 )
-#define NBD_PRINT_DEBUG _IO( 0xab, 6 )
-#define NBD_SET_SIZE_BLOCKS _IO( 0xab, 7 )
-#define NBD_DISCONNECT _IO( 0xab, 8 )
-#define NBD_SET_TIMEOUT _IO( 0xab, 9 )
+#define NBD_SET_SOCK _IO(0xab, 0)
+#define NBD_SET_BLKSIZE _IO(0xab, 1)
+#define NBD_SET_SIZE _IO(0xab, 2)
+#define NBD_DO_IT _IO(0xab, 3)
+#define NBD_CLEAR_SOCK _IO(0xab, 4)
+#define NBD_CLEAR_QUE _IO(0xab, 5)
+#define NBD_PRINT_DEBUG _IO(0xab, 6)
+#define NBD_SET_SIZE_BLOCKS _IO(0xab, 7)
+#define NBD_DISCONNECT _IO(0xab, 8)
+#define NBD_SET_TIMEOUT _IO(0xab, 9)
+#define NBD_SET_FEATURES _IO(0xab, 10)
+
+/* bits 0 and 1 are already used for flags in the protocol, we do
+ * not want any overlap. Bits 32-63 are reserved because they
+ * are inaccessible on 32-bit platforms. */
+#define NBD_FEATURE_RESERVED 0xFFFFFFFF00000003LL
enum {
NBD_CMD_READ = 0,
--
1.7.6
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/3] nbd: map DISCARD requests to a new nbd request type
2011-09-07 14:41 ` Paolo Bonzini
` (2 preceding siblings ...)
(?)
@ 2011-09-07 14:41 ` Paolo Bonzini
2011-09-08 1:13 ` Paul Clements
-1 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2011-09-07 14:41 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-api, Paul Clements
When the feature is enabled, set QUEUE_FLAG_DISCARD and transmit those as
NBD_CMD_TRIM requests.
I used "trim" instead of "discard" to avoid confusion with the existing
NBD_CMD_DISC that is used for disconnect.
Cc: Paul Clements <Paul.Clements@steeleye.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
drivers/block/nbd.c | 15 ++++++++++++---
include/linux/nbd.h | 4 +++-
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index c4fe9c8..71ebaab 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -98,6 +98,7 @@ static const char *nbdcmd_to_ascii(int cmd)
case NBD_CMD_READ: return "read";
case NBD_CMD_WRITE: return "write";
case NBD_CMD_DISC: return "disconnect";
+ case NBD_CMD_TRIM: return "trim (discard)";
}
return "invalid";
}
@@ -454,9 +455,13 @@ static void nbd_handle_req(struct nbd_device *lo, struct request *req)
if (req->cmd_type != REQ_TYPE_FS)
goto error_out;
- if (rq_data_dir(req) == WRITE)
- nbd_cmd(req) = NBD_CMD_WRITE;
- else
+ if (rq_data_dir(req) == WRITE) {
+ if ((req->cmd_flags & REQ_DISCARD)) {
+ WARN_ON(!(lo->flags & NBD_FEATURE_TRIM));
+ nbd_cmd(req) = NBD_CMD_TRIM;
+ } else
+ nbd_cmd(req) = NBD_CMD_WRITE;
+ } else
nbd_cmd(req) = NBD_CMD_READ;
req->errors = 0;
@@ -659,6 +664,9 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *lo,
return -EINVAL;
mutex_unlock(&lo->tx_lock);
+ if (lo->flags & NBD_FEATURE_TRIM)
+ queue_flag_set_unlocked(QUEUE_FLAG_DISCARD,
+ lo->disk->queue);
thread = kthread_create(nbd_thread, lo, lo->disk->disk_name);
if (IS_ERR(thread)) {
@@ -677,6 +685,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *lo,
lo->file = NULL;
nbd_clear_que(lo);
printk(KERN_WARNING "%s: queue cleared\n", lo->disk->disk_name);
+ queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, lo->disk->queue);
if (file)
fput(file);
lo->flags = 0;
@@ -796,6 +805,9 @@ static int __init nbd_init(void)
* Tell the block layer that we are not a rotational device
*/
queue_flag_set_unlocked(QUEUE_FLAG_NONROT, disk->queue);
+ disk->queue->limits.discard_granularity = 512;
+ disk->queue->limits.max_discard_sectors = UINT_MAX;
+ disk->queue->limits.discard_zeroes_data = 0;
}
if (register_blkdev(NBD_MAJOR, "nbd")) {
diff --git a/include/linux/nbd.h b/include/linux/nbd.h
index 926f19d..7048463 100644
--- a/include/linux/nbd.h
+++ b/include/linux/nbd.h
@@ -33,11 +33,13 @@
* not want any overlap. Bits 32-63 are reserved because they
* are inaccessible on 32-bit platforms. */
#define NBD_FEATURE_RESERVED 0xFFFFFFFF00000003LL
+#define NBD_FEATURE_TRIM (1 << 2)
enum {
NBD_CMD_READ = 0,
NBD_CMD_WRITE = 1,
- NBD_CMD_DISC = 2
+ NBD_CMD_DISC = 2,
+ NBD_CMD_TRIM = 3
};
#define nbd_cmd(req) ((req)->cmd[0])
--
1.7.6
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] nbd: add support for feature negotiation
@ 2011-09-07 16:30 ` Paul Clements
0 siblings, 0 replies; 17+ messages in thread
From: Paul Clements @ 2011-09-07 16:30 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, linux-api, nbd-general
Paolo,
thanks for the patch...
On Wed, Sep 7, 2011 at 10:41 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Add a IOCTL to negotiate features between the server and the
> kernel module (mediated by the usermode client). Features are
> stored in the pre-existing flags field.
I'm not crazy about another ioctl, but we need to get this flag
setting functionality in, and I just have not had the time...
> Also zero out the flags when NBD_DO_IT completes.
>
> Cc: Paul Clements <Paul.Clements@steeleye.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> drivers/block/nbd.c | 8 ++++++++
> include/linux/nbd.h | 24 +++++++++++++++---------
> 2 files changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index be23aec..c4fe9c8 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -85,6 +85,7 @@ static const char *ioctl_cmd_to_ascii(int cmd)
> case NBD_PRINT_DEBUG: return "print-debug";
> case NBD_SET_SIZE_BLOCKS: return "set-size-blocks";
> case NBD_DISCONNECT: return "disconnect";
> + case NBD_SET_FEATURES: return "set-features";
Could you name this as NBD_SET_FLAGS, as that's more consistent with
what it's really doing?
Otherwise these look good for inclusion.
Thanks,
Paul
> case BLKROSET: return "set-read-only";
> case BLKFLSBUF: return "flush-buffer-cache";
> }
> @@ -634,6 +635,12 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *lo,
> lo->xmit_timeout = arg * HZ;
> return 0;
>
> + case NBD_SET_FEATURES:
> + if (((u64) arg) & NBD_FEATURE_RESERVED)
> + return -EINVAL;
> + lo->flags = (lo->flags & NBD_FEATURE_RESERVED) | (u64)arg;
> + return 0;
> +
> case NBD_SET_SIZE_BLOCKS:
> lo->bytesize = ((u64) arg) * lo->blksize;
> bdev->bd_inode->i_size = lo->bytesize;
> @@ -672,6 +679,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *lo,
> printk(KERN_WARNING "%s: queue cleared\n", lo->disk->disk_name);
> if (file)
> fput(file);
> + lo->flags = 0;
> lo->bytesize = 0;
> bdev->bd_inode->i_size = 0;
> set_capacity(lo->disk, 0);
> diff --git a/include/linux/nbd.h b/include/linux/nbd.h
> index 0582054..926f19d 100644
> --- a/include/linux/nbd.h
> +++ b/include/linux/nbd.h
> @@ -17,16 +17,22 @@
>
> #include <linux/types.h>
>
> -#define NBD_SET_SOCK _IO( 0xab, 0 )
> -#define NBD_SET_BLKSIZE _IO( 0xab, 1 )
> -#define NBD_SET_SIZE _IO( 0xab, 2 )
> -#define NBD_DO_IT _IO( 0xab, 3 )
> -#define NBD_CLEAR_SOCK _IO( 0xab, 4 )
> -#define NBD_CLEAR_QUE _IO( 0xab, 5 )
> -#define NBD_PRINT_DEBUG _IO( 0xab, 6 )
> -#define NBD_SET_SIZE_BLOCKS _IO( 0xab, 7 )
> -#define NBD_DISCONNECT _IO( 0xab, 8 )
> -#define NBD_SET_TIMEOUT _IO( 0xab, 9 )
> +#define NBD_SET_SOCK _IO(0xab, 0)
> +#define NBD_SET_BLKSIZE _IO(0xab, 1)
> +#define NBD_SET_SIZE _IO(0xab, 2)
> +#define NBD_DO_IT _IO(0xab, 3)
> +#define NBD_CLEAR_SOCK _IO(0xab, 4)
> +#define NBD_CLEAR_QUE _IO(0xab, 5)
> +#define NBD_PRINT_DEBUG _IO(0xab, 6)
> +#define NBD_SET_SIZE_BLOCKS _IO(0xab, 7)
> +#define NBD_DISCONNECT _IO(0xab, 8)
> +#define NBD_SET_TIMEOUT _IO(0xab, 9)
> +#define NBD_SET_FEATURES _IO(0xab, 10)
> +
> +/* bits 0 and 1 are already used for flags in the protocol, we do
> + * not want any overlap. Bits 32-63 are reserved because they
> + * are inaccessible on 32-bit platforms. */
> +#define NBD_FEATURE_RESERVED 0xFFFFFFFF00000003LL
>
> enum {
> NBD_CMD_READ = 0,
> --
> 1.7.6
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] nbd: add support for feature negotiation
@ 2011-09-07 16:30 ` Paul Clements
0 siblings, 0 replies; 17+ messages in thread
From: Paul Clements @ 2011-09-07 16:30 UTC (permalink / raw)
To: Paolo Bonzini
Cc: nbd-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Paolo,
thanks for the patch...
On Wed, Sep 7, 2011 at 10:41 AM, Paolo Bonzini <pbonzini-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Add a IOCTL to negotiate features between the server and the
> kernel module (mediated by the usermode client). Features are
> stored in the pre-existing flags field.
I'm not crazy about another ioctl, but we need to get this flag
setting functionality in, and I just have not had the time...
> Also zero out the flags when NBD_DO_IT completes.
>
> Cc: Paul Clements <Paul.Clements-G8/ITkJZaeZWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Paolo Bonzini <pbonzini-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> drivers/block/nbd.c | 8 ++++++++
> include/linux/nbd.h | 24 +++++++++++++++---------
> 2 files changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index be23aec..c4fe9c8 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -85,6 +85,7 @@ static const char *ioctl_cmd_to_ascii(int cmd)
> case NBD_PRINT_DEBUG: return "print-debug";
> case NBD_SET_SIZE_BLOCKS: return "set-size-blocks";
> case NBD_DISCONNECT: return "disconnect";
> + case NBD_SET_FEATURES: return "set-features";
Could you name this as NBD_SET_FLAGS, as that's more consistent with
what it's really doing?
Otherwise these look good for inclusion.
Thanks,
Paul
> case BLKROSET: return "set-read-only";
> case BLKFLSBUF: return "flush-buffer-cache";
> }
> @@ -634,6 +635,12 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *lo,
> lo->xmit_timeout = arg * HZ;
> return 0;
>
> + case NBD_SET_FEATURES:
> + if (((u64) arg) & NBD_FEATURE_RESERVED)
> + return -EINVAL;
> + lo->flags = (lo->flags & NBD_FEATURE_RESERVED) | (u64)arg;
> + return 0;
> +
> case NBD_SET_SIZE_BLOCKS:
> lo->bytesize = ((u64) arg) * lo->blksize;
> bdev->bd_inode->i_size = lo->bytesize;
> @@ -672,6 +679,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *lo,
> printk(KERN_WARNING "%s: queue cleared\n", lo->disk->disk_name);
> if (file)
> fput(file);
> + lo->flags = 0;
> lo->bytesize = 0;
> bdev->bd_inode->i_size = 0;
> set_capacity(lo->disk, 0);
> diff --git a/include/linux/nbd.h b/include/linux/nbd.h
> index 0582054..926f19d 100644
> --- a/include/linux/nbd.h
> +++ b/include/linux/nbd.h
> @@ -17,16 +17,22 @@
>
> #include <linux/types.h>
>
> -#define NBD_SET_SOCK _IO( 0xab, 0 )
> -#define NBD_SET_BLKSIZE _IO( 0xab, 1 )
> -#define NBD_SET_SIZE _IO( 0xab, 2 )
> -#define NBD_DO_IT _IO( 0xab, 3 )
> -#define NBD_CLEAR_SOCK _IO( 0xab, 4 )
> -#define NBD_CLEAR_QUE _IO( 0xab, 5 )
> -#define NBD_PRINT_DEBUG _IO( 0xab, 6 )
> -#define NBD_SET_SIZE_BLOCKS _IO( 0xab, 7 )
> -#define NBD_DISCONNECT _IO( 0xab, 8 )
> -#define NBD_SET_TIMEOUT _IO( 0xab, 9 )
> +#define NBD_SET_SOCK _IO(0xab, 0)
> +#define NBD_SET_BLKSIZE _IO(0xab, 1)
> +#define NBD_SET_SIZE _IO(0xab, 2)
> +#define NBD_DO_IT _IO(0xab, 3)
> +#define NBD_CLEAR_SOCK _IO(0xab, 4)
> +#define NBD_CLEAR_QUE _IO(0xab, 5)
> +#define NBD_PRINT_DEBUG _IO(0xab, 6)
> +#define NBD_SET_SIZE_BLOCKS _IO(0xab, 7)
> +#define NBD_DISCONNECT _IO(0xab, 8)
> +#define NBD_SET_TIMEOUT _IO(0xab, 9)
> +#define NBD_SET_FEATURES _IO(0xab, 10)
> +
> +/* bits 0 and 1 are already used for flags in the protocol, we do
> + * not want any overlap. Bits 32-63 are reserved because they
> + * are inaccessible on 32-bit platforms. */
> +#define NBD_FEATURE_RESERVED 0xFFFFFFFF00000003LL
>
> enum {
> NBD_CMD_READ = 0,
> --
> 1.7.6
>
>
>
------------------------------------------------------------------------------
Using storage to extend the benefits of virtualization and iSCSI
Virtualization increases hardware utilization and delivers a new level of
agility. Learn what those decisions are and how to modernize your storage
and backup environments for virtualization.
http://www.accelacomm.com/jaw/sfnl/114/51434361/
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] nbd: add support for feature negotiation
@ 2011-09-07 17:52 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2011-09-07 17:52 UTC (permalink / raw)
To: Paul Clements; +Cc: linux-kernel, linux-api, nbd-general
On 09/07/2011 06:30 PM, Paul Clements wrote:
> Could you name this as NBD_SET_FLAGS, as that's more consistent with
> what it's really doing?
I named it differently intentionally, actually, because it should not
use the flags field from the network protocol as is. Also, "features"
sounds more like something that is optional, while unrecognized "flags"
should probably cause a failure.
What about renaming the struct field and leaving this as
NBD_SET_FEATURES or NBD_ENABLE_FEATURES?
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] nbd: add support for feature negotiation
@ 2011-09-07 17:52 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2011-09-07 17:52 UTC (permalink / raw)
To: Paul Clements
Cc: nbd-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On 09/07/2011 06:30 PM, Paul Clements wrote:
> Could you name this as NBD_SET_FLAGS, as that's more consistent with
> what it's really doing?
I named it differently intentionally, actually, because it should not
use the flags field from the network protocol as is. Also, "features"
sounds more like something that is optional, while unrecognized "flags"
should probably cause a failure.
What about renaming the struct field and leaving this as
NBD_SET_FEATURES or NBD_ENABLE_FEATURES?
Paolo
------------------------------------------------------------------------------
Using storage to extend the benefits of virtualization and iSCSI
Virtualization increases hardware utilization and delivers a new level of
agility. Learn what those decisions are and how to modernize your storage
and backup environments for virtualization.
http://www.accelacomm.com/jaw/sfnl/114/51434361/
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] nbd: add support for feature negotiation
@ 2011-09-07 21:54 ` Paul Clements
0 siblings, 0 replies; 17+ messages in thread
From: Paul Clements @ 2011-09-07 21:54 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, linux-api, nbd-general
On Wed, Sep 7, 2011 at 1:52 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 09/07/2011 06:30 PM, Paul Clements wrote:
>>
>> Could you name this as NBD_SET_FLAGS, as that's more consistent with
>> what it's really doing?
>
> I named it differently intentionally, actually, because it should not use
> the flags field from the network protocol as is. Also, "features" sounds
> more like something that is optional, while unrecognized "flags" should
> probably cause a failure.
Alright, I can buy that argument -- I don't feel that strongly about it...
This looks good to me, then.
--
Paul
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] nbd: add support for feature negotiation
@ 2011-09-07 21:54 ` Paul Clements
0 siblings, 0 replies; 17+ messages in thread
From: Paul Clements @ 2011-09-07 21:54 UTC (permalink / raw)
To: Paolo Bonzini
Cc: nbd-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Wed, Sep 7, 2011 at 1:52 PM, Paolo Bonzini <pbonzini-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 09/07/2011 06:30 PM, Paul Clements wrote:
>>
>> Could you name this as NBD_SET_FLAGS, as that's more consistent with
>> what it's really doing?
>
> I named it differently intentionally, actually, because it should not use
> the flags field from the network protocol as is. Also, "features" sounds
> more like something that is optional, while unrecognized "flags" should
> probably cause a failure.
Alright, I can buy that argument -- I don't feel that strongly about it...
This looks good to me, then.
--
Paul
------------------------------------------------------------------------------
Using storage to extend the benefits of virtualization and iSCSI
Virtualization increases hardware utilization and delivers a new level of
agility. Learn what those decisions are and how to modernize your storage
and backup environments for virtualization.
http://www.accelacomm.com/jaw/sfnl/114/51434361/
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] nbd: map DISCARD requests to a new nbd request type
@ 2011-09-08 1:13 ` Paul Clements
0 siblings, 0 replies; 17+ messages in thread
From: Paul Clements @ 2011-09-08 1:13 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, linux-api
On Wed, Sep 7, 2011 at 10:41 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> When the feature is enabled, set QUEUE_FLAG_DISCARD and transmit those as
> NBD_CMD_TRIM requests.
>
> I used "trim" instead of "discard" to avoid confusion with the existing
> NBD_CMD_DISC that is used for disconnect.
This patchset looks good to me.
--
Paul
> Cc: Paul Clements <Paul.Clements@steeleye.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> drivers/block/nbd.c | 15 ++++++++++++---
> include/linux/nbd.h | 4 +++-
> 2 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index c4fe9c8..71ebaab 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -98,6 +98,7 @@ static const char *nbdcmd_to_ascii(int cmd)
> case NBD_CMD_READ: return "read";
> case NBD_CMD_WRITE: return "write";
> case NBD_CMD_DISC: return "disconnect";
> + case NBD_CMD_TRIM: return "trim (discard)";
> }
> return "invalid";
> }
> @@ -454,9 +455,13 @@ static void nbd_handle_req(struct nbd_device *lo, struct request *req)
> if (req->cmd_type != REQ_TYPE_FS)
> goto error_out;
>
> - if (rq_data_dir(req) == WRITE)
> - nbd_cmd(req) = NBD_CMD_WRITE;
> - else
> + if (rq_data_dir(req) == WRITE) {
> + if ((req->cmd_flags & REQ_DISCARD)) {
> + WARN_ON(!(lo->flags & NBD_FEATURE_TRIM));
> + nbd_cmd(req) = NBD_CMD_TRIM;
> + } else
> + nbd_cmd(req) = NBD_CMD_WRITE;
> + } else
> nbd_cmd(req) = NBD_CMD_READ;
>
> req->errors = 0;
> @@ -659,6 +664,9 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *lo,
> return -EINVAL;
>
> mutex_unlock(&lo->tx_lock);
> + if (lo->flags & NBD_FEATURE_TRIM)
> + queue_flag_set_unlocked(QUEUE_FLAG_DISCARD,
> + lo->disk->queue);
>
> thread = kthread_create(nbd_thread, lo, lo->disk->disk_name);
> if (IS_ERR(thread)) {
> @@ -677,6 +685,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *lo,
> lo->file = NULL;
> nbd_clear_que(lo);
> printk(KERN_WARNING "%s: queue cleared\n", lo->disk->disk_name);
> + queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, lo->disk->queue);
> if (file)
> fput(file);
> lo->flags = 0;
> @@ -796,6 +805,9 @@ static int __init nbd_init(void)
> * Tell the block layer that we are not a rotational device
> */
> queue_flag_set_unlocked(QUEUE_FLAG_NONROT, disk->queue);
> + disk->queue->limits.discard_granularity = 512;
> + disk->queue->limits.max_discard_sectors = UINT_MAX;
> + disk->queue->limits.discard_zeroes_data = 0;
> }
>
> if (register_blkdev(NBD_MAJOR, "nbd")) {
> diff --git a/include/linux/nbd.h b/include/linux/nbd.h
> index 926f19d..7048463 100644
> --- a/include/linux/nbd.h
> +++ b/include/linux/nbd.h
> @@ -33,11 +33,13 @@
> * not want any overlap. Bits 32-63 are reserved because they
> * are inaccessible on 32-bit platforms. */
> #define NBD_FEATURE_RESERVED 0xFFFFFFFF00000003LL
> +#define NBD_FEATURE_TRIM (1 << 2)
>
> enum {
> NBD_CMD_READ = 0,
> NBD_CMD_WRITE = 1,
> - NBD_CMD_DISC = 2
> + NBD_CMD_DISC = 2,
> + NBD_CMD_TRIM = 3
> };
>
> #define nbd_cmd(req) ((req)->cmd[0])
> --
> 1.7.6
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] nbd: map DISCARD requests to a new nbd request type
@ 2011-09-08 1:13 ` Paul Clements
0 siblings, 0 replies; 17+ messages in thread
From: Paul Clements @ 2011-09-08 1:13 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA
On Wed, Sep 7, 2011 at 10:41 AM, Paolo Bonzini <pbonzini-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> When the feature is enabled, set QUEUE_FLAG_DISCARD and transmit those as
> NBD_CMD_TRIM requests.
>
> I used "trim" instead of "discard" to avoid confusion with the existing
> NBD_CMD_DISC that is used for disconnect.
This patchset looks good to me.
--
Paul
> Cc: Paul Clements <Paul.Clements-G8/ITkJZaeZWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Paolo Bonzini <pbonzini-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> drivers/block/nbd.c | 15 ++++++++++++---
> include/linux/nbd.h | 4 +++-
> 2 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index c4fe9c8..71ebaab 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -98,6 +98,7 @@ static const char *nbdcmd_to_ascii(int cmd)
> case NBD_CMD_READ: return "read";
> case NBD_CMD_WRITE: return "write";
> case NBD_CMD_DISC: return "disconnect";
> + case NBD_CMD_TRIM: return "trim (discard)";
> }
> return "invalid";
> }
> @@ -454,9 +455,13 @@ static void nbd_handle_req(struct nbd_device *lo, struct request *req)
> if (req->cmd_type != REQ_TYPE_FS)
> goto error_out;
>
> - if (rq_data_dir(req) == WRITE)
> - nbd_cmd(req) = NBD_CMD_WRITE;
> - else
> + if (rq_data_dir(req) == WRITE) {
> + if ((req->cmd_flags & REQ_DISCARD)) {
> + WARN_ON(!(lo->flags & NBD_FEATURE_TRIM));
> + nbd_cmd(req) = NBD_CMD_TRIM;
> + } else
> + nbd_cmd(req) = NBD_CMD_WRITE;
> + } else
> nbd_cmd(req) = NBD_CMD_READ;
>
> req->errors = 0;
> @@ -659,6 +664,9 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *lo,
> return -EINVAL;
>
> mutex_unlock(&lo->tx_lock);
> + if (lo->flags & NBD_FEATURE_TRIM)
> + queue_flag_set_unlocked(QUEUE_FLAG_DISCARD,
> + lo->disk->queue);
>
> thread = kthread_create(nbd_thread, lo, lo->disk->disk_name);
> if (IS_ERR(thread)) {
> @@ -677,6 +685,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *lo,
> lo->file = NULL;
> nbd_clear_que(lo);
> printk(KERN_WARNING "%s: queue cleared\n", lo->disk->disk_name);
> + queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, lo->disk->queue);
> if (file)
> fput(file);
> lo->flags = 0;
> @@ -796,6 +805,9 @@ static int __init nbd_init(void)
> * Tell the block layer that we are not a rotational device
> */
> queue_flag_set_unlocked(QUEUE_FLAG_NONROT, disk->queue);
> + disk->queue->limits.discard_granularity = 512;
> + disk->queue->limits.max_discard_sectors = UINT_MAX;
> + disk->queue->limits.discard_zeroes_data = 0;
> }
>
> if (register_blkdev(NBD_MAJOR, "nbd")) {
> diff --git a/include/linux/nbd.h b/include/linux/nbd.h
> index 926f19d..7048463 100644
> --- a/include/linux/nbd.h
> +++ b/include/linux/nbd.h
> @@ -33,11 +33,13 @@
> * not want any overlap. Bits 32-63 are reserved because they
> * are inaccessible on 32-bit platforms. */
> #define NBD_FEATURE_RESERVED 0xFFFFFFFF00000003LL
> +#define NBD_FEATURE_TRIM (1 << 2)
>
> enum {
> NBD_CMD_READ = 0,
> NBD_CMD_WRITE = 1,
> - NBD_CMD_DISC = 2
> + NBD_CMD_DISC = 2,
> + NBD_CMD_TRIM = 3
> };
>
> #define nbd_cmd(req) ((req)->cmd[0])
> --
> 1.7.6
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] nbd: add support for feature negotiation
@ 2011-09-08 7:00 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2011-09-08 7:00 UTC (permalink / raw)
To: Paul Clements; +Cc: linux-kernel, linux-api, nbd-general
On 09/07/2011 11:54 PM, Paul Clements wrote:
>> > I named it differently intentionally, actually, because it should not use
>> > the flags field from the network protocol as is. Also, "features" sounds
>> > more like something that is optional, while unrecognized "flags" should
>> > probably cause a failure.
> Alright, I can buy that argument -- I don't feel that strongly about it...
>
> This looks good to me, then.
Actually no, I can do better. :)
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] nbd: add support for feature negotiation
@ 2011-09-08 7:00 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2011-09-08 7:00 UTC (permalink / raw)
To: Paul Clements
Cc: nbd-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On 09/07/2011 11:54 PM, Paul Clements wrote:
>> > I named it differently intentionally, actually, because it should not use
>> > the flags field from the network protocol as is. Also, "features" sounds
>> > more like something that is optional, while unrecognized "flags" should
>> > probably cause a failure.
> Alright, I can buy that argument -- I don't feel that strongly about it...
>
> This looks good to me, then.
Actually no, I can do better. :)
Paolo
------------------------------------------------------------------------------
Doing More with Less: The Next Generation Virtual Desktop
What are the key obstacles that have prevented many mid-market businesses
from deploying virtual desktops? How do next-generation virtual desktops
provide companies an easier-to-deploy, easier-to-manage and more affordable
virtual desktop model.http://www.accelacomm.com/jaw/sfnl/114/51426474/
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2011-09-08 7:00 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-07 14:41 [PATCH 0/3] nbd: add support for DISCARD Paolo Bonzini
2011-09-07 14:41 ` Paolo Bonzini
2011-09-07 14:41 ` [PATCH 1/3] nbd: remove unused flags fields Paolo Bonzini
2011-09-07 14:41 ` Paolo Bonzini
2011-09-07 14:41 ` [PATCH 2/3] nbd: add support for feature negotiation Paolo Bonzini
2011-09-07 14:41 ` Paolo Bonzini
2011-09-07 16:30 ` Paul Clements
2011-09-07 16:30 ` Paul Clements
2011-09-07 17:52 ` Paolo Bonzini
2011-09-07 17:52 ` Paolo Bonzini
2011-09-07 21:54 ` Paul Clements
2011-09-07 21:54 ` Paul Clements
2011-09-08 7:00 ` Paolo Bonzini
2011-09-08 7:00 ` Paolo Bonzini
2011-09-07 14:41 ` [PATCH 3/3] nbd: map DISCARD requests to a new nbd request type Paolo Bonzini
2011-09-08 1:13 ` Paul Clements
2011-09-08 1:13 ` Paul Clements
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.