linux-ext4 - Re: [PATCH 2/2] ext3: Add FITRIM handle for ext3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 24 Nov 2010 15:32:59 +0100 (CET)
From:	Lukas Czerner <lczerner@...hat.com>
To:	Jan Kara <jack@...e.cz>
cc:	Lukas Czerner <lczerner@...hat.com>, tytso@....edu,
	linux-fsdevel@...r.kernel.org, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 2/2] ext3: Add FITRIM handle for ext3

On Wed, 24 Nov 2010, Jan Kara wrote:

> On Tue 23-11-10 11:32:46, Lukas Czerner wrote:
> > On Mon, 22 Nov 2010, Jan Kara wrote:
> > 
> > > On Mon 22-11-10 12:29:18, Lukas Czerner wrote:
> > > > It takes fstrim_range structure as an argument. fstrim_range is definec in
> > > > the include/linux/fs.h.
> > > > 
> > > > 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.
> > >   Umm, why do we have to do this when FITRIM is already handled in
> > > fs/ioctl.c? I'd expect us to just provide .trim_fs ioctl, no?
> > 
> > Hi,
> > 
> > see upstream commits:
> > 93bb41f4f8b89ac8b4d0a734bc59634cb0a29a89
> > e681c047e47c0abe67bf95857f23814372793cb0
> > 
> > unfortunately people was not happy with generic ioctl interface for that
> > purpose, there were concerned that it is no common enough to be included
> > in core vfs and there is no need for new super operation since each
> > filesystem can easily setup its own ioctl handling.
> > 
> > When I say people I need to clarify that it was mainly Christoph Hellwig
> > who had objections against the former implementation and I must say that
> > he had a point.
> OK, I see. I had a fresh look at the patches and I've found a few
> suboptimal things which I've fixed. Most notably I don't think we have to
> issue a warning when underlying device does not support FITRIM. Returning
> EOPNOTSUPP should be enough. And I also think that remounting the
> filesystem read-only or even panicking (the result of calling
> ext3_std_error()) is necessary when sb_issue_discard() fails for whatever

Did you meant "is NOT necessary" ? Just for clarification.

> reason. Sure it is suspicious but we can cope just fine with that. BTW, I
> think ext4 might want this as well. Finally, we were missing to set the
> error in a few cases (e.g. when bitmap could not be loaded).
> 
> Attached is a diff with what I did and I'll repost what I currently have in
> my tree.
> 
> 									Honza
> 

Hi,

thanks a lot for your review and fixes. I already posted the patch for
ext4 (remove warning etc.) yesterday. So maybe we should get rid of the
warning message completely (ext3_warning) and just let userspace handle
the error and print out warning ? Otherwise diff looks good.

Thanks!

-Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html