Re: [PATCH] mkfs: add discard support

Re: [PATCH] mkfs: add discard support

[Nathan Scott]

----- "Christoph Hellwig" <hch@infradead.org> wrote:

> 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

It might be a bit cleaner to add this in with the existing platform-
specific code in libxfs/linux.c (or perhaps include/platform_defs.h)
with wrappers for the other platforms, rather than putting it directly
in mkfs like this?  repair may want to use this someday too, I guess.

cheers.

-- 
Nathan

_______________________________________________
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 06:24:33AM +1100, Nathan Scott wrote:
> It might be a bit cleaner to add this in with the existing platform-
> specific code in libxfs/linux.c (or perhaps include/platform_defs.h)
> with wrappers for the other platforms, rather than putting it directly
> in mkfs like this?  repair may want to use this someday too, I guess.

Yeah, that might be better. 

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

Re: [PATCH] mkfs: add discard support

[Christoph Hellwig]

> It might be a bit cleaner to add this in with the existing platform-
> specific code in libxfs/linux.c (or perhaps include/platform_defs.h)
> with wrappers for the other platforms, rather than putting it directly
> in mkfs like this?  repair may want to use this someday too, I guess.

How about this one?

-- 

Subject: [PATCH] mkfs: add discard support
From: Christoph Hellwig <hch@lst.de>

Cal 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:46:06.000000000 +0000
+++ xfsprogs-dev/mkfs/xfs_mkfs.c	2009-10-07 01:09:49.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 nessecary 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,
@@ -1645,6 +1659,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)
Index: xfsprogs-dev/include/linux.h
===================================================================
--- xfsprogs-dev.orig/include/linux.h	2009-10-07 01:06:00.000000000 +0000
+++ xfsprogs-dev/include/linux.h	2009-10-07 01:13:12.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 01:15:38.000000000 +0000
+++ xfsprogs-dev/include/darwin.h	2009-10-07 01:16:19.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 01:15:38.000000000 +0000
+++ xfsprogs-dev/include/freebsd.h	2009-10-07 01:16:06.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 01:15:38.000000000 +0000
+++ xfsprogs-dev/include/irix.h	2009-10-07 01:16:29.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;

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

Re: [PATCH] mkfs: add discard support

[Nathan Scott]

----- "Christoph Hellwig" <hch@infradead.org> wrote:

> > It might be a bit cleaner to add this in with the existing
> platform-
> > specific code in libxfs/linux.c (or perhaps
> include/platform_defs.h)
> > with wrappers for the other platforms, rather than putting it
> directly
> > in mkfs like this?  repair may want to use this someday too, I
> guess.
> 
> How about this one?
> 

Looks good to me.

cheers.

-- 
Nathan

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

Re: [PATCH] mkfs: add discard support

[Eric Sandeen]

Nathan Scott wrote:
> ----- "Christoph Hellwig" <hch@infradead.org> wrote:
> 
>>> It might be a bit cleaner to add this in with the existing
>> platform-
>>> specific code in libxfs/linux.c (or perhaps
>> include/platform_defs.h)
>>> with wrappers for the other platforms, rather than putting it
>> directly
>>> in mkfs like this?  repair may want to use this someday too, I
>> guess.
>>
>> How about this one?
>>
> 
> Looks good to me.
> 
> cheers.
> 

looks good to me too, thanks.

need to get that vertex flashed w/ trim support some day...

-eric

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

Re: [PATCH] mkfs: add discard support

[Christoph Hellwig]

Adding the discard helper to the platform headers now causes the build
to fail for some external users of the xfs headers because off64_t doesn't
exist without the large file configure magic. E.g. for xfstests:

checking xfs/xfs.h usability... no
checking xfs/xfs.h presence... yes
configure: WARNING: xfs/xfs.h: present but cannot be compiled
configure: WARNING: xfs/xfs.h:     check for missing prerequisite headers?
configure: WARNING: xfs/xfs.h: see the Autoconf documentation
configure: WARNING: xfs/xfs.h:     section "Present But Cannot Be Compiled"
configure: WARNING: xfs/xfs.h: proceeding with the compiler's result
checking for xfs/xfs.h... no

commenting out the discard helper fixes this.  We might be able to just
switch to a int64_t or similar instead, even if that's a bit ugly.

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

[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