Re: [PATCH v6] fat: Batched discard support for fat

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Kyungmin Park <kmpark@infradead.org> writes:

>>> 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?

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.

>> 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.

>> 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.

> 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.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>