From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lukas Czerner Subject: Re: fs: Add FITRIM ioctl Date: Fri, 29 Oct 2010 11:50:15 +0200 (CEST) Message-ID: References: <4CC950D5.8000603@panasas.com> <20101028112538.GA16435@infradead.org> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Cc: Boaz Harrosh , Lukas Czerner , linux-fsdevel , "Theodore Ts'o" To: Christoph Hellwig Return-path: Received: from mx1.redhat.com ([209.132.183.28]:51799 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932216Ab0J2Ju2 (ORCPT ); Fri, 29 Oct 2010 05:50:28 -0400 In-Reply-To: <20101028112538.GA16435@infradead.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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