From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Hellwig Subject: Re: fs: Add FITRIM ioctl Date: Thu, 28 Oct 2010 07:25:38 -0400 Message-ID: <20101028112538.GA16435@infradead.org> References: <4CC950D5.8000603@panasas.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Lukas Czerner , linux-fsdevel , Theodore Ts'o To: Boaz Harrosh Return-path: Received: from bombadil.infradead.org ([18.85.46.34]:54473 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753068Ab0J1LZl (ORCPT ); Thu, 28 Oct 2010 07:25:41 -0400 Content-Disposition: inline In-Reply-To: <4CC950D5.8000603@panasas.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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. Why did this common code go in through the ext4 tree as a start, and without hitting linux-next before the merge window? > 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.