Date:	Thu, 24 Feb 2011 21:02:47 +0900
From:	Kyungmin Park <kmpark@...radead.org>
To:	Lukas Czerner <lczerner@...hat.com>
Cc:	OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2] fat: Batched discard support for fat

On Thu, Feb 24, 2011 at 8:40 PM, Lukas Czerner <lczerner@...hat.com> wrote:
> On Thu, 24 Feb 2011, Kyungmin Park wrote:
>
> ...snip...
>> >> >> +     while (count < sbi->max_cluster) {
>> >> >> +             if (fatent.entry >= sbi->max_cluster)
>> >> >> +                     fatent.entry = FAT_START_ENT;
>> >> >> +             /* readahead of fat blocks */
>> >> >> +             if ((cur_block & reada_mask) == 0) {
>> >> >> +                     unsigned long rest = sbi->fat_length - cur_block;
>> >> >> +                     fat_ent_reada(sb, &fatent, min(reada_blocks, rest));
>> >> >> +             }
>> >> >> +             cur_block++;
>> >> >> +
>> >> >> +             err = fat_ent_read_block(sb, &fatent);
>> >> >> +             if (err)
>> >> >> +                     goto out;
>> >> >> +
>> >> >> +             do {
>> >> >> +                     if (ops->ent_get(&fatent) == FAT_ENT_FREE) {
>> >> >> +                             free++;
>> >> >> +                             if (!entry)
>> >> >> +                                     entry = fatent.entry;
>> >> >> +                             if (free >= (len - trimmed) && free >= minlen) {
>> >> >
>> >> > It seems to me that you are using len as number of bytes to trim. This
>> >> > is not right and I am sorry for not explaining it correctly. "len" is
>> >> > supposed to be a number of bytes you want to "investigate" from the start.
>> >> > So it means that you will trim every single free extent bigger than minlen
>> >> > between "start" byte and "start + len" byte of the underlying device, or
>> >> > partition.
>> >> No. len is adjusted at fat cluster number. it's not used byte unit.
>> >> I think it's easy to compare with fat internal units.
>> >
>> > Does not matter what units are you using for len. I it just that you are
>> > checking for (free >= (len - trimmed)) which is wrong because len is not
>> > meant to be "overall length of trimmed data" but rather "overall length
>> > of data to walk through and check for free extents" see ext4
>> > implementation for reference.
>> I think I used it as you said. e.g., I want to trim 256 (* minimum
>> discard granularity),
>> First time, I can find 10 entries. and trimmed has 10 and len has still 256.
>> next time, I found the 246 free entries then trim remaining 246 one.
>>
>> do you want to trim it more than given len?
>
> Let the "O" be free (bytes, blocks, whatever), and "=" be used.
> Now, we have a filesystem like this.
>
>   OOOO==O===OO===OOOOO==O===O===OOOOOOO===
>   ^                                      ^
>   0                                      40
>
> This is how it supposed to wotk if you have called FITIRM with parameters:
>
> start = 0
> minlen = 2
> len = 20
>
> So you will go through (blocks, bytes...) 0 -> 20
>
>   OOOO==O===OO===OOOOO==O===O===OOOOOOO===
>   ^                   ^
>   0                   20
>
> So, you will call discard on extents:
>
> 0-3
> You'll skip 6 because is smaller than minlen
> 10-11
> 15-19
>
>
> But in your implementation you will trim extents:
> 0-3
> 10-11
> 15-19
> 30-36
>
> Hope it is clear now.

Okay you mean it's end address instead of total amount of trim size.
It will be helpful if you add these diagram or comments on
linux/include/fs.h

Thank you,
Kyungmin Park
>
>>
>> Thank you,
>> Kyungmin Park
>> >
>> > Thanks!
>> > -Lukas
>> >
>> >>
>> >> I hope fat peoples comment this one.
>> >>
>> >> Thank you,
>> >> Kyungmin Park
>> >> >
>> >> >> +                                     fat_issue_discard(sb, entry, free);
>> >> >> +                                     trimmed += free;
>> >> >> +                             }
>> >> >> +                             if (trimmed >= len)
>> >> >> +                                     goto done;
>> >> >> +                     } else if (entry) {
>> >> >> +                             if (free >= minlen) {
>> >> >> +                                     fat_issue_discard(sb, entry, free);
>> >> >> +                                     trimmed += free;
>> >> >> +                             }
>> >> >> +                             if (trimmed >= len)
>> >> >> +                                     goto done;
>> >> >> +                             free = 0;
>> >> >> +                             entry = 0;
>> >> >> +                     }
>> >> >> +                     count++;
>> >> >> +             } while (fat_ent_next(sbi, &fatent));
>> >> >> +     }
>> >> >> +     if (free >= minlen) {
>> >> >> +             fat_issue_discard(sb, entry, free);
>> >> >> +             trimmed += free;
>> >> >> +     }
>> >> >> +done:
>> >> >> +     range->len = (trimmed * sbi->sec_per_clus) << sb->s_blocksize_bits;
>> >> >> +     fatent_brelse(&fatent);
>> >> >> +out:
>> >> >> +     unlock_fat(sbi);
>> >> >> +     return err;
>> >> >> +}
>
> ...snip...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists