Re: fs: Add FITRIM ioctl

[Lukas Czerner <lczerner@redhat.com>]

Hi Christoph,

On Thu, 28 Oct 2010, Christoph Hellwig wrote:

> On Thu, Oct 28, 2010 at 12:30:45PM +0200, Boaz Harrosh wrote:
> > I've just noticed this patch. Was it posted to linux-fsdevel? Sorry
> > for missing it.
> 
> It has been, but only very recently.
> 
> 
> I can't say I overly like it.  There's no real point in dispatching this
> out to a separate vector instead of just through ->ioctl.  It's got
> rather useless special case for ommiting the argument, and the whole
> thing just doesn't seem generic enough to do it in the VFS.  

Well, learning from examples, that's what I am doing and ioctl_fiemap()
is doing the same thing so I was using that as an example. And, of
course, I am returning the amount of really discarded Bytes to the user
and I just can not simply use user-space pointer, or maybe I do not
understand you correctly.

About the special case I am not entirely sure that it is useless, since
there is no point in allocating the structure and setting range when you
just want to discard the whole thing (in user-space of course). But the
reason why to do it in ioctl_fstrim() is that otherwise some filesystems
might decide to return EINVAL when no argument has been passed and some
may handle it differently, hence it would not be consistent.

> 
> Why did this common code go in through the ext4 tree as a start, and
> without hitting linux-next before the merge window?

I have posted it on both linux-fsdevel and linux-ext4 and we needed it
for batched discard implementation in ext4 so that is probably why it
went through ext4 tree. Unfortunately, I can not answer the second
question...

> 
> > 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.
> 
> Indeed.  Looks like copied from freeze/thaw which used to be block
> device based, but don't really need this anymore either.
> 

You're right.

Thanks!

-Lukas