Date: Wed, 08 Dec 2010 15:56:59 -0800 From: Kirk McKusick <mckusick@mckusick.com> To: Kostik Belousov <kostikbel@gmail.com> Cc: freebsd-fs@freebsd.org, pjd@freebsd.org, Oliver Fromme <olli@lurza.secnetix.de> Subject: Re: TRIM support for UFS? Message-ID: <201012082356.oB8NuxwX040914@chez.mckusick.com> In-Reply-To: <20101208225352.GK33073@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
> Date: Thu, 9 Dec 2010 00:53:52 +0200 > From: Kostik Belousov <kostikbel@gmail.com> > To: Kirk McKusick <mckusick@mckusick.com> > Cc: Julian Elischer <julian@freebsd.org>, freebsd-fs@freebsd.org, > pjd@freebsd.org, Oliver Fromme <olli@lurza.secnetix.de> > Subject: Re: TRIM support for UFS? > > On Wed, Dec 08, 2010 at 02:23:52PM -0800, Kirk McKusick wrote: > > > > It is safe to do the BIO_DELETE here because at this point we have > > progressed through the file delete to the point where the inode that > > claimed this block has been updated on the disk with a NULL pointer. > > Thanks for the explanation. On the other hand, can my scenario be real > for async mounts ? Async mounts are not compatible with SU or SUJ. You are running the filesystem in a mode where there is no guarantee of recovery after a crash. So yes, you may TRIM blocks that are still claimed by on-disk inodes. But that is likely the least of your worries. Since async makes no claims about filesystem consistency after a crash, adding one more way for it to break does not seem like a big deal :-) > > The above patch looks good though I would do it unconditionally > > (e.g., also for snapshots). It seems sensible to make it conditional > > on a sysctl variable so that folks could experiment with it more > > easily. And I would leave it off by default as non-SSD disks are > > unlikely to benefit from it. If it does prove useful for SSD disks > > then I would make it conditional on a filesystem flag so that it > > is possible to enable it on a filesystem-by-filesystem basis (e.g., > > on for filesystems on the SSD and off for the rest). > > I think it is better to have a flag in superblock instead of the mount > option. > > Most of diff deleted... > > diff --git a/sys/ufs/ffs/fs.h b/sys/ufs/ffs/fs.h > index ba98ed3..13d9ede 100644 > --- a/sys/ufs/ffs/fs.h > +++ b/sys/ufs/ffs/fs.h > @@ -417,6 +417,7 @@ CTASSERT(sizeof(struct fs) == 1376); > #define FS_FLAGS_UPDATED 0x0080 /* flags have been moved to new location */ > #define FS_NFS4ACLS 0x0100 /* file system has NFSv4 ACLs enabled */ > #define FS_INDEXDIRS 0x0200 /* kernel supports indexed directories */ > +#define FS_TRIM 0x0400 /* issue BIO_DELETE for deleted blocks */ > > /* > * Macros to access bits in the fs_active array. I approve of this change and am happy to see it (finally) get added. > On Wed, 8 Dec 2010 at 23:33:27 +0100, Pawel Jakub Dawidek wrote: >> On Wed, Dec 08, 2010 at 02:23:52PM -0800, Kirk McKusick wrote: >> The above patch looks good though I would do it unconditionally >> (e.g., also for snapshots). It seems sensible to make it conditional >> on a sysctl variable so that folks could experiment with it more >> easily. And I would leave it off by default as non-SSD disks are >> unlikely to benefit from it. If it does prove useful for SSD disks >> then I would make it conditional on a filesystem flag so that it >> is possible to enable it on a filesystem-by-filesystem basis (e.g., >> on for filesystems on the SSD and off for the rest). > > We already have a flag for disks: DISKFLAG_CANDELETE, which tells if the > disk support TRIM or not. Next we should add BIO_GETATTR attribute for > DISK class to return true if TRIM is supported. This way UFS can ask if > TRIM is supported on mount and don't bother sending BIO_DELETE if it is > not supported. Clearly asking the underlying device is the most automated way to decide whether to do TRIM. Once it is possible to ask the underlying disk if it supports TRIM, your change could be modified to do the querry to the disk and set the FS_TRIM flag accordingly each time that the filesystem is mounted. If it is easy to add a BIO_GETATTR attribute for the DISK class, then we can skip the intermediate stage of using tunefs to enable/disable it. Kirk McKusick
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201012082356.oB8NuxwX040914>