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:

quoted

On Mon 22-11-10 12:29:18, Lukas Czerner wrote:

quoted

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 [off-list ref]
SUSE Labs, CR