On Tue, May 24, 2011 at 3:39 PM, OGAWA Hirofumi
[off-list ref] wrote:

Kyungmin Park [off-list ref] writes:

quoted

quoted

quoted

Do you still object to merge this feature for .40? As I said the
batched discard design is out-of-scope of this patch.
It's implemented at other batched discard, ext4, xfs and so on.

I hope fat is also support the batched discard.

Any opinions?

I'm also thinking implementing this is good though. Sorry, I'm not going
to apply this for now, and would like to wait to be used by real
userland (I hope guys notice the problem, or userland tackles it somehow
sadly).

I think, to expose the wrong behavior like this would be worse.

E.g. one of problems, userland might do like this (trim chunk from 0 to
number of block)

for chunk in number_of_blocks
       do_trim chunk
done

It's handled at trim implementation. It just trim the fat aware block.
Not trim the blocks which fat doesn't know.
As fat don't use the block 0, 1, it adjust the start block at kernel.

+       if (start < FAT_START_ENT)
+               start = FAT_START_ENT;

and don't exceed the max cluster size.

+       len = (len > sbi->max_cluster) ? sbi->max_cluster : len;

+       for (count = start; count <= len; count++) {

Yes. We _adjust_ from 0 to 2 here, so, the end of block also have to be
_adjusted_.

From other point of view, if userland specified 0 - max-length
(i.e. number of blocks), what happens? It would trim block of 2 -
(max-length - 2), right?

No, length is not changed. so max-length is used.

So, actually, userland specify 2 - (max-length + 2). And userland has to
know 2 is real start, not 0.

But how to know it? Yes, it's FS internal layout, AFAICS, other FSes also
expose internal like this.

user space doesn't know the internal fs layout, it should be handled
at each fs trim implementation.

quoted

quoted

But this is actually wrong, this interface doesn't map blocks to 0-max,
so userland have to know real maximum-block-number somehow for each FS
(and maybe have to know real minimum-block-number).

So, how to fix this? The solutions would be userland workaround, or fix
kernel. If it was userland, userland have to know FS internal sadly. If
it was kernel, we would have backward compatible nightmare, or ignore
compatible :(.

I think basic concept of batched discard is trim at once instead of
deleted entries at filesystem (original one).
So it can set the specific start address or any start (usually 0) with
maximum length.

Yes, I think so too (i.e. 0 - max length). But the implement is not
working like it. It exposes FS internal layout.

quoted

quoted

I really think we have to rethink this and have agreement about common
interface. Or there was real userland app, I think FAT can implement to
work that app.

IMO, we should use the same user space application. user program
doesn't know the which filesystem are underlying.

Indeed.

quoted

So sample user space program used at ext4, xfs should be used. and I
tested it.

I bet it doesn't trim whole FS (due to expose FS layout) actually even
if user asked like above example.

Right, it's fs dependent parts. Do you think it should touch whole FS
blocks when batched discard. I think original discard doesn't support
it also.

Batched discard support is just extension of original discard
implementation. It doesn't modify the original design.

Thank you,
Kyungmin Park

Thanks.
--
OGAWA Hirofumi [off-list ref]