On Thu, 28 Oct 2010, Boaz Harrosh wrote: > Lukas Czerner <lczerner@xxxxxxxxxx> wrote: > > Gitweb: http://git.kernel.org/linus/367a51a339020ba4d9edb0ce0f21d65bd50b00c9 > > Commit: 367a51a339020ba4d9edb0ce0f21d65bd50b00c9 > > Parent: 77ca6cdf0ab8a42f481ec997911bc89e79138723 > > Author: Lukas Czerner <lczerner@xxxxxxxxxx> > > AuthorDate: Wed Oct 27 21:30:11 2010 -0400 > > Committer: Theodore Ts'o <tytso@xxxxxxx> > > CommitDate: Wed Oct 27 21:30:11 2010 -0400 > > > > I've just noticed this patch. Was it posted to linux-fsdevel? Sorry > for missing it. Yes it was, not long time ago. > > So I have a comment, and question. > --- > > fs: Add FITRIM ioctl > > > > Adds an filesystem independent ioctl to allow implementation of file > > system batched discard support. I takes fstrim_range structure as an > > argument. fstrim_range is definec in the include/fs.h and its > > definition is as follows. > > > > struct fstrim_range { > > start; > > len; > > minlen; > > } > > > > start - first Byte to trim > > len - number of Bytes to trim from start > > minlen - minimum extent length to trim, free extents shorter than this > > number of Bytes will be ignored. This will be rounded up to fs > > block size. > > > > Was the range done so it could be called in parts, and progress displayed? Yes, this is one of the reasons. Other is, that discard on some devices may take quite a while to complete and it is not very cheap operation either. So, instead of discarding the whole device, which may cause noticeable slowdown for other IO on that device for some time, we can do it for example by one allocation group at a time (or just generally per partes). > > > It is also possible to specify NULL as an argument. In this case the > > arguments will set itself as follows: > > > > start = 0; > > len = ULLONG_MAX; > > minlen = 0; > > > > So it will trim the whole file system at one run. > > > > After the FITRIM is done, the number of actually discarded Bytes is stored > > in fstrim_range.len to give the user better insight on how much storage > > space has been really released for wear-leveling. > > > > Signed-off-by: Lukas Czerner <lczerner@xxxxxxxxxx> > > Reviewed-by: Dmitry Monakhov <dmonakhov@xxxxxxxxxx> > > Signed-off-by: "Theodore Ts'o" <tytso@xxxxxxx> > > --- > > fs/ioctl.c | 39 +++++++++++++++++++++++++++++++++++++++ > > include/linux/fs.h | 8 ++++++++ > > 2 files changed, 47 insertions(+), 0 deletions(-) > > > > diff --git a/fs/ioctl.c b/fs/ioctl.c > > index f855ea4..e92fdbb 100644 > > --- a/fs/ioctl.c > > +++ b/fs/ioctl.c > > @@ -530,6 +530,41 @@ static int ioctl_fsthaw(struct file *filp) > > return thaw_super(sb); > > } > > > > +static int ioctl_fstrim(struct file *filp, void __user *argp) > > +{ > > + struct super_block *sb = filp->f_path.dentry->d_inode->i_sb; > > + struct fstrim_range range; > > + int ret = 0; > > + > > + if (!capable(CAP_SYS_ADMIN)) > > + return -EPERM; > > + > > + /* If filesystem doesn't support trim feature, return. */ > > + if (sb->s_op->trim_fs == NULL) > > + return -EOPNOTSUPP; > > + > > + /* If a blockdevice-backed filesystem isn't specified, return EINVAL. */ > > + if (sb->s_bdev == NULL) > > + return -EINVAL; > > + > > What is the point of that? sb->s_bdev is not used anywhere in this code. > If an FS published an sb->s_op->trim_fs, it should know what it wants > No? Note that the FS in question does not even need to check. It already > knows if it's block based or not. Right, this was copied from fsfreeze and you are right it is not needed so it can be removed. > > Thanks > Boaz Thanks! -Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
