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