[PATCH 0/3] nbd: add support for DISCARD

[PATCH 0/3] nbd: add support for DISCARD

[Paolo Bonzini]

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

[PATCH 1/3] nbd: remove unused flags fields

[Paolo Bonzini]

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

[PATCH 2/3] nbd: add support for feature negotiation

[Paolo Bonzini]

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

Re: [PATCH 2/3] nbd: add support for feature negotiation

[Paul Clements]

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
>
>
>

Re: [PATCH 2/3] nbd: add support for feature negotiation

[Paolo Bonzini]

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

Re: [PATCH 2/3] nbd: add support for feature negotiation

[Paul Clements]

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

Re: [PATCH 2/3] nbd: add support for feature negotiation

[Paolo Bonzini]

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

[PATCH 3/3] nbd: map DISCARD requests to a new nbd request type

[Paolo Bonzini]

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

Re: [PATCH 3/3] nbd: map DISCARD requests to a new nbd request type

[Paul Clements]

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
>
>