[PATCH] mkfs: add discard support

[PATCH] mkfs: add discard support

[Christoph Hellwig]

Call the BLKDISCARD ioctl to mark the whole disk as unused before creating
a new filesystem.  This will allow SSDs, Arrays with thin provisioning support
and virtual machines to make smarter allocation decisions.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfsprogs-dev/mkfs/xfs_mkfs.c
===================================================================
--- xfsprogs-dev.orig/mkfs/xfs_mkfs.c	2009-10-06 18:02:34.000000000 +0000
+++ xfsprogs-dev/mkfs/xfs_mkfs.c	2009-10-06 18:03:55.000000000 +0000
@@ -605,6 +605,29 @@ done:
 	free(buf);
 }
 
+#ifdef __linux__
+
+#ifndef BLKDISCARD
+#define BLKDISCARD	_IO(0x12,119)
+#endif
+
+static void
+discard_blocks(dev_t dev, __uint64_t nsectors)
+{
+	int fd = libxfs_device_to_fd(dev);
+	__uint64_t range[2] = { 0, nsectors << 9 };
+
+	/*
+	 * We intentionally ignore errors from the discard ioctl.  It is
+	 * not nessecary for the mkfs functionality but just an optimization.
+	 */
+	if (fd > 0)
+		ioctl(fd, BLKDISCARD, &range);
+}
+#else
+#define discard_blocks(dev, nsectors)
+#endif
+
 int
 main(
 	int			argc,
@@ -1645,6 +1668,12 @@ main(
 		}
 	}
 
+	discard_blocks(xi.ddev, xi.dsize);
+	if (xi.rtdev)
+		discard_blocks(xi.rtdev, xi.rtsize);
+	if (xi.logdev && xi.logdev != xi.ddev)
+		discard_blocks(xi.logdev, xi.logBBsize);
+
 	if (!liflag && !ldflag)
 		loginternal = xi.logdev == 0;
 	if (xi.logname)

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] mkfs: add discard support

[Dave Chinner]

On Tue, Oct 06, 2009 at 02:47:58PM -0400, Christoph Hellwig wrote:
> Call the BLKDISCARD ioctl to mark the whole disk as unused before creating
> a new filesystem.  This will allow SSDs, Arrays with thin provisioning support
> and virtual machines to make smarter allocation decisions.

Good idea, but perhaps the discard should be optional rather than
unconditional.  My immediate thought was the SOP for setting up
encrypted devices - fill the empty disk with random data before
setting up the encrypted device. If you then send it a discard....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] mkfs: add discard support

[Michael Monnerie]

On Mittwoch 07 Oktober 2009 Dave Chinner wrote:
> Good idea, but perhaps the discard should be optional rather than
> unconditional.  My immediate thought was the SOP for setting up
> encrypted devices - fill the empty disk with random data before
> setting up the encrypted device. If you then send it a discard....

You could change the order to
mkfs ; mount
dd if=/dev/urandom of=/encrytped/fill.dd
(one day the disk will be full)
sync ; rm /encrytped/fill.dd

That should fill the disk with random garbage too. Should be nearly the 
same result, right?

mfg zmi
-- 
// Michael Monnerie, Ing.BSc    -----      http://it-management.at
// Tel: 0660 / 415 65 31                      .network.your.ideas.
// PGP Key:         "curl -s http://zmi.at/zmi.asc | gpg --import"
// Fingerprint: AC19 F9D5 36ED CD8A EF38  500E CE14 91F7 1C12 09B4
// Keyserver: wwwkeys.eu.pgp.net                  Key-ID: 1C1209B4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] mkfs: add discard support

[Christoph Hellwig]

On Wed, Oct 07, 2009 at 03:42:16PM +1100, Dave Chinner wrote:
> On Tue, Oct 06, 2009 at 02:47:58PM -0400, Christoph Hellwig wrote:
> > Call the BLKDISCARD ioctl to mark the whole disk as unused before creating
> > a new filesystem.  This will allow SSDs, Arrays with thin provisioning support
> > and virtual machines to make smarter allocation decisions.
> 
> Good idea, but perhaps the discard should be optional rather than
> unconditional.  My immediate thought was the SOP for setting up
> encrypted devices - fill the empty disk with random data before
> setting up the encrypted device. If you then send it a discard....

Yeah.  I'm fine with making it the default but adding a option to
disable it.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] mkfs: add discard support

[Andi Kleen]

Dave Chinner <david@fromorbit.com> writes:

> On Tue, Oct 06, 2009 at 02:47:58PM -0400, Christoph Hellwig wrote:
>> Call the BLKDISCARD ioctl to mark the whole disk as unused before creating
>> a new filesystem.  This will allow SSDs, Arrays with thin provisioning support
>> and virtual machines to make smarter allocation decisions.
>
> Good idea, but perhaps the discard should be optional rather than
> unconditional.  My immediate thought was the SOP for setting up
> encrypted devices - fill the empty disk with random data before
> setting up the encrypted device. If you then send it a discard....

This actually doesn't really work for SSDs, because SSDs typically
have more internal capacity than they advertise and when you fill
it up then it will just allocate new blocks and leave some of the
blocks with the existing data around. 

AFAIK there's no way to really reliably delete something physically
on a SSD short of applying a hammer.

For thin provisioning arrays you have similar problems.

So I think Christoph's case of making it default is fine.

-Andi

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] mkfs: add discard support

[Dave Chinner]

On Wed, Oct 07, 2009 at 10:24:07PM +0200, Andi Kleen wrote:
> Dave Chinner <david@fromorbit.com> writes:
> > On Tue, Oct 06, 2009 at 02:47:58PM -0400, Christoph Hellwig wrote:
> >> Call the BLKDISCARD ioctl to mark the whole disk as unused before creating
> >> a new filesystem.  This will allow SSDs, Arrays with thin provisioning support
> >> and virtual machines to make smarter allocation decisions.
> >
> > Good idea, but perhaps the discard should be optional rather than
> > unconditional.  My immediate thought was the SOP for setting up
> > encrypted devices - fill the empty disk with random data before
> > setting up the encrypted device. If you then send it a discard....
>
> This actually doesn't really work for SSDs, because SSDs typically
> have more internal capacity than they advertise and when you fill
> it up then it will just allocate new blocks and leave some of the
> blocks with the existing data around.

Agreed, but initialisation with random data before encryption is not
to delete existing information on the drive - it is to prevent
simple side-channel attacks that can significantly reduce the
strength of the encryption (e.g. an observer can tell the difference
between written, encrypted regions and unused space).  Using
discards during mkfs and during filesystem operation opens up this
avenue of attack, hence my reasoning for making discards
optional....

> So I think Christoph's case of making it default is fine.

So do I, but for different reasons ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] mkfs: add discard support

[Andi Kleen]

On Fri, Oct 09, 2009 at 01:30:22PM +1100, Dave Chinner wrote:
> On Wed, Oct 07, 2009 at 10:24:07PM +0200, Andi Kleen wrote:
> > Dave Chinner <david@fromorbit.com> writes:
> > > On Tue, Oct 06, 2009 at 02:47:58PM -0400, Christoph Hellwig wrote:
> > >> Call the BLKDISCARD ioctl to mark the whole disk as unused before creating
> > >> a new filesystem.  This will allow SSDs, Arrays with thin provisioning support
> > >> and virtual machines to make smarter allocation decisions.
> > >
> > > Good idea, but perhaps the discard should be optional rather than
> > > unconditional.  My immediate thought was the SOP for setting up
> > > encrypted devices - fill the empty disk with random data before
> > > setting up the encrypted device. If you then send it a discard....
> >
> > This actually doesn't really work for SSDs, because SSDs typically
> > have more internal capacity than they advertise and when you fill
> > it up then it will just allocate new blocks and leave some of the
> > blocks with the existing data around.
> 
> Agreed, but initialisation with random data before encryption is not
> to delete existing information on the drive - it is to prevent
> simple side-channel attacks that can significantly reduce the
> strength of the encryption (e.g. an observer can tell the difference

I see. That makes sense.

Although to be pedantic your description above is slightly
wrong then -- you need to fill it up after setting up the encryption,
not before. In this case it might be actually more reasonable
to simply fill the file system with a random file (although on XFS
might need to reset inode limits first to catch the metadata
reservations)

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] mkfs: add discard support

[Christoph Hellwig]

So does any of these means a positive review for the full patch, or just
philosophical discussions?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] mkfs: add discard support

[Andi Kleen]

On Sat, Oct 10, 2009 at 12:24:28PM -0400, Christoph Hellwig wrote:
> So does any of these means a positive review for the full patch, or just
> philosophical discussions?

It was in principle positive, except for the request to add a option
to turn it off to handle Dave's prefill use case.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v2] mkfs: add discard support

[Christoph Hellwig]

Call the BLKDISCARD ioctl to mark the whole disk as unused before creating
a new filesystem.  This will allow SSDs, Arrays with thin provisioning support
and virtual machines to make smarter allocation decisions.

Add a new -K option to prevent mkfs from discarding blocks to aid
trouble-shooting or specialized requirements.

Signed-off-by: Christoph Hellwig <hch@lst.de>


Index: xfsprogs-dev/mkfs/xfs_mkfs.c
===================================================================
--- xfsprogs-dev.orig/mkfs/xfs_mkfs.c	2009-10-07 21:47:31.000000000 +0000
+++ xfsprogs-dev/mkfs/xfs_mkfs.c	2009-10-07 22:25:29.000000000 +0000
@@ -605,6 +605,20 @@ done:
 	free(buf);
 }
 
+static void
+discard_blocks(dev_t dev, __uint64_t nsectors)
+{
+	int fd;
+
+	/*
+	 * We intentionally ignore errors from the discard ioctl.  It is
+	 * not necessary for the mkfs functionality but just an optimization.
+	 */
+	fd = libxfs_device_to_fd(dev);
+	if (fd > 0)
+		platform_discard_blocks(fd, 0, nsectors << 9);
+}
+
 int
 main(
 	int			argc,
@@ -681,6 +695,7 @@ main(
 	int			nvflag;
 	int			nci;
 	int			Nflag;
+	int			discard;
 	char			*p;
 	char			*protofile;
 	char			*protostring;
@@ -741,7 +756,7 @@ main(
 	xi.isdirect = LIBXFS_DIRECT;
 	xi.isreadonly = LIBXFS_EXCLUSIVELY;
 
-	while ((c = getopt(argc, argv, "b:d:i:l:L:n:Np:qr:s:CfV")) != EOF) {
+	while ((c = getopt(argc, argv, "b:d:i:l:L:n:KNp:qr:s:CfV")) != EOF) {
 		switch (c) {
 		case 'C':
 		case 'f':
@@ -1257,6 +1272,9 @@ main(
 		case 'N':
 			Nflag = 1;
 			break;
+		case 'K':
+			discard = 0;
+			break;
 		case 'p':
 			if (protofile)
 				respec('p', NULL, 0);
@@ -1645,6 +1663,14 @@ main(
 		}
 	}
 
+	if (discard) {
+		discard_blocks(xi.ddev, xi.dsize);
+		if (xi.rtdev)
+			discard_blocks(xi.rtdev, xi.rtsize);
+		if (xi.logdev && xi.logdev != xi.ddev)
+			discard_blocks(xi.logdev, xi.logBBsize);
+	}
+
 	if (!liflag && !ldflag)
 		loginternal = xi.logdev == 0;
 	if (xi.logname)
Index: xfsprogs-dev/include/linux.h
===================================================================
--- xfsprogs-dev.orig/include/linux.h	2009-10-07 21:47:31.000000000 +0000
+++ xfsprogs-dev/include/linux.h	2009-10-07 22:15:32.000000000 +0000
@@ -93,6 +93,20 @@ static __inline__ void platform_uuid_cop
 	uuid_copy(*dst, *src);
 }
 
+#ifndef BLKDISCARD
+#define BLKDISCARD	_IO(0x12,119)
+#endif
+
+static __inline__ int
+platform_discard_blocks(int fd, off64_t start, off64_t end)
+{
+	__uint64_t range[2] = { start, end };
+
+	if (ioctl(fd, BLKDISCARD, &range) < 0)
+		return errno;
+	return 0;
+}
+
 #if (__GLIBC__ < 2) || ((__GLIBC__ == 2) && (__GLIBC_MINOR__ <= 1))
 # define constpp	const char * const *
 #else
Index: xfsprogs-dev/include/darwin.h
===================================================================
--- xfsprogs-dev.orig/include/darwin.h	2009-10-07 21:47:31.000000000 +0000
+++ xfsprogs-dev/include/darwin.h	2009-10-07 22:15:32.000000000 +0000
@@ -154,4 +154,10 @@ typedef unsigned char	uchar_t;
 
 #define HAVE_FID	1
 
+static __inline__ int
+platform_discard_blocks(int fd, off64_t start, off64_t end)
+{
+	return 0;
+}
+
 #endif	/* __XFS_DARWIN_H__ */
Index: xfsprogs-dev/include/freebsd.h
===================================================================
--- xfsprogs-dev.orig/include/freebsd.h	2009-10-07 21:47:31.000000000 +0000
+++ xfsprogs-dev/include/freebsd.h	2009-10-07 22:15:32.000000000 +0000
@@ -139,4 +139,10 @@ static __inline__ void platform_uuid_cop
 	memcpy(dst, src, sizeof(uuid_t));
 }
 
+static __inline__ int
+platform_discard_blocks(int fd, off64_t start, off64_t end)
+{
+	return 0;
+}
+
 #endif	/* __XFS_FREEBSD_H__ */
Index: xfsprogs-dev/include/irix.h
===================================================================
--- xfsprogs-dev.orig/include/irix.h	2009-10-07 21:47:31.000000000 +0000
+++ xfsprogs-dev/include/irix.h	2009-10-07 22:15:32.000000000 +0000
@@ -337,6 +337,12 @@ static __inline__ void platform_uuid_cop
 	memcpy(dst, src, sizeof(uuid_t));
 }
 
+static __inline__ int
+platform_discard_blocks(int fd, off64_t start, off64_t end)
+{
+	return 0;
+}
+
 static __inline__ char * strsep(char **s, const char *ct)
 {
 	char *sbegin = *s, *end;
Index: xfsprogs-dev/man/man8/mkfs.xfs.8
===================================================================
--- xfsprogs-dev.orig/man/man8/mkfs.xfs.8	2009-10-07 22:19:23.000000000 +0000
+++ xfsprogs-dev/man/man8/mkfs.xfs.8	2009-10-07 22:22:26.000000000 +0000
@@ -36,6 +36,8 @@ mkfs.xfs \- construct an XFS filesystem
 .I label
 ] [
 .B \-N
+] [
+.B \-K
 ]
 .I device
 .SH DESCRIPTION
@@ -714,6 +716,9 @@ manual entries for additional informatio
 .B \-N
 Causes the file system parameters to be printed out without really
 creating the file system.
+.TP
+.B \-K
+Do not attempt to discard blocks at mkfs time.
 .SH SEE ALSO
 .BR xfs (5),
 .BR mkfs (8),

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] mkfs: add discard support

[Eric Sandeen]

Christoph Hellwig wrote:
> Call the BLKDISCARD ioctl to mark the whole disk as unused before creating
> a new filesystem.  This will allow SSDs, Arrays with thin provisioning support
> and virtual machines to make smarter allocation decisions.
> 
> Add a new -K option to prevent mkfs from discarding blocks to aid
> trouble-shooting or specialized requirements.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 

I might have made the manpage help a bit more for people who don't know 
what discard means, but *shrug*

Reviewed-by: Eric Sandeen <sandeen@sandeen.net>

> Index: xfsprogs-dev/mkfs/xfs_mkfs.c
> ===================================================================
> --- xfsprogs-dev.orig/mkfs/xfs_mkfs.c	2009-10-07 21:47:31.000000000 +0000
> +++ xfsprogs-dev/mkfs/xfs_mkfs.c	2009-10-07 22:25:29.000000000 +0000
> @@ -605,6 +605,20 @@ done:
>  	free(buf);
>  }
>  
> +static void
> +discard_blocks(dev_t dev, __uint64_t nsectors)
> +{
> +	int fd;
> +
> +	/*
> +	 * We intentionally ignore errors from the discard ioctl.  It is
> +	 * not necessary for the mkfs functionality but just an optimization.
> +	 */
> +	fd = libxfs_device_to_fd(dev);
> +	if (fd > 0)
> +		platform_discard_blocks(fd, 0, nsectors << 9);
> +}
> +
>  int
>  main(
>  	int			argc,
> @@ -681,6 +695,7 @@ main(
>  	int			nvflag;
>  	int			nci;
>  	int			Nflag;
> +	int			discard;
>  	char			*p;
>  	char			*protofile;
>  	char			*protostring;
> @@ -741,7 +756,7 @@ main(
>  	xi.isdirect = LIBXFS_DIRECT;
>  	xi.isreadonly = LIBXFS_EXCLUSIVELY;
>  
> -	while ((c = getopt(argc, argv, "b:d:i:l:L:n:Np:qr:s:CfV")) != EOF) {
> +	while ((c = getopt(argc, argv, "b:d:i:l:L:n:KNp:qr:s:CfV")) != EOF) {
>  		switch (c) {
>  		case 'C':
>  		case 'f':
> @@ -1257,6 +1272,9 @@ main(
>  		case 'N':
>  			Nflag = 1;
>  			break;
> +		case 'K':
> +			discard = 0;
> +			break;
>  		case 'p':
>  			if (protofile)
>  				respec('p', NULL, 0);
> @@ -1645,6 +1663,14 @@ main(
>  		}
>  	}
>  
> +	if (discard) {
> +		discard_blocks(xi.ddev, xi.dsize);
> +		if (xi.rtdev)
> +			discard_blocks(xi.rtdev, xi.rtsize);
> +		if (xi.logdev && xi.logdev != xi.ddev)
> +			discard_blocks(xi.logdev, xi.logBBsize);
> +	}
> +
>  	if (!liflag && !ldflag)
>  		loginternal = xi.logdev == 0;
>  	if (xi.logname)
> Index: xfsprogs-dev/include/linux.h
> ===================================================================
> --- xfsprogs-dev.orig/include/linux.h	2009-10-07 21:47:31.000000000 +0000
> +++ xfsprogs-dev/include/linux.h	2009-10-07 22:15:32.000000000 +0000
> @@ -93,6 +93,20 @@ static __inline__ void platform_uuid_cop
>  	uuid_copy(*dst, *src);
>  }
>  
> +#ifndef BLKDISCARD
> +#define BLKDISCARD	_IO(0x12,119)
> +#endif
> +
> +static __inline__ int
> +platform_discard_blocks(int fd, off64_t start, off64_t end)
> +{
> +	__uint64_t range[2] = { start, end };
> +
> +	if (ioctl(fd, BLKDISCARD, &range) < 0)
> +		return errno;
> +	return 0;
> +}
> +
>  #if (__GLIBC__ < 2) || ((__GLIBC__ == 2) && (__GLIBC_MINOR__ <= 1))
>  # define constpp	const char * const *
>  #else
> Index: xfsprogs-dev/include/darwin.h
> ===================================================================
> --- xfsprogs-dev.orig/include/darwin.h	2009-10-07 21:47:31.000000000 +0000
> +++ xfsprogs-dev/include/darwin.h	2009-10-07 22:15:32.000000000 +0000
> @@ -154,4 +154,10 @@ typedef unsigned char	uchar_t;
>  
>  #define HAVE_FID	1
>  
> +static __inline__ int
> +platform_discard_blocks(int fd, off64_t start, off64_t end)
> +{
> +	return 0;
> +}
> +
>  #endif	/* __XFS_DARWIN_H__ */
> Index: xfsprogs-dev/include/freebsd.h
> ===================================================================
> --- xfsprogs-dev.orig/include/freebsd.h	2009-10-07 21:47:31.000000000 +0000
> +++ xfsprogs-dev/include/freebsd.h	2009-10-07 22:15:32.000000000 +0000
> @@ -139,4 +139,10 @@ static __inline__ void platform_uuid_cop
>  	memcpy(dst, src, sizeof(uuid_t));
>  }
>  
> +static __inline__ int
> +platform_discard_blocks(int fd, off64_t start, off64_t end)
> +{
> +	return 0;
> +}
> +
>  #endif	/* __XFS_FREEBSD_H__ */
> Index: xfsprogs-dev/include/irix.h
> ===================================================================
> --- xfsprogs-dev.orig/include/irix.h	2009-10-07 21:47:31.000000000 +0000
> +++ xfsprogs-dev/include/irix.h	2009-10-07 22:15:32.000000000 +0000
> @@ -337,6 +337,12 @@ static __inline__ void platform_uuid_cop
>  	memcpy(dst, src, sizeof(uuid_t));
>  }
>  
> +static __inline__ int
> +platform_discard_blocks(int fd, off64_t start, off64_t end)
> +{
> +	return 0;
> +}
> +
>  static __inline__ char * strsep(char **s, const char *ct)
>  {
>  	char *sbegin = *s, *end;
> Index: xfsprogs-dev/man/man8/mkfs.xfs.8
> ===================================================================
> --- xfsprogs-dev.orig/man/man8/mkfs.xfs.8	2009-10-07 22:19:23.000000000 +0000
> +++ xfsprogs-dev/man/man8/mkfs.xfs.8	2009-10-07 22:22:26.000000000 +0000
> @@ -36,6 +36,8 @@ mkfs.xfs \- construct an XFS filesystem
>  .I label
>  ] [
>  .B \-N
> +] [
> +.B \-K
>  ]
>  .I device
>  .SH DESCRIPTION
> @@ -714,6 +716,9 @@ manual entries for additional informatio
>  .B \-N
>  Causes the file system parameters to be printed out without really
>  creating the file system.
> +.TP
> +.B \-K
> +Do not attempt to discard blocks at mkfs time.
>  .SH SEE ALSO
>  .BR xfs (5),
>  .BR mkfs (8),
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs