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]

Message-ID: <20101124130147.GP6113@quack.suse.cz>
Date:	Wed, 24 Nov 2010 14:01:47 +0100
From:	Jan Kara <jack@...e.cz>
To:	Lukas Czerner <lczerner@...hat.com>
Cc:	Jan Kara <jack@...e.cz>, tytso@....edu,
	linux-fsdevel@...r.kernel.org, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 2/2] ext3: Add FITRIM handle for ext3

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
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
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR

View attachment "ext3-discard-update" of type "text/plain" (2565 bytes)