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

[Kyungmin Park]

On Tue, May 24, 2011 at 3:39 PM, OGAWA Hirofumi
<hirofumi@xxxxxxxxxxxxxxxxxx> wrote:
> Kyungmin Park <kmpark@xxxxxxxxxxxxx> 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?

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.

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

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 <hirofumi@xxxxxxxxxxxxxxxxxx>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/